Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add A Law For Eq Which Ensures Consistency With Universal Equality #4118

Closed

Conversation

isomarcte
Copy link
Member

This commit adds a law sameAsUniversalEquals which ensures that the result of Eq.eqv is the same for == for any type which implements Eq. This similar to the Hash law sameAsUniversalHash.

I can't think of any instance where we wouldn't want to ensure this behavior for a lawful Eq instance. A good sign is that all the cats tests pass without any changes here.

My motivation for this change is I ran into a lawful Eq instance for a type in the wild recently, which did not agree with ==. This was a bug, but wasn't caught by the law tests for the type.

This commit adds a law `sameAsUniversalEquals` which ensures that the result of `Eq.eqv` is the same for `==` for any type which implements `Eq`. This similar to the `Hash` law `sameAsUniversalHash`.

I can't think of any instance where we wouldn't want to ensure this behavior for a lawful `Eq` instance. A good sign is that all the cats tests pass without any changes here.

My motivation for this change is I ran into a lawful `Eq` instance for a type in the wild recently, which did not agree with `==`. This was a bug, but wasn't caught by the law tests for the type.
@armanbilge
Copy link
Member

@johnynek
Copy link
Contributor

I don't think we should have this law.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about java.net.URL, whose universal equality infamously does a network call?

What if we wanted to implement the URI comparison ladder? Non-canonical instances would be out, so this would now require newtypes.

@isomarcte
Copy link
Member Author

Hmm...so I think I follow @djspiewak's point, but I worry about the legacy importance of .equals and .hashCode on the JVM and in the associated std lib collections in Scala/Java.

I'm borrowing this example from @zarthross, but consider some opaque type for a case-insensitive string based on String. In that case we might want to have an Eq instance which deviates from ==, a point against having this law. However I'd be wary such a type is ill-posed on the JVM, since if it ever entered a std lib collection it's behavior would be...undesirable.

@rossabaker I'd be interested in you expanding on that a bit. Are you suggesting that we might have two uris, e.g. "http://localhost:80" and "http://localhost" which under the URI comparison ladder should be equivalent even though their raw representation is different causing a useful divergence between == and Eq.eqv?

@johnynek
Copy link
Contributor

I would argue that it is the collections that are wrong.

:)

i.e. we should work towards having a typeclass based implementation of HashSet, SortedSet, HashMap, SortedMap that use only typeclasses, not the instance methods.

@rossabaker
Copy link
Member

Are you suggesting that we might have two uris, e.g. "http://localhost:80" and "http://localhost" which under the URI comparison ladder should be equivalent even though their raw representation is different causing a useful divergence between == and Eq.eqv?

Those are equivalent under 6.2.3 of the RFC, but not under the five stricter definitions before it. Universal equality needs to pick a winner, but we could provide six lawful instances as laid out in the RFC. Non-canonical instances are controversial, but the alternative is a newtype. Depending on the encoding, this may or may not be consistent with universal equality.

This is not terribly different your the String example. CIString exists as a newtype to provide a canonical Eq. We also overrode equals to participate in scala.collection, and stay consistent with your proposed law. Without your law, but for use in @johnynek's collections, we could use an opaque type in Scala 3.

@isomarcte
Copy link
Member Author

@johnynek I completely agree with you about the collections being wrong. Despite that, I still worry about how practical it would be to have types such as that on the JVM, however if one controlled the usage domain carefully then I suppose it wouldn't be impossible.

I'm going to close this PR. You've convinced me this shouldn't be a law, even if most types want this behavior in general.

Thanks @rossabaker @johnynek and @armanbilge for the discussion and review!

@isomarcte isomarcte closed this Jan 27, 2022
@johnynek
Copy link
Contributor

I agree it is an opportunity for bugs when we conflate Eq with ==.

But if we had the law, why should we have the typeclass? Just use equals right?

@armanbilge
Copy link
Member

On this note, should we deprecate the sameAsUniversalHash law and remove the test? Or is that different?

@rossabaker
Copy link
Member

I think that's an undesirable law for the same reasons, but any laws we repeal could violate assumptions made in code while the law was in effect. It would in some sense be a breaking change.

@armanbilge
Copy link
Member

Ah, that's right. So, we can't remove the test backwards-compatibly, but we can deprecate the law so that Hash consumers know not to rely on it in the future.

@rossabaker
Copy link
Member

I'm not sure what "deprecating a law" means. Is that a documentation thing?

@armanbilge
Copy link
Member

A law is implemented as a method, so we could just slap a @deprecated on it, no?

@johnynek
Copy link
Contributor

can we find anyone using Hash? If they really want that law, why aren't they using .hashCode?

Hash is pretty hard to test: you want something that is hard to test for, namely collisions should be rare. But collisions have to happen for anything with cardinality > 2^32 and usually much before that. So, when do you have too many collisions?

Ideally, you want the law to prevent hashing everything to 0, because that will make the assumptions of hashed datastructures wrong, but how do you distinguish that from just getting really unlucky?

I guess we could have a law like: forAll { (listOf100: List[A]) => listOf100.map(Hash[A].hash(_)).distinct.length > 1 } or something. So here you could randomly fail if you happen to generate 100 collisions, but if collisions have probability at most 1/2, that is 2^(-100) which is a tiny number.

That said, that law would forbid hashing Unit. So, we probably need something more careful.

I would say we should stop testing the law because it makes the typeclass almost useless: if you want the hash just call .hashCode.

BTW: I think it is a reasonably law to put on some things: Strings, primitives, tuples and case classes of these recursively. Those hashes are fine. But putting it on all things isn't great.

@rossabaker
Copy link
Member

Yeah, I don't know how to make a "hash doesn't suck" law without knowing the cardinality of the type. It could be an interesting property outside the typeclass.

The practical matter of ruling out AnyVal and Scala 3 opaque types is more important than the theoretical loss of equational reasoning about an obscure and dubious law. I am not comfortable repealing laws in general, but I'd be okay if this one happened in a minor bump.

@satorg
Copy link
Contributor

satorg commented Jan 27, 2022

Yeah, I don't know how to make a "hash doesn't suck" law without knowing the cardinality of the type.

Maybe a silly idea, but what if we introduce a HashCardinality typeclass in the Laws project?

@joroKr21
Copy link
Member

joroKr21 commented Jan 28, 2022

Since you mentioned Hash laws - note that they are already broken in some cases: #2878

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants