Add bindings for PyDict_SetDefaultRef and use them in PyCode::run by ngoldbaum · Pull Request #5955 · PyO3/pyo3

ngoldbaum · 2026-04-06T18:48:04Z

While experimenting with a build of PyO3 that supports PEP 803 without using critical sections, I noticed that the use in PyCode::run can be replaced by PyDict_SetDefaultRef. This also makes the implementation a lot simpler.

I added bindings for that function on 3.13 and newer and 3.15 and newer in the limited API. I also added a compat shim based on the implementation in pythoncapi-compat: https://github.com/python/pythoncapi-compat/blob/75a796764d63327268d195e2d5c044e564d0dada/pythoncapi_compat.h#L1314.

ngoldbaum · 2026-04-06T21:22:08Z

It looks like we don't have extensive coverage tests for our compat shims. Maybe we should? I'd prefer not to add infrastructure for that in this PR though.

davidhewitt

Thanks, one thought on the usage of this.

Indeed we don't have full coverage of the compat shims, generally coverage of code in pyo3-ffi is quite low because it's hard to test the FFI code without the convenience layers in pyo3.

We have some tests in src/ffi/tests inside the pyo3 lib, we should probably expand those (maybe warrants an issue?).

For this particular case, maybe we should add setdefault to PyDictMethods? (We could then use that in PyCode::run for bonus safety.)

src/types/code.rs

ngoldbaum · 2026-04-13T18:45:22Z

For this particular case, maybe we should add setdefault to PyDictMethods? (We could then use that in PyCode::run for bonus safety.)

Thanks for the suggestion! I added PyDictMethods::{set_default, set_default_with_result}. Please feel free to bikeshed the API I came up with. IMO it makes more sense for the "returns the value" variant to be its own function. I also might push some fixup commits if the CI fails.

davidhewitt

Thanks, various suggestions about the new functions

davidhewitt · 2026-04-14T08:11:15Z

src/types/dict.rs

    }
 }

+fn setdefault_result_from_nonerror_return_code(code: std::ffi::c_int) -> bool {


Nit, possibly move this down in the file below the method implementations, not C where we need a forward reference 😉

davidhewitt · 2026-04-14T08:13:22Z

src/types/dict.rs

+    /// 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.


Is the thread safety note particularly useful for users? It seems like, following the docs, it's basically. "This function is thread safe, which is generally the case for all the APIs we offer."

davidhewitt · 2026-04-14T08:20:50Z

src/types/dict.rs

+        }
+        let py = self.py();
+
+        Ok(setdefault_result_from_nonerror_return_code(inner(


I suggest moving the call to setdefault_result_from_nonerror_return_code inside inner - it doesn't depend on the generic parameters.

davidhewitt · 2026-04-14T08:22:56Z

src/types/dict.rs

+        static UNINITIALIZED: u8 = 0;
+        let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject;


One option is to do this with MaybeUninit, though that would forbid being able to assert the call initialized the result.

Suggested change

static UNINITIALIZED: u8 = 0;

let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject;

let mut ob_result = MaybeUninit::new();

davidhewitt · 2026-04-14T08:27:08Z

src/types/dict.rs

+                .as_borrowed(),
+            &mut ob_result,
+        )?;
+        assert!(ob_result != std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject);


Is the assert necessary? Seems like this effectively defends a bad implementation of the C API, which is an upstream bug and not our fault if incorrect?

davidhewitt · 2026-04-14T08:27:41Z

src/types/dict.rs

+        )?;
+        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) };


Could use assume_owned_unchecked(py) here to avoid a branch, given the safety guarantees.

davidhewitt · 2026-04-14T08:28:11Z

src/types/dict.rs

+        let py = self.py();
+        static UNINITIALIZED: u8 = 0;
+        let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject;
+        let code = inner(


Similar comment that a lot of the return value handling should be moved into inner as it doesn't depend on the generic parameters.

davidhewitt · 2026-04-14T08:29:43Z

src/types/dict.rs

+            let res = dict.set_default_with_result("hello", "foobar");
+            assert!(res.is_ok());
+            let res = res.unwrap();


Given that the .unwrap() will panic, I'd probably just write this as a one-line

Suggested change

let res = dict.set_default_with_result("hello", "foobar");

assert!(res.is_ok());

let res = res.unwrap();

let (inserted, value) = dict.set_default_with_result("hello", "foobar").unwrap();

davidhewitt · 2026-04-14T08:31:11Z

src/types/dict.rs

+            assert!(res.is_ok());
+            let res = res.unwrap();
+            assert!(!res.0);
+            assert!(res.1.extract::<&str>().unwrap() == "world");


Potentially just extract to String to eat the allocation but avoid the need for a feature gate on the test? Not really a perf concern here.

Also looks like this could be done with assert_eq!.

Suggested change

assert!(res.1.extract::<&str>().unwrap() == "world");

assert_eq!(res.1.extract::<String>().unwrap(), "world");

davidhewitt · 2026-04-14T08:32:06Z

src/types/dict.rs

    }
+
+    #[test]
+    // Python 3.10 limited API is needed for .extract::<&str>


Same comment about maybe using String across all the extract points to remove the feature gates.

ngoldbaum added 2 commits April 6, 2026 14:05

Add bindings for PyDict_SetDefaultRef and use them in PyCode::run

74a1dab

fix conditional compilation

13a0dc4

ngoldbaum force-pushed the globals-set-default branch from 69e4dab to 13a0dc4 Compare April 6, 2026 20:07

ngoldbaum added 3 commits April 6, 2026 14:13

simplify conditional predicates

8e14f5d

minor touchups

83c374f

add changelog entries

81325e3

ngoldbaum marked this pull request as ready for review April 6, 2026 20:33

ngoldbaum added FFI refactoring labels Apr 6, 2026

Icxolu mentioned this pull request Apr 11, 2026

Opaque PyObject ABI support #5807

Open

davidhewitt reviewed Apr 11, 2026

View reviewed changes

src/types/code.rs Outdated Show resolved Hide resolved

ngoldbaum mentioned this pull request Apr 13, 2026

Expand test coverage for FFI compat shims #5969

Open

ngoldbaum added 2 commits April 13, 2026 09:24

pass NULL for the result

5010efb

Add new PyDictMethods API wrapping PyDict_SetDefaultRef

de76ca2

ngoldbaum added 2 commits April 13, 2026 13:07

gate new test for 3.9 and older limited API

583362b

fix clippy and typos

5fcc1d8

davidhewitt reviewed Apr 14, 2026

View reviewed changes

		static UNINITIALIZED: u8 = 0;
		let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject;

	static UNINITIALIZED: u8 = 0;
	let mut ob_result = std::ptr::addr_of!(UNINITIALIZED) as *mut ffi::PyObject;
	let mut ob_result = MaybeUninit::new();

	assert!(res.1.extract::<&str>().unwrap() == "world");
	assert_eq!(res.1.extract::<String>().unwrap(), "world");

Conversation

ngoldbaum commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Apr 6, 2026

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngoldbaum commented Apr 13, 2026

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ngoldbaum commented Apr 6, 2026 •

edited

Loading