-
Notifications
You must be signed in to change notification settings - Fork 751
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
Added stable_deref_trait::StableDeref
implementation for PyRef
and PyRefMut
#4195
Conversation
…tableDeref` marker trait for `PyRef` and `PyRefMut`.
Added "newsfragment" entry for PR PyO3#4195
I'm inclined to not accept this PR. I don't want to encourage users to design their programs this way. In your example you can make the pyclass Also, both the |
I completely agree about the It is probably ok for a wrapper around I will note there is probably some similarity here with the ideas encapsulated in the |
Thanks for your time in reviewing; if you see the change as being more risk than it is worth to incorporate into pyo3, then I trust your judgment well above my own and can just keep using a newtype approach to utilize it in my own projects. That said, I am curious to dig further into your reasoning on some of those points. That is an interesting take about the I gave a very simple example for brevity, which could indeed be solved using And your point about keeping the |
I found this repo discussing some issues with This PR doesn't actually involve However, I still think there would be value in facilitating borrowing fields from pub struct PyRefMap<'p, T: PyClass, U: ?Sized> {
owner: PyRef<'p, T>,
reference: *const U,
}
impl<'py, T: PyClass> PyRef<'py, T> {
pub fn into_map<F, U: ?Sized>(self, f: F) -> PyRefMap<'py, T, U>
where F: FnOnce(&T) -> &U
{
PyRefMap {reference: f(&*self), owner: self}
}
}
impl<'p, T: PyClass, U: ?Sized> Deref for PyRefMap<'p, T, U> {
type Target = U;
fn deref(&self) -> &U {
// we own the `PyRef` that is guarding our (shared) access to `T`
unsafe { &*self.reference }
}
} This lets you convert your let bound = Bound::new(py, MyClass{data: [i32; 100]})?;
let data = bound.try_borrow()?.into_map(|c| &c.data); If you see any potential in adding functionality like this I could flesh it out into a new PR for further discussion, but otherwise I think it's fair to move onto more pressing issues. Thanks! |
We do a lot of this with |
The |
Thanks for linking that thread @davidhewitt, I read through it and just submitted a draft PR #4203 implementing a possible solution roughly based on my comment above. |
Introduction
This is a simple change that adds an implementation of the
stable_deref_trait::StableDeref
trait for thePyRef
andPyRefMut
RAII guards. Thisunsafe
trait is used by downstream crates as marker for types thatderef
to a stable address that is guaranteed to stay valid for the lifetime of the type (not just thederef
borrow), which should apply toPyRef(Mut)
. Implementing this trait enables some powerful synergies with the newBound
API, as discussed below.Motivation
I have been working a lot with the
Bound
API and theMyClassMethods
idiom, and I absolutely love it. Thepyclass
macro and related features already provided a powerful interface for using Rust from Python, but the new API has opened up a world of opportunities for using Python from Rust. However, there is one pain point that I have run into in my projects when trying to implement ergonomic Rust interfaces toPyClass
objects, and this pull request makes it possible to work around it.Consider the simple case where you want define an extension trait with a method that exposes a reference to a field of your
Bound
pyclass, as discussed in this section of the PyO3 user guide:This is clearly not possible, since the borrowed
PyRef
gets dropped when the function returns and we cannot return a reference to the data it guards. Of course, this is not unique toBound
/PyRef
; the same problem would arise if we tried to return a reference to data borrowed from aRefCell
which needs theRef
guard to stay alive.In simple cases like this you could find a way work around it, such as by making the
data
field public and have theBound<MyClass>
user explicitly borrow aPyRef
to access it, but this puts a significant limitation on the API's you can provide in more complex use cases.The
owning_ref
crate provides a clever solution to this limitation by packaging the the RAII guard alongside the reference (as*const T
) in anOwningRef
struct thatderef
s to the desired reference. By returning this struct instead of a bare&T
, you are effectively able to return a reference to the field while keeping thePyRef
guard alive to safely protect it:This now works! And the same process can be applied to expose
&mut T
by wrappingPyRefMuf
in anOwnedRefMut
.The only obstacle is that the
OwningRef
methods requireBound<MyClass>
to implementstable_deref_trait::StableDeref
(which it reexports asStableAddress
), which can only be done within thepyo3
crate thanks to the orphan rule (so I have had to use a newtype wrapper as a workaround). This PR hopes to fix that and make this ergonomic solution possible .Safety
Since
StableDeref
is anunsafe
trait, the safety of implementing it forPyRef
andPyRefMut
must be considered. The contract for implementing this trait requires that thePyRef(Mut)
will dereference to a stable address for the duration of its lifetime even when moved, and that the result of callingderef
will also stay alive for this duration (not only for the duration of the&self
lifetime of thederef
call).I am fairly certain that this condition is met for
PyRef
andPyRefMut
, which exist for the sole purpose of guarding access to the underlyingPy
pointer until they are dropped, but please double-check my reasoning in case there are any edge cases that I am missing. For reference, this trait is safely implemented for theRef
andRefMut
guards borrowed from aRefCell
, which are direct analogues toPyRef
andPyRefMut
.Considerations
stable_deref_trait
crate. This crate is mature (with 62 million downloads on crates.io), dual-licensed under MIT/Apache-2.0, and extremely lightweight (with no other dependencies), so I don't see a problem with adding the dependency. That said, l could put it behind a feature flag if you would prefer,unsafe
trait so the PR reviewer should careful consider whether the contract is met, and whether that could change in the future.impl
s to justify theunsafe
, but there may be better ways to word them. Or the comments could be removed if you think the justification is self-evident and they only add clutter; this seems to be how it was done for many of the otherunsafe impl
s.Thank you for your time in considering this PR! If you have any question or concerns I would be happy to iterate on a solution.