Fix del obj.__dict__: improve GC safety, implement lazy re-creation in setattr, and enable passing CPython tests

alok-108 · alok-108 · commit d3cc9d436a56 · 2026-04-07T13:23:57.000+05:30
diff --git a/Lib/test/test_class.py b/Lib/test/test_class.py
@@ -910,7 +910,7 @@ def test_managed_dict_only_for_varsized_subclass(self):
         self.assertEqual(flags & Py_TPFLAGS_INLINE_VALUES, 0)
         self.assertFalse(has_inline_values(VarSizedSubclass()))
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON
+    # TODO: RUSTPYTHON
     def test_has_inline_values(self):
         c = Plain()
         self.assertTrue(has_inline_values(c))
@@ -978,7 +978,7 @@ def __init__(self):
         obj.foo = None # Aborted here
         self.assertEqual(obj.__dict__, {"foo":None})
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON
+    # TODO: RUSTPYTHON
     def test_store_attr_deleted_dict(self):
         class Foo:
             pass
@@ -988,7 +988,7 @@ class Foo:
         f.a = 3
         self.assertEqual(f.a, 3)
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON
+    # TODO: RUSTPYTHON
     def test_rematerialize_object_dict(self):
         # gh-121860: rematerializing an object's managed dictionary after it
         # had been deleted caused a crash.
diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs
@@ -307,9 +307,13 @@ pub(super) struct ObjExt {
 }
 
 impl ObjExt {
-    fn new(dict: Option<PyDictRef>, member_count: usize) -> Self {
+    fn new(dict: Option<PyDictRef>, member_count: usize, has_dict: bool) -> Self {
         Self {
-            dict: dict.map(InstanceDict::new),
+            dict: if has_dict {
+                Some(InstanceDict::from_opt(dict))
+            } else {
+                None
+            },
             slots: core::iter::repeat_with(|| PyRwLock::new(None))
                 .take(member_count)
                 .collect_vec()
@@ -928,6 +932,13 @@ impl InstanceDict {
         }
     }
 
+    #[inline]
+    pub const fn from_opt(d: Option<PyDictRef>) -> Self {
+        Self {
+            d: PyRwLock::new(d),
+        }
+    }
+
     #[inline]
     pub fn get(&self) -> Option<PyDictRef> {
         self.d.read().clone()
@@ -950,11 +961,14 @@ impl InstanceDict {
     }
 
     pub(crate) fn get_or_insert(&self, vm: &VirtualMachine) -> PyDictRef {
+        if let Some(existing) = self.d.read().as_ref() {
+            return existing.clone();
+        }
+        let dict = vm.ctx.new_dict();
         let mut d = self.d.write();
         if let Some(existing) = d.as_ref() {
             existing.clone()
         } else {
-            let dict = vm.ctx.new_dict();
             *d = Some(dict.clone());
             dict
         }
@@ -1071,7 +1085,8 @@ impl<T: PyPayload + core::fmt::Debug> PyInner<T> {
             unsafe {
                 if let Some(offset) = ext_start {
                     let ext_ptr = alloc_ptr.add(offset) as *mut ObjExt;
-                    ext_ptr.write(ObjExt::new(dict, member_count));
+                    let has_dict = typ.slots.flags.has_feature(crate::types::PyTypeFlags::HAS_DICT);
+                    ext_ptr.write(ObjExt::new(dict, member_count, has_dict));
                 }
 
                 if let Some(offset) = weakref_start {
@@ -1785,9 +1800,8 @@ impl PyObject {
             let ext_ptr =
                 core::ptr::with_exposed_provenance_mut::<ObjExt>(self_addr.wrapping_sub(offset));
             let ext = unsafe { &mut *ext_ptr };
-            if let Some(old_dict) = ext.dict.take() {
-                // Get the dict ref before dropping InstanceDict
-                if let Some(dict_ref) = old_dict.into_inner() {
+            if let Some(instance_dict) = &ext.dict {
+                if let Some(dict_ref) = instance_dict.replace(None) {
                     result.push(dict_ref.into());
                 }
             }
diff --git a/crates/vm/src/protocol/object.rs b/crates/vm/src/protocol/object.rs
@@ -199,17 +199,19 @@ impl PyObject {
             }
         }
 
-        if let Some(dict) = self.dict() {
+        if let Some(instance_dict) = self.instance_dict() {
             if let PySetterValue::Assign(value) = value {
-                dict.set_item(attr_name, value, vm)?;
-            } else {
+                instance_dict.get_or_insert(vm).set_item(attr_name, value, vm)?;
+            } else if let Some(dict) = instance_dict.get() {
                 dict.del_item(attr_name, vm).map_err(|e| {
                     if e.fast_isinstance(vm.ctx.exceptions.key_error) {
                         vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned())
                     } else {
                         e
                     }
                 })?;
+            } else {
+                return Err(vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned()));
             }
             Ok(())
         } else {
diff --git a/extra_tests/snippets/test_del_dict.py b/extra_tests/snippets/test_del_dict.py
@@ -0,0 +1,23 @@
+# Test that del obj.__dict__ works and lazy creation happens
+class C:
+    pass
+
+obj = C()
+obj.x = 42
+
+# Delete __dict__
+del obj.__dict__
+
+# After deletion, accessing __dict__ should return a new empty dict
+d = obj.__dict__
+assert isinstance(d, dict)
+assert len(d) == 0
+
+# Old attribute should be gone
+try:
+    obj.x
+    assert False, "AttributeError expected"
+except AttributeError:
+    pass
+
+print("OK")

Original file line number	Diff line number	Diff line change
`@@ -199,17 +199,19 @@ impl PyObject {`
`199`	`199`	`}`
`200`	`200`	`}`
`201`	`201`
`202`		`- if let Some(dict) = self.dict() {`
	`202`	`+ if let Some(instance_dict) = self.instance_dict() {`
`203`	`203`	`if let PySetterValue::Assign(value) = value {`
`204`		`- dict.set_item(attr_name, value, vm)?;`
`205`		`- } else {`
	`204`	`+ instance_dict.get_or_insert(vm).set_item(attr_name, value, vm)?;`
	`205`	`+ } else if let Some(dict) = instance_dict.get() {`
`206`	`206`	`dict.del_item(attr_name, vm).map_err(\|e\| {`
`207`	`207`	`if e.fast_isinstance(vm.ctx.exceptions.key_error) {`
`208`	`208`	`vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned())`
`209`	`209`	`} else {`
`210`	`210`	`e`
`211`	`211`	`}`
`212`	`212`	`})?;`
	`213`	`+ } else {`
	`214`	`+ return Err(vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned()));`
`213`	`215`	`}`
`214`	`216`	`Ok(())`
`215`	`217`	`} else {`