search

typelevel / discipline

Flexible law checking for Scala
http://typelevel.org
MIT License
328 stars 56 forks source link

Rethinking property deduplication #169

Open rossabaker opened 4 years ago

rossabaker commented 4 years ago

When a rule set duplicates ids from a parent ruleset, the rule set's properties are silently dropped in favor of the parent's. This happened twice in Cats (https://github.com/typelevel/cats/pull/3493), both times unwittingly and unintentionally.

The scaladoc requires:

The only requirement here is that ''inside one kind'', the identifier of a property is unique, since duplicates are eliminated.

We could offer an idCollisions: Set[String] on the RuleSet, and leave it up to the test integrations to fail when non-empty, but this would be a breaking change. Alternatively, we could deduplicate with an obnoxious suffix instead of silently dropping duplicate properties, which should be safe unless people are relying on this deduplication. I don't think they should be for anything but optimization, and Discipline's most prominent client, Cats, was bitten by the current behavior.

larsrh commented 4 years ago

Could discipline throw an error instead?

rossabaker commented 4 years ago

I thought about deprecating all and creating an allProps that throws, or perhaps returns Either. It would avoid any semantic change to discipline-core, but all the test integrations would need to adopt it.

If we just make all throw, scala-steward would do most of the rest of the work downstream.