-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve subtyping check for not yet eta-expanded higher kinded types #17139
Improve subtyping check for not yet eta-expanded higher kinded types #17139
Conversation
The CI is failing because of some infrastructural problems #17119 (comment) but the compilation tests passed |
9b92d10
to
03a8f64
Compare
@odersky the build is green now. Could you have a look? |
@@ -580,7 +580,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling | |||
val base = nonExprBaseType(tp1, cls2) | |||
if (base.typeSymbol == cls2) return true | |||
} | |||
else if tp1.isLambdaSub && !tp1.isAnyKind then | |||
else if tp1.typeParams.nonEmpty && !tp1.isAnyKind then |
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.
Not sure about this, this could be quite a drastic change that needs more motivation. Can you push to staging, then I can try some things?
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.
@odersky pushed.
The problem I'm trying to fix here is that when the compiler tries to compare two higher-kinded types like Foo1
and Foo2
in the test example, it returns false
too early without trying to eta-expand the types. In the other cases from the linked issue, where everything compiles successfully, for some reason at least one of the compared types gets already eta-expanded at some earlier stage, which leads the compiler in the right direction to prove that the subtyping relation does occur.
To be more precise, the 2 alternative working workarounds are:
- remove the theoretically redundant package prefix, changing
implicit val bar: Bar[pkg.Foo2]
into
implicit val bar: Bar[Foo2]
- compile both files in a single compiler run (e.g. by renaming
Test_2.scala
toTest_1.scala
when compiling with vulpix)
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.
@odersky have you managed to experiment with these changes? If there any chance to push this forward?
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.
This does avoid the fourthTry logic for the cases where tp1 is a generic class without type parameters. But I checked that this logic does not do anything in this case, so I think the change is OK.
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.
After more review, I agree this is the right change.
Fixes #16183