fix ambiguous ContravariantShow[SortedSet[A]] (& SortedMap) #4575

chwthewke · 2024-03-15T15:43:49Z

Fixes #4574 (I hope...)

joroKr21 · 2024-03-15T15:48:16Z

core/src/main/scala/cats/Show.scala

+  implicit def catsShowForSortedSet[A: Show]: Show[SortedSet[A]] = cats.instances.sortedSet.catsStdShowForSortedSet[A]
+  implicit def catsShowForSortedMap[K: Show, V: Show]: Show[SortedMap[K, V]] =
+    cats.instances.sortedMap.catsStdShowForSortedMap[K, V]


It looks like now Show will be ambiguous. What if we move catsShowForSet and catsShowForMap up instead? Does it work?

Also adding a test would be great (well not really a test, just summon the instance in test code - I think if you look for it there are places that test summoning some instance compiles).

I'm not seeing an error trying to summon, e.g. Show[SortedMap[Int, String]] with this patch. Should I be looking at another test case?

Also, I did not write that anywhere, but my change was largely motivated by the observation that Duration and FiniteDuration are like that and their implicits are OK

Oh, my bad - you're right. The rule goes like this:

class DefinesA { implicit a: A } class DefinesB { implicit b: B }

if A <:< B then a gets a point in the implicit score

if DefinesB <:< DefinesA then b gets a point in the implicit score

so both would be 1:1 so ambiguous

However, Show[SortedSet[A]] is not a subtype of Show[Set[A]] because Show is invariant.
It just looked similar to issues we've had in the past like this one: #4569

But this makes me think that it will pick the wrong instance for SortedSet and SortedMap - I guess that warrants adding a unit test.

Also BitSet has the same issue

But this makes me think that it will pick the wrong instance for SortedSet and SortedMap - I guess that warrants adding a unit test.

It does seem to be the case (for implicitly[ContravariantShow[...]], since as you noted Show[...] cannot have that problem, being invariant).

I'll look into it more when I can, hopefully before Monday.

The relationship between SortedSet and Set is the same as List and Seq, e.g.
And the instance for Seq is even further down, so the opposite - we moved Seq, not List

chwthewke · 2024-03-16T12:12:58Z

Some good news: I've added tests for ContravariantShow implicit resolution and show interpolation for Set/SortedSet/BitSet and Map/SortedMap, and they look good to me (the solution was indeed to pull the superclass instances into parent traits and not the reverse).

Some less good news but it's okay, kinda: Duration/FiniteDuration have the same problem, where implicitly[Show.ContravariantShow[FiniteDuration]] pulls the Show[Duration] instance, and I've exhausted my (limited) bag of tricks (including moving stuff along the trait hierarchy and adding dummy value parameters) trying to make it not do that. On the other hand, the implementation of both Show[FiniteDuration] and Show[Duration] is Show.fromToString, so the "incorrect" instance behaves identically to the correct one. Although I feel it's a teeny tiny bit brittle to rely on that fact, there should never be an issue in practice.

joroKr21 · 2024-03-16T17:21:16Z

core/src/main/scala/cats/Show.scala

@@ -120,8 +117,12 @@ object Show extends ScalaVersionSpecificShowInstances with ShowInstances {
 private[cats] trait ShowInstances extends cats.instances.NTupleShowInstances with ShowInstances0 {
  implicit def catsShowForFiniteDuration: Show[FiniteDuration] =
    cats.instances.finiteDuration.catsStdShowForFiniteDurationUnambiguous
+
+  implicit def catsShowForSortedSet[A: Show]: Show[SortedSet[A]] = cats.instances.sortedSet.catsStdShowForSortedSet[A]


This could've stayed in the old position

If I move catsShowForSortedSet back where it was in the Show companion object, I get an ambiguous implicit error on ContravariantShow[BitSet]

[...]\cats\tests\shared\src\test\scala\cats\tests\ShowSuite.scala:92:15
ambiguous implicit values:
both method catsShowForBitSet in object Show of type cats.Show[scala.collection.immutable.BitSet]
and method catsShowForSortedSet in object Show of type [A](implicit evidence$8: cats.Show[A]): cats.Show[scala.collection.immutable.SortedSet[A]]
match expected type cats.Show.ContravariantShow[scala.collection.immutable.BitSet]
implicitly[ContravariantShow[BitSet]]

Ah yes, BitSet is a subtype of SortedSet, good catch 👍

joroKr21 · 2024-03-16T17:23:09Z

Some less good news but it's okay, kinda: Duration/FiniteDuration have the same problem, where implicitly[Show.ContravariantShow[FiniteDuration]] pulls the Show[Duration] instance, and I've exhausted my (limited) bag of tricks (including moving stuff along the trait hierarchy and adding dummy value parameters) trying to make it not do that. On the other hand, the implementation of both Show[FiniteDuration] and Show[Duration] is Show.fromToString, so the "incorrect" instance behaves identically to the correct one. Although I feel it's a teeny tiny bit brittle to rely on that fact, there should never be an issue in practice.

If you're interested in resolving the theoretical issue with Duration and FiniteDuration, I think it should be enough to just swap their places. Did you try it?

chwthewke · 2024-03-16T17:33:18Z

If you're interested in resolving the theoretical issue with Duration and FiniteDuration, I think it should be enough to just swap their places. Did you try it?

That is exactly the first thing I tried, and it results in ContravariantShow[Duration] eq ContravariantShow[FiniteDuration] being true.

joroKr21 · 2024-03-16T17:48:33Z

That is exactly the first thing I tried, and it results in ContravariantShow[Duration] eq ContravariantShow[FiniteDuration] being true.

That's because it's the same lambda (fromToString) - LMF FTW

chwthewke · 2024-03-17T09:29:10Z

That is exactly the first thing I tried, and it results in ContravariantShow[Duration] eq ContravariantShow[FiniteDuration] being true.

That's because it's the same lambda (fromToString) - LMF FTW

Not sure what "LMF" means, but then I realized that catsStdShowForDurationUnambiguous and catsStdShowForFiniteDurationUnambiguous are also the same instance, and I have no idea why. In any case that means I don't think I want to change anything regarding FiniteDuration/Duration. (If there's only going to be a single Show[Duration] value - also serving as Show[FiniteDuration] - in a given JVM, nothing bad will ever happen).

All in all, I'd say I'm done here.

joroKr21 · 2024-03-17T12:43:19Z

Not sure what "LMF" means

LambdaMetafactory, used to implement lambdas on the JVM

satorg

Thanks!

chwthewke mentioned this pull request Mar 15, 2024

ambiguous implicit resolution of Show.ContravariantShow[immutable.SortedMap[K, V]] #4574

Closed

joroKr21 reviewed Mar 15, 2024

View reviewed changes

chwthewke marked this pull request as draft March 15, 2024 17:39

fix ambiguous implicit ContravariantShow for SortedSet and SortedMap

e1c9b2c

chwthewke force-pushed the cats-4574 branch from 868ede1 to e1c9b2c Compare March 16, 2024 11:58

chwthewke marked this pull request as ready for review March 16, 2024 12:13

joroKr21 approved these changes Mar 16, 2024

View reviewed changes

satorg approved these changes Apr 3, 2024

View reviewed changes

satorg merged commit da8cb9b into typelevel:main Apr 3, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ambiguous ContravariantShow[SortedSet[A]] (& SortedMap) #4575

fix ambiguous ContravariantShow[SortedSet[A]] (& SortedMap) #4575

chwthewke commented Mar 15, 2024

joroKr21 Mar 15, 2024

joroKr21 Mar 15, 2024

chwthewke Mar 15, 2024

chwthewke Mar 15, 2024

joroKr21 Mar 15, 2024

joroKr21 Mar 15, 2024

joroKr21 Mar 15, 2024

chwthewke Mar 15, 2024

joroKr21 Mar 15, 2024

chwthewke commented Mar 16, 2024 •

edited

Loading

joroKr21 Mar 16, 2024

chwthewke Mar 16, 2024

joroKr21 Mar 16, 2024

joroKr21 commented Mar 16, 2024 •

edited

Loading

chwthewke commented Mar 16, 2024

joroKr21 commented Mar 16, 2024

chwthewke commented Mar 17, 2024

joroKr21 commented Mar 17, 2024

satorg left a comment

fix ambiguous ContravariantShow[SortedSet[A]] (& SortedMap) #4575

fix ambiguous ContravariantShow[SortedSet[A]] (& SortedMap) #4575

Conversation

chwthewke commented Mar 15, 2024

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

chwthewke commented Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joroKr21 commented Mar 16, 2024 • edited Loading

chwthewke commented Mar 16, 2024

joroKr21 commented Mar 16, 2024

chwthewke commented Mar 17, 2024

joroKr21 commented Mar 17, 2024

satorg left a comment

Choose a reason for hiding this comment

chwthewke commented Mar 16, 2024 •

edited

Loading

joroKr21 commented Mar 16, 2024 •

edited

Loading