[red-knot] Implement disjointness for Instance types where the underlying class is @final#15539
[red-knot] Implement disjointness for Instance types where the underlying class is @final#15539AlexWaygood merged 6 commits intomainfrom
@final#15539Conversation
…rlying class is `@final`
AlexWaygood
requested review from
MichaReiser,
carljm and
sharkdp
as code owners
| (Type::SubclassOf(subclass_of_ty), instance @ Type::Instance(_)) | ||
| | (instance @ Type::Instance(_), Type::SubclassOf(subclass_of_ty)) => { | ||
| // `type[T]` is disjoint from `S`, where `S` is an instance type, | ||
| // if `U` is disjoint from `S`, | ||
| // where `U` represents all instances of `T`'s metaclass | ||
| let metaclass_instance = subclass_of_ty | ||
| .subclass_of() | ||
| .into_class() | ||
| .map(|class| class.metaclass(db).to_instance(db)) | ||
| .unwrap_or_else(|| KnownClass::Type.to_instance(db)); | ||
| instance.is_disjoint_from(db, metaclass_instance) | ||
| } |
There was a problem hiding this comment.
@carljm I realised almost as soon as our pairing session here that we were missing some subtle edge cases where a type[T] type could still be disjoint from an instance type even if the instance type was a subtype of type. So this logic is a little fiddlier than what we initially implemented. See the mdtest I've added for examples!
There was a problem hiding this comment.
Did a property test reveal this, or you just realized it looking at the code/tests?
There was a problem hiding this comment.
Just me reading the diff on github prior to filing the PR!
|
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Net reduction in lines of code (excluding tests), and improves correctness! Love to see it.
| (Type::SubclassOf(subclass_of_ty), instance @ Type::Instance(_)) | ||
| | (instance @ Type::Instance(_), Type::SubclassOf(subclass_of_ty)) => { | ||
| // `type[T]` is disjoint from `S`, where `S` is an instance type, | ||
| // if `U` is disjoint from `S`, | ||
| // where `U` represents all instances of `T`'s metaclass | ||
| let metaclass_instance = subclass_of_ty | ||
| .subclass_of() | ||
| .into_class() | ||
| .map(|class| class.metaclass(db).to_instance(db)) | ||
| .unwrap_or_else(|| KnownClass::Type.to_instance(db)); | ||
| instance.is_disjoint_from(db, metaclass_instance) | ||
| } |
There was a problem hiding this comment.
Did a property test reveal this, or you just realized it looking at the code/tests?
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_disjoint_from.md
Outdated
Show resolved
Hide resolved
| // `type[T]` is disjoint from `S`, where `S` is an instance type, | ||
| // if `U` is disjoint from `S`, | ||
| // where `U` represents all instances of `T`'s metaclass | ||
| let metaclass_instance = subclass_of_ty | ||
| .subclass_of() | ||
| .into_class() | ||
| .map(|class| class.metaclass(db).to_instance(db)) | ||
| .unwrap_or_else(|| KnownClass::Type.to_instance(db)); | ||
| instance.is_disjoint_from(db, metaclass_instance) |
There was a problem hiding this comment.
Nice! I like that we can delegate to instance-vs-instance, rather than checking finality here also.
It seems right that T actually disappears from this check, apart from its metaclass. There's no difference between type[object] and type[OtherClass] as regards disjointness from an instance type, as long as the metaclass of OtherClass is just type.
…es/is_disjoint_from.md Co-authored-by: Carl Meyer <[email protected]>
2 participants
Summary
Closes #15508
For any two instance types
TandS, we know they are disjoint if eitherTis final andTis not a subclass ofSorSis final andSis not a subclass ofT.Correspondingly, for any two types
type[T]andSwhereSis an instance type,type[T]can be said to be disjoint fromSifSis disjoint fromU, whereUis the type that represents all instances ofT's metaclass.And a heterogeneous tuple type can be said to be disjoint from an instance type if the instance type is disjoint from
tuple(a type representing all instances of thetupleclass at runtime).Test Plan
is_disjoint_from()tests are not written as mdtests just yet, but it's pretty hard to test some of these edge cases from a Rust unit test!QUICKCHECK_TESTS=1000000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stableCo-authored-by: Carl Meyer [email protected]