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

Fix checkKindBoundsHK for higher kinds #9414

Merged
merged 1 commit into from Jan 7, 2021
Merged

Conversation

@joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Dec 24, 2020

We need to flip the direction of the checks on every other level.
This is similar to contravariance of higher-order function types.

Another option would be to heed the comment:

          // Their arguments use different symbols, but are
          // conceptually the same. Could also replace the types by
          // polytypes, but can't just strip the symbols, as ordering
          // is lost then.

although I don't understand the last sentence - what ordering?
If we switch to PolyTypes we would lose the detailed error messages,
but I think this is what Scala 3 does.

fixes scala/bug#2067, fixes scala/bug#12242
This would probably need a community build

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Dec 24, 2020
@joroKr21 joroKr21 marked this pull request as ready for review Dec 24, 2020
@lrytz
Copy link
Member

@lrytz lrytz commented Jan 5, 2021

probably need a community build

cc @SethTisue

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Jan 5, 2021

If we switch to PolyTypes we would lose the detailed error messages,
but I think this is what Scala 3 does

It seems maybe-implied by this remark, but just to confirm: does this PR bring Scala 2 and Scala 3 in closer alignment? (Should scala/bug#2067 be labeled fixed-in-dotty?)

@joroKr21
Copy link
Member Author

@joroKr21 joroKr21 commented Jan 5, 2021

It seems maybe-implied by this remark, but just to confirm: does this PR bring Scala 2 and Scala 3 in closer alignment? (Should scala/bug#2067 be labeled fixed-in-dotty?)

@SethTisue yes, it's fixed in dotty:

Starting scala3 REPL...
scala> trait A[B[D[X, Y <: X]]]
     | trait C[E[T, S]]
     | type Bad = A[C]
3 |type Bad = A[C]
  |             ^
  |         Type argument C does not conform to upper bound [D[X, Y]] =>> Any

scala> trait A[T[_[_]]]
     | trait C[X[+_]]
     | type Bad = A[C]
3 |type Bad = A[C]
  |             ^
  |        Type argument C does not conform to upper bound [_$1[_$2]] =>> Any

Note that the error messages are not very informative.
It's also evident that dotty is reusing type lambdas for kind conformance.

On this branch of scalac:

Welcome to Scala 2.13.5-20201224-113823-22fc021 (OpenJDK 64-Bit Server VM, Java 11.0.9).
Type in expressions for evaluation. Or try :help.

scala> trait A[B[D[X, Y <: X]]]
     | trait C[E[T, S]]
     | type Bad = A[C]
       type Bad = A[C]
                  ^
On line 3: error: kinds of the type arguments (C) do not conform to the expected kinds of the type parameters (type B) in trait A.
       C's type parameters do not match type B's expected parameters:
       type Y's bounds <: X are stricter than type S's declared bounds >: Nothing <: Any

scala> trait A[T[_[_]]]
     | trait C[X[+_]]
     | type Bad = A[C]
       type Bad = A[C]
                  ^
On line 3: error: kinds of the type arguments (C) do not conform to the expected kinds of the type parameters (type T) in trait A.
       C's type parameters do not match type T's expected parameters:
       type _ is invariant, but type _ is declared covariant

Also I just realised this fixes scala/bug#12242 as well (it has a bad kind):

  • dotty:
scala> trait Box[T]
     | trait Adapter[B <: Box[T], T]
     | trait Mixin[+A[B <: Box[S], X], S]
     | trait Super[T] extends Mixin[Adapter, T]
4 |trait Super[T] extends Mixin[Adapter, T]
  |                             ^
  |Type argument Adapter does not conform to upper bound [B <: Box[T], X] =>> Any
  • this branch:
scala> trait Box[T]
     | trait Adapter[B <: Box[T], T]
     | trait Mixin[+A[B <: Box[S], X], S]
     | trait Super[T] extends Mixin[Adapter, T]
       trait Super[T] extends Mixin[Adapter, T]
                              ^
On line 4: error: kinds of the type arguments (Adapter,T) do not conform to the expected kinds of the type parameters (type A,type S) in trait Mixin.
       Adapter's type parameters do not match type A's expected parameters:
       type B (in trait Adapter)'s bounds <: Box[T] are stricter than type B's declared bounds <: Box[S]

Do you think I should add one more test?

@joroKr21
Copy link
Member Author

@joroKr21 joroKr21 commented Jan 5, 2021

@joroKr21
Copy link
Member Author

@joroKr21 joroKr21 commented Jan 5, 2021

Do you think I should add one more test?

I just did

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Jan 6, 2021

The failure in the community build is in a known-flaky repo, so I'd call 2345 a green run.

This needs rebase before merge.

lrytz
lrytz approved these changes Jan 7, 2021
object Test1 {
trait A[B[D[X, Y <: X]]]
trait C[E[T, S]]
trait P[U, V <: U]
Copy link
Member

@lrytz lrytz Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused - remove this line?


// scala/bug#2067
object Test2 {
class P[Y](val y : Y)
Copy link
Member

@lrytz lrytz Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also unused

We need to flip the direction of the checks on every other level.
This is similar to contravariance of higher-order function types.
@lrytz lrytz merged commit 9bb659e into scala:2.13.x Jan 7, 2021
3 checks passed
@joroKr21 joroKr21 deleted the subkinding branch Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants