remove AsPyPointer trait (PyO3#5071)

* remove `AsPyPointer` trait

* fix typo

Co-authored-by: Bruno Kolenbrander <[email protected]>

---------

Co-authored-by: Bruno Kolenbrander <[email protected]>

Author: Icxolu

guide/src/class/protocols.md

@@ -443,9 +443,7 @@ struct ClassWithGCSupport {
 #[pymethods]
 impl ClassWithGCSupport {
     fn __traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
-        if let Some(obj) = &self.obj {
-            visit.call(obj)?
-        }
+        visit.call(&self.obj)?;
         Ok(())
     }
 

guide/src/migration.md

@@ -3,6 +3,17 @@
 This guide can help you upgrade code through breaking changes from one PyO3 version to the next.
 For a detailed list of all changes, see the [CHANGELOG](changelog.md).
 
+## from 0.24.* to 0.25
+### `AsPyPointer` removal
+<details open>
+<summary><small>Click to expand</small></summary>
+The `AsPyPointer` trait is mostly a leftover from the now removed gil-refs API. The last remaining uses were the GC API, namely `PyVisit::call`, and identity comparison (`PyAnyMethods::is` and `Py::is`).
+
+`PyVisit::call` has been updated to take `T: Into<Option<&Py<T>>>`, which allows for arguments of type `&Py<T>`, `&Option<Py<T>>` and `Option<&Py<T>>`. It is unlikely any changes are needed here to migrate.
+
+`PyAnyMethods::is`/`Py::is` has been updated to take `T: AsRef<Py<PyAny>>>`. Additionally `AsRef<Py<PyAny>>>` implementations were added for `Py`, `Bound` and `Borrowed`. Because of the existing `AsRef<Bound<PyAny>> for Bound<T>` implementation this may cause inference issues in non-generic code. This can be easily migrated by switching to `as_any` instead of `as_ref` for these calls.
+</details>
+
 ## from 0.22.* to 0.23
 <details open>
 <summary><small>Click to expand</small></summary>

newsfragments/5071.added.md

@@ -0,0 +1,3 @@
+- `AsRef<Py<PyAny>> for Py<T>`
+- `AsRef<Py<PyAny>> for Bound<T>`
+- `AsRef<Py<PyAny>> for Borrowed<T>`

newsfragments/5071.changed.md

@@ -0,0 +1,3 @@
+- `PyAnyMethods::is` now takes `other: &Bound<T>`
+- `Py::is` now takes `other: &Py<T>`
+- `PyVisit::call` now takes `T: Into<Option<&Py<T>>>`

newsfragments/5071.removed.md

@@ -0,0 +1 @@
+removed `AsPyPointer` trait

src/conversion.rs

@@ -5,67 +5,9 @@ use crate::inspect::types::TypeInfo;
 use crate::pyclass::boolean_struct::False;
 use crate::types::any::PyAnyMethods;
 use crate::types::PyTuple;
-use crate::{
-    ffi, Borrowed, Bound, BoundObject, Py, PyAny, PyClass, PyErr, PyRef, PyRefMut, Python,
-};
+use crate::{Borrowed, Bound, BoundObject, Py, PyAny, PyClass, PyErr, PyRef, PyRefMut, Python};
 use std::convert::Infallible;
 
-/// Returns a borrowed pointer to a Python object.
-///
-/// The returned pointer will be valid for as long as `self` is. It may be null depending on the
-/// implementation.
-///
-/// # Examples
-///
-/// ```rust
-/// use pyo3::prelude::*;
-/// use pyo3::ffi;
-///
-/// Python::with_gil(|py| {
-///     let s = "foo".into_pyobject(py)?;
-///     let ptr = s.as_ptr();
-///
-///     let is_really_a_pystring = unsafe { ffi::PyUnicode_CheckExact(ptr) };
-///     assert_eq!(is_really_a_pystring, 1);
-/// #   Ok::<_, PyErr>(())
-/// })
-/// # .unwrap();
-/// ```
-///
-/// # Safety
-///
-/// For callers, it is your responsibility to make sure that the underlying Python object is not dropped too
-/// early. For example, the following code will cause undefined behavior:
-///
-/// ```rust,no_run
-/// # use pyo3::prelude::*;
-/// # use pyo3::ffi;
-/// #
-/// Python::with_gil(|py| {
-///     // ERROR: calling `.as_ptr()` will throw away the temporary object and leave `ptr` dangling.
-///     let ptr: *mut ffi::PyObject = 0xabad1dea_u32.into_pyobject(py)?.as_ptr();
-///
-///     let isnt_a_pystring = unsafe {
-///         // `ptr` is dangling, this is UB
-///         ffi::PyUnicode_CheckExact(ptr)
-///     };
-/// #   assert_eq!(isnt_a_pystring, 0);
-/// #   Ok::<_, PyErr>(())
-/// })
-/// # .unwrap();
-/// ```
-///
-/// This happens because the pointer returned by `as_ptr` does not carry any lifetime information
-/// and the Python object is dropped immediately after the `0xabad1dea_u32.into_pyobject(py).as_ptr()`
-/// expression is evaluated. To fix the problem, bind Python object to a local variable like earlier
-/// to keep the Python object alive until the end of its scope.
-///
-/// Implementors must ensure this returns a valid pointer to a Python object, which borrows a reference count from `&self`.
-pub unsafe trait AsPyPointer {
-    /// Returns the underlying FFI pointer as a borrowed pointer.
-    fn as_ptr(&self) -> *mut ffi::PyObject;
-}
-
 /// Defines a conversion from a Rust type to a Python object, which may fail.
 ///
 /// This trait has `#[derive(IntoPyObject)]` to automatically implement it for simple types and

src/conversions/std/option.rs

@@ -1,6 +1,6 @@
 use crate::{
-    conversion::IntoPyObject, ffi, types::any::PyAnyMethods, AsPyPointer, Bound, BoundObject,
-    FromPyObject, PyAny, PyResult, Python,
+    conversion::IntoPyObject, types::any::PyAnyMethods, Bound, BoundObject, FromPyObject, PyAny,
+    PyResult, Python,
 };
 
 impl<'py, T> IntoPyObject<'py> for Option<T>
@@ -49,38 +49,3 @@ where
         }
     }
 }
-
-/// Convert `None` into a null pointer.
-unsafe impl<T> AsPyPointer for Option<T>
-where
-    T: AsPyPointer,
-{
-    #[inline]
-    fn as_ptr(&self) -> *mut ffi::PyObject {
-        self.as_ref()
-            .map_or_else(std::ptr::null_mut, |t| t.as_ptr())
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use crate::{PyObject, Python};
-
-    #[test]
-    fn test_option_as_ptr() {
-        Python::with_gil(|py| {
-            use crate::AsPyPointer;
-            let mut option: Option<PyObject> = None;
-            assert_eq!(option.as_ptr(), std::ptr::null_mut());
-
-            let none = py.None();
-            option = Some(none.clone_ref(py));
-
-            let ref_cnt = none.get_refcnt(py);
-            assert_eq!(option.as_ptr(), none.as_ptr());
-
-            // Ensure ref count not changed by as_ptr call
-            assert_eq!(none.get_refcnt(py), ref_cnt);
-        });
-    }
-}

src/impl_/extract_argument.rs

@@ -200,7 +200,7 @@ pub fn from_py_with_with_default<'a, 'py, T>(
 #[doc(hidden)]
 #[cold]
 pub fn argument_extraction_error(py: Python<'_>, arg_name: &str, error: PyErr) -> PyErr {
-    if error.get_type(py).is(&py.get_type::<PyTypeError>()) {
+    if error.get_type(py).is(py.get_type::<PyTypeError>()) {
         let remapped_error =
             PyTypeError::new_err(format!("argument '{}': {}", arg_name, error.value(py)));
         remapped_error.set_cause(py, error.cause(py));

src/instance.rs

@@ -7,8 +7,8 @@ use crate::pyclass::boolean_struct::{False, True};
 use crate::types::{any::PyAnyMethods, string::PyStringMethods, typeobject::PyTypeMethods};
 use crate::types::{DerefToPyAny, PyDict, PyString, PyTuple};
 use crate::{
-    ffi, AsPyPointer, DowncastError, FromPyObject, PyAny, PyClass, PyClassInitializer, PyRef,
-    PyRefMut, PyTypeInfo, Python,
+    ffi, DowncastError, FromPyObject, PyAny, PyClass, PyClassInitializer, PyRef, PyRefMut,
+    PyTypeInfo, Python,
 };
 use crate::{gil, PyTypeCheck};
 use std::marker::PhantomData;
@@ -524,6 +524,13 @@ impl<'py, T> AsRef<Bound<'py, PyAny>> for Bound<'py, T> {
     }
 }
 
+impl<T> AsRef<Py<PyAny>> for Bound<'_, T> {
+    #[inline]
+    fn as_ref(&self) -> &Py<PyAny> {
+        self.as_any().as_unbound()
+    }
+}
+
 impl<T> Clone for Bound<'_, T> {
     #[inline]
     fn clone(&self) -> Self {
@@ -612,13 +619,6 @@ impl<'py, T> Bound<'py, T> {
     }
 }
 
-unsafe impl<T> AsPyPointer for Bound<'_, T> {
-    #[inline]
-    fn as_ptr(&self) -> *mut ffi::PyObject {
-        self.1.as_ptr()
-    }
-}
-
 impl<'py, T> BoundObject<'py, T> for Bound<'py, T> {
     type Any = Bound<'py, PyAny>;
 
@@ -801,6 +801,13 @@ impl<'a, 'py, T> From<&'a Bound<'py, T>> for Borrowed<'a, 'py, T> {
     }
 }
 
+impl<T> AsRef<Py<PyAny>> for Borrowed<'_, '_, T> {
+    #[inline]
+    fn as_ref(&self) -> &Py<PyAny> {
+        self.as_any().as_unbound()
+    }
+}
+
 impl<T> std::fmt::Debug for Borrowed<'_, '_, T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         Bound::fmt(self, f)
@@ -1315,8 +1322,8 @@ impl<T> Py<T> {
     ///
     /// This is equivalent to the Python expression `self is other`.
     #[inline]
-    pub fn is<U: AsPyPointer>(&self, o: &U) -> bool {
-        ptr::eq(self.as_ptr(), o.as_ptr())
+    pub fn is<U: AsRef<Py<PyAny>>>(&self, o: U) -> bool {
+        ptr::eq(self.as_ptr(), o.as_ref().as_ptr())
     }
 
     /// Gets the reference count of the `ffi::PyObject` pointer.
@@ -1692,11 +1699,10 @@ impl<T> Py<T> {
     }
 }
 
-unsafe impl<T> crate::AsPyPointer for Py<T> {
-    /// Gets the underlying FFI pointer, returns a borrowed pointer.
+impl<T> AsRef<Py<PyAny>> for Py<T> {
     #[inline]
-    fn as_ptr(&self) -> *mut ffi::PyObject {
-        self.0.as_ptr()
+    fn as_ref(&self) -> &Py<PyAny> {
+        self.as_any()
     }
 }
 
@@ -2146,6 +2152,17 @@ a = A()
         });
     }
 
+    #[test]
+    fn test_borrowed_identity() {
+        Python::with_gil(|py| {
+            let yes = true.into_pyobject(py).unwrap();
+            let no = false.into_pyobject(py).unwrap();
+
+            assert!(yes.is(yes));
+            assert!(!yes.is(no));
+        });
+    }
+
     #[test]
     fn bound_from_borrowed_ptr_constructors() {
         // More detailed tests of the underlying semantics in pycell.rs

src/lib.rs

@@ -338,7 +338,7 @@
 #![doc = concat!("[Features chapter of the guide]: https://pyo3.rs/v", env!("CARGO_PKG_VERSION"), "/features.html#features-reference \"Features Reference - PyO3 user guide\"")]
 //! [`Ungil`]: crate::marker::Ungil
 pub use crate::class::*;
-pub use crate::conversion::{AsPyPointer, FromPyObject, IntoPyObject, IntoPyObjectExt};
+pub use crate::conversion::{FromPyObject, IntoPyObject, IntoPyObjectExt};
 pub use crate::err::{DowncastError, DowncastIntoError, PyErr, PyErrArguments, PyResult, ToPyErr};
 #[cfg(not(any(PyPy, GraalPy)))]
 pub use crate::gil::{prepare_freethreaded_python, with_embedded_python_interpreter};