From 74a1dab775e26a5b243697a62eb7d7845ca74a9e Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 6 Apr 2026 12:41:20 -0600 Subject: [PATCH 01/11] Add bindings for PyDict_SetDefaultRef and use them in PyCode::run --- pyo3-ffi/src/compat/py_3_13.rs | 48 ++++++++++++++++++++++++++++++ pyo3-ffi/src/cpython/dictobject.rs | 1 - pyo3-ffi/src/dictobject.rs | 7 +++++ src/types/code.rs | 35 ++++++++++------------ 4 files changed, 70 insertions(+), 21 deletions(-) diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs index 08bdf5cba18..11f61d1ddaf 100644 --- a/pyo3-ffi/src/compat/py_3_13.rs +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -130,3 +130,51 @@ compat_function!( crate::_PyThreadState_UncheckedGet() } ); + +compat_function! { + originally_defined_for(any(Py_3_13, all(Py_LIMITED_API, Py_3_15))); + + #[inline] + pub unsafe fn PyDict_SetDefaultRef( + mp: *mut crate::PyObject, + key: *mut crate::PyObject, + default_value: *mut crate::PyObject, + result: *mut *mut crate::PyObject, + ) -> std::ffi::c_int { + use crate::{ + compat::{PyDict_GetItemRef, Py_NewRef}, + PyDict_SetItem, PyObject, Py_DECREF, + }; + let mut value: *mut PyObject = std::ptr::null_mut(); + if PyDict_GetItemRef(mp, key, &mut value) < 0 { + // get error + if !result.is_null() { + *result = std::ptr::null_mut(); + } + return -1; + } + if !value.is_null() { + // present + if !result.is_null() { + *result = value; + } + else { + Py_DECREF(value); + } + return 1; + } + + // missing, set the item + if PyDict_SetItem(mp, key, default_value) < 0 { + // set error + if !result.is_null() { + *result = std::ptr::null_mut(); + } + return -1; + } + if !result.is_null() { + *result = Py_NewRef(default_value); + } + return 0; + } +} diff --git a/pyo3-ffi/src/cpython/dictobject.rs b/pyo3-ffi/src/cpython/dictobject.rs index 37991ee4ebe..4c93f068e2b 100644 --- a/pyo3-ffi/src/cpython/dictobject.rs +++ b/pyo3-ffi/src/cpython/dictobject.rs @@ -43,7 +43,6 @@ pub struct PyDictObject { // skipped private _PyDict_GetItemStringWithError // skipped PyDict_SetDefault -// skipped PyDict_SetDefaultRef // skipped PyDict_GET_SIZE // skipped PyDict_ContainsString diff --git a/pyo3-ffi/src/dictobject.rs b/pyo3-ffi/src/dictobject.rs index 4afa2ffb5e8..c449132fa98 100644 --- a/pyo3-ffi/src/dictobject.rs +++ b/pyo3-ffi/src/dictobject.rs @@ -78,6 +78,13 @@ extern_libpython! { key: *const c_char, result: *mut *mut PyObject, ) -> c_int; + #[cfg(any(Py_3_13, all(Py_LIMITED_API, Py_3_15)))] + pub fn PyDict_SetDefaultRef( + mp: *mut PyObject, + key: *mut PyObject, + default_value: *mut PyObject, + result: *mut *mut PyObject, + ) -> c_int; // skipped 3.10 / ex-non-limited PyObject_GenericGetDict } diff --git a/src/types/code.rs b/src/types/code.rs index 8d38a8fc826..7b2d235249a 100644 --- a/src/types/code.rs +++ b/src/types/code.rs @@ -127,28 +127,23 @@ impl<'py> PyCodeMethods<'py> for Bound<'py, PyCode> { // - https://github.com/python/cpython/pull/24564 (the same fix in CPython 3.10) // - https://github.com/PyO3/pyo3/issues/3370 let builtins_s = crate::intern!(self.py(), "__builtins__"); - let has_builtins = globals.contains(builtins_s)?; - if !has_builtins { - crate::sync::critical_section::with_critical_section(globals, || { - // check if another thread set __builtins__ while this thread was blocked on the critical section - let has_builtins = globals.contains(builtins_s)?; - if !has_builtins { - // Inherit current builtins. - let builtins = unsafe { ffi::PyEval_GetBuiltins() }; - - // `PyDict_SetItem` doesn't take ownership of `builtins`, but `PyEval_GetBuiltins` - // seems to return a borrowed reference, so no leak here. - if unsafe { - ffi::PyDict_SetItem(globals.as_ptr(), builtins_s.as_ptr(), builtins) - } == -1 - { - return Err(PyErr::fetch(self.py())); - } - } - Ok(()) - })?; + let mut result: *mut ffi::PyObject = std::ptr::null_mut(); + if unsafe { + ffi::compat::PyDict_SetDefaultRef( + globals.as_ptr(), + builtins_s.as_ptr(), + // borrowed reference + ffi::PyEval_GetBuiltins(), + &mut result, + ) + } == -1 + { + return Err(PyErr::fetch(self.py())); } + // release ownership of result + unsafe { ffi::Py_DECREF(result) }; + unsafe { ffi::PyEval_EvalCode(self.as_ptr(), globals.as_ptr(), locals.as_ptr()) .assume_owned_or_err(self.py()) From 13a0dc4fdbb0fcdaf7c88a672388068cc85b2672 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 6 Apr 2026 14:06:38 -0600 Subject: [PATCH 02/11] fix conditional compilation --- pyo3-ffi/src/compat/py_3_13.rs | 2 +- pyo3-ffi/src/dictobject.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs index 11f61d1ddaf..f7e3e28808f 100644 --- a/pyo3-ffi/src/compat/py_3_13.rs +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -132,7 +132,7 @@ compat_function!( ); compat_function! { - originally_defined_for(any(Py_3_13, all(Py_LIMITED_API, Py_3_15))); + originally_defined_for(any(all(Py_3_13, not(Py_LIMITED_API)), all(Py_LIMITED_API, Py_3_15))); #[inline] pub unsafe fn PyDict_SetDefaultRef( diff --git a/pyo3-ffi/src/dictobject.rs b/pyo3-ffi/src/dictobject.rs index c449132fa98..e90f9150b38 100644 --- a/pyo3-ffi/src/dictobject.rs +++ b/pyo3-ffi/src/dictobject.rs @@ -78,7 +78,7 @@ extern_libpython! { key: *const c_char, result: *mut *mut PyObject, ) -> c_int; - #[cfg(any(Py_3_13, all(Py_LIMITED_API, Py_3_15)))] + #[cfg(any(all(Py_3_13, not(Py_LIMITED_API)), all(Py_LIMITED_API, Py_3_15)))] pub fn PyDict_SetDefaultRef( mp: *mut PyObject, key: *mut PyObject, From 8e14f5dce4c322f6915a0ca15a59d23608ff1f06 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 6 Apr 2026 14:13:08 -0600 Subject: [PATCH 03/11] simplify conditional predicates --- pyo3-ffi/src/compat/py_3_13.rs | 2 +- pyo3-ffi/src/dictobject.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs index f7e3e28808f..1f8c3baa54d 100644 --- a/pyo3-ffi/src/compat/py_3_13.rs +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -132,7 +132,7 @@ compat_function!( ); compat_function! { - originally_defined_for(any(all(Py_3_13, not(Py_LIMITED_API)), all(Py_LIMITED_API, Py_3_15))); + originally_defined_for(all(Py_3_13, any(not(Py_LIMITED_API), Py_3_15))); #[inline] pub unsafe fn PyDict_SetDefaultRef( diff --git a/pyo3-ffi/src/dictobject.rs b/pyo3-ffi/src/dictobject.rs index e90f9150b38..4df1265d4b8 100644 --- a/pyo3-ffi/src/dictobject.rs +++ b/pyo3-ffi/src/dictobject.rs @@ -78,7 +78,7 @@ extern_libpython! { key: *const c_char, result: *mut *mut PyObject, ) -> c_int; - #[cfg(any(all(Py_3_13, not(Py_LIMITED_API)), all(Py_LIMITED_API, Py_3_15)))] + #[cfg(all(Py_3_13, any(not(Py_LIMITED_API), Py_3_15)))] pub fn PyDict_SetDefaultRef( mp: *mut PyObject, key: *mut PyObject, From 83c374fb21a2ad0f46494f2c18b24d9cf5f8028b Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 6 Apr 2026 14:23:27 -0600 Subject: [PATCH 04/11] minor touchups --- pyo3-ffi/src/compat/py_3_13.rs | 9 ++++----- src/types/code.rs | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs index 1f8c3baa54d..26b9f2698fb 100644 --- a/pyo3-ffi/src/compat/py_3_13.rs +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -131,7 +131,7 @@ compat_function!( } ); -compat_function! { +compat_function!( originally_defined_for(all(Py_3_13, any(not(Py_LIMITED_API), Py_3_15))); #[inline] @@ -157,8 +157,7 @@ compat_function! { // present if !result.is_null() { *result = value; - } - else { + } else { Py_DECREF(value); } return 1; @@ -175,6 +174,6 @@ compat_function! { if !result.is_null() { *result = Py_NewRef(default_value); } - return 0; + 0 } -} +); diff --git a/src/types/code.rs b/src/types/code.rs index 7b2d235249a..fdb2fe4383d 100644 --- a/src/types/code.rs +++ b/src/types/code.rs @@ -132,7 +132,8 @@ impl<'py> PyCodeMethods<'py> for Bound<'py, PyCode> { ffi::compat::PyDict_SetDefaultRef( globals.as_ptr(), builtins_s.as_ptr(), - // borrowed reference + // safety: the interpreter will keep the borrowed reference to + // builtins alive at least until SetDefaultRef finishes ffi::PyEval_GetBuiltins(), &mut result, ) From 81325e373c7af73fa85fa6f78d2104cdc83d5480 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 6 Apr 2026 14:32:11 -0600 Subject: [PATCH 05/11] add changelog entries --- newsfragments/5955.added.md | 2 ++ newsfragments/5955.changed.md | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 newsfragments/5955.added.md create mode 100644 newsfragments/5955.changed.md diff --git a/newsfragments/5955.added.md b/newsfragments/5955.added.md new file mode 100644 index 00000000000..237280ac1e7 --- /dev/null +++ b/newsfragments/5955.added.md @@ -0,0 +1,2 @@ +Added FFI bindings for PyDict_SetDefaultRef for Python 3.13 and newer and 3.15 +and newer in the limited API. Also added a compat shim for older Python versions. diff --git a/newsfragments/5955.changed.md b/newsfragments/5955.changed.md new file mode 100644 index 00000000000..acb04bb2f4f --- /dev/null +++ b/newsfragments/5955.changed.md @@ -0,0 +1,2 @@ +`PyCode::run` uses PyDict_SetDefaultRef instead of a critical section to ensure +thread safety when it adds a reference to `__builtins__` to the globals dict. From 5010efb4894fab3d71b6eab297ed2630b4826bfe Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 13 Apr 2026 09:24:40 -0600 Subject: [PATCH 06/11] pass NULL for the result --- src/types/code.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/types/code.rs b/src/types/code.rs index fdb2fe4383d..867cb049335 100644 --- a/src/types/code.rs +++ b/src/types/code.rs @@ -127,7 +127,6 @@ impl<'py> PyCodeMethods<'py> for Bound<'py, PyCode> { // - https://github.com/python/cpython/pull/24564 (the same fix in CPython 3.10) // - https://github.com/PyO3/pyo3/issues/3370 let builtins_s = crate::intern!(self.py(), "__builtins__"); - let mut result: *mut ffi::PyObject = std::ptr::null_mut(); if unsafe { ffi::compat::PyDict_SetDefaultRef( globals.as_ptr(), @@ -135,16 +134,13 @@ impl<'py> PyCodeMethods<'py> for Bound<'py, PyCode> { // safety: the interpreter will keep the borrowed reference to // builtins alive at least until SetDefaultRef finishes ffi::PyEval_GetBuiltins(), - &mut result, + std::ptr::null_mut(), ) } == -1 { return Err(PyErr::fetch(self.py())); } - // release ownership of result - unsafe { ffi::Py_DECREF(result) }; - unsafe { ffi::PyEval_EvalCode(self.as_ptr(), globals.as_ptr(), locals.as_ptr()) .assume_owned_or_err(self.py()) From de76ca22ef9f7b4b3529c28008fece6a0997c962 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 13 Apr 2026 12:42:37 -0600 Subject: [PATCH 07/11] Add new PyDictMethods API wrapping PyDict_SetDefaultRef --- newsfragments/5955.added.md | 8 +- src/err/mod.rs | 13 +++ src/types/code.rs | 20 +---- src/types/dict.rs | 161 ++++++++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+), 18 deletions(-) diff --git a/newsfragments/5955.added.md b/newsfragments/5955.added.md index 237280ac1e7..39af36e4c17 100644 --- a/newsfragments/5955.added.md +++ b/newsfragments/5955.added.md @@ -1,2 +1,6 @@ -Added FFI bindings for PyDict_SetDefaultRef for Python 3.13 and newer and 3.15 -and newer in the limited API. Also added a compat shim for older Python versions. +* Added FFI bindings for `PyDict_SetDefaultRef` for Python 3.13 and newer and + 3.15 and newer in the limited API. Also added a compat shim for older Python + versions. + +* Added `PyDictMethods::set_default` and `PyDictMethods::set_default_ref` to + allow atomically setting default values in a PyDict. diff --git a/src/err/mod.rs b/src/err/mod.rs index 53f97fa1c45..43d006d2332 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -761,6 +761,19 @@ pub(crate) fn error_on_minusone(py: Python<'_>, result: T) -> } } +/// Returns Ok wrapping the result if the error code is not -1. +#[inline] +pub(crate) fn error_on_minusone_with_result( + py: Python<'_>, + result: T, +) -> PyResult { + if result != T::MINUS_ONE { + Ok(result) + } else { + Err(PyErr::fetch(py)) + } +} + pub(crate) trait SignedInteger: Eq { const MINUS_ONE: Self; } diff --git a/src/types/code.rs b/src/types/code.rs index 867cb049335..d35ca6c545e 100644 --- a/src/types/code.rs +++ b/src/types/code.rs @@ -1,5 +1,5 @@ -use super::PyAnyMethods as _; use super::PyDict; +use super::{PyAnyMethods as _, PyDictMethods as _}; use crate::ffi_ptr_ext::FfiPtrExt; use crate::py_result_ext::PyResultExt; #[cfg(any(Py_LIMITED_API, PyPy))] @@ -8,7 +8,7 @@ use crate::sync::PyOnceLock; use crate::types::{PyType, PyTypeMethods}; #[cfg(any(Py_LIMITED_API, PyPy))] use crate::Py; -use crate::{ffi, Bound, PyAny, PyErr, PyResult, Python}; +use crate::{ffi, Bound, PyAny, PyResult, Python}; use std::ffi::CStr; /// Represents a Python code object. @@ -127,20 +127,8 @@ impl<'py> PyCodeMethods<'py> for Bound<'py, PyCode> { // - https://github.com/python/cpython/pull/24564 (the same fix in CPython 3.10) // - https://github.com/PyO3/pyo3/issues/3370 let builtins_s = crate::intern!(self.py(), "__builtins__"); - if unsafe { - ffi::compat::PyDict_SetDefaultRef( - globals.as_ptr(), - builtins_s.as_ptr(), - // safety: the interpreter will keep the borrowed reference to - // builtins alive at least until SetDefaultRef finishes - ffi::PyEval_GetBuiltins(), - std::ptr::null_mut(), - ) - } == -1 - { - return Err(PyErr::fetch(self.py())); - } - + let builtins = unsafe { ffi::PyEval_GetBuiltins().assume_borrowed_unchecked(self.py()) }; + globals.set_default(builtins_s, builtins)?; unsafe { ffi::PyEval_EvalCode(self.as_ptr(), globals.as_ptr(), locals.as_ptr()) .assume_owned_or_err(self.py()) diff --git a/src/types/dict.rs b/src/types/dict.rs index 0f03a217f79..b9e781a5058 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -94,6 +94,16 @@ impl PyDict { } } +fn setdefault_result_from_nonerror_return_code(code: std::ffi::c_int) -> bool { + match code { + // inserted + 0 => true, + // not inserted + 1 => false, + x => panic!("Unknown return value from PyDict_SetDeaultRef: {x}"), + } +} + /// Implementation of functionality for [`PyDict`]. /// /// These methods are defined for the `Bound<'py, PyDict>` smart pointer, so to use method call @@ -207,6 +217,31 @@ pub trait PyDictMethods<'py>: crate::sealed::Sealed { /// This method uses [`PyDict_Merge`](https://docs.python.org/3/c-api/dict.html#c.PyDict_Merge) internally, /// so should have the same performance as `update`. fn update_if_missing(&self, other: &Bound<'_, PyMapping>) -> PyResult<()>; + + /// Inserts `default_value` into this dictionary with a key of `key` if the key is not already present in the + /// dictionary. If the key was inserted, returns Ok(true), otherwise returns Ok(false), indicating the key was + /// already present. If an error happens, returns PyErr. This function uses + /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally, so it has + /// the same thread safety guarantees as that function. + fn set_default(&self, key: K, default_value: V) -> PyResult + where + K: IntoPyObject<'py>, + V: IntoPyObject<'py>; + + /// Inserts `default_value` into this dictionary with a key of `key` if the key is not already present in the + /// dictionary. If the key was inserted, returns Ok((true, result)), otherwise returns Ok((false, result)) where + /// `result` is the `value` associated with `key` after this function finishes. If an error happens, returns + /// PyErr. This function uses + /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally, so it has + /// the same thread safety guarantees as that function. + fn set_default_with_result( + &self, + key: K, + default_value: V, + ) -> PyResult<(bool, Bound<'py, PyAny>)> + where + K: IntoPyObject<'py>, + V: IntoPyObject<'py>; } impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { @@ -385,6 +420,82 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { ffi::PyDict_Merge(self.as_ptr(), other.as_ptr(), 0) }) } + + fn set_default(&self, key: K, default_value: V) -> PyResult + where + K: IntoPyObject<'py>, + V: IntoPyObject<'py>, + { + fn inner( + dict: &Bound<'_, PyDict>, + key: Borrowed<'_, '_, PyAny>, + value: Borrowed<'_, '_, PyAny>, + ) -> PyResult { + err::error_on_minusone_with_result(dict.py(), unsafe { + ffi::compat::PyDict_SetDefaultRef( + dict.as_ptr(), + key.as_ptr(), + value.as_ptr(), + std::ptr::null_mut(), + ) + }) + } + let py = self.py(); + + Ok(setdefault_result_from_nonerror_return_code(inner( + self, + key.into_pyobject_or_pyerr(py)?.into_any().as_borrowed(), + default_value + .into_pyobject_or_pyerr(py)? + .into_any() + .as_borrowed(), + )?)) + } + + fn set_default_with_result( + &self, + key: K, + default_value: V, + ) -> PyResult<(bool, Bound<'py, PyAny>)> + where + K: IntoPyObject<'py>, + V: IntoPyObject<'py>, + { + fn inner( + dict: &Bound<'_, PyDict>, + key: Borrowed<'_, '_, PyAny>, + value: Borrowed<'_, '_, PyAny>, + result: &mut *mut ffi::PyObject, + ) -> PyResult { + err::error_on_minusone_with_result(dict.py(), unsafe { + ffi::compat::PyDict_SetDefaultRef( + dict.as_ptr(), + key.as_ptr(), + value.as_ptr(), + result, + ) + }) + } + let py = self.py(); + static UNINITIALIZED: u8 = 0; + let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject; + let code = inner( + self, + key.into_pyobject_or_pyerr(py)?.into_any().as_borrowed(), + default_value + .into_pyobject_or_pyerr(py)? + .into_any() + .as_borrowed(), + &mut ob_result, + )?; + assert!(ob_result != std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject); + // SAFETY: the intepreter should have set this to a valid owned PyObject pointer + let out_result = unsafe { ob_result.assume_owned(py) }; + Ok(( + setdefault_result_from_nonerror_return_code(code), + out_result, + )) + } } impl<'a, 'py> Borrowed<'a, 'py, PyDict> { @@ -1669,4 +1780,54 @@ mod tests { assert_eq!(dict.iter().count(), 3); }) } + + #[test] + fn test_set_default() { + Python::attach(|py| { + let dict = PyDict::new(py); + assert!(matches!(dict.set_default("hello", "world"), Ok(true))); + assert!( + dict.get_item("hello") + .unwrap() + .unwrap() + .extract::<&str>() + .unwrap() + == "world" + ); + + assert!(matches!(dict.set_default("hello", "foobar"), Ok(false))); + + let invalid_key = PyList::new(py, vec![0]).unwrap(); + assert!(dict.set_default(invalid_key, "foobar").is_err()); + }) + } + + #[test] + fn test_set_default_with_result() { + Python::attach(|py| { + let dict = PyDict::new(py); + let res = dict.set_default_with_result("hello", "world"); + assert!(res.is_ok()); + let res = res.unwrap(); + assert!(res.0 == true); + assert!(res.1.extract::<&str>().unwrap() == "world"); + assert!( + dict.get_item("hello") + .unwrap() + .unwrap() + .extract::<&str>() + .unwrap() + == "world" + ); + + let res = dict.set_default_with_result("hello", "foobar"); + assert!(res.is_ok()); + let res = res.unwrap(); + assert!(res.0 == false); + assert!(res.1.extract::<&str>().unwrap() == "world"); + + let invalid_key = PyList::new(py, vec![0]).unwrap(); + assert!(dict.set_default_with_result(invalid_key, "foobar").is_err()); + }) + } } From 583362b495b640d79e853dd5b42648f304b4dd59 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 13 Apr 2026 13:07:50 -0600 Subject: [PATCH 08/11] gate new test for 3.9 and older limited API --- src/types/dict.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/types/dict.rs b/src/types/dict.rs index b9e781a5058..257800dfaab 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -1782,6 +1782,8 @@ mod tests { } #[test] + // Python 3.10 limited API is needed for .extract::<&str> + #[cfg(not(all(Py_LIMITED_API, not(Py_3_10))))] fn test_set_default() { Python::attach(|py| { let dict = PyDict::new(py); @@ -1803,6 +1805,8 @@ mod tests { } #[test] + // Python 3.10 is needed for .extract::<&str> + #[cfg(not(all(Py_LIMITED_API, not(Py_3_10))))] fn test_set_default_with_result() { Python::attach(|py| { let dict = PyDict::new(py); From 5fcc1d87a4f1b4f83b0c4bcc70e4ab500d91c468 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 13 Apr 2026 13:12:51 -0600 Subject: [PATCH 09/11] fix clippy and typos --- src/types/dict.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 257800dfaab..8d638438f96 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -100,7 +100,7 @@ fn setdefault_result_from_nonerror_return_code(code: std::ffi::c_int) -> bool { 0 => true, // not inserted 1 => false, - x => panic!("Unknown return value from PyDict_SetDeaultRef: {x}"), + x => panic!("Unknown return value from PyDict_SetDefaultRef: {x}"), } } @@ -489,7 +489,7 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { &mut ob_result, )?; assert!(ob_result != std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject); - // SAFETY: the intepreter should have set this to a valid owned PyObject pointer + // SAFETY: the interpreter should have set this to a valid owned PyObject pointer let out_result = unsafe { ob_result.assume_owned(py) }; Ok(( setdefault_result_from_nonerror_return_code(code), @@ -1813,7 +1813,7 @@ mod tests { let res = dict.set_default_with_result("hello", "world"); assert!(res.is_ok()); let res = res.unwrap(); - assert!(res.0 == true); + assert!(res.0); assert!(res.1.extract::<&str>().unwrap() == "world"); assert!( dict.get_item("hello") @@ -1827,7 +1827,7 @@ mod tests { let res = dict.set_default_with_result("hello", "foobar"); assert!(res.is_ok()); let res = res.unwrap(); - assert!(res.0 == false); + assert!(!res.0); assert!(res.1.extract::<&str>().unwrap() == "world"); let invalid_key = PyList::new(py, vec![0]).unwrap(); From ffd7ab11927a2910e6a9afd8596f9e48c4d1df86 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 14 Apr 2026 09:42:00 -0600 Subject: [PATCH 10/11] Apply code review comments from David --- src/types/dict.rs | 124 ++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 65 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 8d638438f96..5cd39aa954b 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -94,16 +94,6 @@ impl PyDict { } } -fn setdefault_result_from_nonerror_return_code(code: std::ffi::c_int) -> bool { - match code { - // inserted - 0 => true, - // not inserted - 1 => false, - x => panic!("Unknown return value from PyDict_SetDefaultRef: {x}"), - } -} - /// Implementation of functionality for [`PyDict`]. /// /// These methods are defined for the `Bound<'py, PyDict>` smart pointer, so to use method call @@ -221,8 +211,7 @@ pub trait PyDictMethods<'py>: crate::sealed::Sealed { /// Inserts `default_value` into this dictionary with a key of `key` if the key is not already present in the /// dictionary. If the key was inserted, returns Ok(true), otherwise returns Ok(false), indicating the key was /// already present. If an error happens, returns PyErr. This function uses - /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally, so it has - /// the same thread safety guarantees as that function. + /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally. fn set_default(&self, key: K, default_value: V) -> PyResult where K: IntoPyObject<'py>, @@ -232,8 +221,7 @@ pub trait PyDictMethods<'py>: crate::sealed::Sealed { /// dictionary. If the key was inserted, returns Ok((true, result)), otherwise returns Ok((false, result)) where /// `result` is the `value` associated with `key` after this function finishes. If an error happens, returns /// PyErr. This function uses - /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally, so it has - /// the same thread safety guarantees as that function. + /// [`PyDict_SetDefaultRef`](https://docs.python.org/3/c-api/dict.html#c.PyDict_SetDefaultRef) internally. fn set_default_with_result( &self, key: K, @@ -430,26 +418,29 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { dict: &Bound<'_, PyDict>, key: Borrowed<'_, '_, PyAny>, value: Borrowed<'_, '_, PyAny>, - ) -> PyResult { - err::error_on_minusone_with_result(dict.py(), unsafe { - ffi::compat::PyDict_SetDefaultRef( - dict.as_ptr(), - key.as_ptr(), - value.as_ptr(), - std::ptr::null_mut(), - ) - }) + ) -> PyResult { + setdefault_result_from_nonerror_return_code(err::error_on_minusone_with_result( + dict.py(), + unsafe { + ffi::compat::PyDict_SetDefaultRef( + dict.as_ptr(), + key.as_ptr(), + value.as_ptr(), + std::ptr::null_mut(), + ) + }, + )) } let py = self.py(); - Ok(setdefault_result_from_nonerror_return_code(inner( + inner( self, key.into_pyobject_or_pyerr(py)?.into_any().as_borrowed(), default_value .into_pyobject_or_pyerr(py)? .into_any() .as_borrowed(), - )?)) + ) } fn set_default_with_result( @@ -461,40 +452,47 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { K: IntoPyObject<'py>, V: IntoPyObject<'py>, { - fn inner( + fn inner<'py>( dict: &Bound<'_, PyDict>, key: Borrowed<'_, '_, PyAny>, value: Borrowed<'_, '_, PyAny>, - result: &mut *mut ffi::PyObject, - ) -> PyResult { - err::error_on_minusone_with_result(dict.py(), unsafe { - ffi::compat::PyDict_SetDefaultRef( - dict.as_ptr(), - key.as_ptr(), - value.as_ptr(), - result, - ) - }) + py: Python<'py>, + ) -> PyResult<(bool, Bound<'py, PyAny>)> { + let mut result = std::ptr::NonNull::dangling().as_ptr(); + let code = setdefault_result_from_nonerror_return_code( + err::error_on_minusone_with_result(dict.py(), unsafe { + ffi::compat::PyDict_SetDefaultRef( + dict.as_ptr(), + key.as_ptr(), + value.as_ptr(), + &mut result, + ) + }), + )?; + // SAFETY: the interpreter should have set this to a valid owned PyObject pointer + let out_result = unsafe { result.assume_owned_unchecked(py) }; + Ok((code, out_result)) } let py = self.py(); - static UNINITIALIZED: u8 = 0; - let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject; - let code = inner( + inner( self, key.into_pyobject_or_pyerr(py)?.into_any().as_borrowed(), default_value .into_pyobject_or_pyerr(py)? .into_any() .as_borrowed(), - &mut ob_result, - )?; - assert!(ob_result != std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject); - // SAFETY: the interpreter should have set this to a valid owned PyObject pointer - let out_result = unsafe { ob_result.assume_owned(py) }; - Ok(( - setdefault_result_from_nonerror_return_code(code), - out_result, - )) + py, + ) + } +} + +fn setdefault_result_from_nonerror_return_code(code: PyResult) -> PyResult { + match code? { + // inserted + 0 => Ok(true), + // not inserted + 1 => Ok(false), + x => panic!("Unknown return value from PyDict_SetDefaultRef: {x}"), } } @@ -1782,54 +1780,50 @@ mod tests { } #[test] - // Python 3.10 limited API is needed for .extract::<&str> - #[cfg(not(all(Py_LIMITED_API, not(Py_3_10))))] fn test_set_default() { Python::attach(|py| { let dict = PyDict::new(py); assert!(matches!(dict.set_default("hello", "world"), Ok(true))); - assert!( + assert_eq!( dict.get_item("hello") .unwrap() .unwrap() - .extract::<&str>() - .unwrap() - == "world" + .extract::() + .unwrap(), + "world" ); assert!(matches!(dict.set_default("hello", "foobar"), Ok(false))); + // unhashable let invalid_key = PyList::new(py, vec![0]).unwrap(); assert!(dict.set_default(invalid_key, "foobar").is_err()); }) } #[test] - // Python 3.10 is needed for .extract::<&str> - #[cfg(not(all(Py_LIMITED_API, not(Py_3_10))))] fn test_set_default_with_result() { Python::attach(|py| { let dict = PyDict::new(py); let res = dict.set_default_with_result("hello", "world"); assert!(res.is_ok()); - let res = res.unwrap(); - assert!(res.0); - assert!(res.1.extract::<&str>().unwrap() == "world"); + let (inserted, value) = res.unwrap(); + assert!(inserted); + assert!(value.extract::().unwrap() == "world"); assert!( dict.get_item("hello") .unwrap() .unwrap() - .extract::<&str>() + .extract::() .unwrap() == "world" ); - let res = dict.set_default_with_result("hello", "foobar"); - assert!(res.is_ok()); - let res = res.unwrap(); - assert!(!res.0); - assert!(res.1.extract::<&str>().unwrap() == "world"); + let (inserted, value) = dict.set_default_with_result("hello", "foobar").unwrap(); + assert!(inserted); + assert_eq!(value.extract::().unwrap(), "world"); + // unhashable let invalid_key = PyList::new(py, vec![0]).unwrap(); assert!(dict.set_default_with_result(invalid_key, "foobar").is_err()); }) From fb10a6841052ddc4bb4caaa7c83acd7997155afa Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 14 Apr 2026 09:47:17 -0600 Subject: [PATCH 11/11] typo fix --- src/types/dict.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 5cd39aa954b..aa5518b4c49 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -1820,7 +1820,7 @@ mod tests { ); let (inserted, value) = dict.set_default_with_result("hello", "foobar").unwrap(); - assert!(inserted); + assert!(!inserted); assert_eq!(value.extract::().unwrap(), "world"); // unhashable