From e145ae851adca553f62b0f9a0466605cd55fe34a Mon Sep 17 00:00:00 2001 From: Lily Foote Date: Fri, 23 Feb 2024 14:07:54 +0000 Subject: [PATCH] Update new_closure_bound closure signature (#3883) * Update new_closure_bound closure signature Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> * Use anonymous lifetimes in closure bounds Co-authored-by: David Hewitt * Take &Bound in PyCFunction closures --------- Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> Co-authored-by: David Hewitt --- src/types/function.rs | 28 ++++++++++++++++++---------- tests/test_pyfunction.rs | 17 ++++++++++------- tests/ui/invalid_closure.rs | 9 +++++---- tests/ui/invalid_closure.stderr | 4 ++-- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/types/function.rs b/src/types/function.rs index 6c7db09a..7cbb05e2 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -101,7 +101,10 @@ impl PyCFunction { F: Fn(&PyTuple, Option<&PyDict>) -> R + Send + 'static, R: crate::callback::IntoPyCallbackOutput<*mut ffi::PyObject>, { - Self::new_closure_bound(py, name, doc, closure).map(Bound::into_gil_ref) + Self::new_closure_bound(py, name, doc, move |args, kwargs| { + closure(args.as_gil_ref(), kwargs.map(Bound::as_gil_ref)) + }) + .map(Bound::into_gil_ref) } /// Create a new function from a closure. @@ -113,7 +116,7 @@ impl PyCFunction { /// # use pyo3::{py_run, types::{PyCFunction, PyDict, PyTuple}}; /// /// Python::with_gil(|py| { - /// let add_one = |args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<_> { + /// let add_one = |args: &Bound<'_, PyTuple>, _kwargs: Option<&Bound<'_, PyDict>>| -> PyResult<_> { /// let i = args.extract::<(i64,)>()?.0; /// Ok(i+1) /// }; @@ -121,14 +124,14 @@ impl PyCFunction { /// py_run!(py, add_one, "assert add_one(42) == 43"); /// }); /// ``` - pub fn new_closure_bound<'a, F, R>( - py: Python<'a>, + pub fn new_closure_bound<'py, F, R>( + py: Python<'py>, name: Option<&'static str>, doc: Option<&'static str>, closure: F, - ) -> PyResult> + ) -> PyResult> where - F: Fn(&PyTuple, Option<&PyDict>) -> R + Send + 'static, + F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static, R: crate::callback::IntoPyCallbackOutput<*mut ffi::PyObject>, { let method_def = pymethods::PyMethodDef::cfunction_with_keywords( @@ -199,9 +202,11 @@ unsafe extern "C" fn run_closure( kwargs: *mut ffi::PyObject, ) -> *mut ffi::PyObject where - F: Fn(&PyTuple, Option<&PyDict>) -> R + Send + 'static, + F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static, R: crate::callback::IntoPyCallbackOutput<*mut ffi::PyObject>, { + use crate::types::any::PyAnyMethods; + crate::impl_::trampoline::cfunction_with_keywords( capsule_ptr, args, @@ -210,9 +215,12 @@ where let boxed_fn: &ClosureDestructor = &*(ffi::PyCapsule_GetPointer(capsule_ptr, closure_capsule_name().as_ptr()) as *mut ClosureDestructor); - let args = py.from_borrowed_ptr::(args); - let kwargs = py.from_borrowed_ptr_or_opt::(kwargs); - crate::callback::convert(py, (boxed_fn.closure)(args, kwargs)) + let args = Bound::ref_from_ptr(py, &args).downcast_unchecked::(); + let kwargs = Bound::ref_from_ptr_or_opt(py, &kwargs) + .as_ref() + .map(|b| b.downcast_unchecked::()); + let result = (boxed_fn.closure)(args, kwargs); + crate::callback::convert(py, result) }, ) } diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index e06f7c2f..14748418 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -401,7 +401,9 @@ fn test_pycfunction_new_with_keywords() { #[test] fn test_closure() { Python::with_gil(|py| { - let f = |args: &types::PyTuple, _kwargs: Option<&types::PyDict>| -> PyResult<_> { + let f = |args: &Bound<'_, types::PyTuple>, + _kwargs: Option<&Bound<'_, types::PyDict>>| + -> PyResult<_> { Python::with_gil(|py| { let res: Vec<_> = args .iter() @@ -439,12 +441,13 @@ fn test_closure() { fn test_closure_counter() { Python::with_gil(|py| { let counter = std::cell::RefCell::new(0); - let counter_fn = - move |_args: &types::PyTuple, _kwargs: Option<&types::PyDict>| -> PyResult { - let mut counter = counter.borrow_mut(); - *counter += 1; - Ok(*counter) - }; + let counter_fn = move |_args: &Bound<'_, types::PyTuple>, + _kwargs: Option<&Bound<'_, types::PyDict>>| + -> PyResult { + let mut counter = counter.borrow_mut(); + *counter += 1; + Ok(*counter) + }; let counter_py = PyCFunction::new_closure_bound(py, None, None, counter_fn).unwrap(); py_assert!(py, counter_py, "counter_py() == 1"); diff --git a/tests/ui/invalid_closure.rs b/tests/ui/invalid_closure.rs index 0613b027..eca988f1 100644 --- a/tests/ui/invalid_closure.rs +++ b/tests/ui/invalid_closure.rs @@ -6,10 +6,11 @@ fn main() { let local_data = vec![0, 1, 2, 3, 4]; let ref_: &[u8] = &local_data; - let closure_fn = |_args: &PyTuple, _kwargs: Option<&PyDict>| -> PyResult<()> { - println!("This is five: {:?}", ref_.len()); - Ok(()) - }; + let closure_fn = + |_args: &Bound<'_, PyTuple>, _kwargs: Option<&Bound<'_, PyDict>>| -> PyResult<()> { + println!("This is five: {:?}", ref_.len()); + Ok(()) + }; PyCFunction::new_closure_bound(py, None, None, closure_fn) .unwrap() .into() diff --git a/tests/ui/invalid_closure.stderr b/tests/ui/invalid_closure.stderr index 7fed8908..890d7640 100644 --- a/tests/ui/invalid_closure.stderr +++ b/tests/ui/invalid_closure.stderr @@ -6,8 +6,8 @@ error[E0597]: `local_data` does not live long enough 7 | let ref_: &[u8] = &local_data; | ^^^^^^^^^^^ borrowed value does not live long enough ... -13 | PyCFunction::new_closure_bound(py, None, None, closure_fn) +14 | PyCFunction::new_closure_bound(py, None, None, closure_fn) | ---------------------------------------------------------- argument requires that `local_data` is borrowed for `'static` ... -16 | }); +17 | }); | - `local_data` dropped here while still borrowed