From 0ae835a073946f89fee399b2cac41afac9fdd79c Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" <69878+youknowone@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:12:40 +0900 Subject: [PATCH] Give heap types a dict namespace (#8606) Replace `PyType::attributes` (`PyRwLock`) with a `TypeNamespace` enum. A type created while an interpreter is running holds a `PyDict`; the types `Context::genesis` and `PyType::new_static` build keep the interned-key `IndexMap`, since hashing a string needs a VM. `type.__new__` binds `__classdictcell__` to the type's own namespace rather than the namespace dict passed to it, so an annotation scope reads attribute changes made after the class body ran. Removes the `expectedFailure` on `test_type_params.TypeParamsClassScopeTest.test_modified_later`. `PyGetSet` holds its class as a `PyRef` instead of a non-owning `PointerSlot`, and traverses it, because a namespace can outlive the type it belongs to. `PointerSlot` and the `unsafe` on `Context::new_getset` are gone. `subs_parameters` raises `TypeError` when `__typing_subst__` returns a non-tuple in an unpack position, using the new `PyType::fully_qualified_name` for the message. Removes the `expectedFailure` on `test_typing.GenericTests.test_return_non_tuple_while_unpacking`. Assisted-by: Claude --- Lib/test/test_type_params.py | 1 - Lib/test/test_typing.py | 1 - crates/derive-impl/src/pymodule.rs | 4 +- crates/stdlib/src/pyexpat.rs | 2 +- crates/vm/src/builtins/genericalias.rs | 15 +- crates/vm/src/builtins/getset.rs | 33 +- crates/vm/src/builtins/mappingproxy.rs | 45 ++- crates/vm/src/builtins/object.rs | 2 +- crates/vm/src/builtins/type.rs | 465 +++++++++++++++------- crates/vm/src/class.rs | 5 +- crates/vm/src/object/core.rs | 6 +- crates/vm/src/protocol/object.rs | 2 +- crates/vm/src/stdlib/_ctypes/array.rs | 52 +-- crates/vm/src/stdlib/_ctypes/structure.rs | 5 +- crates/vm/src/stdlib/_ctypes/union.rs | 7 +- crates/vm/src/stdlib/sys.rs | 8 +- crates/vm/src/types/slot.rs | 49 +-- crates/vm/src/types/structseq.rs | 6 +- crates/vm/src/vm/context.rs | 10 +- 19 files changed, 429 insertions(+), 289 deletions(-) diff --git a/Lib/test/test_type_params.py b/Lib/test/test_type_params.py index cf9c336f1..27660379e 100644 --- a/Lib/test/test_type_params.py +++ b/Lib/test/test_type_params.py @@ -674,7 +674,6 @@ class TypeParamsClassScopeTest(unittest.TestCase): self.assertEqual(T.__bound__, "global") self.assertEqual(U.__bound__, "class") - @unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: is not def test_modified_later(self): class X: T = int diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index c10fa37be..b5974c72a 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -5887,7 +5887,6 @@ class GenericTests(BaseTestCase): with self.assertRaises(TypeError): a[int] - @unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: ".+__typing_subst__.+tuple.+int.*" does not match "'TypeAliasType' object is not subscriptable" def test_return_non_tuple_while_unpacking(self): # GH-138497: GenericAlias objects didn't ensure that __typing_subst__ actually # returned a tuple diff --git a/crates/derive-impl/src/pymodule.rs b/crates/derive-impl/src/pymodule.rs index 20c94abe9..d74f71356 100644 --- a/crates/derive-impl/src/pymodule.rs +++ b/crates/derive-impl/src/pymodule.rs @@ -763,7 +763,7 @@ impl ModuleItem for ClassItem { // module resolution, e.g. TypeAliasType) { let module_key = rustpython_vm::identifier!(ctx, __module__); - let has_module_getset = new_class.attributes.read() + let has_module_getset = new_class.attributes .get(module_key) .is_some_and(|v| v.downcastable::()); if !has_module_getset { @@ -857,7 +857,7 @@ impl ModuleItem for StructSequenceItem { let new_class = <#pytype_ident as ::rustpython_vm::class::PyClassImpl>::make_static_type(); { let module_key = rustpython_vm::identifier!(ctx, __module__); - let has_module_getset = new_class.attributes.read() + let has_module_getset = new_class.attributes .get(module_key) .is_some_and(|v| v.downcastable::()); if !has_module_getset { diff --git a/crates/stdlib/src/pyexpat.rs b/crates/stdlib/src/pyexpat.rs index 500a8a686..a98839ad3 100644 --- a/crates/stdlib/src/pyexpat.rs +++ b/crates/stdlib/src/pyexpat.rs @@ -173,7 +173,7 @@ mod _pyexpat { #[extend_class] fn extend_class_with_fields(ctx: &Context, class: &'static Py) { - let mut attributes = class.attributes.write(); + let attributes = &class.attributes; create_property!(ctx, attributes, "StartElementHandler", class, start_element); create_property!(ctx, attributes, "EndElementHandler", class, end_element); diff --git a/crates/vm/src/builtins/genericalias.rs b/crates/vm/src/builtins/genericalias.rs index 8004bd535..c9108d632 100644 --- a/crates/vm/src/builtins/genericalias.rs +++ b/crates/vm/src/builtins/genericalias.rs @@ -551,12 +551,15 @@ pub(crate) fn subs_parameters( }; if unpack { - if let Ok(tuple) = substituted_arg.try_to_ref::(vm) { - for elem in tuple { - new_args.push(elem.clone()); - } - } else { - new_args.push(substituted_arg); + let tuple = substituted_arg.try_to_ref::(vm).map_err(|_| { + vm.new_type_error(format!( + "expected __typing_subst__ of {} objects to return a tuple, not {}", + arg.class().fully_qualified_name(vm), + substituted_arg.class().fully_qualified_name(vm), + )) + })?; + for elem in tuple { + new_args.push(elem.clone()); } } else { new_args.push(substituted_arg); diff --git a/crates/vm/src/builtins/getset.rs b/crates/vm/src/builtins/getset.rs index a2f687994..17dcffd4d 100644 --- a/crates/vm/src/builtins/getset.rs +++ b/crates/vm/src/builtins/getset.rs @@ -2,17 +2,19 @@ use super::PyType; use crate::{ - AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyResult, VirtualMachine, - builtins::type_::PointerSlot, + AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, class::PyClassImpl, function::{IntoPyGetterFunc, IntoPySetterFunc, PyGetterFunc, PySetterFunc, PySetterValue}, + object::{Traverse, TraverseFn}, types::{GetDescriptor, Representable}, }; -#[pyclass(module = false, name = "getset_descriptor")] +#[pyclass(module = false, name = "getset_descriptor", traverse = "manual")] pub struct PyGetSet { name: String, - class: PointerSlot>, // A class type freed before getset is non-sense. + /// `d_type`. Owned: a type's namespace can outlive the type, and the + /// descriptors it holds have to stay valid for as long as it does. + class: PyRef, getter: Option, setter: Option, // doc: Option, @@ -38,6 +40,13 @@ impl core::fmt::Debug for PyGetSet { } } +// Only `class` is traced: the getter and setter closures are plain functions. +unsafe impl Traverse for PyGetSet { + fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { + self.class.traverse(tracer_fn); + } +} + impl PyPayload for PyGetSet { #[inline] fn class(ctx: &Context) -> &'static Py { @@ -70,10 +79,10 @@ impl GetDescriptor for PyGetSet { impl PyGetSet { #[must_use] - pub fn new(name: &str, class: &'static Py) -> Self { + pub fn new(name: &str, class: &Py) -> Self { Self { name: name.into(), - class: PointerSlot::from(class), + class: class.to_owned(), getter: None, setter: None, } @@ -128,26 +137,22 @@ impl PyGetSet { #[pygetset] fn __qualname__(&self) -> String { - format!( - "{}.{}", - unsafe { self.class.borrow_static() }.slot_name(), - self.name.clone() - ) + format!("{}.{}", self.class.slot_name(), self.name.clone()) } #[pymember] fn __objclass__(vm: &VirtualMachine, zelf: PyObjectRef) -> PyResult { let zelf: &Py = zelf.try_to_value(vm)?; - Ok(unsafe { zelf.class.borrow_static() }.to_owned().into()) + Ok(zelf.class.clone().into()) } } impl Representable for PyGetSet { #[inline] fn repr_str(zelf: &Py, vm: &VirtualMachine) -> PyResult { - let class = unsafe { zelf.class.borrow_static() }; + let class = &zelf.class; // Special case for object type - if core::ptr::eq(class, vm.ctx.types.object_type) { + if class.is(vm.ctx.types.object_type) { Ok(format!("", zelf.name)) } else { Ok(format!( diff --git a/crates/vm/src/builtins/mappingproxy.rs b/crates/vm/src/builtins/mappingproxy.rs index dd8c689fa..f8c7677e5 100644 --- a/crates/vm/src/builtins/mappingproxy.rs +++ b/crates/vm/src/builtins/mappingproxy.rs @@ -95,13 +95,24 @@ impl PyMappingProxy { fn get_inner(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult> { match &self.mapping { - MappingProxyInner::Class(class) => Ok(key - .as_interned_str(vm) - .and_then(|key| class.attributes.read().get(key).cloned())), + MappingProxyInner::Class(class) => Self::class_get(class, &key, vm), MappingProxyInner::Mapping(mapping) => mapping.mapping().subscript(&*key, vm).map(Some), } } + fn class_get( + class: &Py, + 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))), + } + } + #[pymethod] fn get( &self, @@ -124,15 +135,22 @@ impl PyMappingProxy { fn _contains(&self, key: &PyObject, vm: &VirtualMachine) -> PyResult { match &self.mapping { - MappingProxyInner::Class(class) => Ok(key - .as_interned_str(vm) - .is_some_and(|key| class.attributes.read().contains_key(key))), + 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)), + } + } + pub fn __contains__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { self._contains(&key, vm) } @@ -140,12 +158,17 @@ impl PyMappingProxy { fn to_object(&self, vm: &VirtualMachine) -> PyResult { Ok(match &self.mapping { MappingProxyInner::Mapping(d) => d.as_ref().to_owned(), - MappingProxyInner::Class(c) => { - PyDict::from_attributes(c.attributes.read().clone(), vm)?.to_pyobject(vm) - } + MappingProxyInner::Class(c) => Self::class_to_dict(c, vm)?, }) } + fn class_to_dict(class: &Py, vm: &VirtualMachine) -> PyResult { + if let Some(dict) = class.attributes.as_dict() { + return Ok(dict.copy().to_pyobject(vm)); + } + Ok(PyDict::from_attributes(class.attributes.attributes(&vm.ctx), vm)?.to_pyobject(vm)) + } + #[pymethod] pub fn items(&self, vm: &VirtualMachine) -> PyResult { let obj = self.to_object(vm)?; @@ -170,9 +193,7 @@ impl PyMappingProxy { MappingProxyInner::Mapping(d) => { vm.call_method(d.obj(), identifier!(vm, copy).as_str(), ()) } - MappingProxyInner::Class(c) => { - Ok(PyDict::from_attributes(c.attributes.read().clone(), vm)?.to_pyobject(vm)) - } + MappingProxyInner::Class(c) => Self::class_to_dict(c, vm), } } diff --git a/crates/vm/src/builtins/object.rs b/crates/vm/src/builtins/object.rs index 151b753b2..05d2edd20 100644 --- a/crates/vm/src/builtins/object.rs +++ b/crates/vm/src/builtins/object.rs @@ -220,7 +220,7 @@ fn object_getstate_default(obj: &PyObject, required: bool, vm: &VirtualMachine) } } else { let weakref_name = vm.ctx.intern_str("__weakref__"); - obj.class().attributes.read().contains_key(weakref_name) + obj.class().attributes.contains(weakref_name) }; if has_weakref { basicsize += core::mem::size_of::(); diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index 20ccf64dc..cf7c3abb5 100644 --- a/crates/vm/src/builtins/type.rs +++ b/crates/vm/src/builtins/type.rs @@ -1,6 +1,6 @@ use super::{ - PyClassMethod, PyDictRef, PyList, PyStaticMethod, PyStr, PyStrInterned, PyStrRef, PyTupleRef, - PyUtf8StrRef, PyWeak, mappingproxy::PyMappingProxy, object, union_, + PyClassMethod, PyDict, PyDictRef, PyList, PyStaticMethod, PyStr, PyStrInterned, PyStrRef, + PyTupleRef, PyUtf8StrRef, PyWeak, mappingproxy::PyMappingProxy, object, union_, }; use crate::{ AsObject, Context, Py, PyAtomicRef, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, @@ -33,7 +33,6 @@ use core::{ borrow::Borrow, ops::Deref, pin::Pin, - ptr::NonNull, sync::atomic::{AtomicBool, AtomicPtr, AtomicU32, AtomicU64, Ordering}, }; use indexmap::{IndexMap, map::Entry}; @@ -49,7 +48,7 @@ pub struct PyType { pub bases: PyRwLock>, pub mro: PyRwLock>, pub subclasses: PyRwLock>>, - pub attributes: PyRwLock, + pub attributes: TypeNamespace, pub slots: PyTypeSlots, pub heaptype_ext: Option>>, /// Type version tag for inline caching. 0 means unassigned/invalidated. @@ -245,11 +244,7 @@ unsafe impl crate::object::Traverse for PyType { self.bases.traverse(tracer_fn); self.mro.traverse(tracer_fn); self.subclasses.traverse(tracer_fn); - self.attributes - .read_recursive() - .iter() - .map(|(_, v)| v.traverse(tracer_fn)) - .count(); + self.attributes.traverse(tracer_fn); if let Some(ext) = self.heaptype_ext.as_ref() { ext.specialization_cache.traverse(tracer_fn); } @@ -276,11 +271,7 @@ unsafe impl crate::object::Traverse for PyType { out.push(weak.into()); } } - if let Some(mut guard) = self.attributes.try_write() { - for (_, val) in guard.drain(..) { - out.push(val); - } - } + self.attributes.drain_into(out); if let Some(ext) = self.heaptype_ext.as_ref() { ext.specialization_cache.clear_into(out); } @@ -380,37 +371,6 @@ impl TypeSpecializationCache { } } -pub(crate) struct PointerSlot(NonNull); - -unsafe impl Sync for PointerSlot {} -unsafe impl Send for PointerSlot {} - -impl PointerSlot { - pub(crate) const unsafe fn borrow_static(self) -> &'static T { - unsafe { self.0.as_ref() } - } -} - -impl Clone for PointerSlot { - fn clone(&self) -> Self { - *self - } -} - -impl Copy for PointerSlot {} - -impl From<&'static T> for PointerSlot { - fn from(x: &'static T) -> Self { - Self(NonNull::from(x)) - } -} - -impl AsRef for PointerSlot { - fn as_ref(&self) -> &T { - unsafe { self.0.as_ref() } - } -} - pub type PyTypeRef = PyRef; cfg_select! { @@ -433,6 +393,195 @@ unsafe impl Traverse for PyAttributes { } } +/// A type's namespace, `tp_dict`. +/// +/// Types created while an interpreter is running own a real dict, so the object +/// `__classdictcell__` hands to annotation scopes is the type's own storage. +/// The types `Context::genesis` builds cannot have one: hashing a string needs +/// a VM and none exists yet, so they keep an interned-key map instead. +pub enum TypeNamespace { + Attributes(PyRwLock), + Dict(PyDictRef), +} + +unsafe impl Traverse for TypeNamespace { + fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { + match self { + Self::Attributes(attrs) => attrs.read_recursive().traverse(tracer_fn), + Self::Dict(dict) => tracer_fn(dict.as_object()), + } + } +} + +impl Default for TypeNamespace { + fn default() -> Self { + Self::Attributes(PyRwLock::default()) + } +} + +impl From for TypeNamespace { + fn from(attrs: PyAttributes) -> Self { + Self::Attributes(PyRwLock::new(attrs)) + } +} + +impl TypeNamespace { + /// The namespace as a dict, for the types that have one. + pub const fn as_dict(&self) -> Option<&PyDictRef> { + match self { + Self::Attributes(_) => None, + Self::Dict(dict) => Some(dict), + } + } + + /// 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 { + let built = crate::vm::thread::try_with_current_vm(|vm| { + let dict = ctx.new_dict(); + for (key, value) in &attrs { + dict.set_item(*key, value.clone(), vm)?; + } + PyResult::Ok(dict) + }); + match built { + Some(Ok(dict)) => Self::Dict(dict), + _ => attrs.into(), + } + } + + pub fn get(&self, name: &'static PyStrInterned) -> Option { + match self { + Self::Attributes(attrs) => attrs.read().get(name).cloned(), + Self::Dict(dict) => dict_get(dict, name), + } + } + + pub fn contains(&self, name: &'static PyStrInterned) -> bool { + match self { + Self::Attributes(attrs) => attrs.read().contains_key(name), + Self::Dict(dict) => { + match crate::vm::thread::try_with_current_vm(|vm| dict.contains_key(name, vm)) { + Some(found) => found, + None => dict_get(dict, name).is_some(), + } + } + } + } + + /// Bind `name` to `value`, dropping whatever it displaced. + pub fn set(&self, name: &'static PyStrInterned, value: PyObjectRef) { + match self { + Self::Attributes(attrs) => { + attrs.write().insert(name, value); + } + Self::Dict(dict) => { + if let Some(Err(_)) | None = + crate::vm::thread::try_with_current_vm(|vm| dict.set_item(name, value, vm)) + { + debug_assert!(false, "type namespace write without a running VM"); + } + } + } + } + + /// Bind `name` to `value` and hand back what it displaced, so the caller + /// can decide where the old value is dropped. + pub fn insert(&self, name: &'static PyStrInterned, value: PyObjectRef) -> Option { + match self { + Self::Attributes(attrs) => attrs.write().insert(name, value), + Self::Dict(dict) => { + let previous = dict_get(dict, name); + self.set(name, value); + previous + } + } + } + + pub fn remove(&self, name: &'static PyStrInterned) -> Option { + match self { + Self::Attributes(attrs) => attrs.write().shift_remove(name), + Self::Dict(dict) => { + let previous = dict_get(dict, name)?; + crate::vm::thread::try_with_current_vm(|vm| dict.del_item(name, vm).ok()); + Some(previous) + } + } + } + + /// The namespace contents in insertion order. + pub fn entries(&self) -> Vec<(PyObjectRef, PyObjectRef)> { + match self { + Self::Attributes(attrs) => attrs + .read() + .iter() + .map(|(name, value)| ((*name).to_object(), value.clone())) + .collect(), + Self::Dict(dict) => dict.into_iter().collect(), + } + } + + /// The namespace keyed by interned name; dict keys that are not strings + /// are left out, since `PyAttributes` has nowhere to put them. + pub fn attributes(&self, ctx: &Context) -> PyAttributes { + match self { + Self::Attributes(attrs) => attrs.read().clone(), + Self::Dict(dict) => dict + .into_iter() + .filter_map(|(key, value)| { + let key = key.downcast_ref::()?; + Some((ctx.intern_str(key.as_wtf8()), value)) + }) + .collect(), + } + } + + /// The interned names in the namespace; dict keys that are not strings are + /// left out. + pub fn interned_names(&self, ctx: &Context) -> Vec<&'static PyStrInterned> { + match self { + Self::Attributes(attrs) => attrs.read().keys().copied().collect(), + Self::Dict(dict) => dict + .into_iter() + .filter_map(|(key, _)| { + let key = key.downcast_ref::()?; + Some(ctx.intern_str(key.as_wtf8())) + }) + .collect(), + } + } + + /// Empty the namespace, handing the values to the caller. Used by tp_clear. + fn drain_into(&mut self, out: &mut Vec) { + match self { + Self::Attributes(attrs) => { + if let Some(mut guard) = attrs.try_write() { + out.extend(guard.drain(..).map(|(_, value)| value)); + } + } + Self::Dict(dict) => { + out.extend((&**dict).into_iter().map(|(_, value)| value)); + PyDict::clear(dict); + } + } + } +} + +/// Look a name up in a dict-backed namespace. Falls back to a scan when no VM +/// is running, since hashing the key needs one. +fn dict_get(dict: &Py, name: &'static PyStrInterned) -> Option { + match crate::vm::thread::try_with_current_vm(|vm| dict.get_item_opt(name, vm)) { + Some(found) => found.ok().flatten(), + None => dict + .into_iter() + .find(|(key, _)| { + key.downcast_ref::() + .is_some_and(|key| key.as_wtf8() == name.as_wtf8()) + }) + .map(|(_, value)| value), + } +} + impl core::fmt::Display for PyType { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { core::fmt::Display::fmt(&self.name(), f) @@ -842,7 +991,7 @@ impl PyType { bases: PyRwLock::new(bases), mro: PyRwLock::new(mro), subclasses: PyRwLock::default(), - attributes: PyRwLock::new(attrs), + attributes: TypeNamespace::new(attrs, ctx), slots, heaptype_ext: Some(Pin::new(Box::new(heaptype_ext))), tp_version_tag: AtomicU32::new(0), @@ -904,7 +1053,7 @@ impl PyType { bases, mro: PyRwLock::new(mro), subclasses: PyRwLock::default(), - attributes: PyRwLock::new(attrs), + attributes: attrs.into(), slots, heaptype_ext: None, tp_version_tag: AtomicU32::new(0), @@ -956,13 +1105,13 @@ impl PyType { // mro[0] is self, so skip it; self.attributes is checked separately below for cls in &self.mro.read()[1..] { - for &name in cls.attributes.read().keys() { + for name in cls.attributes.interned_names(ctx) { if name.as_bytes().starts_with(b"__") && name.as_bytes().ends_with(b"__") { slot_name_set.insert(name); } } } - for &name in self.attributes.read().keys() { + for name in self.attributes.interned_names(ctx) { if name.as_bytes().starts_with(b"__") && name.as_bytes().ends_with(b"__") { slot_name_set.insert(name); } @@ -1039,7 +1188,7 @@ impl PyType { // pointers in cache entries are nullified while the source objects // are still alive. self.modified(); - self.attributes.write().insert(attr_name, value); + self.attributes.set(attr_name, value); } /// Internal get_attr implementation for fast lookup on a class. @@ -1233,7 +1382,7 @@ impl PyType { } pub fn get_direct_attr(&self, attr_name: &'static PyStrInterned) -> Option { - self.attributes.read().get(attr_name).cloned() + self.attributes.get(attr_name) } /// find_name_in_mro with method cache (MCACHE). @@ -1251,8 +1400,8 @@ impl PyType { /// Raw MRO walk without cache. fn find_name_in_mro_uncached(&self, name: &'static PyStrInterned) -> Option { for cls in self.mro.read().iter() { - if let Some(value) = cls.attributes.read().get(name) { - return Some(value.clone()); + if let Some(value) = cls.attributes.get(name) { + return Some(value); } } None @@ -1267,7 +1416,7 @@ impl PyType { pub fn get_super_attr(&self, attr_name: &'static PyStrInterned) -> Option { self.mro.read()[1..] .iter() - .find_map(|class| class.attributes.read().get(attr_name).cloned()) + .find_map(|class| class.attributes.get(attr_name)) } /// Fast lookup for attribute existence on a class. @@ -1310,15 +1459,13 @@ impl PyType { self.find_name_in_mro(name).is_some() } - pub fn get_attributes(&self) -> PyAttributes { + pub fn get_attributes(&self, ctx: &Context) -> PyAttributes { // Gather all members here: let mut attributes = PyAttributes::default(); // mro[0] is self, so we iterate through the entire MRO in reverse for bc in self.mro.read().iter().map(|cls| -> &Self { cls }).rev() { - for (name, value) in bc.attributes.read().iter() { - attributes.insert(name.to_owned(), value.clone()); - } + attributes.extend(bc.attributes.attributes(ctx)); } attributes @@ -1361,6 +1508,21 @@ impl PyType { ) } + /// The type's fully qualified name, the way the `%T` format code prints it: + /// `module.qualname`, with a `builtins` or `__main__` module left off. + pub fn fully_qualified_name(&self, vm: &VirtualMachine) -> String { + let qualname = self.__qualname__(vm); + let qualname = qualname + .downcast_ref::() + .and_then(|qualname| qualname.to_str()) + .map_or_else(|| self.name().to_string(), str::to_owned); + let module = self.__module__(vm); + match module.downcast_ref::().and_then(|m| m.to_str()) { + Some("builtins" | "__main__") | None => qualname, + Some(module) => format!("{module}.{qualname}"), + } + } + pub fn name(&self) -> BorrowedValue<'_, str> { self.name_inner( |name| name.rsplit_once('.').map_or(name, |(_, name)| name).into(), @@ -1729,26 +1891,23 @@ impl PyType { let annotate_key = identifier!(vm, __annotate__); let annotate_func_key = identifier!(vm, __annotate_func__); - let attrs = self.attributes.read(); - if let Some(annotate) = attrs.get(annotate_key).cloned() { + if let Some(annotate) = self.attributes.get(annotate_key) { return Ok(annotate); } - if let Some(annotate) = attrs.get(annotate_func_key).cloned() { + if let Some(annotate) = self.attributes.get(annotate_func_key) { return Ok(annotate); } - drop(attrs); let none = vm.ctx.none(); let (result, _prev) = Self::with_type_lock(vm, || { - let mut attrs = self.attributes.write(); - if let Some(annotate) = attrs.get(annotate_key).cloned() { + if let Some(annotate) = self.attributes.get(annotate_key) { return (annotate, None); } - if let Some(annotate) = attrs.get(annotate_func_key).cloned() { + if let Some(annotate) = self.attributes.get(annotate_func_key) { return (annotate, None); } self.modified_inner(); - let prev = attrs.insert(annotate_func_key, none.clone()); + let prev = self.attributes.insert(annotate_func_key, none.clone()); (none, prev) }); Ok(result) @@ -1776,14 +1935,16 @@ impl PyType { let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); - let mut attrs = self.attributes.write(); // Clear cached annotations only when setting to a new callable let removed = if !vm.is_none(&value) { - attrs.swap_remove(identifier!(vm, __annotations_cache__)) + self.attributes + .remove(identifier!(vm, __annotations_cache__)) } else { None }; - let prev = attrs.insert(identifier!(vm, __annotate_func__), value); + let prev = self + .attributes + .insert(identifier!(vm, __annotate_func__), value); (removed, prev) }); @@ -1794,8 +1955,7 @@ impl PyType { fn __annotations__(&self, vm: &VirtualMachine) -> PyResult { let annotations_key = identifier!(vm, __annotations__); let annotations_cache_key = identifier!(vm, __annotations_cache__); - let attrs = self.attributes.read(); - if let Some(annotations) = attrs.get(annotations_key).cloned() { + if let Some(annotations) = self.attributes.get(annotations_key) { // Ignore the __annotations__ descriptor stored on type itself. if !annotations.class().is(vm.ctx.types.getset_type) { if vm.is_none(&annotations) @@ -1810,7 +1970,7 @@ impl PyType { ))); } } - if let Some(annotations) = attrs.get(annotations_cache_key).cloned() { + if let Some(annotations) = self.attributes.get(annotations_cache_key) { if vm.is_none(&annotations) || annotations.class().is(vm.ctx.types.dict_type) || self.slots.flags.has_feature(PyTypeFlags::HEAPTYPE) @@ -1822,7 +1982,6 @@ impl PyType { self.name() ))); } - drop(attrs); if !self.slots.flags.has_feature(PyTypeFlags::HEAPTYPE) { return Err(vm.new_attribute_error(format!( @@ -1848,17 +2007,18 @@ impl PyType { }; let (result, _prev) = Self::with_type_lock(vm, || { - let mut attrs = self.attributes.write(); - if let Some(existing) = attrs.get(annotations_key).cloned() + if let Some(existing) = self.attributes.get(annotations_key) && !existing.class().is(vm.ctx.types.getset_type) { return (existing, None); } - if let Some(existing) = attrs.get(annotations_cache_key).cloned() { + if let Some(existing) = self.attributes.get(annotations_cache_key) { return (existing, None); } self.modified_inner(); - let prev = attrs.insert(annotations_cache_key, annotations.clone()); + let prev = self + .attributes + .insert(annotations_cache_key, annotations.clone()); (annotations, prev) }); Ok(result) @@ -1879,8 +2039,7 @@ impl PyType { let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); - let mut attrs = self.attributes.write(); - let has_annotations = attrs.contains_key(identifier!(vm, __annotations__)); + let has_annotations = self.attributes.contains(identifier!(vm, __annotations__)); let mut prev = Vec::new(); match value { @@ -1890,28 +2049,35 @@ impl PyType { } else { identifier!(vm, __annotations_cache__) }; - prev.extend(attrs.insert(key, value)); + prev.extend(self.attributes.insert(key, value)); if has_annotations { - prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); + prev.extend( + self.attributes + .remove(identifier!(vm, __annotations_cache__)), + ); } } crate::function::PySetterValue::Delete => { let removed = if has_annotations { - attrs.swap_remove(identifier!(vm, __annotations__)) + self.attributes.remove(identifier!(vm, __annotations__)) } else { - attrs.swap_remove(identifier!(vm, __annotations_cache__)) + self.attributes + .remove(identifier!(vm, __annotations_cache__)) }; if removed.is_none() { return Err(vm.new_attribute_error("__annotations__")); } prev.extend(removed); if has_annotations { - prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); + prev.extend( + self.attributes + .remove(identifier!(vm, __annotations_cache__)), + ); } } } - prev.extend(attrs.swap_remove(identifier!(vm, __annotate_func__))); - prev.extend(attrs.swap_remove(identifier!(vm, __annotate__))); + prev.extend(self.attributes.remove(identifier!(vm, __annotate_func__))); + prev.extend(self.attributes.remove(identifier!(vm, __annotate__))); Ok(prev) })?; @@ -1921,9 +2087,7 @@ impl PyType { #[pygetset] pub fn __module__(&self, vm: &VirtualMachine) -> PyObjectRef { self.attributes - .read() .get(identifier!(vm, __module__)) - .cloned() // We need to exclude this method from going into recursion: .filter(|found| !found.fast_isinstance(vm.ctx.types.getset_type)) .unwrap_or_else(|| { @@ -1942,9 +2106,8 @@ impl PyType { self.check_set_special_type_attr(identifier!(vm, __module__), vm)?; let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); - let mut attributes = self.attributes.write(); - let removed = attributes.swap_remove(identifier!(vm, __firstlineno__)); - let prev = attributes.insert(identifier!(vm, __module__), value); + let removed = self.attributes.remove(identifier!(vm, __firstlineno__)); + let prev = self.attributes.insert(identifier!(vm, __module__), value); (removed, prev) }); Ok(()) @@ -2056,10 +2219,9 @@ impl PyType { #[pygetset] fn __type_params__(&self, vm: &VirtualMachine) -> PyTupleRef { - let attrs = self.attributes.read(); let key = identifier!(vm, __type_params__); - if let Some(params) = attrs.get(&key) - && let Ok(tuple) = params.clone().downcast::() + if let Some(params) = self.attributes.get(key) + && let Ok(tuple) = params.downcast::() { return tuple; } @@ -2079,7 +2241,7 @@ impl PyType { self.check_set_special_type_attr(key, vm)?; let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().insert(key, val.into()) + self.attributes.insert(key, val.into()) }); } PySetterValue::Delete => { @@ -2091,7 +2253,7 @@ impl PyType { } let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().shift_remove(&key) + self.attributes.remove(key) }); } } @@ -2441,28 +2603,32 @@ impl Constructor for PyType { } } - { - let mut attrs = typ.attributes.write(); - if let Some(cell) = attrs.get(identifier!(vm, __classcell__)) { - let cell = PyCellRef::try_from_object(vm, cell.clone()).map_err(|_| { - vm.new_type_error(format!( - "__classcell__ must be a nonlocal cell, not {}", - cell.class().name() - )) - })?; - cell.set(Some(typ.clone().into())); - attrs.shift_remove(identifier!(vm, __classcell__)); - } - if let Some(cell) = attrs.get(identifier!(vm, __classdictcell__)) { - let cell = PyCellRef::try_from_object(vm, cell.clone()).map_err(|_| { - vm.new_type_error(format!( - "__classdictcell__ must be a nonlocal cell, not {}", - cell.class().name() - )) - })?; - cell.set(Some(dict.clone().into())); - attrs.shift_remove(identifier!(vm, __classdictcell__)); - } + if let Some(cell) = typ.attributes.get(identifier!(vm, __classcell__)) { + let cell = PyCellRef::try_from_object(vm, cell.clone()).map_err(|_| { + vm.new_type_error(format!( + "__classcell__ must be a nonlocal cell, not {}", + cell.class().name() + )) + })?; + cell.set(Some(typ.clone().into())); + typ.attributes.remove(identifier!(vm, __classcell__)); + } + if let Some(cell) = typ.attributes.get(identifier!(vm, __classdictcell__)) { + let cell = PyCellRef::try_from_object(vm, cell.clone()).map_err(|_| { + vm.new_type_error(format!( + "__classdictcell__ must be a nonlocal cell, not {}", + cell.class().name() + )) + })?; + // Annotation scopes look names up here long after the class body ran, + // so the cell gets the type's own namespace rather than the dict passed + // to __new__: later attribute changes have to be visible through it. + let namespace = typ + .attributes + .as_dict() + .expect("a type built by type.__new__ has a dict namespace"); + cell.set(Some(namespace.clone().into())); + typ.attributes.remove(identifier!(vm, __classdictcell__)); } // All *classes* should have a dict. Exceptions are *instances* of @@ -2482,14 +2648,12 @@ impl Constructor for PyType { .mro .read() .iter() - .any(|base| base.attributes.read().contains_key(&__dict__)); - if !typ.attributes.read().contains_key(&__dict__) && !has_inherited_dict { - unsafe { - let descriptor = - vm.ctx - .new_getset("__dict__", &typ, subtype_get_dict, subtype_set_dict); - typ.attributes.write().insert(__dict__, descriptor.into()); - } + .any(|base| base.attributes.contains(__dict__)); + if !typ.attributes.contains(__dict__) && !has_inherited_dict { + let descriptor = + vm.ctx + .new_getset("__dict__", &typ, subtype_get_dict, subtype_set_dict); + typ.attributes.set(__dict__, descriptor.into()); } } @@ -2500,19 +2664,15 @@ impl Constructor for PyType { .mro .read() .iter() - .any(|base| base.attributes.read().contains_key(&__weakref__)); - if !typ.attributes.read().contains_key(&__weakref__) && !has_inherited_weakref { - unsafe { - let descriptor = vm.ctx.new_getset( - "__weakref__", - &typ, - subtype_get_weakref, - subtype_set_weakref, - ); - typ.attributes - .write() - .insert(__weakref__, descriptor.into()); - } + .any(|base| base.attributes.contains(__weakref__)); + if !typ.attributes.contains(__weakref__) && !has_inherited_weakref { + let descriptor = vm.ctx.new_getset( + "__weakref__", + &typ, + subtype_get_weakref, + subtype_set_weakref, + ); + typ.attributes.set(__weakref__, descriptor.into()); } } @@ -2521,29 +2681,32 @@ impl Constructor for PyType { // which ensures every type has a __doc__ entry in its dict { let __doc__ = identifier!(vm, __doc__); - if !typ.attributes.read().contains_key(&__doc__) { - typ.attributes.write().insert(__doc__, vm.ctx.none()); + if !typ.attributes.contains(__doc__) { + typ.attributes.set(__doc__, vm.ctx.none()); } } // avoid deadlock let attributes = typ .attributes - .read() - .iter() + .entries() + .into_iter() .filter_map(|(name, obj)| { vm.get_method(obj.clone(), identifier!(vm, __set_name__)) - .map(|res| res.map(|meth| (obj.clone(), name.to_owned(), meth))) + .map(|res| res.map(|meth| (obj, name, meth))) }) .collect::>>()?; for (obj, name, set_name) in attributes { + let name_repr = name + .str(vm) + .map_or_else(|_| "?".to_owned(), |name| name.to_string()); set_name.call((typ.clone(), name), vm).inspect_err(|e| { // PEP 678: Add a note to the original exception instead of wrapping it // (Python 3.12+, gh-77757) let note = format!( "Error calling __set_name__ on '{}' instance '{}' in '{}'", obj.class().name(), - name, + name_repr, typ.name() ); // Ignore result - adding a note is best-effort, the original exception is what matters @@ -2721,18 +2884,16 @@ impl Py { let _prev_value = PyType::with_type_lock(vm, || { self.modified_inner(); - self.attributes - .write() - .insert(identifier!(vm, __doc__), value) + self.attributes.insert(identifier!(vm, __doc__), value) }); Ok(()) } #[pymethod] - fn __dir__(&self) -> PyList { + fn __dir__(&self, vm: &VirtualMachine) -> PyList { let attributes: Vec = self - .get_attributes() + .get_attributes(&vm.ctx) .into_iter() .map(|(k, _)| k.to_object()) .collect(); @@ -2796,9 +2957,9 @@ impl SetAttr for PyType { zelf.modified_inner(); let prev_value = if let PySetterValue::Assign(value) = value { - zelf.attributes.write().insert(attr_name, value) + zelf.attributes.insert(attr_name, value) } else { - let prev_value = zelf.attributes.write().shift_remove(attr_name); // TODO: swap_remove applicable? + let prev_value = zelf.attributes.remove(attr_name); if prev_value.is_none() { return Err(vm.new_attribute_error(format!( "type object '{}' has no attribute '{}'", diff --git a/crates/vm/src/class.rs b/crates/vm/src/class.rs index d9f8b848d..0b86c67f7 100644 --- a/crates/vm/src/class.rs +++ b/crates/vm/src/class.rs @@ -46,7 +46,7 @@ pub fn add_operators(class: &'static Py, ctx: &Context) { // Check if attribute already exists in dict let attr_name = ctx.intern_str(def.name); - if class.attributes.read().contains_key(attr_name) { + if class.attributes.contains(attr_name) { continue; } @@ -177,7 +177,7 @@ pub trait PyClassImpl: PyClassDef { // Only set __doc__ if it doesn't already exist (e.g., as a member descriptor) // This matches CPython's behavior in type_dict_set_doc let doc_attr_name = identifier!(ctx, __doc__); - if class.attributes.read().get(doc_attr_name).is_none() { + if class.attributes.get(doc_attr_name).is_none() { class.set_attr(doc_attr_name, ctx.new_str(doc).into()); } } @@ -188,7 +188,6 @@ pub trait PyClassImpl: PyClassDef { // has an instance-level __module__ getset that should not be replaced) let has_getset = class .attributes - .read() .get(module_key) .is_some_and(|v| v.downcastable::()); if !has_getset { diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index 02ffe2c7e..b0ebfb186 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -2773,7 +2773,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { bases: PyRwLock::default(), mro: PyRwLock::default(), subclasses: PyRwLock::default(), - attributes: PyRwLock::new(Default::default()), + attributes: Default::default(), slots: PyType::make_slots(), heaptype_ext: None, tp_version_tag: core::sync::atomic::AtomicU32::new(0), @@ -2784,7 +2784,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { bases: PyRwLock::default(), mro: PyRwLock::default(), subclasses: PyRwLock::default(), - attributes: PyRwLock::new(Default::default()), + attributes: Default::default(), slots: object::PyBaseObject::make_slots(), heaptype_ext: None, tp_version_tag: core::sync::atomic::AtomicU32::new(0), @@ -2885,7 +2885,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { bases: PyRwLock::new(vec![object_type.clone()]), mro: PyRwLock::new(vec![object_type.clone()]), subclasses: PyRwLock::default(), - attributes: PyRwLock::default(), + attributes: Default::default(), slots: PyWeak::make_slots(), heaptype_ext: None, tp_version_tag: core::sync::atomic::AtomicU32::new(0), diff --git a/crates/vm/src/protocol/object.rs b/crates/vm/src/protocol/object.rs index 993f3442a..1e312a2a3 100644 --- a/crates/vm/src/protocol/object.rs +++ b/crates/vm/src/protocol/object.rs @@ -57,7 +57,7 @@ impl PyObjectRef { // PyObject *PyObject_Dir(PyObject *o) pub fn dir(self, vm: &VirtualMachine) -> PyResult { - let attributes = self.class().get_attributes(); + let attributes = self.class().get_attributes(&vm.ctx); let dict = PyDict::from_attributes(attributes, vm)?.into_ref(&vm.ctx); diff --git a/crates/vm/src/stdlib/_ctypes/array.rs b/crates/vm/src/stdlib/_ctypes/array.rs index 965f7257e..9f725cece 100644 --- a/crates/vm/src/stdlib/_ctypes/array.rs +++ b/crates/vm/src/stdlib/_ctypes/array.rs @@ -173,18 +173,10 @@ impl Initializer for PyCArrayType { new_type.check_not_initialized(vm)?; // 1. Get _length_ from class dict first - let direct_length = new_type - .attributes - .read() - .get(vm.ctx.intern_str("_length_")) - .cloned(); + let direct_length = new_type.attributes.get(vm.ctx.intern_str("_length_")); // 2. Get _type_ from class dict first - let direct_type = new_type - .attributes - .read() - .get(vm.ctx.intern_str("_type_")) - .cloned(); + let direct_type = new_type.attributes.get(vm.ctx.intern_str("_type_")); // 3. Find parent StgInfo from MRO (for inheritance) // Note: PyType.mro does NOT include self, so no skip needed @@ -1158,44 +1150,34 @@ fn wchar_array_set_value( /// add_getset for c_char arrays - adds 'value' and 'raw' attributes /// add_getset((PyTypeObject*)self, CharArray_getsets) fn add_char_array_getsets(array_type: &Py, vm: &VirtualMachine) { - // SAFETY: getset is owned by array_type which outlives the getset - let value_getset = unsafe { - vm.ctx.new_getset( - "value", - array_type, - char_array_get_value, - char_array_set_value, - ) - }; - let raw_getset = unsafe { - vm.ctx - .new_getset("raw", array_type, char_array_get_raw, char_array_set_raw) - }; + let value_getset = vm.ctx.new_getset( + "value", + array_type, + char_array_get_value, + char_array_set_value, + ); + let raw_getset = vm + .ctx + .new_getset("raw", array_type, char_array_get_raw, char_array_set_raw); array_type .attributes - .write() .insert(vm.ctx.intern_str("value"), value_getset.into()); array_type .attributes - .write() .insert(vm.ctx.intern_str("raw"), raw_getset.into()); } /// add_getset for c_wchar arrays - adds only 'value' attribute (no 'raw') fn add_wchar_array_getsets(array_type: &Py, vm: &VirtualMachine) { - // SAFETY: getset is owned by array_type which outlives the getset - let value_getset = unsafe { - vm.ctx.new_getset( - "value", - array_type, - wchar_array_get_value, - wchar_array_set_value, - ) - }; + let value_getset = vm.ctx.new_getset( + "value", + array_type, + wchar_array_get_value, + wchar_array_set_value, + ); array_type .attributes - .write() .insert(vm.ctx.intern_str("value"), value_getset.into()); } diff --git a/crates/vm/src/stdlib/_ctypes/structure.rs b/crates/vm/src/stdlib/_ctypes/structure.rs index 12ddf8b5d..e9c36370c 100644 --- a/crates/vm/src/stdlib/_ctypes/structure.rs +++ b/crates/vm/src/stdlib/_ctypes/structure.rs @@ -626,7 +626,6 @@ impl SetAttr for PyCStructType { // Set the _fields_ attribute on the type pytype .attributes - .write() .insert(vm.ctx.intern_str("_fields_"), fields_value); return Ok(()); } @@ -644,9 +643,9 @@ impl SetAttr for PyCStructType { // Store in type's attributes dict if let PySetterValue::Assign(value) = value { - pytype.attributes.write().insert(attr_name_interned, value); + pytype.attributes.set(attr_name_interned, value); } else { - let prev = pytype.attributes.write().shift_remove(attr_name_interned); + let prev = pytype.attributes.remove(attr_name_interned); if prev.is_none() { return Err(vm.new_attribute_error(format!( "type object '{}' has no attribute '{}'", diff --git a/crates/vm/src/stdlib/_ctypes/union.rs b/crates/vm/src/stdlib/_ctypes/union.rs index 326e1fbd7..982c6d711 100644 --- a/crates/vm/src/stdlib/_ctypes/union.rs +++ b/crates/vm/src/stdlib/_ctypes/union.rs @@ -512,13 +512,10 @@ impl SetAttr for PyCUnionType { // Store in type's attributes dict match &value { PySetterValue::Assign(v) => { - pytype - .attributes - .write() - .insert(attr_name_interned, v.clone()); + pytype.attributes.set(attr_name_interned, v.clone()); } PySetterValue::Delete => { - let prev = pytype.attributes.write().shift_remove(attr_name_interned); + let prev = pytype.attributes.remove(attr_name_interned); if prev.is_none() { return Err(vm.new_attribute_error(format!( "type object '{}' has no attribute '{}'", diff --git a/crates/vm/src/stdlib/sys.rs b/crates/vm/src/stdlib/sys.rs index 5ee36b450..85488656a 100644 --- a/crates/vm/src/stdlib/sys.rs +++ b/crates/vm/src/stdlib/sys.rs @@ -1333,15 +1333,11 @@ pub mod sys { return Err(vm.new_type_error("argument is immutable")); } - let mut attributes = type_obj.attributes.write(); - // Remove __dict__ descriptor if present - attributes.swap_remove(identifier!(vm, __dict__)); + type_obj.attributes.remove(identifier!(vm, __dict__)); // Remove __weakref__ descriptor if present - attributes.swap_remove(identifier!(vm, __weakref__)); - - drop(attributes); + type_obj.attributes.remove(identifier!(vm, __weakref__)); // Update slots to notify subclasses and recalculate cached values type_obj.update_slot::(identifier!(vm, __dict__), &vm.ctx); diff --git a/crates/vm/src/types/slot.rs b/crates/vm/src/types/slot.rs index c0b9142c7..d44b0789a 100644 --- a/crates/vm/src/types/slot.rs +++ b/crates/vm/src/types/slot.rs @@ -709,7 +709,7 @@ impl PyType { }; // Skip if subclass has its own definition for this attribute - if subclass.attributes.read().contains_key(name) { + if subclass.attributes.contains(name) { continue; } @@ -780,9 +780,8 @@ impl PyType { // methods (__setitem__ and __delitem__). If any of those methods // is defined, we must use the wrapper to ensure Python method calls. let has_own = { - let guard = self.attributes.read(); // Check the current method name - let mut result = guard.contains_key(name); + let mut result = self.attributes.contains(name); // For ass_item/ass_subscript slots, also check the paired method // (__setitem__ and __delitem__ share the same slot) if !result @@ -791,7 +790,8 @@ impl PyType { { let setitem = ctx.intern_str("__setitem__"); let delitem = ctx.intern_str("__delitem__"); - result = guard.contains_key(setitem) || guard.contains_key(delitem); + result = self.attributes.contains(setitem) + || self.attributes.contains(delitem); } result }; @@ -836,11 +836,11 @@ impl PyType { SlotAccessor::TpHash => { // Special handling for __hash__ = None if ADD { - let method = self.attributes.read().get(name).cloned().or_else(|| { + let method = self.attributes.get(name).or_else(|| { self.mro .read() .iter() - .find_map(|cls| cls.attributes.read().get(name).cloned()) + .find_map(|cls| cls.attributes.get(name)) }); if method.as_ref().is_some_and(|m| m.is(&ctx.none)) { @@ -892,13 +892,13 @@ impl PyType { // builtin __new__ entry (or no entry at all) means the slot // is inherited from the solid base, matching update_one_slot's // tp_new special case over the tp_base-inherited value. - let needs_wrapper = if ADD && self.attributes.read().contains_key(name) { + let needs_wrapper = if ADD && self.attributes.contains(name) { true } else { // mro[0] is self, so skip it self.mro.read()[1..] .iter() - .find(|cls| cls.attributes.read().contains_key(name)) + .find(|cls| cls.attributes.contains(name)) .is_some_and(|cls| { cls.slots.new.load().map(|f| fn_addr(f)) == Some(fn_addr(new_wrapper as NewFunc)) @@ -919,17 +919,14 @@ impl PyType { // because the native slot won't call __getattr__. let __getattr__ = identifier!(ctx, __getattr__); let has_getattr = { - let attrs = self.attributes.read(); - let in_self = attrs.contains_key(__getattr__); - drop(attrs); // mro[0] is self, so skip it - in_self + self.attributes.contains(__getattr__) || self .mro .read() .iter() .skip(1) - .any(|cls| cls.attributes.read().contains_key(__getattr__)) + .any(|cls| cls.attributes.contains(__getattr__)) }; if has_getattr { @@ -1043,23 +1040,15 @@ impl PyType { ]; let has_python_cmp = { - // Check self first - let attrs = self.attributes.read(); - let in_self = cmp_names.iter().any(|n| attrs.contains_key(*n)); - drop(attrs); - - // mro[0] is self, so skip it since we already checked self above - in_self + // Check self first, then the rest of the MRO + cmp_names.iter().any(|n| self.attributes.contains(n)) || self.mro.read()[1..].iter().any(|cls| { - let attrs = cls.attributes.read(); cmp_names.iter().any(|n| { - if let Some(attr) = attrs.get(*n) { + cls.attributes.get(n).is_some_and(|attr| { // Check if it's a Python function (not a native descriptor) !attr.class().is(ctx.types.wrapper_descriptor_type) && !attr.class().is(ctx.types.method_descriptor_type) - } else { - false - } + }) }) }) }; @@ -1515,10 +1504,9 @@ impl PyType { // SqAssItem is shared by __setitem__ (SeqSetItem) and __delitem__ (SeqDelItem) if ADD { let has_own = { - let guard = self.attributes.read(); let setitem = ctx.intern_str("__setitem__"); let delitem = ctx.intern_str("__delitem__"); - guard.contains_key(setitem) || guard.contains_key(delitem) + self.attributes.contains(setitem) || self.attributes.contains(delitem) }; if has_own { self.slots @@ -1568,10 +1556,9 @@ impl PyType { // MpAssSubscript is shared by __setitem__ (MapSetSubscript) and __delitem__ (MapDelSubscript) if ADD { let has_own = { - let guard = self.attributes.read(); let setitem = ctx.intern_str("__setitem__"); let delitem = ctx.intern_str("__delitem__"); - guard.contains_key(setitem) || guard.contains_key(delitem) + self.attributes.contains(setitem) || self.attributes.contains(delitem) }; if has_own { self.slots @@ -1694,7 +1681,7 @@ impl PyType { let mro = self.mro.read(); // Look up in self's dict first - let attr_name = self.attributes.read().get(name).cloned(); + let attr_name = self.attributes.get(name); if let Some(attr) = attr_name { if let Some(func) = try_extract(&attr, &mro) { return SlotLookupResult::NativeSlot(func); @@ -1704,7 +1691,7 @@ impl PyType { // Look up in MRO (mro[0] is self, so skip it) for (i, cls) in mro[1..].iter().enumerate() { - let attr_name = cls.attributes.read().get(name).cloned(); + let attr_name = cls.attributes.get(name); if let Some(attr) = attr_name { // Use the slice starting from this class in MRO if let Some(func) = try_extract(&attr, &mro[i + 1..]) { diff --git a/crates/vm/src/types/structseq.rs b/crates/vm/src/types/structseq.rs index 7f8099e7e..812a8a1fc 100644 --- a/crates/vm/src/types/structseq.rs +++ b/crates/vm/src/types/structseq.rs @@ -441,11 +441,7 @@ pub trait PyStructSequence: StaticType + PyClassImpl + Sized + 'static { // Default __reduce__: only set if not already overridden by the impl's extend_class. // This allows struct sequences like sched_param to provide a custom __reduce__ // (equivalent to METH_COEXIST in structseq.c). - if !class - .attributes - .read() - .contains_key(ctx.intern_str("__reduce__")) - { + if !class.attributes.contains(ctx.intern_str("__reduce__")) { class.set_attr( ctx.intern_str("__reduce__"), DEFAULT_STRUCTSEQ_REDUCE.to_proper_method(class, ctx), diff --git a/crates/vm/src/vm/context.rs b/crates/vm/src/vm/context.rs index 5deaffb3f..334ba7b7d 100644 --- a/crates/vm/src/vm/context.rs +++ b/crates/vm/src/vm/context.rs @@ -672,11 +672,7 @@ impl Context { } /// Creates a new [`PyGetSet`] with a heap type. - /// - /// # Safety - /// In practice, this constructor is safe because a getset is always owned by its `class` type. - /// However, it can be broken if used unconventionally. - pub unsafe fn new_getset( + pub fn new_getset( &self, name: &str, class: &Py, @@ -687,8 +683,8 @@ impl Context { G: IntoPyGetterFunc, S: IntoPySetterFunc, { - let class = unsafe { &*(class as *const _) }; - self.new_static_getset(name, class, g, s) + let getset = PyGetSet::new(name, class).with_get(g).with_set(s); + PyRef::new_ref(getset, self.types.getset_type.to_owned(), None) } pub fn new_base_object(&self, class: PyTypeRef, dict: Option) -> PyObjectRef {