-
Notifications
You must be signed in to change notification settings - Fork 1.4k
support | operation between typing.Union and strings #6983
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
Conversation
📝 WalkthroughWalkthroughType's Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Type as crates::vm::builtins::type::or_
participant Union as crates::vm::builtins::union::or_op
participant Typing as typing._type_check
participant MakeUnion as make_union
Caller->>Type: or_(left, right)
Type->>Union: or_op(left, right, vm)
Union->>Union: has_union_operands / is_unionable fast-path
alt fast-path
Union-->>Type: use operand(s) directly
else fallback
Union->>Typing: call typing._type_check(arg, message)
Typing-->>Union: validated/adjusted arg or error
end
Union->>MakeUnion: make_union(tuple_of_two_args)
MakeUnion-->>Union: union object
Union-->>Type: return union
Type-->>Caller: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/union.rs`:
- Around line 206-214: The helper function _call_typing_func_object is
duplicated in union.rs, typing.rs, and typevar.rs; extract it into a single pub
function at module level in typing.rs (make the existing implementation public,
e.g., pub fn _call_typing_func_object(...)) and remove the duplicate definitions
in union.rs and typevar.rs, replacing them with a use/import of the shared
function (call the same symbol _call_typing_func_object) and call it where
needed; ensure the signature stays compatible with VirtualMachine, AsPyStr and
IntoFuncArgs so existing call sites (including code using TypeAliasType in
union.rs) compile without further edits.
🧹 Nitpick comments (1)
crates/vm/src/builtins/union.rs (1)
240-252: Consider using the?operator for cleaner error propagation.The match statements can be simplified using Rust's
?operator.♻️ Suggested simplification
- let left = match type_check(zelf, vm) { - Ok(v) => v, - err => { - return err; - } - }; - - let right = match type_check(other, vm) { - Ok(v) => v, - err => { - return err; - } - }; + let left = type_check(zelf, vm)?; + let right = type_check(other, vm)?;
crates/vm/src/builtins/union.rs
Outdated
| let left = match type_check(zelf, vm) { | ||
| Ok(v) => v, | ||
| err => { | ||
| return err; | ||
| } | ||
| }; |
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.
| let left = match type_check(zelf, vm) { | |
| Ok(v) => v, | |
| err => { | |
| return err; | |
| } | |
| }; | |
| let left = type_check(zelf, vm)?; |
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.
Ahh, yes. Changed to use ?.
Move _call_typing_func_object() code to stdlib::typing::call_typing_func_object(). Use that function everywhere.
Adds support for performing '|' operation between Union objects and
strings, e.g. forward type references.
For example following code:
from typing import Union
U1 = Union[int, str]
U1 | "float"
The result of the operation above becomes:
int | str | ForwardRef('float')
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/typing.py dependencies:
dependent tests: (10 tests)
Legend:
|
youknowone
left a comment
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.
@elmjag Thank you so much for contributing! And welcome to RustPython project.
moreal
left a comment
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.
Thank you for contribution!
Adds support for performing '|' operation between Union objects and strings, e.g. forward type references.
For example following code:
The result of the operation above becomes:
int | str | ForwardRef('float')Summary by CodeRabbit