From ddb5125f8676c805982f4f4af5c99eea08d9c7f7 Mon Sep 17 00:00:00 2001 From: Alok Pandey <166419621+alok-108@users.noreply.github.com> Date: Thu, 30 Apr 2026 07:39:16 +0530 Subject: [PATCH] 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 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 --- Lib/test/test_class.py | 2 - crates/vm/src/builtins/builtin_func.rs | 5 +- crates/vm/src/builtins/function.rs | 12 ++++- crates/vm/src/builtins/object.rs | 42 +++++++++++++++-- crates/vm/src/builtins/type.rs | 12 +++-- crates/vm/src/object/core.rs | 59 ++++++++++++++++-------- crates/vm/src/protocol/object.rs | 10 ++-- crates/vm/src/stdlib/_functools.rs | 23 ++++++++- extra_tests/snippets/builtin_object.py | 11 +++++ extra_tests/snippets/stdlib_functools.py | 9 +++- extra_tests/snippets/syntax_function2.py | 6 +++ 11 files changed, 153 insertions(+), 38 deletions(-) diff --git a/Lib/test/test_class.py b/Lib/test/test_class.py index 7420f289b..1aee6fb73 100644 --- a/Lib/test/test_class.py +++ b/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. diff --git a/crates/vm/src/builtins/builtin_func.rs b/crates/vm/src/builtins/builtin_func.rs index c41df00fd..a09fb60c1 100644 --- a/crates/vm/src/builtins/builtin_func.rs +++ b/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 { diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index 782daf0b9..f97037560 100644 --- a/crates/vm/src/builtins/function.rs +++ b/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, vm: &VirtualMachine) -> PyResult { + object::object_get_dict(zelf.as_object().to_owned(), vm) + } + + #[pygetset(setter)] + fn set___dict__(zelf: &Py, 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 diff --git a/crates/vm/src/builtins/object.rs b/crates/vm/src/builtins/object.rs index 002b05d38..f600e36be 100644 --- a/crates/vm/src/builtins/object.rs +++ b/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 { - 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, + 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::().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 diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index f39821b45..e900014b6 100644 --- a/crates/vm/src/builtins/type.rs +++ b/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(()) } } diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index 290298f36..39ef93de5 100644 --- 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, member_count: usize) -> Self { + fn new(dict: Option, 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 { } #[derive(Debug)] -pub(super) struct InstanceDict { - pub(super) d: PyRwLock, +pub(crate) struct InstanceDict { + pub(crate) d: PyRwLock>, } impl From for InstanceDict { @@ -947,30 +951,45 @@ impl From 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) -> Self { Self { d: PyRwLock::new(d), } } #[inline] - pub fn get(&self) -> PyDictRef { + pub fn get(&self) -> Option { self.d.read().clone() } #[inline] - pub fn set(&self, d: PyDictRef) { + pub fn set(&self, d: Option) { self.replace(d); } #[inline] - pub fn replace(&self, d: PyDictRef) -> PyDictRef { + pub fn replace(&self, d: Option) -> Option { 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 PyInner { 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 { - 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) -> Result<(), Option> { match self.instance_dict() { Some(d) => { d.set(dict); @@ -1799,9 +1822,7 @@ impl PyObject { let ext_ptr = core::ptr::with_exposed_provenance_mut::(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()); diff --git a/crates/vm/src/protocol/object.rs b/crates/vm/src/protocol/object.rs index 5613458c9..0d4e206ec 100644 --- a/crates/vm/src/protocol/object.rs +++ b/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 { diff --git a/crates/vm/src/stdlib/_functools.rs b/crates/vm/src/stdlib/_functools.rs index 494f0e7fd..110fca675 100644 --- a/crates/vm/src/stdlib/_functools.rs +++ b/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, 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, + value: PySetterValue, + vm: &VirtualMachine, + ) -> PyResult<()> { + object::object_generic_set_dict(zelf.as_object().to_owned(), value, vm) + } + #[pymethod] fn __reduce__(zelf: &Py, vm: &VirtualMachine) -> PyResult { let inner = zelf.inner.read(); diff --git a/extra_tests/snippets/builtin_object.py b/extra_tests/snippets/builtin_object.py index 64486e167..940ca2f5f 100644 --- a/extra_tests/snippets/builtin_object.py +++ b/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 = { diff --git a/extra_tests/snippets/stdlib_functools.py b/extra_tests/snippets/stdlib_functools.py index d3e2495e8..acd112908 100644 --- a/extra_tests/snippets/stdlib_functools.py +++ b/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 diff --git a/extra_tests/snippets/syntax_function2.py b/extra_tests/snippets/syntax_function2.py index 4a04acd51..e7d1a9395 100644 --- a/extra_tests/snippets/syntax_function2.py +++ b/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