cleans up PyCFunction::internal_new (#3910)

This deduplicates some code around `PyCFunction::internal_new`

Author: Icxolu

pyo3-macros-backend/src/pyfunction.rs

@@ -273,7 +273,7 @@ pub fn impl_wrap_pyfunction(
             pub fn add_to_module(module: &#krate::Bound<'_, #krate::types::PyModule>) -> #krate::PyResult<()> {
                 use #krate::prelude::PyModuleMethods;
                 use ::std::convert::Into;
-                module.add_function(#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?)
+                module.add_function(#krate::types::PyCFunction::internal_new(module.py(), &DEF, module.into())?)
             }
         }
 

src/impl_/pyfunction.rs

@@ -1,6 +1,6 @@
 use crate::{
     types::{PyCFunction, PyModule},
-    Borrowed, Bound, PyResult, Python,
+    Borrowed, Bound, PyNativeType, PyResult, Python,
 };
 
 pub use crate::impl_::pymethods::PyMethodDef;
@@ -13,39 +13,40 @@ pub trait WrapPyFunctionArg<'py, 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_bound(self.py(), method_def, Some(&self))
+        PyCFunction::internal_new(self.py(), method_def, 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_bound(self.py(), method_def, Some(self))
+        PyCFunction::internal_new(self.py(), method_def, 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_bound(self.py(), method_def, Some(&self))
+        PyCFunction::internal_new(self.py(), method_def, 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_bound(self.py(), method_def, Some(self))
+        PyCFunction::internal_new(self.py(), method_def, Some(self))
     }
 }
 
 // For Python<'py>, only the GIL Ref form exists to avoid causing type inference to kick in.
 // The `wrap_pyfunction_bound!` macro is needed for the Bound form.
 impl<'py> WrapPyFunctionArg<'py, &'py PyCFunction> for Python<'py> {
     fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<&'py PyCFunction> {
-        PyCFunction::internal_new(method_def, self.into()).map(Bound::into_gil_ref)
+        PyCFunction::internal_new(self, method_def, None).map(Bound::into_gil_ref)
     }
 }
 
 impl<'py> WrapPyFunctionArg<'py, &'py PyCFunction> for &'py PyModule {
     fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<&'py PyCFunction> {
-        PyCFunction::internal_new(method_def, self.into()).map(Bound::into_gil_ref)
+        PyCFunction::internal_new(self.py(), method_def, Some(&self.as_borrowed()))
+            .map(Bound::into_gil_ref)
     }
 }
 
@@ -63,6 +64,6 @@ where
 
 impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for OnlyBound<Python<'py>> {
     fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult<Bound<'py, PyCFunction>> {
-        PyCFunction::internal_new_bound(self.0, method_def, None)
+        PyCFunction::internal_new(self.0, method_def, None)
     }
 }

src/types/function.rs

@@ -9,7 +9,7 @@ use crate::{
     impl_::pymethods::{self, PyMethodDef},
     types::{PyCapsule, PyDict, PyModule, PyString, PyTuple},
 };
-use crate::{Bound, IntoPy, Py, PyAny, PyResult, Python};
+use crate::{Bound, IntoPy, Py, PyAny, PyNativeType, PyResult, Python};
 use std::cell::UnsafeCell;
 use std::ffi::CStr;
 
@@ -34,13 +34,15 @@ impl PyCFunction {
         doc: &'static str,
         py_or_module: PyFunctionArguments<'a>,
     ) -> PyResult<&'a Self> {
+        let (py, module) = py_or_module.into_py_and_maybe_module();
         Self::internal_new(
+            py,
             &PyMethodDef::cfunction_with_keywords(
                 name,
                 pymethods::PyCFunctionWithKeywords(fun),
                 doc,
             ),
-            py_or_module,
+            module.map(PyNativeType::as_borrowed).as_deref(),
         )
         .map(Bound::into_gil_ref)
     }
@@ -53,7 +55,7 @@ impl PyCFunction {
         doc: &'static str,
         module: Option<&Bound<'py, PyModule>>,
     ) -> PyResult<Bound<'py, Self>> {
-        Self::internal_new_bound(
+        Self::internal_new(
             py,
             &PyMethodDef::cfunction_with_keywords(
                 name,
@@ -78,9 +80,11 @@ impl PyCFunction {
         doc: &'static str,
         py_or_module: PyFunctionArguments<'a>,
     ) -> PyResult<&'a Self> {
+        let (py, module) = py_or_module.into_py_and_maybe_module();
         Self::internal_new(
+            py,
             &PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc),
-            py_or_module,
+            module.map(PyNativeType::as_borrowed).as_deref(),
         )
         .map(Bound::into_gil_ref)
     }
@@ -93,7 +97,7 @@ impl PyCFunction {
         doc: &'static str,
         module: Option<&Bound<'py, PyModule>>,
     ) -> PyResult<Bound<'py, Self>> {
-        Self::internal_new_bound(
+        Self::internal_new(
             py,
             &PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc),
             module,
@@ -180,35 +184,6 @@ impl PyCFunction {
 
     #[doc(hidden)]
     pub fn internal_new<'py>(
-        method_def: &PyMethodDef,
-        py_or_module: PyFunctionArguments<'py>,
-    ) -> PyResult<Bound<'py, Self>> {
-        let (py, module) = py_or_module.into_py_and_maybe_module();
-        let (mod_ptr, module_name): (_, Option<Py<PyString>>) = if let Some(m) = module {
-            let mod_ptr = m.as_ptr();
-            (mod_ptr, Some(m.name()?.into_py(py)))
-        } else {
-            (std::ptr::null_mut(), None)
-        };
-        let (def, destructor) = method_def.as_method_def()?;
-
-        // FIXME: stop leaking the def and destructor
-        let def = Box::into_raw(Box::new(def));
-        std::mem::forget(destructor);
-
-        let module_name_ptr = module_name
-            .as_ref()
-            .map_or(std::ptr::null_mut(), Py::as_ptr);
-
-        unsafe {
-            ffi::PyCFunction_NewEx(def, mod_ptr, module_name_ptr)
-                .assume_owned_or_err(py)
-                .downcast_into_unchecked()
-        }
-    }
-
-    #[doc(hidden)]
-    pub(crate) fn internal_new_bound<'py>(
         py: Python<'py>,
         method_def: &PyMethodDef,
         module: Option<&Bound<'py, PyModule>>,