From ac28db31d941eb259cdc3b43c51e3616eea945b3 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 1 Mar 2024 23:45:34 +0000 Subject: [PATCH] rework `create_cell` to `create_class_object` --- pyo3-macros-backend/src/method.rs | 3 +- src/impl_/coroutine.rs | 5 ++- src/impl_/pyclass.rs | 8 ++--- src/impl_/pymethods.rs | 14 ++++++-- src/instance.rs | 35 ++++++++----------- src/pycell.rs | 19 +---------- src/pycell/impl_.rs | 1 - src/pyclass/create_type_object.rs | 5 +-- src/pyclass_init.rs | 57 ++++++++++++++++--------------- src/type_object.rs | 2 +- 10 files changed, 68 insertions(+), 81 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 42f22204601..a9ba960d513 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -645,8 +645,7 @@ impl<'a> FnSpec<'a> { #( #holders )* let result = #call; let initializer: _pyo3::PyClassInitializer::<#cls> = result.convert(py)?; - let cell = initializer.create_cell_from_subtype(py, _slf)?; - ::std::result::Result::Ok(cell as *mut _pyo3::ffi::PyObject) + _pyo3::impl_::pymethods::tp_new_impl(py, initializer, _slf) } } } diff --git a/src/impl_/coroutine.rs b/src/impl_/coroutine.rs index 6084e59cf5c..1d3119400a0 100644 --- a/src/impl_/coroutine.rs +++ b/src/impl_/coroutine.rs @@ -9,7 +9,7 @@ use crate::{ pycell::impl_::PyClassBorrowChecker, pyclass::boolean_struct::False, types::{PyAnyMethods, PyString}, - IntoPy, Py, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult, Python, + IntoPy, Py, PyAny, PyClass, PyErr, PyObject, PyResult, Python, }; pub fn new_coroutine( @@ -32,8 +32,7 @@ where } fn get_ptr(obj: &Py) -> *mut T { - // SAFETY: Py can be casted as *const PyCell - unsafe { &*(obj.as_ptr() as *const PyCell) }.get_ptr() + obj.get_class_object().get_ptr() } pub struct RefGuard(Py); diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 3c9eb28f9e8..1a144f736e0 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -7,8 +7,8 @@ use crate::{ pyclass_init::PyObjectInit, types::any::PyAnyMethods, types::PyBool, - Borrowed, Py, PyAny, PyCell, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, - PyTypeInfo, Python, + Borrowed, Py, PyAny, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, + Python, }; use std::{ borrow::Cow, @@ -25,13 +25,13 @@ pub use lazy_type_object::LazyTypeObject; /// Gets the offset of the dictionary from the start of the object in bytes. #[inline] pub fn dict_offset() -> ffi::Py_ssize_t { - PyCell::::dict_offset() + PyClassObject::::dict_offset() } /// Gets the offset of the weakref list from the start of the object in bytes. #[inline] pub fn weaklist_offset() -> ffi::Py_ssize_t { - PyCell::::weaklist_offset() + PyClassObject::::weaklist_offset() } /// Represents the `__dict__` field for `#[pyclass]`. diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 70c95ca0883..bc950bafee4 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -7,8 +7,8 @@ use crate::pycell::{PyBorrowError, PyBorrowMutError}; use crate::pyclass::boolean_struct::False; use crate::types::{any::PyAnyMethods, PyModule, PyType}; use crate::{ - ffi, Borrowed, Bound, DowncastError, Py, PyAny, PyCell, PyClass, PyErr, PyObject, PyRef, - PyRefMut, PyResult, PyTraverseError, PyTypeCheck, PyVisit, Python, + ffi, Borrowed, Bound, DowncastError, Py, PyAny, PyCell, PyClass, PyClassInitializer, PyErr, + PyObject, PyRef, PyRefMut, PyResult, PyTraverseError, PyTypeCheck, PyVisit, Python, }; use std::borrow::Cow; use std::ffi::CStr; @@ -569,3 +569,13 @@ impl<'py, T> std::ops::Deref for BoundRef<'_, 'py, T> { self.0 } } + +pub unsafe fn tp_new_impl( + py: Python<'_>, + initializer: PyClassInitializer, + target_type: *mut ffi::PyTypeObject, +) -> PyResult<*mut ffi::PyObject> { + initializer + .create_class_object_of_type(py, target_type) + .map(Bound::into_ptr) +} diff --git a/src/instance.rs b/src/instance.rs index 584f6cb420f..67d2375e323 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1,5 +1,4 @@ use crate::err::{self, PyDowncastError, PyErr, PyResult}; -use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pycell::PyClassObject; use crate::pycell::{PyBorrowError, PyBorrowMutError, PyCell}; use crate::pyclass::boolean_struct::{False, True}; @@ -93,14 +92,7 @@ where py: Python<'py>, value: impl Into>, ) -> PyResult> { - let initializer = value.into(); - let obj = initializer.create_cell(py)?; - let ob = unsafe { - obj.cast::() - .assume_owned(py) - .downcast_into_unchecked() - }; - Ok(ob) + value.into().create_class_object(py) } } @@ -338,10 +330,10 @@ where } pub(crate) fn get_class_object(&self) -> &PyClassObject { - let cell = self.as_ptr().cast::>(); + let class_object = self.as_ptr().cast::>(); // SAFETY: Bound is known to contain an object which is laid out in memory as a // PyClassObject. - unsafe { &*cell } + unsafe { &*class_object } } } @@ -884,10 +876,7 @@ where /// # } /// ``` pub fn new(py: Python<'_>, value: impl Into>) -> PyResult> { - let initializer = value.into(); - let obj = initializer.create_cell(py)?; - let ob = unsafe { Py::from_owned_ptr(py, obj as _) }; - Ok(ob) + Bound::new(py, value).map(Bound::unbind) } } @@ -1191,12 +1180,16 @@ where where T: PyClass + Sync, { - let any = self.as_ptr() as *const PyAny; - // SAFETY: The class itself is frozen and `Sync` and we do not access anything but `cell.contents.value`. - unsafe { - let cell: &PyCell = PyNativeType::unchecked_downcast(&*any); - &*cell.get_ptr() - } + // SAFETY: The class itself is frozen and `Sync` + unsafe { &*self.get_class_object().get_ptr() } + } + + /// Get a view on the underlying `PyClass` contents. + pub(crate) fn get_class_object(&self) -> &PyClassObject { + let class_object = self.as_ptr().cast::>(); + // SAFETY: Bound is known to contain an object which is laid out in memory as a + // PyClassObject. + unsafe { &*class_object } } } diff --git a/src/pycell.rs b/src/pycell.rs index c64fa0d2c96..43c75314f00 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -192,8 +192,6 @@ //! [guide]: https://pyo3.rs/latest/class.html#pycell-and-interior-mutability "PyCell and interior mutability" //! [Interior Mutability]: https://doc.rust-lang.org/book/ch15-05-interior-mutability.html "RefCell and the Interior Mutability Pattern - The Rust Programming Language" -#[allow(deprecated)] -use crate::conversion::FromPyPointer; use crate::conversion::{AsPyPointer, ToPyObject}; use crate::exceptions::PyRuntimeError; use crate::ffi_ptr_ext::FfiPtrExt; @@ -272,12 +270,7 @@ impl PyCell { ) )] pub fn new(py: Python<'_>, value: impl Into>) -> PyResult<&Self> { - unsafe { - let initializer = value.into(); - let self_ = initializer.create_cell(py)?; - #[allow(deprecated)] - FromPyPointer::from_owned_ptr_or_err(py, self_ as _) - } + Bound::new(py, value).map(Bound::into_gil_ref) } /// Immutably borrows the value `T`. This borrow lasts as long as the returned `PyRef` exists. @@ -483,16 +476,6 @@ impl PyCell { pub(crate) fn get_ptr(&self) -> *mut T { self.0.get_ptr() } - - /// Gets the offset of the dictionary from the start of the struct in bytes. - pub(crate) fn dict_offset() -> ffi::Py_ssize_t { - PyClassObject::::dict_offset() - } - - /// Gets the offset of the weakref list from the start of the struct in bytes. - pub(crate) fn weaklist_offset() -> ffi::Py_ssize_t { - PyClassObject::::weaklist_offset() - } } unsafe impl PyLayout for PyCell {} diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs index 8f5443bb8c7..78a4f1cffd6 100644 --- a/src/pycell/impl_.rs +++ b/src/pycell/impl_.rs @@ -334,7 +334,6 @@ mod tests { use crate::prelude::*; use crate::pyclass::boolean_struct::{False, True}; - use crate::PyClass; #[pyclass(crate = "crate", subclass)] struct MutableBase; diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index d0c271cf31a..52e346212f0 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -3,6 +3,7 @@ use pyo3_ffi::PyType_IS_GC; use crate::{ exceptions::PyTypeError, ffi, + impl_::pycell::PyClassObject, impl_::pyclass::{ assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, tp_dealloc_with_gc, PyClassItemsIter, @@ -13,7 +14,7 @@ use crate::{ }, types::typeobject::PyTypeMethods, types::PyType, - Py, PyCell, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python, + Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python, }; use std::{ borrow::Cow, @@ -94,7 +95,7 @@ where T::items_iter(), T::NAME, T::MODULE, - std::mem::size_of::>(), + std::mem::size_of::>(), ) } } diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index 454726ea2a5..d945807e357 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -1,7 +1,9 @@ //! Contains initialization utilities for `#[pyclass]`. use crate::callback::IntoPyCallbackOutput; +use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::{PyClassBaseType, PyClassDict, PyClassThreadChecker, PyClassWeakRef}; -use crate::{ffi, Py, PyCell, PyClass, PyErr, PyResult, Python}; +use crate::types::PyAnyMethods; +use crate::{ffi, Bound, Py, PyClass, PyErr, PyResult, Python}; use crate::{ ffi::PyTypeObject, pycell::impl_::{PyClassBorrowChecker, PyClassMutability, PyClassObjectContents}, @@ -204,56 +206,43 @@ impl PyClassInitializer { } /// Creates a new PyCell and initializes it. - #[doc(hidden)] - pub fn create_cell(self, py: Python<'_>) -> PyResult<*mut PyCell> + pub(crate) fn create_class_object(self, py: Python<'_>) -> PyResult> where T: PyClass, { - unsafe { self.create_cell_from_subtype(py, T::type_object_raw(py)) } + unsafe { self.create_class_object_of_type(py, T::type_object_raw(py)) } } - /// Creates a new PyCell and initializes it given a typeobject `subtype`. - /// Called by the Python `tp_new` implementation generated by a `#[new]` function in a `#[pymethods]` block. + /// Creates a new class object and initializes it given a typeobject `subtype`. /// /// # Safety /// `subtype` must be a valid pointer to the type object of T or a subclass. - #[doc(hidden)] - pub unsafe fn create_cell_from_subtype( + pub(crate) unsafe fn create_class_object_of_type( self, py: Python<'_>, - subtype: *mut crate::ffi::PyTypeObject, - ) -> PyResult<*mut PyCell> + target_type: *mut crate::ffi::PyTypeObject, + ) -> PyResult> where T: PyClass, { - self.into_new_object(py, subtype).map(|obj| obj as _) - } -} - -impl PyObjectInit for PyClassInitializer { - unsafe fn into_new_object( - self, - py: Python<'_>, - subtype: *mut PyTypeObject, - ) -> PyResult<*mut ffi::PyObject> { - /// Layout of a PyCell after base new has been called, but the contents have not yet been + /// Layout of a PyClassObject after base new has been called, but the contents have not yet been /// written. #[repr(C)] - struct PartiallyInitializedPyCell { + struct PartiallyInitializedClassObject { _ob_base: ::LayoutAsBase, contents: MaybeUninit>, } let (init, super_init) = match self.0 { - PyClassInitializerImpl::Existing(value) => return Ok(value.into_ptr()), + PyClassInitializerImpl::Existing(value) => return Ok(value.into_bound(py)), PyClassInitializerImpl::New { init, super_init } => (init, super_init), }; - let obj = super_init.into_new_object(py, subtype)?; + let obj = super_init.into_new_object(py, target_type)?; - let cell: *mut PartiallyInitializedPyCell = obj as _; + let part_init: *mut PartiallyInitializedClassObject = obj as _; std::ptr::write( - (*cell).contents.as_mut_ptr(), + (*part_init).contents.as_mut_ptr(), PyClassObjectContents { value: ManuallyDrop::new(UnsafeCell::new(init)), borrow_checker: ::Storage::new(), @@ -262,7 +251,21 @@ impl PyObjectInit for PyClassInitializer { weakref: T::WeakRef::INIT, }, ); - Ok(obj) + + // Safety: obj is a valid pointer to an object of type `target_type`, which` is a known + // subclass of `T` + Ok(obj.assume_owned(py).downcast_into_unchecked()) + } +} + +impl PyObjectInit for PyClassInitializer { + unsafe fn into_new_object( + self, + py: Python<'_>, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject> { + self.create_class_object_of_type(py, subtype) + .map(Bound::into_ptr) } private_impl! {} diff --git a/src/type_object.rs b/src/type_object.rs index 318810b534d..84888bee458 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -6,7 +6,7 @@ use crate::types::{PyAny, PyType}; use crate::{ffi, Bound, PyNativeType, Python}; /// `T: PyLayout` represents that `T` is a concrete representation of `U` in the Python heap. -/// E.g., `PyCell` is a concrete representation of all `pyclass`es, and `ffi::PyObject` +/// E.g., `PyClassObject` is a concrete representation of all `pyclass`es, and `ffi::PyObject` /// is of `PyAny`. /// /// This trait is intended to be used internally.