-
Notifications
You must be signed in to change notification settings - Fork 64
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
Scala 2.13.0 and Hash derivation rework #157
Conversation
implicit def mkHashGenericProduct[A, R <: HList]( | ||
implicit A: Generic.Aux[A, R], R: Lazy[HashBuilder[R]], ev: A <:< Product = null | ||
): MkHash[A] = instance( | ||
x => R.value.hash(A.to(x), if (ev == null) MurmurHash3.productSeed else util.VersionSpecific.productSeed(x)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a pattern match here? like
x match {
case p : Product => util.VersionSpecific.productSeed(p)
case _ => MurmurHash3.productSeed
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the part I'm not sure about. On one hand, ev: A <:< Product = null
looks like a hack, but on the other pattern matching on the type parameter would break parametricity.
But what if the solution is to drop this law altogether from cats-testkit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think about the law. Could we split this one method into two derivation methods with different priorities? One for Product another for the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's also an option 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. We can later remove the version specific code if we decide not to enforce this law.
This uncovered an issue with |
65c637b
to
2c9bbc4
Compare
Case classes' `hashCode` is not backwards compatible in Scala 2.13, because it mixes the `productPrefix` for better distribution. This means we need version specific code to pass the `Hash` laws. * Update dependencies for Scala 2.13.0 * Rework `Hash` derivation for consistency * Split product and coproduct derivation * Add a Scala version specific seed for `scala.Product` subtypes * Optimize `HashBuilder` to a one-pass hash * Replace usages of `Statics` (not recommended) with `MurmurHash3` * Test all variants of `Hash` derivation
Updated to Scala 2.13.0 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hurrah
Case classes'
hashCode
is not backwards compatible in Scala 2.13,because it mixes the
productPrefix
for better distribution.This means we need version specific code to pass the
Hash
laws.Hash
derivation for consistencyscala.Product
subtypesHashBuilder
to a one-pass hashStatics
(not recommended) withMurmurHash3
Hash
derivation