From 082a4be28ea2888a533ee4f4ccf73f80b79993d4 Mon Sep 17 00:00:00 2001 From: dot Date: Wed, 4 Nov 2026 23:33:23 +0900 Subject: [PATCH 1/6] Align SimpleNamespace dictionary and callback controls Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/builtins/namespace.rs | 106 +++++++++++++++------------- 1 file changed, 58 insertions(+), 48 deletions(-) diff --git a/crates/vm/src/builtins/namespace.rs b/crates/vm/src/builtins/namespace.rs index a872fd6c92e..51ad27ca9be 100644 --- a/crates/vm/src/builtins/namespace.rs +++ b/crates/vm/src/builtins/namespace.rs @@ -28,7 +28,7 @@ impl PyPayload for PyNamespace { impl DefaultConstructor for PyNamespace {} #[pyclass( - flags(BASETYPE, HAS_DICT, HAS_WEAKREF), + flags(BASETYPE, HAS_DICT), with(Constructor, Initializer, Comparable, Representable) )] impl PyNamespace { @@ -54,7 +54,7 @@ impl PyNamespace { let cls: PyObjectRef = zelf.class().to_owned().into(); let result = cls.call((), vm)?; - if !zelf.class().is(result.class()) { + if !result.downcastable::() { return Err(vm.new_type_error(format!( "expect {} type, but {}() returned '{}' object", Self::class(&vm.ctx).slot_name(), @@ -72,14 +72,11 @@ impl PyNamespace { // Copy the current namespace dict to the new instance let src_dict = zelf.dict().unwrap(); let dst_dict = result.dict().unwrap(); - for (key, value) in src_dict { - dst_dict.set_item(&*key, value, vm)?; - } + dst_dict.merge_dict(&src_dict, true, vm)?; - // Update with the provided kwargs - for (name, value) in changes { - let name = vm.ctx.new_str(name); - result.set_attr(&name, value, vm)?; + // Bypass attribute setters and retain all argument owners during updates. + for (name, value) in &changes { + dst_dict.inner_setitem(name, value.clone(), vm)?; } Ok(result) @@ -98,38 +95,37 @@ impl Initializer for PyNamespace { type Args = NamespaceArgs; fn init(zelf: &Py, args: Self::Args, vm: &VirtualMachine) -> PyResult<()> { - // If there's a positional argument, treat it as a mapping - if let OptionalArg::Present(mapping) = args.mapping_or_iterable { - // Convert to dict if not already - let dict: PyRef = if let Some(d) = mapping.downcast_ref::() { - d.to_owned() + // Keep the original input alive through conversion and both updates. + if let OptionalArg::Present(mapping) = &args.mapping_or_iterable { + let dict: PyRef = if mapping.class().is(vm.ctx.types.dict_type) { + mapping.downcast_ref::().unwrap().to_owned() } else { - // Call dict() on the mapping + // Dict subclasses may override the mapping conversion protocol. let dict_type: PyObjectRef = vm.ctx.types.dict_type.to_owned().into(); dict_type - .call((mapping,), vm)? + .call((mapping.clone(),), vm)? .downcast() .map_err(|_| vm.new_type_error("dict() did not return a dict"))? }; - // Validate keys are strings and set attributes - for (key, value) in dict { - let key_str = key - .downcast_ref::() - .ok_or_else(|| { - vm.new_type_error(format!( - "keywords must be strings, not '{}'", - key.class().name() - )) - })?; - zelf.as_object().set_attr(key_str, value, vm)?; + // Validate every key before publishing any of the supplied values. + if dict + .keys_vec() + .iter() + .any(|key| !key.downcastable::()) + { + return Err(vm.new_type_error("keywords must be strings")); } + zelf.as_object() + .dict() + .unwrap() + .merge_dict(&dict, true, vm)?; } - // Apply keyword arguments (these override positional mapping values) - for (name, value) in args.kwargs { - let name = vm.ctx.new_str(name); - zelf.as_object().set_attr(&name, value, vm)?; + // Apply keywords directly to the namespace, without invoking descriptors. + let dict = zelf.as_object().dict().unwrap(); + for (name, value) in &args.kwargs { + dict.inner_setitem(name, value.clone(), vm)?; } Ok(()) } @@ -163,25 +159,39 @@ impl Representable for PyNamespace { let repr = if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { let dict = zelf.as_object().dict().unwrap(); - let mut result = Wtf8Buf::from(format!("{name}(")); - let mut first = true; - for (key, value) in dict { - let Some(key_str) = key.downcast_ref::() else { - continue; - }; - if key_str.as_wtf8().is_empty() { - continue; - } - if !first { - result.push_str(", "); + // Retain the entire key snapshot, but look up each value after any + // preceding repr callback has had a chance to change the dictionary. + let keys = dict.keys_vec(); + let result: PyResult = (|| { + let mut result = Wtf8Buf::from(format!("{name}(")); + let mut first = true; + for key in &keys { + let Some(key_str) = key.downcast_ref::() else { + continue; + }; + if key_str.as_wtf8().is_empty() { + continue; + } + let Some(value) = dict.get_item_opt(&**key, vm)? else { + continue; + }; + if !first { + result.push_str(", "); + } + first = false; + result.push_wtf8(key_str.as_wtf8()); + result.push_char('='); + result.push_wtf8(value.repr(vm)?.as_wtf8()); } - first = false; - result.push_wtf8(key_str.as_wtf8()); - result.push_char('='); - result.push_wtf8(value.repr(vm)?.as_wtf8()); + result.push_char(')'); + Ok(result) + })(); + // The key list releases its elements backwards, on success and + // failure, while recursive calls from finalizers still see the guard. + for key in keys.into_iter().rev() { + drop(key); } - result.push_char(')'); - result + result? } else { Wtf8Buf::from(format!("{name}(...)")) }; From be0a9b869a8c5df925494dee1a81e43abf88ce1b Mon Sep 17 00:00:00 2001 From: dot Date: Thu, 5 Nov 2026 00:06:37 +0900 Subject: [PATCH 2/6] Align mapping proxy delegation and dictionary read controls Propagate key callback errors, retain live dictionary-backed class views, and revalidate shared read probes after equality and finalizer callbacks. Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/builtins/mappingproxy.rs | 85 +++++++++++--------------- crates/vm/src/dict_inner.rs | 72 ++++++++++++++-------- 2 files changed, 81 insertions(+), 76 deletions(-) diff --git a/crates/vm/src/builtins/mappingproxy.rs b/crates/vm/src/builtins/mappingproxy.rs index 168bdebed5e..39aa1e35455 100644 --- a/crates/vm/src/builtins/mappingproxy.rs +++ b/crates/vm/src/builtins/mappingproxy.rs @@ -1,4 +1,4 @@ -use super::{PyDict, PyDictRef, PyGenericAlias, PyList, PyTuple, PyType, PyTypeRef}; +use super::{PyDict, PyDictRef, PyGenericAlias, PyList, PyStrRef, PyTuple, PyType, PyTypeRef}; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, atomic_func, @@ -60,11 +60,16 @@ impl From for PyMappingProxy { } } +#[derive(FromArgs)] +pub struct MappingProxyArgs { + mapping: PyObjectRef, +} + impl Constructor for PyMappingProxy { - type Args = PyObjectRef; + type Args = MappingProxyArgs; - fn py_new(_cls: &Py, mapping: Self::Args, vm: &VirtualMachine) -> PyResult { - Self::from_object(mapping, vm) + fn py_new(_cls: &Py, args: Self::Args, vm: &VirtualMachine) -> PyResult { + Self::from_object(args.mapping, vm) } } @@ -102,12 +107,9 @@ impl PyMappingProxy { key: &PyObject, vm: &VirtualMachine, ) -> PyResult> { - match class.attributes.as_dict() { - Some(dict) => dict.get_item_opt(key, vm), - None => Ok(key - .as_interned_str(vm) - .and_then(|key| class.attributes.get(key))), - } + // Use the dictionary lookup for native namespaces too: even a key + // that is not a string can hash or compare equal to an attribute name. + Self::class_mapping(class, vm)?.get_item_opt(key, vm) } pub fn __getitem__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { @@ -116,21 +118,7 @@ impl PyMappingProxy { } fn _contains(&self, key: &PyObject, vm: &VirtualMachine) -> PyResult { - match &self.mapping { - MappingProxyInner::Class(class) => Ok(Self::class_contains(class, key, vm)), - MappingProxyInner::Mapping(mapping) => { - mapping.obj().sequence_unchecked().contains(key, vm) - } - } - } - - fn class_contains(class: &Py, key: &PyObject, vm: &VirtualMachine) -> bool { - match class.attributes.as_dict() { - Some(dict) => dict.contains_key(key, vm), - None => key - .as_interned_str(vm) - .is_some_and(|key| class.attributes.contains(key)), - } + self.to_object(vm)?.sequence_unchecked().contains(key, vm) } pub fn __contains__(&self, key: &PyObject, vm: &VirtualMachine) -> PyResult { @@ -140,15 +128,18 @@ impl PyMappingProxy { fn to_object(&self, vm: &VirtualMachine) -> PyResult { Ok(match &self.mapping { MappingProxyInner::Mapping(d) => d.as_ref().to_owned(), - MappingProxyInner::Class(c) => Self::class_to_dict(c, vm)?, + MappingProxyInner::Class(c) => Self::class_mapping(c, vm)?.into(), }) } - fn class_to_dict(class: &Py, vm: &VirtualMachine) -> PyResult { + fn class_mapping(class: &Py, vm: &VirtualMachine) -> PyResult { if let Some(dict) = class.attributes.as_dict() { - return Ok(dict.copy().to_pyobject(vm)); + // Views and iterators must stay attached to the actual namespace. + return Ok(dict.to_owned()); } - Ok(PyDict::from_attributes(class.attributes.attributes(&vm.ctx), vm)?.to_pyobject(vm)) + // Native namespaces without a backing Python dictionary still need a + // snapshot, but must not bypass dict key callbacks. + Ok(PyDict::from_attributes(class.attributes.attributes(&vm.ctx), vm)?.into_ref(&vm.ctx)) } fn __len__(&self, vm: &VirtualMachine) -> PyResult { @@ -158,22 +149,14 @@ impl PyMappingProxy { fn __ior__(&self, _args: PyObjectRef, vm: &VirtualMachine) -> PyResult { Err(vm.new_type_error(format!( - r#""'|=' is not supported by {}; use '|' instead""#, + "'|=' is not supported by {}; use '|' instead", Self::class(&vm.ctx) ))) } - fn __or__(&self, args: &PyObject, vm: &VirtualMachine) -> PyResult { - vm._or(self.copy(vm)?.as_ref(), args) - } - pub fn copy(&self, vm: &VirtualMachine) -> PyResult { - match &self.mapping { - MappingProxyInner::Mapping(d) => { - vm.call_method(d.obj(), identifier!(vm, copy).as_str(), ()) - } - MappingProxyInner::Class(c) => Self::class_to_dict(c, vm), - } + let obj = self.to_object(vm)?; + vm.call_method(&obj, identifier!(vm, copy).as_str(), ()) } } @@ -205,10 +188,8 @@ impl Py { } let mut obj = zelf.to_object(vm)?; // Unknown comparison methods must not receive a mutable dictionary - // protected by the proxy. Class namespaces are already copied by - // to_object; other exact dicts need the same protection here. - if matches!(&zelf.mapping, MappingProxyInner::Mapping(_)) - && obj.class().is(vm.ctx.types.dict_type) + // protected by the proxy, including a live class namespace. + if obj.class().is(vm.ctx.types.dict_type) && !(other.class().is(vm.ctx.types.dict_type) || other.class().is(vm.ctx.types.frozendict_type) || other.class().is(vm.ctx.types.mappingproxy_type) @@ -224,6 +205,11 @@ impl Py { obj.rich_compare(other.to_owned(), op, vm).map(Either::A) } + #[pymethod] + fn __str__(&self, vm: &VirtualMachine) -> PyResult { + self.to_object(vm)?.str(vm) + } + #[pymethod] fn get( &self, @@ -322,16 +308,15 @@ impl AsNumber for PyMappingProxy { fn as_number() -> &'static PyNumberMethods { static AS_NUMBER: PyNumberMethods = PyNumberMethods { or: Some(|a, b, vm| { - // Mirror CPython's mappingproxy_or: when either side is a - // mappingproxy, unwrap to its underlying mapping and delegate - // to PyNumber_Or so `dict | mp`, `mp | dict`, and `mp | mp` - // all produce a `dict` result. + // Unwrap both proxies, preserving the underlying mappings' + // union methods and arbitrary return values. Keep both + // operands alive until dispatch has finished. let a_obj = match a.downcast_ref::() { - Some(mp) => mp.copy(vm)?, + Some(mp) => mp.to_object(vm)?, None => a.to_pyobject(vm), }; let b_obj = match b.downcast_ref::() { - Some(mp) => mp.copy(vm)?, + Some(mp) => mp.to_object(vm)?, None => b.to_pyobject(vm), }; vm._or(a_obj.as_ref(), b_obj.as_ref()) diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 947bbb91af7..f1186abf277 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -1196,28 +1196,27 @@ impl Dict { Ok(ret) } - /// [`Self::lookup`], additionally reading the matched entry when the probe - /// settles it by key identity. + /// [`Self::lookup`], additionally reading the matched entry under the + /// probe's guard, after any equality callback has been revalidated. /// - /// That is the common case, and it is decided while the read guard is still - /// held — so a caller that only wants the entry's value gets it here instead - /// of taking the lock a second time to re-find what the probe already had. + /// Key identity is the common case and is decided while the read guard is + /// still held. After equality, the current entry is read under the guard + /// that revalidated it, without a second lookup by stale coordinates. /// `extract` therefore runs under the guard and must not run Python. It is - /// not called when the key had to be compared with `key_eq`, which does run - /// Python and so releases the guard first. + /// called only after reacquiring the guard when `key_eq` runs Python. #[cfg_attr(feature = "flame-it", flame("Dict"))] - fn lookup_extract( - &self, + fn lookup_extract<'a, K: DictKey + ?Sized, R>( + &'a self, vm: &VirtualMachine, key: &K, hash_value: HashValue, - mut lock: Option>>, + mut lock: Option>>, extract: impl Fn(&DictEntry) -> R, ) -> PyResult<(LookupResult, Option)> { let mut idxs = None; let mut free_slot = None; - let ret = 'outer: loop { - let (entry_key, ret) = { + loop { + let (entry_key, ret, indices_ptr, indices_len) = { let inner = lock.take().unwrap_or_else(|| self.read()); let mask = (inner.indices.len() - 1) as i64; let idxs = idxs.get_or_insert_with(|| GenIndexes::new(hash_value, mask)); @@ -1261,7 +1260,12 @@ impl Dict { if key.key_is(&entry.key) { return Ok((ret, Some(extract(entry)))); } else if entry.hash == hash_value { - break (entry.key.clone(), ret); + break ( + entry.key.clone(), + ret, + inner.indices.as_ptr(), + inner.indices.len(), + ); } else { // entry mismatch } @@ -1271,20 +1275,36 @@ impl Dict { } }; - #[expect( - clippy::redundant_else, - reason = "Keeping the empty `else` block here for documentation" - )] - // This comparison needs to be done outside the lock. - if key.key_eq(vm, &entry_key)? { - break 'outer ret; - } else { - // hash collision + // Equality and releasing its temporary key can both run Python. + // Drop the owner before checking the table, without holding a lock. + let entry_key_id = entry_key.get_id(); + let equal = key.key_eq(vm, &entry_key); + drop(entry_key); + let equal = equal?; + let inner = self.read(); + let entry = inner + .get_entry_checked(ret.0.index().unwrap(), ret.1) + .filter(|entry| { + inner.indices.as_ptr() == indices_ptr + && inner.indices.len() == indices_len + && inner.indices.get(ret.1) == Some(&ret.0) + && entry.hash == hash_value + && entry.key.get_id() == entry_key_id + }); + let Some(entry) = entry else { + // A callback may recycle the same entry and bucket for a + // different key, or rebuild the table around the same key. + drop(inner); + idxs = None; + free_slot = None; + continue; + }; + if equal { + return Ok((ret, Some(extract(entry)))); } - - // warn!("Perturb value: {}", i); - }; - Ok((ret, None)) + // Hash collision: continue probing the unchanged table. + lock = Some(inner); + } } // returns Err(()) if changed since lookup From 3950911c089e17a36db7051fbf2bd4079b5de9e4 Mon Sep 17 00:00:00 2001 From: dot Date: Thu, 5 Nov 2026 00:38:56 +0900 Subject: [PATCH 3/6] Validate hinted dictionary reads through shared lookup Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/dict_inner.rs | 41 +++++++++---------------------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index f1186abf277..901a5fd00d2 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -703,20 +703,10 @@ impl Dict { key: &K, hash: HashValue, ) -> PyResult)>> { - let ret = loop { - let (entry, index_index) = self.lookup(vm, key, hash, None)?; - if let Some(index) = entry.index() { - let inner = self.read(); - if let Some(entry) = inner.get_entry_checked(index, index_index) { - // The dict was not changed since we did lookup - break Some((entry.value.clone(), u16::try_from(index).ok())); - } - // The dict was changed since we did lookup. Let's try again. - } else { - break None; - } - }; - Ok(ret) + let ((entry, _), value) = + self.lookup_extract(vm, key, hash, None, |entry| entry.value.clone())?; + let hint = entry.index().and_then(|index| u16::try_from(index).ok()); + Ok(value.map(|value| (value, hint))) } /// Replace the value at entry index `hint` if that entry's key is @@ -751,29 +741,18 @@ impl Dict { /// Fast path lookup using a cached entry index (`hint`). /// - /// Returns `None` if the hint is stale or the key no longer matches. + /// Returns `None` if the hint is stale or its key is not identical to `key`. pub(crate) fn get_hint( &self, - vm: &VirtualMachine, + _vm: &VirtualMachine, key: &K, hint: usize, ) -> PyResult> { - let (entry_key, entry_value) = { - let inner = self.read(); - let Some(Some(entry)) = inner.entries.get(hint) else { - return Ok(None); - }; - if key.key_is(&entry.key) { - return Ok(Some(entry.value.clone())); - } - (entry.key.clone(), entry.value.clone()) + let inner = self.read(); + let Some(Some(entry)) = inner.entries.get(hint) else { + return Ok(None); }; - // key_eq may run Python __eq__, so must be outside the lock. - if key.key_eq(vm, &entry_key)? { - Ok(Some(entry_value)) - } else { - Ok(None) - } + Ok(key.key_is(&entry.key).then(|| entry.value.clone())) } /// Read an entry directly when a cached keys-version still describes the From 71849b6ad1dc34fec7665c004ad7d14ccebbbf0e Mon Sep 17 00:00:00 2001 From: dot Date: Thu, 5 Nov 2026 00:44:57 +0900 Subject: [PATCH 4/6] Make cached hint lookup infallible Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/builtins/dict.rs | 2 +- crates/vm/src/dict_inner.rs | 15 ++++----------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index cd20acf8dcf..d1d4eaf86d7 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1215,7 +1215,7 @@ impl Py { vm: &VirtualMachine, ) -> PyResult)>> { if self.exact_dict(vm) { - if let Some(value) = self.entries.get_hint(vm, key, usize::from(hint))? { + if let Some(value) = self.entries.get_hint(key, usize::from(hint)) { return Ok(Some((value, None))); } let hash = PyDict::hash_or_unhashable(key, vm)?; diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 901a5fd00d2..01a62cccf4c 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -742,17 +742,10 @@ impl Dict { /// Fast path lookup using a cached entry index (`hint`). /// /// Returns `None` if the hint is stale or its key is not identical to `key`. - pub(crate) fn get_hint( - &self, - _vm: &VirtualMachine, - key: &K, - hint: usize, - ) -> PyResult> { + pub(crate) fn get_hint(&self, key: &K, hint: usize) -> Option { let inner = self.read(); - let Some(Some(entry)) = inner.entries.get(hint) else { - return Ok(None); - }; - Ok(key.key_is(&entry.key).then(|| entry.value.clone())) + let entry = inner.entries.get(hint)?.as_ref()?; + key.key_is(&entry.key).then(|| entry.value.clone()) } /// Read an entry directly when a cached keys-version still describes the @@ -2298,7 +2291,7 @@ mod tests { assert_eq!(dict.values(), vec![0, 2, 1]); assert_eq!(dict.keys_version(), 0); assert_eq!(dict.get_index_if_keys_version(version, hint), None); - assert_eq!(dict.get_hint(vm, keys[1], hint).unwrap(), None); + assert_eq!(dict.get_hint(keys[1], hint), None); assert!(dict.next_entry_checked(0, &size, |_, v| *v).is_err()); assert!( dict.prev_entry_checked(size.entries_size - 1, &size, |_, v| *v) From fae9924cb37eee1d23ca2001c32a020b526503e1 Mon Sep 17 00:00:00 2001 From: dot Date: Thu, 5 Nov 2026 01:49:13 +0900 Subject: [PATCH 5/6] Share checked dictionary iterator state and reduction Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/builtins/dict.rs | 404 +++++++++++++++------------------ crates/vm/src/dict_inner.rs | 44 ++-- 2 files changed, 205 insertions(+), 243 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d1d4eaf86d7..0acd310c429 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1,7 +1,6 @@ use super::{ - IterStatus, PositionIterInternal, PyBaseExceptionRef, PyFrozenSet, PyGenericAlias, - PyMappingProxy, PySet, PyStr, PyStrRef, PyTupleRef, PyType, PyTypeRef, locked_step, set, - set::PySetInner, + PyBaseExceptionRef, PyFrozenSet, PyGenericAlias, PyMappingProxy, PySet, PyStr, PyStrRef, + PyTupleRef, PyType, PyTypeRef, set, set::PySetInner, }; use crate::common::lock::LazyLock; use crate::object::{Traverse, TraverseFn}; @@ -1787,6 +1786,176 @@ trait DictView: PyPayload + PyClassDef + Iterable + Representable { fn __reversed__(zelf: &Py) -> Self::ReverseIter; } +#[derive(Debug, Clone)] +struct DictIterState { + dict: Option>, + size: dict_inner::DictSize, + position: usize, + remaining: usize, + changed: bool, + reversed: bool, +} + +// SAFETY: the optional dictionary is the sole owned Python reference and is +// visited once. The remaining fields contain only scalar iterator state. +unsafe impl Traverse for DictIterState { + fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { + self.dict.traverse(tracer_fn) + } +} + +enum DictIterError { + SizeChanged, + KeysChanged, +} + +type DictIterStep = (Result, DictIterError>, Option>); + +impl DictIterState { + fn new(dict: PyRef, reversed: bool) -> Self { + let size = dict.size(); + // usize::MAX represents the position before the first reverse entry. + let position = if reversed { + size.entries_size.wrapping_sub(1) + } else { + 0 + }; + Self { + dict: Some(dict), + position, + remaining: size.used, + size, + changed: false, + reversed, + } + } + + fn length_hint(&self) -> usize { + match &self.dict { + Some(dict) + if !self.changed && dict.size().has_same_used_and_relocations(&self.size) => + { + self.remaining + } + _ => 0, + } + } + + // Only clone the projected references while the table and iterator are + // locked. Allocation and the final release of the dictionary happen later. + fn step(&mut self, project: impl FnOnce(&PyObject, &PyObjectRef) -> T) -> DictIterStep { + let Some(dict) = &self.dict else { + return (Ok(None), None); + }; + if self.changed { + return (Err(DictIterError::SizeChanged), None); + } + let remaining = self.remaining; + let project = + |key: &PyObject, value: &PyObjectRef| (remaining != 0).then(|| project(key, value)); + let entries = &dict.as_dict().entries; + let entry = if self.reversed { + entries.prev_entry_checked(self.position, &self.size, project) + } else { + entries.next_dict_entry_checked(self.position, &self.size, project) + }; + match entry { + Err(dict_inner::DictChanged) => { + // Size failure is sticky and retains the dictionary owner. + self.changed = true; + (Err(DictIterError::SizeChanged), None) + } + Ok(Some((_, None))) => { + // Excess keys exhaust the iterator rather than latching a + // size failure. Its detached dictionary must be dropped unlocked. + (Err(DictIterError::KeysChanged), self.dict.take()) + } + Ok(Some((position, Some(item)))) => { + self.position = if self.reversed { + position.wrapping_sub(1) + } else { + position + }; + self.remaining -= 1; + (Ok(Some(item)), None) + } + Ok(None) => (Ok(None), self.dict.take()), + } + } +} + +fn finish_dict_iter_step(step: DictIterStep, vm: &VirtualMachine) -> PyResult> { + let (result, released) = step; + let result = result.map_err(|error| { + let message = match error { + DictIterError::SizeChanged => "dictionary changed size during iteration", + DictIterError::KeysChanged => "dictionary keys changed during iteration", + }; + vm.new_runtime_error(message) + }); + drop(released); + result +} + +macro_rules! dict_iterator { + ($name:ident, $class:ident, $class_name:literal, $reversed:literal, $project:expr, $result:expr) => { + #[pyclass(module = false, name = $class_name, traverse)] + #[derive(Debug)] + pub(crate) struct $name { + internal: PyMutex, + } + + impl PyPayload for $name { + #[inline] + fn class(ctx: &Context) -> &'static Py { + ctx.types.$class + } + } + + impl $name { + pub(crate) fn new(dict: PyRef) -> Self { + Self { + internal: PyMutex::new(DictIterState::new(dict, $reversed)), + } + } + } + + #[pyclass(flags(DISALLOW_INSTANTIATION), with(IterNext, Iterable))] + impl Py<$name> { + #[pymethod] + fn __length_hint__(&self) -> usize { + self.internal.lock().length_hint() + } + + #[pymethod] + fn __reduce__(&self, vm: &VirtualMachine) -> PyResult { + // Walk a copy through the same checks as next(), without + // advancing or invalidating the original iterator on failure. + let mut state = self.internal.lock().clone(); + let mut entries = Vec::new(); + while let Some(item) = finish_dict_iter_step(state.step($project), vm)? { + entries.push(($result)(vm, item)); + } + let list = vm.ctx.new_list(entries); + let iter = builtins_iter(vm)?; + Ok(vm.new_tuple((iter, (list,)))) + } + } + + impl SelfIter for $name {} + + impl IterNext for $name { + fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { + let step = zelf.internal.lock().step($project); + match finish_dict_iter_step(step, vm)? { + Some(item) => Ok(PyIterReturn::Return(($result)(vm, item))), + None => Ok(PyIterReturn::StopIteration(None)), + } + } + } + }; +} + macro_rules! dict_view { ( $name: ident, @@ -1867,232 +2036,13 @@ macro_rules! dict_view { } } - #[pyclass(module = false, name = $iter_class_name, traverse)] - #[derive(Debug)] - pub(crate) struct $iter_name { - #[pytraverse(skip)] - pub(crate) size: dict_inner::DictSize, - /// Whether the dict was found to have changed, which - /// `dictiter_iternextkey()` records by writing a size no dict can - /// have. Sticky: what it makes the iterator answer, it answers - /// from then on. - #[pytraverse(skip)] - changed: PyAtomic, - pub(crate) internal: PyMutex>>, - } - - impl PyPayload for $iter_name { - #[inline] - fn class(ctx: &Context) -> &'static Py { - ctx.types.$iter_class - } - } - - impl $iter_name { - pub(crate) fn new(dict: PyRef) -> Self { - $iter_name { - size: dict.size(), - changed: Radium::new(false), - internal: PyMutex::new(PositionIterInternal::new(dict, 0)), - } - } - } - - #[pyclass(flags(DISALLOW_INSTANTIATION), with(IterNext, Iterable))] - impl Py<$iter_name> { - #[pymethod] - fn __length_hint__(&self) -> usize { - // `dictiter_len()` answers for a dict it can no longer walk - // with nothing, comparing the size it captured against the - // dict's own every time it is asked. - if self.changed.load(Ordering::Relaxed) { - return 0; - } - self.internal.lock().length_hint(|dict| { - if dict.size() == self.size { - self.size.entries_size - } else { - 0 - } - }) - } - - #[pymethod] - fn __reduce__(&self, vm: &VirtualMachine) -> PyResult { - let iter = builtins_iter(vm)?; - let internal = self.internal.lock(); - let entries = match &internal.status { - IterStatus::Active(dict) => { - let mut position = internal.position; - let mut entries = Vec::new(); - while let Some((next_position, key, value)) = - dict.as_dict().entries.next_entry(position) - { - entries.push(($result_fn)(vm, ($project_fn)(&key, &value))); - position = next_position; - } - entries - } - IterStatus::Exhausted => vec![], - }; - Ok(vm.new_tuple((iter, (vm.ctx.new_list(entries),)))) - } - } - - impl SelfIter for $iter_name {} - - impl IterNext for $iter_name { - fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - locked_step(&zelf.internal, |internal| { - let IterStatus::Active(dict) = &internal.status else { - return (Ok(PyIterReturn::StopIteration(None)), None); - }; - let mutated = - || vm.new_runtime_error("dictionary changed size during iteration"); - if zelf.changed.load(Ordering::Relaxed) { - // The dict is not looked at again once it has been - // found to change: an iterator that has raised keeps - // raising. - return (Err(mutated()), None); - } - let entry = dict.as_dict().entries.next_entry_checked( - internal.position, - &zelf.size, - $project_fn, - ); - match entry { - Err(dict_inner::DictChanged) => { - zelf.changed.store(true, Ordering::Relaxed); - (Err(mutated()), None) - } - Ok(Some((position, item))) => { - internal.position = position; - (Ok(PyIterReturn::Return(($result_fn)(vm, item))), None) - } - Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), - } - }) - } - } - - #[pyclass(module = false, name = $reverse_iter_class_name, traverse)] - #[derive(Debug)] - pub(crate) struct $reverse_iter_name { - #[pytraverse(skip)] - pub(crate) size: dict_inner::DictSize, - /// As in `$iter_name`. - #[pytraverse(skip)] - changed: PyAtomic, - #[pytraverse(skip)] - remaining: PyAtomic, - internal: PyMutex>>, - } - - impl PyPayload for $reverse_iter_name { - #[inline] - fn class(ctx: &Context) -> &'static Py { - ctx.types.$reverse_iter_class - } - } - - impl $reverse_iter_name { - pub(crate) fn new(dict: PyRef) -> Self { - let size = dict.size(); - // usize::MAX represents the position before the first entry. - let position = size.entries_size.wrapping_sub(1); - $reverse_iter_name { - remaining: Radium::new(size.used), - size, - changed: Radium::new(false), - internal: PyMutex::new(PositionIterInternal::new(dict, position)), - } - } + dict_iterator! { + $iter_name, $iter_class, $iter_class_name, false, $project_fn, $result_fn } - #[pyclass(flags(DISALLOW_INSTANTIATION), with(IterNext, Iterable))] - impl Py<$reverse_iter_name> { - #[pymethod] - fn __reduce__(&self, vm: &VirtualMachine) -> PyResult { - let iter = builtins_iter(vm)?; - let internal = self.internal.lock(); - let entries = match &internal.status { - IterStatus::Active(dict) => { - let mut position = internal.position; - let mut entries = Vec::new(); - while let Some((found_index, key, value)) = - dict.as_dict().entries.prev_entry(position) - { - entries.push(($result_fn)(vm, ($project_fn)(&key, &value))); - if found_index == 0 { - break; - } - position = found_index - 1; - } - entries - } - IterStatus::Exhausted => vec![], - }; - Ok(vm.new_tuple((iter, (vm.ctx.new_list(entries),)))) - } - - #[pymethod] - fn __length_hint__(&self) -> usize { - // As in `$iter_name`. - if self.changed.load(Ordering::Relaxed) { - return 0; - } - let internal = self.internal.lock(); - match &internal.status { - IterStatus::Active(dict) - if dict.size().has_same_used_and_relocations(&self.size) => - { - self.remaining.load(Ordering::Relaxed) - } - _ => 0, - } - } - } - - impl SelfIter for $reverse_iter_name {} - - impl IterNext for $reverse_iter_name { - fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - locked_step(&zelf.internal, |internal| { - let IterStatus::Active(dict) = &internal.status else { - return (Ok(PyIterReturn::StopIteration(None)), None); - }; - let mutated = - || vm.new_runtime_error("dictionary changed size during iteration"); - if zelf.changed.load(Ordering::Relaxed) { - // The dict is not looked at again once it has been - // found to change: an iterator that has raised keeps - // raising. - return (Err(mutated()), None); - } - let remaining = zelf.remaining.load(Ordering::Relaxed); - let entry = dict.as_dict().entries.prev_entry_checked( - internal.position, - &zelf.size, - |key, value| (remaining != 0).then(|| ($project_fn)(key, value)), - ); - match entry { - Err(dict_inner::DictChanged) => { - zelf.changed.store(true, Ordering::Relaxed); - (Err(mutated()), None) - } - Ok(Some((_, None))) => ( - Err(vm.new_runtime_error("dictionary keys changed during iteration")), - internal.exhaust(), - ), - Ok(Some((found_index, Some(item)))) => { - zelf.remaining.store(remaining - 1, Ordering::Relaxed); - internal.position = found_index.wrapping_sub(1); - (Ok(PyIterReturn::Return(($result_fn)(vm, item))), None) - } - Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), - } - }) - } + dict_iterator! { + $reverse_iter_name, $reverse_iter_class, $reverse_iter_class_name, + true, $project_fn, $result_fn } }; } diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 01a62cccf4c..ae3f047adaf 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -261,7 +261,7 @@ struct DictEntry { } static_assertions::assert_eq_size!(DictEntry, Option>); -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct DictSize { indices_size: usize, pub entries_size: usize, @@ -1003,13 +1003,39 @@ impl Dict { /// another dict lock; it is there so an iterator clones only the field it /// keeps rather than both the key and the value. pub(crate) fn next_entry_checked( + &self, + position: EntryIndex, + old: &DictSize, + project: impl FnOnce(&PyObject, &T) -> R, + ) -> Result, DictChanged> { + self.next_entry_checked_by(position, old, PartialEq::eq, project) + } + + /// Dictionary iterators allow ordinary same-used key replacement, while + /// set iterators retain the stricter layout check above. + pub(crate) fn next_dict_entry_checked( + &self, + position: EntryIndex, + old: &DictSize, + project: impl FnOnce(&PyObject, &T) -> R, + ) -> Result, DictChanged> { + self.next_entry_checked_by( + position, + old, + DictSize::has_same_used_and_relocations, + project, + ) + } + + fn next_entry_checked_by( &self, mut position: EntryIndex, old: &DictSize, + unchanged: impl FnOnce(&DictSize, &DictSize) -> bool, project: impl FnOnce(&PyObject, &T) -> R, ) -> Result, DictChanged> { let inner = self.read(); - if inner.size() != *old { + if !unchanged(&inner.size(), old) { return Err(DictChanged); } loop { @@ -1068,20 +1094,6 @@ impl Dict { } } - pub(crate) fn prev_entry(&self, mut position: EntryIndex) -> Option<(usize, PyObjectRef, T)> { - let inner = self.read(); - loop { - let entry = inner.entries.get(position)?; - if let Some(entry) = entry { - break Some((position, entry.key.clone(), entry.value.clone())); - } - if position == 0 { - break None; - } - position -= 1; - } - } - pub(crate) fn len_from_entry_index(&self, position: EntryIndex) -> usize { self.read().entries.len().saturating_sub(position) } From a9b70671705149eab78e85a34245a7c543ae93cc Mon Sep 17 00:00:00 2001 From: dot Date: Thu, 5 Nov 2026 02:54:51 +0900 Subject: [PATCH 6/6] Preserve super namespace callbacks and MRO ownership Share fallible namespace reads with mapping proxies and retain original query behavior without holding the MRO lock across callbacks. Assisted-by: OpenAI Codex:model version not exposed --- crates/vm/src/builtins/mappingproxy.rs | 18 +--- crates/vm/src/builtins/super.rs | 109 ++++++++++++++----------- crates/vm/src/builtins/type.rs | 29 +++++++ 3 files changed, 96 insertions(+), 60 deletions(-) diff --git a/crates/vm/src/builtins/mappingproxy.rs b/crates/vm/src/builtins/mappingproxy.rs index 39aa1e35455..9d4cdbc1cce 100644 --- a/crates/vm/src/builtins/mappingproxy.rs +++ b/crates/vm/src/builtins/mappingproxy.rs @@ -107,9 +107,9 @@ impl PyMappingProxy { key: &PyObject, vm: &VirtualMachine, ) -> PyResult> { - // Use the dictionary lookup for native namespaces too: even a key - // that is not a string can hash or compare equal to an attribute name. - Self::class_mapping(class, vm)?.get_item_opt(key, vm) + // Non-string and string-subclass keys retain their hash and equality + // callbacks even when the namespace uses a native attributes map. + class.attributes.get_item_opt(key, vm) } pub fn __getitem__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { @@ -128,20 +128,10 @@ impl PyMappingProxy { fn to_object(&self, vm: &VirtualMachine) -> PyResult { Ok(match &self.mapping { MappingProxyInner::Mapping(d) => d.as_ref().to_owned(), - MappingProxyInner::Class(c) => Self::class_mapping(c, vm)?.into(), + MappingProxyInner::Class(c) => c.attributes.to_dict(vm)?.into(), }) } - fn class_mapping(class: &Py, vm: &VirtualMachine) -> PyResult { - if let Some(dict) = class.attributes.as_dict() { - // Views and iterators must stay attached to the actual namespace. - return Ok(dict.to_owned()); - } - // Native namespaces without a backing Python dictionary still need a - // snapshot, but must not bypass dict key callbacks. - Ok(PyDict::from_attributes(class.attributes.attributes(&vm.ctx), vm)?.into_ref(&vm.ctx)) - } - fn __len__(&self, vm: &VirtualMachine) -> PyResult { let obj = self.to_object(vm)?; obj.length(vm) diff --git a/crates/vm/src/builtins/super.rs b/crates/vm/src/builtins/super.rs index 3110c3fee73..c1cbdd35de4 100644 --- a/crates/vm/src/builtins/super.rs +++ b/crates/vm/src/builtins/super.rs @@ -90,6 +90,15 @@ pub struct InitArgs { impl Initializer for PySuper { type Args = InitArgs; + fn slot_init(zelf: &PyObject, args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> { + let zelf = zelf.try_to_ref::(vm)?; + // Reject keywords before either positional arity or type conversion. + if !args.kwargs.is_empty() { + return Err(vm.new_type_error("super() takes no keyword arguments")); + } + Self::init(zelf, args.bind_for(vm, "super")?, vm) + } + fn init( zelf: &Py, Self::Args { py_type, object }: Self::Args, @@ -171,15 +180,13 @@ fn super_init_without_args(vm: &VirtualMachine) -> PyResult<(PyTypeRef, PyObject typ = Some(class.downcast().map_err(|o| { vm.new_runtime_error(format!( "super(): __class__ is not a type ({})", - o.class().name() + o.class().slot_name() )) })?); break; } } - let typ = typ.ok_or_else(|| { - vm.new_type_error("super must be called with 1 argument or from inside class method") - })?; + let typ = typ.ok_or_else(|| vm.new_runtime_error("super(): __class__ cell not found"))?; Ok((typ, obj)) } @@ -206,37 +213,39 @@ impl GetAttr for PySuper { return skip(zelf, name); } - if let Some(name) = vm.ctx.interned_str(name) { - // Walk start_type's MRO by reference (no Vec allocation, no - // per-class clone) up to and including zelf.typ, then look for - // the first class past it that declares `name` directly. - // Both locks are dropped before any arbitrary Python code - // (the descriptor call below) runs, so they can't be held - // across a call that might re-enter and want them again. - let Some(su_type) = zelf.typ.load_owned() else { - return skip(zelf, name); - }; - let descr = { - let mro = start_type.mro.read(); - mro.iter() - .skip_while(|cls| !cls.is(&su_type)) - .skip(1) // skip su->type (if any) - .find_map(|cls| cls.get_direct_attr(name)) - }; - if let Some(descr) = descr { - return vm - .call_get_descriptor_specific( - &descr, - // Only pass 'obj' param if this is instance-mode super (See https://bugs.python.org/issue743267) - if obj.is(&start_type) { - None - } else { - Some(&obj) - }, - Some(start_type.as_object()), - ) - .unwrap_or(Ok(descr)); + let Some(su_type) = zelf.typ.load_owned() else { + return skip(zelf, name); + }; + let descr = { + // Dictionary key callbacks can replace the MRO. Keep its original + // members alive without holding the MRO lock while callbacks run. + let mro = start_type.mro.read().clone(); + let mut descr = None; + for cls in mro.iter().skip_while(|cls| !cls.is(&su_type)).skip(1) { + // Keep the original query object so str-subclass hash/equality + // overrides run, and fresh names reach live class dictionaries. + // Raw lookup must propagate failures rather than try super's + // own attributes or a dictionary subclass's __missing__. + descr = cls.attributes.get_item_opt(name.as_object(), vm)?; + if descr.is_some() { + break; + } } + descr + }; + if let Some(descr) = descr { + return vm + .call_get_descriptor_specific( + &descr, + // Only pass 'obj' param if this is instance-mode super (See https://bugs.python.org/issue743267) + if obj.is(&start_type) { + None + } else { + Some(&obj) + }, + Some(start_type.as_object()), + ) + .unwrap_or(Ok(descr)); } skip(zelf, name) } @@ -275,11 +284,15 @@ impl Representable for PySuper { #[inline] fn repr_str(zelf: &Py, _vm: &VirtualMachine) -> PyResult { let type_name = match zelf.typ.load_owned() { - Some(ty) => ty.name().to_owned(), + Some(ty) => ty.slot_name().to_owned(), None => "NULL".to_owned(), }; let repr = match zelf.obj_type.load_owned() { - Some(ty) => format!(", <{} object>>", type_name, ty.name()), + Some(ty) => format!( + ", <{} object>>", + type_name, + ty.slot_name() + ), None => format!(", NULL>"), }; Ok(repr) @@ -297,8 +310,8 @@ fn super_check(ty: &Py, obj: &PyObject, vm: &VirtualMachine) -> PyResult return Ok(obj.class().to_owned()); } - let class_attr = obj.get_attr("__class__", vm)?; - if let Ok(cls) = class_attr.downcast::() + if let Some(class_attr) = vm.get_attribute_opt(obj, identifier!(vm, __class__))? + && let Ok(cls) = class_attr.downcast::() && !cls.is(obj.class()) && cls.fast_issubclass(ty) { @@ -306,16 +319,20 @@ fn super_check(ty: &Py, obj: &PyObject, vm: &VirtualMachine) -> PyResult } let (type_or_instance, obj_str) = match typ { - Some(t) => ("type", t.name().to_owned()), - None => ("instance of", obj.class().name().to_owned()), + Some(t) => ("type", t.slot_name().to_owned()), + None => ("instance of", obj.class().slot_name().to_owned()), }; - Err(vm.new_type_error(format!( - "super(type, obj): obj ({} {}) is not an instance or subtype of type ({}).", - type_or_instance, - obj_str, - ty.name(), - ))) + let message = { + let type_name = ty.slot_name(); + format!( + "super(type, obj): obj ({} {}) is not an instance or subtype of type ({}).", + type_or_instance, + &obj_str[..obj_str.floor_char_boundary(200)], + &type_name[..type_name.floor_char_boundary(200)], + ) + }; + Err(vm.new_type_error(message)) } pub(crate) fn init(context: &'static Context) { diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index 6d3de977c35..dc44c24f12e 100644 --- a/crates/vm/src/builtins/type.rs +++ b/crates/vm/src/builtins/type.rs @@ -512,6 +512,35 @@ impl TypeNamespace { } } + /// Return the live dictionary, or materialize a native namespace snapshot. + pub(crate) fn to_dict(&self, vm: &VirtualMachine) -> PyResult { + if let Some(dict) = self.as_dict() { + // Views, iterators, and callbacks must use the actual namespace. + return Ok(dict.to_owned()); + } + // Native namespaces without a backing Python dictionary still need a + // snapshot, but must not bypass dict key callbacks. + Ok(PyDict::from_attributes(self.attributes(&vm.ctx), vm)?.into_ref(&vm.ctx)) + } + + /// Look up the original key without suppressing hash or equality errors. + pub(crate) fn get_item_opt( + &self, + key: &PyObject, + vm: &VirtualMachine, + ) -> PyResult> { + if matches!(self, Self::Attributes(_)) + && let Some(name) = key.downcast_ref_if_exact::(vm) + { + // Native map keys are exact interned strings, so this combination + // cannot invoke Python callbacks and does not need a dict snapshot. + return Ok(vm.ctx.interned_str(name).and_then(|name| self.get(name))); + } + // Heap dictionaries can contain arbitrary keys and non-interned names. + // Preserve the original object even when it has a string payload. + self.to_dict(vm)?.inner_getitem_opt(key, vm) + } + /// Build a dict-backed namespace holding `attrs`, or fall back to an /// interned-key map when no VM is running to hash the keys with. fn new(attrs: PyAttributes, ctx: &Context) -> Self {