Integrate PyTupleTyped into PyTuple #5959
Conversation
WalkthroughThe changes refactor the handling of typed Python tuples and related generics throughout the codebase. The 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. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vm/src/frame.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 toimport_fromis 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.
vm/src/builtins/tuple.rs (1)
561-573: Consider relaxing trait bounds on empty() method.The trait bounds
T: fmt::Debug + Send + Sync + 'staticseem 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> {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 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)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (4)
derive-impl/src/pytraverse.rs (1)
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/function.rs (1)
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/frame.rs (1)
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)
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
🧬 Code Graph Analysis (2)
vm/src/object/core.rs (2)
vm/src/builtins/tuple.rs (1)
payload_type_id(515-517)vm/src/object/payload.rs (1)
payload_type_id(23-25)
vm/src/builtins/tuple.rs (3)
vm/src/object/payload.rs (1)
payload_type_id(23-25)vm/src/object/core.rs (4)
TypeId(451-451)traverse(127-129)traverse(135-137)Self(373-376)vm/src/object/traverse.rs (1)
try_traverse(17-17)
⏰ 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)
- 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
🔇 Additional comments (23)
vm/src/object/core.rs (1)
657-659: LGTM! Type identification standardization implemented correctly.The change from
TypeId::of::<T>()toT::payload_type_id()properly utilizes the new standardized method from thePyPayloadtrait. 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
Traversebounds to all generic type parameters ensures that the derivedTraverseimplementation is valid for generic types. The use ofsplit_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 implblock 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
PyObjectPayloadto requirePyPayloadensures that all object payloads provide the standardizedpayload_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>>toOption<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_slotsvariable type is properly updated toOption<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_objectcall 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_objectcall 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>toPyRef<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
TryFromObjectandToPyObjectto 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>>toOption<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_objectproperly 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
PyTupledirectly instead ofPyTupleRefsimplifies the internal representation- The generic parameter
RwithTransmuteFromObjectbound 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
TraverseandMaybeTraverseimplementations properly handle GC traversal by:
- Delegating to the underlying tuple's traverse method
- Setting
IS_TRACE = trueto 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 inas_slice()is justified by the transmutation safety requirements documented in the struct.Also applies to: 577-577
597-602: Clean conversion trait implementation.The
Fromtrait implementation provides a convenient and safe way to convert typed tuple references back to regular tuple references using theinto_untyped()method.
youknowone
force-pushed
the
tuple
branch
2 times, most recently
from
0261ba2 to
fff5446
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
vm/src/builtins/tuple.rs (1)
516-521: Add missing#[repr(transparent)]attribute for safe transmutation.The
PyTupleTypedstruct 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>, }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 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)
🚧 Files skipped from review as they are similar to previous changes (8)
- 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
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
vm/src/builtins/tuple.rs (1)
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
🧬 Code Graph Analysis (1)
vm/src/builtins/tuple.rs (2)
vm/src/object/traverse.rs (1)
try_traverse(17-17)vm/src/convert/transmute_from.rs (2)
check(13-13)check(17-28)
⏰ 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)
- 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
🔇 Additional comments (4)
vm/src/builtins/tuple.rs (4)
6-6: LGTM! Import added for new trait implementation.The
MaybeTraverseimport is correctly added to support the new trait implementation in the code.
523-537: LGTM! Trait implementations correctly simplified.The
Traverseimplementation is properly constrained withR: TransmuteFromObject, and the newMaybeTraverseimplementation correctly delegates to theTraverseimplementation when both traits are available.
577-598: LGTM! Trait implementations and methods are correctly implemented.The
AsReftrait 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
Debugimplementation correctly delegates to the tuple's slice formatting, and theFromtrait implementation properly uses the safeinto_untypedmethod.
vm/src/builtins/tuple.rs
Outdated
| 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>()
}
}
}📝 Committable suggestion
‼️ 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>() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
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.
vm/src/builtins/tuple.rs
Outdated
| 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) }
}
}📝 Committable suggestion
‼️ 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) } | |
| } | |
| } |
🤖 Prompt for AI Agents
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.
81dc877 to
12ecbbd
Compare
youknowone
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vm/src/builtins/tuple.rs (1)
475-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) }) } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 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)
🚧 Files skipped from review as they are similar to previous changes (10)
- 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
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
vm/src/builtins/tuple.rs (2)
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
⏰ 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)
- 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)
🔇 Additional comments (6)
vm/src/builtins/tuple.rs (6)
27-29: Generic tuple implementation looks good.The refactoring to make
PyTuplegeneric over element typeRis 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 parameterR. 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_untypedmethod.
235-238: Layout compatibility confirmed
BothPyRef<T>andPyObjectRefare#[repr(transparent)]newtypes over a singleNonNull<…>pointer (toPy<T>andPyObject, respectively). Since they’re the same size, alignment, and ABI, transmutingVec<PyRef<T>>toVec<PyObjectRef>is sound.
| 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,
+ )
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
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.
Summary by CodeRabbit
Refactor
New Features
Style