[ty] Don't track inferability via different Type variants
#20677
Conversation
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
dcreager
added
internal
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
5 | 5 | 4 |
unresolved-attribute |
3 | 11 | 0 |
invalid-return-type |
0 | 12 | 1 |
no-matching-overload |
1 | 11 | 0 |
unused-ignore-comment |
5 | 0 | 0 |
invalid-assignment |
1 | 1 | 2 |
type-assertion-failure |
3 | 0 | 0 |
redundant-cast |
1 | 0 | 0 |
unsupported-operator |
0 | 1 | 0 |
| Total | 19 | 41 | 7 |
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
6bc7ceb to
417caf0
Compare
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
417caf0 to
7b5684f
Compare
|
ecosystem analysis: Several projects have removed diagnostics, which look like correct better understanding of the types involved:
Still looking at some of the new diagnostics. It also looks like we aren't expanding type aliases in as many places as before. |
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
7a69d29 to
df385d2
Compare
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
f11b47c to
74cc6b7
Compare
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
74cc6b7 to
6d12c00
Compare
dcreager
force-pushed
the
dcreager/non-non-inferable
branch
from
a2266c0 to
5f708ba
Compare
CodSpeed Performance ReportMerging #20677 will improve performances by 4.49%Comparing Summary
Benchmarks breakdown
Footnotes
|
dcreager
requested review from
AlexWaygood,
MichaReiser,
carljm and
sharkdp
as code owners
|
The remaining ecosystem additions seem related to bidi checking of arguments and overload resolution. This overlaps with some of the work @ibraheemdev is doing e.g. in #20796, so for now, I'm opening this up for review, and would like to address the remaining ecosystem results with him as part of that work. |
|
(Note that I pulled the pure refactoring part of this PR out into a separate #20865, so that it's easier to review the logic changes separate from the boilerplate API changes. Please review 20865 first) |
dcreager
added a commit
that referenced
this pull request
Oct 15, 2025
) A large part of the diff on #20677 just involves threading a new `inferable` parameter through all of the type property methods. In the interests of making that PR easier to review, I've pulled that bit out into here, so that it can be reviewed in isolation. This should be a pure refactoring, with no logic changes or behavioral changes.
…rable * origin/main: [ty] Add (unused) `inferable` parameter to type property methods (#20865) Run macos tests on macos (#20889) Remove `release` CI job (#20887) [ty] CI: Faster ecosystem analysis (#20886) Remove `strip` from release profile (#20885) [ty] Sync vendored typeshed stubs (#20876) [ty] Add some completion ranking improvements (#20807) Improved error recovery for unclosed strings (including f- and t-strings) (#20848) Enable lto=fat (#20863) [`pyupgrade`] Extend `UP019` to detect `typing_extensions.Text` (`UP019`) (#20825) [`flake8-bugbear`] Omit annotation in preview fix for `B006` (#20877) fix(docs): Fix typo in `RUF015` description (#20873) [ty] Improve and extend tests for instance attributes redeclared in subclasses (#20866) [ty] Ignore slow seeds as a temporary measure (#20870) Remove parentheses around multiple exception types on Python 3.14+ (#20768) Update Black tests (#20794)
…rable * origin/main: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
AlexWaygood
approved these changes
Oct 16, 2025
There was a problem hiding this comment.
Looks great, thank you! Very relieved that I no longer have to worry about the differences between these two variants!
There's a couple of places where it looks like our behaviour is changing subtly, where I wonder if we could add regression tests if the new behaviour is desirable? Especially if this is the cause of the changes in the ecosystem. But if that's hard to do, then no need to spend too much time on it
Comment on lines
319
to
331
| #[salsa::tracked( | ||
| returns(ref), | ||
| cycle_fn=inferable_typevars_cycle_recover, | ||
| cycle_initial=inferable_typevars_cycle_initial, | ||
| heap_size=ruff_memory_usage::heap_size, | ||
| )] | ||
| fn inferable_typevars_inner(self, db: &'db dyn Db) -> FxHashSet<BoundTypeVarIdentity<'db>> { | ||
| // The second inner function is because the salsa macros seem to not like nested structs | ||
| // and impl blocks inside the function. | ||
| self.inferable_typevars_innerer(db) | ||
| } | ||
|
|
||
| fn inferable_typevars_innerer(self, db: &'db dyn Db) -> FxHashSet<BoundTypeVarIdentity<'db>> { |
There was a problem hiding this comment.
lol. Is it worth nesting these methods inside the inferable_typevars method, so that it's obvious to readers that they're implementation details of that method?
There was a problem hiding this comment.
Do you remember what the issue was here? Might be worth opening a Salsa issue, the macros should be relatively transparent.
There was a problem hiding this comment.
Putting innerer inside of inner didn't work, but I don't remember the specific error. Put all of it inside of the outer function works, though, since the type and impl block don't need to be inside the salsa-tracked function that way.
|
The performance analysis keeps bouncing back and forth between being a regression and an improvement as I rebase to pick up more changes from |
Co-authored-by: Alex Waygood <[email protected]>
the So |
…rable * origin/main: [ty] Support dataclass-transform `field_specifiers` (#20888) Bump 0.14.1 (#20925) Standardize syntax error construction (#20903) [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376) [ty] Fix panic 'missing root' when handling completion request (#20917) [ty] Run file watching tests serial when using nextest (#20918) [ty] Add version hint for failed stdlib attribute accesses (#20909) More CI improvements (#20920) [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
…rable * origin/main: [ty] Avoid unnecessarily widening generic specializations (#20875)
dcreager
added a commit
that referenced
this pull request
Oct 17, 2025
* main: [ty] Prefer declared type for invariant collection literals (#20927) [ty] Don't track inferability via different `Type` variants (#20677) [ty] Use declared variable types as bidirectional type context (#20796) [ty] Avoid unnecessarily widening generic specializations (#20875) [ty] Support dataclass-transform `field_specifiers` (#20888) Bump 0.14.1 (#20925) Standardize syntax error construction (#20903) [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376) [ty] Fix panic 'missing root' when handling completion request (#20917) [ty] Run file watching tests serial when using nextest (#20918) [ty] Add version hint for failed stdlib attribute accesses (#20909) More CI improvements (#20920) [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
We have to track whether a typevar appears in a position where it's inferable or not. In a non-inferable position (in the body of the generic class or function that binds it), assignability must hold for every possible specialization of the typevar. In an inferable position, it only needs to hold for some specialization. #20093 is working on using constraint sets to model assignability of typevars, and the constraint sets that we produce will be the same for inferable vs non-inferable typevars; what changes is what we compare that constraint set to. (For a non-inferable typevar, the constraint set must equal the set of valid specializations; for an inferable typevar, it must not be
never.)When I first added support for tracking inferable vs non-inferable typevars, it seemed like it would be easiest to have separate
Typevariants for each. The alternative (which lines up with the Δ set in POPL15) would be to explicitly plumb through a list of inferable typevars through our type property methods. That seemed cumbersome.In retrospect, that was the wrong decision. We've had to jump through hoops to translate types between the inferable and non-inferable variants, which has been quite brittle. Combined with the original point above, that much of the assignability logic will become more identical between inferable and non-inferable, there is less justification for the two
Typevariants. And plumbing an extrainferableparameter through all of these methods turns out to not be as bad as I anticipated.