-
Notifications
You must be signed in to change notification settings - Fork 729
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
Allow passing non-mutable references to self in PyIterProtocol #856
Conversation
Thank you!
Sounds nice to me 💯
I'll check this tomorrow. |
Can we use |
+1 for For the unwrap error difficulty - I have been working on some changes in #797 to make error handling easier in these callback functions. Maybe this can help you. |
👍 |
Update: I rebased against latest master and renamed the associated types to I added a trait Also, not related to the PR so I didn't include that, but I think |
Hmm yes I was thinking if we could achieve this with generic associated types; but they're unstable anyway. It would be interesting to revisit this problem once we've removed specialization from the protocol implementations.
+1 for this. There's a number of improvements I think needs to come to error handling in pyo3. |
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.
Thank, almost LGTM 👍
Could you please add a test for mutable iterator (fn __iter__(slf: mut PyRefMut<Self>)
) ?
Then this PR would be perfect.
@@ -38,7 +38,9 @@ macro_rules! py_unary_refmut_func { | |||
let py = pool.python(); | |||
$crate::run_callback(py, || { | |||
let slf = py.from_borrowed_ptr::<$crate::PyCell<T>>(slf); | |||
let res = $class::$f(slf.borrow_mut()).into(); | |||
let borrow = <T::Receiver>::try_from_pycell(slf) |
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 guess just try_from_pycell(slf)?
work?
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.
Without the associated <T::Receiver>
, the compiler complains about the next line and can't guess the type of e
in the map_err
method, so I prefer to leave it like that.
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.
Ah, OK 👍
Thank you! |
Current implementation of
PyIterProtocol
methods requires a mutable borrow, but it's not needed for all implementations (exemple in #854).This PR adds the code to make
PyIterProtocol
accept eitherPyRef
orPyRefMut
as its referential argument, by doing the following:UnarySlf
method to the derive backend, which serves to extract the rightSelf
type from the method signatureSlf
associated type toPyIterIterProtocol
andPyIterNextProtocol
UnarySlf
method wrapper useTryFrom<&PyCell>
to be generic about the self-reference type ➡️ this has the shitty side-effect of preventing unwrapping the error, since TryFrom::Error are not constrained to implementDebug
, and I couldn't get the type checker to accept a constraing forDebug
on<T::Slf as TryFrom<&PyCell>>::Error
because of lifetime issuesI also think this should be ported to other protocol: for instance
PyMappingProtocol.__getitem__
may need a mutable reference (imagine a mapping maintaining a LRU cache) whilePyContextProtocol
may not need mutable references in__exit__
.Maintenance
cargo fmt
✅cargo clippy
and check there are no hard errors (There are a bunch of existing warnings; This is also checked by travis)black .
. You can install black withpip install black
)