Fix #4032: Fix direction of instantiation. #4059
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Added" instead of "Add")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
|
Unclear from the description: Does this fix implement Scalac's behavior or just fixes the specific failure? In the latter case, this fix could cause regressions in other projects, right? |
Intrepolation is a pretty delicate scenario. It's difficult to even say what is right and what is wrong. In scala#4032 there's an unconstrained type variable that should be interpolated and it's a coin flip whether we instantiate it to the lower or upper bound. A completely unrelated change in scala#3981 meant that we instantiated the variable to the upper instead of the lower bound which caused the program to fail. The failure was because the type variable appeared covariantly in the lower bound of some other type variable. So maximizing the type first type variable overconstrained the second. We avoid this problem now by computing the variance of the type variable that's eliminated in the rest of the constraint. We take this into account to instantiate the variable so that it narrows the overall constraint the least, defaulting again to lower bound if there is not best direction. Since the test is expensive (linear in the constraint size), we do this only if the variable's lower bound is unconstrained. We could do it for all non-occurring type variables, but have to see how that would affect performance.
|
@Blaisorblade scalac's inference is too different to be able to answer that question. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
I changed the algorithm to be less arbitrary than what was in the initial PR. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4059/ to see the changes. Benchmarks is based on merging with master (7655503) |
| else { | ||
| val otherParam = tv.origin | ||
| val fs1 = if (constraint.isLess(tparam, otherParam)) fs &~ Covariant else fs | ||
| val fs2 = if (constraint.isLess(otherParam, tparam)) fs1 &~ Contravariant else fs1 |
There was a problem hiding this comment.
@odersky We were wondering about
We take this into account to instantiate the variable so that it narrows the overall constraint the least, defaulting again to lower bound if there is not best direction.
Are you trying to give the biggest type to the typing context? If so, do you know the variance of tv's occurrences in the typing context? It could occur in either covariant or contravariant positions, and that affects the polarity of occurrences of tvar in tv's bounds, but it doesn't look like tvar's polarity is accounted for...
| @@ -313,6 +313,29 @@ object Inferencing { | |||
|
|
|||
| propagate(accu(SimpleIdentityMap.Empty, tp)) | |||
| } | |||
|
|
|||
| private def varianceInContext(tvar: TypeVar)(implicit ctx: Context): FlagSet = { | |||
There was a problem hiding this comment.
Can you add documentation for this method
|
Merging this to fix the regression before the release. Code in this PR will be removed in #4065, so no need to further document it. |
Interpolation is a pretty delicate scenario. It's difficult to even say what is right and what
is wrong. In #4032 there's an unconstrained type variable that should be interpolated and it's a coin
flip whether we instantiate it to the lower or upper bound. A completely unrelated change in
#3981 meant that we instantiated the variable to the upper instead of the lower bound which
caused the program to fail. The failure was because the type variable appeared covariantly
in the lower bound of some other type variable. So maximizing the first type variable
overconstrained the second. We avoid this problem now by computing the variance of the type
variable that's eliminated in the rest of the constraint. We take this into account to
instantiate the variable so that it narrows the overall constraint the least, defaulting
again to lower bound if there is not best direction. Since the test is expensive (linear
in the constraint size), we do this only if the variable's lower bound is unconstrained.
We could do it for all non-occurring type variables, but have to see how that would affect
performance.