From 1aaede53b5201fcea5152a1f5a2757b2ccaac863 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 5 Feb 2024 13:02:29 +0000 Subject: [PATCH 1/8] add `PyBackedStr` and `PyBackedBytes` --- src/lib.rs | 1 + src/py_backed.rs | 121 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 src/py_backed.rs diff --git a/src/lib.rs b/src/lib.rs index 5ba0d279aa1..b8589940018 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -426,6 +426,7 @@ pub mod marshal; pub mod sync; pub mod panic; pub mod prelude; +pub mod py_backed; pub mod pycell; pub mod pyclass; pub mod pyclass_init; diff --git a/src/py_backed.rs b/src/py_backed.rs new file mode 100644 index 00000000000..9eff4c6b2a8 --- /dev/null +++ b/src/py_backed.rs @@ -0,0 +1,121 @@ +//! This module provides some wrappers around `str` and `[u8]` where the storage is owned by a Python `str` or `bytes` object. +//! +//! This can help avoid copying text or byte data sourced from Python. + +use std::ops::Deref; + +use crate::{ + types::{ + any::PyAnyMethods, bytearray::PyByteArrayMethods, bytes::PyBytesMethods, + string::PyStringMethods, PyByteArray, PyBytes, PyString, + }, + Bound, DowncastError, FromPyObject, Py, PyAny, PyResult, +}; + +/// A wrapper around `str` where the storage is owned by a Python `bytes` or `str` object. +/// +/// This type gives access to the underlying data via a `Deref` implementation. +pub struct PyBackedStr { + #[allow(dead_code)] + storage: PyBackedStrStorage, + data: *const u8, + length: usize, +} + +#[allow(dead_code)] +enum PyBackedStrStorage { + String(Py), + Bytes(Py), +} + +impl Deref for PyBackedStr { + type Target = str; + fn deref(&self) -> &str { + unsafe { + // Safety: `data` is a pointer to the start of a valid UTF-8 string of length `length`. + std::str::from_utf8_unchecked(std::slice::from_raw_parts(self.data, self.length)) + } + } +} + +impl FromPyObject<'_> for PyBackedStr { + fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult { + let py_string = obj.downcast::()?; + #[cfg(any(Py_3_10, not(Py_LIMITED_API)))] + { + let s = py_string.to_str()?; + let data = s.as_ptr(); + let length = s.len(); + Ok(Self { + storage: PyBackedStrStorage::String(py_string.to_owned().unbind()), + data, + length, + }) + } + #[cfg(not(any(Py_3_10, not(Py_LIMITED_API))))] + { + let bytes = py_string.encode_utf8()?; + let b = bytes.as_bytes(); + let data = b.as_ptr(); + let length = b.len(); + Ok(Self { + storage: PyBackedStrStorage::Bytes(bytes.unbind()), + data, + length, + }) + } + } +} + +/// A wrapper around `[u8]` where the storage is either owned by a Python `bytes` object, or a Rust `Vec`. +/// +/// This type gives access to the underlying data via a `Deref` implementation. +pub struct PyBackedBytes { + #[allow(dead_code)] // only held so that the storage is not dropped + storage: PyBackedBytesStorage, + data: *const u8, + length: usize, +} + +enum PyBackedBytesStorage { + Python(Py), + Rust(Vec), +} + +impl Deref for PyBackedBytes { + type Target = [u8]; + fn deref(&self) -> &[u8] { + unsafe { + // Safety: `data` is a pointer to the start of a buffer of length `length`. + std::slice::from_raw_parts(self.data, self.length) + } + } +} + +impl FromPyObject<'_> for PyBackedBytes { + fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult { + if let Ok(bytes) = obj.downcast::() { + let b = bytes.as_bytes(); + let data = b.as_ptr(); + let len = b.len(); + return Ok(Self { + storage: PyBackedBytesStorage::Python(bytes.to_owned().unbind()), + data, + length: len, + }); + } + + if let Ok(bytearray) = obj.downcast::() { + let s = bytearray.to_vec(); + let data = s.as_ptr(); + let len = s.len(); + return Ok(Self { + storage: PyBackedBytesStorage::Rust(s), + data, + length: len, + }); + } + + return Err(DowncastError::new(obj, "`bytes` or `bytearray`").into()); + } +} From dcd20f41a92ae53e08ab0f5863cda66926b2d820 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 29 Jan 2024 13:23:14 +0000 Subject: [PATCH 2/8] update `extract_argument` to use `Bound` APIs --- guide/src/class.md | 4 +- pyo3-benches/benches/bench_comparisons.rs | 8 +- 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 | 225 +++++++++++++++------- src/impl_/pymethods.rs | 13 +- src/instance.rs | 17 +- src/types/dict.rs | 109 +++++++++++ src/types/string.rs | 2 +- tests/ui/invalid_result_conversion.stderr | 2 +- 15 files changed, 382 insertions(+), 171 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index dfe5c8ae4ae..2e4d43cced3 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -1253,7 +1253,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) } } @@ -1263,7 +1263,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-benches/benches/bench_comparisons.rs b/pyo3-benches/benches/bench_comparisons.rs index ffd4c1a452f..fbd473f06cf 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 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 02024011366..ff62683a8e1 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 31385f4c24c..18a70fe5e7c 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(), @@ -864,12 +877,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) } } @@ -877,7 +885,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) } } @@ -917,13 +925,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 4df674e7639..d9b12be29b0 100644 --- a/src/impl_/extract_argument.rs +++ b/src/impl_/extract_argument.rs @@ -1,10 +1,54 @@ 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, 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 +60,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 +70,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 +103,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 +129,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 +137,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 +147,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 +161,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 +178,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, @@ -165,7 +209,6 @@ pub fn from_py_with_with_default<'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::()) { let remapped_error = PyTypeError::new_err(format!("argument '{}': {}", arg_name, error.value(py))); @@ -183,7 +226,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)] @@ -230,7 +273,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>, @@ -247,8 +290,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); @@ -268,13 +311,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, @@ -303,17 +354,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(); @@ -325,8 +379,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 @@ -335,7 +393,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. @@ -352,11 +415,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, @@ -368,11 +431,25 @@ 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 + .0 + .downcast_unchecked::() + } + .to_str(); + + #[cfg(not(any(Py_3_10, not(Py_LIMITED_API))))] + let kwarg_name = kwarg_name_py.0.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(not(any(Py_3_10, not(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] @@ -391,7 +468,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)); @@ -404,6 +481,11 @@ impl FunctionDescription { } if !positional_only_keyword_arguments.is_empty() { + #[cfg(not(any(Py_3_10, not(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)); } @@ -430,7 +512,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 { @@ -446,7 +528,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) { @@ -491,11 +573,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() )) } @@ -528,7 +610,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 @@ -549,7 +631,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() @@ -569,14 +651,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; } @@ -590,7 +672,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(); @@ -604,7 +686,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(); @@ -621,19 +703,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(); @@ -646,8 +728,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<()>; } @@ -660,8 +742,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)) @@ -672,28 +754,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.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(' ') @@ -772,7 +855,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 4fac39fbdc4..37e05ddaccc 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,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() } } } diff --git a/src/instance.rs b/src/instance.rs index 7f92744ccc4..4b20c66e3ab 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -7,8 +7,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; @@ -504,6 +504,19 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { 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) { + // 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 diff --git a/src/types/dict.rs b/src/types/dict.rs index 741d974e264..20b569a0de6 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -517,6 +517,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 { @@ -655,6 +666,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: Sized { diff --git a/src/types/string.rs b/src/types/string.rs index 2b2635715d3..4d5651df949 100644 --- a/src/types/string.rs +++ b/src/types/string.rs @@ -357,7 +357,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 > > > - >> + >> >> >> > From a676f6ae89a17f61ce18a4f4eea97af9887cc7cf Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 Nov 2023 15:58:57 +0000 Subject: [PATCH 3/8] import_bound --- src/marker.rs | 8 ++++++ src/types/module.rs | 62 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/src/marker.rs b/src/marker.rs index 642ca783904..2ebc4d65941 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -744,6 +744,14 @@ impl<'py> Python<'py> { PyModule::import(self, name) } + /// Imports the Python module with the specified name. + pub fn import_bound(self, name: N) -> PyResult> + where + N: IntoPy>, + { + PyModule::import_bound(self, name) + } + /// Gets the Python builtin value `None`. #[allow(non_snake_case)] // the Python keyword starts with uppercase #[inline] diff --git a/src/types/module.rs b/src/types/module.rs index 8824dfcf030..a7f19c02c12 100644 --- a/src/types/module.rs +++ b/src/types/module.rs @@ -1,6 +1,7 @@ use crate::callback::IntoPyCallbackOutput; use crate::err::{PyErr, PyResult}; use crate::ffi_ptr_ext::FfiPtrExt; +use crate::py_result_ext::PyResultExt; use crate::pyclass::PyClass; use crate::types::{ any::PyAnyMethods, list::PyListMethods, PyAny, PyCFunction, PyDict, PyList, PyString, @@ -39,9 +40,33 @@ impl PyModule { /// # Ok(())} /// ``` pub fn new<'p>(py: Python<'p>, name: &str) -> PyResult<&'p PyModule> { + Self::new_bound(py, name).map(Bound::into_gil_ref) + } + + /// Creates a new module object with the `__name__` attribute set to `name`. + /// + /// # Examples + /// + /// ``` rust + /// use pyo3::prelude::*; + /// + /// # fn main() -> PyResult<()> { + /// Python::with_gil(|py| -> PyResult<()> { + /// let module = PyModule::new(py, "my_module")?; + /// + /// assert_eq!(module.name()?, "my_module"); + /// Ok(()) + /// })?; + /// # Ok(())} + /// ``` + pub fn new_bound<'py>(py: Python<'py>, name: &str) -> PyResult> { // Could use PyModule_NewObject, but it doesn't exist on PyPy. let name = CString::new(name)?; - unsafe { py.from_owned_ptr_or_err(ffi::PyModule_New(name.as_ptr())) } + unsafe { + ffi::PyModule_New(name.as_ptr()) + .assume_owned_or_err(py) + .downcast_into_unchecked() + } } /// Imports the Python module with the specified name. @@ -63,11 +88,40 @@ impl PyModule { /// import antigravity /// ``` pub fn import(py: Python<'_>, name: N) -> PyResult<&PyModule> + where + N: IntoPy>, + { + Self::import_bound(py, name).map(Bound::into_gil_ref) + } + + /// Imports the Python module with the specified name. + /// + /// # Examples + /// + /// ```no_run + /// # fn main() { + /// use pyo3::prelude::*; + /// + /// Python::with_gil(|py| { + /// let module = PyModule::import(py, "antigravity").expect("No flying for you."); + /// }); + /// # } + /// ``` + /// + /// This is equivalent to the following Python expression: + /// ```python + /// import antigravity + /// ``` + pub fn import_bound(py: Python<'_>, name: N) -> PyResult> where N: IntoPy>, { let name: Py = name.into_py(py); - unsafe { py.from_owned_ptr_or_err(ffi::PyImport_Import(name.as_ptr())) } + unsafe { + ffi::PyImport_Import(name.as_ptr()) + .assume_owned_or_err(py) + .downcast_into_unchecked() + } } /// Creates and loads a module named `module_name`, @@ -573,8 +627,6 @@ impl<'py> PyModuleMethods<'py> for Bound<'py, PyModule> { fn name(&self) -> PyResult> { #[cfg(not(PyPy))] { - use crate::py_result_ext::PyResultExt; - unsafe { ffi::PyModule_GetNameObject(self.as_ptr()) .assume_owned_or_err(self.py()) @@ -594,8 +646,6 @@ impl<'py> PyModuleMethods<'py> for Bound<'py, PyModule> { #[cfg(not(PyPy))] fn filename(&self) -> PyResult> { - use crate::py_result_ext::PyResultExt; - unsafe { ffi::PyModule_GetFilenameObject(self.as_ptr()) .assume_owned_or_err(self.py()) From 14cec413c59fef7a6490ced1c296e33ecb31917e Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 Nov 2023 16:19:56 +0000 Subject: [PATCH 4/8] implement `PyTypeMethods` --- src/prelude.rs | 1 + src/types/mod.rs | 2 +- src/types/typeobject.rs | 127 +++++++++++++++++++++++++++++++--------- 3 files changed, 100 insertions(+), 30 deletions(-) diff --git a/src/prelude.rs b/src/prelude.rs index 47951aedcce..3ac239f49e1 100644 --- a/src/prelude.rs +++ b/src/prelude.rs @@ -41,3 +41,4 @@ pub use crate::types::set::PySetMethods; pub use crate::types::string::PyStringMethods; pub use crate::types::traceback::PyTracebackMethods; pub use crate::types::tuple::PyTupleMethods; +pub use crate::types::typeobject::PyTypeMethods; diff --git a/src/types/mod.rs b/src/types/mod.rs index da22b30729f..46909c880bb 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -309,4 +309,4 @@ mod slice; pub(crate) mod string; pub(crate) mod traceback; pub(crate) mod tuple; -mod typeobject; +pub(crate) mod typeobject; diff --git a/src/types/typeobject.rs b/src/types/typeobject.rs index 50eeaa7153d..7befd766a13 100644 --- a/src/types/typeobject.rs +++ b/src/types/typeobject.rs @@ -1,5 +1,8 @@ use crate::err::{self, PyResult}; -use crate::{ffi, PyAny, PyTypeInfo, Python}; +use crate::ffi_ptr_ext::FfiPtrExt; +use crate::instance::Borrowed; +use crate::types::any::PyAnyMethods; +use crate::{ffi, Bound, PyAny, PyNativeType, PyTypeInfo, Python}; use std::borrow::Cow; #[cfg(not(any(Py_LIMITED_API, PyPy)))] use std::ffi::CStr; @@ -20,7 +23,7 @@ impl PyType { /// Retrieves the underlying FFI pointer associated with this Python object. #[inline] pub fn as_type_ptr(&self) -> *mut ffi::PyTypeObject { - self.as_ptr() as *mut ffi::PyTypeObject + self.as_borrowed().as_type_ptr() } /// Retrieves the `PyType` instance for the given FFI pointer. @@ -35,14 +38,81 @@ impl PyType { /// Gets the [qualified name](https://docs.python.org/3/glossary.html#term-qualified-name) of the `PyType`. pub fn qualname(&self) -> PyResult { + self.as_borrowed().qualname() + } + + /// Gets the full name, which includes the module, of the `PyType`. + pub fn name(&self) -> PyResult> { + self.as_borrowed().name() + } + + /// Checks whether `self` is a subclass of `other`. + /// + /// Equivalent to the Python expression `issubclass(self, other)`. + pub fn is_subclass(&self, other: &PyAny) -> PyResult { + self.as_borrowed().is_subclass(&other.as_borrowed()) + } + + /// Checks whether `self` is a subclass of type `T`. + /// + /// Equivalent to the Python expression `issubclass(self, T)`, if the type + /// `T` is known at compile time. + pub fn is_subclass_of(&self) -> PyResult + where + T: PyTypeInfo, + { + self.as_borrowed().is_subclass_of::() + } +} + +/// Implementation of functionality for [`PyType`]. +/// +/// These methods are defined for the `Bound<'py, PyType>` smart pointer, so to use method call +/// syntax these methods are separated into a trait, because stable Rust does not yet support +/// `arbitrary_self_types`. +#[doc(alias = "PyType")] +pub trait PyTypeMethods<'py> { + /// Retrieves the underlying FFI pointer associated with this Python object. + fn as_type_ptr(&self) -> *mut ffi::PyTypeObject; + + /// Gets the full name, which includes the module, of the `PyType`. + fn name(&self) -> PyResult>; + + /// Gets the [qualified name](https://docs.python.org/3/glossary.html#term-qualified-name) of the `PyType`. + fn qualname(&self) -> PyResult; + + /// Checks whether `self` is a subclass of `other`. + /// + /// Equivalent to the Python expression `issubclass(self, other)`. + fn is_subclass(&self, other: &Bound<'_, PyAny>) -> PyResult; + + /// Checks whether `self` is a subclass of type `T`. + /// + /// Equivalent to the Python expression `issubclass(self, T)`, if the type + /// `T` is known at compile time. + fn is_subclass_of(&self) -> PyResult + where + T: PyTypeInfo; +} + +impl<'py> PyTypeMethods<'py> for Bound<'py, PyType> { + /// Retrieves the underlying FFI pointer associated with this Python object. + #[inline] + fn as_type_ptr(&self) -> *mut ffi::PyTypeObject { + self.as_ptr() as *mut ffi::PyTypeObject + } + + /// Gets the name of the `PyType`. + fn name(&self) -> PyResult> { + Borrowed::from(self).name() + } + + fn qualname(&self) -> PyResult { #[cfg(any(Py_LIMITED_API, PyPy, not(Py_3_11)))] let name = self.getattr(intern!(self.py(), "__qualname__"))?.extract(); #[cfg(not(any(Py_LIMITED_API, PyPy, not(Py_3_11))))] let name = { - use crate::ffi_ptr_ext::FfiPtrExt; - use crate::types::any::PyAnyMethods; - let obj = unsafe { ffi::PyType_GetQualName(self.as_type_ptr()).assume_owned_or_err(self.py())? }; @@ -53,8 +123,29 @@ impl PyType { name } - /// Gets the full name, which includes the module, of the `PyType`. - pub fn name(&self) -> PyResult> { + /// Checks whether `self` is a subclass of `other`. + /// + /// Equivalent to the Python expression `issubclass(self, other)`. + fn is_subclass(&self, other: &Bound<'_, PyAny>) -> PyResult { + let result = unsafe { ffi::PyObject_IsSubclass(self.as_ptr(), other.as_ptr()) }; + err::error_on_minusone(self.py(), result)?; + Ok(result == 1) + } + + /// Checks whether `self` is a subclass of type `T`. + /// + /// Equivalent to the Python expression `issubclass(self, T)`, if the type + /// `T` is known at compile time. + fn is_subclass_of(&self) -> PyResult + where + T: PyTypeInfo, + { + self.is_subclass(&T::type_object_bound(self.py())) + } +} + +impl<'a> Borrowed<'a, '_, PyType> { + fn name(self) -> PyResult> { #[cfg(not(any(Py_LIMITED_API, PyPy)))] { let ptr = self.as_type_ptr(); @@ -78,34 +169,12 @@ impl PyType { #[cfg(Py_3_11)] let name = { - use crate::ffi_ptr_ext::FfiPtrExt; - unsafe { ffi::PyType_GetName(self.as_type_ptr()).assume_owned_or_err(self.py())? } }; Ok(Cow::Owned(format!("{}.{}", module, name))) } } - - /// Checks whether `self` is a subclass of `other`. - /// - /// Equivalent to the Python expression `issubclass(self, other)`. - pub fn is_subclass(&self, other: &PyAny) -> PyResult { - let result = unsafe { ffi::PyObject_IsSubclass(self.as_ptr(), other.as_ptr()) }; - err::error_on_minusone(self.py(), result)?; - Ok(result == 1) - } - - /// Checks whether `self` is a subclass of type `T`. - /// - /// Equivalent to the Python expression `issubclass(self, T)`, if the type - /// `T` is known at compile time. - pub fn is_subclass_of(&self) -> PyResult - where - T: PyTypeInfo, - { - self.is_subclass(T::type_object_bound(self.py()).as_gil_ref()) - } } #[cfg(test)] From e705f0a58048a736d710a6097ef0932a0406129d Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 28 Nov 2023 16:19:50 +0000 Subject: [PATCH 5/8] correct pytype in pyanymethods --- src/err/mod.rs | 2 +- src/instance.rs | 3 +-- src/types/any.rs | 10 +++++----- src/types/typeobject.rs | 15 +++++++++++++++ 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/err/mod.rs b/src/err/mod.rs index 18a70fe5e7c..644645875b0 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -2,7 +2,7 @@ use crate::instance::Bound; use crate::panic::PanicException; use crate::type_object::PyTypeInfo; use crate::types::any::PyAnyMethods; -use crate::types::{PyTraceback, PyType}; +use crate::types::{typeobject::PyTypeMethods, PyTraceback, PyType}; use crate::{ exceptions::{self, PyBaseException}, ffi, diff --git a/src/instance.rs b/src/instance.rs index 4b20c66e3ab..cee5bc33ee6 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -3,8 +3,7 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::pycell::{PyBorrowError, PyBorrowMutError, PyCell}; use crate::pyclass::boolean_struct::{False, True}; use crate::type_object::HasPyGilRef; -use crate::types::any::PyAnyMethods; -use crate::types::string::PyStringMethods; +use crate::types::{any::PyAnyMethods, string::PyStringMethods, typeobject::PyTypeMethods}; use crate::types::{PyDict, PyString, PyTuple}; use crate::{ ffi, AsPyPointer, DowncastError, FromPyObject, IntoPy, PyAny, PyClass, PyClassInitializer, diff --git a/src/types/any.rs b/src/types/any.rs index 90c7f2160d3..6420c1587d9 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -667,7 +667,7 @@ impl PyAny { /// Returns the Python type object for this object's type. pub fn get_type(&self) -> &PyType { - self.as_borrowed().get_type() + self.as_borrowed().get_type().into_gil_ref() } /// Returns the Python type pointer for this object. @@ -1499,7 +1499,7 @@ pub trait PyAnyMethods<'py> { fn iter(&self) -> PyResult>; /// Returns the Python type object for this object's type. - fn get_type(&self) -> &'py PyType; + fn get_type(&self) -> Bound<'py, PyType>; /// Returns the Python type pointer for this object. fn get_type_ptr(&self) -> *mut ffi::PyTypeObject; @@ -2107,8 +2107,8 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> { PyIterator::from_bound_object(self) } - fn get_type(&self) -> &'py PyType { - unsafe { PyType::from_type_ptr(self.py(), ffi::Py_TYPE(self.as_ptr())) } + fn get_type(&self) -> Bound<'py, PyType> { + unsafe { PyType::from_type_ptr_bound(self.py(), ffi::Py_TYPE(self.as_ptr())) } } #[inline] @@ -2265,7 +2265,7 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> { #[cfg(not(PyPy))] fn py_super(&self) -> PyResult> { - PySuper::new_bound(&self.get_type().as_borrowed(), self) + PySuper::new_bound(&self.get_type(), self) } } diff --git a/src/types/typeobject.rs b/src/types/typeobject.rs index 7befd766a13..c6ccef6282f 100644 --- a/src/types/typeobject.rs +++ b/src/types/typeobject.rs @@ -36,6 +36,21 @@ impl PyType { py.from_borrowed_ptr(p as *mut ffi::PyObject) } + /// Retrieves the `PyType` instance for the given FFI pointer. + /// + /// # Safety + /// - The pointer must be non-null. + #[inline] + pub unsafe fn from_type_ptr_bound( + py: Python<'_>, + p: *mut ffi::PyTypeObject, + ) -> Bound<'_, PyType> { + p.cast::() + .assume_borrowed(py) + .downcast_unchecked() + .clone() + } + /// Gets the [qualified name](https://docs.python.org/3/glossary.html#term-qualified-name) of the `PyType`. pub fn qualname(&self) -> PyResult { self.as_borrowed().qualname() From 16225f89d2c3a23dc7d2835463e1e5c07174df4d Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 29 Nov 2023 10:34:18 +0000 Subject: [PATCH 6/8] PyRef own a reference for now --- src/pycell.rs | 74 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/src/pycell.rs b/src/pycell.rs index 3744c55f67d..c2fafecf477 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -192,15 +192,18 @@ //! [Interior Mutability]: https://doc.rust-lang.org/book/ch15-05-interior-mutability.html "RefCell and the Interior Mutability Pattern - The Rust Programming Language" use crate::exceptions::PyRuntimeError; +use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::{ PyClassBaseType, PyClassDict, PyClassImpl, PyClassThreadChecker, PyClassWeakRef, }; +use crate::instance::Bound; use crate::pyclass::{ boolean_struct::{False, True}, PyClass, }; use crate::pyclass_init::PyClassInitializer; use crate::type_object::{PyLayout, PySizedLayout}; +use crate::types::any::PyAnyMethods; use crate::types::PyAny; use crate::{ conversion::{AsPyPointer, FromPyPointer, ToPyObject}, @@ -346,17 +349,17 @@ impl PyCell { /// ``` pub fn try_borrow(&self) -> Result, PyBorrowError> { self.ensure_threadsafe(); - self.borrow_checker() - .try_borrow() - .map(|_| PyRef { inner: self }) + self.borrow_checker().try_borrow().map(|_| PyRef { + inner: self.as_borrowed().to_owned(), + }) } /// Variant of [`try_borrow`][Self::try_borrow] which fails instead of panicking if called from the wrong thread pub(crate) fn try_borrow_threadsafe(&self) -> Result, PyBorrowError> { self.check_threadsafe()?; - self.borrow_checker() - .try_borrow() - .map(|_| PyRef { inner: self }) + self.borrow_checker().try_borrow().map(|_| PyRef { + inner: self.as_borrowed().to_owned(), + }) } /// Mutably borrows the value `T`, returning an error if the value is currently borrowed. @@ -385,9 +388,9 @@ impl PyCell { T: PyClass, { self.ensure_threadsafe(); - self.borrow_checker() - .try_borrow_mut() - .map(|_| PyRefMut { inner: self }) + self.borrow_checker().try_borrow_mut().map(|_| PyRefMut { + inner: self.as_borrowed().to_owned(), + }) } /// Immutably borrows the value `T`, returning an error if the value is @@ -641,13 +644,13 @@ impl fmt::Debug for PyCell { /// } /// # Python::with_gil(|py| { /// # let sub = PyCell::new(py, Child::new()).unwrap(); -/// # pyo3::py_run!(py, sub, "assert sub.format() == 'Caterpillar(base: Butterfly, cnt: 3)'"); +/// # pyo3::py_run!(py, sub, "assert sub.format() == 'Caterpillar(base: Butterfly, cnt: 4)', sub.format()"); /// # }); /// ``` /// /// See the [module-level documentation](self) for more information. pub struct PyRef<'p, T: PyClass> { - inner: &'p PyCell, + inner: Bound<'p, T>, } impl<'p, T: PyClass> PyRef<'p, T> { @@ -663,7 +666,7 @@ where U: PyClass, { fn as_ref(&self) -> &T::BaseType { - unsafe { &*self.inner.ob_base.get_ptr() } + unsafe { &*self.inner.as_gil_ref().ob_base.get_ptr() } } } @@ -689,7 +692,7 @@ impl<'p, T: PyClass> PyRef<'p, T> { /// of the pointer or decrease the reference count (e.g. with [`pyo3::ffi::Py_DecRef`](crate::ffi::Py_DecRef)). #[inline] pub fn into_ptr(self) -> *mut ffi::PyObject { - self.inner.into_ptr() + self.inner.clone().into_ptr() } } @@ -744,10 +747,14 @@ where /// # }); /// ``` pub fn into_super(self) -> PyRef<'p, U> { - let PyRef { inner } = self; - std::mem::forget(self); + let py = self.py(); PyRef { - inner: &inner.ob_base, + inner: unsafe { + ManuallyDrop::new(self) + .as_ptr() + .assume_owned(py) + .downcast_into_unchecked() + }, } } } @@ -757,13 +764,13 @@ impl<'p, T: PyClass> Deref for PyRef<'p, T> { #[inline] fn deref(&self) -> &T { - unsafe { &*self.inner.get_ptr() } + unsafe { &*self.inner.as_gil_ref().get_ptr() } } } impl<'p, T: PyClass> Drop for PyRef<'p, T> { fn drop(&mut self) { - self.inner.borrow_checker().release_borrow() + self.inner.as_gil_ref().borrow_checker().release_borrow() } } @@ -775,7 +782,7 @@ impl IntoPy for PyRef<'_, T> { impl IntoPy for &'_ PyRef<'_, T> { fn into_py(self, py: Python<'_>) -> PyObject { - self.inner.into_py(py) + unsafe { PyObject::from_borrowed_ptr(py, self.inner.as_ptr()) } } } @@ -802,7 +809,7 @@ impl fmt::Debug for PyRef<'_, T> { /// /// See the [module-level documentation](self) for more information. pub struct PyRefMut<'p, T: PyClass> { - inner: &'p PyCell, + inner: Bound<'p, T>, } impl<'p, T: PyClass> PyRefMut<'p, T> { @@ -818,7 +825,7 @@ where U: PyClass, { fn as_ref(&self) -> &T::BaseType { - unsafe { &*self.inner.ob_base.get_ptr() } + unsafe { &*self.inner.as_gil_ref().ob_base.get_ptr() } } } @@ -828,7 +835,7 @@ where U: PyClass, { fn as_mut(&mut self) -> &mut T::BaseType { - unsafe { &mut *self.inner.ob_base.get_ptr() } + unsafe { &mut *self.inner.as_gil_ref().ob_base.get_ptr() } } } @@ -854,7 +861,7 @@ impl<'p, T: PyClass> PyRefMut<'p, T> { /// of the pointer or decrease the reference count (e.g. with [`pyo3::ffi::Py_DecRef`](crate::ffi::Py_DecRef)). #[inline] pub fn into_ptr(self) -> *mut ffi::PyObject { - self.inner.into_ptr() + self.inner.clone().into_ptr() } } @@ -867,10 +874,14 @@ where /// /// See [`PyRef::into_super`] for more. pub fn into_super(self) -> PyRefMut<'p, U> { - let PyRefMut { inner } = self; - std::mem::forget(self); + let py = self.py(); PyRefMut { - inner: &inner.ob_base, + inner: unsafe { + ManuallyDrop::new(self) + .as_ptr() + .assume_owned(py) + .downcast_into_unchecked() + }, } } } @@ -880,20 +891,23 @@ impl<'p, T: PyClass> Deref for PyRefMut<'p, T> { #[inline] fn deref(&self) -> &T { - unsafe { &*self.inner.get_ptr() } + unsafe { &*self.inner.as_gil_ref().get_ptr() } } } impl<'p, T: PyClass> DerefMut for PyRefMut<'p, T> { #[inline] fn deref_mut(&mut self) -> &mut T { - unsafe { &mut *self.inner.get_ptr() } + unsafe { &mut *self.inner.as_gil_ref().get_ptr() } } } impl<'p, T: PyClass> Drop for PyRefMut<'p, T> { fn drop(&mut self) { - self.inner.borrow_checker().release_borrow_mut() + self.inner + .as_gil_ref() + .borrow_checker() + .release_borrow_mut() } } @@ -905,7 +919,7 @@ impl> IntoPy for PyRefMut<'_, T> { impl> IntoPy for &'_ PyRefMut<'_, T> { fn into_py(self, py: Python<'_>) -> PyObject { - self.inner.into_py(py) + self.inner.clone().into_py(py) } } From ec7598f22a13ce84f79933808a523ee36b300ab3 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 29 Nov 2023 11:10:36 +0000 Subject: [PATCH 7/8] finish migrations to `FromPyObject::extract_bound` --- src/conversion.rs | 14 +++++++------ src/conversions/num_complex.rs | 4 ++-- src/instance.rs | 14 ++++++++++++- src/pycell.rs | 38 +++++++++++++++++++++++++--------- src/types/any.rs | 8 ++----- src/types/boolobject.rs | 4 ++-- src/types/typeobject.rs | 2 +- tests/test_mapping.rs | 2 +- tests/test_pyfunction.rs | 2 +- tests/test_sequence.rs | 4 ++-- 10 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/conversion.rs b/src/conversion.rs index d9536aa9445..e2f47c47c47 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -2,11 +2,13 @@ use crate::err::{self, PyDowncastError, PyResult}; #[cfg(feature = "experimental-inspect")] use crate::inspect::types::TypeInfo; +use crate::instance::Bound; use crate::pyclass::boolean_struct::False; use crate::type_object::PyTypeInfo; +use crate::types::any::PyAnyMethods; use crate::types::PyTuple; use crate::{ - ffi, gil, Bound, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python, + ffi, gil, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python, }; use std::cell::Cell; use std::ptr::NonNull; @@ -309,8 +311,8 @@ impl> IntoPy for Cell { } impl<'py, T: FromPyObject<'py>> FromPyObject<'py> for Cell { - fn extract(ob: &'py PyAny) -> PyResult { - T::extract(ob).map(Cell::new) + fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult { + obj.extract().map(Cell::new) } } @@ -357,11 +359,11 @@ impl<'py, T> FromPyObject<'py> for Option where T: FromPyObject<'py>, { - fn extract(obj: &'py PyAny) -> PyResult { - if obj.as_ptr() == unsafe { ffi::Py_None() } { + fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult { + if obj.is_none() { Ok(None) } else { - T::extract(obj).map(Some) + obj.extract().map(Some) } } } diff --git a/src/conversions/num_complex.rs b/src/conversions/num_complex.rs index ba741323611..d15121f9bed 100644 --- a/src/conversions/num_complex.rs +++ b/src/conversions/num_complex.rs @@ -151,7 +151,7 @@ macro_rules! complex_conversion { unsafe { let complex; let obj = if obj.is_instance_of::() { - obj + obj.clone() } else if let Some(method) = obj.lookup_special(crate::intern!(obj.py(), "__complex__"))? { @@ -161,7 +161,7 @@ macro_rules! complex_conversion { // `obj` might still implement `__float__` or `__index__`, which will be // handled by `PyComplex_{Real,Imag}AsDouble`, including propagating any // errors if those methods don't exist / raise exceptions. - obj + obj.clone() }; let ptr = obj.as_ptr(); let real = ffi::PyComplex_RealAsDouble(ptr); diff --git a/src/instance.rs b/src/instance.rs index cee5bc33ee6..08fda5a2926 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -137,6 +137,18 @@ impl<'py> Bound<'py, PyAny> { ) -> PyResult { Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) } + + /// Constructs a new Bound from a borrowed pointer, incrementing the reference count. + /// Returns None if ptr is null. + /// + /// # Safety + /// ptr must be a valid pointer to a Python object, or NULL. + pub unsafe fn from_borrowed_ptr_or_opt( + py: Python<'py>, + ptr: *mut ffi::PyObject, + ) -> Option { + Py::from_borrowed_ptr_or_opt(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) + } } impl<'py, T> Bound<'py, T> @@ -1194,7 +1206,7 @@ impl Py { where D: FromPyObject<'py>, { - FromPyObject::extract(unsafe { py.from_borrowed_ptr(self.as_ptr()) }) + self.bind(py).as_any().extract() } /// Retrieves an attribute value. diff --git a/src/pycell.rs b/src/pycell.rs index c2fafecf477..66d998317a4 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -304,7 +304,7 @@ impl PyCell { /// Panics if the value is currently mutably borrowed. For a non-panicking variant, use /// [`try_borrow`](#method.try_borrow). pub fn borrow(&self) -> PyRef<'_, T> { - self.try_borrow().expect("Already mutably borrowed") + PyRef::borrow(&self.as_borrowed()) } /// Mutably borrows the value `T`. This borrow lasts as long as the returned `PyRefMut` exists. @@ -317,7 +317,7 @@ impl PyCell { where T: PyClass, { - self.try_borrow_mut().expect("Already borrowed") + PyRefMut::borrow_mut(&self.as_borrowed()) } /// Immutably borrows the value `T`, returning an error if the value is currently @@ -348,10 +348,7 @@ impl PyCell { /// }); /// ``` pub fn try_borrow(&self) -> Result, PyBorrowError> { - self.ensure_threadsafe(); - self.borrow_checker().try_borrow().map(|_| PyRef { - inner: self.as_borrowed().to_owned(), - }) + PyRef::try_borrow(&self.as_borrowed()) } /// Variant of [`try_borrow`][Self::try_borrow] which fails instead of panicking if called from the wrong thread @@ -387,10 +384,7 @@ impl PyCell { where T: PyClass, { - self.ensure_threadsafe(); - self.borrow_checker().try_borrow_mut().map(|_| PyRefMut { - inner: self.as_borrowed().to_owned(), - }) + PyRefMut::try_borrow_mut(&self.as_borrowed()) } /// Immutably borrows the value `T`, returning an error if the value is @@ -694,6 +688,18 @@ impl<'p, T: PyClass> PyRef<'p, T> { pub fn into_ptr(self) -> *mut ffi::PyObject { self.inner.clone().into_ptr() } + + pub(crate) fn borrow(obj: &Bound<'p, T>) -> Self { + Self::try_borrow(obj).expect("Already mutably borrowed") + } + + pub(crate) fn try_borrow(obj: &Bound<'p, T>) -> Result { + obj.as_gil_ref().ensure_threadsafe(); + obj.as_gil_ref() + .borrow_checker() + .try_borrow() + .map(|_| Self { inner: obj.clone() }) + } } impl<'p, T, U> PyRef<'p, T> @@ -863,6 +869,18 @@ impl<'p, T: PyClass> PyRefMut<'p, T> { pub fn into_ptr(self) -> *mut ffi::PyObject { self.inner.clone().into_ptr() } + + pub(crate) fn borrow_mut(obj: &Bound<'p, T>) -> Self { + Self::try_borrow_mut(obj).expect("Already borrowed") + } + + pub(crate) fn try_borrow_mut(obj: &Bound<'p, T>) -> Result { + obj.as_gil_ref().ensure_threadsafe(); + obj.as_gil_ref() + .borrow_checker() + .try_borrow_mut() + .map(|_| Self { inner: obj.clone() }) + } } impl<'p, T, U> PyRefMut<'p, T> diff --git a/src/types/any.rs b/src/types/any.rs index 6420c1587d9..f892fd539ea 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -2286,7 +2286,7 @@ impl<'py> Bound<'py, PyAny> { N: IntoPy>, { let py = self.py(); - let self_type = self.get_type().as_borrowed(); + let self_type = self.get_type(); let attr = if let Ok(attr) = self_type.getattr(attr_name) { attr } else { @@ -2308,11 +2308,7 @@ impl<'py> Bound<'py, PyAny> { let ret = descr_get(attr.as_ptr(), self.as_ptr(), self_type.as_ptr()); ret.assume_owned_or_err(py).map(Some) } - } else if let Ok(descr_get) = attr - .get_type() - .as_borrowed() - .getattr(crate::intern!(py, "__get__")) - { + } else if let Ok(descr_get) = attr.get_type().getattr(crate::intern!(py, "__get__")) { descr_get.call1((attr, self, self_type)).map(Some) } else { Ok(Some(attr)) diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index c0c4e4ba5c2..e2a5349bf4a 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -1,12 +1,12 @@ #[cfg(feature = "experimental-inspect")] use crate::inspect::types::TypeInfo; +use crate::types::any::PyAnyMethods; +use crate::types::typeobject::PyTypeMethods; use crate::{ exceptions::PyTypeError, ffi, ffi_ptr_ext::FfiPtrExt, instance::Bound, Borrowed, FromPyObject, IntoPy, PyAny, PyNativeType, PyObject, PyResult, Python, ToPyObject, }; -use super::any::PyAnyMethods; - /// Represents a Python `bool`. #[repr(transparent)] pub struct PyBool(PyAny); diff --git a/src/types/typeobject.rs b/src/types/typeobject.rs index c6ccef6282f..a60954cce89 100644 --- a/src/types/typeobject.rs +++ b/src/types/typeobject.rs @@ -48,7 +48,7 @@ impl PyType { p.cast::() .assume_borrowed(py) .downcast_unchecked() - .clone() + .to_owned() } /// Gets the [qualified name](https://docs.python.org/3/glossary.html#term-qualified-name) of the `PyType`. diff --git a/tests/test_mapping.rs b/tests/test_mapping.rs index 9c99d56467f..30001d1a881 100644 --- a/tests/test_mapping.rs +++ b/tests/test_mapping.rs @@ -25,7 +25,7 @@ impl Mapping { if let Some(pylist) = elements { let mut elems = HashMap::with_capacity(pylist.len()); for (i, pyelem) in pylist.into_iter().enumerate() { - let elem = String::extract(pyelem)?; + let elem = pyelem.extract()?; elems.insert(elem, i); } Ok(Self { index: elems }) diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index e06f7c2fb9c..6b80bde85c1 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -215,7 +215,7 @@ struct ValueClass { fn conversion_error( str_arg: &str, int_arg: i64, - tuple_arg: (&str, f64), + tuple_arg: (String, f64), option_arg: Option, struct_arg: Option, ) { diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index d3c84b76c8c..b7527313b50 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -21,7 +21,7 @@ impl ByteSequence { if let Some(pylist) = elements { let mut elems = Vec::with_capacity(pylist.len()); for pyelem in pylist { - let elem = u8::extract(pyelem)?; + let elem = pyelem.extract()?; elems.push(elem); } Ok(Self { elements: elems }) @@ -61,7 +61,7 @@ impl ByteSequence { } fn __contains__(&self, other: &PyAny) -> bool { - match u8::extract(other) { + match other.extract::() { Ok(x) => self.elements.contains(&x), Err(_) => false, } From c3d9e69cb430e537a5670d8040ff366629993c00 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 3 Dec 2023 17:33:31 +0000 Subject: [PATCH 8/8] fixup benchmarks --- pyo3-benches/benches/bench_call.rs | 2 ++ pyo3-benches/benches/bench_decimal.rs | 1 + pyo3-benches/benches/bench_extract.rs | 8 ++++---- pyo3-benches/benches/bench_frompyobject.rs | 2 +- src/conversions/num_complex.rs | 4 ++-- src/err/mod.rs | 6 +++--- src/instance.rs | 11 ++++++++++- src/pycell.rs | 1 - tests/ui/invalid_result_conversion.stderr | 2 +- 9 files changed, 24 insertions(+), 13 deletions(-) diff --git a/pyo3-benches/benches/bench_call.rs b/pyo3-benches/benches/bench_call.rs index 50772097961..3347b548603 100644 --- a/pyo3-benches/benches/bench_call.rs +++ b/pyo3-benches/benches/bench_call.rs @@ -13,6 +13,7 @@ fn bench_call_0(b: &mut Bencher<'_>) { let module = test_module!(py, "def foo(): pass"); let foo_module = module.getattr("foo").unwrap(); + let foo_module = &foo_module.as_borrowed(); b.iter(|| { for _ in 0..1000 { @@ -34,6 +35,7 @@ class Foo: ); let foo_module = module.getattr("Foo").unwrap().call0().unwrap(); + let foo_module = &foo_module.as_borrowed(); b.iter(|| { for _ in 0..1000 { diff --git a/pyo3-benches/benches/bench_decimal.rs b/pyo3-benches/benches/bench_decimal.rs index 6db6704bf8e..896305ec84b 100644 --- a/pyo3-benches/benches/bench_decimal.rs +++ b/pyo3-benches/benches/bench_decimal.rs @@ -17,6 +17,7 @@ py_dec = decimal.Decimal("0.0") ) .unwrap(); let py_dec = locals.get_item("py_dec").unwrap().unwrap(); + let py_dec = &py_dec.as_borrowed(); b.iter(|| { let _: Decimal = black_box(&py_dec).extract().unwrap(); diff --git a/pyo3-benches/benches/bench_extract.rs b/pyo3-benches/benches/bench_extract.rs index 1c783c3b706..1df8bad0891 100644 --- a/pyo3-benches/benches/bench_extract.rs +++ b/pyo3-benches/benches/bench_extract.rs @@ -50,7 +50,7 @@ fn extract_str_downcast_fail(bench: &mut Bencher<'_>) { fn extract_int_extract_success(bench: &mut Bencher<'_>) { Python::with_gil(|py| { let int_obj: PyObject = 123.into_py(py); - let int = int_obj.as_ref(py); + let int = int_obj.bind(py); bench.iter(|| { let v = black_box(int).extract::().unwrap(); @@ -73,7 +73,7 @@ fn extract_int_extract_fail(bench: &mut Bencher<'_>) { fn extract_int_downcast_success(bench: &mut Bencher<'_>) { Python::with_gil(|py| { let int_obj: PyObject = 123.into_py(py); - let int = int_obj.as_ref(py); + let int = int_obj.bind(py); bench.iter(|| { let py_int = black_box(int).downcast::().unwrap(); @@ -97,7 +97,7 @@ fn extract_int_downcast_fail(bench: &mut Bencher<'_>) { fn extract_float_extract_success(bench: &mut Bencher<'_>) { Python::with_gil(|py| { let float_obj: PyObject = 23.42.into_py(py); - let float = float_obj.as_ref(py); + let float = float_obj.bind(py); bench.iter(|| { let v = black_box(float).extract::().unwrap(); @@ -120,7 +120,7 @@ fn extract_float_extract_fail(bench: &mut Bencher<'_>) { fn extract_float_downcast_success(bench: &mut Bencher<'_>) { Python::with_gil(|py| { let float_obj: PyObject = 23.42.into_py(py); - let float = float_obj.as_ref(py); + let float = float_obj.bind(py); bench.iter(|| { let py_int = black_box(float).downcast::().unwrap(); diff --git a/pyo3-benches/benches/bench_frompyobject.rs b/pyo3-benches/benches/bench_frompyobject.rs index 8114ee5a802..2bba0d0a12a 100644 --- a/pyo3-benches/benches/bench_frompyobject.rs +++ b/pyo3-benches/benches/bench_frompyobject.rs @@ -62,7 +62,7 @@ fn not_a_list_via_extract_enum(b: &mut Bencher<'_>) { Python::with_gil(|py| { let any: &Bound<'_, PyAny> = &PyString::new_bound(py, "foobar"); - b.iter(|| match black_box(any).extract::>() { + b.iter(|| match black_box(&any).extract::>() { Ok(ListOrNotList::List(_list)) => panic!(), Ok(ListOrNotList::NotList(any)) => any, Err(_) => panic!(), diff --git a/src/conversions/num_complex.rs b/src/conversions/num_complex.rs index d15121f9bed..ba741323611 100644 --- a/src/conversions/num_complex.rs +++ b/src/conversions/num_complex.rs @@ -151,7 +151,7 @@ macro_rules! complex_conversion { unsafe { let complex; let obj = if obj.is_instance_of::() { - obj.clone() + obj } else if let Some(method) = obj.lookup_special(crate::intern!(obj.py(), "__complex__"))? { @@ -161,7 +161,7 @@ macro_rules! complex_conversion { // `obj` might still implement `__float__` or `__index__`, which will be // handled by `PyComplex_{Real,Imag}AsDouble`, including propagating any // errors if those methods don't exist / raise exceptions. - obj.clone() + obj }; let ptr = obj.as_ptr(); let real = ffi::PyComplex_RealAsDouble(ptr); diff --git a/src/err/mod.rs b/src/err/mod.rs index 644645875b0..75d7ab9e1c6 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -875,15 +875,15 @@ impl PyErrArguments for PyDowncastErrorArguments { } /// Convert `PyDowncastError` to Python `TypeError`. -impl<'a> std::convert::From> for PyErr { +impl std::convert::From> for PyErr { fn from(err: PyDowncastError<'_>) -> PyErr { PyErr::from(err.0) } } -impl<'a> std::error::Error for PyDowncastError<'a> {} +impl std::error::Error for PyDowncastError<'_> {} -impl<'a> std::fmt::Display for PyDowncastError<'a> { +impl std::fmt::Display for PyDowncastError<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { self.0.fmt(f) } diff --git a/src/instance.rs b/src/instance.rs index 08fda5a2926..38659d51396 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -149,6 +149,14 @@ impl<'py> Bound<'py, PyAny> { ) -> Option { Py::from_borrowed_ptr_or_opt(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) } + + /// Constructs a new Bound from a borrowed pointer, incrementing the reference count. + /// + /// # Safety + /// ptr must be a valid pointer to a Python object. + pub unsafe fn from_borrowed_ptr_unchecked(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + Self::from_borrowed_ptr_or_opt(py, ptr).unwrap() + } } impl<'py, T> Bound<'py, T> @@ -511,7 +519,8 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// This is similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by /// the caller and it's the caller's responsibility to ensure that the reference this is /// derived from is valid for the lifetime `'a`. - pub(crate) unsafe fn from_ptr_unchecked(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + #[doc(hidden)] // Used in macro code. + pub unsafe fn from_ptr_unchecked(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { Self(NonNull::new_unchecked(ptr), PhantomData, py) } diff --git a/src/pycell.rs b/src/pycell.rs index 66d998317a4..fecc1a3f040 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -196,7 +196,6 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::{ PyClassBaseType, PyClassDict, PyClassImpl, PyClassThreadChecker, PyClassWeakRef, }; -use crate::instance::Bound; use crate::pyclass::{ boolean_struct::{False, True}, PyClass, diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index d2417a86336..a7af0b898db 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -5,10 +5,10 @@ error[E0277]: the trait bound `PyErr: From` is not satisfied | ^^^^^^^^^^^^^ the trait `From` is not implemented for `PyErr` | = help: the following other types implement trait `From`: + >> > > > - >> >> >> >