diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 686f0798311..8a69cdd5c2b 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -583,22 +583,7 @@ pub fn impl_py_setter_def( #deprecations _pyo3::class::PySetterDef::new( #python_name, - _pyo3::impl_::pymethods::PySetter({ - unsafe extern "C" fn trampoline( - slf: *mut _pyo3::ffi::PyObject, - value: *mut _pyo3::ffi::PyObject, - closure: *mut ::std::os::raw::c_void, - ) -> ::std::os::raw::c_int - { - _pyo3::impl_::trampoline::setter( - slf, - value, - closure, - #cls::#wrapper_ident - ) - } - trampoline - }), + _pyo3::impl_::pymethods::PySetter(#cls::#wrapper_ident), #doc ) }) @@ -719,20 +704,7 @@ pub fn impl_py_getter_def( #deprecations _pyo3::class::PyGetterDef::new( #python_name, - _pyo3::impl_::pymethods::PyGetter({ - unsafe extern "C" fn trampoline( - slf: *mut _pyo3::ffi::PyObject, - closure: *mut ::std::os::raw::c_void, - ) -> *mut _pyo3::ffi::PyObject - { - _pyo3::impl_::trampoline::getter( - slf, - closure, - #cls::#wrapper_ident - ) - } - trampoline - }), + _pyo3::impl_::pymethods::PyGetter(#cls::#wrapper_ident), #doc ) }) diff --git a/src/impl_/pyclass/lazy_type_object.rs b/src/impl_/pyclass/lazy_type_object.rs index a43d44f97a6..551b052ae71 100644 --- a/src/impl_/pyclass/lazy_type_object.rs +++ b/src/impl_/pyclass/lazy_type_object.rs @@ -9,10 +9,10 @@ use std::{ use crate::{ exceptions::PyRuntimeError, ffi, - pyclass::create_type_object, + pyclass::{create_type_object, PyClassTypeObject}, sync::{GILOnceCell, GILProtected}, types::PyType, - AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python, + AsPyPointer, IntoPyPointer, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python, }; use super::PyClassItemsIter; @@ -23,7 +23,7 @@ pub struct LazyTypeObject(LazyTypeObjectInner, PhantomData); // Non-generic inner of LazyTypeObject to keep code size down struct LazyTypeObjectInner { - value: GILOnceCell>, + value: GILOnceCell, // Threads which have begun initialization of the `tp_dict`. Used for // reentrant initialization detection. initializing_threads: GILProtected>>, @@ -67,12 +67,16 @@ impl LazyTypeObjectInner { fn get_or_try_init<'py>( &'py self, py: Python<'py>, - init: fn(Python<'py>) -> PyResult>, + init: fn(Python<'py>) -> PyResult, name: &str, items_iter: PyClassItemsIter, ) -> PyResult<&'py PyType> { (|| -> PyResult<_> { - let type_object = self.value.get_or_try_init(py, || init(py))?.as_ref(py); + let type_object = self + .value + .get_or_try_init(py, || init(py))? + .type_object + .as_ref(py); self.ensure_init(type_object, name, items_iter)?; Ok(type_object) })() diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index c6b3fe2e94d..fde97430001 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -39,7 +39,6 @@ impl IPowModulo { /// `PyMethodDefType` represents different types of Python callable objects. /// It is used by the `#[pymethods]` attribute. -#[derive(Debug)] pub enum PyMethodDefType { /// Represents class method Class(PyMethodDef), @@ -72,10 +71,10 @@ pub struct PyCFunctionWithKeywords(pub ffi::PyCFunctionWithKeywords); #[cfg(not(Py_LIMITED_API))] #[derive(Clone, Copy, Debug)] pub struct PyCFunctionFastWithKeywords(pub ffi::_PyCFunctionFastWithKeywords); -#[derive(Clone, Copy, Debug)] -pub struct PyGetter(pub ffi::getter); -#[derive(Clone, Copy, Debug)] -pub struct PySetter(pub ffi::setter); +#[derive(Clone, Copy)] +pub struct PyGetter(pub Getter); +#[derive(Clone, Copy)] +pub struct PySetter(pub Setter); #[derive(Clone, Copy)] pub struct PyClassAttributeFactory(pub for<'p> fn(Python<'p>) -> PyResult); @@ -102,18 +101,18 @@ impl PyClassAttributeDef { } } -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct PyGetterDef { pub(crate) name: &'static str, pub(crate) meth: PyGetter, - doc: &'static str, + pub(crate) doc: &'static str, } -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct PySetterDef { pub(crate) name: &'static str, pub(crate) meth: PySetter, - doc: &'static str, + pub(crate) doc: &'static str, } unsafe impl Sync for PyMethodDef {} @@ -212,6 +211,12 @@ impl fmt::Debug for PyClassAttributeDef { } } +/// Class getter / setters +pub(crate) type Getter = + for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<*mut ffi::PyObject>; +pub(crate) type Setter = + for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::PyObject) -> PyResult; + impl PyGetterDef { /// Define a getter. pub const fn new(name: &'static str, getter: PyGetter, doc: &'static str) -> Self { @@ -221,23 +226,6 @@ impl PyGetterDef { doc, } } - - /// Copy descriptor information to `ffi::PyGetSetDef` - pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { - if dst.name.is_null() { - let name = get_name(self.name).unwrap(); - dst.name = name.as_ptr() as _; - // FIXME: stop leaking name - std::mem::forget(name); - } - if dst.doc.is_null() { - let doc = get_doc(self.doc).unwrap(); - dst.doc = doc.as_ptr() as _; - // FIXME: stop leaking doc - std::mem::forget(doc); - } - dst.get = Some(self.meth.0); - } } impl PySetterDef { @@ -249,31 +237,6 @@ impl PySetterDef { doc, } } - - /// Copy descriptor information to `ffi::PyGetSetDef` - pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { - if dst.name.is_null() { - let name = get_name(self.name).unwrap(); - dst.name = name.as_ptr() as _; - // FIXME: stop leaking name - std::mem::forget(name); - } - if dst.doc.is_null() { - let doc = get_doc(self.doc).unwrap(); - dst.doc = doc.as_ptr() as _; - // FIXME: stop leaking doc - std::mem::forget(doc); - } - dst.set = Some(self.meth.0); - } -} - -fn get_name(name: &'static str) -> PyResult> { - extract_c_string(name, "Function name cannot contain NUL byte.") -} - -fn get_doc(doc: &'static str) -> PyResult> { - extract_c_string(doc, "Document cannot contain NUL byte.") } /// Unwraps the result of __traverse__ for tp_traverse @@ -319,3 +282,11 @@ where self.map(|o| o.into_py(py)) } } + +pub(crate) fn get_name(name: &'static str) -> PyResult> { + extract_c_string(name, "function name cannot contain NUL byte.") +} + +pub(crate) fn get_doc(doc: &'static str) -> PyResult> { + extract_c_string(doc, "function doc cannot contain NUL byte.") +} diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs index c7bea9abe2e..efe50e20727 100644 --- a/src/impl_/trampoline.rs +++ b/src/impl_/trampoline.rs @@ -5,7 +5,7 @@ use std::{ any::Any, - os::raw::{c_int, c_void}, + os::raw::c_int, panic::{self, UnwindSafe}, }; @@ -64,29 +64,6 @@ trampolines!( ) -> *mut ffi::PyObject; ); -#[inline] -pub unsafe fn getter( - slf: *mut ffi::PyObject, - closure: *mut c_void, - f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<*mut ffi::PyObject>, -) -> *mut ffi::PyObject { - // PyO3 doesn't use the closure argument at present. - debug_assert!(closure.is_null()); - trampoline_inner(|py| f(py, slf)) -} - -#[inline] -pub unsafe fn setter( - slf: *mut ffi::PyObject, - value: *mut ffi::PyObject, - closure: *mut c_void, - f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::PyObject) -> PyResult, -) -> c_int { - // PyO3 doesn't use the closure argument at present. - debug_assert!(closure.is_null()); - trampoline_inner(|py| f(py, slf, value)) -} - // Trampolines used by slot methods trampolines!( pub fn getattrofunc(slf: *mut ffi::PyObject, attr: *mut ffi::PyObject) -> *mut ffi::PyObject; diff --git a/src/pyclass.rs b/src/pyclass.rs index 432f45a1c71..84d1902edbf 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -8,7 +8,7 @@ use std::{cmp::Ordering, os::raw::c_int}; mod create_type_object; mod gc; -pub(crate) use self::create_type_object::create_type_object; +pub(crate) use self::create_type_object::{create_type_object, PyClassTypeObject}; pub use self::gc::{PyTraverseError, PyVisit}; /// Types that can be used as Python classes. diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index 29e09739d94..bc2058f50f0 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -5,10 +5,15 @@ use crate::{ assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, PyClassItemsIter, }, + impl_::{ + pymethods::{get_doc, get_name, Getter, Setter}, + trampoline::trampoline_inner, + }, types::PyType, - Py, PyClass, PyMethodDefType, PyResult, PyTypeInfo, Python, + Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python, }; use std::{ + borrow::Cow, collections::HashMap, convert::TryInto, ffi::{CStr, CString}, @@ -16,7 +21,13 @@ use std::{ ptr, }; -pub(crate) fn create_type_object(py: Python<'_>) -> PyResult> +pub(crate) struct PyClassTypeObject { + pub type_object: Py, + #[allow(dead_code)] // This is purely a cache that must live as long as the type object + getset_destructors: Vec, +} + +pub(crate) fn create_type_object(py: Python<'_>) -> PyResult where T: PyClass, { @@ -40,7 +51,7 @@ type PyTypeBuilderCleanup = Box; struct PyTypeBuilder { slots: Vec, method_defs: Vec, - property_defs_map: HashMap<&'static str, ffi::PyGetSetDef>, + getset_builders: HashMap<&'static str, GetSetDefBuilder>, /// Used to patch the type objects for the things there's no /// PyType_FromSpec API for... there's no reason this should work, /// except for that it does and we have tests. @@ -111,28 +122,18 @@ impl PyTypeBuilder { } fn pymethod_def(&mut self, def: &PyMethodDefType) { - const PY_GET_SET_DEF_INIT: ffi::PyGetSetDef = ffi::PyGetSetDef { - name: ptr::null_mut(), - get: None, - set: None, - doc: ptr::null(), - closure: ptr::null_mut(), - }; - match def { PyMethodDefType::Getter(getter) => { - getter.copy_to( - self.property_defs_map - .entry(getter.name) - .or_insert(PY_GET_SET_DEF_INIT), - ); + self.getset_builders + .entry(getter.name) + .or_default() + .add_getter(getter); } PyMethodDefType::Setter(setter) => { - setter.copy_to( - self.property_defs_map - .entry(setter.name) - .or_insert(PY_GET_SET_DEF_INIT), - ); + self.getset_builders + .entry(setter.name) + .or_default() + .add_setter(setter); } PyMethodDefType::Method(def) | PyMethodDefType::Class(def) @@ -147,22 +148,30 @@ impl PyTypeBuilder { } } - fn finalize_methods_and_properties(&mut self) { - let method_defs = std::mem::take(&mut self.method_defs); + fn finalize_methods_and_properties(&mut self) -> PyResult> { + let method_defs: Vec = std::mem::take(&mut self.method_defs); // Safety: Py_tp_methods expects a raw vec of PyMethodDef unsafe { self.push_raw_vec_slot(ffi::Py_tp_methods, method_defs) }; - let property_defs = std::mem::take(&mut self.property_defs_map); - // TODO: use into_values when on MSRV Rust >= 1.54 + let mut getset_destructors = Vec::with_capacity(self.getset_builders.len()); + #[allow(unused_mut)] - let mut property_defs: Vec<_> = property_defs.into_iter().map(|(_, value)| value).collect(); + let mut property_defs: Vec<_> = self + .getset_builders + .iter() + .map(|(name, builder)| { + let (def, destructor) = builder.as_get_set_def(name)?; + getset_destructors.push(destructor); + Ok(def) + }) + .collect::>()?; // PyPy doesn't automatically add __dict__ getter / setter. // PyObject_GenericGetDict not in the limited API until Python 3.10. if self.has_dict { #[cfg(not(any(PyPy, all(Py_LIMITED_API, not(Py_3_10)))))] property_defs.push(ffi::PyGetSetDef { - name: "__dict__\0".as_ptr() as *mut c_char, + name: "__dict__\0".as_ptr().cast(), get: Some(ffi::PyObject_GenericGetDict), set: Some(ffi::PyObject_GenericSetDict), doc: ptr::null(), @@ -200,6 +209,8 @@ impl PyTypeBuilder { ) } } + + Ok(getset_destructors) } fn set_is_basetype(mut self, is_basetype: bool) -> Self { @@ -324,12 +335,12 @@ impl PyTypeBuilder { name: &'static str, module_name: Option<&'static str>, basicsize: usize, - ) -> PyResult> { + ) -> PyResult { // `c_ulong` and `c_uint` have the same size // on some platforms (like windows) #![allow(clippy::useless_conversion)] - self.finalize_methods_and_properties(); + let getset_destructors = self.finalize_methods_and_properties()?; if !self.has_new { // Safety: This is the correct slot type for Py_tp_new @@ -380,7 +391,10 @@ impl PyTypeBuilder { cleanup(&self, type_object.as_ref(py).as_type_ptr()); } - Ok(type_object) + Ok(PyClassTypeObject { + type_object, + getset_destructors, + }) } } @@ -399,9 +413,152 @@ unsafe extern "C" fn no_constructor_defined( _args: *mut ffi::PyObject, _kwds: *mut ffi::PyObject, ) -> *mut ffi::PyObject { - crate::impl_::trampoline::trampoline_inner(|_| { + trampoline_inner(|_| { Err(crate::exceptions::PyTypeError::new_err( "No constructor defined", )) }) } + +#[derive(Default)] +struct GetSetDefBuilder { + doc: Option<&'static str>, + getter: Option, + setter: Option, +} + +impl GetSetDefBuilder { + fn add_getter(&mut self, getter: &PyGetterDef) { + // TODO: be smarter about merging getter and setter docs + if self.doc.is_none() { + self.doc = Some(getter.doc); + } + // TODO: return an error if getter already defined? + self.getter = Some(getter.meth.0) + } + + fn add_setter(&mut self, setter: &PySetterDef) { + // TODO: be smarter about merging getter and setter docs + if self.doc.is_none() { + self.doc = Some(setter.doc); + } + // TODO: return an error if setter already defined? + self.setter = Some(setter.meth.0) + } + + fn as_get_set_def( + &self, + name: &'static str, + ) -> PyResult<(ffi::PyGetSetDef, GetSetDefDestructor)> { + let name = get_name(name)?; + let doc = self.doc.map(get_doc).transpose()?; + + let getset_type = match (self.getter, self.setter) { + (Some(getter), None) => GetSetDefType::Getter(getter), + (None, Some(setter)) => GetSetDefType::Setter(setter), + (Some(getter), Some(setter)) => { + GetSetDefType::GetterAndSetter(Box::new(GetterAndSetter { getter, setter })) + } + (None, None) => { + unreachable!("GetSetDefBuilder expected to always have either getter or setter") + } + }; + + let getset_def = getset_type.create_py_get_set_def(&name, doc.as_deref()); + let destructor = GetSetDefDestructor { + name, + doc, + closure: getset_type, + }; + Ok((getset_def, destructor)) + } +} + +#[allow(dead_code)] // a stack of fields which are purely to cache until dropped +struct GetSetDefDestructor { + name: Cow<'static, CStr>, + doc: Option>, + closure: GetSetDefType, +} + +/// Possible forms of property - either a getter, setter, or both +enum GetSetDefType { + Getter(Getter), + Setter(Setter), + // The box is here so that the `GetterAndSetter` has a stable + // memory address even if the `GetSetDefType` enum is moved + GetterAndSetter(Box), +} + +pub(crate) struct GetterAndSetter { + getter: Getter, + setter: Setter, +} + +impl GetSetDefType { + /// Fills a PyGetSetDef structure + /// It is only valid for as long as this GetSetDefType remains alive, + /// as well as name and doc members + pub(crate) fn create_py_get_set_def( + &self, + name: &CStr, + doc: Option<&CStr>, + ) -> ffi::PyGetSetDef { + let (get, set, closure): (Option, Option, *mut c_void) = + match self { + &Self::Getter(closure) => { + unsafe extern "C" fn getter( + slf: *mut ffi::PyObject, + closure: *mut c_void, + ) -> *mut ffi::PyObject { + // Safety: PyO3 sets the closure when constructing the ffi getter so this cast should always be valid + let getter: Getter = std::mem::transmute(closure); + trampoline_inner(|py| getter(py, slf)) + } + (Some(getter), None, closure as Getter as _) + } + &Self::Setter(closure) => { + unsafe extern "C" fn setter( + slf: *mut ffi::PyObject, + value: *mut ffi::PyObject, + closure: *mut c_void, + ) -> c_int { + // Safety: PyO3 sets the closure when constructing the ffi setter so this cast should always be valid + let setter: Setter = std::mem::transmute(closure); + trampoline_inner(|py| setter(py, slf, value)) + } + (None, Some(setter), closure as Setter as _) + } + Self::GetterAndSetter(closure) => { + unsafe extern "C" fn getset_getter( + slf: *mut ffi::PyObject, + closure: *mut c_void, + ) -> *mut ffi::PyObject { + let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter); + trampoline_inner(|py| (getset.getter)(py, slf)) + } + + unsafe extern "C" fn getset_setter( + slf: *mut ffi::PyObject, + value: *mut ffi::PyObject, + closure: *mut c_void, + ) -> c_int { + let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter); + trampoline_inner(|py| (getset.setter)(py, slf, value)) + } + ( + Some(getset_getter), + Some(getset_setter), + closure.as_ref() as *const GetterAndSetter as _, + ) + } + }; + ffi::PyGetSetDef { + name: name.as_ptr(), + doc: doc.map_or(ptr::null(), CStr::as_ptr), + get, + set, + closure, + } + } +}