From 5498ce7fe6eb80ad888c30e0cb66cf2fece11e8c Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 17 Feb 2024 23:32:10 +0000 Subject: [PATCH 1/4] make `Borrowed` ptr constructors public --- src/instance.rs | 79 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/src/instance.rs b/src/instance.rs index 2db63d06a43..09caf31269c 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -480,33 +480,43 @@ impl<'py, T> Borrowed<'_, 'py, T> { } impl<'a, 'py> Borrowed<'a, 'py, PyAny> { + /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Panics if `ptr` is null. + /// /// # Safety - /// This is similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by - /// the caller and it's the caller's responsibility to ensure that the reference this is - /// derived from is valid for the lifetime `'a`. - pub(crate) unsafe fn from_ptr_or_err( - py: Python<'py>, - ptr: *mut ffi::PyObject, - ) -> PyResult { + /// + /// - `ptr` must be a valid pointer to a Python object + /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by + /// the caller and it is the caller's responsibility to ensure that the reference this is + /// derived from is valid for the lifetime `'a`. + pub unsafe fn from_ptr_or_err(py: Python<'py>, ptr: *mut ffi::PyObject) -> PyResult { NonNull::new(ptr).map_or_else( || Err(PyErr::fetch(py)), |ptr| Ok(Self(ptr, PhantomData, py)), ) } + /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. + /// /// # Safety - /// This is similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by - /// the caller and it's the caller's responsibility to ensure that the reference this is - /// derived from is valid for the lifetime `'a`. - pub(crate) unsafe fn from_ptr_or_opt(py: Python<'py>, ptr: *mut ffi::PyObject) -> Option { + /// + /// - `ptr` must be a valid pointer to a Python object, or null + /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by + /// the caller and it is the caller's responsibility to ensure that the reference this is + /// derived from is valid for the lifetime `'a`. + pub unsafe fn from_ptr_or_opt(py: Python<'py>, ptr: *mut ffi::PyObject) -> Option { NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData, py)) } + /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch` + /// if `ptr` is null. + /// /// # Safety - /// This is similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by - /// the caller and it's the caller's responsibility to ensure that the reference this is - /// derived from is valid for the lifetime `'a`. - pub(crate) unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + /// + /// - `ptr` must be a valid pointer to a Python object, or null + /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by + /// the caller and it is the caller's responsibility to ensure that the reference this is + /// derived from is valid for the lifetime `'a`. + pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { Self( NonNull::new(ptr).unwrap_or_else(|| crate::err::panic_after_error(py)), PhantomData, @@ -1762,8 +1772,9 @@ impl PyObject { #[cfg_attr(not(feature = "gil-refs"), allow(deprecated))] mod tests { use super::{Bound, Py, PyObject}; + use crate::types::PyCapsule; use crate::types::{dict::IntoPyDict, PyDict, PyString}; - use crate::{PyAny, PyNativeType, PyResult, Python, ToPyObject}; + use crate::{ffi, Borrowed, PyAny, PyNativeType, PyResult, Python, ToPyObject}; #[test] fn test_call() { @@ -1962,6 +1973,42 @@ a = A() }); } + #[test] + fn borrowed_ptr_constructors() { + // More detailed tests of the underlying semantics in pycell.rs + Python::with_gil(|py| { + fn check_drop<'py>( + py: Python<'py>, + method: impl FnOnce(&*mut ffi::PyObject) -> Borrowed<'_, 'py, PyAny>, + ) { + let mut dropped = false; + let capsule = PyCapsule::new_bound_with_destructor( + py, + (&mut dropped) as *mut _ as usize, + None, + |ptr, _| unsafe { std::ptr::write(ptr as *mut bool, true) }, + ) + .unwrap(); + + let ptr = &capsule.as_ptr(); + let _borrowed = method(ptr); + assert_eq!(dropped, false); + + // creating the borrow should not have increased the refcount + drop(capsule); + assert_eq!(dropped, true); + } + + check_drop(py, |&ptr| unsafe { Borrowed::from_ptr(py, ptr) }); + check_drop(py, |&ptr| unsafe { + Borrowed::from_ptr_or_opt(py, ptr).unwrap() + }); + check_drop(py, |&ptr| unsafe { + Borrowed::from_ptr_or_err(py, ptr).unwrap() + }); + }) + } + #[cfg(feature = "macros")] mod using_macros { use crate::PyCell; From c8b0b6e0111fdd41b8380531ef9f30a2f275ca95 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 17 Feb 2024 23:54:00 +0000 Subject: [PATCH 2/4] introduce `Bound::from_borrowed_ptr` constructors --- src/instance.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/src/instance.rs b/src/instance.rs index 09caf31269c..1e6d3bb6fd5 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -115,7 +115,7 @@ impl<'py> Bound<'py, PyAny> { Self(py, ManuallyDrop::new(Py::from_owned_ptr(py, ptr))) } - /// Constructs a new `Bound<'py, PyAny>` from a pointer. Returns `None`` if `ptr` is null. + /// Constructs a new `Bound<'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. /// /// # Safety /// @@ -139,6 +139,42 @@ impl<'py> Bound<'py, PyAny> { Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) } + /// Constructs a new `Bound<'py, PyAny>` from a pointer by creating a new Python reference. + /// Panics if `ptr` is null. + /// + /// # Safety + /// + /// - `ptr` must be a valid pointer to a Python object + pub unsafe fn from_borrowed_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + Self(py, ManuallyDrop::new(Py::from_borrowed_ptr(py, ptr))) + } + + /// Constructs a new `Bound<'py, PyAny>` from a pointer by creating a new Python reference. + /// Returns `None` if `ptr` is null. + /// + /// # Safety + /// + /// - `ptr` must be a valid pointer to a Python object, or null + pub unsafe fn from_borrowed_ptr_or_opt( + py: Python<'py>, + ptr: *mut ffi::PyObject, + ) -> Option { + Py::from_borrowed_ptr_or_opt(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) + } + + /// Constructs a new `Bound<'py, PyAny>` from a pointer by creating a new Python reference. + /// Returns an `Err` by calling `PyErr::fetch` if `ptr` is null. + /// + /// # Safety + /// + /// - `ptr` must be a valid pointer to a Python object, or null + pub unsafe fn from_borrowed_ptr_or_err( + py: Python<'py>, + ptr: *mut ffi::PyObject, + ) -> PyResult { + Py::from_borrowed_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj))) + } + /// This slightly strange method is used to obtain `&Bound` from a pointer in macro code /// where we need to constrain the lifetime `'a` safely. /// @@ -482,6 +518,9 @@ impl<'py, T> Borrowed<'_, 'py, T> { impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Panics if `ptr` is null. /// + /// Prefer to use [`Bound::from_borrowed_ptr_or_err`], as that avoids the major safety risk + /// of needing to precisely define the lifetime `'a` for which the borrow is valid. + /// /// # Safety /// /// - `ptr` must be a valid pointer to a Python object @@ -497,6 +536,9 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. /// + /// Prefer to use [`Bound::from_borrowed_ptr_or_opt`], as that avoids the major safety risk + /// of needing to precisely define the lifetime `'a` for which the borrow is valid. + /// /// # Safety /// /// - `ptr` must be a valid pointer to a Python object, or null @@ -510,6 +552,9 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch` /// if `ptr` is null. /// + /// Prefer to use [`Bound::from_borrowed_ptr`], as that avoids the major safety risk + /// of needing to precisely define the lifetime `'a` for which the borrow is valid. + /// /// # Safety /// /// - `ptr` must be a valid pointer to a Python object, or null @@ -1973,6 +2018,45 @@ a = A() }); } + #[test] + fn bound_from_borrowed_ptr_constructors() { + // More detailed tests of the underlying semantics in pycell.rs + Python::with_gil(|py| { + fn check_drop<'py>( + py: Python<'py>, + method: impl FnOnce(*mut ffi::PyObject) -> Bound<'py, PyAny>, + ) { + let mut dropped = false; + let capsule = PyCapsule::new_bound_with_destructor( + py, + (&mut dropped) as *mut _ as usize, + None, + |ptr, _| unsafe { std::ptr::write(ptr as *mut bool, true) }, + ) + .unwrap(); + + let bound = method(capsule.as_ptr()); + assert_eq!(dropped, false); + + // creating the bound should have increased the refcount + drop(capsule); + assert_eq!(dropped, false); + + // dropping the bound should now also decrease the refcount and free the object + drop(bound); + assert_eq!(dropped, true); + } + + check_drop(py, |ptr| unsafe { Bound::from_borrowed_ptr(py, ptr) }); + check_drop(py, |ptr| unsafe { + Bound::from_borrowed_ptr_or_opt(py, ptr).unwrap() + }); + check_drop(py, |ptr| unsafe { + Bound::from_borrowed_ptr_or_err(py, ptr).unwrap() + }); + }) + } + #[test] fn borrowed_ptr_constructors() { // More detailed tests of the underlying semantics in pycell.rs From 04bf15442330ef377607e914e5af5778f75b4e0f Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 18 Feb 2024 01:03:05 +0000 Subject: [PATCH 3/4] clippy `assert_eq` -> `assert` --- src/instance.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/instance.rs b/src/instance.rs index 1e6d3bb6fd5..354e3ce0f67 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2036,15 +2036,15 @@ a = A() .unwrap(); let bound = method(capsule.as_ptr()); - assert_eq!(dropped, false); + assert!(!dropped); // creating the bound should have increased the refcount drop(capsule); - assert_eq!(dropped, false); + assert!(!dropped); // dropping the bound should now also decrease the refcount and free the object drop(bound); - assert_eq!(dropped, true); + assert!(dropped); } check_drop(py, |ptr| unsafe { Bound::from_borrowed_ptr(py, ptr) }); @@ -2076,11 +2076,11 @@ a = A() let ptr = &capsule.as_ptr(); let _borrowed = method(ptr); - assert_eq!(dropped, false); + assert!(!dropped); // creating the borrow should not have increased the refcount drop(capsule); - assert_eq!(dropped, true); + assert!(dropped); } check_drop(py, |&ptr| unsafe { Borrowed::from_ptr(py, ptr) }); From bc53537d5e4ed54160a845954e35a4698a3bd04c Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 18 Feb 2024 21:40:40 +0000 Subject: [PATCH 4/4] rerrange function order and correct docstrings --- src/instance.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/instance.rs b/src/instance.rs index 354e3ce0f67..336cd018afc 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -516,9 +516,9 @@ impl<'py, T> Borrowed<'_, 'py, T> { } impl<'a, 'py> Borrowed<'a, 'py, PyAny> { - /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Panics if `ptr` is null. + /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Panics if `ptr` is null. /// - /// Prefer to use [`Bound::from_borrowed_ptr_or_err`], as that avoids the major safety risk + /// Prefer to use [`Bound::from_borrowed_ptr`], as that avoids the major safety risk /// of needing to precisely define the lifetime `'a` for which the borrow is valid. /// /// # Safety @@ -527,14 +527,15 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by /// the caller and it is the caller's responsibility to ensure that the reference this is /// derived from is valid for the lifetime `'a`. - pub unsafe fn from_ptr_or_err(py: Python<'py>, ptr: *mut ffi::PyObject) -> PyResult { - NonNull::new(ptr).map_or_else( - || Err(PyErr::fetch(py)), - |ptr| Ok(Self(ptr, PhantomData, py)), + pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { + Self( + NonNull::new(ptr).unwrap_or_else(|| crate::err::panic_after_error(py)), + PhantomData, + py, ) } - /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. + /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Returns `None` if `ptr` is null. /// /// Prefer to use [`Bound::from_borrowed_ptr_or_opt`], as that avoids the major safety risk /// of needing to precisely define the lifetime `'a` for which the borrow is valid. @@ -549,10 +550,10 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData, py)) } - /// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch` + /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch` /// if `ptr` is null. /// - /// Prefer to use [`Bound::from_borrowed_ptr`], as that avoids the major safety risk + /// Prefer to use [`Bound::from_borrowed_ptr_or_err`], as that avoids the major safety risk /// of needing to precisely define the lifetime `'a` for which the borrow is valid. /// /// # Safety @@ -561,11 +562,10 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> { /// - similar to `std::slice::from_raw_parts`, the lifetime `'a` is completely defined by /// the caller and it is the caller's responsibility to ensure that the reference this is /// derived from is valid for the lifetime `'a`. - pub unsafe fn from_ptr(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self { - Self( - NonNull::new(ptr).unwrap_or_else(|| crate::err::panic_after_error(py)), - PhantomData, - py, + pub unsafe fn from_ptr_or_err(py: Python<'py>, ptr: *mut ffi::PyObject) -> PyResult { + NonNull::new(ptr).map_or_else( + || Err(PyErr::fetch(py)), + |ptr| Ok(Self(ptr, PhantomData, py)), ) }