From 0b2f19b3c9bb6aca0a6ca9316a9f8afeb25a0cfc Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 16 Jun 2024 08:57:44 +0100 Subject: [PATCH] fix `__dict__` on Python 3.9 with limited API (#4251) * fix `__dict__` on Python 3.9 with limited API * [review] Icxolu suggestions Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> * [review] Icxolu * missing import --------- Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> --- newsfragments/4251.fixed.md | 1 + pytests/src/pyclasses.rs | 12 +++++ pytests/tests/test_pyclasses.py | 8 ++++ src/pyclass/create_type_object.rs | 75 ++++++++++++++++++++++--------- tests/test_class_basics.rs | 13 +++--- 5 files changed, 82 insertions(+), 27 deletions(-) create mode 100644 newsfragments/4251.fixed.md diff --git a/newsfragments/4251.fixed.md b/newsfragments/4251.fixed.md new file mode 100644 index 00000000000..5cc23c7a126 --- /dev/null +++ b/newsfragments/4251.fixed.md @@ -0,0 +1 @@ +Fix `__dict__` attribute missing for `#[pyclass(dict)]` instances when building for `abi3` on Python 3.9. diff --git a/pytests/src/pyclasses.rs b/pytests/src/pyclasses.rs index 6338596b481..f7e4681af70 100644 --- a/pytests/src/pyclasses.rs +++ b/pytests/src/pyclasses.rs @@ -66,12 +66,24 @@ impl AssertingBaseClass { #[pyclass] struct ClassWithoutConstructor; +#[pyclass(dict)] +struct ClassWithDict; + +#[pymethods] +impl ClassWithDict { + #[new] + fn new() -> Self { + ClassWithDict + } +} + #[pymodule] pub fn pyclasses(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; Ok(()) } diff --git a/pytests/tests/test_pyclasses.py b/pytests/tests/test_pyclasses.py index efef178d489..0c336ecf2e7 100644 --- a/pytests/tests/test_pyclasses.py +++ b/pytests/tests/test_pyclasses.py @@ -84,3 +84,11 @@ def test_no_constructor_defined_propagates_cause(cls: Type): assert exc_info.type is TypeError assert exc_info.value.args == ("No constructor defined",) assert exc_info.value.__context__ is original_error + + +def test_dict(): + d = pyclasses.ClassWithDict() + assert d.__dict__ == {} + + d.foo = 42 + assert d.__dict__ == {"foo": 42} diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index 262d1e8ffc7..01b357763ad 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -1,5 +1,3 @@ -use pyo3_ffi::PyType_IS_GC; - use crate::{ exceptions::PyTypeError, ffi, @@ -68,7 +66,7 @@ where has_setitem: false, has_traverse: false, has_clear: false, - has_dict: false, + dict_offset: None, class_flags: 0, #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] buffer_procs: Default::default(), @@ -121,7 +119,7 @@ struct PyTypeBuilder { has_setitem: bool, has_traverse: bool, has_clear: bool, - has_dict: bool, + dict_offset: Option, class_flags: c_ulong, // Before Python 3.9, need to patch in buffer methods manually (they don't work in slots) #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] @@ -218,16 +216,56 @@ impl PyTypeBuilder { }) .collect::>()?; - // PyPy doesn't automatically add __dict__ getter / setter. - // PyObject_GenericGetDict not in the limited API until Python 3.10. - if self.has_dict { - #[cfg(not(any(PyPy, all(Py_LIMITED_API, not(Py_3_10)))))] + // PyPy automatically adds __dict__ getter / setter. + #[cfg(not(PyPy))] + // Supported on unlimited API for all versions, and on 3.9+ for limited API + #[cfg(any(Py_3_9, not(Py_LIMITED_API)))] + if let Some(dict_offset) = self.dict_offset { + let get_dict; + let closure; + // PyObject_GenericGetDict not in the limited API until Python 3.10. + #[cfg(any(not(Py_LIMITED_API), Py_3_10))] + { + let _ = dict_offset; + get_dict = ffi::PyObject_GenericGetDict; + closure = ptr::null_mut(); + } + + // ... so we write a basic implementation ourselves + #[cfg(not(any(not(Py_LIMITED_API), Py_3_10)))] + { + extern "C" fn get_dict_impl( + object: *mut ffi::PyObject, + closure: *mut c_void, + ) -> *mut ffi::PyObject { + unsafe { + trampoline(|_| { + let dict_offset = closure as ffi::Py_ssize_t; + // we don't support negative dict_offset here; PyO3 doesn't set it negative + assert!(dict_offset > 0); + // TODO: use `.byte_offset` on MSRV 1.75 + let dict_ptr = object + .cast::() + .offset(dict_offset) + .cast::<*mut ffi::PyObject>(); + if (*dict_ptr).is_null() { + std::ptr::write(dict_ptr, ffi::PyDict_New()); + } + Ok(ffi::_Py_XNewRef(*dict_ptr)) + }) + } + } + + get_dict = get_dict_impl; + closure = dict_offset as _; + } + property_defs.push(ffi::PyGetSetDef { name: "__dict__\0".as_ptr().cast(), - get: Some(ffi::PyObject_GenericGetDict), + get: Some(get_dict), set: Some(ffi::PyObject_GenericSetDict), doc: ptr::null(), - closure: ptr::null_mut(), + closure, }); } @@ -315,20 +353,17 @@ impl PyTypeBuilder { dict_offset: Option, #[allow(unused_variables)] weaklist_offset: Option, ) -> Self { - self.has_dict = dict_offset.is_some(); + self.dict_offset = dict_offset; #[cfg(Py_3_9)] { #[inline(always)] - fn offset_def( - name: &'static str, - offset: ffi::Py_ssize_t, - ) -> ffi::structmember::PyMemberDef { - ffi::structmember::PyMemberDef { - name: name.as_ptr() as _, - type_code: ffi::structmember::T_PYSSIZET, + fn offset_def(name: &'static str, offset: ffi::Py_ssize_t) -> ffi::PyMemberDef { + ffi::PyMemberDef { + name: name.as_ptr().cast(), + type_code: ffi::Py_T_PYSSIZET, offset, - flags: ffi::structmember::READONLY, + flags: ffi::Py_READONLY, doc: std::ptr::null_mut(), } } @@ -391,7 +426,7 @@ impl PyTypeBuilder { unsafe { self.push_slot(ffi::Py_tp_new, no_constructor_defined as *mut c_void) } } - let tp_dealloc = if self.has_traverse || unsafe { PyType_IS_GC(self.tp_base) == 1 } { + let tp_dealloc = if self.has_traverse || unsafe { ffi::PyType_IS_GC(self.tp_base) == 1 } { self.tp_dealloc_with_gc } else { self.tp_dealloc diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 325b3d52c3d..bc8d2dab275 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -435,7 +435,7 @@ struct DunderDictSupport { } #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_9)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn dunder_dict_support() { Python::with_gil(|py| { let inst = Py::new( @@ -456,9 +456,8 @@ fn dunder_dict_support() { }); } -// Accessing inst.__dict__ only supported in limited API from Python 3.10 #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_10)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn access_dunder_dict() { Python::with_gil(|py| { let inst = Py::new( @@ -486,7 +485,7 @@ struct InheritDict { } #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_9)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn inherited_dict() { Python::with_gil(|py| { let inst = Py::new( @@ -517,7 +516,7 @@ struct WeakRefDunderDictSupport { } #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_9)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn weakref_dunder_dict_support() { Python::with_gil(|py| { let inst = Py::new( @@ -541,7 +540,7 @@ struct WeakRefSupport { } #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_9)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn weakref_support() { Python::with_gil(|py| { let inst = Py::new( @@ -566,7 +565,7 @@ struct InheritWeakRef { } #[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_9)), ignore)] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] fn inherited_weakref() { Python::with_gil(|py| { let inst = Py::new(