Skip to content
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

Merged
merged 4 commits into from
Jun 1, 2020
Merged

Add auto variance as additional import #3436

merged 4 commits into from
Jun 1, 2020

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented May 27, 2020

Fixes #3434

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #3436 into master will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            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     

@LukaJCB LukaJCB marked this pull request as ready for review May 29, 2020 00:30
djspiewak
djspiewak previously approved these changes May 29, 2020
@rossabaker
Copy link
Member

Is making it a separate import just conservatism toward what could be a sweeping change, or have we identified actual negatives?

@rossabaker
Copy link
Member

Ah, never mind, I see discussion of that on #3434 now.

rossabaker
rossabaker previously approved these changes May 31, 2020
@LukaJCB LukaJCB requested a review from travisbrown May 31, 2020 03:29
Copy link
Contributor

@travisbrown travisbrown left a 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.

core/src/main/scala/cats/AutoVariance.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/AutoVariance.scala Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@LukaJCB LukaJCB dismissed stale reviews from rossabaker and djspiewak via 3eb7a76 May 31, 2020 20:00
@LukaJCB LukaJCB requested a review from travisbrown May 31, 2020 20:00
@LukaJCB
Copy link
Member Author

LukaJCB commented May 31, 2020

@travisbrown I think you make some good points, I changed it to more closely match syntax and instances, but didn't mix it into implicits just yet.

travisbrown
travisbrown previously approved these changes May 31, 2020
Copy link
Contributor

@travisbrown travisbrown left a 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).

@LukaJCB
Copy link
Member Author

LukaJCB commented May 31, 2020

I'm conflicted about that, I think I'd like to see them in implicits long term, but I have no idea what kind of unintended consequences they might cause. So I'm inclined to have them in their own import for the moment and over time we can let the community decide if they should go into implicits.

@LukaJCB
Copy link
Member Author

LukaJCB commented May 31, 2020

I worry that not enough people might use the milestones compared to who this change would end of impacting.

@travisbrown
Copy link
Contributor

Fair enough, sounds reasonable to me!

@LukaJCB LukaJCB merged commit 4b3d3ff into master Jun 1, 2020
@LukaJCB LukaJCB deleted the auto-variance branch June 1, 2020 01:20
@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 2, 2020
@smarter
Copy link
Contributor

smarter commented Jul 15, 2020

I'm conflicted about that, I think I'd like to see them in implicits long term, but I have no idea what kind of unintended consequences they might cause.

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 F[+_] and this widening business wouldn't be needed, but getting there might be hard or impossible (has anyone tried it?)

@djspiewak
Copy link
Member

In an ideal world I think Functor should take an F[+_] and this widening business wouldn't be needed, but getting there might be hard or impossible (has anyone tried it?)

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 F[+_]. Unfortunately, this turned out to be problematic in several ways. Three major ones:

  • Compiler bugs. These still remain to a significant degree, though many of them have been resolved, but at the time it was abjectly awful. Plenty of completely unsound things just… worked, and plenty of completely sound things didn't work unless you tapdanced on your head under a full moon.
  • Monad Transformers. Universally passing variance through monad transformers turns out to be quite difficult in general. I had an example years and years ago involving Monad and Hoist and WriterT (I think?) where I made the argument that variance is fundamentally antithetical to type constructor polymorphism in Scala. I think that argument was probably too strident, but it's definitely problematic.
  • Enforced complexity on users. If the the entire abstraction hierarchy uses explicit variance, then everyone who defines their own abstraction must take on the burden of getting variance right, and that is not a trivial thing by any stretch of the imagination. This also shows up in very subtle ways in user code (really, anything that involves closing over an instance of F[A] where F[+_]), and in practice, people really do not understand it and have a hard time getting it right. This complexity becomes optional and is under the control of the user if the abstractions stick with the polyvariant F[_].

There's also a fourth minor problem, which is that you can't express Const.

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 F[_], giving users maximal control. The abstractions can be polyvariant to the degree that they don't care about it, and then users can choose to instantiate those abstractions with concrete forms that are more specific. In cases where variance is required to unify subtype constraints, I would like users to be able to carry along an implicit proof that their concrete instantiation is soundly variant, and then that proof is applied. This is a vastly more flexible system than the bluntness of F[+_].

And, as it turns out, such a system is directly isomorphic to Functor itself. If you have a Functor[F], then you're saying that F is soundly variant, whether it is declared as such or not. In other words, Functor is the implicit variance evidence, provided that F[_] is polyvariant (which, at present, it is). This PR is one stab at attempting to automatically apply that evidence.

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 F[+_]/F[-_] are good tools, fundamentally. They're overconstraining in weird ways. They don't interact well with the rest of the type specificity system (though much of that is fixed in Dotty). They are incredibly verbose and they force users to deal with a massive amount of complexity unless they a) keep everything religiously first-order without any abstraction (the ZIO approach), and b) never define their own data structures which need to participate in the abstraction hierarchy.

I would very much like it if the language itself exposed some kind of mechanism (which Functor could participate in) which allowed for a more first-class version of what we're trying to achieve in this PR, without resorting to general implicit coercion. Such a mechanism would have fewer caveats than the one exposed here, while maintaining the flexibility and generality of the polyvariant abstraction approach.

@smarter
Copy link
Contributor

smarter commented Jul 15, 2020

Thanks for the detailed response!

I had an example years and years ago involving Monad and Hoist and WriterT (I think?) where I made the argument that variance is fundamentally antithetical to type constructor polymorphism in Scala. I think that argument was probably too strident, but it's definitely problematic.

That might be worth digging up, if only for historical purposes :).

And, as it turns out, such a system is directly isomorphic to Functor itself. If you have a Functor[F], then you're saying that F is soundly variant, whether it is declared as such or not.

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?

I would very much like it if the language itself exposed some kind of mechanism (which Functor could participate in) which allowed for a more first-class version of what we're trying to achieve in this PR, without resorting to general implicit coercion.

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.

@djspiewak
Copy link
Member

That might be worth digging up, if only for historical purposes :).

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 Eq is very bad, but I can't remember the essence of it. That would probably also be worth digging up, particularly since any complete functor hierarchy involving variance is going to need to have a contravariant Eq as well.

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?

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 A is phantom. For example:

class Upper
class Lower extends Upper

sealed trait Foo[A]
case object Bar extends Foo[Lower]

You certainly can't define Functor for Foo, but it seems somewhat valid to define Co[Foo], since any instance of Foo[Lower] can be safely widened to Foo[Upper].

Which is to say, I think Functor is actually a bit stronger than variance.

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.

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 A is substitutable for type B in this scope". Basically, ad hoc subtyping. I'll give some thought to this and jump into the contributors thread.

@smarter
Copy link
Contributor

smarter commented Jul 16, 2020

trait Co[F[_]]

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 F[A] <: F[B] to succeed if there's an instance of Co[F] and A <: B, but for unlawful instances that sounds like a bad idea). Functor obeys that law since it obeys fa.map(x => x) = fa, here's some other lawful instances:

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
tricky examples from https://failex.blogspot.com/2016/09/the-missing-diamond-of-scala-variance.html:

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[_[+_]] ?

@djspiewak
Copy link
Member

fa.widen = fa

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.

but for unlawful instances that sounds like a bad idea

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 F[_], those restrictions are arguably a paper door (especially with pattern types). Then again, if someone's going to define something weird, it's basically like doing an asInstanceOf (but requiring a lot more effort), and that's probably outside the realm of hand-holding.

Also fun is to consider how this could be generalized to higher-kinds:

Exactly this! You basically get a supercharged Liskov.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add auto-widen/auto-narrow implicits
7 participants