From 595ca4b3c17863ad3b36dc5659df7e2f2aab087b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 27 Dec 2023 08:56:01 +0000 Subject: [PATCH 1/2] Add `extract_bound` method to `FromPyObject` --- guide/src/migration.md | 30 ++++++++++++++++++++++++++++++ newsfragments/3706.added.md | 1 + src/conversion.rs | 25 ++++++++++++++++++++----- 3 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 newsfragments/3706.added.md diff --git a/guide/src/migration.md b/guide/src/migration.md index e04b477a345..9fb5adba387 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -241,12 +241,42 @@ To minimise breakage of code using the GIL-Refs API, the `Bound` smart pointe For example, the following APIs have gained updated variants: - `PyList::new`, `PyTyple::new` and similar constructors have replacements `PyList::new_bound`, `PyTuple::new_bound` etc. +- `FromPyObject::extract` has a new `FromPyObject::extract_bound` (see the section below) Because the new `Bound` API brings ownership out of the PyO3 framework and into user code, there are a few places where user code is expected to need to adjust while switching to the new API: - Code will need to add the occasional `&` to borrow the new smart pointer as `&Bound` to pass these types around (or use `.clone()` at the very small cost of increasing the Python reference count) - `Bound` and `Bound` cannot support indexing with `list[0]`, you should use `list.get_item(0)` instead. - `Bound::iter_borrowed` is slightly more efficient than `Bound::iter`. The default iteration of `Bound` cannot return borrowed references because Rust does not (yet) have "lending iterators". Similarly `Bound::get_borrowed_item` is more efficient than `Bound::get_item` for the same reason. +#### Migrating `FromPyObject` implementations + +`FromPyObject` has had a new method `extract_bound` which takes `&Bound<'py, PyAny>` as an argument instead of `&PyAny`. Both `extract` and `extract_bound` have been given default implementations in terms of the other, to avoid breaking code immediately on update to 0.21. + +All implementations of `FromPyObject` should be switched from `extract` to `extract_bound`. + +Before: + +```rust,ignore +impl<'py> FromPyObject<'py> for MyType { + fn extract(obj: &'py PyAny) -> PyResult { + /* ... */ + } +} +``` + +After: + +```rust,ignore +impl<'py> FromPyObject<'py> for MyType { + fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult { + /* ... */ + } +} +``` + + +The expectation is that in 0.22 `extract_bound` will have the default implementation removed and in 0.23 `extract` will be removed. + ## from 0.19.* to 0.20 ### Drop support for older technologies diff --git a/newsfragments/3706.added.md b/newsfragments/3706.added.md new file mode 100644 index 00000000000..31db8b96cef --- /dev/null +++ b/newsfragments/3706.added.md @@ -0,0 +1 @@ +Add `FromPyObject::extract_bound` method, which can be implemented to avoid using the GIL Ref API in `FromPyObject` implementations. diff --git a/src/conversion.rs b/src/conversion.rs index 429ca9e8779..46b39436e88 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -6,7 +6,7 @@ use crate::pyclass::boolean_struct::False; use crate::type_object::PyTypeInfo; use crate::types::PyTuple; use crate::{ - ffi, gil, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python, + ffi, gil, Bound, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python, }; use std::cell::Cell; use std::ptr::NonNull; @@ -215,11 +215,26 @@ pub trait IntoPy: Sized { /// Since which case applies depends on the runtime type of the Python object, /// both the `obj` and `prepared` variables must outlive the resulting string slice. /// -/// The trait's conversion method takes a `&PyAny` argument but is called -/// `FromPyObject` for historical reasons. +/// During the migration of PyO3 from the "GIL Refs" API to the `Bound` smart pointer, this trait +/// has two methods `extract` and `extract_bound` which are defaulted to call each other. To avoid +/// infinite recursion, implementors must implement at least one of these methods. The recommendation +/// is to implement `extract_bound` and leave `extract` as the default implementation. pub trait FromPyObject<'source>: Sized { - /// Extracts `Self` from the source `PyObject`. - fn extract(ob: &'source PyAny) -> PyResult; + /// Extracts `Self` from the source GIL Ref `obj`. + /// + /// Implementors are encouraged to implement `extract_bound` and leave this method as the + /// default implementation, which will forward calls to `extract_bound`. + fn extract(ob: &'source PyAny) -> PyResult { + Self::extract_bound(&ob.as_borrowed()) + } + + /// Extracts `Self` from the bound smart pointer `obj`. + /// + /// Implementors are encouraged to implement this method and leave `extract` defaulted, as + /// this will be most compatible with PyO3's future API. + fn extract_bound(ob: &Bound<'source, PyAny>) -> PyResult { + Self::extract(ob.clone().into_gil_ref()) + } /// Extracts the type hint information for this type when it appears as an argument. /// From ffaa03e3f1959aa6c714896920dd7830a12fe964 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 27 Dec 2023 08:56:26 +0000 Subject: [PATCH 2/2] Migrate some conversions to `extract_bound` --- src/conversions/num_complex.rs | 13 ++-- src/instance.rs | 7 +- src/types/any.rs | 123 ++++++++++++++++++--------------- src/types/boolobject.rs | 10 +-- src/types/tuple.rs | 12 ++-- 5 files changed, 90 insertions(+), 75 deletions(-) diff --git a/src/conversions/num_complex.rs b/src/conversions/num_complex.rs index d488cfd466d..ba741323611 100644 --- a/src/conversions/num_complex.rs +++ b/src/conversions/num_complex.rs @@ -93,8 +93,11 @@ //! result = get_eigenvalues(m11,m12,m21,m22) //! assert result == [complex(1,-1), complex(-2,0)] //! ``` +#[cfg(any(Py_LIMITED_API, PyPy))] +use crate::types::any::PyAnyMethods; use crate::{ - ffi, types::PyComplex, FromPyObject, PyAny, PyErr, PyObject, PyResult, Python, ToPyObject, + ffi, types::PyComplex, Bound, FromPyObject, PyAny, PyErr, PyObject, PyResult, Python, + ToPyObject, }; use num_complex::Complex; use std::os::raw::c_double; @@ -131,8 +134,8 @@ macro_rules! complex_conversion { } #[cfg_attr(docsrs, doc(cfg(feature = "num-complex")))] - impl<'source> FromPyObject<'source> for Complex<$float> { - fn extract(obj: &'source PyAny) -> PyResult> { + impl FromPyObject<'_> for Complex<$float> { + fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult> { #[cfg(not(any(Py_LIMITED_API, PyPy)))] unsafe { let val = ffi::PyComplex_AsCComplex(obj.as_ptr()); @@ -146,12 +149,14 @@ macro_rules! complex_conversion { #[cfg(any(Py_LIMITED_API, PyPy))] unsafe { + let complex; let obj = if obj.is_instance_of::() { obj } else if let Some(method) = obj.lookup_special(crate::intern!(obj.py(), "__complex__"))? { - method.call0()? + complex = method.call0()?; + &complex } else { // `obj` might still implement `__float__` or `__index__`, which will be // handled by `PyComplex_{Real,Imag}AsDouble`, including propagating any diff --git a/src/instance.rs b/src/instance.rs index ececf090b4f..d7edfe06365 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1425,11 +1425,8 @@ where T: PyTypeInfo, { /// Extracts `Self` from the source `PyObject`. - fn extract(ob: &'a PyAny) -> PyResult { - ob.as_borrowed() - .downcast() - .map(Clone::clone) - .map_err(Into::into) + fn extract_bound(ob: &Bound<'a, PyAny>) -> PyResult { + ob.downcast().map(Clone::clone).map_err(Into::into) } } diff --git a/src/types/any.rs b/src/types/any.rs index 4536c2b889f..901b3451c3c 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -136,51 +136,6 @@ impl PyAny { .map(Bound::into_gil_ref) } - /// Retrieve an attribute value, skipping the instance dictionary during the lookup but still - /// binding the object to the instance. - /// - /// This is useful when trying to resolve Python's "magic" methods like `__getitem__`, which - /// are looked up starting from the type object. This returns an `Option` as it is not - /// typically a direct error for the special lookup to fail, as magic methods are optional in - /// many situations in which they might be called. - /// - /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used - /// to intern `attr_name`. - #[allow(dead_code)] // Currently only used with num-complex+abi3, so dead without that. - pub(crate) fn lookup_special(&self, attr_name: N) -> PyResult> - where - N: IntoPy>, - { - let py = self.py(); - let self_type = self.get_type(); - let attr = if let Ok(attr) = self_type.getattr(attr_name) { - attr - } else { - return Ok(None); - }; - - // Manually resolve descriptor protocol. - if cfg!(Py_3_10) - || unsafe { ffi::PyType_HasFeature(attr.get_type_ptr(), ffi::Py_TPFLAGS_HEAPTYPE) } != 0 - { - // This is the preferred faster path, but does not work on static types (generally, - // types defined in extension modules) before Python 3.10. - unsafe { - let descr_get_ptr = ffi::PyType_GetSlot(attr.get_type_ptr(), ffi::Py_tp_descr_get); - if descr_get_ptr.is_null() { - return Ok(Some(attr)); - } - let descr_get: ffi::descrgetfunc = std::mem::transmute(descr_get_ptr); - let ret = descr_get(attr.as_ptr(), self.as_ptr(), self_type.as_ptr()); - py.from_owned_ptr_or_err(ret).map(Some) - } - } 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)) - } - } - /// Sets an attribute value. /// /// This is equivalent to the Python expression `self.attr_name = value`. @@ -1666,9 +1621,9 @@ pub trait PyAnyMethods<'py> { /// Extracts some type from the Python object. /// /// This is a wrapper function around [`FromPyObject::extract()`]. - fn extract<'a, D>(&'a self) -> PyResult + fn extract(&self) -> PyResult where - D: FromPyObject<'a>; + D: FromPyObject<'py>; /// Returns the reference count for the Python object. fn get_refcnt(&self) -> isize; @@ -2202,11 +2157,11 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> { std::mem::transmute(self) } - fn extract<'a, D>(&'a self) -> PyResult + fn extract(&self) -> PyResult where - D: FromPyObject<'a>, + D: FromPyObject<'py>, { - FromPyObject::extract(self.as_gil_ref()) + FromPyObject::extract_bound(self) } fn get_refcnt(&self) -> isize { @@ -2293,13 +2248,64 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> { } } +impl<'py> Bound<'py, PyAny> { + /// Retrieve an attribute value, skipping the instance dictionary during the lookup but still + /// binding the object to the instance. + /// + /// This is useful when trying to resolve Python's "magic" methods like `__getitem__`, which + /// are looked up starting from the type object. This returns an `Option` as it is not + /// typically a direct error for the special lookup to fail, as magic methods are optional in + /// many situations in which they might be called. + /// + /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used + /// to intern `attr_name`. + #[allow(dead_code)] // Currently only used with num-complex+abi3, so dead without that. + pub(crate) fn lookup_special(&self, attr_name: N) -> PyResult>> + where + N: IntoPy>, + { + let py = self.py(); + let self_type = self.get_type().as_borrowed(); + let attr = if let Ok(attr) = self_type.getattr(attr_name) { + attr + } else { + return Ok(None); + }; + + // Manually resolve descriptor protocol. + if cfg!(Py_3_10) + || unsafe { ffi::PyType_HasFeature(attr.get_type_ptr(), ffi::Py_TPFLAGS_HEAPTYPE) } != 0 + { + // This is the preferred faster path, but does not work on static types (generally, + // types defined in extension modules) before Python 3.10. + unsafe { + let descr_get_ptr = ffi::PyType_GetSlot(attr.get_type_ptr(), ffi::Py_tp_descr_get); + if descr_get_ptr.is_null() { + return Ok(Some(attr)); + } + let descr_get: ffi::descrgetfunc = std::mem::transmute(descr_get_ptr); + 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__")) + { + descr_get.call1((attr, self, self_type)).map(Some) + } else { + Ok(Some(attr)) + } + } +} + #[cfg(test)] #[cfg_attr(not(feature = "gil-refs"), allow(deprecated))] mod tests { use crate::{ basic::CompareOp, - types::{IntoPyDict, PyAny, PyBool, PyList, PyLong, PyModule}, - PyTypeInfo, Python, ToPyObject, + types::{any::PyAnyMethods, IntoPyDict, PyAny, PyBool, PyList, PyLong, PyModule}, + PyNativeType, PyTypeInfo, Python, ToPyObject, }; #[test] @@ -2344,8 +2350,13 @@ class NonHeapNonDescriptorInt: .unwrap(); let int = crate::intern!(py, "__int__"); - let eval_int = - |obj: &PyAny| obj.lookup_special(int)?.unwrap().call0()?.extract::(); + let eval_int = |obj: &PyAny| { + obj.as_borrowed() + .lookup_special(int)? + .unwrap() + .call0()? + .extract::() + }; let simple = module.getattr("SimpleInt").unwrap().call0().unwrap(); assert_eq!(eval_int(simple).unwrap(), 1); @@ -2354,7 +2365,7 @@ class NonHeapNonDescriptorInt: let no_descriptor = module.getattr("NoDescriptorInt").unwrap().call0().unwrap(); assert_eq!(eval_int(no_descriptor).unwrap(), 1); let missing = module.getattr("NoInt").unwrap().call0().unwrap(); - assert!(missing.lookup_special(int).unwrap().is_none()); + assert!(missing.as_borrowed().lookup_special(int).unwrap().is_none()); // Note the instance override should _not_ call the instance method that returns 2, // because that's not how special lookups are meant to work. let instance_override = module.getattr("instance_override").unwrap(); @@ -2364,7 +2375,7 @@ class NonHeapNonDescriptorInt: .unwrap() .call0() .unwrap(); - assert!(descriptor_error.lookup_special(int).is_err()); + assert!(descriptor_error.as_borrowed().lookup_special(int).is_err()); let nonheap_nondescriptor = module .getattr("NonHeapNonDescriptorInt") .unwrap() diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index 5c246de380d..0decc0b4c80 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -5,6 +5,8 @@ use crate::{ PyObject, PyResult, Python, ToPyObject, }; +use super::any::PyAnyMethods; + /// Represents a Python `bool`. #[repr(transparent)] pub struct PyBool(PyAny); @@ -75,8 +77,8 @@ impl IntoPy for bool { /// Converts a Python `bool` to a Rust `bool`. /// /// Fails with `TypeError` if the input is not a Python `bool`. -impl<'source> FromPyObject<'source> for bool { - fn extract(obj: &'source PyAny) -> PyResult { +impl FromPyObject<'_> for bool { + fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult { let err = match obj.downcast::() { Ok(obj) => return Ok(obj.is_true()), Err(err) => err, @@ -87,7 +89,7 @@ impl<'source> FromPyObject<'source> for bool { .name() .map_or(false, |name| name == "numpy.bool_") { - let missing_conversion = |obj: &PyAny| { + let missing_conversion = |obj: &Bound<'_, PyAny>| { PyTypeError::new_err(format!( "object of type '{}' does not define a '__bool__' conversion", obj.get_type() @@ -117,7 +119,7 @@ impl<'source> FromPyObject<'source> for bool { .lookup_special(crate::intern!(obj.py(), "__bool__"))? .ok_or_else(|| missing_conversion(obj))?; - let obj = meth.call0()?.downcast::()?; + let obj = meth.call0()?.downcast_into::()?; return Ok(obj.is_true()); } } diff --git a/src/types/tuple.rs b/src/types/tuple.rs index 4860b8de30e..38f5f7e94e4 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -917,13 +917,13 @@ mod tests { assert_eq!(iter.size_hint(), (3, Some(3))); - assert_eq!(1_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(1, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (2, Some(2))); - assert_eq!(2_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(2, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (1, Some(1))); - assert_eq!(3_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(3, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (0, Some(0))); assert!(iter.next().is_none()); @@ -940,13 +940,13 @@ mod tests { assert_eq!(iter.size_hint(), (3, Some(3))); - assert_eq!(3_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(3, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (2, Some(2))); - assert_eq!(2_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(2, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (1, Some(1))); - assert_eq!(1_i32, iter.next().unwrap().extract::<'_, i32>().unwrap()); + assert_eq!(1, iter.next().unwrap().extract::().unwrap()); assert_eq!(iter.size_hint(), (0, Some(0))); assert!(iter.next().is_none());