-
-
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
Add auto variance as additional import #3436
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3436 +/- ##
==========================================
+ Coverage 91.64% 91.71% +0.07%
==========================================
Files 381 382 +1
Lines 8268 10961 +2693
Branches 225 265 +40
==========================================
+ Hits 7577 10053 +2476
- Misses 691 908 +217 |
Is making it a separate import just conservatism toward what could be a sweeping change, or have we identified actual negatives? |
Ah, never mind, I see discussion of that on #3434 now. |
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 like this, and it would definitely help some common Circe usage.
I'm not 100% sure about the naming. The current version means everyone who imports cats.implicits._
suddenly has this variance
definition in scope, which isn't a big deal, I guess, but also doesn't seem ideal. I'm also not sure about AutoVariance
, which doesn't really make it clear what's happening automatically. Maybe VarianceConversions
would be better (if the trait needs to be in the public API at all)?
I think my preference would be to start a new cats.conversions
package and put this stuff there, and also to have it included in cats.implicits
. Then there's still just one kitchen-sink implicits import for people who want the full Cats experience, and people who want to be more careful about compile times have a uniform experience across cats.{ instances, syntax, conversions }
.
I don't want to slow this down too much or get too bike-sheddy, but I do think we should be careful when adding things to the public API.
@travisbrown I think you make some good points, I changed it to more closely match |
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.
👍, changes look good to me!
Do you think we should keep these conversions out of implicits
in the long term? I don't have a strong opinion, but think I would be inclined to go ahead and include them in in a 2.2.0 milestone, to help with discoverability, and then if it causes issues we could remove it in 2.2.0 (if necessary we could even "remove" the conversions later, by making them non-implicit).
I'm conflicted about that, I think I'd like to see them in |
I worry that not enough people might use the milestones compared to who this change would end of impacting. |
Fair enough, sounds reasonable to me! |
Besides the performance concerns, the future of implicit conversion in general is uncertain, see https://contributors.scala-lang.org/t/can-we-wean-scala-off-implicit-conversions/4388 where you might want to present your usecase since it's an interesting one. In an ideal world I think Functor should take an |
Yes. It's been tried several times. A brief history of prior art here… Scalaz originally used variance in its type constructors. Prior to Scalaz 7, everything was kind of all over the map, but Scalaz 7.0 was quite uniform (thanks to Jason's code generation), and everything in the functor hierarchy used
There's also a fourth minor problem, which is that you can't express All three of these factors are still relevant, though the first one is reduced significantly in 2.13, and all-but eliminated in Dotty. Anyway, it was all too much. The largest change in Scalaz 7.1 was the removal of all of the variance, which led us to the standard encoding of the functor hierarchy we see today reflected in Cats. I ported a few scalaz-dependent codebases from 7.0 to 7.1, and I can tell you that the results were radically simpler and less verbose in all but one area: branching conditionals returning case class instances. Which is what the OP solves. Today, I believe https://github.com/7mind/izumi has a functor hierarchy with full variance, so it's not as if it isn't being tried. I remain exceptionally unconvinced, particularly when stuff like typelevel/cats-effect#849 keeps happening. Taking a step back… In theory, what I want is I want to abstract over the variance in And, as it turns out, such a system is directly isomorphic to As you pointed out, removing implicit conversions (which in general I actually think would be a good thing) throws a significant wrench into this, because it makes it harder or impossible to guide type inference through these kinds of unifications. But if the discussion is on the table… I don't really think that I would very much like it if the language itself exposed some kind of mechanism (which |
Thanks for the detailed response!
That might be worth digging up, if only for historical purposes :).
That's an interesting way to think about it, does it work in the other direction or is it possible to come up with a covariant F which is not mappable?
Doing that efficiently and without running into the arbitrary limitations of implicit conversions seems really hard to me, but it certainly seems worth considering and I encourage you to mention this in the contributors thread I linked to above where some dosict of what features would be needed to get rid of implicit conversions has already started. |
I'll see if I can reason my way back to it. I definitely don't have it written down anymore. I think it was on the Precog IRC back in like, 2010. Also as a historical note, I believe @paulp has an old argument where he lays out the view that contravariant
That's a very interesting question, and my "isomorphism" may have been too strong. More specifically, I wonder if we really have something like the following: trait Co[F[_]] {
def widen[A, B >: A](fa: F[A]): F[B]
}
trait Functor[F] extends Co[F] {
def map[A, B](fa: F[A])(f: A => B): F[B]
def widen[A, B >: A](fa: F[A]): F[B] = map(fa)(a => a: B)
}
trait Contra[F[_]] {
def narrow[A, B <: A](fa: F[A]): F[B]
}
// etc I suspect this would apply in many cases where class Upper
class Lower extends Upper
sealed trait Foo[A]
case object Bar extends Foo[Lower] You certainly can't define Which is to say, I think
Ultimately I think that implicit conversions is not a great mechanism to use for this. What I really want is a way of telling the compiler "type |
Yes, I had something like that in mind to. To nail down the meaning of covariance, I think we also need a law: fa.widen = fa (which brings up an interesting point, you want type Id[A] = A
implicit val coId: Co[Id] = new Co[Id] {
def widen[A, B >: A](fa: Id[A]): Id[B] = fa
}
final case class Phantom[A]()
implicit val coPhantom: Co[Phantom] = new Co[Phantom] {
def widen[A, B >: A](fa: Phantom[A]): Phantom[B] = Phantom()
} If the language actually took these instances into account when doing subtyping checks, we could start encoding some of the final case class Compose[F[_], G[_], A](run: F[G[A]])
implicit def coCompose1[F[_]: Co, G[_]: Co]: Co[[[X] => Compose[F, G, X]] = new Co[[[X] => Compose[F, G, X]] {
def widen[A, B >: A](fa: Compose[F, G, A]): Compose[F, G, B] =
Compose(fa.run) // A <: B, therefore G[A] <: G[B] by Co[G], therefore F[G[A]] <: F[G[B]] by Co[F]
}
// ... repeat for every combination of Co and Contra for F and G Also fun is to consider how this could be generalized to higher-kinds: trait Co2[F[_[_]]] { // F[+_[_]]
def widen[G[_], H[X] >: G[X]](fa: F[G]): F[H]
}
// What about F[_[+_]] ? |
I think it can actually be two laws (or one, if you feel like golfing): def widenIdentity[A](fa: F[A]) =
fa.widen[A] <-> fa
def widenCast[A, B >: A](fa: F[A]) =
fa.widen[B] <-> fa.asInstanceOf[F[B]] The second one is equivalent to the first by reflexivity of subtyping.
As the law above postulates, what we're doing here is effectively reifying casts in a typeclass, subject to some restrictions. Given that it's definable though for any
Exactly this! You basically get a supercharged |
Fixes #3434