From 8a12970c9694f8340318235173e53b80ee9d4ff7 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 28 Feb 2024 19:36:20 +0000 Subject: [PATCH] update `extract_argument` to use Bound APIs (#3708) * update `extract_argument` to use `Bound` APIs * tidy up borrow in macros expression * update `trybuild` output * more concise form for `DowncastError::new` Co-authored-by: Lily Foote * use `Borrowed` instead of newtype * use `Borrowed::from_ptr` methods in extract_argument * update UI tests * avoid double-negative `#[cfg]` clauses Co-authored-by: Lily Foote * review: LilyFoote, Icxolu feedback --------- Co-authored-by: Lily Foote --- guide/src/class.md | 4 +- pyo3-benches/benches/bench_comparisons.rs | 8 +- pyo3-macros-backend/src/method.rs | 12 +- pyo3-macros-backend/src/params.rs | 18 +-- pyo3-macros-backend/src/pyclass.rs | 6 +- pyo3-macros-backend/src/pymethod.rs | 31 ++-- pytests/src/pyfunctions.rs | 74 +++++----- src/impl_/extract_argument.rs | 172 ++++++++++++++-------- src/impl_/pymethods.rs | 17 ++- src/types/dict.rs | 68 +++++++++ src/types/string.rs | 2 +- src/types/tuple.rs | 6 +- 12 files changed, 274 insertions(+), 144 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index 9662e8f6..8ce896f5 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -1254,7 +1254,7 @@ impl<'a, 'py> pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a type Holder = ::std::option::Option>; #[inline] - fn extract(obj: &'py pyo3::PyAny, holder: &'a mut Self::Holder) -> pyo3::PyResult { + fn extract(obj: &'a pyo3::Bound<'py, PyAny>, holder: &'a mut Self::Holder) -> pyo3::PyResult { pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1264,7 +1264,7 @@ impl<'a, 'py> pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a type Holder = ::std::option::Option>; #[inline] - fn extract(obj: &'py pyo3::PyAny, holder: &'a mut Self::Holder) -> pyo3::PyResult { + fn extract(obj: &'a pyo3::Bound<'py, PyAny>, holder: &'a mut Self::Holder) -> pyo3::PyResult { pyo3::impl_::extract_argument::extract_pyclass_ref_mut(obj, holder) } } diff --git a/pyo3-benches/benches/bench_comparisons.rs b/pyo3-benches/benches/bench_comparisons.rs index ffd4c1a4..fbd473f0 100644 --- a/pyo3-benches/benches/bench_comparisons.rs +++ b/pyo3-benches/benches/bench_comparisons.rs @@ -45,8 +45,8 @@ impl OrderedRichcmp { fn bench_ordered_dunder_methods(b: &mut Bencher<'_>) { Python::with_gil(|py| { - let obj1 = Py::new(py, OrderedDunderMethods(0)).unwrap().into_ref(py); - let obj2 = Py::new(py, OrderedDunderMethods(1)).unwrap().into_ref(py); + let obj1 = &Bound::new(py, OrderedDunderMethods(0)).unwrap().into_any(); + let obj2 = &Bound::new(py, OrderedDunderMethods(1)).unwrap().into_any(); b.iter(|| obj2.gt(obj1).unwrap()); }); @@ -54,8 +54,8 @@ fn bench_ordered_dunder_methods(b: &mut Bencher<'_>) { fn bench_ordered_richcmp(b: &mut Bencher<'_>) { Python::with_gil(|py| { - let obj1 = Py::new(py, OrderedRichcmp(0)).unwrap().into_ref(py); - let obj2 = Py::new(py, OrderedRichcmp(1)).unwrap().into_ref(py); + let obj1 = &Bound::new(py, OrderedRichcmp(0)).unwrap().into_any(); + let obj2 = &Bound::new(py, OrderedRichcmp(1)).unwrap().into_any(); b.iter(|| obj2.gt(obj1).unwrap()); }); diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 2d21c265..6ee87fb9 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -196,10 +196,11 @@ impl SelfType { holders.push(quote_spanned! { *span => #[allow(clippy::let_unit_value)] let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT; + let mut #slf = _pyo3::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf); }); error_mode.handle_error(quote_spanned! { *span => _pyo3::impl_::extract_argument::#method::<#cls>( - #py.from_borrowed_ptr::<_pyo3::PyAny>(#slf), + &#slf, &mut #holder, ) }) @@ -582,7 +583,8 @@ impl<'a> FnSpec<'a> { ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { let function = #rust_name; // Shadow the function name to avoid #3017 #( #holders )* - #call + let result = #call; + result } } } @@ -601,7 +603,8 @@ impl<'a> FnSpec<'a> { let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #( #holders )* - #call + let result = #call; + result } } } @@ -619,7 +622,8 @@ impl<'a> FnSpec<'a> { let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #( #holders )* - #call + let result = #call; + result } } } diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 3781b41b..679d3e42 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -44,8 +44,8 @@ pub fn impl_arg_params( .collect::>()?; return Ok(( quote! { - let _args = py.from_borrowed_ptr::<_pyo3::types::PyTuple>(_args); - let _kwargs: ::std::option::Option<&_pyo3::types::PyDict> = py.from_borrowed_ptr_or_opt(_kwargs); + let _args = _pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &_args); + let _kwargs = _pyo3::impl_::pymethods::BoundRef::ref_from_ptr_or_opt(py, &_kwargs); }, arg_convert, )); @@ -180,7 +180,7 @@ fn impl_arg_param( let holder = push_holder(); return Ok(quote_arg_span! { _pyo3::impl_::extract_argument::extract_argument( - _args, + &_args, &mut #holder, #name_str )? @@ -193,7 +193,7 @@ fn impl_arg_param( let holder = push_holder(); return Ok(quote_arg_span! { _pyo3::impl_::extract_argument::extract_optional_argument( - _kwargs.map(::std::convert::AsRef::as_ref), + _kwargs.as_deref(), &mut #holder, #name_str, || ::std::option::Option::None @@ -217,7 +217,7 @@ fn impl_arg_param( quote_arg_span! { #[allow(clippy::redundant_closure)] _pyo3::impl_::extract_argument::from_py_with_with_default( - #arg_value.map(_pyo3::PyNativeType::as_borrowed).as_deref(), + #arg_value.as_deref(), #name_str, #expr_path as fn(_) -> _, || #default @@ -226,7 +226,7 @@ fn impl_arg_param( } else { quote_arg_span! { _pyo3::impl_::extract_argument::from_py_with( - &_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value).as_borrowed(), + &_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value), #name_str, #expr_path as fn(_) -> _, )? @@ -237,7 +237,7 @@ fn impl_arg_param( quote_arg_span! { #[allow(clippy::redundant_closure)] _pyo3::impl_::extract_argument::extract_optional_argument( - #arg_value, + #arg_value.as_deref(), &mut #holder, #name_str, || #default @@ -248,7 +248,7 @@ fn impl_arg_param( quote_arg_span! { #[allow(clippy::redundant_closure)] _pyo3::impl_::extract_argument::extract_argument_with_default( - #arg_value, + #arg_value.as_deref(), &mut #holder, #name_str, || #default @@ -258,7 +258,7 @@ fn impl_arg_param( let holder = push_holder(); quote_arg_span! { _pyo3::impl_::extract_argument::extract_argument( - _pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value), + &_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value), &mut #holder, #name_str )? diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 1547e78b..5ec6ac17 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1378,7 +1378,7 @@ impl<'a> PyClassImplsBuilder<'a> { type Holder = ::std::option::Option<_pyo3::PyRef<'py, #cls>>; #[inline] - fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + fn extract(obj: &'a _pyo3::Bound<'py, _pyo3::PyAny>, holder: &'a mut Self::Holder) -> _pyo3::PyResult { _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1390,7 +1390,7 @@ impl<'a> PyClassImplsBuilder<'a> { type Holder = ::std::option::Option<_pyo3::PyRef<'py, #cls>>; #[inline] - fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + fn extract(obj: &'a _pyo3::Bound<'py, _pyo3::PyAny>, holder: &'a mut Self::Holder) -> _pyo3::PyResult { _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1400,7 +1400,7 @@ impl<'a> PyClassImplsBuilder<'a> { type Holder = ::std::option::Option<_pyo3::PyRefMut<'py, #cls>>; #[inline] - fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + fn extract(obj: &'a _pyo3::Bound<'py, _pyo3::PyAny>, holder: &'a mut Self::Holder) -> _pyo3::PyResult { _pyo3::impl_::extract_argument::extract_pyclass_ref_mut(obj, holder) } } diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 358d327e..4cb07a9a 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -698,7 +698,8 @@ pub fn impl_py_getter_def( _slf: *mut _pyo3::ffi::PyObject ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { #( #holders )* - #body + let result = #body; + result } }; @@ -930,39 +931,31 @@ impl Ty { extract_error_mode, holders, &name_str, - quote! { - py.from_borrowed_ptr::<_pyo3::PyAny>(#ident) - }, + quote! { #ident }, ), Ty::MaybeNullObject => extract_object( extract_error_mode, holders, &name_str, quote! { - py.from_borrowed_ptr::<_pyo3::PyAny>( - if #ident.is_null() { - _pyo3::ffi::Py_None() - } else { - #ident - } - ) + if #ident.is_null() { + _pyo3::ffi::Py_None() + } else { + #ident + } }, ), Ty::NonNullObject => extract_object( extract_error_mode, holders, &name_str, - quote! { - py.from_borrowed_ptr::<_pyo3::PyAny>(#ident.as_ptr()) - }, + quote! { #ident.as_ptr() }, ), Ty::IPowModulo => extract_object( extract_error_mode, holders, &name_str, - quote! { - #ident.to_borrowed_any(py) - }, + quote! { #ident.as_ptr() }, ), Ty::CompareOp => extract_error_mode.handle_error( quote! { @@ -988,7 +981,7 @@ fn extract_object( extract_error_mode: ExtractErrorMode, holders: &mut Vec, name: &str, - source: TokenStream, + source_ptr: TokenStream, ) -> TokenStream { let holder = syn::Ident::new(&format!("holder_{}", holders.len()), Span::call_site()); holders.push(quote! { @@ -997,7 +990,7 @@ fn extract_object( }); extract_error_mode.handle_error(quote! { _pyo3::impl_::extract_argument::extract_argument( - #source, + &_pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &#source_ptr), &mut #holder, #name ) diff --git a/pytests/src/pyfunctions.rs b/pytests/src/pyfunctions.rs index 1eef9704..a92733c2 100644 --- a/pytests/src/pyfunctions.rs +++ b/pytests/src/pyfunctions.rs @@ -4,62 +4,66 @@ use pyo3::types::{PyDict, PyTuple}; #[pyfunction(signature = ())] fn none() {} +type Any<'py> = Bound<'py, PyAny>; +type Dict<'py> = Bound<'py, PyDict>; +type Tuple<'py> = Bound<'py, PyTuple>; + #[pyfunction(signature = (a, b = None, *, c = None))] -fn simple<'a>( - a: &'a PyAny, - b: Option<&'a PyAny>, - c: Option<&'a PyAny>, -) -> (&'a PyAny, Option<&'a PyAny>, Option<&'a PyAny>) { +fn simple<'py>( + a: Any<'py>, + b: Option>, + c: Option>, +) -> (Any<'py>, Option>, Option>) { (a, b, c) } #[pyfunction(signature = (a, b = None, *args, c = None))] -fn simple_args<'a>( - a: &'a PyAny, - b: Option<&'a PyAny>, - args: &'a PyTuple, - c: Option<&'a PyAny>, -) -> (&'a PyAny, Option<&'a PyAny>, &'a PyTuple, Option<&'a PyAny>) { +fn simple_args<'py>( + a: Any<'py>, + b: Option>, + args: Tuple<'py>, + c: Option>, +) -> (Any<'py>, Option>, Tuple<'py>, Option>) { (a, b, args, c) } #[pyfunction(signature = (a, b = None, c = None, **kwargs))] -fn simple_kwargs<'a>( - a: &'a PyAny, - b: Option<&'a PyAny>, - c: Option<&'a PyAny>, - kwargs: Option<&'a PyDict>, +fn simple_kwargs<'py>( + a: Any<'py>, + b: Option>, + c: Option>, + kwargs: Option>, ) -> ( - &'a PyAny, - Option<&'a PyAny>, - Option<&'a PyAny>, - Option<&'a PyDict>, + Any<'py>, + Option>, + Option>, + Option>, ) { (a, b, c, kwargs) } #[pyfunction(signature = (a, b = None, *args, c = None, **kwargs))] -fn simple_args_kwargs<'a>( - a: &'a PyAny, - b: Option<&'a PyAny>, - args: &'a PyTuple, - c: Option<&'a PyAny>, - kwargs: Option<&'a PyDict>, +fn simple_args_kwargs<'py>( + a: Any<'py>, + b: Option>, + args: Tuple<'py>, + c: Option>, + kwargs: Option>, ) -> ( - &'a PyAny, - Option<&'a PyAny>, - &'a PyTuple, - Option<&'a PyAny>, - Option<&'a PyDict>, + Any<'py>, + Option>, + Tuple<'py>, + Option>, + Option>, ) { (a, b, args, c, kwargs) } #[pyfunction(signature = (*args, **kwargs))] -fn args_kwargs<'a>( - args: &'a PyTuple, - kwargs: Option<&'a PyDict>, -) -> (&'a PyTuple, Option<&'a PyDict>) { +fn args_kwargs<'py>( + args: Tuple<'py>, + kwargs: Option>, +) -> (Tuple<'py>, Option>) { (args, kwargs) } diff --git a/src/impl_/extract_argument.rs b/src/impl_/extract_argument.rs index 27728c6d..d2e2b18c 100644 --- a/src/impl_/extract_argument.rs +++ b/src/impl_/extract_argument.rs @@ -2,10 +2,16 @@ use crate::{ exceptions::PyTypeError, ffi, pyclass::boolean_struct::False, - types::{PyDict, PyString, PyTuple}, - Bound, FromPyObject, PyAny, PyClass, PyErr, PyRef, PyRefMut, PyResult, PyTypeCheck, Python, + types::{any::PyAnyMethods, dict::PyDictMethods, tuple::PyTupleMethods, PyDict, PyTuple}, + Borrowed, Bound, FromPyObject, PyAny, PyClass, PyErr, PyRef, PyRefMut, PyResult, PyTypeCheck, + Python, }; +/// Helper type used to keep implementation more concise. +/// +/// (Function argument extraction borrows input arguments.) +type PyArg<'py> = Borrowed<'py, 'py, PyAny>; + /// A trait which is used to help PyO3 macros extract function arguments. /// /// `#[pyclass]` structs need to extract as `PyRef` and `PyRefMut` @@ -16,7 +22,7 @@ use crate::{ /// There exists a trivial blanket implementation for `T: FromPyObject` with `Holder = ()`. pub trait PyFunctionArgument<'a, 'py>: Sized + 'a { type Holder: FunctionArgumentHolder; - fn extract(obj: &'py PyAny, holder: &'a mut Self::Holder) -> PyResult; + fn extract(obj: &'a Bound<'py, PyAny>, holder: &'a mut Self::Holder) -> PyResult; } impl<'a, 'py, T> PyFunctionArgument<'a, 'py> for T @@ -26,20 +32,23 @@ where type Holder = (); #[inline] - fn extract(obj: &'py PyAny, _: &'a mut ()) -> PyResult { + fn extract(obj: &'a Bound<'py, PyAny>, _: &'a mut ()) -> PyResult { obj.extract() } } -impl<'a, 'py, T> PyFunctionArgument<'a, 'py> for &'a Bound<'py, T> +impl<'a, 'py, T: 'py> PyFunctionArgument<'a, 'py> for &'a Bound<'py, T> where T: PyTypeCheck, { - type Holder = Option>; + type Holder = Option<&'a Bound<'py, T>>; #[inline] - fn extract(obj: &'py PyAny, holder: &'a mut Option>) -> PyResult { - Ok(&*holder.insert(obj.extract()?)) + fn extract( + obj: &'a Bound<'py, PyAny>, + holder: &'a mut Option<&'a Bound<'py, T>>, + ) -> PyResult { + Ok(holder.insert(obj.downcast()?)) } } @@ -59,7 +68,7 @@ impl FunctionArgumentHolder for Option { #[inline] pub fn extract_pyclass_ref<'a, 'py: 'a, T: PyClass>( - obj: &'py PyAny, + obj: &'a Bound<'py, PyAny>, holder: &'a mut Option>, ) -> PyResult<&'a T> { Ok(&*holder.insert(obj.extract()?)) @@ -67,7 +76,7 @@ pub fn extract_pyclass_ref<'a, 'py: 'a, T: PyClass>( #[inline] pub fn extract_pyclass_ref_mut<'a, 'py: 'a, T: PyClass>( - obj: &'py PyAny, + obj: &'a Bound<'py, PyAny>, holder: &'a mut Option>, ) -> PyResult<&'a mut T> { Ok(&mut *holder.insert(obj.extract()?)) @@ -76,7 +85,7 @@ pub fn extract_pyclass_ref_mut<'a, 'py: 'a, T: PyClass>( /// The standard implementation of how PyO3 extracts a `#[pyfunction]` or `#[pymethod]` function argument. #[doc(hidden)] pub fn extract_argument<'a, 'py, T>( - obj: &'py PyAny, + obj: &'a Bound<'py, PyAny>, holder: &'a mut T::Holder, arg_name: &str, ) -> PyResult @@ -93,7 +102,7 @@ where /// does not implement `PyFunctionArgument` for `T: PyClass`. #[doc(hidden)] pub fn extract_optional_argument<'a, 'py, T>( - obj: Option<&'py PyAny>, + obj: Option<&'a Bound<'py, PyAny>>, holder: &'a mut T::Holder, arg_name: &str, default: fn() -> Option, @@ -117,7 +126,7 @@ where /// Alternative to [`extract_argument`] used when the argument has a default value provided by an annotation. #[doc(hidden)] pub fn extract_argument_with_default<'a, 'py, T>( - obj: Option<&'py PyAny>, + obj: Option<&'a Bound<'py, PyAny>>, holder: &'a mut T::Holder, arg_name: &str, default: fn() -> T, @@ -165,7 +174,6 @@ pub fn from_py_with_with_default<'a, 'py, T>( #[doc(hidden)] #[cold] pub fn argument_extraction_error(py: Python<'_>, arg_name: &str, error: PyErr) -> PyErr { - use crate::types::any::PyAnyMethods; if error .get_type_bound(py) .is(&py.get_type_bound::()) @@ -189,7 +197,7 @@ pub fn argument_extraction_error(py: Python<'_>, arg_name: &str, error: PyErr) - /// `argument` must not be `None` #[doc(hidden)] #[inline] -pub unsafe fn unwrap_required_argument(argument: Option<&PyAny>) -> &PyAny { +pub unsafe fn unwrap_required_argument(argument: Option>) -> PyArg<'_> { match argument { Some(value) => value, #[cfg(debug_assertions)] @@ -236,7 +244,7 @@ impl FunctionDescription { args: *const *mut ffi::PyObject, nargs: ffi::Py_ssize_t, kwnames: *mut ffi::PyObject, - output: &mut [Option<&'py PyAny>], + output: &mut [Option>], ) -> PyResult<(V::Varargs, K::Varkeywords)> where V: VarargsHandler<'py>, @@ -253,8 +261,10 @@ impl FunctionDescription { ); // Handle positional arguments - // Safety: Option<&PyAny> has the same memory layout as `*mut ffi::PyObject` - let args: *const Option<&PyAny> = args.cast(); + // Safety: + // - Option has the same memory layout as `*mut ffi::PyObject` + // - we both have the GIL and can borrow these input references for the `'py` lifetime. + let args: *const Option> = args.cast(); let positional_args_provided = nargs as usize; let remaining_positional_args = if args.is_null() { debug_assert_eq!(positional_args_provided, 0); @@ -274,13 +284,20 @@ impl FunctionDescription { // Handle keyword arguments let mut varkeywords = K::Varkeywords::default(); - if let Some(kwnames) = py.from_borrowed_ptr_or_opt::(kwnames) { - // Safety: &PyAny has the same memory layout as `*mut ffi::PyObject` - let kwargs = - ::std::slice::from_raw_parts((args as *const &PyAny).offset(nargs), kwnames.len()); + + // Safety: kwnames is known to be a pointer to a tuple, or null + // - we both have the GIL and can borrow this input reference for the `'py` lifetime. + let kwnames: Option> = + Borrowed::from_ptr_or_opt(py, kwnames).map(|kwnames| kwnames.downcast_unchecked()); + if let Some(kwnames) = kwnames { + // Safety: PyArg has the same memory layout as `*mut ffi::PyObject` + let kwargs = ::std::slice::from_raw_parts( + (args as *const PyArg<'py>).offset(nargs), + kwnames.len(), + ); self.handle_kwargs::( - kwnames.iter().zip(kwargs.iter().copied()), + kwnames.iter_borrowed().zip(kwargs.iter().copied()), &mut varkeywords, num_positional_parameters, output, @@ -312,14 +329,20 @@ impl FunctionDescription { py: Python<'py>, args: *mut ffi::PyObject, kwargs: *mut ffi::PyObject, - output: &mut [Option<&'py PyAny>], + output: &mut [Option>], ) -> PyResult<(V::Varargs, K::Varkeywords)> where V: VarargsHandler<'py>, K: VarkeywordsHandler<'py>, { - let args = py.from_borrowed_ptr::(args); - let kwargs: ::std::option::Option<&PyDict> = py.from_borrowed_ptr_or_opt(kwargs); + // Safety: + // - `args` is known to be a tuple + // - `kwargs` is known to be a dict or null + // - we both have the GIL and can borrow these input references for the `'py` lifetime. + let args: Borrowed<'py, 'py, PyTuple> = + Borrowed::from_ptr(py, args).downcast_unchecked::(); + let kwargs: Option> = + Borrowed::from_ptr_or_opt(py, kwargs).map(|kwargs| kwargs.downcast_unchecked()); let num_positional_parameters = self.positional_parameter_names.len(); @@ -331,17 +354,26 @@ impl FunctionDescription { ); // Copy positional arguments into output - for (i, arg) in args.iter().take(num_positional_parameters).enumerate() { + for (i, arg) in args + .iter_borrowed() + .take(num_positional_parameters) + .enumerate() + { output[i] = Some(arg); } // If any arguments remain, push them to varargs (if possible) or error - let varargs = V::handle_varargs_tuple(args, self)?; + let varargs = V::handle_varargs_tuple(&args, self)?; // Handle keyword arguments let mut varkeywords = K::Varkeywords::default(); if let Some(kwargs) = kwargs { - self.handle_kwargs::(kwargs, &mut varkeywords, num_positional_parameters, output)? + self.handle_kwargs::( + kwargs.iter_borrowed(), + &mut varkeywords, + num_positional_parameters, + output, + )? } // Once all inputs have been processed, check that all required arguments have been provided. @@ -358,11 +390,11 @@ impl FunctionDescription { kwargs: I, varkeywords: &mut K::Varkeywords, num_positional_parameters: usize, - output: &mut [Option<&'py PyAny>], + output: &mut [Option>], ) -> PyResult<()> where K: VarkeywordsHandler<'py>, - I: IntoIterator, + I: IntoIterator, PyArg<'py>)>, { debug_assert_eq!( num_positional_parameters, @@ -374,11 +406,21 @@ impl FunctionDescription { ); let mut positional_only_keyword_arguments = Vec::new(); for (kwarg_name_py, value) in kwargs { - // All keyword arguments should be UTF-8 strings, but we'll check, just in case. - // If it isn't, then it will be handled below as a varkeyword (which may raise an - // error if this function doesn't accept **kwargs). Rust source is always UTF-8 - // and so all argument names in `#[pyfunction]` signature must be UTF-8. - if let Ok(kwarg_name) = kwarg_name_py.downcast::()?.to_str() { + // Safety: All keyword arguments should be UTF-8 strings, but if it's not, `.to_str()` + // will return an error anyway. + #[cfg(any(Py_3_10, not(Py_LIMITED_API)))] + let kwarg_name = + unsafe { kwarg_name_py.downcast_unchecked::() }.to_str(); + + #[cfg(all(not(Py_3_10), Py_LIMITED_API))] + let kwarg_name = kwarg_name_py.extract::(); + + if let Ok(kwarg_name_owned) = kwarg_name { + #[cfg(any(Py_3_10, not(Py_LIMITED_API)))] + let kwarg_name = kwarg_name_owned; + #[cfg(all(not(Py_3_10), Py_LIMITED_API))] + let kwarg_name: &str = &kwarg_name_owned; + // Try to place parameter in keyword only parameters if let Some(i) = self.find_keyword_parameter_in_keyword_only(kwarg_name) { if output[i + num_positional_parameters] @@ -397,7 +439,7 @@ impl FunctionDescription { // kwarg to conflict with a postional-only argument - the value // will go into **kwargs anyway. if K::handle_varkeyword(varkeywords, kwarg_name_py, value, self).is_err() { - positional_only_keyword_arguments.push(kwarg_name); + positional_only_keyword_arguments.push(kwarg_name_owned); } } else if output[i].replace(value).is_some() { return Err(self.multiple_values_for_argument(kwarg_name)); @@ -410,6 +452,11 @@ impl FunctionDescription { } if !positional_only_keyword_arguments.is_empty() { + #[cfg(all(not(Py_3_10), Py_LIMITED_API))] + let positional_only_keyword_arguments: Vec<_> = positional_only_keyword_arguments + .iter() + .map(std::ops::Deref::deref) + .collect(); return Err(self.positional_only_keyword_arguments(&positional_only_keyword_arguments)); } @@ -436,7 +483,7 @@ impl FunctionDescription { #[inline] fn ensure_no_missing_required_positional_arguments( &self, - output: &[Option<&PyAny>], + output: &[Option>], positional_args_provided: usize, ) -> PyResult<()> { if positional_args_provided < self.required_positional_parameters { @@ -452,7 +499,7 @@ impl FunctionDescription { #[inline] fn ensure_no_missing_required_keyword_arguments( &self, - output: &[Option<&PyAny>], + output: &[Option>], ) -> PyResult<()> { let keyword_output = &output[self.positional_parameter_names.len()..]; for (param, out) in self.keyword_only_parameters.iter().zip(keyword_output) { @@ -497,11 +544,11 @@ impl FunctionDescription { } #[cold] - fn unexpected_keyword_argument(&self, argument: &PyAny) -> PyErr { + fn unexpected_keyword_argument(&self, argument: PyArg<'_>) -> PyErr { PyTypeError::new_err(format!( "{} got an unexpected keyword argument '{}'", self.full_name(), - argument + argument.as_any() )) } @@ -534,7 +581,7 @@ impl FunctionDescription { } #[cold] - fn missing_required_keyword_arguments(&self, keyword_outputs: &[Option<&PyAny>]) -> PyErr { + fn missing_required_keyword_arguments(&self, keyword_outputs: &[Option>]) -> PyErr { debug_assert_eq!(self.keyword_only_parameters.len(), keyword_outputs.len()); let missing_keyword_only_arguments: Vec<_> = self @@ -555,7 +602,7 @@ impl FunctionDescription { } #[cold] - fn missing_required_positional_arguments(&self, output: &[Option<&PyAny>]) -> PyErr { + fn missing_required_positional_arguments(&self, output: &[Option>]) -> PyErr { let missing_positional_arguments: Vec<_> = self .positional_parameter_names .iter() @@ -575,14 +622,14 @@ pub trait VarargsHandler<'py> { /// Called by `FunctionDescription::extract_arguments_fastcall` with any additional arguments. fn handle_varargs_fastcall( py: Python<'py>, - varargs: &[Option<&PyAny>], + varargs: &[Option>], function_description: &FunctionDescription, ) -> PyResult; /// Called by `FunctionDescription::extract_arguments_tuple_dict` with the original tuple. /// /// Additional arguments are those in the tuple slice starting from `function_description.positional_parameter_names.len()`. fn handle_varargs_tuple( - args: &'py PyTuple, + args: &Bound<'py, PyTuple>, function_description: &FunctionDescription, ) -> PyResult; } @@ -596,7 +643,7 @@ impl<'py> VarargsHandler<'py> for NoVarargs { #[inline] fn handle_varargs_fastcall( _py: Python<'py>, - varargs: &[Option<&PyAny>], + varargs: &[Option>], function_description: &FunctionDescription, ) -> PyResult { let extra_arguments = varargs.len(); @@ -610,7 +657,7 @@ impl<'py> VarargsHandler<'py> for NoVarargs { #[inline] fn handle_varargs_tuple( - args: &'py PyTuple, + args: &Bound<'py, PyTuple>, function_description: &FunctionDescription, ) -> PyResult { let positional_parameter_count = function_description.positional_parameter_names.len(); @@ -627,19 +674,19 @@ impl<'py> VarargsHandler<'py> for NoVarargs { pub struct TupleVarargs; impl<'py> VarargsHandler<'py> for TupleVarargs { - type Varargs = &'py PyTuple; + type Varargs = Bound<'py, PyTuple>; #[inline] fn handle_varargs_fastcall( py: Python<'py>, - varargs: &[Option<&PyAny>], + varargs: &[Option>], _function_description: &FunctionDescription, ) -> PyResult { - Ok(PyTuple::new_bound(py, varargs).into_gil_ref()) + Ok(PyTuple::new_bound(py, varargs)) } #[inline] fn handle_varargs_tuple( - args: &'py PyTuple, + args: &Bound<'py, PyTuple>, function_description: &FunctionDescription, ) -> PyResult { let positional_parameters = function_description.positional_parameter_names.len(); @@ -652,8 +699,8 @@ pub trait VarkeywordsHandler<'py> { type Varkeywords: Default; fn handle_varkeyword( varkeywords: &mut Self::Varkeywords, - name: &'py PyAny, - value: &'py PyAny, + name: PyArg<'py>, + value: PyArg<'py>, function_description: &FunctionDescription, ) -> PyResult<()>; } @@ -666,8 +713,8 @@ impl<'py> VarkeywordsHandler<'py> for NoVarkeywords { #[inline] fn handle_varkeyword( _varkeywords: &mut Self::Varkeywords, - name: &'py PyAny, - _value: &'py PyAny, + name: PyArg<'py>, + _value: PyArg<'py>, function_description: &FunctionDescription, ) -> PyResult<()> { Err(function_description.unexpected_keyword_argument(name)) @@ -678,28 +725,29 @@ impl<'py> VarkeywordsHandler<'py> for NoVarkeywords { pub struct DictVarkeywords; impl<'py> VarkeywordsHandler<'py> for DictVarkeywords { - type Varkeywords = Option<&'py PyDict>; + type Varkeywords = Option>; #[inline] fn handle_varkeyword( varkeywords: &mut Self::Varkeywords, - name: &'py PyAny, - value: &'py PyAny, + name: PyArg<'py>, + value: PyArg<'py>, _function_description: &FunctionDescription, ) -> PyResult<()> { varkeywords - .get_or_insert_with(|| PyDict::new_bound(name.py()).into_gil_ref()) + .get_or_insert_with(|| PyDict::new_bound(name.py())) .set_item(name, value) } } fn push_parameter_list(msg: &mut String, parameter_names: &[&str]) { + let len = parameter_names.len(); for (i, parameter) in parameter_names.iter().enumerate() { if i != 0 { - if parameter_names.len() > 2 { + if len > 2 { msg.push(','); } - if i == parameter_names.len() - 1 { + if i == len - 1 { msg.push_str(" and ") } else { msg.push(' ') @@ -778,7 +826,7 @@ mod tests { }; assert_eq!( err.to_string(), - "TypeError: 'int' object cannot be converted to 'PyString'" + "TypeError: example() got an unexpected keyword argument '1'" ); }) } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 0b73ce9b..70c95ca0 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -38,14 +38,15 @@ pub type ipowfunc = unsafe extern "C" fn( impl IPowModulo { #[cfg(Py_3_8)] #[inline] - pub fn to_borrowed_any(self, py: Python<'_>) -> &PyAny { - unsafe { py.from_borrowed_ptr::(self.0) } + pub fn as_ptr(self) -> *mut ffi::PyObject { + self.0 } #[cfg(not(Py_3_8))] #[inline] - pub fn to_borrowed_any(self, py: Python<'_>) -> &PyAny { - unsafe { py.from_borrowed_ptr::(ffi::Py_None()) } + pub fn as_ptr(self) -> *mut ffi::PyObject { + // Safety: returning a borrowed pointer to Python `None` singleton + unsafe { ffi::Py_None() } } } @@ -560,3 +561,11 @@ impl From> for Py { bound.0.clone().unbind() } } + +impl<'py, T> std::ops::Deref for BoundRef<'_, 'py, T> { + type Target = Bound<'py, T>; + #[inline] + fn deref(&self) -> &Self::Target { + self.0 + } +} diff --git a/src/types/dict.rs b/src/types/dict.rs index 2d215c1f..daae753c 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -524,6 +524,16 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { } } +impl<'a, 'py> Borrowed<'a, 'py, PyDict> { + /// Iterates over the contents of this dictionary without incrementing reference counts. + /// + /// # Safety + /// It must be known that this dictionary will not be modified during iteration. + pub(crate) unsafe fn iter_borrowed(self) -> BorrowedDictIter<'a, 'py> { + BorrowedDictIter::new(self) + } +} + fn dict_len(dict: &Bound<'_, PyDict>) -> Py_ssize_t { #[cfg(any(not(Py_3_8), PyPy, Py_LIMITED_API))] unsafe { @@ -662,6 +672,64 @@ impl<'py> IntoIterator for Bound<'py, PyDict> { } } +mod borrowed_iter { + use super::*; + + /// Variant of the above which is used to iterate the items of the dictionary + /// without incrementing reference counts. This is only safe if it's known + /// that the dictionary will not be modified during iteration. + pub struct BorrowedDictIter<'a, 'py> { + dict: Borrowed<'a, 'py, PyDict>, + ppos: ffi::Py_ssize_t, + len: ffi::Py_ssize_t, + } + + impl<'a, 'py> Iterator for BorrowedDictIter<'a, 'py> { + type Item = (Borrowed<'a, 'py, PyAny>, Borrowed<'a, 'py, PyAny>); + + #[inline] + fn next(&mut self) -> Option { + let mut key: *mut ffi::PyObject = std::ptr::null_mut(); + let mut value: *mut ffi::PyObject = std::ptr::null_mut(); + + // Safety: self.dict lives sufficiently long that the pointer is not dangling + if unsafe { ffi::PyDict_Next(self.dict.as_ptr(), &mut self.ppos, &mut key, &mut value) } + != 0 + { + let py = self.dict.py(); + self.len -= 1; + // Safety: + // - PyDict_Next returns borrowed values + // - we have already checked that `PyDict_Next` succeeded, so we can assume these to be non-null + Some(unsafe { (key.assume_borrowed(py), value.assume_borrowed(py)) }) + } else { + None + } + } + + #[inline] + fn size_hint(&self) -> (usize, Option) { + let len = self.len(); + (len, Some(len)) + } + } + + impl ExactSizeIterator for BorrowedDictIter<'_, '_> { + fn len(&self) -> usize { + self.len as usize + } + } + + impl<'a, 'py> BorrowedDictIter<'a, 'py> { + pub(super) fn new(dict: Borrowed<'a, 'py, PyDict>) -> Self { + let len = dict_len(&dict); + BorrowedDictIter { dict, ppos: 0, len } + } + } +} + +pub(crate) use borrowed_iter::BorrowedDictIter; + /// Conversion trait that allows a sequence of tuples to be converted into `PyDict` /// Primary use case for this trait is `call` and `call_method` methods as keywords argument. pub trait IntoPyDict: Sized { diff --git a/src/types/string.rs b/src/types/string.rs index 4a33236b..88ebb30b 100644 --- a/src/types/string.rs +++ b/src/types/string.rs @@ -354,7 +354,7 @@ impl<'py> PyStringMethods<'py> for Bound<'py, PyString> { impl<'a> Borrowed<'a, '_, PyString> { #[cfg(any(Py_3_10, not(Py_LIMITED_API)))] #[allow(clippy::wrong_self_convention)] - fn to_str(self) -> PyResult<&'a str> { + pub(crate) fn to_str(self) -> PyResult<&'a str> { // PyUnicode_AsUTF8AndSize only available on limited API starting with 3.10. let mut size: ffi::Py_ssize_t = 0; let data: *const u8 = diff --git a/src/types/tuple.rs b/src/types/tuple.rs index caad7d9c..51d33f93 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -411,7 +411,7 @@ impl<'py> PyTupleMethods<'py> for Bound<'py, PyTuple> { } fn iter_borrowed<'a>(&'a self) -> BorrowedTupleIterator<'a, 'py> { - BorrowedTupleIterator::new(self.as_borrowed()) + self.as_borrowed().iter_borrowed() } fn to_list(&self) -> Bound<'py, PyList> { @@ -433,6 +433,10 @@ impl<'a, 'py> Borrowed<'a, 'py, PyTuple> { unsafe fn get_borrowed_item_unchecked(self, index: usize) -> Borrowed<'a, 'py, PyAny> { ffi::PyTuple_GET_ITEM(self.as_ptr(), index as Py_ssize_t).assume_borrowed(self.py()) } + + pub(crate) fn iter_borrowed(self) -> BorrowedTupleIterator<'a, 'py> { + BorrowedTupleIterator::new(self) + } } /// Used by `PyTuple::iter()`.