Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
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
2 changes: 0 additions & 2 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 @@ -978,7 +978,6 @@ def __init__(self):
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
Expand All @@ -988,7 +987,6 @@ class Foo:
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.
Expand Down
20 changes: 17 additions & 3 deletions crates/vm/src/builtins/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -554,10 +554,24 @@ 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__"))
}
Expand Down
11 changes: 8 additions & 3 deletions crates/vm/src/builtins/type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2622,7 +2622,11 @@ 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 {
Expand All @@ -2634,13 +2638,14 @@ 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)?;
let value = value.map(|v| v.try_into_value(vm)).transpose()?;
object::object_set_dict(obj, value, vm)?;
Ok(())
}
}
Expand Down
62 changes: 44 additions & 18 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 @@ -909,8 +913,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 {
Expand All @@ -923,31 +927,52 @@ 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.
/// Consume the InstanceDict and return the inner Option<PyDictRef>.
#[inline]
pub fn into_inner(self) -> PyDictRef {
pub fn into_inner(self) -> Option<PyDictRef> {
self.d.into_inner()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

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
}
}
}

impl<T: PyPayload> PyInner<T> {
Expand Down Expand Up @@ -1060,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 @@ -1451,18 +1477,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);
Expand Down Expand Up @@ -1774,10 +1800,10 @@ 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();
result.push(dict_ref.into());
if let Some(instance_dict) = &ext.dict {
if let Some(dict_ref) = instance_dict.replace(None) {
result.push(dict_ref.into());
}
}
for slot in ext.slots.iter() {
if let Some(val) = slot.write().take() {
Expand Down Expand Up @@ -2444,7 +2470,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());
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")
Loading