stop leaking PyMethodDef inside macro-generated code

newsfragments/5459.fixed.md

@@ -0,0 +1 @@
+Stop leaking `PyMethodDef` instances inside `#[pyfunction]` macro generated code.

pyo3-macros-backend/src/pyfunction.rs

@@ -433,17 +433,18 @@ pub fn impl_wrap_pyfunction(
         #[doc(hidden)]
         #vis mod #name {
             pub(crate) struct MakeDef;
-            pub const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = MakeDef::_PYO3_DEF;
+            pub static _PYO3_DEF: #pyo3_path::impl_::pyfunction::PyFunctionDef = MakeDef::_PYO3_DEF;
             #introspection_id
         }
 
-        // Generate the definition inside an anonymous function in the same scope as the original function -
+        // Generate the definition in the same scope as the original function -
         // this avoids complications around the fact that the generated module has a different scope
         // (and `super` doesn't always refer to the outer scope, e.g. if the `#[pyfunction] is
         // inside a function body)
         #[allow(unknown_lints, non_local_definitions)]
         impl #name::MakeDef {
-            const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = #methoddef;
+            const _PYO3_DEF: #pyo3_path::impl_::pyfunction::PyFunctionDef =
+                #pyo3_path::impl_::pyfunction::PyFunctionDef::from_method_def(#methoddef);
         }
 
         #[allow(non_snake_case)]

src/impl_/pyclass.rs

@@ -1444,7 +1444,7 @@ mod tests {
 
         for items in FrozenClass::items_iter() {
             methods.extend(items.methods.iter().map(|m| match m {
-                MaybeRuntimePyMethodDef::Static(m) => m.clone(),
+                MaybeRuntimePyMethodDef::Static(m) => *m,
                 MaybeRuntimePyMethodDef::Runtime(r) => r(),
             }));
             slots.extend_from_slice(items.slots);
@@ -1482,7 +1482,7 @@ mod tests {
 
         for items in FrozenClass::items_iter() {
             methods.extend(items.methods.iter().map(|m| match m {
-                MaybeRuntimePyMethodDef::Static(m) => m.clone(),
+                MaybeRuntimePyMethodDef::Static(m) => *m,
                 MaybeRuntimePyMethodDef::Runtime(r) => r(),
             }));
             slots.extend_from_slice(items.slots);

src/impl_/pyfunction.rs

@@ -1,42 +1,152 @@
+use std::cell::UnsafeCell;
+
 use crate::{
-    types::{PyCFunction, PyModule},
+    ffi,
+    ffi_ptr_ext::FfiPtrExt,
+    py_result_ext::PyResultExt,
+    types::{PyCFunction, PyModule, PyModuleMethods},
     Borrowed, Bound, PyResult, Python,
 };
 
 pub use crate::impl_::pymethods::PyMethodDef;
 
+/// Wrapper around `ffi::PyMethodDef` suitable to use as a static variable for `#[pyfunction]` values.
+///
+/// The `UnsafeCell` is used because the Python interpreter consumes these as `*mut ffi::PyMethodDef`.
+pub struct PyFunctionDef(UnsafeCell<ffi::PyMethodDef>);
+
+// Safety: contents are only ever used by the Python interpreter, which uses global statics in this way.
+unsafe impl Sync for PyFunctionDef {}
+
+impl PyFunctionDef {
+    pub const fn new(def: ffi::PyMethodDef) -> Self {
+        Self(UnsafeCell::new(def))
+    }
+
+    pub const fn from_method_def(def: PyMethodDef) -> Self {
+        Self::new(def.into_raw())
+    }
+
+    pub(crate) fn create_py_c_function<'py>(
+        &'static self,
+        py: Python<'py>,
+        module: Option<&Bound<'py, PyModule>>,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        // Safety: self is static
+        unsafe { create_py_c_function(py, self.0.get(), module) }
+    }
+}
+
 /// Trait to enable the use of `wrap_pyfunction` with both `Python` and `PyModule`,
 /// and also to infer the return type of either `&'py PyCFunction` or `Bound<'py, PyCFunction>`.
 pub trait WrapPyFunctionArg<'py, T> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<T>;
+    fn wrap_pyfunction(self, function_def: &'static PyFunctionDef) -> PyResult<T>;
 }
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Bound<'py, PyModule> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new(self.py(), method_def, Some(&self))
+    fn wrap_pyfunction(
+        self,
+        function_def: &'static PyFunctionDef,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        function_def.create_py_c_function(self.py(), Some(&self))
     }
 }
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Bound<'py, PyModule> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new(self.py(), method_def, Some(self))
+    fn wrap_pyfunction(
+        self,
+        function_def: &'static PyFunctionDef,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        function_def.create_py_c_function(self.py(), Some(self))
     }
 }
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Borrowed<'_, 'py, PyModule> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new(self.py(), method_def, Some(&self))
+    fn wrap_pyfunction(
+        self,
+        function_def: &'static PyFunctionDef,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        function_def.create_py_c_function(self.py(), Some(&self))
     }
 }
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Borrowed<'_, 'py, PyModule> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new(self.py(), method_def, Some(self))
+    fn wrap_pyfunction(
+        self,
+        function_def: &'static PyFunctionDef,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        function_def.create_py_c_function(self.py(), Some(self))
     }
 }
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Python<'py> {
-    fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new(self, method_def, None)
+    fn wrap_pyfunction(
+        self,
+        function_def: &'static PyFunctionDef,
+    ) -> PyResult<Bound<'py, PyCFunction>> {
+        function_def.create_py_c_function(self, None)
+    }
+}
+
+/// Creates a `PyCFunction` object from a `PyMethodDef`.
+///
+/// # Safety
+///
+/// The `method_def` pointer must be valid for the lifetime of the returned `PyCFunction`
+/// (effectively, it must be a static variable).
+pub unsafe fn create_py_c_function<'py>(
+    py: Python<'py>,
+    method_def: *mut ffi::PyMethodDef,
+    module: Option<&Bound<'py, PyModule>>,
+) -> PyResult<Bound<'py, PyCFunction>> {
+    let (mod_ptr, module_name) = if let Some(m) = module {
+        let mod_ptr = m.as_ptr();
+        (mod_ptr, Some(m.name()?))
+    } else {
+        (std::ptr::null_mut(), None)
+    };
+
+    let module_name_ptr = module_name
+        .as_ref()
+        .map_or(std::ptr::null_mut(), Bound::as_ptr);
+
+    unsafe {
+        ffi::PyCFunction_NewEx(method_def, mod_ptr, module_name_ptr)
+            .assume_owned_or_err(py)
+            .cast_into_unchecked()
+    }
+}
+
+#[cfg(test)]
+#[cfg(feature = "macros")]
+mod tests {
+    #[test]
+    fn test_wrap_pyfunction_forms() {
+        use crate::types::{PyAnyMethods, PyModule};
+        use crate::{wrap_pyfunction, Python};
+
+        #[crate::pyfunction(crate = "crate")]
+        fn f() {}
+
+        Python::attach(|py| {
+            let module = PyModule::new(py, "test_wrap_pyfunction_forms").unwrap();
+
+            let func = wrap_pyfunction!(f, module.clone()).unwrap();
+            func.call0().unwrap();
+
+            let func = wrap_pyfunction!(f, &module).unwrap();
+            func.call0().unwrap();
+
+            let module_borrowed = module.as_borrowed();
+
+            let func = wrap_pyfunction!(f, module_borrowed).unwrap();
+            func.call0().unwrap();
+
+            let func = wrap_pyfunction!(f, &module_borrowed).unwrap();
+            func.call0().unwrap();
+
+            let func = wrap_pyfunction!(f, py).unwrap();
+            func.call0().unwrap();
+        });
     }
 }

src/impl_/pymethods.rs

@@ -58,7 +58,7 @@ impl IPowModulo {
 
 /// `PyMethodDefType` represents different types of Python callable objects.
 /// It is used by the `#[pymethods]` attribute.
-#[cfg_attr(test, derive(Clone))]
+#[derive(Copy, Clone)]
 pub enum PyMethodDefType {
     /// Represents class method
     Class(PyMethodDef),
@@ -86,10 +86,7 @@ pub enum PyMethodType {
 
 pub type PyClassAttributeFactory = for<'p> fn(Python<'p>) -> PyResult<Py<PyAny>>;
 
-// TODO: it would be nice to use CStr in these types, but then the constructors can't be const fn
-// until `CStr::from_bytes_with_nul_unchecked` is const fn.
-
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub struct PyMethodDef {
     pub(crate) ml_name: &'static CStr,
     pub(crate) ml_meth: PyMethodType,
@@ -103,26 +100,20 @@ pub struct PyClassAttributeDef {
     pub(crate) meth: PyClassAttributeFactory,
 }
 
-#[derive(Clone)]
+#[derive(Copy, Clone)]
 pub struct PyGetterDef {
     pub(crate) name: &'static CStr,
     pub(crate) meth: Getter,
     pub(crate) doc: &'static CStr,
 }
 
-#[derive(Clone)]
+#[derive(Copy, Clone)]
 pub struct PySetterDef {
     pub(crate) name: &'static CStr,
     pub(crate) meth: Setter,
     pub(crate) doc: &'static CStr,
 }
 
-unsafe impl Sync for PyMethodDef {}
-
-unsafe impl Sync for PyGetterDef {}
-
-unsafe impl Sync for PySetterDef {}
-
 impl PyMethodDef {
     /// Define a function with no `*args` and `**kwargs`.
     pub const fn noargs(
@@ -172,8 +163,7 @@ impl PyMethodDef {
         self
     }
 
-    /// Convert `PyMethodDef` to Python method definition struct `ffi::PyMethodDef`
-    pub(crate) fn as_method_def(&self) -> ffi::PyMethodDef {
+    pub const fn into_raw(self) -> ffi::PyMethodDef {
         let meth = match self.ml_meth {
             PyMethodType::PyCFunction(meth) => ffi::PyMethodDefPointer { PyCFunction: meth },
             PyMethodType::PyCFunctionWithKeywords(meth) => ffi::PyMethodDefPointer {
@@ -737,10 +727,21 @@ mod tests {
     #[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
     fn test_fastcall_function_with_keywords() {
         use super::PyMethodDef;
-        use crate::types::{PyAnyMethods, PyCFunction};
+        use crate::impl_::pyfunction::PyFunctionDef;
+        use crate::types::PyAnyMethods;
         use crate::{ffi, Python};
 
         Python::attach(|py| {
+            let def =
+                PyFunctionDef::from_method_def(PyMethodDef::fastcall_cfunction_with_keywords(
+                    ffi::c_str!("test"),
+                    accepts_no_arguments,
+                    ffi::c_str!("doc"),
+                ));
+            // leak to make it 'static
+            // deliberately done at runtime to have coverage of `PyFunctionDef::from_method_def`
+            let def = Box::leak(Box::new(def));
+
             unsafe extern "C" fn accepts_no_arguments(
                 _slf: *mut ffi::PyObject,
                 _args: *const *mut ffi::PyObject,
@@ -752,16 +753,7 @@ mod tests {
                 unsafe { Python::assume_attached().None().into_ptr() }
             }
 
-            let f = PyCFunction::internal_new(
-                py,
-                &PyMethodDef::fastcall_cfunction_with_keywords(
-                    ffi::c_str!("test"),
-                    accepts_no_arguments,
-                    ffi::c_str!("doc"),
-                ),
-                None,
-            )
-            .unwrap();
+            let f = def.create_py_c_function(py, None).unwrap();
 
             f.call0().unwrap();
         });

src/impl_/pymodule.rs

@@ -25,9 +25,9 @@ use crate::prelude::PyTypeMethods;
 use crate::PyErr;
 use crate::{
     ffi,
-    impl_::pymethods::PyMethodDef,
+    impl_::pyfunction::PyFunctionDef,
     sync::PyOnceLock,
-    types::{PyCFunction, PyModule, PyModuleMethods},
+    types::{PyModule, PyModuleMethods},
     Bound, Py, PyClass, PyResult, PyTypeInfo, Python,
 };
 
@@ -203,9 +203,10 @@ impl<T: PyClass> PyAddToModule for AddClassToModule<T> {
 }
 
 /// For adding a function to a module.
-impl PyAddToModule for PyMethodDef {
+impl PyAddToModule for PyFunctionDef {
     fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> {
-        module.add_function(PyCFunction::internal_new(module.py(), self, Some(module))?)
+        // safety: self is static
+        module.add_function(self.create_py_c_function(module.py(), Some(module))?)
     }
 }
 

src/pyclass/create_type_object.rs

@@ -195,7 +195,7 @@ impl PyTypeBuilder {
                 .add_setter(setter),
             PyMethodDefType::Method(def)
             | PyMethodDefType::Class(def)
-            | PyMethodDefType::Static(def) => self.method_defs.push(def.as_method_def()),
+            | PyMethodDefType::Static(def) => self.method_defs.push(def.into_raw()),
             // These class attributes are added after the type gets created by LazyStaticType
             PyMethodDefType::ClassAttribute(_) => {}
             PyMethodDefType::StructMember(def) => self.member_defs.push(*def),

src/sealed.rs

@@ -1,3 +1,4 @@
+use crate::impl_::pyfunction::PyFunctionDef;
 use crate::types::{
     PyBool, PyByteArray, PyBytes, PyCapsule, PyComplex, PyDict, PyFloat, PyFrozenSet, PyList,
     PyMapping, PyMappingProxy, PyModule, PyRange, PySequence, PySet, PySlice, PyString,
@@ -50,6 +51,7 @@ impl Sealed for Bound<'_, PyWeakrefReference> {}
 impl<T> Sealed for AddTypeToModule<T> {}
 impl<T> Sealed for AddClassToModule<T> {}
 impl Sealed for PyMethodDef {}
+impl Sealed for PyFunctionDef {}
 impl Sealed for ModuleDef {}
 
 impl<T: crate::type_object::PyTypeInfo> Sealed for PyNativeTypeInitializer<T> {}