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 — Fix TypeParams, TypeAlias compile by youknowone · Pull Request #5862 · RustPython/RustPython Conversation
|
WalkthroughThe changes update the compilation and execution of Python's Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (2)
vm/src/frame.rs (1)
1269-1280: UsePyTupleRef::try_from_objectfor a cleaner & more permissive tuple-conversionThe new
None || tuplehandling looks correct 👍.
You can remove the manualdowncastboilerplate (and transparently accept tuple sub-classes, mirroring CPython) by leaning on the existing helper:- let type_params: PyTupleRef = if vm.is_none(&type_params_obj) { - // If None, use empty tuple (matching CPython's behavior) - vm.ctx.empty_tuple.clone() - } else { - type_params_obj - .downcast() - .map_err(|_| vm.new_type_error("Type params must be a tuple."))? - }; + let type_params: PyTupleRef = if vm.is_none(&type_params_obj) { + // If None, use the shared empty tuple (CPython behaviour) + vm.ctx.empty_tuple.clone() + } else { + PyTupleRef::try_from_object(vm, type_params_obj)? + };This keeps semantics identical while reducing code and aligning the acceptance rules with CPython’s (i.e. tuple subclasses are valid).
compiler/codegen/src/compile.rs (1)
1031-1076: LGTM! Solid PEP 695 TypeAlias implementation with minor optimization opportunity.The TypeAlias compilation logic correctly handles both cases:
- With type parameters: Proper symbol table management, type params compiled first (making them available for value expression), and correct stack order via
Rotate2- Without type parameters: Clean path that uses
Noneinstead of empty tuple for CPython compatibilityThe stack manipulation logic is sound and the comments clearly explain the PEP 695 compliance requirements.
Minor optimization: Consider avoiding the extra string clone:
let name_string = name.id.to_string(); - self.emit_load_const(ConstantData::Str { - value: name_string.clone().into(), - }); + self.emit_load_const(ConstantData::Str { + value: name_string.as_str().into(), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
compiler/codegen/src/compile.rs(1 hunks)vm/src/builtins/dict.rs(1 hunks)vm/src/builtins/memory.rs(2 hunks)vm/src/builtins/tuple.rs(0 hunks)vm/src/builtins/type.rs(4 hunks)vm/src/builtins/union.rs(3 hunks)vm/src/exceptions.rs(1 hunks)vm/src/frame.rs(1 hunks)vm/src/protocol/object.rs(1 hunks)vm/src/types/structseq.rs(1 hunks)
💤 Files with no reviewable changes (1)
- vm/src/builtins/tuple.rs
🧰 Additional context used
🧠 Learnings (2)
vm/src/builtins/memory.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Minimize modifications to CPython standard library files in the Lib/ directory; bug fixes should be made through Rust code modifications whenever possible.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Follow Rust best practices for error handling and memory management in all RustPython Rust code.
vm/src/builtins/type.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (15)
vm/src/builtins/type.rs (6)
161-168: New utility function for MRO-based subtype checking looks good.The
is_subtype_with_mrofunction provides a clean way to check subtype relationships by iterating through the MRO. The implementation correctly uses reference equality withitem.is(b).
209-213: Proper implementation of CPython's PyType_Check equivalent.The
checkmethod correctly implements downcasting to check if an object is an instance of PyType (or its subclass), matching CPython's PyType_Check macro behavior.
457-459: Well-implemented subtype checking method.The
is_subtypemethod properly uses the newis_subtype_with_mroutility with the MRO read guard. The implementation is clean and follows Rust patterns.
461-465: Correct implementation of exact type checking.The
check_exactmethod properly implements CPython's PyType_CheckExact behavior usingdowncast_ref_if_exact, which ensures the object is exactly a PyType and not a subclass.
470-470: Minor cleanup in type signature.Removing the fully qualified path
crate::from the type parameter is a good cleanup - the unqualifiedPyObjectis clearer and more consistent.
1244-1248: Excellent improvement to avoid recursion in subclass checking.The change to use
subclass.real_is_subclass(self.as_object(), vm)instead of callingfast_issubclassdirectly is a significant improvement. This matches CPython's behavior by using_PyObject_RealIsSubclassand prevents potential infinite recursion through__subclasscheck__.vm/src/types/structseq.rs (1)
100-100: Proper replacement of fast_getitem with direct indexing.The change from
zelf.fast_getitem(i.into())tozelf[i as usize].to_owned()correctly follows the systematic removal of thefast_getitemmethod. Direct indexing is more idiomatic and the.to_owned()maintains the same ownership semantics.vm/src/protocol/object.rs (1)
407-407: Correct replacement of fast_getitem with direct indexing.The change from
tuple.fast_getitem(0).clone()totuple[0].clone()properly replaces the method call with direct indexing. This is part of the systematic removal offast_getitemand maintains the same semantics.vm/src/builtins/memory.rs (2)
213-219: LGTM: Safe direct indexing replacementThe replacement of
fast_getitemwith direct indexingx[0].to_owned()is correct and safe. The bounds checkif x.len() == 1ensures the indexing won't panic, and the semantics are preserved.
1068-1074: LGTM: Consistent refactoring patternSame safe replacement pattern as the
unpack_singlemethod. Direct indexing with proper bounds checking maintains correctness while improving performance and consistency.vm/src/exceptions.rs (1)
1669-1671: LGTM: Safe tuple element accessThe replacement of
fast_getitemwith direct indexinglocation_tuple[i].to_owned()is safe due to the bounds checkif location_tup_len > i. This change maintains consistency with the broader codebase refactoring effort.vm/src/builtins/dict.rs (1)
238-243: LGTM: Clean refactoring from fast_getitem to direct indexing.The replacement of
fast_getitemcalls with direct tuple indexing is correct and maintains the same semantics. The tuple has already been validated to have length 2, making direct indexing safe.vm/src/builtins/union.rs (3)
43-46: LGTM: Useful addition for internal access.The new
get_argsmethod provides direct access to the internal args tuple, aligning with CPython's_Py_union_argsattribute. This is a reasonable addition for internal VM use.
202-202: LGTM: Correct refactoring from fast_getitem to direct indexing.The replacement of
args.fast_getitem(0)withargs[0].to_owned()is safe since this code path is only reached whenargs.len() == 1.
220-220: LGTM: Consistent tuple access pattern.The change from
new_args.fast_getitem(0)tonew_args[0].to_owned()maintains the same semantics and is safe in this context where we know the tuple is non-empty.
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.
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.