GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||
GeezerVison is showing github.com — typing _Py_subs_paramters by youknowone · Pull Request #5880 · RustPython/RustPython Conversation
|
WalkthroughThis update refines the handling of unpacked generic alias arguments and type variable substitution in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PyGenericAlias
participant VM
Caller->>PyGenericAlias: with_tuple_args(origin, args, starred, vm)
PyGenericAlias->>VM: (constructs alias with starred flag)
Caller->>PyGenericAlias: subs_parameters(alias, args, parameters, item, vm)
PyGenericAlias->>PyGenericAlias: unpack_args(item, vm)
PyGenericAlias->>PyGenericAlias: __typing_prepare_subst__ (on each parameter)
PyGenericAlias->>PyGenericAlias: Validate argument count
PyGenericAlias->>PyGenericAlias: Substitute parameters (using __typing_subst__ or subs_tvars)
PyGenericAlias->>Caller: Return substituted tuple
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
🔭 Outside diff range comments (1)
vm/src/builtins/genericalias.rs (1)
410-503: Fix multiple malformed comments throughout the function.The refactored logic is correct and well-structured with clear steps, but there are several malformed comments that need fixing:
Apply this diff to fix the comments:
- } else { - // Create a tuple with the single item's "O(O)" format - let tuple_args = PyTuple::new_ref(vec![item.clone()], &vm.ctx); + } else { + // Create a tuple with the single item + let tuple_args = PyTuple::new_ref(vec![item.clone()], &vm.ctx); for arg in args.iter() { - // Skip PyType objects + // Skip PyType objects - they can't be substituted if arg.class().is(vm.ctx.types.type_type) { - // Check if this is an unpacked TypeVarTuple's _is_unpacked_typevartuple + // Check if this is an unpacked TypeVarTuple let unpack = is_unpacked_typevartuple(arg, vm)?; - // Try __typing_subst__ method first, + // Try __typing_subst__ method first let substituted_arg = if let Ok(subst) = arg.get_attr(identifier!(vm, __typing_subst__), vm) { - // Find parameter index's tuple_index + // Find parameter index if let Some(iparam) = tuple_index(parameters.as_slice(), arg) { if unpack { - // Handle unpacked TypeVarTuple's tuple_extend + // Handle unpacked TypeVarTuple - extend with tuple elements if let Ok(tuple) = substituted_arg.try_to_ref::<PyTuple>(vm) {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_typing.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/builtins/genericalias.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/builtins/genericalias.rs
🧬 Code Graph Analysis (1)
vm/src/builtins/genericalias.rs (1)
vm/src/stdlib/typevar.rs (8)
args(190-190)args(422-428)__typing_prepare_subst__(183-229)__typing_prepare_subst__(516-524)__typing_prepare_subst__(700-708)__typing_subst__(158-165)__typing_subst__(506-513)__typing_subst__(695-697)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (4)
vm/src/builtins/genericalias.rs (4)
29-29: LGTM!Adding
__typing_unpacked_tuple_args__toATTR_EXCEPTIONSis consistent with the new getter method implementation.
99-116: LGTM!The explicit initialization of
starredtofalseimproves clarity, and the newwith_tuple_argsconstructor correctly follows the existing pattern while allowing explicit control of thestarredflag.
184-192: LGTM!The
__typing_unpacked_tuple_args__getter correctly exposes tuple arguments only when the alias is starred and its origin istuple, aligning with CPython's behavior.
595-616: LGTM!The modified
iterimplementation correctly yields a single starredGenericAliasinstance, aligning with CPython's behavior. The TODO comment appropriately notes that this is a temporary implementation untilgaiterobjectis available.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
8dbc58c to
77cc2c3
Compare
77cc2c3 to
fc78c43
Compare
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: 0
🧹 Nitpick comments (2)
vm/src/builtins/genericalias.rs (2)
391-391: Fix malformed comment.The comment contains incomplete text ending with "PyList_SetSlice" which appears to be a remnant from CPython code and doesn't make sense in this Rust context.
Apply this diff to fix the comment:
- // Safe to unpack - add all elements's PyList_SetSlice + // Safe to unpack - add all elements
401-401: Fix malformed comment.The comment contains incomplete text ending with "PyList_Append" which appears to be a remnant from CPython code and doesn't make sense in this Rust context.
Apply this diff to fix the comment:
- // Default case: add the item as-is's PyList_Append + // Default case: add the item as-is
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_typing.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/builtins/genericalias.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/builtins/genericalias.rs
🧠 Learnings (1)
vm/src/builtins/genericalias.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to Lib/test/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to tests in `Lib/test`
🧬 Code Graph Analysis (1)
vm/src/builtins/genericalias.rs (1)
vm/src/stdlib/typevar.rs (8)
args(190-190)args(422-428)__typing_prepare_subst__(183-229)__typing_prepare_subst__(516-524)__typing_prepare_subst__(700-708)__typing_subst__(158-165)__typing_subst__(506-513)__typing_subst__(695-697)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (5)
vm/src/builtins/genericalias.rs (5)
99-100: LGTM: Proper initialization of starred field.The default
starred: falseinitialization is correct and aligns with the struct definition.
103-116: LGTM: Well-structured constructor method.The
with_tuple_argsmethod provides clean separation between regular construction and starred alias construction. The implementation correctly mirrors the existingnewmethod while allowing explicit control over thestarredflag.
184-191: LGTM: Correct implementation of unpacked tuple args property.The conditional logic properly checks both the
starredflag and tuple origin type before returning the args tuple. The implementation correctly returnsNonefor non-qualifying cases.
408-502: LGTM: Excellent refactoring with improved algorithm structure.The refactored
subs_parametersfunction implements a clear 4-step algorithm that significantly improves the parameter substitution logic:
- Argument unpacking: Uses the new
unpack_argsfunction for proper handling- Parameter preparation: Calls
__typing_prepare_subst__with proper argument formatting- Validation: Enforces exact argument count matching with improved error messages
- Substitution: Handles both
__typing_subst__and recursive substitution with proper TypeVarTuple unpackingThe error messages are more descriptive and the parameter name change from
needletoitemimproves clarity. The TypeVarTuple handling logic is particularly well-implemented.
594-615: LGTM: Correct alignment with CPython iteration behavior.The modified
itermethod correctly implements CPython's behavior by yielding a single starredGenericAliasinstance instead of directly iterating over the args tuple. The implementation properly:
- Creates a starred alias using
with_tuple_args- Wraps it in a tuple for iteration
- Returns the appropriate iterator
The TODO comment about
gaiterobjectindicates future enhancement potential but doesn't affect the current correct implementation.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.