From 36189d496eec6e8c861355b81068e5f8726347ec Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 29 Jan 2024 13:23:14 +0000 Subject: [PATCH] update `extract_argument` to use `Bound` APIs --- guide/src/class.md | 4 +- pyo3-macros-backend/src/method.rs | 2 +- pyo3-macros-backend/src/params.rs | 12 +- pyo3-macros-backend/src/pyclass.rs | 6 +- pyo3-macros-backend/src/pymethod.rs | 28 ++-- pyo3-macros-backend/src/quotes.rs | 3 +- pytests/src/pyfunctions.rs | 74 ++++---- src/err/mod.rs | 48 +++--- src/impl_/extract_argument.rs | 195 +++++++++++++++------- src/impl_/pymethods.rs | 12 +- src/instance.rs | 25 ++- src/types/dict.rs | 109 ++++++++++++ src/types/string.rs | 2 +- tests/ui/invalid_result_conversion.stderr | 2 +- 14 files changed, 362 insertions(+), 160 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index 37265aaa2bc..e47542f0c02 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -1223,7 +1223,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: pyo3::impl_::extract_argument::PyArg<'py>, holder: &'a mut Self::Holder) -> pyo3::PyResult { pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1233,7 +1233,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: pyo3::impl_::extract_argument::PyArg<'py>, holder: &'a mut Self::Holder) -> pyo3::PyResult { pyo3::impl_::extract_argument::extract_pyclass_ref_mut(obj, holder) } } diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 7050be23d5c..2bbc9bbc1d5 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -191,7 +191,7 @@ impl SelfType { }); error_mode.handle_error(quote_spanned! { *span => _pyo3::impl_::extract_argument::#method::<#cls>( - #py.from_borrowed_ptr::<_pyo3::PyAny>(#slf), + _pyo3::impl_::extract_argument::PyArg::from_ptr(#py, #slf), &mut #holder, ) }) diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 1ef31867406..0fed72c2b81 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_::extract_argument::PyArg::from_ptr(py, _args); + let _kwargs = _pyo3::impl_::extract_argument::PyArg::from_ptr_or_opt(py, _kwargs); }, arg_convert, )); @@ -132,7 +132,7 @@ pub fn impl_arg_params( keyword_only_parameters: &[#(#keyword_only_parameters),*], }; let mut #args_array = [::std::option::Option::None; #num_params]; - let (_args, _kwargs) = #extract_expression; + let (_args, _kwargs) = &#extract_expression; }, param_conversion, )) @@ -180,7 +180,8 @@ fn impl_arg_param( let holder = push_holder(); return Ok(quote_arg_span! { _pyo3::impl_::extract_argument::extract_argument( - _args, + #[allow(clippy::useless_conversion)] + ::std::convert::From::from(_args), &mut #holder, #name_str )? @@ -193,7 +194,8 @@ 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), + #[allow(clippy::useless_conversion, clippy::redundant_closure)] + _kwargs.as_ref().map(|kwargs| ::std::convert::From::from(kwargs)), &mut #holder, #name_str, || ::std::option::Option::None diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index e2f849492a0..3759c4a0f4d 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1369,7 +1369,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: _pyo3::impl_::extract_argument::PyArg<'py>, holder: &'a mut Self::Holder) -> _pyo3::PyResult { _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1381,7 +1381,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: _pyo3::impl_::extract_argument::PyArg<'py>, holder: &'a mut Self::Holder) -> _pyo3::PyResult { _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) } } @@ -1391,7 +1391,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: _pyo3::impl_::extract_argument::PyArg<'py>, 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 d45d2e12f26..293879da5f4 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -930,39 +930,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 +980,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 +989,7 @@ fn extract_object( }); extract_error_mode.handle_error(quote! { _pyo3::impl_::extract_argument::extract_argument( - #source, + _pyo3::impl_::extract_argument::PyArg::from_ptr(py, #source_ptr), &mut #holder, #name ) diff --git a/pyo3-macros-backend/src/quotes.rs b/pyo3-macros-backend/src/quotes.rs index 239036ef3ca..b7a96aa10be 100644 --- a/pyo3-macros-backend/src/quotes.rs +++ b/pyo3-macros-backend/src/quotes.rs @@ -16,6 +16,7 @@ pub(crate) fn ok_wrap(obj: TokenStream) -> TokenStream { pub(crate) fn map_result_into_ptr(result: TokenStream) -> TokenStream { quote! { - _pyo3::impl_::wrap::map_result_into_ptr(py, #result) + let result = _pyo3::impl_::wrap::map_result_into_ptr(py, #result); + result } } diff --git a/pytests/src/pyfunctions.rs b/pytests/src/pyfunctions.rs index 1eef970430e..a92733c2898 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/err/mod.rs b/src/err/mod.rs index 13e154b3860..a8e42c42ce1 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -7,7 +7,7 @@ use crate::{ exceptions::{self, PyBaseException}, ffi, }; -use crate::{IntoPy, Py, PyAny, PyNativeType, PyObject, Python, ToPyObject}; +use crate::{Borrowed, IntoPy, Py, PyAny, PyNativeType, PyObject, Python, ToPyObject}; use std::borrow::Cow; use std::cell::UnsafeCell; use std::ffi::CString; @@ -46,27 +46,29 @@ unsafe impl Sync for PyErr {} pub type PyResult = Result; /// Error that indicates a failure to convert a PyAny to a more specific Python type. -#[derive(Debug)] -pub struct PyDowncastError<'a> { - from: &'a PyAny, - to: Cow<'static, str>, -} +pub struct PyDowncastError<'py>(DowncastError<'py, 'py>); impl<'a> PyDowncastError<'a> { /// Create a new `PyDowncastError` representing a failure to convert the object /// `from` into the type named in `to`. pub fn new(from: &'a PyAny, to: impl Into>) -> Self { - PyDowncastError { - from, - to: to.into(), - } + PyDowncastError(DowncastError::new_from_borrowed(from.as_borrowed(), to)) + } +} + +impl std::fmt::Debug for PyDowncastError<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("PyDowncastError") + .field("from", &self.0.from) + .field("to", &self.0.to) + .finish() } } /// Error that indicates a failure to convert a PyAny to a more specific Python type. #[derive(Debug)] pub struct DowncastError<'a, 'py> { - from: &'a Bound<'py, PyAny>, + from: Borrowed<'a, 'py, PyAny>, to: Cow<'static, str>, } @@ -74,6 +76,17 @@ impl<'a, 'py> DowncastError<'a, 'py> { /// Create a new `PyDowncastError` representing a failure to convert the object /// `from` into the type named in `to`. pub fn new(from: &'a Bound<'py, PyAny>, to: impl Into>) -> Self { + DowncastError { + from: from.as_borrowed(), + to: to.into(), + } + } + + /// Similar to [`DowncastError::new`], but from a `Borrowed` instead of a `Bound`. + pub(crate) fn new_from_borrowed( + from: Borrowed<'a, 'py, PyAny>, + to: impl Into>, + ) -> Self { DowncastError { from, to: to.into(), @@ -829,12 +842,7 @@ impl PyErrArguments for PyDowncastErrorArguments { /// Convert `PyDowncastError` to Python `TypeError`. impl<'a> std::convert::From> for PyErr { fn from(err: PyDowncastError<'_>) -> PyErr { - let args = PyDowncastErrorArguments { - from: err.from.get_type().into(), - to: err.to, - }; - - exceptions::PyTypeError::new_err(args) + PyErr::from(err.0) } } @@ -842,7 +850,7 @@ impl<'a> std::error::Error for PyDowncastError<'a> {} impl<'a> std::fmt::Display for PyDowncastError<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - display_downcast_error(f, &self.from.as_borrowed(), &self.to) + self.0.fmt(f) } } @@ -882,13 +890,13 @@ impl std::error::Error for DowncastIntoError<'_> {} impl std::fmt::Display for DowncastIntoError<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - display_downcast_error(f, &self.from, &self.to) + display_downcast_error(f, self.from.as_borrowed(), &self.to) } } fn display_downcast_error( f: &mut std::fmt::Formatter<'_>, - from: &Bound<'_, PyAny>, + from: Borrowed<'_, '_, PyAny>, to: &str, ) -> std::fmt::Result { write!( diff --git a/src/impl_/extract_argument.rs b/src/impl_/extract_argument.rs index cd63fe270b0..da187da8ea7 100644 --- a/src/impl_/extract_argument.rs +++ b/src/impl_/extract_argument.rs @@ -1,10 +1,56 @@ use crate::{ exceptions::PyTypeError, ffi, + ffi_ptr_ext::FfiPtrExt, 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, PyString, PyTuple, + }, + Borrowed, Bound, FromPyObject, PyAny, PyClass, PyErr, PyRef, PyRefMut, PyResult, PyTypeCheck, + Python, }; +use crate::{PyObject, ToPyObject}; + +/// Function argument extraction borrows input arguments. +/// +/// This might potentially be removed in favour of just using `Borrowed` directly in future, +/// for now it's used just to control the traits and methods available while the `Bound` API +/// is being rounded out. +#[derive(Copy, Clone)] +#[repr(transparent)] +pub struct PyArg<'py>(Borrowed<'py, 'py, PyAny>); + +impl<'py> PyArg<'py> { + #[inline] + pub unsafe fn from_ptr_or_opt(py: Python<'py>, ptr: *mut ffi::PyObject) -> Option { + ptr.assume_borrowed_or_opt(py).map(Self) + } + + #[inline] + pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + Self(ptr.assume_borrowed(py)) + } +} + +// These two `From` implementations help make various macro code pieces compile +impl<'py, T> From<&'py Bound<'py, T>> for PyArg<'py> { + fn from(other: &'py Bound<'py, T>) -> Self { + Self(Borrowed::from(other.as_any())) + } +} + +impl<'py> From<&'_ Self> for PyArg<'py> { + fn from(other: &'_ Self) -> Self { + Self(other.0) + } +} + +impl ToPyObject for PyArg<'_> { + #[inline] + fn to_object(&self, py: Python<'_>) -> PyObject { + self.0.to_object(py) + } +} /// A trait which is used to help PyO3 macros extract function arguments. /// @@ -16,7 +62,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: PyArg<'py>, holder: &'a mut Self::Holder) -> PyResult; } impl<'a, 'py, T> PyFunctionArgument<'a, 'py> for T @@ -26,20 +72,20 @@ where type Holder = (); #[inline] - fn extract(obj: &'py PyAny, _: &'a mut ()) -> PyResult { - obj.extract() + fn extract(obj: PyArg<'py>, _: &'a mut ()) -> PyResult { + obj.0.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>; #[inline] - fn extract(obj: &'py PyAny, holder: &'a mut Option>) -> PyResult { - Ok(&*holder.insert(obj.extract()?)) + fn extract(obj: PyArg<'py>, holder: &'a mut Option>) -> PyResult { + Ok(&*holder.insert(obj.0.downcast()?)) } } @@ -59,24 +105,24 @@ impl FunctionArgumentHolder for Option { #[inline] pub fn extract_pyclass_ref<'a, 'py: 'a, T: PyClass>( - obj: &'py PyAny, + obj: PyArg<'py>, holder: &'a mut Option>, ) -> PyResult<&'a T> { - Ok(&*holder.insert(obj.extract()?)) + Ok(&*holder.insert(obj.0.extract()?)) } #[inline] pub fn extract_pyclass_ref_mut<'a, 'py: 'a, T: PyClass>( - obj: &'py PyAny, + obj: PyArg<'py>, holder: &'a mut Option>, ) -> PyResult<&'a mut T> { - Ok(&mut *holder.insert(obj.extract()?)) + Ok(&mut *holder.insert(obj.0.extract()?)) } /// 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: PyArg<'py>, holder: &'a mut T::Holder, arg_name: &str, ) -> PyResult @@ -85,7 +131,7 @@ where { match PyFunctionArgument::extract(obj, holder) { Ok(value) => Ok(value), - Err(e) => Err(argument_extraction_error(obj.py(), arg_name, e)), + Err(e) => Err(argument_extraction_error(obj.0.py(), arg_name, e)), } } @@ -93,7 +139,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>, holder: &'a mut T::Holder, arg_name: &str, default: fn() -> Option, @@ -103,7 +149,7 @@ where { match obj { Some(obj) => { - if obj.is_none() { + if obj.0.is_none() { // Explicit `None` will result in None being used as the function argument Ok(None) } else { @@ -117,7 +163,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>, holder: &'a mut T::Holder, arg_name: &str, default: fn() -> T, @@ -134,20 +180,20 @@ where /// Alternative to [`extract_argument`] used when the argument has a `#[pyo3(from_py_with)]` annotation. #[doc(hidden)] pub fn from_py_with<'py, T>( - obj: &'py PyAny, + obj: PyArg<'py>, arg_name: &str, extractor: fn(&'py PyAny) -> PyResult, ) -> PyResult { - match extractor(obj) { + match extractor(obj.0.into_gil_ref()) { Ok(value) => Ok(value), - Err(e) => Err(argument_extraction_error(obj.py(), arg_name, e)), + Err(e) => Err(argument_extraction_error(obj.0.py(), arg_name, e)), } } /// Alternative to [`extract_argument`] used when the argument has a `#[pyo3(from_py_with)]` annotation and also a default value. #[doc(hidden)] pub fn from_py_with_with_default<'py, T>( - obj: Option<&'py PyAny>, + obj: Option>, arg_name: &str, extractor: fn(&'py PyAny) -> PyResult, default: fn() -> T, @@ -182,7 +228,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)] @@ -229,7 +275,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>, @@ -246,8 +292,8 @@ 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` + 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); @@ -267,13 +313,21 @@ impl FunctionDescription { // Handle keyword arguments let mut varkeywords = K::Varkeywords::default(); - if let Some(kwnames) = py.from_borrowed_ptr_or_opt::(kwnames) { + + // Safety: kwnames is known to be a pointer to a tuple, or null + let kwnames: &'py Option> = std::mem::transmute(&kwnames); + if let Some(kwnames) = kwnames.as_ref() { // 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()); + 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() + .map(PyArg) + .zip(kwargs.iter().copied()), &mut varkeywords, num_positional_parameters, output, @@ -302,17 +356,20 @@ impl FunctionDescription { /// - `kwargs` must be a pointer to a PyDict, or NULL. pub unsafe fn extract_arguments_tuple_dict<'py, V, K>( &self, - py: Python<'py>, + _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: Bound has the same layout as a raw pointer, and reference is known to be + // borrowed for 'py. + let args: &'py Bound<'py, PyTuple> = std::mem::transmute(&args); + let kwargs: &'py Option> = std::mem::transmute(&kwargs); + let kwargs = kwargs.as_ref(); let num_positional_parameters = self.positional_parameter_names.len(); @@ -324,8 +381,12 @@ impl FunctionDescription { ); // Copy positional arguments into output - for (i, arg) in args.iter().take(num_positional_parameters).enumerate() { - output[i] = Some(arg); + for (i, arg) in args + .iter_borrowed() + .take(num_positional_parameters) + .enumerate() + { + output[i] = Some(PyArg(arg)); } // If any arguments remain, push them to varargs (if possible) or error @@ -334,7 +395,12 @@ impl FunctionDescription { // 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().map(|(k, v)| (PyArg(k), PyArg(v))), + &mut varkeywords, + num_positional_parameters, + output, + )? } // Once all inputs have been processed, check that all required arguments have been provided. @@ -351,11 +417,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, @@ -371,7 +437,7 @@ impl FunctionDescription { // 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() { + if let Ok(kwarg_name) = kwarg_name_py.0.downcast::()?.to_str() { // 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] @@ -429,7 +495,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 { @@ -445,7 +511,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) { @@ -490,11 +556,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.0.as_any() )) } @@ -527,7 +593,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 @@ -548,7 +614,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() @@ -568,14 +634,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; } @@ -589,7 +655,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(); @@ -603,7 +669,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(); @@ -620,19 +686,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(); @@ -645,8 +711,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<()>; } @@ -659,8 +725,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)) @@ -671,28 +737,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(name.py())) + .get_or_insert_with(|| PyDict::new_bound(name.0.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(' ') diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 4fac39fbdc4..9c0b4fe27f1 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -3,9 +3,7 @@ use crate::exceptions::PyStopAsyncIteration; use crate::gil::LockGIL; use crate::impl_::panic::PanicTrap; use crate::internal_tricks::extract_c_string; -use crate::{ - ffi, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult, PyTraverseError, PyVisit, Python, -}; +use crate::{ffi, PyCell, PyClass, PyErr, PyObject, PyResult, PyTraverseError, PyVisit, Python}; use std::borrow::Cow; use std::ffi::CStr; use std::fmt; @@ -34,14 +32,14 @@ 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 { + ffi::Py_None() } } diff --git a/src/instance.rs b/src/instance.rs index 492bef56b32..02fe4cd9905 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -6,8 +6,8 @@ use crate::types::any::PyAnyMethods; use crate::types::string::PyStringMethods; use crate::types::{PyDict, PyString, PyTuple}; use crate::{ - ffi, AsPyPointer, FromPyObject, IntoPy, PyAny, PyClass, PyClassInitializer, PyRef, PyRefMut, - PyTypeInfo, Python, ToPyObject, + ffi, AsPyPointer, DowncastError, FromPyObject, IntoPy, PyAny, PyClass, PyClassInitializer, + PyRef, PyRefMut, PyTypeInfo, Python, ToPyObject, }; use crate::{gil, PyTypeCheck}; use std::marker::PhantomData; @@ -321,6 +321,27 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { pub(crate) unsafe fn from_ptr_unchecked(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { Self(NonNull::new_unchecked(ptr), PhantomData, py) } + + #[inline] + pub(crate) fn downcast(self) -> Result, DowncastError<'a, 'py>> + where + T: PyTypeCheck, + { + if T::type_check(self.as_gil_ref()) { + // Safety: type_check is responsible for ensuring that the type is correct + Ok(unsafe { self.downcast_unchecked() }) + } else { + Err(DowncastError::new_from_borrowed(self, T::NAME)) + } + } + + /// Converts this `PyAny` to a concrete Python type without checking validity. + /// + /// # Safety + /// Callers must ensure that the type is valid or risk type confusion. + pub(crate) unsafe fn downcast_unchecked(self) -> Borrowed<'a, 'py, T> { + Borrowed(self.0, PhantomData, self.2) + } } impl<'a, 'py, T> From<&'a Bound<'py, T>> for Borrowed<'a, 'py, T> { diff --git a/src/types/dict.rs b/src/types/dict.rs index 3a54e742ed5..aeca9105d6c 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -510,6 +510,17 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { } } +impl<'py> Bound<'py, PyDict> { + /// Iterates over the contents of this dictionary without incrementing reference counts. + ///`` + /// # Safety + /// The corresponding key in the dictionary cannot be mutated for the duration + /// of the borrow. + pub(crate) unsafe fn iter_borrowed<'a>(&'a 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 { @@ -648,6 +659,104 @@ 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: &'a Bound<'py, PyDict>, + ppos: ffi::Py_ssize_t, + di_used: 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 ma_used = dict_len(self.dict); + + // These checks are similar to what CPython does. + // + // If the dimension of the dict changes e.g. key-value pairs are removed + // or added during iteration, this will panic next time when `next` is called + if self.di_used != ma_used { + self.di_used = -1; + panic!("dictionary changed size during iteration"); + }; + + // If the dict is changed in such a way that the length remains constant + // then this will panic at the end of iteration - similar to this: + // + // d = {"a":1, "b":2, "c": 3} + // + // for k, v in d.items(): + // d[f"{k}_"] = 4 + // del d[k] + // print(k) + // + if self.len == -1 { + self.di_used = -1; + panic!("dictionary keys changed during iteration"); + }; + + let ret = unsafe { self.next_unchecked() }; + if ret.is_some() { + self.len -= 1 + } + ret + } + + #[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: &'a Bound<'py, PyDict>) -> Self { + let len = dict_len(dict); + BorrowedDictIter { + dict, + ppos: 0, + di_used: len, + len, + } + } + + /// Advances the iterator without checking for concurrent modification. + /// + /// See [`PyDict_Next`](https://docs.python.org/3/c-api/dict.html#c.PyDict_Next) + /// for more information. + unsafe fn next_unchecked( + &mut self, + ) -> Option<(Borrowed<'a, 'py, PyAny>, Borrowed<'a, 'py, PyAny>)> { + let mut key: *mut ffi::PyObject = std::ptr::null_mut(); + let mut value: *mut ffi::PyObject = std::ptr::null_mut(); + + if ffi::PyDict_Next(self.dict.as_ptr(), &mut self.ppos, &mut key, &mut value) != 0 { + let py = self.dict.py(); + // PyDict_Next returns borrowed values; for safety must make them owned (see #890) + Some((key.assume_borrowed(py), value.assume_borrowed(py))) + } else { + None + } + } + } +} + +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 { diff --git a/src/types/string.rs b/src/types/string.rs index b1457950bb8..82490867a1f 100644 --- a/src/types/string.rs +++ b/src/types/string.rs @@ -346,7 +346,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/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index b3e65517e36..d2417a86336 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -8,7 +8,7 @@ error[E0277]: the trait bound `PyErr: From` is not satisfied > > > - >> + >> >> >> >