-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Show inherit from a contravariant base trait #1649
Conversation
import scala.collection.immutable.BitSet | ||
import cats.Show | ||
|
||
trait BitSetInstances extends cats.kernel.instances.BitSetInstances { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two other instances from kernel
provided in cats.implicits
through this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh whoops. I'll add that back in.
17bf536
to
20add21
Compare
Codecov Report
@@ Coverage Diff @@
## master #1649 +/- ##
=======================================
Coverage 94.17% 94.17%
=======================================
Files 256 256
Lines 4208 4208
Branches 93 93
=======================================
Hits 3963 3963
Misses 245 245
Continue to review full report at Codecov.
|
|
||
test("show BitSet"){ | ||
BitSet(1, 1, 2, 3).show should === ("BitSet(1, 2, 3)") | ||
BitSet.empty.show should === ("BitSet()") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the new result will be "Set(1, 2, 3)" instead of "BitSet(1, 2, 3)" right?
I'm -1 on this change. I'd rather add a |
I don't think Bitset opting into something can solve anything. The issue is a continuous stream of variations on this theme: scala> def f[A: Show](x: Some[A]) = show"$x"
<console>:17: error: type mismatch;
found : Some[A]
required: cats.Show.Shown
def f[A: Show](x: Some[A]) = show"$x"
^
scala> def f[A: Show](x: Some[A]) = show"${ x: Option[A] }"
f: [A](x: Some[A])(implicit evidence$1: cats.Show[A])String Has anyone used the cats Show more than in a trivial test? I would like to see how they're managing that satisfactorily. |
I have used it more than trivially. So, I think that variance is generally frowned on in the typeclasses in cats. For instance, Order or Eq could be contravariant, but they are not. The example that @paulp gives is a pretty well known one and generally solved by not writing methods that accept subtypes of an ADT, or by writing an explicit type: I don't actually know all the objections, but @non was telling me once how contravariant typeclasses in particular had inference issues, so I am just spreading fear. That said, if we do this, why wouldn't we add variance all over the place in cats? I thought the idea was to use By the way, this is why cats ADTs generally give constructor methods that return the outer type. For instance in the old days |
One hesitates to ever provide examples because inevitably one hears things like "not writing methods that accept subtypes of an ADT". That's not how it arises. That's how it's demonstrated in a github issue in an easy-to-reproduce way. It's usually the type of a pattern, and you can't even use a type ascription to upcast a nested binding because the pattern matcher will unify the upcast with the stronger type and use the stronger type.
|
@johnynek Can you point me to the more-than-trivial usage? It wasn't a rhetorical device, I would really like to see in practice. Maybe my objections would evaporate. |
@paulp I can't, it is closed source. Can you replace |
If this seems useful enough to you then I guess we have established where the difference of opinion lies. scala> implicitly[Order[List[Int]]]
res7: cats.Order[List[Int]] = cats.kernel.instances.ListOrder@c02a72a
scala> implicitly[Order[Seq[Int]]]
<console>:18: error: could not find implicit value for parameter e: cats.Order[Seq[Int]]
implicitly[Order[Seq[Int]]]
^
scala> implicitly[Order[Vector[Int]]]
res9: cats.Order[Vector[Int]] = cats.kernel.instances.VectorOrder@17d7cf3e
scala> implicitly[Order[scala.collection.immutable.LinearSeq[Int]]]
<console>:18: error: could not find implicit value for parameter e: cats.Order[scala.collection.immutable.LinearSeq[Int]]
implicitly[Order[scala.collection.immutable.LinearSeq[Int]]]
^ So basically there is an Order if someone has decided it's a popular enough subtype of Seq. As user experiences go I think it leaves something to be desired. |
So, my first thought is that if we're going to do this for In the past, we've avoided doing this due to the (IMO buggy) way contravariance and specificity interact. See SI-2509 and its related links for examples of why this can be a problem. I know @paulp had a language change that fixed these issues, but it didn't make it into the language. Here's a restatement of the issue from the ticket: package contra
class Dog
class Puppy extends Dog
trait Observer[-A] {
def observe(a: A): String
}
object Observer {
implicit val dogObserver: Observer[Dog] =
new Observer[Dog] { def observe(d: Dog) = "saw a dog" }
implicit val puppyObserver: Observer[Puppy] =
new Observer[Puppy] { def observe(p: Puppy) = "saw a puppy" }
}
object Test {
implicit class Observed[A](a: A)(implicit o: Observer[A]) {
def observed: String = o.observe(a)
}
val dog = new Dog {}
val pup = new Puppy {}
def main(args: Array[String]): Unit = {
println(Observer.dogObserver.observe(dog))
println(Observer.puppyObserver.observe(pup))
println(dog.observed)
println(pup.observed)
println(implicitly[Observer[Dog]].observe(dog))
println(implicitly[Observer[Puppy]].observe(pup))
}
// output:
//saw a dog
//saw a puppy
//saw a dog
//saw a dog
//saw a dog
//saw a dog
} |
One side-effect of this is that if |
No, you restructure things. Using companion scope does make it impossible to use contravariance, but if the implicits are in the package object then they can be masked by name when desired. Stacking traits lets you specify specificity rather than letting the compiler make choices. |
There are a few more arrows in the quiver as well, which maybe could be put to good effect even if you don't want to introduce contravariance. See the last method at the above link:
In other words, you can in principle print an |
OK, right, I do agree that if we changed the way people are expected to use implicits we could mitigate these problems. It's a bigger task, but not impossible. I personally would like to solve the issues you raise around
I know in the past folks have pushed back against supporting the collection interfaces which support possibly mutable collections (e.g. What do you think @paulp? I'm happy to try to put together a PR that demonstrates how to cover the highlights of the collections library. Does this seem like a fool's errand? |
All non-final collections are possibly mutable (as are some of the final and supposedly immutable ones, like List.) Some just invite it more via their interfaces than others. It's easy to make a mutable instance of However, Seq too was just an example - I figured it has to rub people the wrong way to have a finite list of manually created instances all doing the same thing. You get both code duplication and arbitrarily incomplete coverage of the problem space. If you find a way to make Seq and friends work uniformly which leaves you with less code instead of more, that'll be an improvement but not so much a solution from where I sit, because every ADT offers a problem isomorphic to the Option/Some issue. Independently of contravariant Show, I think there's a lot to be said for not using companion scope in a library. People can do that at the leaves because they can change it when there's an issue. But when you ship things in companion scope, people are stuck. |
I don't know how, when or why SI-2509 was assigned to me, but it definitely looks like it would be a good candidate for a fix in Typelevel Scala ... would there be any appetite for it if it happened? |
Hmm ... apparently I reopened it and assigned it to myself about a year ago ... |
The user issue here, from my perspective, is the |
@edmundnoble Do you think you could bridge the gap with contravariance in the interpolator (or implicits leading up to it) instead of the type class? @milessabin Yes! Although it's a language change, so we'd definitely want to advertise it, hide it behind |
@edmundnoble Yes, I should have specified that it was that battle which led psp-std to where it is today. Finding something like |
@erik-stripe I tried that first; that's most of my point. It's not possible with implicit search working the way it does, and in general it doesn't work for any ops class. @milessabin Pretty pretty please 👍 👍 |
I think that I'm 👎 on this, at least with contravariant implicit resolution being what it is in scala right now. My experience with variance (even covariance) in type classes has overwhelmingly been negative. As @edmundnoble said, there's certainly a tradeoff. But as I see it, the two main things that you gain are:
These two conveniences seem to come at a pretty high complexity cost. If you control all of the relevant instances, then you can use stack trait hierarchies to order them, but doesn't that model break down if some instances are coming from different places? Also one could argue that the second benefit listed above can at times be an anti-feature, as you may not necessarily want |
@ceedubs Two notes:
My primary point is this: the |
I guess I should also point out that if Show is contravariant, you can easily have it both ways. The reverse is not true.
Also, it bears repeating that you CAN reconcile matters even "if some instances are coming from different places" regardless of the implicit selection algorithm, because the names are in scope and you can manage them by name.
which though baroque because that's all scala gives us is a better situation than you have when the library has consumed the whole companion scope and you can't influence that at all. |
Oh yes, here's a real life example of using an invariant type alias on a contravariant type to make implicit resolution succeed where it would otherwise fail. /** Scala, so aggravating.
* [error] could not find implicit value for parameter equiv: Eq[A => Bool]
* The parameter can be given explicitly, it just won't be found unless the
* function type is invariant. The same issue arises with intensional sets.
*/
type InvariantPredicate[A] = A => Bool |
Despite the fact that you can reconcile However; I am having second thoughts about cats providing any type class instances in companion objects because of the modularity problem you mentioned. Why even offer a la carte imports for some instances if the user can't control instances for datatypes from cats itself? Edit: Can we not, then, use |
I find it much less annoying than the alternatives, personally. You do it once, in your package object, and you get the same semantics everywhere in the package. You don't have to do a la carte import babysitting on a file by file basis. And there's no reason to think people wouldn't mostly be able to use the defaults anyway. Also, I put all the implicits in a trait which can itself be mixed into a package object. Then whatever you put in your package object has the upper hand from the get-go. You can maybe make the interpolator work with a private |
My motivation for making Also, when it comes to changing cats' import scheme, I recommend opening a new issue to make a more focused case that this can and should work with cats, as well as the API changes that would be required and the use-cases allowed and disallowed by it, seeing as it's not strictly necessary if we go with this |
732d4d5
to
e723099
Compare
So now, this PR does two things.
This makes the |
it's no longer API breaking right? |
@@ -27,7 +32,7 @@ object Show { | |||
|
|||
final case class Shown(override val toString: String) extends AnyVal | |||
object Shown { | |||
implicit def mat[A](x: A)(implicit z: Show[A]): Shown = Shown(z show x) | |||
implicit def mat[A](x: A)(implicit z: ContravariantShow[A]): Shown = Shown(z show x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can get the test to pass by asking for Show[_ >: A]
which is effectively what this trick is doing, and not having to introduce the new type. I think we can (at least on 2.12).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately type inference looks to be just slightly shy, like it was when I asked for [A, B >: A](x: A)(implicit z: Show[B])
: it never converts anything to a Shown
.
[error] /Users/edmund/workspace/scala/cats/tests/src/test/scala/cats/tests/ShowTests.scala:29: type mismatch;
[error] found : TimeOfDay
[error] required: cats.Show.Shown
[error] assertResult("Good morning, Whiskers!")(show"Good $tod, $cat!")
[error] ^
[error] /Users/edmund/workspace/scala/cats/tests/src/test/scala/cats/tests/ShowTests.scala:29: type mismatch;
[error] found : Cat
[error] required: cats.Show.Shown
[error] assertResult("Good morning, Whiskers!")(show"Good $tod, $cat!")
[error] ^
[error] /Users/edmund/workspace/scala/cats/tests/src/test/scala/cats/tests/ShowTests.scala:31: type mismatch;
[error] found : TimeOfDay
[error] required: cats.Show.Shown
[error] assertResult("Good morning, Whiskers!")(show"Good $tod, ${List(cat).head}!")
[error] ^
[error] /Users/edmund/workspace/scala/cats/tests/src/test/scala/cats/tests/ShowTests.scala:31: type mismatch;
[error] found : Cat
[error] required: cats.Show.Shown
[error] assertResult("Good morning, Whiskers!")(show"Good $tod, ${List(cat).head}!")
[error] ^
[error] /Users/edmund/workspace/scala/cats/tests/src/test/scala/cats/tests/ShowTests.scala:46: type mismatch;
[error] found : Cat
[error] required: cats.Show.Shown
[error] assertResult("Good morning, Whiskers!")(show"Good morning, $cat!")
[error] ^
[error] 5 errors found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I did a simple test in the repl this worked when I did implicitly[Foo[_ >: A]]
did you try that form (not putting a specific type)? you never know with scala which minor translation of the code will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An implicit in the repl is like you've imported it. His is in the Shown companion object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. That clarifies it for me.
@kailuowang I believe it is not API breaking any longer. |
I'm on board. This is an interesting trick to put in the bag of scalahacks. 👍 |
e723099
to
59822c1
Compare
merged with 3 sign-offs |
Contravariance in type class parameters is well-documented to be an issue for the compiler. Specifically, the compiler has a useless version of "specificity" which forces the least specific instance to be used, if there are overlapping instances and the user does not use an explicit inheritance-based prioritization.
However, the benefit is still there: firstly there is no need for a
Show[BitSet]
, because the existingShow[Set[Int]]
is narrowed implicitly. In the same vein,Some(1).show
works even if your instance only exists forOption[Int]
and notSome[Int]
. These benefits become more apparent when you construct values from inside ashow
interpolator, and need to use type ascriptions with the existing version ofShow
.For examples of how users can make overlapping
Show
work, see https://github.com/paulp/psp-std/blob/master/std/src/main/scala/std/Show.scala.This PR breaks the API in the following ways:
a) Not only was the BitSet show instance no longer necessary, but it actually began to conflict with the Set[Int] instance.
b) Implicit specificity no longer functions properly. Specifically, users cannot have a
Show[Supertype]
andShow[Subtype]
and expect thatimplicitly[Show[Subtype]]
will materialize the latter.To be clear this is a trade-off. I am not sure how, practically, user usage of Show benefits and is hurt by this change.