Don't allow wildcard types in constraints #12703
Conversation
| * The idea is that under either of these conditions we are not interested | ||
| * in creating a fresh type variable to replace the wildcard. I verified |
There was a problem hiding this comment.
It's not clear to me why useNecessaryEither is treated specially here, is there a good justification or test case that illustrates this? Also in practice we also call necessaryEither when inferring GADTs:
https://github.com/lampepfl/dotty/blob/a82af21ff48ea07dbae042239286e5d80ef0e92e/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L1564
Should that condition also appear here? /cc @abgruszecki
There was a problem hiding this comment.
Yes, it probably should. Perhaps no GADT tests failed, because the interaction of GADTs and wildcards is undertested.
As a side remark, I'm not sure when exactly we should consider useNecessaryEither in separation from the GADT mode. The name doesn't help either in reminding that we should do both. Perhaps we should hide this condition behind a definition and rename useNecessaryEither?
There was a problem hiding this comment.
I believe 12677 is the test case that fails otherwise.
There was a problem hiding this comment.
Not sure about GADT inference. Does it even come up?
There was a problem hiding this comment.
I verified that all tests pass with or without the added condition ctx.mode.is(Mode.GadtConstraintInference). Someone else could go to the bottom of this in a separate PR.
| @@ -34,6 +34,9 @@ object Config { | |||
| */ | |||
| inline val checkConstraintsPropagated = false | |||
|
|
|||
| /** Check that constraint bounds do not contain wildcard types */ | |||
| inline val checkNoWildcardsInConstraint = true | |||
There was a problem hiding this comment.
I assume this should be set to false before merging?
There was a problem hiding this comment.
yes, it's false in latest version
| @@ -46,7 +49,7 @@ object Config { | |||
| * compiling all of dotty together (source seems to be in GenBCode which | |||
| * accesses javac's settings.) | |||
| * | |||
| * It is recommended to turn this option on only when chasing down | |||
| * It is recommended to turn this option on only w zhen chasing down | |||
There was a problem hiding this comment.
| * It is recommended to turn this option on only w zhen chasing down | |
| * It is recommended to turn this option on only when chasing down |
Wildcards in constraints essentially mean that the constrained variable is a sub/supertype of _every_ instance of the bound. For useNecesaryEither, this is wrong. Approximate the bound not to use wildcards instead.
Ther semantics is fishy. Instead, use avoidance in necessarySubType and in TypeVarsMiss context mode, and use fresh type variables elsehwere.
There was a problem hiding this comment.
LGTM, I'll let @abgruszecki figure out if he can make a gadt-version of i12677.scala :).
|
Causes regression #19955 |
Fixes #12677