deprecate from_borrowed_ptr methods
#3915
Conversation
guide/src/class.md
Outdated
| fn set(mut self_: PyRefMut<'_, Self>, key: String, value: &PyAny) -> PyResult<()> { | ||
| self_.counter.entry(key.clone()).or_insert(0); | ||
| let py = self_.py(); | ||
| # #[allow(deprecated)] | ||
| let dict: &PyDict = unsafe { py.from_borrowed_ptr_or_err(self_.as_ptr())? }; | ||
| dict.set_item(key, value) | ||
| } |
There was a problem hiding this comment.
I updates this example in #3914
| not(feature = "gil-refs"), | ||
| deprecated( | ||
| since = "0.21.0", | ||
| note = "use `Py::from_borrowed_ptr(py, ptr)` or `Bound::from_borrowed_ptr(py, ptr)` instead" |
There was a problem hiding this comment.
I think technically the equivalent method would be on Borrowed, but I thought we might want to nudge user here to the safer option.
There was a problem hiding this comment.
Agreed I think Borrowed::from_ptr is hard to get right, as we keep finding by not using it in our own macro code!
pyo3-macros-backend/src/method.rs
Outdated
| @@ -513,15 +513,15 @@ impl<'a> FnSpec<'a> { | |||
| holders.pop().unwrap(); // does not actually use holder created by `self_arg` | |||
|
|
|||
| quote! {{ | |||
| let __guard = _pyo3::impl_::coroutine::RefGuard::<#cls>::new(py.from_borrowed_ptr::<_pyo3::types::PyAny>(_slf))?; | |||
| let __guard = _pyo3::impl_::coroutine::RefGuard::<#cls>::new(_pyo3::Bound::from_borrowed_ptr(py, _slf))?; | |||
There was a problem hiding this comment.
Today I had the thought that we could also use BoundRef here, to defer the ref count bump until after the downcast check in RefGuard::new. This would prevent touching the ref count in the error case.
src/impl_/coroutine.rs
Outdated
| let owned = obj.downcast::<T>()?; | ||
| mem::forget(owned.try_borrow()?); |
There was a problem hiding this comment.
Hmm, now that I see these men::forget calls I suspect that the changes I made to make PyRef and PyRefMut contain Bound mean that we're leaking a reference count here.
Probably not related to this PR. I'll pause reviewing here just so I remember to reread this in the morning.
| not(feature = "gil-refs"), | ||
| deprecated( | ||
| since = "0.21.0", | ||
| note = "use `Py::from_borrowed_ptr(py, ptr)` or `Bound::from_borrowed_ptr(py, ptr)` instead" |
There was a problem hiding this comment.
Agreed I think Borrowed::from_ptr is hard to get right, as we keep finding by not using it in our own macro code!
|
Ah, there's a merge conflict now :) |
Icxolu
force-pushed
the
deprecate-from-borrowed-ptrs
branch
from
3c532d1 to
f315b7d
Compare
|
Rebased 😇 Edit: Seems like some autoformatter messed with the markdown on resolving the conflict, I'll remove that |
This deprecates the methods on the `Python` marker, aswell as `FromPyPointer`
Icxolu
force-pushed
the
deprecate-from-borrowed-ptrs
branch
from
f315b7d to
6ff042b
Compare
Part of #3684
This deprecates the remaining
from_borrowed_ptrmethods onPythonandFromPyPointer, as well as that trait itself (since all methods are deprecated now)