-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[red-knot] support typing.Union
in type annotations
#14499
Conversation
Is it possible to auto tag my PR as redknot by using a specific word in the description or title? |
typing.Union
in type annotations
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few small things.
crates/red_knot_python_semantic/resources/mdtest/annotations/union.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/union.md
Outdated
Show resolved
Hide resolved
…nion.md Co-authored-by: Carl Meyer <[email protected]>
…nion.md Co-authored-by: Carl Meyer <[email protected]>
No, unfortunately we don't have anything like that set up. It would be nice to have! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! Will commit my one remaining comment tweak, and then merge!
crates/red_knot_python_semantic/resources/mdtest/annotations/union.md
Outdated
Show resolved
Hide resolved
Noticed a couple more small changes on reviewing the latest, pushed those too... |
KnownInstanceType::Union => match parameters { | ||
ast::Expr::Tuple(t) => UnionType::from_elements( | ||
self.db, | ||
t.iter().map(|elt| self.infer_type_expression(elt)), | ||
), | ||
_ => self.infer_type_expression(parameters), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extremely subtle, but with this change, we now fail to infer a type for the ast::Expr::Tuple
expression itself. We only infer types for the sub-expressions via self.infer_type_expression(elt)
, but we don't store a type for the overall ast::Expr::Tuple
(only for the whole slice expression).
In other words, when we have something like Union[str, int]
, we do infer types for the whole slice expression Union[str, int]
. We also infer types for int
and str
, but we do not infer/store a type for the int, str
tuple expression.
Unfortunately, the test that would have caught this is currently deactivated (see #14391), but I hope we can re-activate it soon.
I'll open a PR with a fix (Edit: #14510).
) ## Summary Fixes a panic related to sub-expressions of `typing.Union` where we fail to store a type for the `int, str` tuple-expression in code like this: ``` x: Union[int, str] = 1 ``` relates to [my comment](#14499 (comment)) on #14499. ## Test Plan New corpus test
Fix #14498
Summary
This PR adds
typing.Union
supportTest Plan
I created new tests in mdtest.