From 809f0d5a9cb13af7b59e557b5a8427d2e2369275 Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Sat, 24 Aug 2024 21:55:42 +0300 Subject: [PATCH 1/2] Remove the `BoundObject` impl for `&Bound`, use `Borrowed` instead For rationale see https://github.com/PyO3/pyo3/issues/4467. I chose to make `bound_object_sealed::Sealed` unsafe instead of `BoundObject` so that users won't see the `# Safety` section, as it's not relevant for them. --- newsfragments/4487.changed.md | 1 + src/conversion.rs | 8 ++++---- src/instance.rs | 38 +++++++++-------------------------- src/pycell.rs | 10 ++++----- 4 files changed, 20 insertions(+), 37 deletions(-) create mode 100644 newsfragments/4487.changed.md diff --git a/newsfragments/4487.changed.md b/newsfragments/4487.changed.md new file mode 100644 index 00000000000..0cc7a899cda --- /dev/null +++ b/newsfragments/4487.changed.md @@ -0,0 +1 @@ +Remove the `BoundObject` impl for `&Bound`, use `Borrowed` instead. For rationale see https://github.com/PyO3/pyo3/issues/4467. \ No newline at end of file diff --git a/src/conversion.rs b/src/conversion.rs index d909382bdb9..c67562b86f9 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -192,8 +192,8 @@ pub trait IntoPyObject<'py>: Sized { type Target; /// The smart pointer type to use. /// - /// This will usually be [`Bound<'py, Target>`], but can special cases `&'a Bound<'py, Target>` - /// or [`Borrowed<'a, 'py, Target>`] can be used to minimize reference counting overhead. + /// This will usually be [`Bound<'py, Target>`], but in special cases [`Borrowed<'a, 'py, Target>`] can be + /// used to minimize reference counting overhead. type Output: BoundObject<'py, Self::Target>; /// The type returned in the event of a conversion error. type Error; @@ -273,11 +273,11 @@ impl<'py, T> IntoPyObject<'py> for Bound<'py, T> { impl<'a, 'py, T> IntoPyObject<'py> for &'a Bound<'py, T> { type Target = T; - type Output = &'a Bound<'py, Self::Target>; + type Output = Borrowed<'a, 'py, Self::Target>; type Error = Infallible; fn into_pyobject(self, _py: Python<'py>) -> Result { - Ok(self) + Ok(self.as_borrowed()) } } diff --git a/src/instance.rs b/src/instance.rs index cb537027203..6e3dcb0d578 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -16,6 +16,8 @@ use std::ops::Deref; use std::ptr::NonNull; /// Owned or borrowed gil-bound Python smart pointer +/// +/// This is implemented for [`Bound`] and [`Borrowed`]. pub trait BoundObject<'py, T>: bound_object_sealed::Sealed { /// Type erased version of `Self` type Any: BoundObject<'py, PyAny>; @@ -32,11 +34,15 @@ pub trait BoundObject<'py, T>: bound_object_sealed::Sealed { } mod bound_object_sealed { - pub trait Sealed {} + /// # Safety + /// + /// Type must be layout-compatible with `*mut ffi::PyObject`. + pub unsafe trait Sealed {} - impl<'py, T> Sealed for super::Bound<'py, T> {} - impl<'a, 'py, T> Sealed for &'a super::Bound<'py, T> {} - impl<'a, 'py, T> Sealed for super::Borrowed<'a, 'py, T> {} + // SAFETY: `Bound` is layout-compatible with `*mut ffi::PyObject`. + unsafe impl<'py, T> Sealed for super::Bound<'py, T> {} + // SAFETY: `Borrowed` is layout-compatible with `*mut ffi::PyObject`. + unsafe impl<'a, 'py, T> Sealed for super::Borrowed<'a, 'py, T> {} } /// A GIL-attached equivalent to [`Py`]. @@ -614,30 +620,6 @@ impl<'py, T> BoundObject<'py, T> for Bound<'py, T> { } } -impl<'a, 'py, T> BoundObject<'py, T> for &'a Bound<'py, T> { - type Any = &'a Bound<'py, PyAny>; - - fn as_borrowed(&self) -> Borrowed<'a, 'py, T> { - Bound::as_borrowed(self) - } - - fn into_bound(self) -> Bound<'py, T> { - self.clone() - } - - fn into_any(self) -> Self::Any { - self.as_any() - } - - fn into_ptr(self) -> *mut ffi::PyObject { - self.clone().into_ptr() - } - - fn unbind(self) -> Py { - self.clone().unbind() - } -} - /// A borrowed equivalent to `Bound`. /// /// The advantage of this over `&Bound` is that it avoids the need to have a pointer-to-pointer, as Bound diff --git a/src/pycell.rs b/src/pycell.rs index 0a05acd0f02..dc8b8b495a5 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -199,7 +199,7 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::internal_tricks::{ptr_from_mut, ptr_from_ref}; use crate::pyclass::{boolean_struct::False, PyClass}; use crate::types::any::PyAnyMethods; -use crate::{ffi, Bound, IntoPy, PyErr, PyObject, Python}; +use crate::{ffi, Borrowed, Bound, IntoPy, PyErr, PyObject, Python}; use std::convert::Infallible; use std::fmt; use std::mem::ManuallyDrop; @@ -479,11 +479,11 @@ impl<'py, T: PyClass> IntoPyObject<'py> for PyRef<'py, T> { impl<'a, 'py, T: PyClass> IntoPyObject<'py> for &'a PyRef<'py, T> { type Target = T; - type Output = &'a Bound<'py, T>; + type Output = Borrowed<'a, 'py, T>; type Error = Infallible; fn into_pyobject(self, _py: Python<'py>) -> Result { - Ok(&self.inner) + Ok(self.inner.as_borrowed()) } } @@ -668,11 +668,11 @@ impl<'py, T: PyClass> IntoPyObject<'py> for PyRefMut<'py, T> { impl<'a, 'py, T: PyClass> IntoPyObject<'py> for &'a PyRefMut<'py, T> { type Target = T; - type Output = &'a Bound<'py, T>; + type Output = Borrowed<'a, 'py, T>; type Error = Infallible; fn into_pyobject(self, _py: Python<'py>) -> Result { - Ok(&self.inner) + Ok(self.inner.as_borrowed()) } } From 223e5c80c7d5553e827e5c5d8d1bdf80c0250ba8 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 25 Aug 2024 08:03:46 +0100 Subject: [PATCH 2/2] delete newsfragment --- newsfragments/4487.changed.md | 1 - 1 file changed, 1 deletion(-) delete mode 100644 newsfragments/4487.changed.md diff --git a/newsfragments/4487.changed.md b/newsfragments/4487.changed.md deleted file mode 100644 index 0cc7a899cda..00000000000 --- a/newsfragments/4487.changed.md +++ /dev/null @@ -1 +0,0 @@ -Remove the `BoundObject` impl for `&Bound`, use `Borrowed` instead. For rationale see https://github.com/PyO3/pyo3/issues/4467. \ No newline at end of file