typing _Py_subs_paramters - #5880
Conversation
Sorry, something went wrong.
WalkthroughThis update refines the handling of unpacked generic alias arguments and type variable substitution in PyGenericAlias. It introduces new methods for tuple argument construction and unpacking, adjusts substitution logic for stricter argument matching, and modifies iteration to yield a single starred alias, aligning behavior more closely with CPython. 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. ❤️ Share 🪧 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
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)vm/src/builtins/genericalias.rs (1)📜 Review details410-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) {
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 9f3c34a and 8dbc58c.
⛔ Files ignored due to path filters (1)- Lib/test/test_typing.py is excluded by !Lib/**
- vm/src/builtins/genericalias.rs (5 hunks)
**/*.rs: Follow the default rustfmt code style (cargo fmt to 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
vm/src/stdlib/typevar.rs (8)⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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)
- 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)
vm/src/builtins/genericalias.rs (4)29-29: LGTM!
Adding __typing_unpacked_tuple_args__ to ATTR_EXCEPTIONS is consistent with the new getter method implementation.
99-116: LGTM!
The explicit initialization of starred to false improves clarity, and the new with_tuple_args constructor correctly follows the existing pattern while allowing explicit control of the starred flag.
184-192: LGTM!
The __typing_unpacked_tuple_args__ getter correctly exposes tuple arguments only when the alias is starred and its origin is tuple, aligning with CPython's behavior.
595-616: LGTM!
The modified iter implementation correctly yields a single starred GenericAlias instance, aligning with CPython's behavior. The TODO comment appropriately notes that this is a temporary implementation until gaiterobject is available.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)vm/src/builtins/genericalias.rs (2)📜 Review details391-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
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 77cc2c3 and fc78c43.
⛔ Files ignored due to path filters (1)- Lib/test/test_typing.py is excluded by !Lib/**
- vm/src/builtins/genericalias.rs (5 hunks)
**/*.rs: Follow the default rustfmt code style (cargo fmt to 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
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`
vm/src/stdlib/typevar.rs (8)⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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)
- 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)
vm/src/builtins/genericalias.rs (5)99-100: LGTM: Proper initialization of starred field.
The default starred: false initialization is correct and aligns with the struct definition.
103-116: LGTM: Well-structured constructor method.
The with_tuple_args method provides clean separation between regular construction and starred alias construction. The implementation correctly mirrors the existing new method while allowing explicit control over the starred flag.
184-191: LGTM: Correct implementation of unpacked tuple args property.
The conditional logic properly checks both the starred flag and tuple origin type before returning the args tuple. The implementation correctly returns None for non-qualifying cases.
408-502: LGTM: Excellent refactoring with improved algorithm structure.
The refactored subs_parameters function implements a clear 4-step algorithm that significantly improves the parameter substitution logic:
- Argument unpacking: Uses the new unpack_args function 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 unpacking
The error messages are more descriptive and the parameter name change from needle to item improves clarity. The TypeVarTuple handling logic is particularly well-implemented.
594-615: LGTM: Correct alignment with CPython iteration behavior.
The modified iter method correctly implements CPython's behavior by yielding a single starred GenericAlias instance 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 gaiterobject indicates future enhancement potential but doesn't affect the current correct implementation.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements