[ty] Improve handling of disjointness for NominalInstanceTypes and SubclassOfTypes#18864
Merged
AlexWaygood merged 23 commits intomainfrom Jun 24, 2025
Merged
[ty] Improve handling of disjointness for NominalInstanceTypes and SubclassOfTypes#18864AlexWaygood merged 23 commits intomainfrom
NominalInstanceTypes and SubclassOfTypes#18864AlexWaygood merged 23 commits intomainfrom
Conversation
AlexWaygood
changed the title
NominalInstanceTypes and SubclassOfTypes
Contributor
|
AlexWaygood
force-pushed
the
alex/disjointness
branch
2 times, most recently
from
3802c9e to
28a7054
Compare
AlexWaygood
commented
Jun 23, 2025
Member
Author
|
Primer analysis:
In general, I think this is all positive. Though the from c_extension import SomethingUnresolved
class Foo: ...
def f(x: Foo | SomethingUnresolved):
if isinstance(x, SomethingUnresolved):
reveal_type(x) # currently `Foo | Unknown`; `(Foo & Unknown) | Unknown` would probably respect the gradual guarantee better? |
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Member
Author
|
Some screenshots of what the
|
Contributor
Yes, I think we should -- it's clearly a violation of the gradual guarantee not to do so. |
Member
Author
See #18900 |
AlexWaygood
force-pushed
the
alex/disjointness
branch
2 times, most recently
from
220fcbf to
e703f0d
Compare
Member
|
I never heard the term solid base before and searching for it on google also doesn't give any good results. Is there another term we could use or can we explain the term or link to some reference documentation (or hint that it is explained in the rule's documentation) |
carljm
reviewed
Jun 23, 2025
crates/ty_python_semantic/resources/mdtest/narrow/conditionals/elif_else.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/narrow/type_guards.md
Outdated
Show resolved
Hide resolved
Contributor
AFAIK the term is only used internally in the CPython code base, not in user-facing docs or error messages. I think we should follow a similar pattern. The term (and concept) are useful in implementing the semantics, and the alternatives are more verbose, so we should go ahead and use the term internally (with a clear definition in a doc comment, which I elaborate on in a review comment above), but avoid the term in user-facing documentation or diagnostics (which I also addressed in review comments above.) |
Member
Author
Yeah, this stuff is really badly documented unfortunately. It's also an important part of Python's semantics (builtin types are common!), though, so I do think it's important that we emulate the semantics... I'll try to improve the docs along the lines of @carljm's suggestions. I think the best explanation I've found of the semantics might be this stack overflow answer: https://stackoverflow.com/a/48136198/13990016 |
AlexWaygood
force-pushed
the
alex/disjointness
branch
from
e703f0d to
e54b1a4
Compare
CodSpeed WallTime Performance ReportMerging #18864 will degrade performances by 4.79%Comparing Summary
Benchmarks breakdown
|
Member
Author
Co-authored-by: Carl Meyer <carl@astral.sh>
…into alex/disjointness
AlexWaygood
force-pushed
the
alex/disjointness
branch
from
4fdd7b6 to
242ccab
Compare
AlexWaygood
force-pushed
the
alex/disjointness
branch
from
5e902dd to
2a8e985
Compare
AlexWaygood
force-pushed
the
alex/disjointness
branch
from
3846c71 to
ad3d451
Compare
Co-authored-by: Carl Meyer <carl@astral.sh>
carljm
approved these changes
Jun 24, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Nice!
There's still an open comment about a rephrasing away from "solid base" in a code comment, but I don't think this is necessary, it's OK if we refer to solid bases in code.
...nstance_layout_conf…_-_Tests_for_ty's_`inst…_-_Built-ins_with_impli…_(f5857d64ce69ca1d).snap
Outdated
Show resolved
Hide resolved
carljm
approved these changes
Jun 24, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
The new diagnostic docs look great!
Member
Author
Great to hear -- thanks for the excellent review!! |
Member
Author
|
For posterity, this is what the final version of these diagnostics looks like:
|
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
3 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
This PR makes a number of improvements to our disjointness handling for instance types and
type[]types:The calculation of whether two nominal instance types are disjoint is moved from the somewhat-awkardly-named
NominalInstanceType::is_disjoint_from_nominal_instance_of_classfunction to a newClassLiteral::could_coexist_in_mro_with()method. This reflects the fact that the relation between types (the disjointness relation) is derived from a relation between runtime class objects (MRO compatibility). It also allows us to reuse the method for determining disjointness between twoSubclassOfTypes. Prior to now, that wouldn't have been helpful, but with the other improvements in this PR (see below), it is helpful.Following [red-knot] Report classes inheriting from bases with incompatible
__slots__#15129, we have understood that classes like this raiseTypeErrorat runtime, and have emitted errors if we see such class definitions in first-party code:However, we haven't understood that this also makes the types
AandBdisjoint: the intersectionA & Bsimplifies toNever, sinceAandBcan never both exist in any single MRO! This PR generalizes and incorporates our understanding of__slots__so that we now recognize any class with a non-empty__slots__definition as being a "solid base". If there are two solid basesAandB, andAis not a subclass ofBandBis not a subclass ofA,AandBwill be disjoint. Similarly, ifAhas a subclassCandBhas a subclassD,CandDwill be disjoint because they have disjoint solid bases in their respective MROs.Two bugs are also fixed in our handling of
__slots__. One long-standing one is that we used to think that this class definition raisedTypeError; this PR teaches ty that it doesn't:Another one was only introduced recently: see [ty] Improve handling of disjointness for
NominalInstanceTypes andSubclassOfTypes #18864 (comment)In addition to understanding that classes with non-empty
__slots__definitions are solid bases, this PR also adds an understanding to ty that certain builtin classes are also solid bases due to the special way that they are implemented in C. This is done via aKnownClass::is_solid_basemethod.Lastly, diagnostics for classes that have conflicting solid bases are improved:
TypeErrorTest Plan
QUICKCHECK_TESTS=100000 cargo test --release -p ty_python_semantic -- --ignored types::property_tests::stable