From 5919ef21ebb9902ef58bf30736e36ff1ce8d46a0 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:23:52 -0400 Subject: [PATCH 01/14] Implement TypeVar identity refactoring - Create TypeVarIdentity salsa-interned type with name, definition, and kind - Create BoundTypeVarIdentity for identity comparisons - Update TypeVarInstance to use identity field instead of separate fields - Update GenericContext and SpecializationBuilder to use BoundTypeVarIdentity as map keys - Remove is_identical_to methods in favor of direct identity comparison - All tests pass (271 passed, 0 failed) This ensures that typevars with the same logical identity but different materialized bounds are recognized as identical, fixing the regression where method calls on Top-materialized types fail. Generated with Claude Code assistance. --- PLAN.md | 210 ++++++++++++++++++ crates/ty_python_semantic/src/types.rs | 150 +++++++++---- .../ty_python_semantic/src/types/generics.rs | 81 ++++--- .../src/types/infer/builder.rs | 27 ++- 4 files changed, 381 insertions(+), 87 deletions(-) create mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 00000000000000..b187514fdf4153 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,210 @@ +# Plan: Fix TypeVar Identity Issue with Materialization + +## Problem + +When a `Top[C[T: (int, str)]]` type is used, method calls fail because the `Self` typevar in the method signature has a Bottom-materialized upper bound, while the `Self` in the class's inferable typevars set has no materialization. These are different `TypeVarInstance` objects in Salsa, causing `is_inferable()` to return false and the assignability check to fail. + +## Root Cause + +Two `TypeVarInstance` objects representing the same logical typevar (e.g., `Self` for a class) are treated as distinct when they differ only in their bounds/constraints due to materialization. This happens because: + +1. `TypeVarInstance` is salsa-interned and includes all fields (name, definition, bounds, variance, etc.) +2. When bounds are materialized, a new `TypeVarInstance` is created +3. Checks for "same typevar" use full equality, which includes the materialized bounds +4. The inferable typevars set uses `BoundTypeVarInstance` as keys, which includes the full `TypeVarInstance` + +## Solution + +Introduce a new concept of "typevar identity" that is separate from the full typevar instance. Two typevars have the same identity if they represent the same logical typevar, regardless of how their bounds have been materialized. + +## Implementation Steps + +### 1. Create `TypeVarIdentity` (salsa-interned) + +**Location**: `crates/ty_python_semantic/src/types.rs` + +**Fields** (moved from `TypeVarInstance`): +- `name: Name<'db>` - The typevar's name +- `definition: Option>` - Where the typevar was defined +- `kind: TypeVarKind` - Whether it's PEP 695, Legacy, or TypingSelf + +**Traits to implement**: +- `Debug` (via salsa) +- `Clone, Copy` (via salsa) +- `PartialEq, Eq` (via salsa) +- `Hash` (via salsa) +- `get_size2::GetSize` (via salsa) + +**Salsa attributes**: +```rust +#[salsa::interned(debug)] +pub struct TypeVarIdentity<'db> { + pub(crate) name: Name<'db>, + pub(crate) definition: Option>, + pub(crate) kind: TypeVarKind, +} +``` + +### 2. Create `BoundTypeVarIdentity` (non-interned) + +**Location**: `crates/ty_python_semantic/src/types.rs` + +**Fields**: +- `identity: TypeVarIdentity<'db>` - The typevar's identity +- `binding_context: BindingContext<'db>` - Where the typevar is bound + +**Traits to implement**: +- `Debug` +- `Clone, Copy` +- `PartialEq, Eq` +- `Hash` +- `get_size2::GetSize` + +This type identifies a specific binding of a typevar (e.g., `T@ClassC1` vs `T@FunctionF`). + +### 3. Update `TypeVarInstance` + +**Changes**: +- Add new field: `identity: TypeVarIdentity<'db>` +- Keep existing fields: `_bound_or_constraints`, `explicit_variance`, `_default`, `original` +- Remove fields moved to `TypeVarIdentity`: `name`, `definition`, `kind` + +**Constructor updates**: +- Create `TypeVarIdentity` first, then use it in `TypeVarInstance::new` +- Update all call sites that construct `TypeVarInstance` + +**Accessor methods**: +- Add forwarding methods for `name()`, `definition()`, `kind()` that delegate to `identity()` +- Keep existing methods for other fields + +### 4. Add `identity()` method to `BoundTypeVarInstance` + +**Method signature**: +```rust +pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { + BoundTypeVarIdentity { + identity: self.typevar(db).identity(db), + binding_context: self.binding_context(db), + } +} +``` + +### 5. Update `GenericContext` + +**Location**: `crates/ty_python_semantic/src/types/generics.rs` + +**Changes**: +- Change `variables_inner` from `FxOrderMap, ...>` to `FxOrderMap, ...>` +- Update `variables()` method to return `BoundTypeVarInstance` by looking up the full instance +- Update all methods that use `variables_inner` as a map key + +**Affected methods**: +- `from_typevar_instances()` - use `btv.identity(db)` as map key +- `variables()` - reconstruct `BoundTypeVarInstance` from stored identity +- `lookup()` - use identity for lookup + +### 6. Update `Specialization` + +**Location**: `crates/ty_python_semantic/src/types/generics.rs` + +**Changes**: +- The `generic_context` field already uses `GenericContext`, which will now use identities +- Verify that `types` array stays synchronized with context (might need to store parallel arrays or restructure) +- Update methods that iterate over typevars and types together + +### 7. Update `ConstraintSet` + +**Location**: `crates/ty_python_semantic/src/types.rs` (if it uses typevar keys) + +**Changes**: +- If `ConstraintSet` uses `BoundTypeVarInstance` as keys in any internal maps, update to use `BoundTypeVarIdentity` +- Search for uses of `BoundTypeVarInstance` as map keys or set elements + +### 8. Update `InferableTypeVars` + +**Location**: `crates/ty_python_semantic/src/types/generics.rs` + +**Changes**: +- Change from `FxHashSet>` to `FxHashSet>` +- Update `is_inferable()` to use `bound_typevar.identity(db)` for lookup +- Update `inferable_typevars_innerer()` to collect identities instead of full instances + +### 9. Remove `is_identical_to` methods + +**Location**: `crates/ty_python_semantic/src/types.rs` + +**Changes**: +- Remove `TypeVarInstance::is_identical_to()` method entirely +- Remove `BoundTypeVarInstance::is_identical_to()` method entirely +- Update all call sites to use direct equality comparison instead: + - `btv1.identity(db) == btv2.identity(db)` for bound typevars + - `tv1.identity(db) == tv2.identity(db)` for typevar instances +- Search for all uses of `is_identical_to` and replace with identity comparisons + +**Rationale**: With explicit identity types, we can use standard `==` comparison instead of custom methods. The identity types already implement `Eq` and `PartialEq` correctly. + +### 10. Update Display implementations + +**Location**: `crates/ty_python_semantic/src/types/display.rs` + +**Changes**: +- Update `DisplayBoundTypeVarInstance` to use `typevar.identity(db).name(db)` +- Verify all display code still works correctly + +## Testing Strategy + +### Primary Testing: Existing Test Suite +Since this branch is based on `main` (not `work`), the regression we identified doesn't exist yet in this branch. Our primary testing goal is to ensure all existing tests continue to pass after the refactoring. + +**Process**: +1. Run the full test suite in the new branch after each major step +2. Ensure no regressions are introduced by the refactoring +3. Fix any test failures that arise from the structural changes + +### Testing the Regression Fix +To verify that this change fixes the regression that would be introduced by the `work` branch changes: + +**Process**: +1. In the `work` worktree, temporarily merge the `dcreager/typevar-identity` branch: + ```bash + cd /home/dcreager/git/ruff/work + git merge dcreager/typevar-identity + ``` + +2. Build and run the test case from `/home/dcreager/Documents/scratch/ty/top.py`: + ```bash + cargo build --bin ty + target/debug/ty check /home/dcreager/Documents/scratch/ty/top.py + ``` + +3. Verify that the `invalid-argument-type` error no longer occurs for `x.method()` + +4. Revert the merge to restore the `work` branch: + ```bash + git merge --abort # or git reset --hard HEAD if merge was completed + ``` + +### Why This Approach? +- The `main` branch doesn't have the inferable typevar changes yet, so the bug doesn't manifest +- The `work` branch has the inferable changes that trigger the bug +- By merging our fix into `work`, we can test that it resolves the issue +- We revert to keep the `work` and new feature branches independent + +## Migration Notes + +- This is a significant refactoring that touches core type system code +- All places that construct `TypeVarInstance` must be updated +- All places that use `BoundTypeVarInstance` for identity/lookup must use `BoundTypeVarIdentity` +- Salsa will need to recompute caches after these changes +- Performance should be similar or slightly better (smaller identity keys for lookups) + +## Rollout + +1. Implement changes incrementally, ensuring tests pass at each step +2. Start with creating new types (`TypeVarIdentity`, `BoundTypeVarIdentity`) +3. Update `TypeVarInstance` structure +4. Update all construction sites +5. Update data structures (`GenericContext`, `InferableTypeVars`) +6. Update comparison logic +7. Run full test suite +8. Test with real-world cases including the motivating example diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index dc27519dbca7a5..1e64834615acab 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -1635,7 +1635,7 @@ impl<'db> Type<'db> { ( Type::NonInferableTypeVar(lhs_bound_typevar), Type::NonInferableTypeVar(rhs_bound_typevar), - ) if lhs_bound_typevar.is_identical_to(db, rhs_bound_typevar) => { + ) if lhs_bound_typevar.identity(db) == rhs_bound_typevar.identity(db) => { ConstraintSet::from(true) } @@ -7782,19 +7782,42 @@ pub enum TypeVarKind { /// the typevar is defined and immediately bound to a single generic context. Just like in the /// legacy case, we will create a `TypeVarInstance` and [`BoundTypeVarInstance`], and the type of /// `T` at `[1]` and `[2]` will be that `TypeVarInstance` and `BoundTypeVarInstance`, respectively. + +/// The identity of a type variable. +/// +/// This represents the core identity of a typevar, independent of its bounds or constraints. +/// Two typevars have the same identity if they represent the same logical typevar, even if +/// their bounds have been materialized differently. /// /// # Ordering -/// Ordering is based on the type var instance's salsa-assigned id and not on its values. -/// The id may change between runs, or when the type var instance was garbage collected and recreated. +/// Ordering is based on the identity's salsa-assigned id and not on its values. +/// The id may change between runs, or when the identity was garbage collected and recreated. #[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] #[derive(PartialOrd, Ord)] -pub struct TypeVarInstance<'db> { +pub struct TypeVarIdentity<'db> { /// The name of this TypeVar (e.g. `T`) #[returns(ref)] - name: ast::name::Name, + pub(crate) name: ast::name::Name, /// The type var's definition (None if synthesized) - pub definition: Option>, + pub(crate) definition: Option>, + + /// The kind of typevar (PEP 695, Legacy, or TypingSelf) + pub(crate) kind: TypeVarKind, +} + +impl get_size2::GetSize for TypeVarIdentity<'_> {} + +/// A specific instance of a type variable with its bounds and constraints. +/// +/// # Ordering +/// Ordering is based on the type var instance's salsa-assigned id and not on its values. +/// The id may change between runs, or when the type var instance was garbage collected and recreated. +#[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] +#[derive(PartialOrd, Ord)] +pub struct TypeVarInstance<'db> { + /// The identity of this typevar + pub(crate) identity: TypeVarIdentity<'db>, /// The upper bound or constraint on the type of this TypeVar, if any. Don't use this field /// directly; use the `bound_or_constraints` (or `upper_bound` and `constraints`) methods @@ -7808,8 +7831,6 @@ pub struct TypeVarInstance<'db> { /// `default_type` method instead (to evaluate any lazy default). _default: Option>, - pub kind: TypeVarKind, - /// If this typevar was transformed from another typevar via `mark_typevars_inferable`, this /// records the identity of the "original" typevar, so we can recognize them as the same /// typevar in `bind_typevar`. TODO: this (and the `is_identical_to` methods) should be @@ -7860,6 +7881,21 @@ impl<'db> TypeVarInstance<'db> { BoundTypeVarInstance::new(db, self, BindingContext::Definition(binding_context)) } + /// Get the name of this typevar (forwarded from identity) + pub(crate) fn name(self, db: &'db dyn Db) -> &'db ast::name::Name { + self.identity(db).name(db) + } + + /// Get the definition of this typevar (forwarded from identity) + pub(crate) fn definition(self, db: &'db dyn Db) -> Option> { + self.identity(db).definition(db) + } + + /// Get the kind of this typevar (forwarded from identity) + pub(crate) fn kind(self, db: &'db dyn Db) -> TypeVarKind { + self.identity(db).kind(db) + } + pub(crate) fn is_self(self, db: &'db dyn Db) -> bool { matches!(self.kind(db), TypeVarKind::TypingSelf) } @@ -7903,8 +7939,7 @@ impl<'db> TypeVarInstance<'db> { pub(crate) fn normalized_impl(self, db: &'db dyn Db, visitor: &NormalizedVisitor<'db>) -> Self { Self::new( db, - self.name(db), - self.definition(db), + self.identity(db), self._bound_or_constraints(db) .and_then(|bound_or_constraints| match bound_or_constraints { TypeVarBoundOrConstraintsEvaluation::Eager(bound_or_constraints) => { @@ -7924,7 +7959,6 @@ impl<'db> TypeVarInstance<'db> { .lazy_default(db) .map(|ty| ty.normalized_impl(db, visitor).into()), }), - self.kind(db), self.original(db), ) } @@ -7937,8 +7971,7 @@ impl<'db> TypeVarInstance<'db> { ) -> Self { Self::new( db, - self.name(db), - self.definition(db), + self.identity(db), self._bound_or_constraints(db) .and_then(|bound_or_constraints| match bound_or_constraints { TypeVarBoundOrConstraintsEvaluation::Eager(bound_or_constraints) => Some( @@ -7970,7 +8003,6 @@ impl<'db> TypeVarInstance<'db> { .lazy_default(db) .map(|ty| ty.materialize(db, materialization_kind, visitor).into()), }), - self.kind(db), self.original(db), ) } @@ -8021,20 +8053,14 @@ impl<'db> TypeVarInstance<'db> { Self::new( db, - self.name(db), - self.definition(db), + self.identity(db), new_bound_or_constraints, self.explicit_variance(db), new_default, - self.kind(db), new_original, ) } - fn is_identical_to(self, db: &'db dyn Db, other: Self) -> bool { - self == other || (self.original(db) == Some(other) || other.original(db) == Some(self)) - } - fn to_instance(self, db: &'db dyn Db) -> Option { let bound_or_constraints = match self.bound_or_constraints(db)? { TypeVarBoundOrConstraints::UpperBound(upper_bound) => { @@ -8044,14 +8070,18 @@ impl<'db> TypeVarInstance<'db> { TypeVarBoundOrConstraints::Constraints(constraints.to_instance(db)?.into_union()?) } }; - Some(Self::new( + let identity = TypeVarIdentity::new( db, Name::new(format!("{}'instance", self.name(db))), - None, + None, // definition + self.kind(db), + ); + Some(Self::new( + db, + identity, Some(bound_or_constraints.into()), self.explicit_variance(db), - None, - self.kind(db), + None, // _default self.original(db), )) } @@ -8179,6 +8209,25 @@ impl<'db> BindingContext<'db> { } } +/// The identity of a bound type variable. +/// +/// This identifies a specific binding of a typevar to a context (e.g., `T@ClassC` vs `T@FunctionF`), +/// independent of the typevar's bounds or constraints. Two bound typevars have the same identity +/// if they represent the same logical typevar bound in the same context, even if their bounds +/// have been materialized differently. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct BoundTypeVarIdentity<'db> { + pub(crate) identity: TypeVarIdentity<'db>, + pub(crate) binding_context: BindingContext<'db>, +} + +// The Salsa heap is tracked separately for the inner types. +impl get_size2::GetSize for BoundTypeVarIdentity<'_> { + fn get_heap_size(&self) -> usize { + 0 + } +} + /// A type variable that has been bound to a generic context, and which can be specialized to a /// concrete type. #[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] @@ -8192,6 +8241,17 @@ pub struct BoundTypeVarInstance<'db> { impl get_size2::GetSize for BoundTypeVarInstance<'_> {} impl<'db> BoundTypeVarInstance<'db> { + /// Get the identity of this bound typevar. + /// + /// This is used for comparing whether two bound typevars represent the same logical + /// typevar, regardless of differences in their bounds due to materialization. + pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { + BoundTypeVarIdentity { + identity: self.typevar(db).identity(db), + binding_context: self.binding_context(db), + } + } + /// Create a new PEP 695 type variable that can be used in signatures /// of synthetic generic functions. pub(crate) fn synthetic( @@ -8199,17 +8259,22 @@ impl<'db> BoundTypeVarInstance<'db> { name: &'static str, variance: TypeVarVariance, ) -> Self { + let identity = TypeVarIdentity::new( + db, + Name::new_static(name), + None, // definition + TypeVarKind::Pep695, + ); + Self::new( db, TypeVarInstance::new( db, - Name::new_static(name), - None, // definition + identity, None, // _bound_or_constraints Some(variance), None, // _default - TypeVarKind::Pep695, - None, + None, // original ), BindingContext::Synthetic, ) @@ -8221,34 +8286,27 @@ impl<'db> BoundTypeVarInstance<'db> { upper_bound: Type<'db>, binding_context: BindingContext<'db>, ) -> Self { + let identity = TypeVarIdentity::new( + db, + Name::new_static("Self"), + None, // definition + TypeVarKind::TypingSelf, + ); + Self::new( db, TypeVarInstance::new( db, - Name::new_static("Self"), - None, + identity, Some(TypeVarBoundOrConstraints::UpperBound(upper_bound).into()), Some(TypeVarVariance::Invariant), - None, - TypeVarKind::TypingSelf, - None, + None, // _default + None, // original ), binding_context, ) } - pub(crate) fn is_identical_to(self, db: &'db dyn Db, other: Self) -> bool { - if self == other { - return true; - } - - if self.binding_context(db) != other.binding_context(db) { - return false; - } - - self.typevar(db).is_identical_to(db, other.typevar(db)) - } - pub(crate) fn variance_with_polarity( self, db: &'db dyn Db, diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index eaf1c73f434cd2..42ddc3603e75ef 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -14,11 +14,11 @@ use crate::types::instance::{Protocol, ProtocolInstanceType}; use crate::types::signatures::{Parameter, Parameters, Signature}; use crate::types::tuple::{TupleSpec, TupleType, walk_tuple_type}; use crate::types::{ - ApplyTypeMappingVisitor, BoundTypeVarInstance, ClassLiteral, FindLegacyTypeVarsVisitor, - HasRelationToVisitor, IsDisjointVisitor, IsEquivalentVisitor, KnownClass, KnownInstanceType, - MaterializationKind, NormalizedVisitor, Type, TypeContext, TypeMapping, TypeRelation, - TypeVarBoundOrConstraints, TypeVarInstance, TypeVarKind, TypeVarVariance, UnionType, - binding_type, declaration_type, + ApplyTypeMappingVisitor, BoundTypeVarIdentity, BoundTypeVarInstance, ClassLiteral, + FindLegacyTypeVarsVisitor, HasRelationToVisitor, IsDisjointVisitor, IsEquivalentVisitor, + KnownClass, KnownInstanceType, MaterializationKind, NormalizedVisitor, Type, TypeContext, + TypeMapping, TypeRelation, TypeVarBoundOrConstraints, TypeVarIdentity, TypeVarInstance, + TypeVarKind, TypeVarVariance, UnionType, binding_type, declaration_type, }; use crate::{Db, FxOrderMap, FxOrderSet}; @@ -109,10 +109,16 @@ pub(crate) fn typing_self<'db>( ) -> Option> { let index = semantic_index(db, scope_id.file(db)); - let typevar = TypeVarInstance::new( + let identity = TypeVarIdentity::new( db, ast::name::Name::new_static("Self"), Some(class.definition(db)), + TypeVarKind::TypingSelf, + ); + + let typevar = TypeVarInstance::new( + db, + identity, Some( TypeVarBoundOrConstraints::UpperBound(Type::instance( db, @@ -125,7 +131,6 @@ pub(crate) fn typing_self<'db>( // [spec]: https://typing.python.org/en/latest/spec/generics.html#self Some(TypeVarVariance::Invariant), None, - TypeVarKind::TypingSelf, None, ); @@ -160,7 +165,7 @@ impl GenericContextTypeVarOptions { #[derive(PartialOrd, Ord)] pub struct GenericContext<'db> { #[returns(ref)] - variables_inner: FxOrderMap, GenericContextTypeVarOptions>, + variables_inner: FxOrderMap, (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions)>, } pub(super) fn walk_generic_context<'db, V: super::visitor::TypeVisitor<'db> + ?Sized>( @@ -181,7 +186,13 @@ impl<'db> GenericContext<'db> { db: &'db dyn Db, variables: impl IntoIterator, GenericContextTypeVarOptions)>, ) -> Self { - Self::new_internal(db, variables.into_iter().collect::>()) + Self::new_internal( + db, + variables + .into_iter() + .map(|(btv, opts)| (btv.identity(db), (btv, opts))) + .collect::>(), + ) } /// Creates a generic context from a list of PEP-695 type parameters. @@ -218,7 +229,7 @@ impl<'db> GenericContext<'db> { db, self.variables_inner(db) .iter() - .map(|(bound_typevar, options)| (*bound_typevar, options.promote_literals())), + .map(|(_, (bound_typevar, options))| (*bound_typevar, options.promote_literals())), ) } @@ -230,7 +241,7 @@ impl<'db> GenericContext<'db> { self.variables_inner(db) .iter() .chain(other.variables_inner(db).iter()) - .map(|(bound_typevar, options)| (*bound_typevar, *options)), + .map(|(_, (bound_typevar, options))| (*bound_typevar, *options)), ) } @@ -238,7 +249,7 @@ impl<'db> GenericContext<'db> { self, db: &'db dyn Db, ) -> impl ExactSizeIterator> + Clone { - self.variables_inner(db).keys().copied() + self.variables_inner(db).values().map(|(btv, _)| *btv) } fn variable_from_type_param( @@ -388,7 +399,7 @@ impl<'db> GenericContext<'db> { pub(crate) fn is_subset_of(self, db: &'db dyn Db, other: GenericContext<'db>) -> bool { let other_variables = other.variables_inner(db); self.variables(db) - .all(|bound_typevar| other_variables.contains_key(&bound_typevar)) + .all(|bound_typevar| other_variables.contains_key(&bound_typevar.identity(db))) } pub(crate) fn binds_typevar( @@ -396,8 +407,9 @@ impl<'db> GenericContext<'db> { db: &'db dyn Db, typevar: TypeVarInstance<'db>, ) -> Option> { - self.variables(db) - .find(|self_bound_typevar| self_bound_typevar.typevar(db).is_identical_to(db, typevar)) + self.variables(db).find(|self_bound_typevar| { + self_bound_typevar.typevar(db).identity(db) == typevar.identity(db) + }) } /// Creates a specialization of this generic context. Panics if the length of `types` does not @@ -481,7 +493,10 @@ impl<'db> GenericContext<'db> { } fn heap_size( - (variables,): &(FxOrderMap, GenericContextTypeVarOptions>,), + (variables,): &(FxOrderMap< + BoundTypeVarIdentity<'db>, + (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), + >,), ) -> usize { ruff_memory_usage::order_map_heap_size(variables) } @@ -733,7 +748,7 @@ impl<'db> Specialization<'db> { let restricted_variables = generic_context.variables(db); let restricted_types: Option> = restricted_variables .map(|variable| { - let index = self_variables.get_index_of(&variable)?; + let index = self_variables.get_index_of(&variable.identity(db))?; self_types.get(index).copied() }) .collect(); @@ -761,7 +776,7 @@ impl<'db> Specialization<'db> { let index = self .generic_context(db) .variables_inner(db) - .get_index_of(&bound_typevar)?; + .get_index_of(&bound_typevar.identity(db))?; self.types(db).get(index).copied() } @@ -1114,7 +1129,7 @@ impl<'db> PartialSpecialization<'_, 'db> { let index = self .generic_context .variables_inner(db) - .get_index_of(&bound_typevar)?; + .get_index_of(&bound_typevar.identity(db))?; self.types.get(index).copied() } } @@ -1123,7 +1138,7 @@ impl<'db> PartialSpecialization<'_, 'db> { /// specialization of a generic function. pub(crate) struct SpecializationBuilder<'db> { db: &'db dyn Db, - types: FxHashMap, Type<'db>>, + types: FxHashMap, Type<'db>>, } impl<'db> SpecializationBuilder<'db> { @@ -1143,20 +1158,22 @@ impl<'db> SpecializationBuilder<'db> { .annotation .and_then(|annotation| annotation.specialization_of(self.db, None)); - let types = (generic_context.variables_inner(self.db).iter()).map(|(variable, options)| { - let mut ty = self.types.get(variable).copied(); + let types = (generic_context.variables_inner(self.db).iter()).map( + |(identity, (variable, options))| { + let mut ty = self.types.get(identity).copied(); - // When inferring a specialization for a generic class typevar from a constructor call, - // promote any typevars that are inferred as a literal to the corresponding instance type. - if options.should_promote_literals { - let tcx = tcx_specialization - .and_then(|specialization| specialization.get(self.db, *variable)); + // When inferring a specialization for a generic class typevar from a constructor call, + // promote any typevars that are inferred as a literal to the corresponding instance type. + if options.should_promote_literals { + let tcx = tcx_specialization + .and_then(|specialization| specialization.get(self.db, *variable)); - ty = ty.map(|ty| ty.promote_literals(self.db, TypeContext::new(tcx))); - } + ty = ty.map(|ty| ty.promote_literals(self.db, TypeContext::new(tcx))); + } - ty - }); + ty + }, + ); // TODO Infer the tuple spec for a tuple type generic_context.specialize_partial(self.db, types) @@ -1164,7 +1181,7 @@ impl<'db> SpecializationBuilder<'db> { fn add_type_mapping(&mut self, bound_typevar: BoundTypeVarInstance<'db>, ty: Type<'db>) { self.types - .entry(bound_typevar) + .entry(bound_typevar.identity(self.db)) .and_modify(|existing| { *existing = UnionType::from_elements(self.db, [*existing, ty]); }) diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index c6b817c78962d2..f736bf0d18f927 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -95,8 +95,9 @@ use crate::types::{ MemberLookupPolicy, MetaclassCandidate, PEP695TypeAliasType, Parameter, ParameterForm, Parameters, SpecialFormType, SubclassOfType, TrackedConstraintSet, Truthiness, Type, TypeAliasType, TypeAndQualifiers, TypeContext, TypeQualifiers, - TypeVarBoundOrConstraintsEvaluation, TypeVarDefaultEvaluation, TypeVarInstance, TypeVarKind, - TypeVarVariance, TypedDictType, UnionBuilder, UnionType, binding_type, todo_type, + TypeVarBoundOrConstraintsEvaluation, TypeVarDefaultEvaluation, TypeVarIdentity, + TypeVarInstance, TypeVarKind, TypeVarVariance, TypedDictType, UnionBuilder, UnionType, + binding_type, todo_type, }; use crate::types::{ClassBase, add_inferred_python_version_hint_to_diagnostic}; use crate::unpack::{EvaluationMode, UnpackPosition}; @@ -2959,15 +2960,19 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { if bound_or_constraint.is_some() || default.is_some() { self.deferred.insert(definition); } - let ty = Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( + let identity = TypeVarIdentity::new( self.db(), &name.id, Some(definition), + TypeVarKind::Pep695, + ); + let ty = Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( + self.db(), + identity, bound_or_constraint, - None, + None, // explicit_variance default.as_deref().map(|_| TypeVarDefaultEvaluation::Lazy), - TypeVarKind::Pep695, - None, + None, // original ))); self.add_declaration_with_binding( node.into(), @@ -4298,15 +4303,19 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { self.deferred.insert(definition); } - Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( + let identity = TypeVarIdentity::new( db, target_name, Some(definition), + TypeVarKind::Legacy, + ); + Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( + db, + identity, bound_or_constraints, Some(variance), default, - TypeVarKind::Legacy, - None, + None, // original ))) } From 7cc7faac46e0198e35254e61b4629e8df2b2da5c Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:36:41 -0400 Subject: [PATCH 02/14] clean up claudisms --- crates/ty_python_semantic/src/types.rs | 110 +++++++----------- .../ty_python_semantic/src/types/generics.rs | 28 ++--- 2 files changed, 60 insertions(+), 78 deletions(-) diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 1e64834615acab..58f4ec350156a6 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -7750,7 +7750,32 @@ pub enum TypeVarKind { TypingSelf, } -/// A type variable that has not been bound to a generic context yet. +/// The identity of a type variable. +/// +/// This represents the core identity of a typevar, independent of its bounds or constraints. Two +/// typevars have the same identity if they represent the same logical typevar, even if their +/// bounds have been materialized differently. +/// +/// # Ordering +/// Ordering is based on the identity's salsa-assigned id and not on its values. +/// The id may change between runs, or when the identity was garbage collected and recreated. +#[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] +#[derive(PartialOrd, Ord)] +pub struct TypeVarIdentity<'db> { + /// The name of this TypeVar (e.g. `T`) + #[returns(ref)] + pub(crate) name: ast::name::Name, + + /// The type var's definition (None if synthesized) + pub(crate) definition: Option>, + + /// The kind of typevar (PEP 695, Legacy, or TypingSelf) + pub(crate) kind: TypeVarKind, +} + +impl get_size2::GetSize for TypeVarIdentity<'_> {} + +/// A specific instance of a type variable that has not been bound to a generic context yet. /// /// This is usually not the type that you want; if you are working with a typevar, in a generic /// context, which might be specialized to a concrete type, you want [`BoundTypeVarInstance`]. This @@ -7782,33 +7807,6 @@ pub enum TypeVarKind { /// the typevar is defined and immediately bound to a single generic context. Just like in the /// legacy case, we will create a `TypeVarInstance` and [`BoundTypeVarInstance`], and the type of /// `T` at `[1]` and `[2]` will be that `TypeVarInstance` and `BoundTypeVarInstance`, respectively. - -/// The identity of a type variable. -/// -/// This represents the core identity of a typevar, independent of its bounds or constraints. -/// Two typevars have the same identity if they represent the same logical typevar, even if -/// their bounds have been materialized differently. -/// -/// # Ordering -/// Ordering is based on the identity's salsa-assigned id and not on its values. -/// The id may change between runs, or when the identity was garbage collected and recreated. -#[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] -#[derive(PartialOrd, Ord)] -pub struct TypeVarIdentity<'db> { - /// The name of this TypeVar (e.g. `T`) - #[returns(ref)] - pub(crate) name: ast::name::Name, - - /// The type var's definition (None if synthesized) - pub(crate) definition: Option>, - - /// The kind of typevar (PEP 695, Legacy, or TypingSelf) - pub(crate) kind: TypeVarKind, -} - -impl get_size2::GetSize for TypeVarIdentity<'_> {} - -/// A specific instance of a type variable with its bounds and constraints. /// /// # Ordering /// Ordering is based on the type var instance's salsa-assigned id and not on its values. @@ -7881,17 +7879,14 @@ impl<'db> TypeVarInstance<'db> { BoundTypeVarInstance::new(db, self, BindingContext::Definition(binding_context)) } - /// Get the name of this typevar (forwarded from identity) pub(crate) fn name(self, db: &'db dyn Db) -> &'db ast::name::Name { self.identity(db).name(db) } - /// Get the definition of this typevar (forwarded from identity) pub(crate) fn definition(self, db: &'db dyn Db) -> Option> { self.identity(db).definition(db) } - /// Get the kind of this typevar (forwarded from identity) pub(crate) fn kind(self, db: &'db dyn Db) -> TypeVarKind { self.identity(db).kind(db) } @@ -8215,19 +8210,12 @@ impl<'db> BindingContext<'db> { /// independent of the typevar's bounds or constraints. Two bound typevars have the same identity /// if they represent the same logical typevar bound in the same context, even if their bounds /// have been materialized differently. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, get_size2::GetSize)] pub struct BoundTypeVarIdentity<'db> { pub(crate) identity: TypeVarIdentity<'db>, pub(crate) binding_context: BindingContext<'db>, } -// The Salsa heap is tracked separately for the inner types. -impl get_size2::GetSize for BoundTypeVarIdentity<'_> { - fn get_heap_size(&self) -> usize { - 0 - } -} - /// A type variable that has been bound to a generic context, and which can be specialized to a /// concrete type. #[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] @@ -8243,8 +8231,8 @@ impl get_size2::GetSize for BoundTypeVarInstance<'_> {} impl<'db> BoundTypeVarInstance<'db> { /// Get the identity of this bound typevar. /// - /// This is used for comparing whether two bound typevars represent the same logical - /// typevar, regardless of differences in their bounds due to materialization. + /// This is used for comparing whether two bound typevars represent the same logical typevar, + /// regardless of e.g. differences in their bounds or constraints due to materialization. pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { BoundTypeVarIdentity { identity: self.typevar(db).identity(db), @@ -8265,19 +8253,15 @@ impl<'db> BoundTypeVarInstance<'db> { None, // definition TypeVarKind::Pep695, ); - - Self::new( + let typevar = TypeVarInstance::new( db, - TypeVarInstance::new( - db, - identity, - None, // _bound_or_constraints - Some(variance), - None, // _default - None, // original - ), - BindingContext::Synthetic, - ) + identity, + None, // _bound_or_constraints + Some(variance), + None, // _default + None, // original + ); + Self::new(db, typevar, BindingContext::Synthetic) } /// Create a new synthetic `Self` type variable with the given upper bound. @@ -8292,19 +8276,15 @@ impl<'db> BoundTypeVarInstance<'db> { None, // definition TypeVarKind::TypingSelf, ); - - Self::new( + let typevar = TypeVarInstance::new( db, - TypeVarInstance::new( - db, - identity, - Some(TypeVarBoundOrConstraints::UpperBound(upper_bound).into()), - Some(TypeVarVariance::Invariant), - None, // _default - None, // original - ), - binding_context, - ) + identity, + Some(TypeVarBoundOrConstraints::UpperBound(upper_bound).into()), + Some(TypeVarVariance::Invariant), + None, // _default + None, // original + ); + Self::new(db, typevar, binding_context) } pub(crate) fn variance_with_polarity( diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index 42ddc3603e75ef..5ca9085fa5931f 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -115,17 +115,14 @@ pub(crate) fn typing_self<'db>( Some(class.definition(db)), TypeVarKind::TypingSelf, ); - + let bounds = TypeVarBoundOrConstraints::UpperBound(Type::instance( + db, + class.identity_specialization(db, typevar_to_type), + )); let typevar = TypeVarInstance::new( db, identity, - Some( - TypeVarBoundOrConstraints::UpperBound(Type::instance( - db, - class.identity_specialization(db, typevar_to_type), - )) - .into(), - ), + Some(bounds.into()), // According to the [spec], we can consider `Self` // equivalent to an invariant type variable // [spec]: https://typing.python.org/en/latest/spec/generics.html#self @@ -165,7 +162,10 @@ impl GenericContextTypeVarOptions { #[derive(PartialOrd, Ord)] pub struct GenericContext<'db> { #[returns(ref)] - variables_inner: FxOrderMap, (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions)>, + variables_inner: FxOrderMap< + BoundTypeVarIdentity<'db>, + (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), + >, } pub(super) fn walk_generic_context<'db, V: super::visitor::TypeVisitor<'db> + ?Sized>( @@ -493,10 +493,12 @@ impl<'db> GenericContext<'db> { } fn heap_size( - (variables,): &(FxOrderMap< - BoundTypeVarIdentity<'db>, - (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), - >,), + (variables,): &( + FxOrderMap< + BoundTypeVarIdentity<'db>, + (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), + >, + ), ) -> usize { ruff_memory_usage::order_map_heap_size(variables) } From 0644e0285463c381f32bdc326e8e3f43855d7fc9 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:43:31 -0400 Subject: [PATCH 03/14] Refactor GenericContext to use GenericContextTypeVar struct Replace tuple value type in GenericContext.variables_inner with a proper struct. This change: - Renames GenericContextTypeVarOptions to GenericContextTypeVar - Moves BoundTypeVarInstance into the struct alongside should_promote_literals - Updates all methods that use variables_inner (promote_literals, merge, variables, heap_size, and SpecializationBuilder.build) This makes the code cleaner and more maintainable by using a proper struct instead of a tuple for the map values. --- .../ty_python_semantic/src/types/generics.rs | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index 5ca9085fa5931f..718b197b4c958a 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -141,12 +141,20 @@ pub(crate) fn typing_self<'db>( .map(typevar_to_type) } -#[derive(Copy, Clone, Debug, Default, Eq, Hash, PartialEq, get_size2::GetSize)] -pub struct GenericContextTypeVarOptions { +#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, get_size2::GetSize)] +pub struct GenericContextTypeVar<'db> { + bound_typevar: BoundTypeVarInstance<'db>, should_promote_literals: bool, } -impl GenericContextTypeVarOptions { +impl<'db> GenericContextTypeVar<'db> { + fn new(bound_typevar: BoundTypeVarInstance<'db>) -> Self { + Self { + bound_typevar, + should_promote_literals: false, + } + } + fn promote_literals(mut self) -> Self { self.should_promote_literals = true; self @@ -162,10 +170,7 @@ impl GenericContextTypeVarOptions { #[derive(PartialOrd, Ord)] pub struct GenericContext<'db> { #[returns(ref)] - variables_inner: FxOrderMap< - BoundTypeVarIdentity<'db>, - (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), - >, + variables_inner: FxOrderMap, GenericContextTypeVar<'db>>, } pub(super) fn walk_generic_context<'db, V: super::visitor::TypeVisitor<'db> + ?Sized>( @@ -184,13 +189,13 @@ impl get_size2::GetSize for GenericContext<'_> {} impl<'db> GenericContext<'db> { fn from_variables( db: &'db dyn Db, - variables: impl IntoIterator, GenericContextTypeVarOptions)>, + variables: impl IntoIterator>, ) -> Self { Self::new_internal( db, variables .into_iter() - .map(|(btv, opts)| (btv.identity(db), (btv, opts))) + .map(|var| (var.bound_typevar.identity(db), var)) .collect::>(), ) } @@ -218,7 +223,7 @@ impl<'db> GenericContext<'db> { db, type_params .into_iter() - .map(|bound_typevar| (bound_typevar, GenericContextTypeVarOptions::default())), + .map(GenericContextTypeVar::new), ) } @@ -228,8 +233,8 @@ impl<'db> GenericContext<'db> { Self::from_variables( db, self.variables_inner(db) - .iter() - .map(|(_, (bound_typevar, options))| (*bound_typevar, options.promote_literals())), + .values() + .map(|var| var.promote_literals()), ) } @@ -239,9 +244,9 @@ impl<'db> GenericContext<'db> { Self::from_variables( db, self.variables_inner(db) - .iter() - .chain(other.variables_inner(db).iter()) - .map(|(_, (bound_typevar, options))| (*bound_typevar, *options)), + .values() + .chain(other.variables_inner(db).values()) + .copied(), ) } @@ -249,7 +254,7 @@ impl<'db> GenericContext<'db> { self, db: &'db dyn Db, ) -> impl ExactSizeIterator> + Clone { - self.variables_inner(db).values().map(|(btv, _)| *btv) + self.variables_inner(db).values().map(|var| var.bound_typevar) } fn variable_from_type_param( @@ -493,12 +498,7 @@ impl<'db> GenericContext<'db> { } fn heap_size( - (variables,): &( - FxOrderMap< - BoundTypeVarIdentity<'db>, - (BoundTypeVarInstance<'db>, GenericContextTypeVarOptions), - >, - ), + (variables,): &(FxOrderMap, GenericContextTypeVar<'db>>,), ) -> usize { ruff_memory_usage::order_map_heap_size(variables) } @@ -1161,14 +1161,14 @@ impl<'db> SpecializationBuilder<'db> { .and_then(|annotation| annotation.specialization_of(self.db, None)); let types = (generic_context.variables_inner(self.db).iter()).map( - |(identity, (variable, options))| { + |(identity, var)| { let mut ty = self.types.get(identity).copied(); // When inferring a specialization for a generic class typevar from a constructor call, // promote any typevars that are inferred as a literal to the corresponding instance type. - if options.should_promote_literals { + if var.should_promote_literals { let tcx = tcx_specialization - .and_then(|specialization| specialization.get(self.db, *variable)); + .and_then(|specialization| specialization.get(self.db, var.bound_typevar)); ty = ty.map(|ty| ty.promote_literals(self.db, TypeContext::new(tcx))); } From 782214ae162864b3f49860505e22222a91d83e34 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:49:43 -0400 Subject: [PATCH 04/14] var -> variable --- .../ty_python_semantic/src/types/generics.rs | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index 718b197b4c958a..3b8904d8de0d9e 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -195,7 +195,7 @@ impl<'db> GenericContext<'db> { db, variables .into_iter() - .map(|var| (var.bound_typevar.identity(db), var)) + .map(|variable| (variable.bound_typevar.identity(db), variable)) .collect::>(), ) } @@ -219,12 +219,7 @@ impl<'db> GenericContext<'db> { db: &'db dyn Db, type_params: impl IntoIterator>, ) -> Self { - Self::from_variables( - db, - type_params - .into_iter() - .map(GenericContextTypeVar::new), - ) + Self::from_variables(db, type_params.into_iter().map(GenericContextTypeVar::new)) } /// Returns a copy of this generic context where we will promote literal types in any inferred @@ -234,7 +229,7 @@ impl<'db> GenericContext<'db> { db, self.variables_inner(db) .values() - .map(|var| var.promote_literals()), + .map(|variable| variable.promote_literals()), ) } @@ -254,7 +249,9 @@ impl<'db> GenericContext<'db> { self, db: &'db dyn Db, ) -> impl ExactSizeIterator> + Clone { - self.variables_inner(db).values().map(|var| var.bound_typevar) + self.variables_inner(db) + .values() + .map(|variable| variable.bound_typevar) } fn variable_from_type_param( @@ -1160,22 +1157,21 @@ impl<'db> SpecializationBuilder<'db> { .annotation .and_then(|annotation| annotation.specialization_of(self.db, None)); - let types = (generic_context.variables_inner(self.db).iter()).map( - |(identity, var)| { + let types = + (generic_context.variables_inner(self.db).iter()).map(|(identity, variable)| { let mut ty = self.types.get(identity).copied(); // When inferring a specialization for a generic class typevar from a constructor call, // promote any typevars that are inferred as a literal to the corresponding instance type. - if var.should_promote_literals { - let tcx = tcx_specialization - .and_then(|specialization| specialization.get(self.db, var.bound_typevar)); - + if variable.should_promote_literals { + let tcx = tcx_specialization.and_then(|specialization| { + specialization.get(self.db, variable.bound_typevar) + }); ty = ty.map(|ty| ty.promote_literals(self.db, TypeContext::new(tcx))); } ty - }, - ); + }); // TODO Infer the tuple spec for a tuple type generic_context.specialize_partial(self.db, types) From 2ca6c1ec44083c25b3d46c7902ead47498428671 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:57:51 -0400 Subject: [PATCH 05/14] Remove unused 'original' field from TypeVarInstance The original field was used to track the identity of typevars that were transformed via mark_typevars_inferable, allowing them to be recognized as the same typevar. This was needed for the old is_identical_to methods. Since we now use TypeVarIdentity for identity comparisons and have removed the is_identical_to methods, the original field is no longer needed. It was only being passed through but never actually read or used. This simplifies TypeVarInstance::new by removing an unused parameter. --- crates/ty_python_semantic/src/types.rs | 38 ++++++------------- .../ty_python_semantic/src/types/generics.rs | 1 - .../src/types/infer/builder.rs | 2 - 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 58f4ec350156a6..f4dd675cd44811 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -7828,12 +7828,6 @@ pub struct TypeVarInstance<'db> { /// The default type for this TypeVar, if any. Don't use this field directly, use the /// `default_type` method instead (to evaluate any lazy default). _default: Option>, - - /// If this typevar was transformed from another typevar via `mark_typevars_inferable`, this - /// records the identity of the "original" typevar, so we can recognize them as the same - /// typevar in `bind_typevar`. TODO: this (and the `is_identical_to` methods) should be - /// removable once we remove `mark_typevars_inferable`. - pub(crate) original: Option>, } // The Salsa heap is tracked separately. @@ -7954,7 +7948,6 @@ impl<'db> TypeVarInstance<'db> { .lazy_default(db) .map(|ty| ty.normalized_impl(db, visitor).into()), }), - self.original(db), ) } @@ -7998,7 +7991,6 @@ impl<'db> TypeVarInstance<'db> { .lazy_default(db) .map(|ty| ty.materialize(db, materialization_kind, visitor).into()), }), - self.original(db), ) } @@ -8035,25 +8027,20 @@ impl<'db> TypeVarInstance<'db> { }), }); - // Ensure that we only modify the `original` field if we are going to modify one or both of - // `_bound_or_constraints` and `_default`; don't trigger creation of a new - // `TypeVarInstance` unnecessarily. - let new_original = if new_bound_or_constraints == self._bound_or_constraints(db) + // Don't trigger creation of a new `TypeVarInstance` unnecessarily. + if new_bound_or_constraints == self._bound_or_constraints(db) && new_default == self._default(db) { - self.original(db) + self } else { - Some(self) - }; - - Self::new( - db, - self.identity(db), - new_bound_or_constraints, - self.explicit_variance(db), - new_default, - new_original, - ) + Self::new( + db, + self.identity(db), + new_bound_or_constraints, + self.explicit_variance(db), + new_default, + ) + } } fn to_instance(self, db: &'db dyn Db) -> Option { @@ -8077,7 +8064,6 @@ impl<'db> TypeVarInstance<'db> { Some(bound_or_constraints.into()), self.explicit_variance(db), None, // _default - self.original(db), )) } @@ -8259,7 +8245,6 @@ impl<'db> BoundTypeVarInstance<'db> { None, // _bound_or_constraints Some(variance), None, // _default - None, // original ); Self::new(db, typevar, BindingContext::Synthetic) } @@ -8282,7 +8267,6 @@ impl<'db> BoundTypeVarInstance<'db> { Some(TypeVarBoundOrConstraints::UpperBound(upper_bound).into()), Some(TypeVarVariance::Invariant), None, // _default - None, // original ); Self::new(db, typevar, binding_context) } diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs index 3b8904d8de0d9e..38fbc4a40a553e 100644 --- a/crates/ty_python_semantic/src/types/generics.rs +++ b/crates/ty_python_semantic/src/types/generics.rs @@ -128,7 +128,6 @@ pub(crate) fn typing_self<'db>( // [spec]: https://typing.python.org/en/latest/spec/generics.html#self Some(TypeVarVariance::Invariant), None, - None, ); bind_typevar( diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index f736bf0d18f927..73decdb14a542c 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -2972,7 +2972,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { bound_or_constraint, None, // explicit_variance default.as_deref().map(|_| TypeVarDefaultEvaluation::Lazy), - None, // original ))); self.add_declaration_with_binding( node.into(), @@ -4315,7 +4314,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { bound_or_constraints, Some(variance), default, - None, // original ))) } From e6645bb9962ced77260b4a06383269bddc061bec Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 14:58:54 -0400 Subject: [PATCH 06/14] superfluous --- crates/ty_python_semantic/src/types.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index f4dd675cd44811..9870a89ff465a0 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -8027,20 +8027,13 @@ impl<'db> TypeVarInstance<'db> { }), }); - // Don't trigger creation of a new `TypeVarInstance` unnecessarily. - if new_bound_or_constraints == self._bound_or_constraints(db) - && new_default == self._default(db) - { - self - } else { - Self::new( - db, - self.identity(db), - new_bound_or_constraints, - self.explicit_variance(db), - new_default, - ) - } + Self::new( + db, + self.identity(db), + new_bound_or_constraints, + self.explicit_variance(db), + new_default, + ) } fn to_instance(self, db: &'db dyn Db) -> Option { From 6dab7e7a236a785c7e3e1196f0880e5ced1223f7 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:00:47 -0400 Subject: [PATCH 07/14] precommit --- PLAN.md | 80 ++++++++++++------- .../src/types/infer/builder.rs | 15 +--- 2 files changed, 55 insertions(+), 40 deletions(-) diff --git a/PLAN.md b/PLAN.md index b187514fdf4153..380305a9ed1009 100644 --- a/PLAN.md +++ b/PLAN.md @@ -9,9 +9,9 @@ When a `Top[C[T: (int, str)]]` type is used, method calls fail because the `Self Two `TypeVarInstance` objects representing the same logical typevar (e.g., `Self` for a class) are treated as distinct when they differ only in their bounds/constraints due to materialization. This happens because: 1. `TypeVarInstance` is salsa-interned and includes all fields (name, definition, bounds, variance, etc.) -2. When bounds are materialized, a new `TypeVarInstance` is created -3. Checks for "same typevar" use full equality, which includes the materialized bounds -4. The inferable typevars set uses `BoundTypeVarInstance` as keys, which includes the full `TypeVarInstance` +1. When bounds are materialized, a new `TypeVarInstance` is created +1. Checks for "same typevar" use full equality, which includes the materialized bounds +1. The inferable typevars set uses `BoundTypeVarInstance` as keys, which includes the full `TypeVarInstance` ## Solution @@ -24,11 +24,13 @@ Introduce a new concept of "typevar identity" that is separate from the full typ **Location**: `crates/ty_python_semantic/src/types.rs` **Fields** (moved from `TypeVarInstance`): + - `name: Name<'db>` - The typevar's name - `definition: Option>` - Where the typevar was defined - `kind: TypeVarKind` - Whether it's PEP 695, Legacy, or TypingSelf **Traits to implement**: + - `Debug` (via salsa) - `Clone, Copy` (via salsa) - `PartialEq, Eq` (via salsa) @@ -36,6 +38,7 @@ Introduce a new concept of "typevar identity" that is separate from the full typ - `get_size2::GetSize` (via salsa) **Salsa attributes**: + ```rust #[salsa::interned(debug)] pub struct TypeVarIdentity<'db> { @@ -50,10 +53,12 @@ pub struct TypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types.rs` **Fields**: + - `identity: TypeVarIdentity<'db>` - The typevar's identity - `binding_context: BindingContext<'db>` - Where the typevar is bound **Traits to implement**: + - `Debug` - `Clone, Copy` - `PartialEq, Eq` @@ -65,21 +70,25 @@ This type identifies a specific binding of a typevar (e.g., `T@ClassC1` vs `T@Fu ### 3. Update `TypeVarInstance` **Changes**: + - Add new field: `identity: TypeVarIdentity<'db>` - Keep existing fields: `_bound_or_constraints`, `explicit_variance`, `_default`, `original` - Remove fields moved to `TypeVarIdentity`: `name`, `definition`, `kind` **Constructor updates**: + - Create `TypeVarIdentity` first, then use it in `TypeVarInstance::new` - Update all call sites that construct `TypeVarInstance` **Accessor methods**: + - Add forwarding methods for `name()`, `definition()`, `kind()` that delegate to `identity()` - Keep existing methods for other fields ### 4. Add `identity()` method to `BoundTypeVarInstance` **Method signature**: + ```rust pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { BoundTypeVarIdentity { @@ -94,11 +103,13 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types/generics.rs` **Changes**: + - Change `variables_inner` from `FxOrderMap, ...>` to `FxOrderMap, ...>` - Update `variables()` method to return `BoundTypeVarInstance` by looking up the full instance - Update all methods that use `variables_inner` as a map key **Affected methods**: + - `from_typevar_instances()` - use `btv.identity(db)` as map key - `variables()` - reconstruct `BoundTypeVarInstance` from stored identity - `lookup()` - use identity for lookup @@ -108,6 +119,7 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types/generics.rs` **Changes**: + - The `generic_context` field already uses `GenericContext`, which will now use identities - Verify that `types` array stays synchronized with context (might need to store parallel arrays or restructure) - Update methods that iterate over typevars and types together @@ -117,6 +129,7 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types.rs` (if it uses typevar keys) **Changes**: + - If `ConstraintSet` uses `BoundTypeVarInstance` as keys in any internal maps, update to use `BoundTypeVarIdentity` - Search for uses of `BoundTypeVarInstance` as map keys or set elements @@ -125,6 +138,7 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types/generics.rs` **Changes**: + - Change from `FxHashSet>` to `FxHashSet>` - Update `is_inferable()` to use `bound_typevar.identity(db)` for lookup - Update `inferable_typevars_innerer()` to collect identities instead of full instances @@ -134,11 +148,12 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types.rs` **Changes**: + - Remove `TypeVarInstance::is_identical_to()` method entirely - Remove `BoundTypeVarInstance::is_identical_to()` method entirely - Update all call sites to use direct equality comparison instead: - - `btv1.identity(db) == btv2.identity(db)` for bound typevars - - `tv1.identity(db) == tv2.identity(db)` for typevar instances + - `btv1.identity(db) == btv2.identity(db)` for bound typevars + - `tv1.identity(db) == tv2.identity(db)` for typevar instances - Search for all uses of `is_identical_to` and replace with identity comparisons **Rationale**: With explicit identity types, we can use standard `==` comparison instead of custom methods. The identity types already implement `Eq` and `PartialEq` correctly. @@ -148,43 +163,52 @@ pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { **Location**: `crates/ty_python_semantic/src/types/display.rs` **Changes**: + - Update `DisplayBoundTypeVarInstance` to use `typevar.identity(db).name(db)` - Verify all display code still works correctly ## Testing Strategy ### Primary Testing: Existing Test Suite + Since this branch is based on `main` (not `work`), the regression we identified doesn't exist yet in this branch. Our primary testing goal is to ensure all existing tests continue to pass after the refactoring. **Process**: + 1. Run the full test suite in the new branch after each major step -2. Ensure no regressions are introduced by the refactoring -3. Fix any test failures that arise from the structural changes +1. Ensure no regressions are introduced by the refactoring +1. Fix any test failures that arise from the structural changes ### Testing the Regression Fix + To verify that this change fixes the regression that would be introduced by the `work` branch changes: **Process**: + 1. In the `work` worktree, temporarily merge the `dcreager/typevar-identity` branch: - ```bash - cd /home/dcreager/git/ruff/work - git merge dcreager/typevar-identity - ``` -2. Build and run the test case from `/home/dcreager/Documents/scratch/ty/top.py`: - ```bash - cargo build --bin ty - target/debug/ty check /home/dcreager/Documents/scratch/ty/top.py - ``` + ```bash + cd /home/dcreager/git/ruff/work + git merge dcreager/typevar-identity + ``` + +1. Build and run the test case from `/home/dcreager/Documents/scratch/ty/top.py`: -3. Verify that the `invalid-argument-type` error no longer occurs for `x.method()` + ```bash + cargo build --bin ty + target/debug/ty check /home/dcreager/Documents/scratch/ty/top.py + ``` -4. Revert the merge to restore the `work` branch: - ```bash - git merge --abort # or git reset --hard HEAD if merge was completed - ``` +1. Verify that the `invalid-argument-type` error no longer occurs for `x.method()` + +1. Revert the merge to restore the `work` branch: + + ```bash + git merge --abort # or git reset --hard HEAD if merge was completed + ``` ### Why This Approach? + - The `main` branch doesn't have the inferable typevar changes yet, so the bug doesn't manifest - The `work` branch has the inferable changes that trigger the bug - By merging our fix into `work`, we can test that it resolves the issue @@ -201,10 +225,10 @@ To verify that this change fixes the regression that would be introduced by the ## Rollout 1. Implement changes incrementally, ensuring tests pass at each step -2. Start with creating new types (`TypeVarIdentity`, `BoundTypeVarIdentity`) -3. Update `TypeVarInstance` structure -4. Update all construction sites -5. Update data structures (`GenericContext`, `InferableTypeVars`) -6. Update comparison logic -7. Run full test suite -8. Test with real-world cases including the motivating example +1. Start with creating new types (`TypeVarIdentity`, `BoundTypeVarIdentity`) +1. Update `TypeVarInstance` structure +1. Update all construction sites +1. Update data structures (`GenericContext`, `InferableTypeVars`) +1. Update comparison logic +1. Run full test suite +1. Test with real-world cases including the motivating example diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index 73decdb14a542c..74c0a008bdf05a 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -2960,12 +2960,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { if bound_or_constraint.is_some() || default.is_some() { self.deferred.insert(definition); } - let identity = TypeVarIdentity::new( - self.db(), - &name.id, - Some(definition), - TypeVarKind::Pep695, - ); + let identity = + TypeVarIdentity::new(self.db(), &name.id, Some(definition), TypeVarKind::Pep695); let ty = Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( self.db(), identity, @@ -4302,12 +4298,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { self.deferred.insert(definition); } - let identity = TypeVarIdentity::new( - db, - target_name, - Some(definition), - TypeVarKind::Legacy, - ); + let identity = TypeVarIdentity::new(db, target_name, Some(definition), TypeVarKind::Legacy); Type::KnownInstance(KnownInstanceType::TypeVar(TypeVarInstance::new( db, identity, From a4dc1719502f5d73ca09a11d3e0034fa016ab2c2 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:00:54 -0400 Subject: [PATCH 08/14] remove finished plan --- PLAN.md | 234 -------------------------------------------------------- 1 file changed, 234 deletions(-) delete mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 380305a9ed1009..00000000000000 --- a/PLAN.md +++ /dev/null @@ -1,234 +0,0 @@ -# Plan: Fix TypeVar Identity Issue with Materialization - -## Problem - -When a `Top[C[T: (int, str)]]` type is used, method calls fail because the `Self` typevar in the method signature has a Bottom-materialized upper bound, while the `Self` in the class's inferable typevars set has no materialization. These are different `TypeVarInstance` objects in Salsa, causing `is_inferable()` to return false and the assignability check to fail. - -## Root Cause - -Two `TypeVarInstance` objects representing the same logical typevar (e.g., `Self` for a class) are treated as distinct when they differ only in their bounds/constraints due to materialization. This happens because: - -1. `TypeVarInstance` is salsa-interned and includes all fields (name, definition, bounds, variance, etc.) -1. When bounds are materialized, a new `TypeVarInstance` is created -1. Checks for "same typevar" use full equality, which includes the materialized bounds -1. The inferable typevars set uses `BoundTypeVarInstance` as keys, which includes the full `TypeVarInstance` - -## Solution - -Introduce a new concept of "typevar identity" that is separate from the full typevar instance. Two typevars have the same identity if they represent the same logical typevar, regardless of how their bounds have been materialized. - -## Implementation Steps - -### 1. Create `TypeVarIdentity` (salsa-interned) - -**Location**: `crates/ty_python_semantic/src/types.rs` - -**Fields** (moved from `TypeVarInstance`): - -- `name: Name<'db>` - The typevar's name -- `definition: Option>` - Where the typevar was defined -- `kind: TypeVarKind` - Whether it's PEP 695, Legacy, or TypingSelf - -**Traits to implement**: - -- `Debug` (via salsa) -- `Clone, Copy` (via salsa) -- `PartialEq, Eq` (via salsa) -- `Hash` (via salsa) -- `get_size2::GetSize` (via salsa) - -**Salsa attributes**: - -```rust -#[salsa::interned(debug)] -pub struct TypeVarIdentity<'db> { - pub(crate) name: Name<'db>, - pub(crate) definition: Option>, - pub(crate) kind: TypeVarKind, -} -``` - -### 2. Create `BoundTypeVarIdentity` (non-interned) - -**Location**: `crates/ty_python_semantic/src/types.rs` - -**Fields**: - -- `identity: TypeVarIdentity<'db>` - The typevar's identity -- `binding_context: BindingContext<'db>` - Where the typevar is bound - -**Traits to implement**: - -- `Debug` -- `Clone, Copy` -- `PartialEq, Eq` -- `Hash` -- `get_size2::GetSize` - -This type identifies a specific binding of a typevar (e.g., `T@ClassC1` vs `T@FunctionF`). - -### 3. Update `TypeVarInstance` - -**Changes**: - -- Add new field: `identity: TypeVarIdentity<'db>` -- Keep existing fields: `_bound_or_constraints`, `explicit_variance`, `_default`, `original` -- Remove fields moved to `TypeVarIdentity`: `name`, `definition`, `kind` - -**Constructor updates**: - -- Create `TypeVarIdentity` first, then use it in `TypeVarInstance::new` -- Update all call sites that construct `TypeVarInstance` - -**Accessor methods**: - -- Add forwarding methods for `name()`, `definition()`, `kind()` that delegate to `identity()` -- Keep existing methods for other fields - -### 4. Add `identity()` method to `BoundTypeVarInstance` - -**Method signature**: - -```rust -pub(crate) fn identity(self, db: &'db dyn Db) -> BoundTypeVarIdentity<'db> { - BoundTypeVarIdentity { - identity: self.typevar(db).identity(db), - binding_context: self.binding_context(db), - } -} -``` - -### 5. Update `GenericContext` - -**Location**: `crates/ty_python_semantic/src/types/generics.rs` - -**Changes**: - -- Change `variables_inner` from `FxOrderMap, ...>` to `FxOrderMap, ...>` -- Update `variables()` method to return `BoundTypeVarInstance` by looking up the full instance -- Update all methods that use `variables_inner` as a map key - -**Affected methods**: - -- `from_typevar_instances()` - use `btv.identity(db)` as map key -- `variables()` - reconstruct `BoundTypeVarInstance` from stored identity -- `lookup()` - use identity for lookup - -### 6. Update `Specialization` - -**Location**: `crates/ty_python_semantic/src/types/generics.rs` - -**Changes**: - -- The `generic_context` field already uses `GenericContext`, which will now use identities -- Verify that `types` array stays synchronized with context (might need to store parallel arrays or restructure) -- Update methods that iterate over typevars and types together - -### 7. Update `ConstraintSet` - -**Location**: `crates/ty_python_semantic/src/types.rs` (if it uses typevar keys) - -**Changes**: - -- If `ConstraintSet` uses `BoundTypeVarInstance` as keys in any internal maps, update to use `BoundTypeVarIdentity` -- Search for uses of `BoundTypeVarInstance` as map keys or set elements - -### 8. Update `InferableTypeVars` - -**Location**: `crates/ty_python_semantic/src/types/generics.rs` - -**Changes**: - -- Change from `FxHashSet>` to `FxHashSet>` -- Update `is_inferable()` to use `bound_typevar.identity(db)` for lookup -- Update `inferable_typevars_innerer()` to collect identities instead of full instances - -### 9. Remove `is_identical_to` methods - -**Location**: `crates/ty_python_semantic/src/types.rs` - -**Changes**: - -- Remove `TypeVarInstance::is_identical_to()` method entirely -- Remove `BoundTypeVarInstance::is_identical_to()` method entirely -- Update all call sites to use direct equality comparison instead: - - `btv1.identity(db) == btv2.identity(db)` for bound typevars - - `tv1.identity(db) == tv2.identity(db)` for typevar instances -- Search for all uses of `is_identical_to` and replace with identity comparisons - -**Rationale**: With explicit identity types, we can use standard `==` comparison instead of custom methods. The identity types already implement `Eq` and `PartialEq` correctly. - -### 10. Update Display implementations - -**Location**: `crates/ty_python_semantic/src/types/display.rs` - -**Changes**: - -- Update `DisplayBoundTypeVarInstance` to use `typevar.identity(db).name(db)` -- Verify all display code still works correctly - -## Testing Strategy - -### Primary Testing: Existing Test Suite - -Since this branch is based on `main` (not `work`), the regression we identified doesn't exist yet in this branch. Our primary testing goal is to ensure all existing tests continue to pass after the refactoring. - -**Process**: - -1. Run the full test suite in the new branch after each major step -1. Ensure no regressions are introduced by the refactoring -1. Fix any test failures that arise from the structural changes - -### Testing the Regression Fix - -To verify that this change fixes the regression that would be introduced by the `work` branch changes: - -**Process**: - -1. In the `work` worktree, temporarily merge the `dcreager/typevar-identity` branch: - - ```bash - cd /home/dcreager/git/ruff/work - git merge dcreager/typevar-identity - ``` - -1. Build and run the test case from `/home/dcreager/Documents/scratch/ty/top.py`: - - ```bash - cargo build --bin ty - target/debug/ty check /home/dcreager/Documents/scratch/ty/top.py - ``` - -1. Verify that the `invalid-argument-type` error no longer occurs for `x.method()` - -1. Revert the merge to restore the `work` branch: - - ```bash - git merge --abort # or git reset --hard HEAD if merge was completed - ``` - -### Why This Approach? - -- The `main` branch doesn't have the inferable typevar changes yet, so the bug doesn't manifest -- The `work` branch has the inferable changes that trigger the bug -- By merging our fix into `work`, we can test that it resolves the issue -- We revert to keep the `work` and new feature branches independent - -## Migration Notes - -- This is a significant refactoring that touches core type system code -- All places that construct `TypeVarInstance` must be updated -- All places that use `BoundTypeVarInstance` for identity/lookup must use `BoundTypeVarIdentity` -- Salsa will need to recompute caches after these changes -- Performance should be similar or slightly better (smaller identity keys for lookups) - -## Rollout - -1. Implement changes incrementally, ensuring tests pass at each step -1. Start with creating new types (`TypeVarIdentity`, `BoundTypeVarIdentity`) -1. Update `TypeVarInstance` structure -1. Update all construction sites -1. Update data structures (`GenericContext`, `InferableTypeVars`) -1. Update comparison logic -1. Run full test suite -1. Test with real-world cases including the motivating example From bbacf203c7b360bbc6828edcf2ceef78d55710c4 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:33:54 -0400 Subject: [PATCH 09/14] Move display method to BoundTypeVarIdentity Update DisplayBoundTypeVarInstance to DisplayBoundTypeVarIdentity and move the display() method from BoundTypeVarInstance to BoundTypeVarIdentity. Since the display implementation only uses the identity fields (name and binding context), it makes more sense for it to operate on the identity type directly. This also reduces coupling to the full BoundTypeVarInstance. Updated all callers to use .identity(db).display(db) when displaying a BoundTypeVarInstance. --- .../src/types/constraints.rs | 4 ++-- .../ty_python_semantic/src/types/display.rs | 24 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index 1db37c3e56341e..a8cc0c5e36e2d2 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -348,7 +348,7 @@ impl<'db> ConstrainedTypeVar<'db> { return write!( f, "({} {} {})", - self.constraint.typevar(self.db).display(self.db), + self.constraint.typevar(self.db).identity(self.db).display(self.db), if self.negated { "≠" } else { "=" }, lower.display(self.db) ); @@ -361,7 +361,7 @@ impl<'db> ConstrainedTypeVar<'db> { if !lower.is_never() { write!(f, "{} ≤ ", lower.display(self.db))?; } - self.constraint.typevar(self.db).display(self.db).fmt(f)?; + self.constraint.typevar(self.db).identity(self.db).display(self.db).fmt(f)?; if !upper.is_object() { write!(f, " ≤ {}", upper.display(self.db))?; } diff --git a/crates/ty_python_semantic/src/types/display.rs b/crates/ty_python_semantic/src/types/display.rs index 1bee509e260407..191a71460b66e0 100644 --- a/crates/ty_python_semantic/src/types/display.rs +++ b/crates/ty_python_semantic/src/types/display.rs @@ -24,9 +24,9 @@ use crate::types::signatures::{CallableSignature, Parameter, Parameters, Signatu use crate::types::tuple::TupleSpec; use crate::types::visitor::TypeVisitor; use crate::types::{ - BoundTypeVarInstance, CallableType, IntersectionType, KnownBoundMethodType, KnownClass, - MaterializationKind, Protocol, ProtocolInstanceType, StringLiteralType, SubclassOfInner, Type, - UnionType, WrapperDescriptorKind, visitor, + BoundTypeVarIdentity, BoundTypeVarInstance, CallableType, IntersectionType, + KnownBoundMethodType, KnownClass, MaterializationKind, Protocol, ProtocolInstanceType, + StringLiteralType, SubclassOfInner, Type, UnionType, WrapperDescriptorKind, visitor, }; use ruff_db::parsed::parsed_module; @@ -561,7 +561,7 @@ impl Display for DisplayRepresentation<'_> { literal_name = enum_literal.name(self.db) ), Type::NonInferableTypeVar(bound_typevar) | Type::TypeVar(bound_typevar) => { - bound_typevar.display(self.db).fmt(f) + bound_typevar.identity(self.db).display(self.db).fmt(f) } Type::AlwaysTruthy => f.write_str("AlwaysTruthy"), Type::AlwaysFalsy => f.write_str("AlwaysFalsy"), @@ -600,24 +600,24 @@ impl Display for DisplayRepresentation<'_> { } } -impl<'db> BoundTypeVarInstance<'db> { +impl<'db> BoundTypeVarIdentity<'db> { pub(crate) fn display(self, db: &'db dyn Db) -> impl Display { - DisplayBoundTypeVarInstance { - bound_typevar: self, + DisplayBoundTypeVarIdentity { + bound_typevar_identity: self, db, } } } -struct DisplayBoundTypeVarInstance<'db> { - bound_typevar: BoundTypeVarInstance<'db>, +struct DisplayBoundTypeVarIdentity<'db> { + bound_typevar_identity: BoundTypeVarIdentity<'db>, db: &'db dyn Db, } -impl Display for DisplayBoundTypeVarInstance<'_> { +impl Display for DisplayBoundTypeVarIdentity<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - f.write_str(self.bound_typevar.typevar(self.db).name(self.db))?; - if let Some(binding_context) = self.bound_typevar.binding_context(self.db).name(self.db) { + f.write_str(self.bound_typevar_identity.identity.name(self.db))?; + if let Some(binding_context) = self.bound_typevar_identity.binding_context.name(self.db) { write!(f, "@{binding_context}")?; } Ok(()) From 119bcb40e3945761d6af721b1da7be12bbf39baf Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:35:07 -0400 Subject: [PATCH 10/14] clippy --- crates/ty_python_semantic/src/types/display.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ty_python_semantic/src/types/display.rs b/crates/ty_python_semantic/src/types/display.rs index 191a71460b66e0..5812592a97ad38 100644 --- a/crates/ty_python_semantic/src/types/display.rs +++ b/crates/ty_python_semantic/src/types/display.rs @@ -24,9 +24,9 @@ use crate::types::signatures::{CallableSignature, Parameter, Parameters, Signatu use crate::types::tuple::TupleSpec; use crate::types::visitor::TypeVisitor; use crate::types::{ - BoundTypeVarIdentity, BoundTypeVarInstance, CallableType, IntersectionType, - KnownBoundMethodType, KnownClass, MaterializationKind, Protocol, ProtocolInstanceType, - StringLiteralType, SubclassOfInner, Type, UnionType, WrapperDescriptorKind, visitor, + BoundTypeVarIdentity, CallableType, IntersectionType, KnownBoundMethodType, KnownClass, + MaterializationKind, Protocol, ProtocolInstanceType, StringLiteralType, SubclassOfInner, Type, + UnionType, WrapperDescriptorKind, visitor, }; use ruff_db::parsed::parsed_module; From 9adc068fdbc43344c9c27fdba5608bf67d8f70e2 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:41:13 -0400 Subject: [PATCH 11/14] Update ConstraintSet to use BoundTypeVarIdentity Change ConstrainedTypeVar to store BoundTypeVarIdentity instead of BoundTypeVarInstance. This makes constraints independent of typevar materialization, which is appropriate since constraints should only care about the logical identity of a typevar, not its specific bounds. Updated: - ConstrainedTypeVar::typevar field to BoundTypeVarIdentity - ConstraintSet::range() and negated_range() to accept BoundTypeVarIdentity - ConstrainedTypeVar::new_node() signature - All callers in function.rs to pass .identity(db) - Display code which now receives BoundTypeVarIdentity directly --- crates/ty_python_semantic/src/types/constraints.rs | 14 +++++++------- crates/ty_python_semantic/src/types/function.rs | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index a8cc0c5e36e2d2..7c2696cdf13dc9 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -60,7 +60,7 @@ use itertools::Itertools; use rustc_hash::FxHashSet; use crate::Db; -use crate::types::{BoundTypeVarInstance, IntersectionType, Type, UnionType}; +use crate::types::{BoundTypeVarIdentity, BoundTypeVarInstance, IntersectionType, Type, UnionType}; /// An extension trait for building constraint sets from [`Option`] values. pub(crate) trait OptionConstraintsExtension { @@ -223,7 +223,7 @@ impl<'db> ConstraintSet<'db> { pub(crate) fn range( db: &'db dyn Db, lower: Type<'db>, - typevar: BoundTypeVarInstance<'db>, + typevar: BoundTypeVarIdentity<'db>, upper: Type<'db>, ) -> Self { let lower = lower.bottom_materialization(db); @@ -236,7 +236,7 @@ impl<'db> ConstraintSet<'db> { pub(crate) fn negated_range( db: &'db dyn Db, lower: Type<'db>, - typevar: BoundTypeVarInstance<'db>, + typevar: BoundTypeVarIdentity<'db>, upper: Type<'db>, ) -> Self { Self::range(db, lower, typevar, upper).negate(db) @@ -258,7 +258,7 @@ impl From for ConstraintSet<'_> { #[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] #[derive(PartialOrd, Ord)] pub(crate) struct ConstrainedTypeVar<'db> { - typevar: BoundTypeVarInstance<'db>, + typevar: BoundTypeVarIdentity<'db>, lower: Type<'db>, upper: Type<'db>, } @@ -274,7 +274,7 @@ impl<'db> ConstrainedTypeVar<'db> { fn new_node( db: &'db dyn Db, lower: Type<'db>, - typevar: BoundTypeVarInstance<'db>, + typevar: BoundTypeVarIdentity<'db>, upper: Type<'db>, ) -> Node<'db> { debug_assert_eq!(lower, lower.bottom_materialization(db)); @@ -348,7 +348,7 @@ impl<'db> ConstrainedTypeVar<'db> { return write!( f, "({} {} {})", - self.constraint.typevar(self.db).identity(self.db).display(self.db), + self.constraint.typevar(self.db).display(self.db), if self.negated { "≠" } else { "=" }, lower.display(self.db) ); @@ -361,7 +361,7 @@ impl<'db> ConstrainedTypeVar<'db> { if !lower.is_never() { write!(f, "{} ≤ ", lower.display(self.db))?; } - self.constraint.typevar(self.db).identity(self.db).display(self.db).fmt(f)?; + self.constraint.typevar(self.db).display(self.db).fmt(f)?; if !upper.is_object() { write!(f, " ≤ {}", upper.display(self.db))?; } diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index 2f6b5858d75963..b1d42b9154b029 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -1666,7 +1666,7 @@ impl KnownFunction { return; }; - let constraints = ConstraintSet::range(db, *lower, *typevar, *upper); + let constraints = ConstraintSet::range(db, *lower, typevar.identity(db), *upper); let tracked = TrackedConstraintSet::new(db, constraints); overload.set_return_type(Type::KnownInstance(KnownInstanceType::ConstraintSet( tracked, @@ -1683,7 +1683,7 @@ impl KnownFunction { return; }; - let constraints = ConstraintSet::negated_range(db, *lower, *typevar, *upper); + let constraints = ConstraintSet::negated_range(db, *lower, typevar.identity(db), *upper); let tracked = TrackedConstraintSet::new(db, constraints); overload.set_return_type(Type::KnownInstance(KnownInstanceType::ConstraintSet( tracked, From 7dc5144e568a4544bf7a6f589ae4e5fa0f4452a9 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:43:36 -0400 Subject: [PATCH 12/14] clippy --- crates/ty_python_semantic/src/types/constraints.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index 7c2696cdf13dc9..3d2b23c09f8454 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -60,7 +60,7 @@ use itertools::Itertools; use rustc_hash::FxHashSet; use crate::Db; -use crate::types::{BoundTypeVarIdentity, BoundTypeVarInstance, IntersectionType, Type, UnionType}; +use crate::types::{BoundTypeVarIdentity, IntersectionType, Type, UnionType}; /// An extension trait for building constraint sets from [`Option`] values. pub(crate) trait OptionConstraintsExtension { From f589ad04e3027aed48b07987afbea161f773f480 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 15:44:04 -0400 Subject: [PATCH 13/14] pre-commit --- crates/ty_python_semantic/src/types/function.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index b1d42b9154b029..bcced84655ddc7 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -1683,7 +1683,8 @@ impl KnownFunction { return; }; - let constraints = ConstraintSet::negated_range(db, *lower, typevar.identity(db), *upper); + let constraints = + ConstraintSet::negated_range(db, *lower, typevar.identity(db), *upper); let tracked = TrackedConstraintSet::new(db, constraints); overload.set_return_type(Type::KnownInstance(KnownInstanceType::ConstraintSet( tracked, From dd9abdff5debf410ab3abdcd2570dce83e52edfa Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Sat, 11 Oct 2025 16:58:20 -0400 Subject: [PATCH 14/14] missed one --- crates/ty_python_semantic/src/types.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index e96835ee15f3fc..ce1bd3a640fbf1 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -2417,7 +2417,9 @@ impl<'db> Type<'db> { ( Type::NonInferableTypeVar(self_bound_typevar), Type::NonInferableTypeVar(other_bound_typevar), - ) if self_bound_typevar == other_bound_typevar => ConstraintSet::from(false), + ) if self_bound_typevar.identity(db) == other_bound_typevar.identity(db) => { + ConstraintSet::from(false) + } (tvar @ Type::NonInferableTypeVar(_), Type::Intersection(intersection)) | (Type::Intersection(intersection), tvar @ Type::NonInferableTypeVar(_))