From a3d13234f43f82719dad8fb3049fd2a220fa469d Mon Sep 17 00:00:00 2001 From: David Hewitt <mail@davidhewitt.dev> Date: Fri, 16 Aug 2024 15:27:19 +0100 Subject: [PATCH 1/5] ffi: define compat for `Py_NewRef` and `Py_XNewRef` --- newsfragments/4445.added.md | 1 + newsfragments/4445.removed.md | 1 + pyo3-ffi/src/compat.rs | 60 ------------------------------- pyo3-ffi/src/compat/mod.rs | 39 ++++++++++++++++++++ pyo3-ffi/src/compat/py_3_10.rs | 18 ++++++++++ pyo3-ffi/src/compat/py_3_13.rs | 39 ++++++++++++++++++++ pyo3-ffi/src/object.rs | 27 ++++++-------- src/pyclass/create_type_object.rs | 2 +- 8 files changed, 109 insertions(+), 78 deletions(-) create mode 100644 newsfragments/4445.added.md create mode 100644 newsfragments/4445.removed.md delete mode 100644 pyo3-ffi/src/compat.rs create mode 100644 pyo3-ffi/src/compat/mod.rs create mode 100644 pyo3-ffi/src/compat/py_3_10.rs create mode 100644 pyo3-ffi/src/compat/py_3_13.rs diff --git a/newsfragments/4445.added.md b/newsfragments/4445.added.md new file mode 100644 index 00000000000..ee6af52d97e --- /dev/null +++ b/newsfragments/4445.added.md @@ -0,0 +1 @@ +Add FFI definitions `compat::Py_NewRef` and `compat::Py_XNewRef`. diff --git a/newsfragments/4445.removed.md b/newsfragments/4445.removed.md new file mode 100644 index 00000000000..b5b7e450331 --- /dev/null +++ b/newsfragments/4445.removed.md @@ -0,0 +1 @@ +Remove private FFI definitions `_Py_NewRef` and `_Py_XNewRef`. diff --git a/pyo3-ffi/src/compat.rs b/pyo3-ffi/src/compat.rs deleted file mode 100644 index 85986817d93..00000000000 --- a/pyo3-ffi/src/compat.rs +++ /dev/null @@ -1,60 +0,0 @@ -//! 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 crate::pyport::Py_ssize_t; -#[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 - } -} - -#[cfg_attr(docsrs, doc(cfg(all())))] -#[cfg(Py_3_13)] -pub use crate::PyList_GetItemRef; - -#[cfg(not(Py_3_13))] -#[cfg_attr(docsrs, doc(cfg(all())))] -pub unsafe fn PyList_GetItemRef(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject { - use crate::{PyList_GetItem, Py_XINCREF}; - - let item: *mut PyObject = PyList_GetItem(arg1, arg2); - Py_XINCREF(item); - item -} diff --git a/pyo3-ffi/src/compat/mod.rs b/pyo3-ffi/src/compat/mod.rs new file mode 100644 index 00000000000..fd8dc077001 --- /dev/null +++ b/pyo3-ffi/src/compat/mod.rs @@ -0,0 +1,39 @@ +//! 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 + +/// Internal helper macro which defines compatibility shims for C API functions, deferring to a +/// re-export when that's available. +macro_rules! compat_function { + ( + originally_defined_for($cfg:meta); + + $(#[$attrs:meta])* + pub unsafe fn $name:ident($($arg_names:ident: $arg_types:ty),* $(,)?) -> $ret:ty $body:block + ) => { + // Define as a standalone function under docsrs cfg so that this shows as a unique function in the docs, + // not a re-export (the re-export has the wrong visibility) + #[cfg(any(docsrs, not($cfg)))] + #[cfg_attr(docsrs, doc(cfg(all())))] + $(#[$attrs])* + pub unsafe fn $name( + $($arg_names: $arg_types,)* + ) -> $ret $body + + #[cfg(all($cfg, not(docsrs)))] + pub use $crate::$name; + }; +} + +mod py_3_10; +mod py_3_13; + +pub use self::py_3_10::*; +pub use self::py_3_13::*; diff --git a/pyo3-ffi/src/compat/py_3_10.rs b/pyo3-ffi/src/compat/py_3_10.rs new file mode 100644 index 00000000000..2a92fc3b1da --- /dev/null +++ b/pyo3-ffi/src/compat/py_3_10.rs @@ -0,0 +1,18 @@ +compat_function!( + originally_defined_for(Py_3_10); + + pub unsafe fn Py_NewRef(obj: *mut crate::PyObject) -> *mut crate::PyObject { + crate::Py_INCREF(obj); + obj + } +); + +compat_function!( + originally_defined_for(Py_3_10); + + #[inline] + pub unsafe fn Py_XNewRef(obj: *mut crate::PyObject) -> *mut crate::PyObject { + crate::Py_XINCREF(obj); + obj + } +); diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs new file mode 100644 index 00000000000..f771b853d77 --- /dev/null +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -0,0 +1,39 @@ +compat_function!( + originally_defined_for(Py_3_13); + + #[inline] + pub unsafe fn PyDict_GetItemRef( + dp: *mut crate::PyObject, + key: *mut crate::PyObject, + result: *mut *mut crate::PyObject, + ) -> std::ffi::c_int { + use crate::{compat::Py_NewRef, PyDict_GetItemWithError, PyErr_Occurred}; + + let item = 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 + } +); + +compat_function!( + originally_defined_for(Py_3_13); + + #[inline] + pub unsafe fn PyList_GetItemRef( + arg1: *mut crate::PyObject, + arg2: crate::Py_ssize_t, + ) -> *mut crate::PyObject { + use crate::{PyList_GetItem, Py_XINCREF}; + + let item = PyList_GetItem(arg1, arg2); + Py_XINCREF(item); + item + } +); diff --git a/pyo3-ffi/src/object.rs b/pyo3-ffi/src/object.rs index 9181cb17dd2..4408ef38b5e 100644 --- a/pyo3-ffi/src/object.rs +++ b/pyo3-ffi/src/object.rs @@ -648,37 +648,30 @@ pub unsafe fn Py_XDECREF(op: *mut PyObject) { extern "C" { #[cfg(all(Py_3_10, Py_LIMITED_API))] + #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] pub fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject; #[cfg(all(Py_3_10, Py_LIMITED_API))] + #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] pub fn Py_XNewRef(obj: *mut PyObject) -> *mut PyObject; } -// Technically these macros are only available in the C header from 3.10 and up, however their -// implementation works on all supported Python versions so we define these macros on all -// versions for simplicity. - -#[inline] -pub unsafe fn _Py_NewRef(obj: *mut PyObject) -> *mut PyObject { - Py_INCREF(obj); - obj -} - -#[inline] -pub unsafe fn _Py_XNewRef(obj: *mut PyObject) -> *mut PyObject { - Py_XINCREF(obj); - obj -} +// macro _Py_NewRef not public; reimplemented directly inside Py_NewRef here +// macro _Py_XNewRef not public; reimplemented directly inside Py_XNewRef here #[cfg(all(Py_3_10, not(Py_LIMITED_API)))] +#[cfg_attr(docsrs, doc(cfg(Py_3_10)))] #[inline] pub unsafe fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject { - _Py_NewRef(obj) + Py_INCREF(obj); + obj } #[cfg(all(Py_3_10, not(Py_LIMITED_API)))] +#[cfg_attr(docsrs, doc(cfg(Py_3_10)))] #[inline] pub unsafe fn Py_XNewRef(obj: *mut PyObject) -> *mut PyObject { - _Py_XNewRef(obj) + Py_XINCREF(obj); + obj } #[cfg_attr(windows, link(name = "pythonXY"))] diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index ac5b6287e86..b05d164e2b0 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -250,7 +250,7 @@ impl PyTypeBuilder { if (*dict_ptr).is_null() { std::ptr::write(dict_ptr, ffi::PyDict_New()); } - Ok(ffi::_Py_XNewRef(*dict_ptr)) + Ok(ffi::compat::Py_XNewRef(*dict_ptr)) }) } } From 9db87b912d4cfea7bd5378720c4e5d64858b18a5 Mon Sep 17 00:00:00 2001 From: David Hewitt <mail@davidhewitt.dev> Date: Fri, 16 Aug 2024 21:19:34 +0100 Subject: [PATCH 2/5] add missing inline hint Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com> --- pyo3-ffi/src/compat/py_3_10.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/pyo3-ffi/src/compat/py_3_10.rs b/pyo3-ffi/src/compat/py_3_10.rs index 2a92fc3b1da..463131e2e1d 100644 --- a/pyo3-ffi/src/compat/py_3_10.rs +++ b/pyo3-ffi/src/compat/py_3_10.rs @@ -1,6 +1,7 @@ compat_function!( originally_defined_for(Py_3_10); +#[inline] pub unsafe fn Py_NewRef(obj: *mut crate::PyObject) -> *mut crate::PyObject { crate::Py_INCREF(obj); obj From 4080bb3680eabb3e0b8ab6e6c866c125482afc49 Mon Sep 17 00:00:00 2001 From: David Hewitt <mail@davidhewitt.dev> Date: Fri, 16 Aug 2024 21:39:12 +0100 Subject: [PATCH 3/5] don't use std::ffi::c_int (requires MSRV 1.64) --- pyo3-ffi/src/compat/py_3_13.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyo3-ffi/src/compat/py_3_13.rs b/pyo3-ffi/src/compat/py_3_13.rs index f771b853d77..75c4cd101ae 100644 --- a/pyo3-ffi/src/compat/py_3_13.rs +++ b/pyo3-ffi/src/compat/py_3_13.rs @@ -6,7 +6,7 @@ compat_function!( dp: *mut crate::PyObject, key: *mut crate::PyObject, result: *mut *mut crate::PyObject, - ) -> std::ffi::c_int { + ) -> std::os::raw::c_int { use crate::{compat::Py_NewRef, PyDict_GetItemWithError, PyErr_Occurred}; let item = PyDict_GetItemWithError(dp, key); From 4d7c8ff48a00f8e5c7aae911eb650fbd017a006e Mon Sep 17 00:00:00 2001 From: David Hewitt <mail@davidhewitt.dev> Date: Fri, 16 Aug 2024 23:01:36 +0100 Subject: [PATCH 4/5] add test to guard against ambiguity --- pyo3-ffi/Cargo.toml | 3 +++ pyo3-ffi/src/compat/mod.rs | 18 ++++++++++++++++++ pyo3-ffi/src/compat/py_3_10.rs | 2 +- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/pyo3-ffi/Cargo.toml b/pyo3-ffi/Cargo.toml index 8e1b203fcd1..0e58197bbfd 100644 --- a/pyo3-ffi/Cargo.toml +++ b/pyo3-ffi/Cargo.toml @@ -37,6 +37,9 @@ abi3-py312 = ["abi3", "pyo3-build-config/abi3-py312"] # Automatically generates `python3.dll` import libraries for Windows targets. generate-import-lib = ["pyo3-build-config/python3-dll-a"] +[dev-dependencies] +paste = "1" + [build-dependencies] pyo3-build-config = { path = "../pyo3-build-config", version = "=0.23.0-dev", features = ["resolve-config"] } diff --git a/pyo3-ffi/src/compat/mod.rs b/pyo3-ffi/src/compat/mod.rs index fd8dc077001..db41c834b3d 100644 --- a/pyo3-ffi/src/compat/mod.rs +++ b/pyo3-ffi/src/compat/mod.rs @@ -29,6 +29,24 @@ macro_rules! compat_function { #[cfg(all($cfg, not(docsrs)))] pub use $crate::$name; + + #[cfg(test)] + paste::paste! { + // Test that the compat function does not overlap with the original function. If the + // cfgs line up, then the the two glob imports will resolve to the same item via the + // re-export. If the cfgs mismatch, then the use of $name will be ambiguous in cases + // where the function is defined twice, and the test will fail to compile. + #[allow(unused_imports)] + mod [<test_ $name _export>] { + use $crate::*; + use $crate::compat::*; + + #[test] + fn test_export() { + let _ = $name; + } + } + } }; } diff --git a/pyo3-ffi/src/compat/py_3_10.rs b/pyo3-ffi/src/compat/py_3_10.rs index 463131e2e1d..c6e8c2cb5ca 100644 --- a/pyo3-ffi/src/compat/py_3_10.rs +++ b/pyo3-ffi/src/compat/py_3_10.rs @@ -1,7 +1,7 @@ compat_function!( originally_defined_for(Py_3_10); -#[inline] + #[inline] pub unsafe fn Py_NewRef(obj: *mut crate::PyObject) -> *mut crate::PyObject { crate::Py_INCREF(obj); obj From cd7e53ebfa11be00b159047bd079c70dfc552625 Mon Sep 17 00:00:00 2001 From: David Hewitt <mail@davidhewitt.dev> Date: Sat, 17 Aug 2024 08:03:52 +0100 Subject: [PATCH 5/5] fix `Py_NewRef` cfg on PyPy --- pyo3-ffi/src/object.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pyo3-ffi/src/object.rs b/pyo3-ffi/src/object.rs index 4408ef38b5e..73b07ec2cf2 100644 --- a/pyo3-ffi/src/object.rs +++ b/pyo3-ffi/src/object.rs @@ -647,10 +647,10 @@ pub unsafe fn Py_XDECREF(op: *mut PyObject) { } extern "C" { - #[cfg(all(Py_3_10, Py_LIMITED_API))] + #[cfg(all(Py_3_10, Py_LIMITED_API, not(PyPy)))] #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] pub fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject; - #[cfg(all(Py_3_10, Py_LIMITED_API))] + #[cfg(all(Py_3_10, Py_LIMITED_API, not(PyPy)))] #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] pub fn Py_XNewRef(obj: *mut PyObject) -> *mut PyObject; } @@ -658,7 +658,7 @@ extern "C" { // macro _Py_NewRef not public; reimplemented directly inside Py_NewRef here // macro _Py_XNewRef not public; reimplemented directly inside Py_XNewRef here -#[cfg(all(Py_3_10, not(Py_LIMITED_API)))] +#[cfg(all(Py_3_10, any(not(Py_LIMITED_API), PyPy)))] #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] #[inline] pub unsafe fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject { @@ -666,7 +666,7 @@ pub unsafe fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject { obj } -#[cfg(all(Py_3_10, not(Py_LIMITED_API)))] +#[cfg(all(Py_3_10, any(not(Py_LIMITED_API), PyPy)))] #[cfg_attr(docsrs, doc(cfg(Py_3_10)))] #[inline] pub unsafe fn Py_XNewRef(obj: *mut PyObject) -> *mut PyObject {