Cats: Ambiguous Eq instances for tuples

[ceedubs]

I think that the situations in which this arises are somewhat uncommon, but I don't know of any good way to work around it when it does happen. For example here's what happens when you try to summon the Eq[(Set[Boolean], Set[Boolean])] instance:

scala> Eq[(Set[Boolean], Set[Boolean])]
<console>:18: error: ambiguous implicit values:
 both method catsKernelStdHashForTuple2 in trait TupleInstances1 of type [A0, A1](implicit A0: cats.kernel.Hash[A0], implicit A1: cats.kernel.Hash[A1])cats.kernel.Hash[(A0, A1)]                          
 and method catsKernelStdPartialOrderForTuple2 in trait TupleInstances1 of type [A0, A1](implicit A0: cats.kernel.PartialOrder[A0], implicit A1: cats.kernel.PartialOrder[A1])cats.kernel.PartialOrder[(A0,
A1)]
 match expected type cats.kernel.Eq[(Set[Boolean], Set[Boolean])]
       Eq[(Set[Boolean], Set[Boolean])]
         ^

Summoning Eq[Set[Boolean]] by itself works fine:

scala> Eq[Set[Boolean]]
res0: cats.kernel.Eq[Set[Boolean]] = cats.kernel.instances.SetHash@4493c47

And summoning Eq[(List[Boolean], List[Boolean])] works:

scala> Eq[(List[Boolean], List[Boolean])]
res2: cats.kernel.Eq[(List[Boolean], List[Boolean])] = cats.kernel.instances.TupleInstances$$anon$90@155e7e 

I think that the difference is due to implicit resolution happening differently for List since its Eq and Hash instances have constraints on the A type, while Set's instances do not. I think that it comes down to this code, where the Hash and PartialOrder instances should be in different layers of the hierarchy since they share a common Eq superclass. However, I think that changing that to not be the case would be binary incompatible (but maybe only prior to 2.12?).

[kailuowang]

A slightly convoluted way to do it without breaking BC might be disabling the existing ones by making them not implicit and create a new hierarchy of traits and let the objects extending them. Of course, the fact these methods are all generated code makes things more complicated, but I feel that this should be possible.

[ceedubs]

Thanks @kailuowang that's a good idea. I can give it a try. Do we have a naming convention for the implicit methods that we wish had the name of the old versions that we had to keep around because of binary compatibility? 😅

[kailuowang]

It's not a very established convention, but we used BinCompat suffix a couple of times.

[ceedubs]

Doing this generates a bunch of ReversedMissingMethodProblem mima errors. It's probably safe to exclude those since we only care about backward compatibility and not forward compatibility, right? Do we ever care about ReversedMissingMethodProblem?

[kailuowang]

I don't fully understand the ramification for ReversedMissingMethodProblem, but the best way to find out is to add tests to https://github.com/typelevel/cats/blob/master/binCompatTest/src/main/scala/catsBC/MimaExceptions.scala

[ceedubs]

Another good idea :)

I need to focus on work for a while, but I'll try to come back to this soon.

[ceedubs]

Oh I get it now. The problem is with adding it to the trait pre-Scala 2.12. Hmmm I can't think of a way around this that's binary-compatible before Scala 2.12 😬.

[kailuowang]

I wasn't suggesting adding methods to exiting traits. I was suggesting adding a new trait hierarchy and only let cats.instances.all cats.instances.tuple and cats.implicits extend it. Is that not going to work?

[ceedubs]

@kailuowang the Order instance for tuples is defined at the top level of the TupleInstances trait, so I _think_ that if we try to create a new trait and add an Eq or Hash instance to it, we could potentially end up with ambiguities when both are mixed into all/tuple/implicits. Correct me if I'm misunderstanding.

[kailuowang]

@ceedubs we can "move" the OrderInstance into the new hierarchy too right?

Update: But yeah, it's convoluted.

[ceedubs]

Oh I see. This is going to get messy but yeah we could probably do that.

I'd like to see Cats move in a direction where we commit the reasonable
change to master and have some people working on backporting the messy
bincompat changes. This is a high burden to place on a non-maintainer who
wants to commit.

On Thu, Jan 17, 2019, 6:42 PM Kai(luo) Wang <[email protected] wrote:

@ceedubs https://github.com/ceedubs we can "move" the OrderInstance
into the new hierarchy right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/typelevel/cats/issues/2701#issuecomment-455407757,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7sCTQ7KZPUYXCW8JXObbpnsRr1BPkBks5vETR5gaJpZM4aF_aT
.

[kailuowang]

TBH, a high burden for maintainers as well. Having dedicated force to do the backporting could be a good midterm solution to support 2.11 longer. (eventually, the diversion between this branch and master might be too messy to be sustainable).

[ceedubs]

I came back to this today, and I'm a bit inclined to only fix this in Cats 2.1+ (once we drop pre-scala 2.12 bincompat support). Doing this in a backwards-compatible way gets messy and leads to lots of duplicated auto-generated boilerplate code. So far I'm the only one who has reported hitting trouble with this, so I suspect that it's hit rarely enough that taking this approach wouldn't lead to troubles for many people.

[travisbrown]

The fix in #3105 ended up breaking bincompat (MiMa missed it), so we had to revert it for now.