Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement Py2Borrowed #3654

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/ffi_ptr_ext.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use crate::{ffi, instance::Py2, PyAny, PyResult, Python};
use crate::{
ffi,
instance::{Py2, Py2Borrowed},
PyAny, PyResult, Python,
};

mod sealed {
use super::*;
Expand All @@ -13,6 +17,24 @@ use sealed::Sealed;
pub(crate) trait FfiPtrExt: Sealed {
unsafe fn assume_owned_or_err(self, py: Python<'_>) -> PyResult<Py2<'_, PyAny>>;
unsafe fn assume_owned(self, py: Python<'_>) -> Py2<'_, PyAny>;

/// Assumes this pointer is borrowed from a parent object.
///
/// Warning: the lifetime `'a` is not bounded by the function arguments; the caller is
/// responsible to ensure this is tied to some appropriate lifetime.
unsafe fn assume_borrowed_or_err<'a, 'py>(
self,
py: Python<'py>,
) -> PyResult<Py2Borrowed<'a, 'py, PyAny>>;

/// Same as `assume_borrowed_or_err`, but doesn't fetch an error on NULL.
unsafe fn assume_borrowed_or_opt<'a, 'py>(
self,
py: Python<'py>,
) -> Option<Py2Borrowed<'a, 'py, PyAny>>;

/// Same as `assume_borrowed_or_err`, but panics on NULL.
unsafe fn assume_borrowed<'a, 'py>(self, py: Python<'py>) -> Py2Borrowed<'a, 'py, PyAny>;
}

impl FfiPtrExt for *mut ffi::PyObject {
Expand All @@ -25,4 +47,25 @@ impl FfiPtrExt for *mut ffi::PyObject {
unsafe fn assume_owned(self, py: Python<'_>) -> Py2<'_, PyAny> {
Py2::from_owned_ptr(py, self)
}

#[inline]
unsafe fn assume_borrowed_or_err<'a, 'py>(
self,
py: Python<'py>,
) -> PyResult<Py2Borrowed<'a, 'py, PyAny>> {
Py2Borrowed::from_ptr_or_err(py, self)
}

#[inline]
unsafe fn assume_borrowed_or_opt<'a, 'py>(
self,
py: Python<'py>,
) -> Option<Py2Borrowed<'a, 'py, PyAny>> {
Py2Borrowed::from_ptr_or_opt(py, self)
}

#[inline]
unsafe fn assume_borrowed<'a, 'py>(self, py: Python<'py>) -> Py2Borrowed<'a, 'py, PyAny> {
Py2Borrowed::from_ptr(py, self)
}
}
128 changes: 128 additions & 0 deletions src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,17 @@ impl<'py> Py2<'py, PyAny> {
) -> PyResult<Self> {
Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj)))
}

/// Constructs a new Py2 from a pointer. Does not check for null.
pub(crate) unsafe fn from_owned_ptr_unchecked(
py: Python<'py>,
ptr: *mut ffi::PyObject,
) -> Self {
Self(
py,
ManuallyDrop::new(Py(NonNull::new_unchecked(ptr), PhantomData)),
)
}
}

impl<'py, T> Py2<'py, T> {
Expand Down Expand Up @@ -214,6 +225,123 @@ unsafe impl<T> AsPyPointer for Py2<'_, T> {
}
}

/// A borrowed equivalent to `Py2`.
Copy link
Member

@adamreichold adamreichold Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just give up and call this Py3. ;-) But more seriously, I think we should get rid off Py2 sooner or later and commit to something reasonable. By now the pain of reading Py2 has made my mind pliable and I will probably accept almost anything even remotely sane.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, maybe I will open a PR to rotate Py<T> -> PyDetached<T> (or PySend<T>) and Py2<'py, T> -> Py<'py, T>. 🙈

///
/// The advantage of this over `&Py2` is that it avoids the need to have a pointer-to-pointer, as Py2
/// is already a pointer to a PyObject.
#[repr(transparent)]
pub(crate) struct Py2Borrowed<'a, 'py, T>(NonNull<ffi::PyObject>, PhantomData<&'a Py2<'py, T>>); // TODO is it useful to have the generic form?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what is the verdict on the TODO? Is there already a use case for that? Otherwise, I'd suggest leaving it non-generic for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #3651 might be the first use case, if we do actually think the thing I just pushed there has value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, but then I'd suggest removing the TODO comment now (as the whole PR is in anticipation of future uses).


impl<'a, 'py> Py2Borrowed<'a, 'py, PyAny> {
Copy link
Member

@adamreichold adamreichold Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should at least this block or even the struct definition itself have a bound ensuring that 'py outlives 'a to avoid all too obvious lifetime errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an edge case; isn't this a valid construction where 'a outlives 'py:

let any: Py<PyAny> = /* ... */;
let any = &any;  // start borrow 'a
Python::with_gil(|py| Py2Borrowed::from(any.attach(py)));  // 'py shorter than 'a

Maybe in practice this is never useful though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case, the borrow of any could always be shortened to at most 'py by reborrowing. So if the bound does not prevent the above but prevents some illegal uses, I would prefer to have it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it turns out that exactly in the Py<PyBytes>::as_bytes case we're looking at in #3651 it's the case that we want 'a to outlive 'py: d6f18b5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't attach_borrow sort of require the immutability guarantee of Py<PyBytes>::as_bytes now to avoid unsafely extending references into e.g. mutable pyclasses beyond the duration for which the GIL is held? Wouldn't this imply that attach_borrow needs to be unsafe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I am also sort of confused now whether we discuss #3651 or #3654. Maybe these should be folded into #3651 with Py2Borrowed just being a separate commit?)

/// # Safety
/// 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_or_err(
py: Python<'py>,
ptr: *mut ffi::PyObject,
) -> PyResult<Self> {
NonNull::new(ptr).map_or_else(|| Err(PyErr::fetch(py)), |ptr| Ok(Self(ptr, PhantomData)))
}

/// # Safety
/// 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_or_opt(
_py: Python<'py>,
ptr: *mut ffi::PyObject,
) -> Option<Self> {
NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData))
}

/// # Safety
/// 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(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self {
Self(
NonNull::new(ptr).unwrap_or_else(|| crate::err::panic_after_error(py)),
PhantomData,
)
}
}

impl<'a, 'py, T> From<&'a Py2<'py, T>> for Py2Borrowed<'a, 'py, PyAny> {
/// Create borrow on a Py2
fn from(instance: &'a Py2<'py, T>) -> Self {
Self(
unsafe { NonNull::new_unchecked(instance.as_ptr()) },
PhantomData,
)
}
}

impl<'py, T> Py2Borrowed<'_, 'py, T> {
pub(crate) fn clone_ref(self) -> Py2<'py, T> {
unsafe {
Py2::from_owned_ptr_unchecked(self.py(), ffi::_Py_NewRef(self.as_ptr()))
.downcast_into_unchecked()
}
}
}

impl<'py, T> Py2Borrowed<'py, 'py, T>
where
T: HasPyGilRef,
{
pub(crate) fn from_gil_ref(gil_ref: &'py T::AsRefTarget) -> Self {
// Safety: &'py T::AsRefTarget is expected to be a Python pointer,
// so &'py T::AsRefTarget has the same layout as Self.
unsafe { std::mem::transmute(gil_ref) }
}

pub(crate) fn into_gil_ref(self) -> &'py T::AsRefTarget {
// Safety: self is a borrow over `'py`.
unsafe { self.py().from_borrowed_ptr(self.0.as_ptr()) }
}
}

impl<T> Clone for Py2Borrowed<'_, '_, T> {
fn clone(&self) -> Self {
*self
}
}

impl<T> Copy for Py2Borrowed<'_, '_, T> {}

impl<T> std::fmt::Debug for Py2Borrowed<'_, '_, T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Py2::fmt(self, f)
}
}

impl<'py, T> Deref for Py2Borrowed<'_, 'py, T> {
type Target = Py2<'py, T>;

#[inline]
fn deref(&self) -> &Py2<'py, T> {
// safety: Py2 has the same layout as NonNull<ffi::PyObject>
unsafe { &*(&self.0 as *const _ as *const Py2<'py, T>) }
}
}
Comment on lines +319 to +327
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main frustration I found with using this type is the deref here forces us to reduce the lifetime; it's not the original 'awhich thePy2Borrowed` was created with, but a reduced lifetime which is the duration of the deref.

I think this generally only matters in interactions with the gil-ref APIs, where the reduced lifetime means that this type isn't a solution to #3651 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would almost say this is make-or-break or are the performance improvements for argument extraction themselves sufficient to motivate the type? Meaning if this does not help to solve that problem, I would suggest avoiding the additional complexity for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it, the only think really working would be to implement all Methods traits directly on Py2Borrowed and make the implementations of these traits on Py2 forward to those on Py2Borrowed?

Since these would not be user-visible, I could live with that from an API perspective. But the amount of internal boiler plate would be staggering...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think another alternative is to make Py2 generic over ownership, something like

struct Py2<'py, T, O: Ownership>(Python<'py>, ManuallyDrop<Py<T>>, PhantomData<O>);

trait Ownership { /* I think this just needs to describe how to clone and drop the reference */ } 
struct Owned;
struct Borrowed<'a>(PhantomData<&'a Py<PyAny>>);

// implement `Ownership` for each of `Owned` and `Borrowed`

In which case we can then make all the Methods traits generic over O: Ownership, and not need to have a ton of internal boilerplate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like pest versus cholera: Internal trait level complexity which might be short but needs to be "expanded in one's head" versus internal boiler plate which is trivial but needs to be read to "sift from the syntax noise".

If the lifetimes can work without this, I would prefer not generalizing the method traits and have only the Py2-based trait implementations.


impl<T> ToPyObject for Py2Borrowed<'_, '_, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be reachable via the Deref impl or are these used in generic context somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they were only used in extract_argument.rs and I can probably work around them not existing, so can remove for now.

/// Converts `Py` instance -> PyObject.
fn to_object(&self, py: Python<'_>) -> PyObject {
unsafe { PyObject::from_borrowed_ptr(py, self.as_ptr()) }
}
}

impl<T> IntoPy<PyObject> for Py2Borrowed<'_, '_, T> {
/// Converts a `Py` instance to `PyObject`.
/// Consumes `self` without calling `Py_DECREF()`.
#[inline]
fn into_py(self, py: Python<'_>) -> PyObject {
self.to_object(py)
}
}

/// A GIL-independent reference to an object allocated on the Python heap.
///
/// This type does not auto-dereference to the inner object because you must prove you hold the GIL to access it.
Expand Down