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

Compare productPrefix in synthetic case class canEqual #21606

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Sep 18, 2024

Since 2.13, case class hashCode mixes in the hash code of the productPrefix string. The synthetic equals method has fallen out of sync in that regard, so two instances can be equal but have different hash codes.

This commit changes the synthetic canEqual method to also compare the productPrefix of the two instances.

Scala 3 forward port of scala/scala#10859, for scala/bug#13033.


object Test extends App {
val c = C(1)
assert(c != new D)
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a semantic change, in 3.5.0 and 2.13.15, c == new D is true.

Since 2.13, case class `hashCode` mixes in the hash code of the
`productPrefix` string. The synthetic `equals` method has fallen
out of sync in that regard, so two instances can be equal but have
different hash codes.

This commit changes the synthetic `canEqual` method to also compare
the `productPrefix` of the two instances.
@lrytz
Copy link
Member Author

lrytz commented Sep 19, 2024

@sjrd pointed out that the change introduces this discrepancy:

scala> case class C(x: Int)
scala> class D extends C(1) { override def productPrefix = "D" }

scala> new D match { case C(1) => true }
val res0: Boolean = true

scala> new D == C(1)
val res1: Boolean = false

@lrytz
Copy link
Member Author

lrytz commented Sep 19, 2024

What it fixes is this:

scala> case class C(x: Int)
scala> class D extends C(1) { override def productPrefix = "D" }
scala> val s = collection.mutable.HashSet(C(1))

scala> s.contains(new D)
val res0: Boolean = false

scala> s.exists(_ == new D)
val res1: Boolean = true

@sjrd
Copy link
Member

sjrd commented Sep 19, 2024

Have we considered removing productPrefix from the hashCode computation instead?

Should we perhaps use a "hard-coded" seed instead (computed at compile-time from the hash code of the full class name, for example)?

@lrytz
Copy link
Member Author

lrytz commented Oct 17, 2024

Sorry, I missed your reply.

I initially suggested that on private chat, but @retronym pointed out that then case class hash codes would be inconsistent with MurmurHash3.productHash. Not sure what that implications that would have.

@sjrd
Copy link
Member

sjrd commented Oct 17, 2024

I don't think that's a problem. Case classes don't compare equal to other Products with the same elements.

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.

2 participants