use dynamic trampoline for all getters and setters

pyo3-macros-backend/src/pymethod.rs

@@ -580,22 +580,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
             )
         })
@@ -716,20 +701,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
             )
         })

src/impl_/pyclass/lazy_type_object.rs

@@ -8,9 +8,12 @@ use std::{
 use parking_lot::{const_mutex, Mutex};
 
 use crate::{
-    exceptions::PyRuntimeError, ffi, once_cell::GILOnceCell, pyclass::create_type_object,
-    types::PyType, AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject,
-    PyResult, Python,
+    exceptions::PyRuntimeError,
+    ffi,
+    once_cell::GILOnceCell,
+    pyclass::{create_type_object, PyClassTypeObject},
+    types::PyType,
+    AsPyPointer, IntoPyPointer, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python,
 };
 
 use super::PyClassItemsIter;
@@ -21,7 +24,7 @@ pub struct LazyTypeObject<T>(LazyTypeObjectInner, PhantomData<T>);
 
 // Non-generic inner of LazyTypeObject to keep code size down
 struct LazyTypeObjectInner {
-    value: GILOnceCell<Py<PyType>>,
+    value: GILOnceCell<PyClassTypeObject>,
     // Threads which have begun initialization of the `tp_dict`. Used for
     // reentrant initialization detection.
     initializing_threads: Mutex<Vec<ThreadId>>,
@@ -65,12 +68,16 @@ impl LazyTypeObjectInner {
     fn get_or_try_init<'py>(
         &'py self,
         py: Python<'py>,
-        init: fn(Python<'py>) -> PyResult<Py<PyType>>,
+        init: fn(Python<'py>) -> PyResult<PyClassTypeObject>,
         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)
         })()

src/impl_/pymethods.rs

@@ -2,8 +2,8 @@ use crate::exceptions::PyValueError;
 use crate::{ffi, IntoPy, Py, PyAny, PyErr, PyObject, PyResult, PyTraverseError, Python};
 use std::borrow::Cow;
 use std::ffi::{CStr, CString};
-use std::fmt;
 use std::os::raw::c_int;
+use std::{fmt, ptr};
 
 /// Python 3.8 and up - __ipow__ has modulo argument correctly populated.
 #[cfg(Py_3_8)]
@@ -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<PyObject>);
 
@@ -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,59 @@ 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<c_int>;
+
+/// Used as the "closure" argument for ffi::PyGetSetDef structures to support a single trampoline
+/// for each of getters and setters.
+#[derive(Default)]
+pub(crate) struct GetSetDefClosure {
+    pub doc: Option<&'static str>,
+    pub getter: Option<Getter>,
+    pub setter: Option<Setter>,
+}
+
+impl GetSetDefClosure {
+    pub(crate) fn add_getter(&mut self, getter: &PyGetterDef) {
+        if self.doc.is_none() {
+            self.doc = Some(getter.doc);
+        }
+        self.getter = Some(getter.meth.0)
+    }
+
+    pub(crate) fn add_setter(&mut self, setter: &PySetterDef) {
+        if self.doc.is_none() {
+            self.doc = Some(setter.doc);
+        }
+        self.setter = Some(setter.meth.0)
+    }
+
+    pub(crate) fn as_get_set_def(&mut self, name: &'static str) -> PyResult<ffi::PyGetSetDef> {
+        let name = get_name(name)?;
+
+        let mut getset_def = ffi::PyGetSetDef {
+            name: name.as_ptr(),
+            get: self.getter.map(|_| crate::impl_::trampoline::getter as _),
+            set: self.setter.map(|_| crate::impl_::trampoline::setter as _),
+            doc: ptr::null(),
+            closure: (self as *mut GetSetDefClosure).cast(),
+        };
+
+        std::mem::forget(name);
+
+        if let Some(doc) = self.doc {
+            let doc = get_doc(doc)?;
+            getset_def.doc = doc.as_ptr();
+            std::mem::forget(doc)
+        }
+
+        Ok(getset_def)
+    }
+}
+
 impl PyGetterDef {
     /// Define a getter.
     pub const fn new(name: &'static str, getter: PyGetter, doc: &'static str) -> Self {
@@ -221,23 +273,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,23 +284,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<Cow<'static, CStr>> {

src/impl_/trampoline.rs

@@ -10,8 +10,13 @@ use std::{
 };
 
 use crate::{
-    callback::PyCallbackOutput, ffi, impl_::panic::PanicTrap, methods::IPowModulo,
-    panic::PanicException, types::PyModule, GILPool, IntoPyPointer, Py, PyResult, Python,
+    callback::PyCallbackOutput,
+    ffi,
+    impl_::{panic::PanicTrap, pymethods::GetSetDefClosure},
+    methods::IPowModulo,
+    panic::PanicException,
+    types::PyModule,
+    GILPool, IntoPyPointer, Py, PyResult, Python,
 };
 
 #[inline]
@@ -65,26 +70,34 @@ trampolines!(
 );
 
 #[inline]
-pub unsafe fn getter(
+pub(crate) unsafe extern "C" 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))
+    // Safety: PyO3 sets the closure when constructing the ffi getter so this cast should always be valid
+    let closure: &GetSetDefClosure = &*(closure as *const GetSetDefClosure);
+    // Safety: PyO3 sets the getter field when constructing the ffi getter
+    let getter = match closure.getter {
+        Some(getter) => getter,
+        None => std::hint::unreachable_unchecked(),
+    };
+    trampoline_inner(|py| getter(py, slf))
 }
 
 #[inline]
-pub unsafe fn setter(
+pub(crate) unsafe extern "C" 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>,
 ) -> c_int {
-    // PyO3 doesn't use the closure argument at present.
-    debug_assert!(closure.is_null());
-    trampoline_inner(|py| f(py, slf, value))
+    // Safety: PyO3 sets the closure when constructing the ffi setter so this cast should always be valid
+    let closure: &GetSetDefClosure = &*(closure as *const GetSetDefClosure);
+    // Safety: PyO3 sets the setter field when constructing the ffi setter
+    let setter = match closure.setter {
+        Some(setter) => setter,
+        None => std::hint::unreachable_unchecked(),
+    };
+    trampoline_inner(|py| setter(py, slf, value))
 }
 
 // Trampolines used by slot methods

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.