Hash typeclass #1712

ctongfei · 2017-05-30T00:01:43Z

The following modifications are made:

A Hash type class that extends Eq
- Hash#on is not implemented to avoid the diamond inheritance problem. Use the method Hash.by in the companion object instead.
Common Hash instances that are compatible with Scala's default are written for nearly all types in cats.kernel.instances:
- For simple types T, their Hash instances are directly coded using T#hashCode;
- For generic types (e.g. List, Stream etc.), their Hash instances require Hash instances on the type of their elements, and the Hash instances on generic types is equivalent to the Scala's default hashCode if the Hash instances on element types are from the universal T#hashCode;
- For tuples, their Hash instances follow the same rule mentioned above, but are automatically generated in the KernelBoiler.scala file.
- ~~For Set, it is not implemented yet because there's an issue (SetPartialOrder[A] uses the universal == instead of Eq #1710) regarding the Order/Eq instances on Sets.~~ Implemented using universal equals/hashCode.
Added automatic test cases in cats.kernel.laws.
Added a Contravariant instance in cats-core for Hash.
Added a type alias in the package object of cats for Hash.
Adding Hash results in ambiguous implicit resolution for Eq (since both Hash and Order are Eqs). Resolved by introducing precedence of implicits in KernelBoiler.scala. This file now produces 3 files with different implicit precedences.
Added a hash method on any type that has a Hash instance.

… Eq; Hash > Eq.

ctongfei · 2017-06-04T14:55:16Z

I need some help here: why does the JS version builds but the JVM version failed on serializability?

durban · 2017-06-06T07:59:37Z

I guess it's due to Java serialization not existing on JS. Or at least it isn't tested: https://github.com/typelevel/cats/blob/master/laws/src/main/scala/cats/laws/SerializableLaws.scala#L33.

ctongfei · 2017-06-06T13:51:19Z

@durban So basically it means that the Hash typeclass current does not serialize correctly on JVM. Do you have any idea how to fix this?

johnynek · 2017-06-06T17:44:15Z

kernel/src/main/scala/cats/kernel/instances/bigDecimal.scala

@@ -17,7 +17,9 @@ class BigDecimalGroup extends CommutativeGroup[BigDecimal] {
  override def remove(x: BigDecimal, y: BigDecimal): BigDecimal = x - y
 }

-class BigDecimalOrder extends Order[BigDecimal] {
+class BigDecimalEq extends Order[BigDecimal] with Hash[BigDecimal] {


can we not change the class name to preserve binary compatibility?

johnynek · 2017-06-06T17:44:28Z

kernel/src/main/scala/cats/kernel/instances/bigDecimal.scala

@@ -4,8 +4,8 @@ package instances
 package object bigDecimal extends BigDecimalInstances // scalastyle:ignore package.object.name

 trait BigDecimalInstances {
-  implicit val catsKernelStdOrderForBigDecimal: Order[BigDecimal] =
-    new BigDecimalOrder
+  implicit val catsKernelStdEqForBigDecimal: Order[BigDecimal] with Hash[BigDecimal] =


can we not change this name for binary compatibility?

OK I will revert these.

johnynek · 2017-06-06T17:44:37Z

kernel/src/main/scala/cats/kernel/instances/bigInt.scala

@@ -4,8 +4,8 @@ package instances
 package object bigInt extends BigIntInstances // scalastyle:ignore package.object.name

 trait BigIntInstances {
-  implicit val catsKernelStdOrderForBigInt: Order[BigInt] =
-    new BigIntOrder
+  implicit val catsKernelStdEqForBigInt: Order[BigInt] with Hash[BigInt] =


same binary compat issue.

johnynek · 2017-06-06T17:44:42Z

kernel/src/main/scala/cats/kernel/instances/bigInt.scala

@@ -17,8 +17,9 @@ class BigIntGroup extends CommutativeGroup[BigInt] {
  override def remove(x: BigInt, y: BigInt): BigInt = x - y
 }

-class BigIntOrder extends Order[BigInt] {
+class BigIntEq extends Order[BigInt] with Hash[BigInt] {


johnynek · 2017-06-06T17:44:57Z

kernel/src/main/scala/cats/kernel/instances/bitSet.scala

@@ -6,14 +6,16 @@ import scala.collection.immutable.BitSet
 package object bitSet extends BitSetInstances

 trait BitSetInstances {
-  implicit val catsKernelStdPartialOrderForBitSet: PartialOrder[BitSet] =
-    new BitSetPartialOrder
+  implicit val catsKernelStdEqForBitSet: PartialOrder[BitSet] with Hash[BitSet] =


please don't change name

johnynek · 2017-06-06T17:48:27Z

kernel/src/main/scala/cats/kernel/instances/either.scala

-            case Left(_) => false
-            case Right(yy) => B.eqv(xx, yy)
-          }
+          case Left(xx) => Left(A.hash(xx)).##


can we not allocate just to get the hashcode.

Something like Left(xx) => A.hash(xx) * 65521 Right(xx) => B.hash(xx) seems fine:
mixing the left with a prime about half the size of the int space (taken from the largest 16 bit here):
http://www.risc.jku.at/people/hemmecke/AldorCombinat/combinatse20.html

I want to have the default Hash instance for Either[A, B] perform the exact same way as the universal hash (##) given the Hash instance for A and B are universal hashes.
So maybe I'll copy the function implementation for case classes in Scala?
Basically I want EitherHash[A, B](DefaultHashInstance[A], DefaultHashInstance[B])(Left(a)) == Left(a).##.

So maybe I'll copy the function implementation for case classes in Scala?

👍

durban · 2017-06-21T11:25:53Z

@ctongfei Well, since Hash itself is Serializable, we should find out which object is the one which causes the serialization failure ... -Dsun.io.serialization.extendedDebugInfo=true is a useful system property for debugging things like this (it prints the path of references from the original serialized object to the one which is not serializable).

fommil · 2017-08-06T20:13:49Z

kernel/src/main/scala/cats/kernel/Hash.scala

+  /**
+   * Returns the hash code of the given object under this hashing scheme.
+   */
+  def hash(x: A): Int


consider using Long and move away from the JVM legacy?

@fommil What are the justifications of using Long? JVM-default Int works good enough I think?

The Object#hashCode: Int in Java is basically used for distribution within Set or Map-like data structures and for that purpose Int will remain sufficient for some time, for one because the size of an array in Java can't exceed Int.MaxValue either and if you instantiate an Array<Int> of size Int.MaxValue that's around ~ 8 GB of RAM.

So I'm not seeing the size of an array evolving to a Long, for one because of backwards compatibility, but also it's probably counter productive to allow contiguous memory blocks bigger than that. For now at least.
I'm guessing that's the reason other languages use 32 bit integers (or sometimes less) for their hash codes as well. For example: https://hackage.haskell.org/package/hashable-1.2.6.1/docs/Data-Hashable.html

The questions are:

besides doing distribution in data structures like HashMap / HashSet, are there any other use cases?

doesn't Long have performance implications? it's an important question to answer, as HashMaps tend to be abused

are there any languages using Long for their universal hashCode already and if so, I'd be interested in why?

64 bit JVMs store both Int and Long as 64 bit numbers, so there is no advantage to use Int for space preserving reasons. However if you want to make use of existing hashCode algorithms that return Int then that would be a good reason to keep it that way.

I think that for now we should stay with what Object#hashCode is doing: returning an Int. Agree with @alexandru .

I disagree about using Int. If you have a Long you can easily get an Int from int, but not the other way around. Some nice algorithms using hashes want more than 32 bits. For instance hyperloglog with only 32 bits would not be great.

+1 to using Long here. If you want to just give me an Int upcast to Long that's on you, but it would be better to enable better algorithms.

alexandru

On the whole PR seems fine to me.

alexandru · 2017-08-09T14:34:25Z

core/src/main/scala/cats/instances/hash.scala

+trait HashInstances {
+  implicit val catsFunctorContravariantForHash: Contravariant[Hash] =
+    new Contravariant[Hash] {
+      /** Derive an `Hash` for `B` given an `Hash[A]` and a function `B => A`.


Small nitpick, I obsess over details, but comment style isn't compatible with either ScalaDoc or JavaDoc. This title needs a new-line.

alexandru · 2017-08-09T14:38:14Z

kernel-laws/src/main/scala/cats/kernel/laws/HashLaws.scala

+
+  def hash(implicit A: Hash[A]): HashProperties = new HashProperties(
+    name = "hash",
+    parent = None,


Shouldn't this have EqLaws as parent?

OrderLaws's parent is also None. But EqLaws are listed below in the props. I copied that behavior.

seems like we should have EqLaws as a parent.

I couldn't find EqLaws in the kernel-laws package.

alexandru · 2017-08-09T14:51:57Z

kernel/src/main/scala/cats/kernel/Hash.scala

+  /**
+   * Returns the hash code of the given object under this hashing scheme.
+   */
+  def hash(x: A): Int


The Object#hashCode: Int in Java is basically used for distribution within Set or Map-like data structures and for that purpose Int will remain sufficient for some time, for one because the size of an array in Java can't exceed Int.MaxValue either and if you instantiate an Array<Int> of size Int.MaxValue that's around ~ 8 GB of RAM.

So I'm not seeing the size of an array evolving to a Long, for one because of backwards compatibility, but also it's probably counter productive to allow contiguous memory blocks bigger than that. For now at least.
I'm guessing that's the reason other languages use 32 bit integers (or sometimes less) for their hash codes as well. For example: https://hackage.haskell.org/package/hashable-1.2.6.1/docs/Data-Hashable.html

The questions are:

besides doing distribution in data structures like HashMap / HashSet, are there any other use cases?

doesn't Long have performance implications? it's an important question to answer, as HashMaps tend to be abused

are there any languages using Long for their universal hashCode already and if so, I'd be interested in why?

alexandru · 2017-08-09T14:54:40Z

kernel/src/main/scala/cats/kernel/instances/either.scala

-            case Left(_) => false
-            case Right(yy) => B.eqv(xx, yy)
-          }
+          case Left(xx) => Left(A.hash(xx)).##


So maybe I'll copy the function implementation for case classes in Scala?

👍

durban · 2017-08-26T18:28:08Z

@ctongfei If you move EitherInstances1#EitherEq to the toplevel (i.e., make it so that it isn't a nested class), that fixes the serialization problem. (Since the problem is caused by the outer class reference not being serializable.)

…ject

codecov-io · 2017-08-29T03:48:42Z

Codecov Report

Merging #1712 into master will increase coverage by 0.02%.
The diff coverage is 98.47%.

@@            Coverage Diff            @@
##           master   #1712      +/-   ##
=========================================
+ Coverage   95.57%   95.6%   +0.02%     
=========================================
  Files         248     252       +4     
  Lines        4454    4570     +116     
  Branches      115     117       +2     
=========================================
+ Hits         4257    4369     +112     
- Misses        197     201       +4

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/hash.scala	`0% <0%> (ø)`
core/src/main/scala/cats/instances/hash.scala	`100% <100%> (ø)`
...aws/src/main/scala/cats/kernel/laws/HashLaws.scala	`100% <100%> (ø)`
...nel/src/main/scala/cats/kernel/instances/set.scala	`100% <100%> (ø)`	⬆️
...src/main/scala/cats/kernel/instances/boolean.scala	`100% <100%> (ø)`	⬆️
...el/src/main/scala/cats/kernel/instances/char.scala	`100% <100%> (ø)`	⬆️
kernel/src/main/scala/cats/kernel/Hash.scala	`100% <100%> (ø)`
.../src/main/scala/cats/kernel/instances/stream.scala	`100% <100%> (ø)`	⬆️
testkit/src/main/scala/cats/tests/Helpers.scala	`100% <100%> (ø)`	⬆️
.../src/main/scala/cats/kernel/instances/double.scala	`75% <100%> (+1.66%)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a915afb...601e18d. Read the comment docs.

…s.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder

ctongfei · 2017-08-29T15:59:13Z

It seems that the current version has some binary incompatibility issues with Scala 2.10/2.11 as is indicated by Travis CI:

filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]
("cats.kernel.instances.MapInstances.catsKernelStdHashForMap")

etc. because these are new methods added to existing traits.
Should we just wait until cats 1.0.0?

kailuowang · 2017-08-29T16:07:46Z

as of now we haven't decided if we want to break kernel bin compat in 1.0.0. @johnynek thoughts?

ctongfei · 2017-09-18T00:47:19Z

Apart from binary incompatibility issues with JVM Scala 2.10/2.11, I think that this PR is ready to be merged now.

There are several lines not covered by tests (I don't think they should):
cats/syntax/hash.scala Line 9: This line should be inlined by the macro.
cats/laws/discipline/Eq.scala Line 93: This line should never be run: it means that arbitrary values are not properly generated.

Thanks :-)

johnynek · 2017-09-28T01:02:03Z

can you add the mima exclusions as was done in #1925 ?

ctongfei · 2017-09-28T14:22:18Z

@johnynek I don't fully understand the rules of these binary checks? Could you elaborate? Thanks!

kailuowang · 2017-09-28T15:38:28Z

@ctongfei in sbt console if you run

++2.11.11
project kernelJVM
mimaReportBinaryIssues

you will get a report of the violating classes as well as the filters needed to make them exceptions
Add those filters to here https://github.com/typelevel/cats/blob/master/build.sbt#L239 and you should be good to go.

kailuowang · 2017-10-03T21:38:07Z

@ctongfei do you want to merge in master?

johnynek

This is great.

When we merge with master, I think we are green.

Thanks for carrying this one so far! I look forward to someone building HashSet and HashMap based on this Hash typeclass (not to mention small bloom filters or hll).

alexandru · 2017-10-04T08:04:36Z

👍

ctongfei · 2017-10-04T15:12:06Z

Thanks everyone :-)

kailuowang · 2017-10-04T15:13:30Z

Thank you @ctongfei !

* more instances to Hash: Queue/Duration * more instances to Hash: Queue/Duration

ctongfei and others added 8 commits May 29, 2017 17:45

cats.kernel.Hash and related instances (#1690)

9587384

Hash laws

a1ef839

all test passed

b7fb209

Hash instances for tuples

04c1f96

introduce implicit precedence in KernelBoiler: Order > PartialOrder >…

e6e6159

… Eq; Hash > Eq.

Add type alias in cats._; Add contravariant instance for Hash

66468e0

HashFunctions extends EqFunctions

92f808c

Merge branch 'master' into hash

69d4259

ctongfei changed the title ~~Hash typeclass (#1690)~~ Hash typeclass May 30, 2017

ctongfei mentioned this pull request May 30, 2017

Add Hash typeclasses to cats #1690

Closed

Move contravariant instances to separate trait, in accordance with (#…

932903d

…1659)

johnynek requested changes Jun 6, 2017

View reviewed changes

revert name change

c8b3d5e

ctongfei changed the title ~~Hash typeclass~~ Hash typeclass (WIP) Aug 1, 2017

fommil reviewed Aug 6, 2017

View reviewed changes

kailuowang added this to the 1.0.0-RC1 milestone Aug 9, 2017

alexandru reviewed Aug 9, 2017

View reviewed changes

ctongfei added 2 commits August 28, 2017 23:12

move EitherInstances1#EitherEq out

650e4f5

Optimize hash computation on case classes without allocating a new ob…

bb043a6

…ject

fixing the problem in CI build: method catsKernelStdOrderForChar()cat…

d087613

…s.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder

This was referenced Aug 31, 2017

Add some instances we were missing. #1878

Merged

Decide on a solution for adding new features to cats-kernel avoiding breaking bin-compat. #1879

Open

ctongfei added 2 commits September 17, 2017 16:54

increase coverage

cc034ea

increase coverage, ## => hashCode, kernelBoiler

54b04eb

kailuowang mentioned this pull request Sep 21, 2017

temporarily disable MiMa check on kernel #1925

Merged

suppress mimaReportBinaryIssues

136e203

johnynek previously approved these changes Oct 4, 2017

View reviewed changes

Merge branch 'master' into hash

526757b

LukaJCB dismissed johnynek’s stale review via 526757b October 4, 2017 07:08

LukaJCB added 2 commits October 4, 2017 09:19

Remove cats.functor

49403c6

Remove cats.functor

601e18d

LukaJCB approved these changes Oct 4, 2017

View reviewed changes

LukaJCB merged commit c131fe4 into typelevel:master Oct 4, 2017

ctongfei deleted the hash branch October 4, 2017 16:03

ctongfei mentioned this pull request Oct 5, 2017

more instances for Hash (#1712): Queue/Duration #1950

Merged

kailuowang pushed a commit that referenced this pull request Oct 6, 2017

more instances for Hash (#1712): Queue/Duration (#1950)

4ffd3a1

* more instances to Hash: Queue/Duration * more instances to Hash: Queue/Duration

ctongfei mentioned this pull request Oct 8, 2017

Hash families #1957

Open

kailuowang modified the milestone: 1.0.0-RC1 Oct 13, 2017

kailuowang added the enhancement label Oct 15, 2017

kailuowang mentioned this pull request Dec 18, 2017

conversion PartialOrder to PartialOrdering and Hash to Hashing #2116

Merged

ctongfei mentioned this pull request Jun 10, 2019

Hash instances derived from Hash[Long] (e.g. Hash[List[Long]]) do not pass laws #2878

Closed

kailuowang mentioned this pull request Jul 19, 2019

Unlawful Hash instances for Function0 #2952

Closed

armanbilge mentioned this pull request Jul 13, 2022

Suppress Compiler Warnings (kernel) #4187

Closed

Hash typeclass #1712

Hash typeclass #1712

Conversation

ctongfei commented May 30, 2017 • edited Loading

ctongfei commented Jun 4, 2017

durban commented Jun 6, 2017

ctongfei commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durban commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctongfei Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durban commented Aug 26, 2017

codecov-io commented Aug 29, 2017 • edited Loading

Codecov Report

ctongfei commented Aug 29, 2017

kailuowang commented Aug 29, 2017

ctongfei commented Sep 18, 2017 • edited Loading

johnynek commented Sep 28, 2017

ctongfei commented Sep 28, 2017

kailuowang commented Sep 28, 2017

kailuowang commented Oct 3, 2017

johnynek left a comment

Choose a reason for hiding this comment

alexandru commented Oct 4, 2017

ctongfei commented Oct 4, 2017

kailuowang commented Oct 4, 2017

ctongfei commented May 30, 2017 •

edited

Loading

ctongfei Sep 5, 2017 •

edited

Loading

codecov-io commented Aug 29, 2017 •

edited

Loading

ctongfei commented Sep 18, 2017 •

edited

Loading