FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Integrate PyTupleTyped into PyTuple by youknowone · Pull Request #5959 · RustPython/RustPython · GitHub

/ RustPython Public

Integrate PyTupleTyped into PyTuple #5959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 4 commits into from
Jul 14, 2025
Conversation 9 Commits 4 Checks 11 Files changed
Merged

Integrate PyTupleTyped into PyTuple #5959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
635b4af
Fix derive(Traverse)
youknowone Jul 13, 2025
0948971
PyPayload::payload_type_of
youknowone Jul 13, 2025
14ce76e
PyTupleTyped as alias of PyTuple
youknowone Jul 13, 2025
6342ad4
Fully integrate PyTupleTyped into PyTuple
youknowone Jul 14, 2025
File filter

Filter by extension

Filter by extension .rs  (11) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
13 changes: 12 additions & 1 deletion derive-impl/src/pytraverse.rs
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,19 @@ pub(crate) fn impl_pytraverse(mut item: DeriveInput) -> Result<TokenStream> {

let ty = &item.ident;

// Add Traverse bound to all type parameters
for param in &mut item.generics.params {
if let syn::GenericParam::Type(type_param) = param {
type_param
.bounds
.push(syn::parse_quote!(::rustpython_vm::object::Traverse));
}
}

let (impl_generics, ty_generics, where_clause) = item.generics.split_for_impl();

let ret = quote! {
unsafe impl ::rustpython_vm::object::Traverse for #ty {
unsafe impl #impl_generics ::rustpython_vm::object::Traverse for #ty #ty_generics #where_clause {
fn traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn) {
#trace_code
}
Expand Down
19 changes: 10 additions & 9 deletions vm/src/builtins/function.rs
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@
mod jit;

use super::{
PyAsyncGen, PyCode, PyCoroutine, PyDictRef, PyGenerator, PyStr, PyStrRef, PyTupleRef, PyType,
PyTypeRef, tuple::PyTupleTyped,
PyAsyncGen, PyCode, PyCoroutine, PyDictRef, PyGenerator, PyStr, PyStrRef, PyTuple, PyTupleRef,
PyType, PyTypeRef,
};
#[cfg(feature = "jit")]
use crate::common::lock::OnceCell;
use crate::common::lock::PyMutex;
use crate::convert::{ToPyObject, TryFromObject};
use crate::function::ArgMapping;
use crate::object::{Traverse, TraverseFn};
use crate::{
Expand All @@ -32,7 +31,7 @@ pub struct PyFunction {
code: PyRef<PyCode>,
globals: PyDictRef,
builtins: PyObjectRef,
closure: Option<PyTupleTyped<PyCellRef>>,
closure: Option<PyRef<PyTuple<PyCellRef>>>,
defaults_and_kwdefaults: PyMutex<(Option<PyTupleRef>, Option<PyDictRef>)>,
name: PyMutex<PyStrRef>,
qualname: PyMutex<PyStrRef>,
Expand All @@ -47,7 +46,9 @@ pub struct PyFunction {
unsafe impl Traverse for PyFunction {
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
self.globals.traverse(tracer_fn);
self.closure.traverse(tracer_fn);
if let Some(closure) = self.closure.as_ref() {
closure.as_untyped().traverse(tracer_fn);
}
self.defaults_and_kwdefaults.traverse(tracer_fn);
}
}
Expand All @@ -58,7 +59,7 @@ impl PyFunction {
pub(crate) fn new(
code: PyRef<PyCode>,
globals: PyDictRef,
closure: Option<PyTupleTyped<PyCellRef>>,
closure: Option<PyRef<PyTuple<PyCellRef>>>,
defaults: Option<PyTupleRef>,
kw_only_defaults: Option<PyDictRef>,
qualname: PyStrRef,
Expand Down Expand Up @@ -326,6 +327,7 @@ impl Py<PyFunction> {
) -> PyResult {
#[cfg(feature = "jit")]
if let Some(jitted_code) = self.jitted_code.get() {
use crate::convert::ToPyObject;
match jit::get_jit_args(self, &func_args, jitted_code, vm) {
Ok(args) => {
return Ok(args.invoke().to_pyobject(vm));
Expand Down Expand Up @@ -427,7 +429,7 @@ impl PyFunction {
#[pymember]
fn __closure__(vm: &VirtualMachine, zelf: PyObjectRef) -> PyResult {
let zelf = Self::_as_pyref(&zelf, vm)?;
Ok(vm.unwrap_or_none(zelf.closure.clone().map(|x| x.to_pyobject(vm))))
Ok(vm.unwrap_or_none(zelf.closure.clone().map(|x| x.into())))
}

#[pymember]
Expand Down Expand Up @@ -612,8 +614,7 @@ impl Constructor for PyFunction {
}

// Validate that all items are cells and create typed tuple
let typed_closure =
PyTupleTyped::<PyCellRef>::try_from_object(vm, closure_tuple.into())?;
let typed_closure = closure_tuple.try_into_typed::<PyCell>(vm)?;
Some(typed_closure)
} else if !args.code.freevars.is_empty() {
return Err(vm.new_type_error("arg 5 (closure) must be tuple"));
Expand Down
203 changes: 86 additions & 117 deletions vm/src/builtins/tuple.rs
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use crate::common::{
hash::{PyHash, PyUHash},
lock::PyMutex,
};
use crate::object::{Traverse, TraverseFn};
use crate::{
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject,
atomic_func,
Expand All @@ -22,14 +21,14 @@ use crate::{
utils::collection_repr,
vm::VirtualMachine,
};
use std::{fmt, marker::PhantomData, sync::LazyLock};
use std::{fmt, sync::LazyLock};

#[pyclass(module = false, name = "tuple", traverse)]
pub struct PyTuple {
elements: Box<[PyObjectRef]>,
pub struct PyTuple<R = PyObjectRef> {
elements: Box<[R]>,
}

impl fmt::Debug for PyTuple {
impl<R> fmt::Debug for PyTuple<R> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// TODO: implement more informational, non-recursive Debug formatter
f.write_str("tuple")
Expand Down Expand Up @@ -140,39 +139,60 @@ impl Constructor for PyTuple {
}
}

impl AsRef<[PyObjectRef]> for PyTuple {
fn as_ref(&self) -> &[PyObjectRef] {
self.as_slice()
impl<R> AsRef<[R]> for PyTuple<R> {
fn as_ref(&self) -> &[R] {
&self.elements
}
}

impl std::ops::Deref for PyTuple {
type Target = [PyObjectRef];
impl<R> std::ops::Deref for PyTuple<R> {
type Target = [R];

fn deref(&self) -> &[PyObjectRef] {
self.as_slice()
fn deref(&self) -> &[R] {
&self.elements
}
}

impl<'a> std::iter::IntoIterator for &'a PyTuple {
type Item = &'a PyObjectRef;
type IntoIter = std::slice::Iter<'a, PyObjectRef>;
impl<'a, R> std::iter::IntoIterator for &'a PyTuple<R> {
type Item = &'a R;
type IntoIter = std::slice::Iter<'a, R>;

fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}

impl<'a> std::iter::IntoIterator for &'a Py<PyTuple> {
type Item = &'a PyObjectRef;
type IntoIter = std::slice::Iter<'a, PyObjectRef>;
impl<'a, R> std::iter::IntoIterator for &'a Py<PyTuple<R>> {
type Item = &'a R;
type IntoIter = std::slice::Iter<'a, R>;

fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}

impl PyTuple {
impl<R> PyTuple<R> {
pub const fn as_slice(&self) -> &[R] {
&self.elements
}

#[inline]
pub fn len(&self) -> usize {
self.elements.len()
}

#[inline]
pub fn is_empty(&self) -> bool {
self.elements.is_empty()
}

#[inline]
pub fn iter(&self) -> std::slice::Iter<'_, R> {
self.elements.iter()
}
}

impl PyTuple<PyObjectRef> {
pub fn new_ref(elements: Vec<PyObjectRef>, ctx: &Context) -> PyRef<Self> {
if elements.is_empty() {
ctx.empty_tuple.clone()
Expand All @@ -189,10 +209,6 @@ impl PyTuple {
Self { elements }
}

pub const fn as_slice(&self) -> &[PyObjectRef] {
&self.elements
}

fn repeat(zelf: PyRef<Self>, value: isize, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
Ok(if zelf.elements.is_empty() || value == 0 {
vm.ctx.empty_tuple.clone()
Expand All @@ -214,6 +230,18 @@ impl PyTuple {
}
}

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)
}
}
}
Comment on lines +233 to +243

coderabbitai Bot Jul 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🛠️ 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.


#[pyclass(
flags(BASETYPE),
with(
Expand Down Expand Up @@ -272,11 +300,6 @@ impl PyTuple {
self.elements.len()
}

#[inline]
pub const fn is_empty(&self) -> bool {
self.elements.is_empty()
}

#[pymethod(name = "__rmul__")]
#[pymethod]
fn __mul__(zelf: PyRef<Self>, value: ArgSize, vm: &VirtualMachine) -> PyResult<PyRef<Self>> {
Expand Down Expand Up @@ -449,6 +472,41 @@ impl Representable for PyTuple {
}
}

impl PyRef<PyTuple<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
Ok(unsafe { std::mem::transmute::<PyRef<PyTuple>, PyRef<PyTuple<PyRef<T>>>>(self) })
}
}

impl<T: PyPayload> PyRef<PyTuple<PyRef<T>>> {
pub fn into_untyped(self) -> PyRef<PyTuple> {
// SAFETY: PyTuple<PyRef<T>> has the same layout as PyTuple
unsafe { std::mem::transmute::<PyRef<PyTuple<PyRef<T>>>, PyRef<PyTuple>>(self) }
}
}

impl<T: PyPayload> Py<PyTuple<PyRef<T>>> {
pub fn as_untyped(&self) -> &Py<PyTuple> {
// SAFETY: PyTuple<PyRef<T>> has the same layout as PyTuple
unsafe { std::mem::transmute::<&Py<PyTuple<PyRef<T>>>, &Py<PyTuple>>(self) }
}
}

impl<T: PyPayload> From<PyRef<PyTuple<PyRef<T>>>> for PyTupleRef {
#[inline]
fn from(tup: PyRef<PyTuple<PyRef<T>>>) -> Self {
tup.into_untyped()
}
}

#[pyclass(module = false, name = "tuple_iterator", traverse)]
#[derive(Debug)]
pub(crate) struct PyTupleIterator {
Expand Down Expand Up @@ -500,95 +558,6 @@ pub(crate) fn init(context: &Context) {
PyTupleIterator::extend_class(context, context.types.tuple_iterator_type);
}

pub struct PyTupleTyped<T: TransmuteFromObject> {
// SAFETY INVARIANT: T must be repr(transparent) over PyObjectRef, and the
// elements must be logically valid when transmuted to T
tuple: PyTupleRef,
_marker: PhantomData<Vec<T>>,
}

unsafe impl<T> Traverse for PyTupleTyped<T>
where
T: TransmuteFromObject + Traverse,
{
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
self.tuple.traverse(tracer_fn);
}
}

impl<T: TransmuteFromObject> TryFromObject for PyTupleTyped<T> {
fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
let tuple = PyTupleRef::try_from_object(vm, obj)?;
for elem in &*tuple {
T::check(vm, elem)?
}
// SAFETY: the contract of TransmuteFromObject upholds the variant on `tuple`
Ok(Self {
tuple,
_marker: PhantomData,
})
}
}

impl<T: TransmuteFromObject> AsRef<[T]> for PyTupleTyped<T> {
fn as_ref(&self) -> &[T] {
self.as_slice()
}
}

impl<T: TransmuteFromObject> PyTupleTyped<T> {
pub fn empty(vm: &VirtualMachine) -> Self {
Self {
tuple: vm.ctx.empty_tuple.clone(),
_marker: PhantomData,
}
}

#[inline]
pub fn as_slice(&self) -> &[T] {
unsafe { &*(self.tuple.as_slice() as *const [PyObjectRef] as *const [T]) }
}

#[inline]
pub fn len(&self) -> usize {
self.tuple.len()
}

#[inline]
pub fn is_empty(&self) -> bool {
self.tuple.is_empty()
}
}

impl<T: TransmuteFromObject> Clone for PyTupleTyped<T> {
fn clone(&self) -> Self {
Self {
tuple: self.tuple.clone(),
_marker: PhantomData,
}
}
}

impl<T: TransmuteFromObject + fmt::Debug> fmt::Debug for PyTupleTyped<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.as_slice().fmt(f)
}
}

impl<T: TransmuteFromObject> From<PyTupleTyped<T>> for PyTupleRef {
#[inline]
fn from(tup: PyTupleTyped<T>) -> Self {
tup.tuple
}
}

impl<T: TransmuteFromObject> ToPyObject for PyTupleTyped<T> {
#[inline]
fn to_pyobject(self, _vm: &VirtualMachine) -> PyObjectRef {
self.tuple.into()
}
}

pub(super) fn tuple_hash(elements: &[PyObjectRef], vm: &VirtualMachine) -> PyResult<PyHash> {
#[cfg(target_pointer_width = "64")]
const PRIME1: PyUHash = 11400714785074694791;
Expand Down
Loading
Loading
Back | FazBrowse Home | New Git URL