-
Notifications
You must be signed in to change notification settings - Fork 784
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 PyRefMap
and PyRefMapMut
for borrowing data nested within a PyRef
/PyRefMut
#4203
Draft
JRRudy1
wants to merge
7
commits into
PyO3:main
Choose a base branch
from
JRRudy1:pyref-map
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b76d919
Implemented first draft of `PyRefMap` and `PyRefMapMut`.
JRRudy1 c5ffcfa
Implemented second draft of `PyRefMap` and `PyRefMapMut`. These struc…
JRRudy1 ce80650
Implemented third draft of `PyRefMap` and `PyRefMapMut`. The `PyRefMa…
JRRudy1 f9308d4
Implemented fourth draft of `PyRefMap` and `PyRefMapMut`. Both types …
JRRudy1 323a1eb
Implemented fifth draft of `PyRefMap` and `PyRefMapMut`. The is simil…
JRRudy1 6c040f6
Cleanup and documentation.
JRRudy1 134a5d7
Merge branch 'PyO3:main' into pyref-map
JRRudy1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
|
||
use std::ptr::NonNull; | ||
use std::marker::PhantomData; | ||
use std::ops::{Deref, DerefMut}; | ||
|
||
use crate::pyclass::PyClass; | ||
use crate::pycell::{PyRef, PyRefMut}; | ||
use crate::pyclass::boolean_struct::{True, False, private::Boolean}; | ||
|
||
|
||
/// Represents a `PyRef` or `PyRefMut` with an opaque pyclass type. | ||
trait OpaquePyRef<'py>: 'py {} | ||
|
||
impl<'py, T: PyClass> OpaquePyRef<'py> for PyRef<'py, T> {} | ||
impl<'py, T: PyClass<Frozen=False>> OpaquePyRef<'py> for PyRefMut<'py, T> {} | ||
|
||
|
||
/// Base wrapper type for a [`PyRef<'py, T>`] or [`PyRefMut<'py, T>`] that | ||
/// dereferences to data of type `U` that is nested within a pyclass `T` | ||
/// instead of `T` itself. | ||
/// | ||
/// See the type aliases [`PyRefMap<'py, U>`] and [`PyRefMapMut<'py, U>`] | ||
/// for more information. | ||
pub struct PyRefMapBase<'py, U: 'py, Mut: Boolean> { | ||
// Either `PyRef` or `PyRefMut` guarding the opaque pyclass from which | ||
// the target is borrowed. | ||
owner: Box<dyn OpaquePyRef<'py>>, | ||
// Pointer to the `Deref` target which is (probably) borrowed from `owner`. | ||
// This pointer is derived from either `&U` or `&mut U` as indicated by | ||
// the `Mut` parameter. | ||
target: NonNull<U>, | ||
// Marks whether mutable methods are supported. If `Mut` is `True` then | ||
// `owner` is a `PyRefMut` and `target` was derived from `&mut U`, so | ||
// the pointer may be mutably dereferenced safely; if `False`, then | ||
// `owner` may be either `PyRef` or `PyRefMut` and `target` was derived | ||
// from `&U` so mutable dereferencing is forbidden. | ||
_mut: PhantomData<Mut> | ||
} | ||
|
||
/// A wrapper type for an _immutable_ reference to data of type `U` that is | ||
/// nested within a [`PyRef<'py, T>`] or [`PyRefMut<'py, T>`]. | ||
pub type PyRefMap<'py, U> = PyRefMapBase<'py, U, False>; | ||
|
||
/// A wrapper type for a _mutable_ reference to data of type `U` that is | ||
/// nested within a [`PyRefMut<'py, T>`]. | ||
pub type PyRefMapMut<'py, U> = PyRefMapBase<'py, U, True>; | ||
|
||
|
||
impl<'py, T: PyClass> PyRefMap<'py, T> { | ||
|
||
/// Construct a no-op `PyRefMap` that dereferences to the same | ||
/// value as the given [`PyRef`] or [`PyRefMut`]. | ||
pub fn new<R>(owner: R) -> PyRefMap<'py, T> | ||
where R: OpaquePyRef<'py> + Deref<Target=T>, | ||
{ | ||
let target = NonNull::from(&*owner); | ||
PyRefMap {target, owner: Box::new(owner), _mut: PhantomData} | ||
} | ||
} | ||
|
||
impl<'py, T: PyClass<Frozen = False>> PyRefMapMut<'py, T> { | ||
|
||
/// Construct a no-op `PyRefMapMut` that dereferences to the same | ||
/// value as the given [`PyRefMut`]. | ||
pub fn new(mut owner: PyRefMut<'py, T>) -> PyRefMapMut<'py, T> { | ||
let target = NonNull::from(&mut *owner); | ||
PyRefMapMut {target, owner: Box::new(owner), _mut: PhantomData} | ||
} | ||
} | ||
|
||
impl<'py, U: 'py, Mut: Boolean> PyRefMapBase<'py, U, Mut> { | ||
|
||
/// Applies the given function to the wrapped reference and wrap the | ||
/// return value in a new `PyRefMap`. | ||
pub fn map<F, V>(mut self, f: F) -> PyRefMap<'py, V> | ||
where F: FnOnce(&U) -> &V | ||
{ | ||
let target = NonNull::from(f(&*self)); | ||
PyRefMap {target, owner: self.owner, _mut: PhantomData} | ||
} | ||
} | ||
|
||
impl<'py, U: 'py> PyRefMapMut<'py, U> { | ||
|
||
/// Applies the given function to the wrapped mutable reference and | ||
/// wrap the return value in a new `PyRefMapMut`. | ||
pub fn map_mut<F, V>(mut self, f: F) -> PyRefMapMut<'py, V> | ||
where F: FnOnce(&mut U) -> &mut V | ||
{ | ||
let target = NonNull::from(f(&mut *self)); | ||
PyRefMapMut {target, owner: self.owner, _mut: PhantomData} | ||
} | ||
} | ||
|
||
// either flavor can safely implement `Deref` | ||
impl<'py, U: 'py, Mut: Boolean> Deref for PyRefMapBase<'py, U, Mut> { | ||
type Target = U; | ||
fn deref(&self) -> &U { | ||
// we own the `PyRef` or `PyRefMut` that is guarding our access to `T` | ||
unsafe { self.target.as_ref() } | ||
} | ||
} | ||
|
||
// only the `Mut=True` flavor can safely implement `DerefMut` | ||
impl<'py, U: 'py> DerefMut for PyRefMapMut<'py, U> { | ||
fn deref_mut(&mut self) -> &mut U { | ||
// we own the `PyRefMut` that is guarding our exclusive access to `T` | ||
unsafe { self.target.as_mut() } | ||
} | ||
} | ||
|
||
impl<'py, T: PyClass> PyRef<'py, T> { | ||
pub fn into_map<F, U: 'py>(self, f: F) -> PyRefMap<'py, U> | ||
where F: FnOnce(&T) -> &U | ||
{ | ||
PyRefMap::new(self).map(f) | ||
} | ||
} | ||
|
||
impl<'py, T: PyClass<Frozen = False>> PyRefMut<'py, T> { | ||
|
||
pub fn into_map<F, U: 'py>(self, f: F) -> PyRefMap<'py, U> | ||
where F: FnOnce(&T) -> &U | ||
{ | ||
PyRefMap::new(self).map(f) | ||
} | ||
|
||
pub fn into_map_mut<F, U: 'py>(self, f: F) -> PyRefMapMut<'py, U> | ||
where F: FnOnce(&mut T) -> &mut U | ||
{ | ||
PyRefMapMut::new(self).map_mut(f) | ||
} | ||
} | ||
|
||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use crate::prelude::*; | ||
use crate::types::PyString; | ||
|
||
#[pyclass] | ||
#[pyo3(crate = "crate")] | ||
pub struct MyClass { | ||
data: [i32; 100] | ||
} | ||
|
||
#[test] | ||
fn pyref_map() -> PyResult<()> { | ||
Python::with_gil(|py| -> PyResult<()> { | ||
let bound = Bound::new(py, MyClass{data: [0; 100]})?; | ||
let data = bound.try_borrow()?.into_map(|c| &c.data); | ||
assert_eq!(data[0], 0); | ||
Ok(()) | ||
}) | ||
} | ||
|
||
#[test] | ||
fn pyrefmut_map() -> PyResult<()> { | ||
Python::with_gil(|py| -> PyResult<()> { | ||
let bound = Bound::new(py, MyClass{data: [0; 100]})?; | ||
let data = bound.try_borrow_mut()?.into_map(|c| &c.data); | ||
assert_eq!(data[0], 0); | ||
Ok(()) | ||
}) | ||
} | ||
|
||
#[test] | ||
fn pyrefmut_map_mut() -> PyResult<()> { | ||
Python::with_gil(|py| -> PyResult<()> { | ||
let bound = Bound::new(py, MyClass{data: [0; 100]})?; | ||
let mut data = bound | ||
.try_borrow_mut()? | ||
.into_map_mut(|c| &mut c.data); | ||
data[0] = 5; | ||
assert_eq!(data[0], 5); | ||
Ok(()) | ||
}) | ||
} | ||
|
||
#[test] | ||
fn pyref_map_unrelated() -> PyResult<()> { | ||
Python::with_gil(|py| -> PyResult<()> { | ||
let bound = Bound::new(py, MyClass{data: [0; 100]})?; | ||
let string = PyString::new_bound(py, "pyo3"); | ||
// there is nothing stopping the user from returning something not | ||
// borrowing from the pyref, but that shouldn't matter. The borrow | ||
// checker still enforces the `'py` lifetime | ||
let refmap = bound.try_borrow()?.into_map(|_| &string); | ||
assert_eq!(refmap.to_str()?, "pyo3"); | ||
Ok(()) | ||
}) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this could be a lot simpler, something like:
similar for a mutable variant. I haven't tried if that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I'll play around with it a bit to see if it can be simplified.
However, I'm not sure that your method would work unless I am missing something. My motivation for storing the original
PyRef
/PyRefMut
within thePyRefMap
struct was to hold the borrow on theT: PyClass
that theNonNull<U>
is pointing into so that the pointer is guaranteed to be safely dereferenceable. If I allow thePyRef
to be dropped, itsDrop
impl will callrelease_borrow
and stop protectingT
from being mutably aliased. The innerBound<T>
would also get dropped, which would decrement the reference count and potentially letT
get garbage collected. The next timePyRefMap
get's dereferenced would then be UB.So to prevent all this I would have to do something messy, perhaps
ManuallyDrop
thePyRef
and then add aDrop
impl forPyRefMap
that releases the borrow; and then I'd still have to find a way to decrement theT
's reference count, which wouldn't be possible if I only had access to&dyn PyClassBorrowChecker
. I'm also not sure how I would even get a reference to the borrow checker with the'py
lifetime; it seems like the only way to get it is by borrowing it from a&'a Bound<'py, T>
which could only give you&'a dyn PyClassBorrowChecker
. But even if there is a way to get around all this, it seems a lot more complicated/unsafe to me than just storing the originalPyRef
and letting it handle all the protection and cleanup with its existing mechanisms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. I was just sketching out an approach. I would really prefer to not allocate in
PyRefMap::new
, this is another approach to erase the pyclass type without boxing things.A solution would be to refactor that into its own struct:
Another solution is your simplified solution that stores the PyRef inline but does not erase the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree allocating wouldn't be ideal, and I had pretty mixed feeling about taking that approach.
I took you up on this and was able to simplify
PyRef
/PyRefMut
quite a bit without breaking anything. I will submit a PR for it soon to see what you guys think.I personally do prefer storing the
PyRef
inline, and keeping the type un-erased is probably no big deal. Users can just implement their own wrapper if they want to erase the type, so I agree that it isn't worth allocating a box internally.I have a new implementation you may like based on the reworks to
PyRef
/PyRefMut
that I'll be proposing soon, so let's circle back after that.