Update dict.get_item binding to use PyDict_GetItemRef (#4355)

* Update dict.get_item binding to use PyDict_GetItemRef

Refs #4265

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* fix: fix logic error in dict.get_item bindings

* update: apply david's review suggestions for dict.get_item bindings

* update: create ffi::compat to store compatibility shims

* update: move PyDict_GetItemRef bindings to spot in order from dictobject.h

* build: fix build warning with --no-default-features

* doc: expand release note fragments

* fix: fix clippy warnings

* respond to review comments

* Apply suggestion from @mejrs

* refactor so cfg is applied to functions

* properly set cfgs

* fix clippy lints

* Apply @davidhewitt's suggestion

* deal with upstream deprecation of new_bound

Author: ngoldbaum

newsfragments/4355.added.md

@@ -0,0 +1,10 @@
+* Added an `ffi::compat` namespace to store compatibility shims for C API
+  functions added in recent versions of Python.
+
+* Added bindings for `PyDict_GetItemRef` on Python 3.13 and newer. Also added
+  `ffi::compat::PyDict_GetItemRef` which re-exports the FFI binding on Python
+  3.13 or newer and defines a compatibility version on older versions of
+  Python. This function is inherently safer to use than `PyDict_GetItem` and has
+  an API that is easier to use than `PyDict_GetItemWithError`. It returns a
+  strong reference to value, as opposed to the two older functions which return
+  a possibly unsafe borrowed reference.

newsfragments/4355.fixed.md

@@ -0,0 +1,2 @@
+Avoid creating temporary borrowed reference in dict.get_item bindings. Borrowed
+references like this are unsafe in the free-threading build.

pyo3-ffi/src/compat.rs

@@ -0,0 +1,44 @@
+//! C API Compatibility Shims
+//!
+//! Some CPython C API functions added in recent versions of Python are
+//! inherently safer to use than older C API constructs. This module
+//! exposes functions available on all Python versions that wrap the
+//! old C API on old Python versions and wrap the function directly
+//! on newer Python versions.
+
+// Unless otherwise noted, the compatibility shims are adapted from
+// the pythoncapi-compat project: https://github.com/python/pythoncapi-compat
+
+#[cfg(not(Py_3_13))]
+use crate::object::PyObject;
+#[cfg(not(Py_3_13))]
+use std::os::raw::c_int;
+
+#[cfg_attr(docsrs, doc(cfg(all)))]
+#[cfg(Py_3_13)]
+pub use crate::dictobject::PyDict_GetItemRef;
+
+#[cfg_attr(docsrs, doc(cfg(all)))]
+#[cfg(not(Py_3_13))]
+pub unsafe fn PyDict_GetItemRef(
+    dp: *mut PyObject,
+    key: *mut PyObject,
+    result: *mut *mut PyObject,
+) -> c_int {
+    {
+        use crate::dictobject::PyDict_GetItemWithError;
+        use crate::object::_Py_NewRef;
+        use crate::pyerrors::PyErr_Occurred;
+
+        let item: *mut PyObject = PyDict_GetItemWithError(dp, key);
+        if !item.is_null() {
+            *result = _Py_NewRef(item);
+            return 1; // found
+        }
+        *result = std::ptr::null_mut();
+        if PyErr_Occurred().is_null() {
+            return 0; // not found
+        }
+        -1
+    }
+}

pyo3-ffi/src/dictobject.rs

@@ -66,6 +66,12 @@ extern "C" {
     ) -> c_int;
     #[cfg_attr(PyPy, link_name = "PyPyDict_DelItemString")]
     pub fn PyDict_DelItemString(dp: *mut PyObject, key: *const c_char) -> c_int;
+    #[cfg(Py_3_13)]
+    pub fn PyDict_GetItemRef(
+        dp: *mut PyObject,
+        key: *mut PyObject,
+        result: *mut *mut PyObject,
+    ) -> c_int;
     // skipped 3.10 / ex-non-limited PyObject_GenericGetDict
 }
 

pyo3-ffi/src/lib.rs

@@ -1,3 +1,4 @@
+#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
 //! Raw FFI declarations for Python's C API.
 //!
 //! PyO3 can be used to write native Python modules or run Python code and modules from Rust.
@@ -290,6 +291,8 @@ pub const fn _cstr_from_utf8_with_nul_checked(s: &str) -> &CStr {
 
 use std::ffi::CStr;
 
+pub mod compat;
+
 pub use self::abstract_::*;
 pub use self::bltinmodule::*;
 pub use self::boolobject::*;

src/types/dict.rs

@@ -436,13 +436,13 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> {
             key: Bound<'_, PyAny>,
         ) -> PyResult<Option<Bound<'py, PyAny>>> {
             let py = dict.py();
+            let mut result: *mut ffi::PyObject = std::ptr::null_mut();
             match unsafe {
-                ffi::PyDict_GetItemWithError(dict.as_ptr(), key.as_ptr())
-                    .assume_borrowed_or_opt(py)
-                    .map(Borrowed::to_owned)
+                ffi::compat::PyDict_GetItemRef(dict.as_ptr(), key.as_ptr(), &mut result)
             } {
-                some @ Some(_) => Ok(some),
-                None => PyErr::take(py).map(Err).transpose(),
+                std::os::raw::c_int::MIN..=-1 => Err(PyErr::fetch(py)),
+                0 => Ok(None),
+                1..=std::os::raw::c_int::MAX => Ok(Some(unsafe { result.assume_owned(py) })),
             }
         }
 
@@ -957,6 +957,42 @@ mod tests {
         });
     }
 
+    #[cfg(feature = "macros")]
+    #[test]
+    fn test_get_item_error_path() {
+        use crate::exceptions::PyTypeError;
+
+        #[crate::pyclass(crate = "crate")]
+        struct HashErrors;
+
+        #[crate::pymethods(crate = "crate")]
+        impl HashErrors {
+            #[new]
+            fn new() -> Self {
+                HashErrors {}
+            }
+
+            fn __hash__(&self) -> PyResult<isize> {
+                Err(PyTypeError::new_err("Error from __hash__"))
+            }
+        }
+
+        Python::with_gil(|py| {
+            let class = py.get_type_bound::<HashErrors>();
+            let instance = class.call0().unwrap();
+            let d = PyDict::new_bound(py);
+            match d.get_item(instance) {
+                Ok(_) => {
+                    panic!("this get_item call should always error")
+                }
+                Err(err) => {
+                    assert!(err.is_instance_of::<PyTypeError>(py));
+                    assert_eq!(err.value_bound(py).to_string(), "Error from __hash__")
+                }
+            }
+        })
+    }
+
     #[test]
     #[allow(deprecated)]
     #[cfg(all(not(any(PyPy, GraalPy)), feature = "gil-refs"))]