Added obj() getter method to PyUntypedBuffer#5870
Added obj() getter method to PyUntypedBuffer#5870
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an obj() accessor on PyUntypedBuffer to expose the owning Python object (backed by ffi::Py_buffer::obj) and includes a unit test validating the returned object identity/type.
Changes:
- Add
PyUntypedBuffer::obj(py) -> Bound<'py, PyAny>getter for the buffer owner. - Add
test_obj_getterto validateobj()returns the expected owner object.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/buffer.rs
Outdated
| pub fn obj<'py>(&self, py: Python<'py>) -> Bound<'py, PyAny> { | ||
| // Safety: `PyObject_GetBuffer` increments the reference count of `obj` automatically | ||
| // and `PyBuffer_Release` decrements it on drop. The `obj` is guaranteed to be valid | ||
| // and non-null for the entire lifetime of `self`. | ||
| unsafe { Bound::from_borrowed_ptr(py, self.raw().obj) } |
There was a problem hiding this comment.
obj() uses Bound::from_borrowed_ptr(py, self.raw().obj) but there’s no invariant in PyUntypedBuffer::get() that raw.obj is non-null. Since ffi::Py_buffer::obj is just a raw pointer (and is initialized as null in Py_buffer::new()), a misbehaving exporter could yield a null obj, making this safe method invoke UB. Consider either validating raw.obj is non-null when constructing the buffer (returning a PyBufferError if it is), or changing obj() to return Option<Bound<'py, PyAny>>/PyResult<...> and handling the null case explicitly.
| pub fn obj<'py>(&self, py: Python<'py>) -> Bound<'py, PyAny> { | |
| // Safety: `PyObject_GetBuffer` increments the reference count of `obj` automatically | |
| // and `PyBuffer_Release` decrements it on drop. The `obj` is guaranteed to be valid | |
| // and non-null for the entire lifetime of `self`. | |
| unsafe { Bound::from_borrowed_ptr(py, self.raw().obj) } | |
| pub fn obj<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | |
| let obj_ptr = self.raw().obj; | |
| if obj_ptr.is_null() { | |
| // A misbehaving exporter returned a null obj pointer, which would be UB to use. | |
| return Err(PyBufferError::new_err( | |
| "buffer exporter returned a null obj pointer in Py_buffer", | |
| )); | |
| } | |
| // Safety: `PyObject_GetBuffer` increments the reference count of `obj` automatically | |
| // and `PyBuffer_Release` decrements it on drop. We additionally checked that `obj_ptr` | |
| // is non-null before converting it to a `Bound`. | |
| unsafe { Ok(Bound::from_borrowed_ptr(py, obj_ptr)) } |
src/buffer.rs
Outdated
| let buf = PyUntypedBuffer::get(&bytes).unwrap(); | ||
| let owner = buf.obj(py); | ||
| assert!(owner.is_instance_of::<crate::types::PyBytes>()); | ||
| //owner and bytes should point to the same object | ||
| assert!(owner.is(&bytes)); |
There was a problem hiding this comment.
test_obj_getter only checks that obj() returns the same object, but it doesn’t cover the key behavior described in the docs/PR rationale: that you can keep the owning object alive after releasing the buffer (e.g., by cloning/unbinding the result of obj(), releasing/dropping the buffer, dropping the original bytes, and asserting the cloned reference still resolves). Adding an assertion around release() would protect against regressions in refcount handling.
| let buf = PyUntypedBuffer::get(&bytes).unwrap(); | |
| let owner = buf.obj(py); | |
| assert!(owner.is_instance_of::<crate::types::PyBytes>()); | |
| //owner and bytes should point to the same object | |
| assert!(owner.is(&bytes)); | |
| let mut buf = PyUntypedBuffer::get(&bytes).unwrap(); | |
| let owner = buf.obj(py); | |
| assert!(owner.is_instance_of::<crate::types::PyBytes>()); | |
| //owner and bytes should point to the same object | |
| assert!(owner.is(&bytes)); | |
| // Clone/unbind the owner so it can outlive the buffer and original bytes | |
| let owner_handle = owner.unbind(); | |
| // Explicitly release the buffer and drop it, then drop the original bytes | |
| buf.release(py); | |
| drop(buf); | |
| drop(bytes); | |
| // The unbound handle should still keep the owning object alive and usable | |
| let bound_owner = owner_handle.bind(py); | |
| assert!(bound_owner.is_instance_of::<crate::types::PyBytes>()); |
src/buffer.rs
Outdated
| #[test] | ||
| fn test_obj_getter() { | ||
| Python::attach(|py| { | ||
| let bytes = py.eval(ffi::c_str!("b'hello'"), None, None).unwrap(); |
There was a problem hiding this comment.
The new test constructs the bytes object via py.eval(...). For consistency with the other buffer tests in this module (which use PyBytes::new) and to avoid relying on Python code execution/parsing in a unit test, consider creating the bytes object directly with PyBytes::new(py, b"hello").
| let bytes = py.eval(ffi::c_str!("b'hello'"), None, None).unwrap(); | |
| let bytes = PyBytes::new(py, b"hello"); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| let ptr = self.raw().obj; | ||
| // SAFETY: Py_buffer.obj is a borrowed reference to a Python object, so it is always valid to create a Bound from it | ||
| if ptr.is_null() { | ||
| None | ||
| } else { | ||
| Some(unsafe { Bound::from_borrowed_ptr(py, ptr) }) | ||
| } |
There was a problem hiding this comment.
Can use Bound::from_borrowed_ptr_or_opt here to simplify the null checking.
| ///Returns the Python object that owns the buffer data. | ||
| /// | ||
| ///This is the object that was passed to [`PyBuffer::get()`] when the buffer was created. | ||
| /// Returns the Python object that owns the buffer data. | ||
| /// | ||
| /// This is the object passed to [`PyUntypedBuffer::get()`]. |
There was a problem hiding this comment.
Sorry, applying the copilot suggestion messed things up a bit.
| ///Returns the Python object that owns the buffer data. | |
| /// | |
| ///This is the object that was passed to [`PyBuffer::get()`] when the buffer was created. | |
| /// Returns the Python object that owns the buffer data. | |
| /// | |
| /// This is the object passed to [`PyUntypedBuffer::get()`]. | |
| /// Returns the Python object that owns the buffer data. | |
| /// | |
| /// This is the object that was passed to [`PyBuffer::get()`] | |
| /// when the buffer was created. |
Closes #5780
Adds an obj() method to PyUntypedBuffer to retreive the Python object which owns the buffer:
pub fn obj<'py>(&self, py: Python<'py>) -> Bound<'py, PyAny>
--this should be useful for inspecting the owning object or keeping a live reference to it after calling release() via .unbind()
Apologies if this duplicates work already in progress. I saw the issue was claimed but couldn't find an associated PR. Happy to close if this helps.