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>
PyO3 · Jun 16, 2024 · 0b2f19b · 0b2f19b
1 parent 591cdb0
commit 0b2f19b
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 27 deletions.
diff --git a/newsfragments/4251.fixed.md 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
@@ -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::<EmptyClass>()?;
     m.add_class::<PyClassIter>()?;
     m.add_class::<AssertingBaseClass>()?;
     m.add_class::<ClassWithoutConstructor>()?;
+    m.add_class::<ClassWithDict>()?;
 
     Ok(())
 }
diff --git 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
@@ -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<ffi::Py_ssize_t>,
     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::<PyResult<_>>()?;
 
-        // 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::<u8>()
+                                .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<ffi::Py_ssize_t>,
         #[allow(unused_variables)] weaklist_offset: Option<ffi::Py_ssize_t>,
     ) -> 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
@@ -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(