Make Id covariant #3264

travisbrown · 2020-01-21T15:57:44Z

Making Id covariant was discussed briefly in 2015:

Should Id be covariant? I ask mainly because you say,

We can freely treat values of type A as values of type Id[A], and vice-versa.

which is only strictly true if it is.

As far as I know this was never followed up on, but I just ran into an issue while trying to cross-build the Cats tests on Dotty where implicit conversions for Id weren't being found because it's invariant. Making it covariant would fix this problem (and also just generally seems like the right thing to do, even if the Dotty team decides that the issue is a bug and fixes it on the Dotty side).

Unfortunately if we make Id covariant we hit a weird Scala 2 bug when defining instances for it, which minimized looks like this:

scala> trait Tc[F[_]]
defined trait Tc

scala> object X {
     |   type Id[+A] = A
     |   val x: Tc[Id] = new Tc[Id] {}
     | }
<console>:15: error: covariant type Id occurs in invariant position in type => Tc[X.Id] of value x
         val x: Tc[Id] = new Tc[Id] {}
             ^

And it gets even weirder:

scala> val x: Tc[Id] = new Tc[Id] {}
x: Tc[Id] = $anon$1@673c4f6e

scala> object X { type Id[+A] = A; new Tc[Id] {} }
defined object X

There's some explanation by @smarter in a conversation from this afternoon on Gitter.

Update: @smarter also has a fix for the issue.

We can work around this by changing the Id instances to λ[A => A]. ~~We could also use ({ type L[+A] => A })#L or the weird λ[`+A` => A] thing but λ[A => A] seems to work fine.~~ Update: Scala 2 doesn't care if the variance doesn't match, but Dotty does, so I've gone with ({ type L[+A] => A })#L for now.

No other changes are necessary, and we can verify that these instances are available without imports:

scala> cats.Monad[cats.Id]
res0: cats.Monad[cats.Id] = cats.package$$anon$1@9a00a50

scala> cats.Parallel[cats.Id, cats.Id]
res1: cats.Parallel.Aux[cats.Id,cats.Id] = cats.Parallel$$anon$2@12376480

scala> cats.Parallel[cats.Id]
res2: cats.Parallel.Aux[cats.Id,cats.package.catsParallelForId.F] = cats.Parallel$$anon$2@12376480

In the longer run, we're going to come up against another change in Dotty (this one definitely not a bug) that means that these type class instances for Id won't be available in implicit scope—they'll have to be imported explicitly.

There are a few ways we could address this. The first would be to do nothing and expect users to import cats._ if they need these instances. We could also create a new cats.instances.id that they could be imported from more idiomatically (also making them available in cats.instances.all and cats.implicits).

In my view the best approach would be to provide these instances in the type class companion objects, which would ensure that they're in implicit scope on both Scala 2 and Dotty. This is something I've already done in #3043, but we could do it separately if necessary.

…otty

codecov-io · 2020-01-21T16:19:23Z

Codecov Report

Merging #3264 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3264   +/-   ##
=======================================
  Coverage   93.08%   93.08%           
=======================================
  Files         376      376           
  Lines        7428     7428           
  Branches      196      196           
=======================================
  Hits         6914     6914           
  Misses        514      514

Flag	Coverage Δ
#scala_version_212	`93.41% <100%> (ø)`	⬆️
#scala_version_213	`92.85% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/package.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4c994a...540ea50. Read the comment docs.

The variance of a type alias was taken to be the variance of its right-hand side, but that doesn't make sense for a parameterized type alias which is free to appear in any position: variance checking should only kick in when it is applied to something. It was possible to work around this by using a type lambda instead of a type alias, cats just had to do that: typelevel/cats#3264

smarter · 2020-01-21T16:31:37Z

Unfortunately if we make Id covariant we hit a weird Scala 2 bug when defining instances for it

PR: scala/scala#8651

The variance of a type alias was taken to be the variance of its right-hand side, but that doesn't make sense for a parameterized type alias which is free to appear in any position: variance checking should only kick in when it is applied to something. It was possible to work around this by using a type lambda instead of a type alias, cats just had to do that: typelevel/cats#3264

dwijnand · 2020-01-21T16:51:21Z

Does it keep working if you type alias { type L[+x] = x }? Just to kill the repetition.

Welcome to Scala 2.13.1 (OpenJDK 64-Bit Server VM, Java 11.0.1).
Type in expressions for evaluation. Or try :help.

scala> trait Monad[M[_]]
defined trait Monad

scala> type Foo = { type L[+x] = x }
defined type alias Foo

scala> new Monad[Foo#L] {}
res2: Monad[[+x]x] = $anon$1@589d831e

travisbrown · 2020-01-21T16:57:32Z

@dwijnand Yes, thanks, I guess that's better even if this is just a temporary workaround. I've pushed an update with a private type alias for { type L[+A] = A }.

kailuowang · 2020-01-23T15:44:30Z

core/src/main/scala/cats/package.scala

-  type Id[A] = A
+  type Id[+A] = A
+
+  // Workaround for a compiler bug that should be fixed soon.


now that the PR on scala is in, can we refer to it here?
scala/scala#8651

@kailuowang Yes, good idea (I linked it in the PR description but the comment would be more helpful).

travisbrown · 2020-01-27T06:20:04Z

@LukaJCB To follow up on your question on Gitter yesterday (about whether users will also have to use the type lambda workaround), there's a type class instance for Id in IdSuite that shows that you don't need the workaround there.

smarter · 2020-01-30T09:56:33Z

There's an open PR (scala/scala3#8082) against Dotty which will allow it to infer the variance of the parameters of a type alias / type lambda based on its structure:

type T[X] >: A              // X is invariant
type T[X] <: List[X]        // X is invariant
type T[X] = List[X]         // X is covariant (determined structurally)
opaque type T[X] = List[X]  // X is invariant
opaque type T[+X] = List[X] // X is covariant
type T[A, B] = A => B       // A is contravariant, B is covariant (determined structurally)
type T[A, +B] = A => B      // A is invariant, B is covariant

If that gets in, this PR shouldn't be necessary anymore.

travisbrown · 2020-01-30T11:22:30Z

@smarter Is your sense that we can backport this story to Scala 2 in this case, since it's the way things happen to work anyway?

smarter · 2020-01-30T11:38:20Z

Well if the current way of doing things works well enough with scalac in practice, it's probably not worth changing

neko-kai · 2020-02-05T11:00:56Z

@travisbrown

In my view the best approach would be to provide these instances in the type class companion objects, which would ensure that they're in implicit scope on both Scala 2 and Dotty. This is something I've already done in #3043, but we could do it separately if necessary.

Another way would be to alias Id to an Id in an object. Package prefixes aren't part of implicit scope in Dotty, but object prefixes still are:

package object cats {
  type Id[+A] = data.id.Id[A]
}
package data {
  object id {
    type Id[+A] = A
    implicit lazy val instances = ...
  }
}

Although I guess there's no harm in putting the instances into type class companions, if the other stdlib types are there already and work fine

djspiewak · 2020-02-05T17:25:30Z

Package prefixes aren't part of implicit scope, but object prefixes are

I'm assuming you mean on Dotty? Package prefixes are most definitely part of the scope now. #2276 demonstrates how insidious this can be, and also seems quite related to this effort.

travisbrown · 2020-02-25T21:31:05Z

Putting this on hold since it's no longer necessary on either Dotty or Scala 2. If someone wants to argue that it's more correct, it might be better to take that up once the Scala 2 fix is available in a release.

Make Id covariant

f5913dd

travisbrown requested review from djspiewak, milessabin and LukaJCB January 21, 2020 15:57

Use non-kind-projector type lambda syntax to get variance right for D…

07ea952

…otty

smarter mentioned this pull request Jan 21, 2020

Fix variance handling for parameterized type aliases scala/scala#8651

Merged

This comment has been minimized.

Sign in to view

Simplify

c68351f

kailuowang reviewed Jan 23, 2020

View reviewed changes

Add link to Scala PR with variance fix for Id

540ea50

travisbrown added this to the 2.2.0-M1 milestone Jan 30, 2020

travisbrown added the enhancement label Jan 30, 2020

travisbrown removed this from the 2.2.0-M1 milestone Feb 20, 2020

travisbrown closed this Feb 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Id covariant #3264

Make Id covariant #3264

travisbrown commented Jan 21, 2020 •

edited

Loading

codecov-io commented Jan 21, 2020 •

edited

Loading

smarter commented Jan 21, 2020

dwijnand commented Jan 21, 2020

This comment has been minimized.

This comment has been minimized.

travisbrown commented Jan 21, 2020

kailuowang Jan 23, 2020

travisbrown Jan 23, 2020

travisbrown commented Jan 27, 2020

smarter commented Jan 30, 2020 •

edited

Loading

travisbrown commented Jan 30, 2020

smarter commented Jan 30, 2020

neko-kai commented Feb 5, 2020 •

edited

Loading

djspiewak commented Feb 5, 2020

travisbrown commented Feb 25, 2020

Make Id covariant #3264

Make Id covariant #3264

Conversation

travisbrown commented Jan 21, 2020 • edited Loading

codecov-io commented Jan 21, 2020 • edited Loading

Codecov Report

smarter commented Jan 21, 2020

dwijnand commented Jan 21, 2020

This comment has been minimized.

This comment has been minimized.

travisbrown commented Jan 21, 2020

kailuowang Jan 23, 2020

Choose a reason for hiding this comment

travisbrown Jan 23, 2020

Choose a reason for hiding this comment

travisbrown commented Jan 27, 2020

smarter commented Jan 30, 2020 • edited Loading

travisbrown commented Jan 30, 2020

smarter commented Jan 30, 2020

neko-kai commented Feb 5, 2020 • edited Loading

djspiewak commented Feb 5, 2020

travisbrown commented Feb 25, 2020

travisbrown commented Jan 21, 2020 •

edited

Loading

codecov-io commented Jan 21, 2020 •

edited

Loading

smarter commented Jan 30, 2020 •

edited

Loading

neko-kai commented Feb 5, 2020 •

edited

Loading