Hash[Map[K, V]] has unnecessary constraint #3039 #3060

vasiliybondarenko · 2019-09-13T14:52:28Z

Removed Order constraint from Hash[Map[K, V]]but bincomp is broken. Please advise how it can be overcome

travisbrown · 2019-09-15T07:28:26Z

@vasiliybondarenko Adding a method to a trait breaks bincompat on Scala 2.11. Once #3051 is merged you'll be able to merge master and the bincompat check should be fine.

travisbrown · 2019-09-15T07:31:00Z

But these functions are ambiguous I using the same name

Personally I'd prefer to make the old over-constrained version of the method package-private if necessary in 2.1.0 rather than add a suffix to the unbroken one, but I think the overload should be fine as long as the broken one isn't implicit, right?

vasiliybondarenko · 2019-09-16T12:35:26Z

But these functions are ambiguous I using the same name

Personally I'd prefer to make the old over-constrained version of the method package-private if necessary in 2.1.0 rather than add a suffix to the unbroken one, but I think the overload should be fine as long as the broken one isn't implicit, right?

Overloading for catsKernelStdHashForSortedSet works unless this function has no explicit usage but it has in the following traits:
cats.instances.SortedSetInstances1
cats.instances.SortedSetInstancesBinCompat1
cats.instances.LowPrioritySortedSetInstancesBinCompat1

So I assume the only solution is to add suffix to unbroken function with a proper constraint.

codecov-io · 2019-10-05T11:32:18Z

Codecov Report

Merging #3060 into master will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #3060      +/-   ##
==========================================
- Coverage   93.51%   93.48%   -0.03%     
==========================================
  Files         368      368              
  Lines        6983     6985       +2     
  Branches      195      195              
==========================================
  Hits         6530     6530              
- Misses        453      455       +2

Impacted Files	Coverage Δ
core/src/main/scala/cats/instances/sortedSet.scala	`88.23% <33.33%> (ø)`	⬆️
...ala/cats/kernel/instances/SortedSetInstances.scala	`91.66% <33.33%> (-8.34%)`	⬇️

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 ed77f8e...88e221a. Read the comment docs.

larsrh · 2019-10-05T11:33:28Z

I took the liberty of rebasing this to current master. Let's see if this works.

larsrh · 2019-10-05T12:49:01Z

MiMa still complains:

[error]  * method this(cats.kernel.Order,cats.kernel.Hash)Unit in class cats.kernel.instances.SortedSetHash does not have a correspondent in current version

I think we'll have to add a deprecated extra constructor in there. I'll take care of it.

vasiliybondarenko · 2019-10-05T12:55:20Z

MiMa still complains:
[error]  * method this(cats.kernel.Order,cats.kernel.Hash)Unit in class cats.kernel.instances.SortedSetHash does not have a correspondent in current version
I think we'll have to add a deprecated extra constructor in there. I'll take care of it.

Cool! Thanks

missed something

kailuowang · 2019-10-08T13:51:25Z

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

@@ -94,7 +94,7 @@ private[instances] trait SortedSetInstancesBinCompat0 {
 private[instances] trait SortedSetInstancesBinCompat1 extends LowPrioritySortedSetInstancesBinCompat1 {
  // TODO: Remove when this is no longer necessary for binary compatibility.
  implicit override def catsKernelStdHashForSortedSet[A: Order: Hash]: Hash[SortedSet[A]] =
-    cats.kernel.instances.sortedSet.catsKernelStdHashForSortedSet[A]
+    cats.kernel.instances.sortedSet.catsKernelStdHashForSortedSet1[A]


@travisbrown
Just noticed this, should this method be deprecated and un-implicit like the one above? The Todo comment should probably be removed too?

@kailuowang What's happening here is tricky: these instances were in the wrong place (cats.instances.SortedSetInstances1 instead of cats.kernel.instances), and now that they're in the right place these bincompat traits get the methods from both, which results in an inherits conflicting members compiler error, which means we have to bubble overrides all the way to the top.

kailuowang · 2019-10-08T13:52:07Z

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

@@ -104,7 +104,7 @@ private[instances] trait LowPrioritySortedSetInstancesBinCompat1
    cats.kernel.instances.sortedSet.catsKernelStdOrderForSortedSet[A]

  override def catsKernelStdHashForSortedSet[A: Order: Hash]: Hash[SortedSet[A]] =
-    cats.kernel.instances.sortedSet.catsKernelStdHashForSortedSet[A]
+    cats.kernel.instances.sortedSet.catsKernelStdHashForSortedSet1[A]


@travisbrown, not sure what this override is for? Can we add a comment?

kailuowang · 2019-10-08T13:53:00Z

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

 import scala.collection.immutable.SortedSet

 trait SortedSetInstances extends SortedSetInstances1 {
-  implicit def catsKernelStdHashForSortedSet[A: Order: Hash]: Hash[SortedSet[A]] =
+  @deprecated("Will be removed after dropping Scala 2.11 support", "")


Can the deprecated message be "Use catsKernelStdHashForSortedSet1 instead", "2.1.0" ?

kailuowang · 2019-10-08T13:53:52Z

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

  import scala.util.hashing.MurmurHash3._

+  @deprecated("Use the constructor _without_ Order instead, since Order is not required", "2.0.1")


Nit: more likely that the derrecate version is 2.1.0

kailuowang · 2019-10-08T13:58:14Z

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

@@ -28,9 +31,13 @@ class SortedSetOrder[A: Order] extends Order[SortedSet[A]] {
    StaticMethods.iteratorEq(s1.iterator, s2.iterator)
 }

-class SortedSetHash[A: Order: Hash] extends Hash[SortedSet[A]] {
+// FIXME use context bound in 3.x


Just curious, context bound breaks BC here (even with the deprecated constructor)?

Yes, because it affects the constructor.

travisbrown

I've got a branch lined up that includes @vasiliybondarenko's commit and also addresses the feedback here, simplifies things a bit, and documents the weird override stuff. I'll open it as soon as #3125 is merged, since they overlap a lot, and that one is trivial.

Hash[Map[K, V]] has unnecessary constraint typelevel#3039

c5e5e37

larsrh force-pushed the ordering-remove branch from 2c0a30e to c5e5e37 Compare October 5, 2019 11:32

larsrh changed the title ~~[WIP] Hash[Map[K, V]] has unnecessary constraint #3039~~ Hash[Map[K, V]] has unnecessary constraint #3039 Oct 5, 2019

larsrh previously approved these changes Oct 5, 2019

View reviewed changes

fix binary compatibility

88e221a

larsrh dismissed their stale review via 88e221a October 5, 2019 12:57

larsrh approved these changes Oct 5, 2019

View reviewed changes

larsrh requested review from travisbrown and kailuowang October 5, 2019 12:58

kailuowang previously approved these changes Oct 8, 2019

View reviewed changes

kailuowang reviewed Oct 8, 2019

View reviewed changes

travisbrown suggested changes Nov 5, 2019

View reviewed changes

larsrh closed this Nov 5, 2019

travisbrown mentioned this pull request Nov 5, 2019

Remove Order constraint from Hash instance for Map #3126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hash[Map[K, V]] has unnecessary constraint #3039 #3060

Hash[Map[K, V]] has unnecessary constraint #3039 #3060

vasiliybondarenko commented Sep 13, 2019 •

edited by larsrh

Loading

travisbrown commented Sep 15, 2019

travisbrown commented Sep 15, 2019

vasiliybondarenko commented Sep 16, 2019

codecov-io commented Oct 5, 2019 •

edited

Loading

larsrh commented Oct 5, 2019 •

edited

Loading

larsrh commented Oct 5, 2019

vasiliybondarenko commented Oct 5, 2019

kailuowang Oct 8, 2019

travisbrown Nov 5, 2019

kailuowang Oct 8, 2019

kailuowang Oct 8, 2019

kailuowang Oct 8, 2019

kailuowang Oct 8, 2019

larsrh Oct 8, 2019

travisbrown left a comment

		import scala.util.hashing.MurmurHash3._

		@deprecated("Use the constructor _without_ Order instead, since Order is not required", "2.0.1")

Hash[Map[K, V]] has unnecessary constraint #3039 #3060

Hash[Map[K, V]] has unnecessary constraint #3039 #3060

Conversation

vasiliybondarenko commented Sep 13, 2019 • edited by larsrh Loading

travisbrown commented Sep 15, 2019

travisbrown commented Sep 15, 2019

vasiliybondarenko commented Sep 16, 2019

codecov-io commented Oct 5, 2019 • edited Loading

Codecov Report

larsrh commented Oct 5, 2019 • edited Loading

larsrh commented Oct 5, 2019

vasiliybondarenko commented Oct 5, 2019

kailuowang Oct 8, 2019

Choose a reason for hiding this comment

travisbrown Nov 5, 2019

Choose a reason for hiding this comment

kailuowang Oct 8, 2019

Choose a reason for hiding this comment

kailuowang Oct 8, 2019

Choose a reason for hiding this comment

kailuowang Oct 8, 2019

Choose a reason for hiding this comment

kailuowang Oct 8, 2019

Choose a reason for hiding this comment

larsrh Oct 8, 2019

Choose a reason for hiding this comment

travisbrown left a comment

Choose a reason for hiding this comment

vasiliybondarenko commented Sep 13, 2019 •

edited by larsrh

Loading

codecov-io commented Oct 5, 2019 •

edited

Loading

larsrh commented Oct 5, 2019 •

edited

Loading