[ty] defer inference of legacy TypeVar bound/constraints/defaults #20598
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
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-09 21:06:16.051150791 +0000
+++ new-output.txt 2025-10-09 21:06:19.328159431 +0000
@@ -1,6 +1,6 @@
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/29ab321/src/function/execute.rs:217:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(cc17)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/29ab321/src/function/execute.rs:217:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1643f)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/29ab321/src/function/execute.rs:217:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1603f)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__`
@@ -364,6 +364,7 @@
generics_base_class.py:49:22: error[too-many-positional-arguments] Too many positional arguments to class `LinkedList`: expected 1, got 2
generics_base_class.py:61:18: error[too-many-positional-arguments] Too many positional arguments to class `MyDict`: expected 1, got 2
generics_basic.py:34:12: error[unsupported-operator] Operator `+` is unsupported between objects of type `AnyStr@concat` and `AnyStr@concat`
+generics_basic.py:49:44: error[invalid-legacy-type-variable] A `TypeVar` cannot have exactly one constraint
generics_basic.py:139:5: error[type-assertion-failure] Argument does not have asserted type `int`
generics_basic.py:140:5: error[type-assertion-failure] Argument does not have asserted type `int`
generics_basic.py:157:5: error[invalid-argument-type] Method `__getitem__` of type `bound method MyMap1[str, int].__getitem__(key: str, /) -> int` cannot be called with key of type `Literal[0]` on object of type `MyMap1[str, int]`
@@ -575,7 +576,8 @@
generics_upper_bound.py:43:1: error[type-assertion-failure] Argument does not have asserted type `list[int] | set[int]`
generics_upper_bound.py:51:8: error[invalid-argument-type] Argument to function `longer` is incorrect: Argument type `Literal[3]` does not satisfy upper bound `Sized` of type variable `ST`
generics_upper_bound.py:51:11: error[invalid-argument-type] Argument to function `longer` is incorrect: Argument type `Literal[3]` does not satisfy upper bound `Sized` of type variable `ST`
-generics_variance.py:14:6: error[invalid-legacy-type-variable] A legacy `typing.TypeVar` cannot be both covariant and contravariant
+generics_upper_bound.py:56:10: error[invalid-legacy-type-variable] A `TypeVar` cannot have both a bound and constraints
+generics_variance.py:14:6: error[invalid-legacy-type-variable] A `TypeVar` cannot be both covariant and contravariant
generics_variance.py:26:27: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Iterator[T_co@ImmutableList]`
generics_variance.py:57:28: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `B_co@func`
generics_variance.py:175:25: error[non-subscriptable] Cannot subscript object of type `<class 'Contra[typing.TypeVar]'>` with no `__class_getitem__` method
@@ -898,5 +900,5 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 899 diagnostics
+Found 901 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
Contributor
|
carljm
force-pushed
the
cjm/legacytv
branch
2 times, most recently
from
b7662e9 to
46a2e8b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
CodSpeed Performance ReportMerging #20598 will improve performances by 5.56%Comparing Summary
Benchmarks breakdown
Footnotes
|
carljm
force-pushed
the
cjm/legacytv
branch
2 times, most recently
from
9f019f3 to
125dff3
Compare
carljm
added a commit
that referenced
this pull request
Oct 8, 2025
## 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 as `def method(self) -> Self` correctly. 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.
carljm
requested review from
AlexWaygood,
MichaReiser,
dcreager and
sharkdp
as code owners
MichaReiser
reviewed
Oct 9, 2025
| | ^^^^^ | ||
| | | ||
| "#); | ||
| // TODO: This should jump to the definition of `Alias` above. |
Member
There was a problem hiding this comment.
Can we create an issue for this. It's otherwise hard to remember
Contributor
Author
There was a problem hiding this comment.
I don't think it will be hard to remember, because I have to fix it in order to fix the cases in astral-sh/ty#256 related to TypeAliasType. So in that sense we already have an issue that ensures we won't forget it.
Contributor
Author
There was a problem hiding this comment.
(And I plan to do this PR next.)
AlexWaygood
reviewed
Oct 9, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Only reviewed the tests so far
crates/ty_python_semantic/resources/mdtest/generics/legacy/classes.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/generics/legacy/variables.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/generics/legacy/variables.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/generics/legacy/variables.md
Outdated
Show resolved
Hide resolved
AlexWaygood
reviewed
Oct 9, 2025
carljm
commented
Oct 9, 2025
AlexWaygood
approved these changes
Oct 9, 2025
crates/ty_python_semantic/resources/mdtest/diagnostics/legacy_typevars.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Contributor
Author
|
Thanks @AlexWaygood for the excellent review here! |
dcreager
added a commit
that referenced
this pull request
Oct 14, 2025
As part of #20598, we added `is_identical_to` methods to `TypeVarInstance` and `BoundTypeVarInstance`, which compare when two typevar instances refer to "the same" underlying typevar, even if we have forced their lazy bounds/constraints as part of marking typevars as inferable. (Doing so results in a different salsa interned struct ID, since we've changed the contents of the `bounds_or_constraints` field.) It turns out that marking typevars as inferable is not the only way that we might force lazy bounds/constraints; it also happens when we materialize a type containing a typevar. This surfaced as ecosystem report failures on #20677. That means that we need a more long-term fix to this problem. (`is_identical_to`, and its underlying `original` field, were meant to be a temporary fix until we removed the `MarkTypeVarsInferable` type mapping.) This PR extracts out a separate type (`TypeVarIdentity`) that only includes the fields that actually inform whether two typevars are "the same". All other properties of the typevar (default, bounds/constraints, etc) still live in `TypeVarInstance`. Call sites that care about typevar identity can now either store just `TypeVarIdentity` (if they never need access to those other properties), or continue to store `TypeVarInstance` but pull out its `identity` when performing those "are they the same typevar" comparisons. (All of this also applies respectively to `BoundTypeVar{Identity,Instance}`.) In particular, constraint sets now work on `BoundTypeVarIdentity`, and generic contexts still _store_ a `BoundTypeVarInstance` (since we might need access to defaults when specializing), but are keyed on `BoundTypeVarIdentity`.
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
This allows us to handle self-referential bounds/constraints/defaults without panicking.
Handles more cases from astral-sh/ty#256
This also changes the way we infer the types of legacy TypeVars. Rather than understanding a constructor call to
typing[_extension].TypeVarinside of any (arbitrarily nested) expression, and having to use a specialassigned_tofield of the semantic index to try to best-effort figure out what name the typevar was assigned to, we instead understand the creation of a legacyTypeVaronly in the supported syntactic position (RHS of a simple un-annotated assignment with one target). In any other position, we just infer it as creating an opaque instance oftyping.TypeVar. (This behavior matches all other type checkers.)So we now special-case TypeVar creation in
TypeInferenceBuilder, as a special case of an assignment definition, rather than deeper inside call binding. This does mean we re-implement slightly more of argument-parsing, but in practice this is minimal and easy to handle correctly.This is easier to implement if we also make the RHS of a simple (no unpacking) one-target assignment statement no longer a standalone expression. Which is fine to do, because simple one-target assignments don't need to infer the RHS more than once. This is a bonus performance (0-3% across various projects) and significant memory-usage win, since most assignment statements are simple one-target assignment statements, meaning we now create many fewer standalone-expression salsa ingredients.
This change does mean that inference of manually-constructed
TypeAliasTypeinstances can no longer find its Definition inassigned_to, which regresses go-to-definition for these aliases. In a future PR,TypeAliasTypewill receive the same treatment thatTypeVardid in this PR (moving its special-case inference intoTypeInferenceBuilderand supporting it only in the correct syntactic position, and lazily inferring its value type to support recursion), which will also fix the go-to-definition regression. (I decided a temporary edge-case regression is better in this case than doubling the size of this PR.)This PR also tightens up and fixes various aspects of the validation of
TypeVarcreation, as seen in the tests.We still (for now) treat all typevars as instances of
typing.TypeVar, even if they were created usingtyping_extensions.TypeVar. This means we'll wrongly error on e.g.T.__default__on Python 3.11, even ifTis atyping_extensions.TypeVarinstance at runtime. We share this wrong behavior with both mypy and pyrefly. It will be easier to fix after we pull in python/typeshed#14840.There are some issues that showed up here with typevar identity and
MarkTypeVarsInferable; the fix here (using the neworiginalfield andis_identical_tomethods onBoundTypeVarInstanceandTypeVarInstance) is a bit kludgy, but it can go away when we eliminateMarkTypeVarsInferable.Test Plan
Added and updated mdtests.
Conformance suite impact
The impact here is all positive:
Ecosystem impact
Basically none; in the setuptools case we just issue slightly different errors on an invalid TypeVar definition, due to the modified validation code.