Give heap types a dict namespace (#8606) · sheeeng/rustpython-rustpython@27d13d2 · GitHub
Skip to content

Commit 27d13d2

Browse files
youknowoneyouknowone
authored
Give heap types a dict namespace (RustPython#8606)
Replace `PyType::attributes` (`PyRwLock<PyAttributes>`) 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<PyType>` 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
1 parent ec6fec2 commit 27d13d2

19 files changed

Lines changed: 429 additions & 289 deletions

File tree

‎Lib/test/test_type_params.py‎

Lines changed: 0 additions & 1 deletion

‎Lib/test/test_typing.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5887,7 +5887,6 @@ class A:
58875887
with self.assertRaises(TypeError):
58885888
a[int]
58895889

5890-
@unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: ".+__typing_subst__.+tuple.+int.*" does not match "'TypeAliasType' object is not subscriptable"
58915890
def test_return_non_tuple_while_unpacking(self):
58925891
# GH-138497: GenericAlias objects didn't ensure that __typing_subst__ actually
58935892
# returned a tuple

‎crates/derive-impl/src/pymodule.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,7 @@ impl ModuleItem for ClassItem {
763763
// module resolution, e.g. TypeAliasType)
764764
{
765765
let module_key = rustpython_vm::identifier!(ctx, __module__);
766-
let has_module_getset = new_class.attributes.read()
766+
let has_module_getset = new_class.attributes
767767
.get(module_key)
768768
.is_some_and(|v| v.downcastable::<rustpython_vm::builtins::PyGetSet>());
769769
if !has_module_getset {
@@ -857,7 +857,7 @@ impl ModuleItem for StructSequenceItem {
857857
let new_class = <#pytype_ident as ::rustpython_vm::class::PyClassImpl>::make_static_type();
858858
{
859859
let module_key = rustpython_vm::identifier!(ctx, __module__);
860-
let has_module_getset = new_class.attributes.read()
860+
let has_module_getset = new_class.attributes
861861
.get(module_key)
862862
.is_some_and(|v| v.downcastable::<rustpython_vm::builtins::PyGetSet>());
863863
if !has_module_getset {

‎crates/stdlib/src/pyexpat.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ mod _pyexpat {
173173

174174
#[extend_class]
175175
fn extend_class_with_fields(ctx: &Context, class: &'static Py<PyType>) {
176-
let mut attributes = class.attributes.write();
176+
let attributes = &class.attributes;
177177

178178
create_property!(ctx, attributes, "StartElementHandler", class, start_element);
179179
create_property!(ctx, attributes, "EndElementHandler", class, end_element);

‎crates/vm/src/builtins/genericalias.rs‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -551,12 +551,15 @@ pub(crate) fn subs_parameters(
551551
};
552552

553553
if unpack {
554-
if let Ok(tuple) = substituted_arg.try_to_ref::<PyTuple>(vm) {
555-
for elem in tuple {
556-
new_args.push(elem.clone());
557-
}
558-
} else {
559-
new_args.push(substituted_arg);
554+
let tuple = substituted_arg.try_to_ref::<PyTuple>(vm).map_err(|_| {
555+
vm.new_type_error(format!(
556+
"expected __typing_subst__ of {} objects to return a tuple, not {}",
557+
arg.class().fully_qualified_name(vm),
558+
substituted_arg.class().fully_qualified_name(vm),
559+
))
560+
})?;
561+
for elem in tuple {
562+
new_args.push(elem.clone());
560563
}
561564
} else {
562565
new_args.push(substituted_arg);

‎crates/vm/src/builtins/getset.rs‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,19 @@
22
33
use super::PyType;
44
use crate::{
5-
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyResult, VirtualMachine,
6-
builtins::type_::PointerSlot,
5+
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
76
class::PyClassImpl,
87
function::{IntoPyGetterFunc, IntoPySetterFunc, PyGetterFunc, PySetterFunc, PySetterValue},
8+
object::{Traverse, TraverseFn},
99
types::{GetDescriptor, Representable},
1010
};
1111

12-
#[pyclass(module = false, name = "getset_descriptor")]
12+
#[pyclass(module = false, name = "getset_descriptor", traverse = "manual")]
1313
pub struct PyGetSet {
1414
name: String,
15-
class: PointerSlot<Py<PyType>>, // A class type freed before getset is non-sense.
15+
/// `d_type`. Owned: a type's namespace can outlive the type, and the
16+
/// descriptors it holds have to stay valid for as long as it does.
17+
class: PyRef<PyType>,
1618
getter: Option<PyGetterFunc>,
1719
setter: Option<PySetterFunc>,
1820
// doc: Option<String>,
@@ -38,6 +40,13 @@ impl core::fmt::Debug for PyGetSet {
3840
}
3941
}
4042

43+
// Only `class` is traced: the getter and setter closures are plain functions.
44+
unsafe impl Traverse for PyGetSet {
45+
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
46+
self.class.traverse(tracer_fn);
47+
}
48+
}
49+
4150
impl PyPayload for PyGetSet {
4251
#[inline]
4352
fn class(ctx: &Context) -> &'static Py<PyType> {
@@ -70,10 +79,10 @@ impl GetDescriptor for PyGetSet {
7079

7180
impl PyGetSet {
7281
#[must_use]
73-
pub fn new(name: &str, class: &'static Py<PyType>) -> Self {
82+
pub fn new(name: &str, class: &Py<PyType>) -> Self {
7483
Self {
7584
name: name.into(),
76-
class: PointerSlot::from(class),
85+
class: class.to_owned(),
7786
getter: None,
7887
setter: None,
7988
}
@@ -128,26 +137,22 @@ impl PyGetSet {
128137

129138
#[pygetset]
130139
fn __qualname__(&self) -> String {
131-
format!(
132-
"{}.{}",
133-
unsafe { self.class.borrow_static() }.slot_name(),
134-
self.name.clone()
135-
)
140+
format!("{}.{}", self.class.slot_name(), self.name.clone())
136141
}
137142

138143
#[pymember]
139144
fn __objclass__(vm: &VirtualMachine, zelf: PyObjectRef) -> PyResult {
140145
let zelf: &Py<Self> = zelf.try_to_value(vm)?;
141-
Ok(unsafe { zelf.class.borrow_static() }.to_owned().into())
146+
Ok(zelf.class.clone().into())
142147
}
143148
}
144149

145150
impl Representable for PyGetSet {
146151
#[inline]
147152
fn repr_str(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<String> {
148-
let class = unsafe { zelf.class.borrow_static() };
153+
let class = &zelf.class;
149154
// Special case for object type
150-
if core::ptr::eq(class, vm.ctx.types.object_type) {
155+
if class.is(vm.ctx.types.object_type) {
151156
Ok(format!("<attribute '{}'>", zelf.name))
152157
} else {
153158
Ok(format!(

‎crates/vm/src/builtins/mappingproxy.rs‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,24 @@ impl PyMappingProxy {
9595

9696
fn get_inner(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult<Option<PyObjectRef>> {
9797
match &self.mapping {
98-
MappingProxyInner::Class(class) => Ok(key
99-
.as_interned_str(vm)
100-
.and_then(|key| class.attributes.read().get(key).cloned())),
98+
MappingProxyInner::Class(class) => Self::class_get(class, &key, vm),
10199
MappingProxyInner::Mapping(mapping) => mapping.mapping().subscript(&*key, vm).map(Some),
102100
}
103101
}
104102

103+
fn class_get(
104+
class: &Py<PyType>,
105+
key: &PyObject,
106+
vm: &VirtualMachine,
107+
) -> PyResult<Option<PyObjectRef>> {
108+
match class.attributes.as_dict() {
109+
Some(dict) => dict.get_item_opt(key, vm),
110+
None => Ok(key
111+
.as_interned_str(vm)
112+
.and_then(|key| class.attributes.get(key))),
113+
}
114+
}
115+
105116
#[pymethod]
106117
fn get(
107118
&self,
@@ -124,28 +135,40 @@ impl PyMappingProxy {
124135

125136
fn _contains(&self, key: &PyObject, vm: &VirtualMachine) -> PyResult<bool> {
126137
match &self.mapping {
127-
MappingProxyInner::Class(class) => Ok(key
128-
.as_interned_str(vm)
129-
.is_some_and(|key| class.attributes.read().contains_key(key))),
138+
MappingProxyInner::Class(class) => Ok(Self::class_contains(class, key, vm)),
130139
MappingProxyInner::Mapping(mapping) => {
131140
mapping.obj().sequence_unchecked().contains(key, vm)
132141
}
133142
}
134143
}
135144

145+
fn class_contains(class: &Py<PyType>, key: &PyObject, vm: &VirtualMachine) -> bool {
146+
match class.attributes.as_dict() {
147+
Some(dict) => dict.contains_key(key, vm),
148+
None => key
149+
.as_interned_str(vm)
150+
.is_some_and(|key| class.attributes.contains(key)),
151+
}
152+
}
153+
136154
pub fn __contains__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult<bool> {
137155
self._contains(&key, vm)
138156
}
139157

140158
fn to_object(&self, vm: &VirtualMachine) -> PyResult {
141159
Ok(match &self.mapping {
142160
MappingProxyInner::Mapping(d) => d.as_ref().to_owned(),
143-
MappingProxyInner::Class(c) => {
144-
PyDict::from_attributes(c.attributes.read().clone(), vm)?.to_pyobject(vm)
145-
}
161+
MappingProxyInner::Class(c) => Self::class_to_dict(c, vm)?,
146162
})
147163
}
148164

165+
fn class_to_dict(class: &Py<PyType>, vm: &VirtualMachine) -> PyResult {
166+
if let Some(dict) = class.attributes.as_dict() {
167+
return Ok(dict.copy().to_pyobject(vm));
168+
}
169+
Ok(PyDict::from_attributes(class.attributes.attributes(&vm.ctx), vm)?.to_pyobject(vm))
170+
}
171+
149172
#[pymethod]
150173
pub fn items(&self, vm: &VirtualMachine) -> PyResult {
151174
let obj = self.to_object(vm)?;
@@ -170,9 +193,7 @@ impl PyMappingProxy {
170193
MappingProxyInner::Mapping(d) => {
171194
vm.call_method(d.obj(), identifier!(vm, copy).as_str(), ())
172195
}
173-
MappingProxyInner::Class(c) => {
174-
Ok(PyDict::from_attributes(c.attributes.read().clone(), vm)?.to_pyobject(vm))
175-
}
196+
MappingProxyInner::Class(c) => Self::class_to_dict(c, vm),
176197
}
177198
}
178199

‎crates/vm/src/builtins/object.rs‎

Lines changed: 1 addition & 1 deletion

0 commit comments

Comments
 (0)