[ty] Detect invalid partially stringified PEP-604 unions#23285
Merged
AlexWaygood merged 8 commits intomainfrom Mar 6, 2026
Merged
[ty] Detect invalid partially stringified PEP-604 unions#23285AlexWaygood merged 8 commits intomainfrom
AlexWaygood merged 8 commits intomainfrom
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 87.07% to 87.10%. The percentage of expected errors that received a diagnostic increased from 77.62% to 77.81%. SummaryHow are test cases classified?Each test case represents one expected error annotation or a group of annotations sharing a tag. Counts are per test case, not per diagnostic — multiple diagnostics on the same line count as one. Required annotations (
Test file breakdown1 file altered
True positives added (2)2 diagnostics
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
|
82402fc to
46cd0f0
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unsupported-operator |
286 | 0 | 0 |
invalid-await |
40 | 0 | 0 |
unused-type-ignore-comment |
0 | 3 | 0 |
invalid-return-type |
1 | 0 | 0 |
| Total | 327 | 3 | 0 |
This comment was marked as resolved.
This comment was marked as resolved.
32606e9 to
65e68c5
Compare
carljm
approved these changes
Feb 16, 2026
crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
Outdated
Show resolved
Hide resolved
Contributor
|
Oh, Codex found a regression here that we should add a test for and fix before landing; this PR wrongly emits a diagnostic on this: def f(x: "int | 'Foo'"): ...Fix is to skip the diagnostic if |
AlexWaygood
commented
Feb 16, 2026
AlexWaygood
commented
Feb 16, 2026
AlexWaygood
commented
Feb 16, 2026
Comment on lines
+166
to
+168
| | Self::Unknown | ||
| | Self::AlwaysTruthy | ||
| | Self::AlwaysFalsy |
Member
Author
There was a problem hiding this comment.
it's necessary to change the fallback types of these special forms now, or we start complaining that they can't be used in | unions on Python <3.14
05cfb07 to
38f53e9
Compare
AlexWaygood
commented
Feb 20, 2026
4fb14d6 to
c23ef35
Compare
c23ef35 to
288f4c8
Compare
carljm
approved these changes
Mar 3, 2026
Contributor
carljm
left a comment
There was a problem hiding this comment.
Nice! Sorry for the delayed review.
| x: int | ||
|
|
||
| class Meta(type): | ||
| def __or__(cls, other: str) -> str: |
Contributor
There was a problem hiding this comment.
This is an invalid override of type.__or__; not sure why we don't catch it?
The invalid override part is the return type, which doesn't seem essential to this test.
In fact I don't even think defining __or__ here is necessary to the test, since type.__or__ accepts Any for its argument. But maybe the test is clearer with it.
Member
Author
There was a problem hiding this comment.
This is an invalid override of
type.__or__; not sure why we don't catch it?
hmmm... it's something to do with the | _typeshed.Self union in the return type of type.__or__. _typeshed.Self is just a common-or-garden TypeVar; I can repro with just this:
from typing import TypeVar
T = TypeVar("T")
class Foo1:
def method(self: T) -> T:
return self
class Foo2(Foo1):
def method(self) -> str:
return ""
Member
Author
There was a problem hiding this comment.
I suspect it's due to the fact that we check the types of methods as accessed on instances when doing Liskov checks
Member
Author
There was a problem hiding this comment.
But for now I changed the return annotation in this test
crates/ty_python_semantic/resources/mdtest/annotations/string.md
Outdated
Show resolved
Hide resolved
carljm
requested changes
Mar 3, 2026
Comment on lines
+162
to
+165
| self.infer_expression(&binary.left, TypeContext::default()) | ||
| }; | ||
| let right_type_value = | ||
| self.infer_expression(&binary.right, TypeContext::default()); |
Contributor
There was a problem hiding this comment.
Oh, I think there is a problem here if there are other diagnostics that will show up either in type-expression or value-expression inference, for example an undefined name; we now get those diagnostics twice. For example in def f(x: int | Foo): ... if Foo is undefined, we now get duplicate diagnostics.
This seems a bit nettlesome to fix. It might require adding a no-diagnostics mode to TypeInferenceBuilder?
Member
Author
There was a problem hiding this comment.
This seems a bit nettlesome to fix. It might require adding a no-diagnostics mode to
TypeInferenceBuilder?
Not that nettlesome at all! Ibraheem already added it for his previous multi-inference shenanigans; it turns out we already had all the infrastructure we needed to easily fix this.
Contributor
|
I think we could consider going back to your first implementation here instead of fixing my latest comment? :/ Just not sure how much extra work it's worth to do better here. |
AlexWaygood
commented
Mar 5, 2026
carljm
reviewed
Mar 6, 2026
| let right_ty = self.infer_type_expression(&binary.right); | ||
|
|
||
| // Detect runtime errors from e.g. `int | "bytes"` on Python <3.14 without `__future__` annotations. | ||
| if !self.deferred_state.is_deferred() { |
Contributor
There was a problem hiding this comment.
Should we have an exemption here for if TYPE_CHECKING: blocks?
Member
Author
There was a problem hiding this comment.
Sure, I can add that. Though there's really no reason ever to have partially stringized PEP-604 unions in your code; it's just an antipattern, especially in regions that don't require any stringized annotations at all (because they will never even be executed at runtime).
But you're right that we may as well avoid false positives here.
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
2 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
PEP-604 unions such as
"int" | strfail at runtime on Python <3.14 without__future__annotations. There are conformance-suite tests that mandate that we should detect this runtime error even inside type expressions.This PR employs multi-inference in order to detect these errors, even inside type expressions.
Test Plan
Mdtests and snapshots added and updated.