Fix signature computation and caching involving type variables #18092
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
smarter
force-pushed
the
fix-undef-erasure
branch
3 times, most recently
from
d84d6d6
to
47b2f48
Compare
smarter
force-pushed
the
fix-undef-erasure
branch
from
47b2f48
to
4870ade
Compare
smarter
changed the title
smarter
force-pushed
the
fix-undef-erasure
branch
2 times, most recently
from
17012c8
to
429dff2
Compare
dwijnand
reviewed
Jun 30, 2023
odersky
requested changes
Jul 1, 2023
| else if (arity <= Definitions.MaxTupleArity) defn.TupleType(arity).nn | ||
| private def erasePair(tp: Type)(using Context): Type | Null = { | ||
| val arity = tupleArity(tp) | ||
| if arity == -2 then null // erasure depends on an uninstantiated type variable or WildcardType |
There was a problem hiding this comment.
Elsewhere we make sure that we return null only if inSigName is true. But here it seems no such check is done.
There was a problem hiding this comment.
erasePair is called from TypeErasure#apply which does the null check, elsewhere we check that our input is not null which isn't necessary here with non-nullable types.
| @@ -630,7 +671,10 @@ class TypeErasure(sourceLanguage: SourceLanguage, semiEraseVCs: Boolean, isConst | |||
| else if sourceLanguage.isScala2 then | |||
| this(Scala2Erasure.intersectionDominator(Scala2Erasure.flattenedParents(tp))) | |||
| else | |||
| erasedGlb(this(tp1), this(tp2)) | |||
| val e1 = this(tp1) | |||
There was a problem hiding this comment.
can be streamlined by moving to WildcardType instead of null
| val e1 = this(tp1) | ||
| val e2 = this(tp2) | ||
| if e1 == null || e2 == null then null | ||
| else TypeComparer.orType(e1, e2, isErased = true) |
There was a problem hiding this comment.
can be streamlined by moving to WildcardType instead of null
There was a problem hiding this comment.
I switched to WildcardType but didn't find a better place to check for the special value than here, let me know if you had something specific in mind.
smarter
force-pushed
the
fix-undef-erasure
branch
2 times, most recently
from
9a59970
to
4f1c26c
Compare
Its meaning will be expanded in the next commit.
During typechecking, we sometimes need to compute the signature of a type involving uninstantiated type variables or wildcards (in particular, because signature matching is doen as part of subtype checking for methods), this is handled with `tpnme.Uninstantiated` which is handled specially in `Signature`. Before this commit, `sigName` only checked for wildcards and type variables at the top-level of the type, even though nested types can still have an impact on type erasure, in particular when they appear as part of: - an intersection - a union - an underlying type of a derived value class - the element type of an array type - an element type of a tuple (... *: X *: ...) We keep track of all these situations by returning `null` in `TypeErasure#apply` when encountering a wildcard or uninstantiated type variable, which we then propage upwards to the `sigName` call. This propagation only happens for `sigName` calls (where `inSigName` is true), otherwise we throw an assertion since erasure shouldn't normally be computed for underdefined types.
Signatures should not be cached if they may change. The existing `isUnderDefined` check is not enough to guarantee this because the signature might have been computed based on a type variable instantiated in a TyperState which was subsequently retracted. To guard against this, we now rely on `Type#isProvisional`. This is a stricter check than needed since the provisional part of the type does not always have an impact on its signature, but in practice this is fine: when compiling Dotty, signature cache misses increased by less than 2%.
I'm afraid we'll have to get rid of it if we want our caches to be correct, but I don't have the time to look into it right now.
As requested in the review, this is slightly shorter but means we have to be very careful to not accidentally forget to propagate a WildcardType to the top-level when computing signatures.
smarter
force-pushed
the
fix-undef-erasure
branch
from
4f1c26c
to
4499bc0
Compare
smarter
added
the
backport:nominated
Kordyjan
removed
the
backport:nominated
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.
This fixes the logic taking the signature of a type uninstantiated type variables (using tpnme.Uninstantiated) and non-permanently instantiated type variables (which have a legitimate signature in the currenty TyperState but cannot be cached).
The test case was minimized from fmonniot/scala3mock#2.