Integrate PyTupleTyped into PyTuple - #5959
Conversation
Sorry, something went wrong.
WalkthroughThe changes refactor the handling of typed Python tuples and related generics throughout the codebase. The PyTupleTyped struct is removed and replaced by a generic PyTuple<R> parameterized over element type, using PyRef<T> payloads. Conversion logic is updated to use reference-counted pointers (PyRef) for tuple elements. This affects function closures, type slots, and import operations, with corresponding updates to trait bounds, type conversions, and trait implementations for payloads and traversal. Additional changes introduce a standardized method for obtaining payload type IDs and enforce stricter trait bounds for object payloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Frame as ExecutingFrame
participant VM as VirtualMachine
participant Tuple as PyTuple<PyRef<T>>
participant Function as PyFunction
Frame->>Frame: pop_value() for closure or from_list
Frame->>Tuple: PyRef::<PyTuple<PyRef<...>>>::try_from_object(...)
Frame->>Function: PyFunction::new(..., closure: Option<PyRef<PyTuple<PyCellRef>>>)
Frame->>VM: import_from(..., from_list: &Py<PyTuple<PyRef<PyStrRef>>>, ...)
VM->>Tuple: Context::empty_tuple_typed()
VM->>VM: import_inner(..., from_list: &Py<PyTuple<PyRef<PyStrRef>>>, ...)
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
🧹 Nitpick comments (2)vm/src/frame.rs (1)📜 Review detailsvm/src/builtins/tuple.rs (1)1349-1355: Good refactor aligning with PyRef pattern, but consider error handling improvement.
The changes correctly implement the new PyRef<PyTupleTyped<PyStrRef>> pattern and improve code clarity by separating the pop operation from the conversion. The reference passing to import_from is also consistent with the broader refactor.
Consider making the empty tuple handling more explicit:
- let from_list = <Option<PyRef<PyTupleTyped<PyStrRef>>>>::try_from_object(vm, top)? - .unwrap_or_else(|| PyTupleTyped::<PyStrRef>::empty(&vm.ctx).to_owned()); + let from_list = <Option<PyRef<PyTupleTyped<PyStrRef>>>>::try_from_object(vm, top)? + .unwrap_or_else(|| PyTupleTyped::<PyStrRef>::empty(&vm.ctx).to_owned());This makes the ownership transfer of the empty tuple more explicit and matches the pattern used elsewhere in the codebase.
561-573: Consider relaxing trait bounds on empty() method.
The trait bounds T: fmt::Debug + Send + Sync + 'static seem unnecessary for an empty tuple since there are no elements to satisfy these constraints. These bounds might unnecessarily restrict usage of the empty tuple.
Consider removing or relaxing these bounds:
- pub fn empty(ctx: &Context) -> &Py<Self> - where - T: fmt::Debug + Send + Sync + 'static, - { + pub fn empty(ctx: &Context) -> &Py<Self> {
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 8ab7aa2 and a52fc91.
📒 Files selected for processing (8)- derive-impl/src/pytraverse.rs (1 hunks)
- vm/src/builtins/function.rs (4 hunks)
- vm/src/builtins/tuple.rs (3 hunks)
- vm/src/builtins/type.rs (3 hunks)
- vm/src/frame.rs (2 hunks)
- vm/src/object/core.rs (1 hunks)
- vm/src/object/payload.rs (2 hunks)
- vm/src/vm/mod.rs (4 hunks)
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
vm/src/builtins/tuple.rs (1)vm/src/builtins/tuple.rs (3)vm/src/object/payload.rs (1)
- payload_type_id (515-517)
- payload_type_id (23-25)
vm/src/object/payload.rs (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)vm/src/object/core.rs (4)
- payload_type_id (23-25)
vm/src/object/traverse.rs (1)
- TypeId (451-451)
- traverse (127-129)
- traverse (135-137)
- Self (373-376)
- try_traverse (17-17)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
vm/src/object/core.rs (1)657-659: LGTM! Type identification standardization implemented correctly.
The change from TypeId::of::<T>() to T::payload_type_id() properly utilizes the new standardized method from the PyPayload trait. This centralizes type identity checks and aligns with the broader refactoring to unify payload type handling across the VM.
derive-impl/src/pytraverse.rs (2)108-117: LGTM! Proper trait bounds added for generic traversal.
The automatic addition of Traverse bounds to all generic type parameters ensures that the derived Traverse implementation is valid for generic types. The use of split_for_impl() correctly handles the generics splitting for the macro-generated code.
120-120: Correctly applies generics and bounds to the implementation.
The generated unsafe impl block now properly includes the generics and where clause, ensuring type safety for the derived traversal implementations.
vm/src/object/payload.rs (2)22-25: LGTM! Standardized type ID method implemented correctly.
The new payload_type_id() method provides a centralized and standardized way to obtain type identifiers for payload types. The inline annotation is appropriate for this frequently-used method.
82-82: Proper trait hierarchy established.
Expanding PyObjectPayload to require PyPayload ensures that all object payloads provide the standardized payload_type_id() method, creating a consistent trait hierarchy across the VM.
vm/src/builtins/type.rs (4)65-65: LGTM! Reference-counted tuple field type updated correctly.
The change from Option<PyTupleTyped<PyStrRef>> to Option<PyRef<PyTupleTyped<PyStrRef>>> aligns with the broader refactoring to use reference-counted pointers for tuple elements, improving type safety and consistency.
1039-1039: Variable type correctly updated to match field type.
The heaptype_slots variable type is properly updated to Option<PyRef<PyTupleTyped<PyStrRef>>> to match the corresponding field type change.
1042-1046: Conversion logic updated for single string slots.
The PyRef::<PyTupleTyped<PyStrRef>>::try_from_object call correctly handles the conversion for single string slots with the new reference-counted tuple type.
1055-1058: Conversion logic updated for iterable slots.
The PyRef::<PyTupleTyped<PyStrRef>>::try_from_object call correctly handles the conversion for iterable slots with the new reference-counted tuple type.
vm/src/frame.rs (1)1843-1843: Correctly updates closure type to align with PyRef pattern.
The change from PyTupleTyped<PyCellRef> to PyRef<PyTupleTyped<PyCellRef>> is consistent with the broader refactor described in the summary and properly maintains the reference-counted semantics for tuple elements.
vm/src/builtins/function.rs (4)14-17: LGTM! Import reorganization aligns with module structure.
The relocation of TryFromObject and ToPyObject to the main import block is appropriate for their usage patterns.
35-35: Good architectural change to use reference-counted tuples.
The change from Option<PyTupleTyped<PyCellRef>> to Option<PyRef<PyTupleTyped<PyCellRef>>> aligns with Python's reference-counting semantics and improves memory efficiency by avoiding unnecessary tuple cloning.
61-61: Constructor signature correctly updated to match field type.
The closure parameter type change is consistent with the field declaration, and the usage at line 356 properly handles the reference semantics.
Also applies to: 356-356
616-617: Conversion correctly updated for PyRef-wrapped typed tuples.
The change to use PyRef::<PyTupleTyped<PyCellRef>>::try_from_object properly converts the closure tuple to the expected reference-counted typed tuple format.
vm/src/vm/mod.rs (4)602-602: Good use of singleton empty tuple pattern.
Using PyTupleTyped::<PyStrRef>::empty(&self.ctx) leverages the singleton pattern for empty tuples, improving memory efficiency.
612-612: Efficient parameter passing with reference instead of owned value.
The change to accept &Py<PyTupleTyped<PyStrRef>> avoids unnecessary cloning and follows Rust's borrowing best practices.
622-622: Parameter type consistently updated in import_inner.
The reference parameter is properly used throughout the method, including the is_empty() check at line 627.
Also applies to: 627-627
660-660: Correct ownership conversion for function call.
The to_owned().to_pyobject(self) properly converts the borrowed reference to an owned PyObjectRef as required by the import function call.
vm/src/builtins/tuple.rs (5)503-508: Struct refactoring improves type safety and ownership model.
The changes establish a clearer ownership model:
- Storing PyTuple directly instead of PyTupleRef simplifies the internal representation
- The generic parameter R with TransmuteFromObject bound ensures type safety for transmutation
- The safety invariant comment properly documents the transmutation requirements
510-522: Well-designed PyPayload implementation for type identity.
The implementation correctly:
- Restricts PyPayload to PyTupleTyped<PyRef<T>> ensuring type safety
- Returns PyTuple's type ID for proper type identity checks
- Maintains compatibility with the Python type system
524-538: Correct garbage collection traversal implementation.
The Traverse and MaybeTraverse implementations properly handle GC traversal by:
- Delegating to the underlying tuple's traverse method
- Setting IS_TRACE = true to ensure typed tuples are always traced
- Adding appropriate trait bounds for type safety
554-558: AsRef implementation correctly updated for PyRef elements.
The change to return &[PyRef<T>] aligns with the new type structure. The unsafe cast in as_slice() is justified by the transmutation safety requirements documented in the struct.
Also applies to: 577-577
597-602: Clean conversion trait implementation.
The From trait implementation provides a convenient and safe way to convert typed tuple references back to regular tuple references using the into_untyped() method.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)vm/src/builtins/tuple.rs (1)📜 Review details516-521: Add missing #[repr(transparent)] attribute for safe transmutation.
The PyTupleTyped struct uses unsafe transmute operations throughout the code but lacks the #[repr(transparent)] attribute that is required for these operations to be sound.
+#[repr(transparent)] pub struct PyTupleTyped<R> { // SAFETY INVARIANT: T must be repr(transparent) over PyObjectRef, and the // elements must be logically valid when transmuted to T tuple: PyTuple, _marker: PhantomData<R>, }
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 0261ba2 and fff5446.
📒 Files selected for processing (9)- vm/src/builtins/function.rs (5 hunks)
- vm/src/builtins/tuple.rs (4 hunks)
- vm/src/builtins/type.rs (3 hunks)
- vm/src/convert/try_from.rs (1 hunks)
- vm/src/frame.rs (3 hunks)
- vm/src/object/core.rs (10 hunks)
- vm/src/object/traverse_object.rs (2 hunks)
- vm/src/vm/context.rs (2 hunks)
- vm/src/vm/mod.rs (4 hunks)
- vm/src/object/traverse_object.rs
- vm/src/vm/context.rs
- vm/src/convert/try_from.rs
- vm/src/frame.rs
- vm/src/builtins/type.rs
- vm/src/vm/mod.rs
- vm/src/builtins/function.rs
- vm/src/object/core.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
vm/src/object/traverse.rs (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)vm/src/convert/transmute_from.rs (2)
- try_traverse (17-17)
- check (13-13)
- check (17-28)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
vm/src/builtins/tuple.rs (4)6-6: LGTM! Import added for new trait implementation.
The MaybeTraverse import is correctly added to support the new trait implementation in the code.
523-537: LGTM! Trait implementations correctly simplified.
The Traverse implementation is properly constrained with R: TransmuteFromObject, and the new MaybeTraverse implementation correctly delegates to the Traverse implementation when both traits are available.
577-598: LGTM! Trait implementations and methods are correctly implemented.
The AsRef trait implementation and the accessor methods (as_slice, len, is_empty) are properly implemented using safe operations that delegate to the underlying tuple.
600-611: LGTM! Debug and From implementations are correctly implemented.
The Debug implementation correctly delegates to the tuple's slice formatting, and the From trait implementation properly uses the safe into_untyped method.
Sorry, something went wrong.
| impl<T: PyPayload> PyTupleTyped<PyRef<T>> { | ||
| pub fn new_ref(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<Self> { | ||
| unsafe { | ||
| let elements: Vec<PyObjectRef> = | ||
| std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements); | ||
| let tuple = PyTuple::new_ref(elements, ctx); | ||
| tuple.into_typed_unchecked::<T>() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify transmute safety and add documentation.
The new_ref constructor uses unsafe transmute to convert between Vec<PyRef<T>> and Vec<PyObjectRef>. While this should be safe given that PyRef<T> should have the same representation as PyObjectRef, it needs proper safety documentation.
impl<T: PyPayload> PyTupleTyped<PyRef<T>> {
+ /// Creates a new typed tuple from a vector of typed references.
+ ///
+ /// # Safety
+ ///
+ /// This method relies on `PyRef<T>` having the same memory layout as `PyObjectRef`.
+ /// The transmute is safe because `PyRef<T>` is a thin wrapper around `PyObjectRef`
+ /// with compile-time type guarantees.
pub fn new_ref(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<Self> {
unsafe {
+ // SAFETY: PyRef<T> has the same layout as PyObjectRef
let elements: Vec<PyObjectRef> =
std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements);
let tuple = PyTuple::new_ref(elements, ctx);
tuple.into_typed_unchecked::<T>()
}
}
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl<T: PyPayload> PyTupleTyped<PyRef<T>> { | |
| pub fn new_ref(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<Self> { | |
| unsafe { | |
| let elements: Vec<PyObjectRef> = | |
| std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements); | |
| let tuple = PyTuple::new_ref(elements, ctx); | |
| tuple.into_typed_unchecked::<T>() | |
| } | |
| } | |
| } | |
| impl<T: PyPayload> PyTupleTyped<PyRef<T>> { | |
| /// Creates a new typed tuple from a vector of typed references. | |
| /// | |
| /// # Safety | |
| /// | |
| /// This method relies on `PyRef<T>` having the same memory layout as `PyObjectRef`. | |
| /// The transmute is safe because `PyRef<T>` is a thin wrapper around `PyObjectRef` | |
| /// with compile-time type guarantees. | |
| pub fn new_ref(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<Self> { | |
| unsafe { | |
| // SAFETY: `PyRef<T>` has the same layout as `PyObjectRef` | |
| let elements: Vec<PyObjectRef> = | |
| std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements); | |
| let tuple = PyTuple::new_ref(elements, ctx); | |
| tuple.into_typed_unchecked::<T>() | |
| } | |
| } | |
| } |
In vm/src/builtins/tuple.rs around lines 539 to 548, the use of unsafe transmute to convert Vec<PyRef<T>> to Vec<PyObjectRef> requires explicit safety documentation. Add a comment explaining why this transmute is safe, specifically that PyRef<T> and PyObjectRef have the same memory representation, and ensure the reasoning is clear to maintainers. This will clarify the safety assumptions behind the unsafe block.
Sorry, something went wrong.
| impl<T: PyPayload> PyRef<PyTupleTyped<PyRef<T>>> { | ||
| pub fn into_pyobject(self) -> PyObjectRef { | ||
| self.into_untyped().into() | ||
| } | ||
|
|
||
| pub fn into_untyped(self) -> PyRef<PyTuple> { | ||
| // SAFETY: PyTupleTyped is transparent over PyTuple | ||
| unsafe { std::mem::transmute::<PyRef<PyTupleTyped<PyRef<T>>>, PyRef<PyTuple>>(self) } | ||
| } | ||
|
|
||
| pub fn try_from_untyped(tuple: PyTupleRef, vm: &VirtualMachine) -> PyResult<Self> { | ||
| // Check that all elements are of the correct type | ||
| for elem in tuple.as_slice() { | ||
| <PyRef<T> as TransmuteFromObject>::check(vm, elem)?; | ||
| } | ||
| // SAFETY: We just verified all elements are of type T, and PyTupleTyped has the same layout as PyTuple | ||
| Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTupleTyped<PyRef<T>>>>(tuple) }) | ||
| } | ||
| } | ||
|
|
||
| impl<T: PyPayload> Py<PyTupleTyped<PyRef<T>>> { | ||
| pub fn as_untyped(&self) -> &Py<PyTuple> { | ||
| // SAFETY: PyTupleTyped is transparent over PyTuple | ||
| unsafe { std::mem::transmute::<&Py<PyTupleTyped<PyRef<T>>>, &Py<PyTuple>>(self) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance safety documentation for transmute operations.
The conversion methods use multiple unsafe transmute operations. While the safety comments are present, they should be more comprehensive to justify the safety of these operations.
impl<T: PyPayload> PyRef<PyTupleTyped<PyRef<T>>> {
pub fn into_pyobject(self) -> PyObjectRef {
self.into_untyped().into()
}
pub fn into_untyped(self) -> PyRef<PyTuple> {
- // SAFETY: PyTupleTyped is transparent over PyTuple
+ // SAFETY: PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple,
+ // so the memory layout is identical and transmute is safe
unsafe { std::mem::transmute::<PyRef<PyTupleTyped<PyRef<T>>>, PyRef<PyTuple>>(self) }
}
pub fn try_from_untyped(tuple: PyTupleRef, vm: &VirtualMachine) -> PyResult<Self> {
// Check that all elements are of the correct type
for elem in tuple.as_slice() {
<PyRef<T> as TransmuteFromObject>::check(vm, elem)?;
}
- // SAFETY: We just verified all elements are of type T, and PyTupleTyped has the same layout as PyTuple
+ // SAFETY: We verified all elements are valid PyRef<T> instances via TransmuteFromObject::check,
+ // and PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple with identical memory layout
Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTupleTyped<PyRef<T>>>>(tuple) })
}
}
impl<T: PyPayload> Py<PyTupleTyped<PyRef<T>>> {
pub fn as_untyped(&self) -> &Py<PyTuple> {
- // SAFETY: PyTupleTyped is transparent over PyTuple
+ // SAFETY: PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple,
+ // so reference transmutation preserves memory layout and lifetime
unsafe { std::mem::transmute::<&Py<PyTupleTyped<PyRef<T>>>, &Py<PyTuple>>(self) }
}
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl<T: PyPayload> PyRef<PyTupleTyped<PyRef<T>>> { | |
| pub fn into_pyobject(self) -> PyObjectRef { | |
| self.into_untyped().into() | |
| } | |
| pub fn into_untyped(self) -> PyRef<PyTuple> { | |
| // SAFETY: PyTupleTyped is transparent over PyTuple | |
| unsafe { std::mem::transmute::<PyRef<PyTupleTyped<PyRef<T>>>, PyRef<PyTuple>>(self) } | |
| } | |
| pub fn try_from_untyped(tuple: PyTupleRef, vm: &VirtualMachine) -> PyResult<Self> { | |
| // Check that all elements are of the correct type | |
| for elem in tuple.as_slice() { | |
| <PyRef<T> as TransmuteFromObject>::check(vm, elem)?; | |
| } | |
| // SAFETY: We just verified all elements are of type T, and PyTupleTyped has the same layout as PyTuple | |
| Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTupleTyped<PyRef<T>>>>(tuple) }) | |
| } | |
| } | |
| impl<T: PyPayload> Py<PyTupleTyped<PyRef<T>>> { | |
| pub fn as_untyped(&self) -> &Py<PyTuple> { | |
| // SAFETY: PyTupleTyped is transparent over PyTuple | |
| unsafe { std::mem::transmute::<&Py<PyTupleTyped<PyRef<T>>>, &Py<PyTuple>>(self) } | |
| } | |
| } | |
| impl<T: PyPayload> PyRef<PyTupleTyped<PyRef<T>>> { | |
| pub fn into_pyobject(self) -> PyObjectRef { | |
| self.into_untyped().into() | |
| } | |
| pub fn into_untyped(self) -> PyRef<PyTuple> { | |
| // SAFETY: PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple, | |
| // so the memory layout is identical and transmute is safe | |
| unsafe { std::mem::transmute::<PyRef<PyTupleTyped<PyRef<T>>>, PyRef<PyTuple>>(self) } | |
| } | |
| pub fn try_from_untyped(tuple: PyTupleRef, vm: &VirtualMachine) -> PyResult<Self> { | |
| // Check that all elements are of the correct type | |
| for elem in tuple.as_slice() { | |
| <PyRef<T> as TransmuteFromObject>::check(vm, elem)?; | |
| } | |
| // SAFETY: We verified all elements are valid PyRef<T> instances via TransmuteFromObject::check, | |
| // and PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple with identical memory layout | |
| Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTupleTyped<PyRef<T>>>>(tuple) }) | |
| } | |
| } | |
| impl<T: PyPayload> Py<PyTupleTyped<PyRef<T>>> { | |
| pub fn as_untyped(&self) -> &Py<PyTuple> { | |
| // SAFETY: PyTupleTyped<PyRef<T>> is repr(transparent) over PyTuple, | |
| // so reference transmutation preserves memory layout and lifetime | |
| unsafe { std::mem::transmute::<&Py<PyTupleTyped<PyRef<T>>>, &Py<PyTuple>>(self) } | |
| } | |
| } |
In vm/src/builtins/tuple.rs around lines 550 to 575, the unsafe transmute operations lack detailed safety justification. Enhance the safety comments by explicitly explaining why the layout compatibility holds, such as stating that PyTupleTyped is a transparent wrapper over PyTuple and that the element type checks guarantee the validity of the transmute. Clarify that these conditions ensure no undefined behavior occurs during the conversions.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)vm/src/builtins/tuple.rs (1)📜 Review details475-487: Enhance safety documentation for try_into_typed.
The transmute operation needs more detailed safety justification, particularly about layout compatibility between the generic types.
impl PyRef<PyTuple<PyObjectRef>> { + /// Attempts to convert an untyped tuple to a typed tuple. + /// + /// # Safety + /// + /// This method uses transmute after verifying all elements are of type T. + /// The transmute is safe because PyTuple<PyRef<T>> has the same memory layout + /// as PyTuple<PyObjectRef> when PyRef<T> has the same layout as PyObjectRef. pub fn try_into_typed<T: PyPayload>( self, vm: &VirtualMachine, ) -> PyResult<PyRef<PyTuple<PyRef<T>>>> { // Check that all elements are of the correct type for elem in self.as_slice() { <PyRef<T> as TransmuteFromObject>::check(vm, elem)?; } - // SAFETY: We just verified all elements are of type T + // SAFETY: We verified all elements are valid PyRef<T> instances via TransmuteFromObject::check, + // and PyTuple<PyRef<T>> has the same memory layout as PyTuple<PyObjectRef> Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTuple<PyRef<T>>>>(self) }) } }
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 12ecbbd and 6342ad4.
📒 Files selected for processing (11)- derive-impl/src/pytraverse.rs (1 hunks)
- vm/src/builtins/function.rs (7 hunks)
- vm/src/builtins/tuple.rs (4 hunks)
- vm/src/builtins/type.rs (5 hunks)
- vm/src/convert/try_from.rs (1 hunks)
- vm/src/frame.rs (3 hunks)
- vm/src/object/core.rs (10 hunks)
- vm/src/object/payload.rs (2 hunks)
- vm/src/object/traverse_object.rs (2 hunks)
- vm/src/vm/context.rs (1 hunks)
- vm/src/vm/mod.rs (5 hunks)
- vm/src/convert/try_from.rs
- vm/src/vm/context.rs
- derive-impl/src/pytraverse.rs
- vm/src/builtins/type.rs
- vm/src/vm/mod.rs
- vm/src/frame.rs
- vm/src/builtins/function.rs
- vm/src/object/payload.rs
- vm/src/object/traverse_object.rs
- vm/src/object/core.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to Lib/test/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to tests in `Lib/test`
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- 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/tuple.rs (6)27-29: Generic tuple implementation looks good.
The refactoring to make PyTuple generic over element type R is a clean approach that eliminates the need for a separate wrapper struct.
142-193: Generic trait implementations are correctly updated.
All the trait implementations (AsRef, Deref, IntoIterator) have been properly updated to work with the generic parameter R. The implementations maintain the same semantics while being more flexible.
489-494: Safety comment is adequate for into_untyped.
The transmute operation is properly documented and the safety reasoning is sound.
496-501: Safety comment is adequate for as_untyped.
The reference transmute operation is properly documented with appropriate safety reasoning.
503-508: From trait implementation is correctly implemented.
The conversion from typed to untyped tuple reference is straightforward and reuses the existing into_untyped method.
235-238: Layout compatibility confirmed
Both PyRef<T> and PyObjectRef are #[repr(transparent)] newtypes over a single NonNull<…> pointer (to Py<T> and PyObject, respectively). Since they’re the same size, alignment, and ABI, transmuting Vec<PyRef<T>> to Vec<PyObjectRef> is sound.
Sorry, something went wrong.
| impl<T> PyTuple<PyRef<T>> { | ||
| pub fn new_ref_typed(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<PyTuple<PyRef<T>>> { | ||
| // SAFETY: PyRef<T> has the same layout as PyObjectRef | ||
| unsafe { | ||
| let elements: Vec<PyObjectRef> = | ||
| std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements); | ||
| let tuple = PyTuple::<PyObjectRef>::new_ref(elements, ctx); | ||
| std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTuple<PyRef<T>>>>(tuple) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify double transmutation in new_ref_typed.
The method performs two unsafe transmutes in sequence, which is unnecessarily complex and increases the risk of errors. The conversion can be simplified.
impl<T> PyTuple<PyRef<T>> {
pub fn new_ref_typed(elements: Vec<PyRef<T>>, ctx: &Context) -> PyRef<PyTuple<PyRef<T>>> {
- // SAFETY: PyRef<T> has the same layout as PyObjectRef
- unsafe {
- let elements: Vec<PyObjectRef> =
- std::mem::transmute::<Vec<PyRef<T>>, Vec<PyObjectRef>>(elements);
- let tuple = PyTuple::<PyObjectRef>::new_ref(elements, ctx);
- std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTuple<PyRef<T>>>>(tuple)
- }
+ if elements.is_empty() {
+ // SAFETY: Empty tuple conversion is always safe
+ unsafe { std::mem::transmute(ctx.empty_tuple.clone()) }
+ } else {
+ // SAFETY: PyRef<T> has the same layout as PyObjectRef
+ let elements: Box<[PyRef<T>]> = elements.into_boxed_slice();
+ let elements: Box<[PyObjectRef]> = unsafe {
+ std::mem::transmute::<Box<[PyRef<T>]>, Box<[PyObjectRef]>>(elements)
+ };
+ PyRef::new_ref(
+ PyTuple { elements },
+ ctx.types.tuple_type.to_owned(),
+ None,
+ )
+ }
}
}🤖 Prompt for AI AgentsCommittable suggestion skipped: line range outside the PR's diff.
In vm/src/builtins/tuple.rs around lines 233 to 243, the new_ref_typed method uses two unsafe transmute calls in sequence, which is overly complex and error-prone. Simplify this by performing a single transmute or by restructuring the code to avoid double transmutation, ensuring the conversion from Vec<PyRef<T>> to Vec<PyObjectRef> and from PyRef<PyTuple> to PyRef<PyTuple<PyRef<T>>> is done safely and clearly in one step.
Sorry, something went wrong.
Summary by CodeRabbit
Refactor
New Features
Style