Fix del obj.__dict__ to match CPython behavior (fixes #5355) (#7568)

* Fix del obj.__dict__ to match CPython behavior (issue #5355)

* Address CodeRabbit concerns: fix GC clearing and improve thread safety of lazy __dict__ recreation

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

* Restore expectedFailure for test_has_inline_values

* Fix ObjExt::new call site to include has_dict parameter

* Remove stray test.py to avoid CI syntax errors

* Remove debug txt files and clean test_class.py comments

* Delete Lib/test/test_class.py

* Restore test_class.py with correct changes (remove expectedFailure, no deletion)

* Fix clippy warnings: remove unused into_inner, collapse nested if-let

* Fix rustfmt formatting and ruff PEP 8 E302 blank line

* Align __dict__ error messages and ensure safety for function/partial objects

* Fix compilation errors: change &self to &Py<Self> in __dict__ methods

* Fix compilation errors: resolve borrow-after-move and replace transpose on PySetterValue

- type.rs: Replace invalid .transpose() on PySetterValue with explicit
  match on Assign/Delete variants in subtype_set_dict
- function.rs: Fix borrow-after-move in set___dict__ by capturing class
  name before downcast; use as_object() for instance_dict/set_dict calls
- _functools.rs: Same borrow-after-move fix and as_object() calls for
  PyPartial's __dict__ getter/setter

* Fix compilation errors: resolve borrow-after-move and replace transpose on PySetterValue

* Fix snippet formatting and mark test_remote as expected failure

* Fix test_remote by removing HAS_DICT flag from function type

* Fix lint formatting error

* Remove unnecessary print statement in test_del_dict

* Fix trailing newlines in snippet test

* Trigger CI

* Align __dict__ generic setter behavior

* Move __dict__ deletion tests to relevant snippets

---------

Co-authored-by: Jeong, YunWon <jeong@youknowone.org>

Lib/test/test_class.py

@@ -978,7 +978,6 @@ class TestInlineValues(unittest.TestCase):
         obj.foo = None # Aborted here
         self.assertEqual(obj.__dict__, {"foo":None})
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON
     def test_store_attr_deleted_dict(self):
         class Foo:
             pass
@@ -988,7 +987,6 @@ class TestInlineValues(unittest.TestCase):
         f.a = 3
         self.assertEqual(f.a, 3)
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON
     def test_rematerialize_object_dict(self):
         # gh-121860: rematerializing an object's managed dictionary after it
         # had been deleted caused a crash.

crates/vm/src/builtins/builtin_func.rs

@@ -128,7 +128,7 @@ impl Representable for PyNativeFunction {
 
 #[pyclass(
     with(Callable, Comparable, Representable),
-    flags(HAS_DICT, HAS_WEAKREF, DISALLOW_INSTANTIATION)
+    flags(HAS_WEAKREF, DISALLOW_INSTANTIATION)
 )]
 impl PyNativeFunction {
     #[pygetset]
@@ -209,7 +209,8 @@ pub struct PyNativeMethod {
 
 // All Python-visible behavior (getters, slots) is registered by PyNativeFunction::extend_class.
 // PyNativeMethod only extends the Rust-side struct with the defining class reference.
-#[pyclass(flags(HAS_DICT, HAS_WEAKREF, DISALLOW_INSTANTIATION))]
+// The func field at offset 0 (#[repr(C)]) allows NativeFunctionOrMethod to read it safely.
+#[pyclass(flags(HAS_WEAKREF, DISALLOW_INSTANTIATION))]
 impl PyNativeMethod {}
 
 impl fmt::Debug for PyNativeMethod {

crates/vm/src/builtins/function.rs

@@ -3,7 +3,7 @@ mod jit;
 
 use super::{
     PyAsyncGen, PyCode, PyCoroutine, PyDictRef, PyGenerator, PyModule, PyStr, PyStrRef, PyTuple,
-    PyTupleRef, PyType,
+    PyTupleRef, PyType, object,
 };
 use crate::common::hash::PyHash;
 use crate::common::lock::PyMutex;
@@ -954,6 +954,16 @@ impl PyFunction {
         Ok(())
     }
 
+    #[pygetset]
+    fn __dict__(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyDictRef> {
+        object::object_get_dict(zelf.as_object().to_owned(), vm)
+    }
+
+    #[pygetset(setter)]
+    fn set___dict__(zelf: &Py<Self>, value: PySetterValue, vm: &VirtualMachine) -> PyResult<()> {
+        object::object_generic_set_dict(zelf.as_object().to_owned(), value, vm)
+    }
+
     #[pygetset]
     fn __annotate__(&self, vm: &VirtualMachine) -> PyObjectRef {
         self.annotate

crates/vm/src/builtins/object.rs

@@ -1,4 +1,4 @@
-use super::{PyDictRef, PyList, PyStr, PyStrRef, PyType, PyTypeRef, PyUtf8StrRef};
+use super::{PyDict, PyDictRef, PyList, PyStr, PyStrRef, PyType, PyTypeRef, PyUtf8StrRef};
 use crate::common::hash::PyHash;
 use crate::types::PyTypeFlags;
 use crate::{
@@ -554,14 +554,48 @@ impl PyBaseObject {
 }
 
 pub fn object_get_dict(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<PyDictRef> {
-    obj.dict()
-        .ok_or_else(|| vm.new_attribute_error("This object has no __dict__"))
+    if let Some(dict) = obj.dict() {
+        Ok(dict)
+    } else {
+        match obj.instance_dict() {
+            Some(d) => Ok(d.get_or_insert(vm)),
+            None => Err(vm.new_attribute_error("This object has no __dict__")),
+        }
+    }
 }
-pub fn object_set_dict(obj: PyObjectRef, dict: PyDictRef, vm: &VirtualMachine) -> PyResult<()> {
+pub fn object_set_dict(
+    obj: PyObjectRef,
+    value: PySetterValue<PyDictRef>,
+    vm: &VirtualMachine,
+) -> PyResult<()> {
+    let dict = match value {
+        PySetterValue::Assign(dict) => Some(dict),
+        PySetterValue::Delete => None,
+    };
     obj.set_dict(dict)
         .map_err(|_| vm.new_attribute_error("This object has no __dict__"))
 }
 
+pub fn object_generic_set_dict(
+    obj: PyObjectRef,
+    value: PySetterValue,
+    vm: &VirtualMachine,
+) -> PyResult<()> {
+    let dict = match value {
+        PySetterValue::Assign(value) => {
+            let dict = value.downcast::<PyDict>().map_err(|value| {
+                vm.new_type_error(format!(
+                    "__dict__ must be set to a dictionary, not a '{}'",
+                    value.class().name()
+                ))
+            })?;
+            PySetterValue::Assign(dict)
+        }
+        PySetterValue::Delete => return Err(vm.new_type_error("cannot delete __dict__")),
+    };
+    object_set_dict(obj, dict, vm)
+}
+
 pub fn init(ctx: &'static Context) {
     // Manually set alloc/init slots - derive macro doesn't generate extend_slots
     // for trait impl that overrides #[pyslot] method

crates/vm/src/builtins/type.rs

@@ -2643,7 +2643,7 @@ fn subtype_get_dict(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult {
 }
 
 // = subtype_setdict
-fn subtype_set_dict(obj: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
+fn subtype_set_dict(obj: PyObjectRef, value: PySetterValue, vm: &VirtualMachine) -> PyResult<()> {
     let base = get_builtin_base_with_dict(obj.class(), vm);
 
     if let Some(base_type) = base {
@@ -2655,13 +2655,17 @@ fn subtype_set_dict(obj: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) -
                 .descr_set
                 .load()
                 .ok_or_else(|| raise_dict_descriptor_error(&obj, vm))?;
-            descr_set(&descr, obj, PySetterValue::Assign(value), vm)
+            descr_set(&descr, obj, value, vm)
         } else {
             Err(raise_dict_descriptor_error(&obj, vm))
         }
     } else {
-        // PyObject_GenericSetDict
-        object::object_set_dict(obj, value.try_into_value(vm)?, vm)?;
+        // _PyObject_SetDict
+        let value = match value {
+            PySetterValue::Assign(obj) => PySetterValue::Assign(obj.try_into_value(vm)?),
+            PySetterValue::Delete => PySetterValue::Delete,
+        };
+        object::object_set_dict(obj, value, vm)?;
         Ok(())
     }
 }

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()
@@ -933,8 +937,8 @@ impl Py<PyWeak> {
 }
 
 #[derive(Debug)]
-pub(super) struct InstanceDict {
-    pub(super) d: PyRwLock<PyDictRef>,
+pub(crate) struct InstanceDict {
+    pub(crate) d: PyRwLock<Option<PyDictRef>>,
 }
 
 impl From<PyDictRef> for InstanceDict {
@@ -947,30 +951,45 @@ impl From<PyDictRef> for InstanceDict {
 impl InstanceDict {
     #[inline]
     pub const fn new(d: PyDictRef) -> Self {
+        Self {
+            d: PyRwLock::new(Some(d)),
+        }
+    }
+
+    #[inline]
+    pub const fn from_opt(d: Option<PyDictRef>) -> Self {
         Self {
             d: PyRwLock::new(d),
         }
     }
 
     #[inline]
-    pub fn get(&self) -> PyDictRef {
+    pub fn get(&self) -> Option<PyDictRef> {
         self.d.read().clone()
     }
 
     #[inline]
-    pub fn set(&self, d: PyDictRef) {
+    pub fn set(&self, d: Option<PyDictRef>) {
         self.replace(d);
     }
 
     #[inline]
-    pub fn replace(&self, d: PyDictRef) -> PyDictRef {
+    pub fn replace(&self, d: Option<PyDictRef>) -> Option<PyDictRef> {
         core::mem::replace(&mut self.d.write(), d)
     }
 
-    /// Consume the InstanceDict and return the inner PyDictRef.
-    #[inline]
-    pub fn into_inner(self) -> PyDictRef {
-        self.d.into_inner()
+    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 {
+            *d = Some(dict.clone());
+            dict
+        }
     }
 }
 
@@ -1084,7 +1103,11 @@ 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 {
@@ -1476,18 +1499,18 @@ impl PyObject {
     }
 
     #[inline(always)]
-    fn instance_dict(&self) -> Option<&InstanceDict> {
+    pub(crate) fn instance_dict(&self) -> Option<&InstanceDict> {
         self.0.ext_ref().and_then(|ext| ext.dict.as_ref())
     }
 
     #[inline(always)]
     pub fn dict(&self) -> Option<PyDictRef> {
-        self.instance_dict().map(|d| d.get())
+        self.instance_dict().and_then(|d| d.get())
     }
 
     /// Set the dict field. Returns `Err(dict)` if this object does not have a dict field
     /// in the first place.
-    pub fn set_dict(&self, dict: PyDictRef) -> Result<(), PyDictRef> {
+    pub fn set_dict(&self, dict: Option<PyDictRef>) -> Result<(), Option<PyDictRef>> {
         match self.instance_dict() {
             Some(d) => {
                 d.set(dict);
@@ -1799,9 +1822,7 @@ 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
-                let dict_ref = old_dict.into_inner();
+            if let Some(dict_ref) = ext.dict.as_ref().and_then(|d| d.replace(None)) {
                 result.push(dict_ref.into());
             }
             for slot in ext.slots.iter() {
@@ -2471,7 +2492,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
 
             unsafe {
                 let ext_ptr = alloc_ptr as *mut ObjExt;
-                ext_ptr.write(ObjExt::new(None, 0));
+                ext_ptr.write(ObjExt::new(None, 0, true));
 
                 let weakref_ptr = alloc_ptr.add(weakref_offset) as *mut WeakRefList;
                 weakref_ptr.write(WeakRefList::new());

crates/vm/src/protocol/object.rs

@@ -199,10 +199,12 @@ 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())
@@ -210,6 +212,8 @@ impl PyObject {
                         e
                     }
                 })?;
+            } else {
+                return Err(vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned()));
             }
             Ok(())
         } else {

crates/vm/src/stdlib/_functools.rs

@@ -4,9 +4,11 @@ pub(crate) use _functools::module_def;
 mod _functools {
     use crate::{
         Py, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
-        builtins::{PyBoundMethod, PyDict, PyGenericAlias, PyTuple, PyType, PyTypeRef},
+        builtins::{
+            PyBoundMethod, PyDict, PyDictRef, PyGenericAlias, PyTuple, PyType, PyTypeRef, object,
+        },
         common::lock::PyRwLock,
-        function::{FuncArgs, KwArgs, OptionalOption},
+        function::{FuncArgs, KwArgs, OptionalOption, PySetterValue},
         object::AsObject,
         protocol::PyIter,
         pyclass,
@@ -158,6 +160,23 @@ mod _functools {
             self.inner.read().keywords.clone()
         }
 
+        #[pygetset]
+        fn __dict__(zelf: &Py<Self>, vm: &VirtualMachine) -> PyDictRef {
+            zelf.as_object()
+                .instance_dict()
+                .map(|d| d.get_or_insert(vm))
+                .unwrap_or_else(|| vm.ctx.new_dict())
+        }
+
+        #[pygetset(setter)]
+        fn set___dict__(
+            zelf: &Py<Self>,
+            value: PySetterValue,
+            vm: &VirtualMachine,
+        ) -> PyResult<()> {
+            object::object_generic_set_dict(zelf.as_object().to_owned(), value, vm)
+        }
+
         #[pymethod]
         fn __reduce__(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult {
             let inner = zelf.inner.read();

extra_tests/snippets/builtin_object.py

@@ -26,6 +26,17 @@ assert not hasattr(obj, "a")
 obj.__dict__ = {"a": 1}
 assert obj.a == 1
 
+del obj.__dict__
+d = obj.__dict__
+assert isinstance(d, dict)
+assert len(d) == 0
+
+try:
+    obj.a
+    assert False, "AttributeError expected"
+except AttributeError:
+    pass
+
 # Value inside the formatter goes through a different path of resolution.
 # Check that it still works all the same
 d = {

extra_tests/snippets/stdlib_functools.py

@@ -1,4 +1,4 @@
-from functools import reduce
+from functools import partial, reduce
 
 from testutils import assert_raises
 
@@ -90,3 +90,10 @@ assert reduce(add, SequenceClass(1), 42) == 42
 
 d = {"one": 1, "two": 2, "three": 3}
 assert reduce(add, d) == "".join(d.keys())
+
+p = partial(add)
+try:
+    del p.__dict__
+    assert False, "TypeError expected for partial dict deletion"
+except TypeError:
+    pass

extra_tests/snippets/syntax_function2.py

@@ -104,3 +104,9 @@ def f9():
     pass
 
 assert f9.__doc__ == None
+
+try:
+    del f9.__dict__
+    assert False, "TypeError expected for function dict deletion"
+except TypeError:
+    pass