[ty] Typevars should be considered equivalent even after lazy defaults/bounds/etc are forced #21506
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| fn is_equivalent_to(self, db: &'db dyn Db, other: Self) -> bool { | ||
| match (self, other) { | ||
| (Self::TypeVar(self_typevar), Self::TypeVar(other_typevar)) => { | ||
| self_typevar.is_same_typevar_as(db, other_typevar) | ||
| } | ||
| ( | ||
| Self::SubscriptedProtocol(_) | ||
| | Self::SubscriptedGeneric(_) | ||
| | Self::TypeVar(_) | ||
| | Self::TypeAliasType(_) | ||
| | Self::Deprecated(_) | ||
| | Self::Field(_) | ||
| | Self::ConstraintSet(_) | ||
| | Self::UnionType(_) | ||
| | Self::Literal(_) | ||
| | Self::Annotated(_) | ||
| | Self::TypeGenericAlias(_) | ||
| | Self::NewType(_), | ||
| Self::SubscriptedProtocol(_) | ||
| | Self::SubscriptedGeneric(_) | ||
| | Self::TypeVar(_) | ||
| | Self::TypeAliasType(_) | ||
| | Self::Deprecated(_) | ||
| | Self::Field(_) | ||
| | Self::ConstraintSet(_) | ||
| | Self::UnionType(_) | ||
| | Self::Literal(_) | ||
| | Self::Annotated(_) | ||
| | Self::TypeGenericAlias(_) | ||
| | Self::NewType(_), | ||
| ) => self == other, | ||
| } | ||
| } |
There was a problem hiding this comment.
IIUC, all of these KnownInstance type variants essentially represent the type of a parameterized special form in a value position. so for T = TypeVar("T"), we represent the type of T as being a Type::KnownInstanceType(KnownInstanceType::TypeVar(..)). Given this, why is it important to check two typevars for equivalence using .is_same_typevar_as(), but not do the same for other special forms in value positions (which might be parameterized with typevars). E.g. when comparing whether T | int is equivalent to T | int, where T is a type variable -- here T | int will be represented in our model as a Type::KnownInstanceType(KnownInstanceType::UnionType(..)), where one of the elements in that union is a Type::KnownInstanceType(KnownInstanceType::TypeVar(..)): ISTM we need to account for the possibility of Type::KnownInstanceType(KnownInstanceType::TypeVar(..))s being in nested positions here, which will require recursing into the types rather than just using == checks
There was a problem hiding this comment.
Hmm yes I think you're right!
This was a lingering place where we were still using
TypeVarInstanceequality to check whether two typevars are the same. We need to fall back on comparing the typevaridentity, since that remains the same even after evaluating lazy defaults, bounds, or constraints.This was showing up as a bug on #20933, since the new constraint solver normalizes types as part of constructing constraint sets. That had the effect of making a
TypeVarinstance fail the argument/parameter assignability check after we apply the inferred specialization in something likesince the argument type has a lazy default, but the normalized/specialized parameter type ends up with the evaluated/eager default.