Merge pull request #3706 from davidhewitt/frompyobject2

Add `extract_bound` method to FromPyObject

guide/src/migration.md

@@ -241,12 +241,42 @@ To minimise breakage of code using the GIL-Refs API, the `Bound<T>` 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<T>` 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<T>` to pass these types around (or use `.clone()` at the very small cost of increasing the Python reference count)
 - `Bound<PyList>` and `Bound<PyTuple>` cannot support indexing with `list[0]`, you should use `list.get_item(0)` instead.
 - `Bound<PyTuple>::iter_borrowed` is slightly more efficient than `Bound<PyTuple>::iter`. The default iteration of `Bound<PyTuple>` cannot return borrowed references because Rust does not (yet) have "lending iterators". Similarly `Bound<PyTuple>::get_borrowed_item` is more efficient than `Bound<PyTuple>::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<Self> {
+        /* ... */
+    }
+}
+```
+
+After:
+
+```rust,ignore
+impl<'py> FromPyObject<'py> for MyType {
+    fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult<Self> {
+        /* ... */
+    }
+}
+```
+
+
+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

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.

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<T>: 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
+/// During the migration of PyO3 from the "GIL Refs" API to the `Bound<T>` smart pointer, this trait
-/// `FromPyObject` for historical reasons.
+/// 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`.
+    /// Extracts `Self` from the source GIL Ref `obj`.
-    fn extract(ob: &'source PyAny) -> PyResult<Self>;
+    ///
+    /// 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> {
+        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> {
+        Self::extract(ob.clone().into_gil_ref())
+    }
 
     /// Extracts the type hint information for this type when it appears as an argument.
     ///

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> {
+        impl FromPyObject<'_> for Complex<$float> {
-            fn extract(obj: &'source PyAny) -> PyResult<Complex<$float>> {
+            fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult<Complex<$float>> {
                 #[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::<PyComplex>() {
                         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

src/instance.rs

@@ -1425,11 +1425,8 @@ where
     T: PyTypeInfo,
 {
     /// Extracts `Self` from the source `PyObject`.
-    fn extract(ob: &'a PyAny) -> PyResult<Self> {
+    fn extract_bound(ob: &Bound<'a, PyAny>) -> PyResult<Self> {
-        ob.as_borrowed()
+        ob.downcast().map(Clone::clone).map_err(Into::into)
-            .downcast()
-            .map(Clone::clone)
-            .map_err(Into::into)
     }
 }
 

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<N>(&self, attr_name: N) -> PyResult<Option<&PyAny>>
-    where
-        N: IntoPy<Py<PyString>>,
-    {
-        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<D>
+    fn extract<D>(&self) -> PyResult<D>
     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<D>
+    fn extract<D>(&self) -> PyResult<D>
     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<N>(&self, attr_name: N) -> PyResult<Option<Bound<'py, PyAny>>>
+    where
+        N: IntoPy<Py<PyString>>,
+    {
+        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},
+        types::{any::PyAnyMethods, IntoPyDict, PyAny, PyBool, PyList, PyLong, PyModule},
-        PyTypeInfo, Python, ToPyObject,
+        PyNativeType, PyTypeInfo, Python, ToPyObject,
     };
 
     #[test]
@@ -2344,8 +2350,13 @@ class NonHeapNonDescriptorInt:
             .unwrap();
 
             let int = crate::intern!(py, "__int__");
-            let eval_int =
+            let eval_int = |obj: &PyAny| {
-                |obj: &PyAny| obj.lookup_special(int)?.unwrap().call0()?.extract::<u32>();
+                obj.as_borrowed()
+                    .lookup_special(int)?
+                    .unwrap()
+                    .call0()?
+                    .extract::<u32>()
+            };
 
             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()

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<PyObject> 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 {
+impl FromPyObject<'_> for bool {
-    fn extract(obj: &'source PyAny) -> PyResult<Self> {
+    fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult<Self> {
         let err = match obj.downcast::<PyBool>() {
             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::<PyBool>()?;
+                let obj = meth.call0()?.downcast_into::<PyBool>()?;
                 return Ok(obj.is_true());
             }
         }

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::<i32>().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::<i32>().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::<i32>().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::<i32>().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::<i32>().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::<i32>().unwrap());
             assert_eq!(iter.size_hint(), (0, Some(0)));
 
             assert!(iter.next().is_none());