Merge pull request #4319 from armanbilge/issue/4284

Deprecate `HashLaws#sameAsUniversalHash`
typelevel · Oct 18, 2022 · 5fe56f0 · 5fe56f0
2 parents 9b5d664 + e4630d3
commit 5fe56f0
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 20 deletions.
diff --git a/core/src/main/scala/cats/TraverseFilter.scala b/core/src/main/scala/cats/TraverseFilter.scala
@@ -23,7 +23,7 @@ package cats
 
 import cats.data.State
 
-import scala.collection.immutable.{HashSet, TreeSet}
+import scala.collection.immutable.{IntMap, TreeSet}
 
 /**
  * `TraverseFilter`, also known as `Witherable`, represents list-like structures
@@ -141,12 +141,19 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
    * This is usually faster than ordDistinct, especially for things that have a slow comparion (like String).
    */
   def hashDistinct[A](fa: F[A])(implicit H: Hash[A]): F[A] =
-    traverseFilter[State[HashSet[A], *], A, A](fa)(a =>
-      State(alreadyIn => if (alreadyIn(a)) (alreadyIn, None) else (alreadyIn + a, Some(a)))
-    )
-      .run(HashSet.empty)
-      .value
-      ._2
+    traverseFilter(fa) { a =>
+      State { (distinct: IntMap[List[A]]) =>
+        val ahash = H.hash(a)
+        distinct.get(ahash) match {
+          case None => (distinct.updated(ahash, a :: Nil), Some(a))
+          case Some(existing) =>
+            if (Traverse[List].contains_(existing, a))
+              (distinct, None)
+            else
+              (distinct.updated(ahash, a :: existing), Some(a))
+        }
+      }
+    }.run(IntMap.empty).value._2
 }
 
 object TraverseFilter {

diff --git a/free/src/test/scala-2.13+/cats/free/FreeStructuralSuite.scala b/free/src/test/scala-2.13+/cats/free/FreeStructuralSuite.scala
@@ -22,7 +22,7 @@
 package cats.free
 
 import cats.{Applicative, Eq, Eval, Functor, Show, Traverse}
-import cats.kernel.laws.discipline.{EqTests, PartialOrderTests}
+import cats.kernel.laws.discipline.{EqTests, HashTests, PartialOrderTests}
 import cats.syntax.all._
 import cats.tests.CatsSuite
 
@@ -46,8 +46,7 @@ class FreeStructuralSuite extends CatsSuite {
 
   Show[Free[Option, Int]]
 
-  // TODO HashLaws#sameAsUniversalHash is really dodgy
-  // checkAll("Free[Option, Int]", HashTests[Free[Option, Int]].hash)
+  checkAll("Free[Option, Int]", HashTests[Free[Option, Int]].hash)
   checkAll("Free[Option, Int]", PartialOrderTests[Free[Option, Int]].partialOrder)
   checkAll("Free[ExprF, String]", EqTests[Free[ExprF, String]].eqv)
 }

diff --git a/kernel-laws/shared/src/main/scala/cats/kernel/laws/HashLaws.scala b/kernel-laws/shared/src/main/scala/cats/kernel/laws/HashLaws.scala
@@ -30,10 +30,12 @@ trait HashLaws[A] extends EqLaws[A] {
   def hashCompatibility(x: A, y: A): IsEq[Boolean] =
     (!E.eqv(x, y) || (Hash.hash(x) == Hash.hash(y))) <-> true
 
+  @deprecated("This law is no longer enforced", "2.9.0")
   def sameAsUniversalHash(x: A, y: A): IsEq[Boolean] =
     ((E.hash(x) == x.hashCode) && (Hash.fromUniversalHashCode[A].hash(x) == x.hashCode()) &&
       (E.eqv(x, y) == Hash.fromUniversalHashCode[A].eqv(x, y))) <-> true
 
+  @deprecated("This law is no longer enforced", "2.9.0")
   def sameAsScalaHashing(x: A, y: A, scalaHashing: Hashing[A]): IsEq[Boolean] =
     ((E.hash(x) == Hash.fromHashing(scalaHashing).hash(x)) &&
       (E.eqv(x, y) == Hash.fromHashing(scalaHashing).eqv(x, y))) <-> true

diff --git a/kernel-laws/shared/src/main/scala/cats/kernel/laws/discipline/HashTests.scala b/kernel-laws/shared/src/main/scala/cats/kernel/laws/discipline/HashTests.scala
@@ -38,9 +38,7 @@ trait HashTests[A] extends EqTests[A] {
     new DefaultRuleSet(
       "hash",
       Some(eqv),
-      "hash compatibility" -> forAll(laws.hashCompatibility _),
-      "same as universal hash" -> forAll(laws.sameAsUniversalHash _),
-      "same as scala hashing" -> forAll((x: A, y: A) => laws.sameAsScalaHashing(x, y, hashA))
+      "hash compatibility" -> forAll(laws.hashCompatibility _)
     )
 
 }

diff --git a/tests/shared/src/test/scala/cats/tests/FunctionSuite.scala b/tests/shared/src/test/scala/cats/tests/FunctionSuite.scala
@@ -36,7 +36,6 @@ import cats.{
 }
 import cats.arrow.{ArrowChoice, Choice, CommutativeArrow}
 import cats.kernel._
-import cats.kernel.laws.HashLaws
 import cats.kernel.laws.discipline.{
   BandTests,
   BoundedSemilatticeTests,
@@ -45,6 +44,7 @@ import cats.kernel.laws.discipline.{
   CommutativeSemigroupTests,
   EqTests,
   GroupTests,
+  HashTests,
   MonoidTests,
   OrderTests,
   PartialOrderTests,
@@ -136,12 +136,7 @@ class FunctionSuite extends CatsSuite {
   checkAll("Function0[Grp]", GroupTests[Function0[Grp]].group)
   checkAll("Function0[CGrp]", CommutativeGroupTests[Function0[CGrp]].commutativeGroup)
   checkAll("Function0[Distributive]", DistributiveTests[Function0].distributive[Int, Int, Int, Id, Function0])
-
-  property("Function0[Hsh]") {
-    forAll { (x: Function0[Hsh], y: Function0[Hsh]) =>
-      HashLaws[Function0[Hsh]].hashCompatibility(x, y)
-    }
-  }
+  checkAll("Function0[Hsh]", HashTests[Function0[Hsh]].hash)
 
   // Test for Arrow applicative
   Applicative[String => *]