[ty] Fix infinite hang on mutually recursive TypeAliasType definitions#23397
Merged
carljm merged 7 commits intoastral-sh:mainfrom Feb 25, 2026
Merged
Conversation
knutwannheden
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 84.87% to 84.96%. The percentage of expected errors that received a diagnostic held steady at 75.14%. Summary
False positives removedDetails
|
Memory usage reportMemory usage unchanged ✅ |
|
knutwannheden
force-pushed
the
fix/ty-recursive-typealiastype
branch
from
75a22ce to
5d2914e
Compare
## Problem
ty hangs indefinitely when type-checking files with mutually recursive
`TypeAliasType` definitions created via the manual constructor:
```python
A = TypeAliasType('A', Union[str, 'B'])
B = TypeAliasType('B', list[A])
```
This was discovered while testing ty on pydantic's `test_type_alias_type.py`.
## Root Cause
`ManualPEP695TypeAliasType` was a `#[salsa::interned]` struct with the
resolved value type as one of its fields:
```rust
#[salsa::interned]
pub struct ManualPEP695TypeAliasType<'db> {
pub name: ast::name::Name,
pub definition: Option<Definition<'db>>,
pub value: Type<'db>, // <-- causes non-convergence
}
```
With `#[salsa::interned]`, ALL fields contribute to identity. When Salsa
detects a cycle in `infer_definition_types`, it re-executes the function
iteratively until the result converges (stops changing). But each iteration
produces a different `value` type as the cycle resolves, creating a new
interned ID each time:
- Iteration 1: value=`Union[str, Divergent]` → interned ID X1
- Iteration 2: value=`Union[str, TypeAlias_B]` → interned ID X2
- Iteration 3: yet another value → interned ID X3
- ... never converges
This contrasts with `PEP695TypeAliasType` (the `type X = ...` form), which
does NOT store the value in the interned struct and instead computes it
lazily via a separate tracked method with its own cycle detection.
## Approaches Explored
### Approach 1: `#[salsa::tracked]` with `#[no_eq]` on value
Changed from `#[salsa::interned]` to `#[salsa::tracked]` with `#[no_eq]`
on the value field, so that value changes don't affect the struct's identity.
**Result**: Still hangs. Tracked struct recycling does NOT work during Salsa
cycle re-iterations — each iteration creates a new tracked struct with a
fresh ID (observed IDs growing unboundedly: 9800, 9801, 9802, ...).
Additionally, `normalized_impl` and `recursive_type_normalized_impl` called
`Self::new()` which created additional tracked structs. Even after fixing
those to return `self`, the fundamental ID proliferation remained.
### Approach 2: Remove value from interned struct + register RHS as standalone expression
Removed the `value` field from the interned struct and computed it lazily
via a tracked `value_type()` method (mirroring PEP695TypeAliasType). But
the lazy method needs the `Definition` to find the AST node, and for simple
assignments (`X = expr`), the semantic index builder does NOT register the
RHS as a standalone expression — so `try_expression()` returns None and the
definition is unavailable.
Attempted to fix this by registering the RHS call expression as standalone
in the semantic index builder.
**Result**: Breaks TypeVar, ParamSpec, NamedTuple, and NewType handling.
`infer_assignment_definition_impl` has two code paths: if a standalone
expression exists, it uses generic `infer_standalone_expression_impl`; if
not, it checks for TypeVar/ParamSpec/NamedTuple/NewType/etc. and uses
specialized handlers. Registering call expressions as standalone causes ALL
of these to take the wrong path.
### Approach 3 (final): Remove value + thread definition via infer_assignment_definition_impl
Same lazy value computation as Approach 2, but instead of modifying the
semantic index, thread the definition through the existing type inference
path. In `infer_assignment_definition_impl`, `TypeAliasType` calls already
pass through the `else if let ast::Expr::Call` branch where the `definition`
is in scope. Added a `KnownClass::TypeAliasType` match arm that:
1. Does normal call inference (which creates a ManualPEP695TypeAliasType
with definition=None via check_call)
2. Patches up the result with the definition that's available in scope
This is the same pattern used for TypeVar, ParamSpec, NewType, and
builtins.type — all have special handling in this match.
## Changes
- **types.rs**: Removed `value` field from `ManualPEP695TypeAliasType`.
Added lazy `value_type()` tracked method with Salsa cycle annotations.
`normalized_impl` and `recursive_type_normalized_impl` return `self`
since the interned identity no longer depends on the value.
- **class.rs**: Updated `check_call` to not pass `value` to `new()`.
- **infer/builder.rs**: Added `KnownClass::TypeAliasType` match arm in
`infer_assignment_definition_impl` to supply the definition that
`check_call` can't find via `try_expression` for simple assignments.
- **arguments.rs**: Removed `expand_pep695_type_alias` unit test (can't
construct ManualPEP695TypeAliasType with a value in unit tests anymore;
functionality is covered by mdtests).
- **pep695_type_aliases.md**: Added mdtest for mutually recursive
TypeAliasType definitions.
- **cycle.md**: Updated test expectation for self-referential JSONValue
TypeAliasType — now correctly resolves to the actual type instead of
Divergent, since lazy evaluation means the alias is already bound by
the time value_type() runs.
knutwannheden
force-pushed
the
fix/ty-recursive-typealiastype
branch
from
5d2914e to
87f7816
Compare
knutwannheden
requested review from
BurntSushi,
Gankra and
MichaReiser
as code owners
Contributor
Author
|
To me the |
Member
ah, no, we're just quite nondeterministic right now unfortunately. Those are known flakes. Sorry for the false alarm. |
knutwannheden
added a commit
to openrewrite/ruff
that referenced
this pull request
Feb 19, 2026
…hang on recursive TypeAliasType
# Conflicts: # crates/ty_python_semantic/src/types/infer/builder.rs
carljm
requested changes
Feb 25, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Thank you for this fix! I think this is on the right track, but should go even further towards using the same pattern as e.g. TypeVar and NewType calls.
Replace the hybrid approach (check_call + builder patch) with a dedicated `infer_typealiastype_call` method, following the pattern used by TypeVar and NewType. This removes the TypeAliasType branch from `KnownClass::check_call` and consolidates all logic into the builder. Also adds validation that the name argument matches the assignment target, and emits `invalid-type-alias-type` when TypeAliasType is used outside a simple variable assignment.
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unsupported-operator |
4 | 0 | 0 |
invalid-argument-type |
2 | 0 | 0 |
unresolved-attribute |
2 | 0 | 0 |
| Total | 8 | 0 | 0 |
carljm
approved these changes
Feb 25, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good, thank you! I pushed a few minor cleanups (definition no longer needs to be Option, and we can further remove/simplify some other methods now) as well as one fix (we broke the binding context for typevars in generic manual aliases).
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
4 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.
Summary
Fix an infinite hang when type-checking files with mutually recursive
TypeAliasTypedefinitions created via the manual constructor:This was discovered while testing ty on pydantic's
tests/test_type_alias_type.py, which contains self-referentialTypeAliasTypedefinitions such as:Root Cause
ManualPEP695TypeAliasTypewas a#[salsa::interned]struct with the resolved value type as one of its fields:With
#[salsa::interned], all fields contribute to identity/deduplication. When Salsa detects a cycle ininfer_definition_types, it re-executes the function iteratively until the result converges (stops changing). But each iteration produces a differentvaluetype as the cycle resolves, creating a new interned ID each time:Union[str, Divergent](B not yet resolved) → interned ID X1Union[str, TypeAlias_B](B resolved) → interned ID X2This contrasts with
PEP695TypeAliasType(thetype X = ...form), which does NOT store the value in the interned struct and instead computes it lazily via a separate tracked method with its own cycle detection.Fix
Remove the
valuefield fromManualPEP695TypeAliasTypeand compute it lazily (matching thePEP695TypeAliasTypepattern):types.rs: Removedvaluefield. Added a lazyvalue_type()tracked method with Salsa cycle annotations that re-parses the value from the definition's AST on demand.infer/builder.rs: Created a dedicatedinfer_typealiastype_callmethod, following the same pattern used byTypeVar,ParamSpec, andNewType. This validates the arguments, constructs theManualPEP695TypeAliasType, and defers inference of the value argument to avoid cycles. Also added an "invalid context" diagnostic whenTypeAliasTypeis used outside a simple variable assignment.class.rs: Removed theTypeAliasTypehandling fromKnownClass::check_callentirely, since it is now fully handled by the dedicated inference method.Additional improvements:
TypeVarandNewType).typing.TypeAlias, now saysTypeAliasType).JSONValuetest case incycle.mdnow correctly resolves to the actual type instead ofDivergent, since lazy evaluation means the alias is already bound by the timevalue_type()runs.TypeAliasTypevariables now correctly navigates to the assignment (previously returned "No type definitions found" because the definition wasNone).Approaches Explored (and rejected)
#[salsa::tracked]with#[no_eq]on valueChanged from
#[salsa::interned]to#[salsa::tracked]with#[no_eq]on the value field. Result: Still hangs — tracked struct recycling does NOT work during Salsa cycle re-iterations. Each iteration creates a new tracked struct with a fresh ID (observed IDs growing unboundedly: 9800, 9801, 9802, ...).Register RHS as standalone expression in the semantic index builder
Registered the call expression RHS as a standalone expression so that
check_callcould find the definition viatry_expression. Result: BreaksTypeVar,ParamSpec,NamedTuple, andNewTypehandling, sinceinfer_assignment_definition_implhas a code path for standalone expressions (generic inference) and a separate code path with specialized handlers for these known classes. Registering call expressions as standalone causes all of them to take the wrong path.Test plan
TypeAliasTypedefinitions inpep695_type_aliases.mdcycle.mdtest expectation (improved fromDivergentto actual resolved type)ty_idesnapshot forgoto_type_of_bare_type_alias_type(now correctly finds the definition)