add Bound API constructors from borrowed pointers (#3858)

* make `Borrowed` ptr constructors public

* introduce `Bound::from_borrowed_ptr` constructors

* clippy `assert_eq` -> `assert`

* rerrange function order and correct docstrings

src/instance.rs

@@ -114,7 +114,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
     ///
@@ -138,6 +138,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<Self> {
+        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<Self> {
+        Py::from_borrowed_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj)))
+    }
+
     /// This slightly strange method is used to obtain `&Bound<PyAny>` from a pointer in macro code
     /// where we need to constrain the lifetime `'a` safely.
     ///
@@ -479,33 +515,18 @@ impl<'py, T> Borrowed<'_, 'py, T> {
 }
 
 impl<'a, 'py> Borrowed<'a, 'py, PyAny> {
+    /// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Panics 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
-    /// 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<Self> {
-        NonNull::new(ptr).map_or_else(
-            || Err(PyErr::fetch(py)),
-            |ptr| Ok(Self(ptr, PhantomData, py)),
-        )
-    }
-
-    /// # 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<Self> {
-        NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData, py))
-    }
-
-    /// # 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
+    /// - 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,
@@ -513,6 +534,40 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> {
         )
     }
 
+    /// 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.
+    ///
+    /// # Safety
+    ///
+    /// - `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<Self> {
+        NonNull::new(ptr).map(|ptr| Self(ptr, PhantomData, py))
+    }
+
+    /// 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_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, 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_err(py: Python<'py>, ptr: *mut ffi::PyObject) -> PyResult<Self> {
+        NonNull::new(ptr).map_or_else(
+            || Err(PyErr::fetch(py)),
+            |ptr| Ok(Self(ptr, PhantomData, py)),
+        )
+    }
+
     /// # 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
@@ -1798,8 +1853,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() {
@@ -1998,6 +2054,81 @@ 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!(!dropped);
+
+                // creating the bound should have increased the refcount
+                drop(capsule);
+                assert!(!dropped);
+
+                // dropping the bound should now also decrease the refcount and free the object
+                drop(bound);
+                assert!(dropped);
+            }
+
+            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
+        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!(!dropped);
+
+                // creating the borrow should not have increased the refcount
+                drop(capsule);
+                assert!(dropped);
+            }
+
+            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;