deprecate Py<T> constructors (#5585)

* deprecate `Py<T>` constructors

* sentence per line

* newsfragment

* change some additional uses of `Py` constructors

* fix pre-3.12 implementation

* coverage

* fixup cfg for pre-3.10

Author: davidhewitt

guide/src/migration.md

@@ -15,6 +15,43 @@ To migrate use either
 - `from_py_object` to keep the automatic derive, or
 - `skip_from_py_object` to accept the new behaviour
 
+### Deprecation of `Py<T>` constructors from raw pointer
+
+The constructors `Py::from_owned_ptr`, `Py::from_owned_ptr_or_opt`, and `Py::from_owned_ptr_or_err` (and similar "borrowed" variants) perform an unchecked cast to the `Py<T>` target type `T`.
+This unchecked cast is a footgun on APIs where the primary concern is about constructing PyO3's safe smart pointer types correctly from the raw pointer value.
+
+The equivalent constructors on `Bound` always produce a `Bound<PyAny>`, which encourages any subsequent cast to be done explicitly as either checked or unchecked.
+These should be used instead.
+
+Before:
+
+```rust
+#![allow(deprecated)]
+# use pyo3::prelude::*;
+# use pyo3::types::PyNone;
+# Python::attach(|py| {
+let raw_ptr = py.None().into_ptr();
+
+let _: Py<PyNone> = unsafe { Py::from_borrowed_ptr(py, raw_ptr) };
+let _: Py<PyNone> = unsafe { Py::from_owned_ptr(py, raw_ptr) };
+# })
+```
+
+Before:
+
+```rust
+# use pyo3::prelude::*;
+# use pyo3::types::PyNone;
+# Python::attach(|py| {
+let raw_ptr = py.None().into_ptr();
+
+// Bound APIs require choice of doing unchecked or checked cast. Optionally `.unbind()` to
+// produce `Py<T>` values.
+let _: Bound<'_, PyNone> = unsafe { Bound::from_borrowed_ptr(py, raw_ptr).cast_into_unchecked() };
+let _: Bound<'_, PyNone> = unsafe { Bound::from_owned_ptr(py, raw_ptr).cast_into_unchecked() };
+# })
+```
+
 ## from 0.26.* to 0.27
 
 ### `FromPyObject` reworked for flexibility and efficiency

newsfragments/5585.changed.md

@@ -0,0 +1 @@
+Deprecate `Py<T>::from_{owned,borrowed}[or_{err,opt}]` constructors from raw pointer.

src/conversions/num_bigint.rs

@@ -50,8 +50,8 @@
 #[cfg(Py_LIMITED_API)]
 use crate::types::{bytes::PyBytesMethods, PyBytes};
 use crate::{
-    conversion::IntoPyObject, ffi, types::PyInt, Borrowed, Bound, FromPyObject, Py, PyAny, PyErr,
-    PyResult, Python,
+    conversion::IntoPyObject, std::num::nb_index, types::PyInt, Borrowed, Bound, FromPyObject,
+    PyAny, PyErr, PyResult, Python,
 };
 
 use num_bigint::{BigInt, BigUint};
@@ -129,14 +129,13 @@ impl<'py> FromPyObject<'_, 'py> for BigInt {
     type Error = PyErr;
 
     fn extract(ob: Borrowed<'_, 'py, PyAny>) -> Result<BigInt, Self::Error> {
-        let py = ob.py();
         // fast path - checking for subclass of `int` just checks a bit in the type object
-        let num_owned: Py<PyInt>;
+        let num_owned: Bound<'_, PyInt>;
         let num = if let Ok(long) = ob.cast::<PyInt>() {
             long
         } else {
-            num_owned = unsafe { Py::from_owned_ptr_or_err(py, ffi::PyNumber_Index(ob.as_ptr()))? };
-            num_owned.bind_borrowed(py)
+            num_owned = nb_index(&ob)?;
+            num_owned.as_borrowed()
         };
         #[cfg(not(Py_LIMITED_API))]
         {
@@ -179,14 +178,13 @@ impl<'py> FromPyObject<'_, 'py> for BigUint {
     type Error = PyErr;
 
     fn extract(ob: Borrowed<'_, 'py, PyAny>) -> Result<BigUint, Self::Error> {
-        let py = ob.py();
         // fast path - checking for subclass of `int` just checks a bit in the type object
-        let num_owned: Py<PyInt>;
+        let num_owned: Bound<'_, PyInt>;
         let num = if let Ok(long) = ob.cast::<PyInt>() {
             long
         } else {
-            num_owned = unsafe { Py::from_owned_ptr_or_err(py, ffi::PyNumber_Index(ob.as_ptr()))? };
-            num_owned.bind_borrowed(py)
+            num_owned = nb_index(&ob)?;
+            num_owned.as_borrowed()
         };
         #[cfg(not(Py_LIMITED_API))]
         {
@@ -208,6 +206,8 @@ impl<'py> FromPyObject<'_, 'py> for BigUint {
 #[cfg(not(any(Py_LIMITED_API, Py_3_13)))]
 #[inline]
 fn int_to_u32_vec<const SIGNED: bool>(long: &Bound<'_, PyInt>) -> PyResult<Vec<u32>> {
+    use crate::ffi;
+
     let mut buffer = Vec::new();
     let n_bits = int_n_bits(long)?;
     if n_bits == 0 {
@@ -242,6 +242,8 @@ fn int_to_u32_vec<const SIGNED: bool>(long: &Bound<'_, PyInt>) -> PyResult<Vec<u
 #[cfg(all(not(Py_LIMITED_API), Py_3_13))]
 #[inline]
 fn int_to_u32_vec<const SIGNED: bool>(long: &Bound<'_, PyInt>) -> PyResult<Vec<u32>> {
+    use crate::ffi;
+
     let mut buffer = Vec::new();
     let mut flags = ffi::Py_ASNATIVEBYTES_LITTLE_ENDIAN;
     if !SIGNED {
@@ -304,7 +306,7 @@ fn int_n_bits(long: &Bound<'_, PyInt>) -> PyResult<usize> {
     #[cfg(not(Py_LIMITED_API))]
     {
         // fast path
-        let n_bits = unsafe { ffi::_PyLong_NumBits(long.as_ptr()) };
+        let n_bits = unsafe { crate::ffi::_PyLong_NumBits(long.as_ptr()) };
         if n_bits == (-1isize as usize) {
             return Err(crate::PyErr::fetch(py));
         }
@@ -323,6 +325,7 @@ fn int_n_bits(long: &Bound<'_, PyInt>) -> PyResult<usize> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::exceptions::PyTypeError;
     use crate::test_utils::generate_unique_module_name;
     use crate::types::{PyAnyMethods as _, PyDict, PyModule};
     use indoc::indoc;
@@ -413,6 +416,7 @@ mod tests {
             locals.set_item("index", index).unwrap();
             let ob = py.eval(c"index.C(10)", None, Some(&locals)).unwrap();
             let _: BigInt = ob.extract().unwrap();
+            let _: BigUint = ob.extract().unwrap();
         });
     }
 
@@ -455,4 +459,16 @@ mod tests {
             }
         });
     }
+
+    #[test]
+    fn from_py_float_type_error() {
+        Python::attach(|py| {
+            let obj = (12.3f64).into_pyobject(py).unwrap();
+            let err = obj.extract::<BigInt>().unwrap_err();
+            assert!(err.is_instance_of::<PyTypeError>(py));
+
+            let err = obj.extract::<BigUint>().unwrap_err();
+            assert!(err.is_instance_of::<PyTypeError>(py));
+        });
+    }
 }

src/conversions/std/num.rs

@@ -5,6 +5,7 @@ use crate::ffi_ptr_ext::FfiPtrExt;
 use crate::inspect::types::TypeInfo;
 #[cfg(feature = "experimental-inspect")]
 use crate::inspect::TypeHint;
+use crate::py_result_ext::PyResultExt;
 use crate::types::{PyByteArray, PyByteArrayMethods, PyBytes, PyInt};
 use crate::{exceptions, ffi, Borrowed, Bound, FromPyObject, PyAny, PyErr, PyResult, Python};
 use std::convert::Infallible;
@@ -93,7 +94,7 @@ macro_rules! extract_int {
             err_if_invalid_value($obj.py(), $error_val, unsafe { $pylong_as(long.as_ptr()) })
         } else {
             unsafe {
-                let num = ffi::PyNumber_Index($obj.as_ptr()).assume_owned_or_err($obj.py())?;
+                let num = nb_index(&$obj)?;
                 err_if_invalid_value($obj.py(), $error_val, $pylong_as(num.as_ptr()))
             }
         }
@@ -459,8 +460,7 @@ mod fast_128bit_int_conversion {
                 const INPUT_TYPE: TypeHint = TypeHint::builtin("int");
 
                 fn extract(ob: Borrowed<'_, '_, PyAny>) -> Result<$rust_type, Self::Error> {
-                    let num =
-                        unsafe { ffi::PyNumber_Index(ob.as_ptr()).assume_owned_or_err(ob.py())? };
+                    let num = nb_index(&ob)?;
                     let mut buffer = [0u8; std::mem::size_of::<$rust_type>()];
                     #[cfg(not(Py_3_13))]
                     {
@@ -545,6 +545,11 @@ pub(crate) fn int_from_ne_bytes<'py, const IS_SIGNED: bool>(
     }
 }
 
+pub(crate) fn nb_index<'py>(obj: &Bound<'py, PyAny>) -> PyResult<Bound<'py, PyInt>> {
+    // SAFETY: PyNumber_Index returns a new reference or NULL on error
+    unsafe { ffi::PyNumber_Index(obj.as_ptr()).assume_owned_or_err(obj.py()) }.cast_into()
+}
+
 // For ABI3 we implement the conversion manually.
 #[cfg(Py_LIMITED_API)]
 mod slow_128bit_int_conversion {

src/conversions/std/osstr.rs

@@ -75,23 +75,25 @@ impl FromPyObject<'_, '_> for OsString {
 
         #[cfg(not(windows))]
         {
+            use crate::types::{PyBytes, PyBytesMethods};
+
             // Decode from Python's lossless bytes string representation back into raw bytes
+            // SAFETY: PyUnicode_EncodeFSDefault returns a new reference or null on error, known to
+            // be a `bytes` object, thread is attached to the interpreter
             let fs_encoded_bytes = unsafe {
-                crate::Py::<crate::types::PyBytes>::from_owned_ptr(
-                    ob.py(),
-                    ffi::PyUnicode_EncodeFSDefault(pystring.as_ptr()),
-                )
+                ffi::PyUnicode_EncodeFSDefault(pystring.as_ptr())
+                    .assume_owned_or_err(ob.py())?
+                    .cast_into_unchecked::<PyBytes>()
             };
 
             // Create an OsStr view into the raw bytes from Python
             //
             // For WASI: OS strings are UTF-8 by definition.
             #[cfg(target_os = "wasi")]
-            let os_str: &OsStr =
-                OsStr::new(std::str::from_utf8(fs_encoded_bytes.as_bytes(ob.py()))?);
+            let os_str: &OsStr = OsStr::new(std::str::from_utf8(fs_encoded_bytes.as_bytes())?);
             #[cfg(not(target_os = "wasi"))]
             let os_str: &OsStr =
-                std::os::unix::ffi::OsStrExt::from_bytes(fs_encoded_bytes.as_bytes(ob.py()));
+                std::os::unix::ffi::OsStrExt::from_bytes(fs_encoded_bytes.as_bytes());
 
             Ok(os_str.to_os_string())
         }

src/err/err_state.rs

@@ -147,10 +147,9 @@ impl PyErrStateNormalized {
             ptype: pvalue.get_type().into(),
             #[cfg(not(Py_3_12))]
             ptraceback: unsafe {
-                Py::from_owned_ptr_or_opt(
-                    pvalue.py(),
-                    ffi::PyException_GetTraceback(pvalue.as_ptr()),
-                )
+                ffi::PyException_GetTraceback(pvalue.as_ptr())
+                    .assume_owned_or_opt(pvalue.py())
+                    .map(|b| b.cast_into_unchecked().unbind())
             },
             pvalue: pvalue.into(),
         }
@@ -240,11 +239,22 @@ impl PyErrStateNormalized {
         ptraceback: *mut ffi::PyObject,
     ) -> Self {
         PyErrStateNormalized {
-            ptype: unsafe { Py::from_owned_ptr_or_opt(py, ptype).expect("Exception type missing") },
+            ptype: unsafe {
+                ptype
+                    .assume_owned_or_opt(py)
+                    .expect("Exception type missing")
+                    .cast_into_unchecked()
+            }
+            .unbind(),
             pvalue: unsafe {
-                Py::from_owned_ptr_or_opt(py, pvalue).expect("Exception value missing")
-            },
-            ptraceback: unsafe { Py::from_owned_ptr_or_opt(py, ptraceback) },
+                pvalue
+                    .assume_owned_or_opt(py)
+                    .expect("Exception value missing")
+                    .cast_into_unchecked()
+            }
+            .unbind(),
+            ptraceback: unsafe { ptraceback.assume_owned_or_opt(py) }
+                .map(|b| unsafe { b.cast_into_unchecked() }.unbind()),
         }
     }
 

src/err/mod.rs

@@ -1,7 +1,9 @@
+use crate::ffi_ptr_ext::FfiPtrExt;
 use crate::instance::Bound;
 #[cfg(Py_3_11)]
 use crate::intern;
 use crate::panic::PanicException;
+use crate::py_result_ext::PyResultExt;
 use crate::type_object::PyTypeInfo;
 use crate::types::any::PyAnyMethods;
 #[cfg(Py_3_11)]
@@ -361,9 +363,14 @@ impl PyErr {
             None => std::ptr::null(),
         };
 
-        let ptr = unsafe { ffi::PyErr_NewExceptionWithDoc(name.as_ptr(), doc_ptr, base, dict) };
-
-        unsafe { Py::from_owned_ptr_or_err(py, ptr) }
+        // SAFETY: correct call to FFI function, return value is known to be a new
+        // exception type or null on error
+        unsafe {
+            ffi::PyErr_NewExceptionWithDoc(name.as_ptr(), doc_ptr, base, dict)
+                .assume_owned_or_err(py)
+                .cast_into_unchecked()
+        }
+        .map(Bound::unbind)
     }
 
     /// Prints a standard traceback to `sys.stderr`.

src/impl_/pyclass.rs

@@ -1,6 +1,7 @@
 use crate::{
     exceptions::{PyAttributeError, PyNotImplementedError, PyRuntimeError},
     ffi,
+    ffi_ptr_ext::FfiPtrExt,
     impl_::{
         freelist::PyObjectFreeList,
         pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectLayout},
@@ -332,7 +333,8 @@ slot_fragment_trait! {
         attr: *mut ffi::PyObject,
     ) -> PyResult<*mut ffi::PyObject> {
         Err(PyErr::new::<PyAttributeError, _>(
-            (unsafe {Py::<PyAny>::from_borrowed_ptr(py, attr)},)
+            // SAFETY: caller has upheld the safety contract
+            (unsafe { attr.assume_borrowed_unchecked(py) }.to_owned().unbind(),)
         ))
     }
 }