Fix specifity comparison for extensions in polymorphic givens #13509
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add Context#withTyperState - Add a committable parameter to TyperState#fresh
Previously, the code in the closure passed to `comparing` could have been using the wrong context. I don't think this happened in practice so far because comparingCtx was always equal to ctx but it will matter in the next commit.
When we compare polymorphic methods for specificity, we replace their type parameters by type variables constrained in the current context (see isAsSpecific), but for extension methods in polymorphic givens, the comparison was done with a constraint set that does not include the type parameters of the givens, this lead to ambiguity errors as experienced in typelevel/spotted-leopards#2. We fix this by ensuring the TyperState we use for the comparison contains any additional constraint specific to the TyperState of either alternative. This required generalizing TyperState#mergeConstraintWith to handle `this` not being committable (because in that case `this` does not need to take ownership of the type variables owned by `that`, therefore `that` itself is allowed to be committable).
|
test performance please |
odersky
approved these changes
Sep 16, 2021
| // alternatives correctly we need a TyperState that includes | ||
| // constraints from both sides, see | ||
| // tests/*/extension-specificity2.scala for test cases. | ||
| val constraintsIn1 = alt1.tstate.constraint ne ctx.typerState.constraint |
There was a problem hiding this comment.
Is constraintsIn1/2 expected to be rarely true? So, no performance implications?
There was a problem hiding this comment.
I'd expect this codepath to be rarely hit since it's already in a fallback path and will only concern polymorphic givens with matching extension methods.
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13509/ to see the changes. Benchmarks is based on merging with master (8d9542c) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we compare polymorphic methods for specificity, we replace their
type parameters by type variables constrained in the current
context (see isAsSpecific), but for extension methods in polymorphic
givens, the comparison was done with a constraint set that does not
include the type parameters of the givens, this lead to ambiguity errors
as experienced in
typelevel/spotted-leopards#2.
We fix this by ensuring the TyperState we use for the comparison
contains any additional constraint specific to the TyperState of either
alternative. This required generalizing TyperState#mergeConstraintWith
to handle
thisnot being committable (because in that casethisdoesnot need to take ownership of the type variables owned by
that,therefore
thatitself is allowed to be committable).