support Bound for classmethod and pass_module
#3831
Conversation
| @@ -179,11 +179,11 @@ error: macros cannot be used as items in `#[pymethods]` impl blocks | |||
| 197 | macro_invocation!(); | |||
| | ^^^^^^^^^^^^^^^^ | |||
|
|
|||
| error[E0277]: the trait bound `i32: From<&PyType>` is not satisfied | |||
| error[E0277]: the trait bound `i32: From<BoundRef<'_, '_, PyType>>` is not satisfied | |||
There was a problem hiding this comment.
The error message is notably worse than it was before, but I think once we remove the GIL Ref compatibility and go back to just &Bound<PyType> it improves. I can live with it being a bit janky for a couple of releases. If reviewers feel strongly that we should try to make this nicer I can experiment at cost of making the implementation more complex.
There was a problem hiding this comment.
(One upside of a more complex implementation is that I might be able to emit deprecation warnings for using GIL Refs in these positions.)
src/instance.rs
Outdated
| @@ -138,6 +138,14 @@ impl<'py> Bound<'py, PyAny> { | |||
| ) -> PyResult<Self> { | |||
| Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub(crate) unsafe fn from_ref_to_ptr<'a>( | |||
There was a problem hiding this comment.
I don't really get this name. How about from_borrowed_ptr?
Same on BoundRef
There was a problem hiding this comment.
Ugh, I couldn't work out a good name either. I didn't feel borrowed is quite right here because that's got a very specific meaning in the Python C API to refer to a pointer return value which doesn't carry ownership.
How about just from_raw?
There was a problem hiding this comment.
(I think this API will remain crate-private for the foreseeable future, so the name doesn't have to be perfect 😄)
There was a problem hiding this comment.
I agree, since this is internal its not super important. I'm also fine with from_raw. I think I also get now that you were intending to reference this (unusual) construct & *mut _, but yesterday i read it like it was turning a reference into a pointer, while it was doing kind of the opposite 🙈
There was a problem hiding this comment.
Ah, I see why the from_x_to_y bit reads wrong.
On reflection I think Bound::from_raw isn't quite the right name for this API. I still can't think of anything better. from_shared_ref_ptr? from_borrow_on_raw_pointer? from_ptr_shared_ref?
I'm kinda ok with a clumsy name here given this is an internal API really only intended to give a lifetime 'a in &'a Bound<'_, PyAny>.
I think of the options I wrote above, from_ptr_shared_ref might make the most sense to me. Any of those which you like?
There was a problem hiding this comment.
I don't think I have a particular preference among these. If i had to pick i would probably choose between from_shared_ref_ptr and from_ptr_shared_ref. So I'm fine with your preference. Another option that came to my mind was something like ref_from_ptr or shared_from_ptr, but I dont't feel strongly about these either.
There was a problem hiding this comment.
I quite like ref_from_ptr, let's go with that 👍
|
Thanks for the review! I decided to add a doc to |
CodSpeed Performance ReportMerging #3831 will degrade performances by 15.02%Comparing Summary
Benchmarks breakdown
|
This is a tidied-up copy of #3744 without any attempts to change
#[pymodule].This PR focuses on updating
#[classmethod]and#[pyfunction(pass_module)]to accept&Bound<PyType>and&Bound<PyModule>respectively. It allows them to accept GIL Refs for backwards compatibility.We were already using
Intofor these conversions, so I created a new helper typeBoundRef(&Bound<T>)which implements theIntoconversions for&T,&Bound<T>, andPy<T>. The reason for the new helper type was because I didn't want to supportFrom<&Bound<T>> for &T.