[ty] fix implicit Self on generic class with typevar default#20754
Merged
[ty] fix implicit Self on generic class with typevar default#20754
Conversation
9617d66 to
a2396cd
Compare
Contributor
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Contributor
|
This comment was marked as outdated.
This comment was marked as outdated.
CodSpeed Performance ReportMerging #20754 will degrade performances by 4.96%Comparing Summary
Benchmarks breakdown
Footnotes
|
a2396cd to
78a377c
Compare
78a377c to
ae1e527
Compare
This file contains hidden or 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
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.
Summary
Typevar attributes (bound/constraints/default) can be either lazily evaluated or eagerly evaluated. Currently they are lazily evaluated for PEP 695 typevars, and eager for legacy and synthetic typevars. #20598 will make them lazy also for legacy typevars, and the ecosystem report on that PR surfaced the issue fixed here (because legacy typevars are much more common in the ecosystem than PEP 695 typevars.)
Applying a transform to a typevar (normalization, materialization, or mark-inferable) will reify all lazy attributes and create a new typevar with eager attributes. In terms of Salsa identity, this transformed typevar will be considered different from the original typevar, whether or not the attributes were actually transformed.
In general, this is not a problem, since all typevars in a given generic context will be transformed, or not, together.
The exception to this was implicit-self vs explicit Self annotations. The typevar we created for implicit self was created initially using inferable typevars, whereas an explicit Self annotation is initially non-inferable, then transformed via mark-inferable when accessed as part of a function signature. If the containing class (which becomes the upper bound of
Self) is generic, and has e.g. a lazily-evaluated default, then the explicit-Self annotation will reify that default in the upper bound, and the implicit-self would not, leading them to be treated as different typevars, and causing us to fail to solve a call to a method such asdef method(self) -> Selfcorrectly.The fix here is to treat implicit-self more like explicit-Self, initially creating it as non-inferable and then using the mark-inferable transform on it. This is less efficient, but restores the invariant that all typevars in a given generic context are transformed together, or not, fixing the bug.
In the improved-constraint-solver work, the separation of typevars into "inferable" and "non-inferable" is expected to disappear, along with the mark-inferable transform, which would render both this bug and the fix moot. So this fix is really just temporary until that lands.
There is a performance regression, but not a huge one: 1-2% on most projects, 5% on one outlier. This seems acceptable, given that it should be fully recovered by removing the mark-inferable transform.
Test Plan
Added mdtests that failed before this change.