Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix del obj.__dict__: improve GC safety, implement lazy re-creation i…
…n setattr, and enable passing CPython tests
  • Loading branch information
alok-108 committed Apr 7, 2026
commit d3cc9d436a56259e224c7807531703c585019fc0
6 changes: 3 additions & 3 deletions Lib/test/test_class.py
Comment thread
ShaharNaveh marked this conversation as resolved.
Comment thread
ShaharNaveh marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
28 changes: 21 additions & 7 deletions crates/vm/src/object/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
slots: core::iter::repeat_with(|| PyRwLock::new(None))
.take(member_count)
.collect_vec()
Expand Down Expand Up @@ -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()
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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());
}
}
Expand Down
8 changes: 5 additions & 3 deletions crates/vm/src/protocol/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
23 changes: 23 additions & 0 deletions extra_tests/snippets/test_del_dict.py
Original file line number Diff line number Diff line change
@@ -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")