Skip to content

Commit a14141c

Browse files
youknowoneyouknowone
committed
Fix CodeRabbit review issues and improve genericalias
- genericalias: split ATTR_EXCEPTIONS/ATTR_BLOCKED, add starred equality, indexed repr_arg for mutation safety, recursive make_parameters/subs_parameters for list/tuple args, implement gaiterobject (PyGenericAliasIterator), starred pickle via gaiterobject - typing: remove Hashable from TypeAliasType (identity hash), fix evaluate_value for eager aliases, dynamic type name in error message, add spell-checker ignore for typevartuples - symboltable: use scope type check instead of class_name for can_see_class_scope, use parent can_see_class_scope for type param bound/default - pyast: add IMMUTABLETYPE removal for NodeExprConstant
1 parent e5f325b commit a14141c

File tree

6 files changed

+58
-48
lines changed

6 files changed

+58
-48
lines changed

‎Lib/test/test_typing.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -986,7 +986,6 @@ class C(Generic[T]): pass
986986
)
987987

988988

989-
@unittest.expectedFailure # TODO: RUSTPYTHON
990989
def test_two_parameters(self):
991990
T1 = TypeVar('T1')
992991
T2 = TypeVar('T2')

‎crates/codegen/src/symboltable.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1466,6 +1466,11 @@ impl SymbolTableBuilder {
14661466
}) => {
14671467
let was_in_type_alias = self.in_type_alias;
14681468
self.in_type_alias = true;
1469+
// Check before entering any sub-scopes
1470+
let in_class = self
1471+
.tables
1472+
.last()
1473+
.is_some_and(|t| t.typ == CompilerScope::Class);
14691474
let is_generic = type_params.is_some();
14701475
if let Some(type_params) = type_params {
14711476
self.enter_type_param_block(
@@ -1480,7 +1485,7 @@ impl SymbolTableBuilder {
14801485
CompilerScope::TypeParams,
14811486
self.line_index_start(value.range()),
14821487
);
1483-
if self.class_name.is_some() {
1488+
if in_class {
14841489
if let Some(table) = self.tables.last_mut() {
14851490
table.can_see_class_scope = true;
14861491
}
@@ -1982,12 +1987,11 @@ impl SymbolTableBuilder {
19821987
) -> SymbolTableResult {
19831988
// Enter a new TypeParams scope for the bound/default expression
19841989
// This allows the expression to access outer scope symbols
1990+
let in_class = self.tables.last().is_some_and(|t| t.can_see_class_scope);
19851991
let line_number = self.line_index_start(expr.range());
19861992
self.enter_scope(scope_name, CompilerScope::TypeParams, line_number);
19871993

1988-
// Propagate can_see_class_scope: if we're inside a class (class_name is set),
1989-
// this scope should be able to access the class namespace via __classdict__
1990-
if self.class_name.is_some() {
1994+
if in_class {
19911995
if let Some(table) = self.tables.last_mut() {
19921996
table.can_see_class_scope = true;
19931997
}

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

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// spell-checker:ignore iparam
1+
// spell-checker:ignore iparam gaiterobject
22
use std::sync::LazyLock;
33

44
use super::type_;
@@ -155,9 +155,10 @@ impl PyGenericAlias {
155155
let mut parts = Vec::with_capacity(len);
156156
// Use indexed access so list mutation during repr causes IndexError
157157
for i in 0..len {
158-
let item = list.borrow_vec().get(i).cloned().ok_or_else(|| {
159-
vm.new_index_error("list index out of range".to_owned())
160-
})?;
158+
let item =
159+
list.borrow_vec().get(i).cloned().ok_or_else(|| {
160+
vm.new_index_error("list index out of range".to_owned())
161+
})?;
161162
parts.push(repr_item(item, vm)?);
162163
}
163164
Ok(format!("[{}]", parts.join(", ")))
@@ -251,21 +252,15 @@ impl PyGenericAlias {
251252
}
252253
.into_pyobject(vm);
253254
Ok(PyTuple::new_ref(
254-
vec![
255-
next_fn,
256-
PyTuple::new_ref(vec![iter_obj], &vm.ctx).into(),
257-
],
255+
vec![next_fn, PyTuple::new_ref(vec![iter_obj], &vm.ctx).into()],
258256
&vm.ctx,
259257
))
260258
} else {
261259
Ok(PyTuple::new_ref(
262260
vec![
263261
vm.ctx.types.generic_alias_type.to_owned().into(),
264-
PyTuple::new_ref(
265-
vec![zelf.origin.clone(), zelf.args.clone().into()],
266-
&vm.ctx,
267-
)
268-
.into(),
262+
PyTuple::new_ref(vec![zelf.origin.clone(), zelf.args.clone().into()], &vm.ctx)
263+
.into(),
269264
],
270265
&vm.ctx,
271266
))
@@ -619,13 +614,13 @@ impl Comparable for PyGenericAlias {
619614
return Ok(PyComparisonValue::Implemented(false));
620615
}
621616
Ok(PyComparisonValue::Implemented(
622-
zelf.__origin__().rich_compare_bool(
623-
&other.__origin__(),
624-
PyComparisonOp::Eq,
625-
vm,
626-
)? && zelf
627-
.__args__()
628-
.rich_compare_bool(&other.__args__(), PyComparisonOp::Eq, vm)?,
617+
zelf.__origin__()
618+
.rich_compare_bool(&other.__origin__(), PyComparisonOp::Eq, vm)?
619+
&& zelf.__args__().rich_compare_bool(
620+
&other.__args__(),
621+
PyComparisonOp::Eq,
622+
vm,
623+
)?,
629624
))
630625
})
631626
}

‎crates/vm/src/stdlib/ast/pyast.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,13 @@ pub(crate) struct NodeExprConstant(NodeExpr);
544544
impl NodeExprConstant {
545545
#[extend_class]
546546
fn extend_class_with_fields(ctx: &Context, class: &'static Py<PyType>) {
547+
// AST types are mutable (heap types, not IMMUTABLETYPE)
548+
// Safety: called during type initialization before any concurrent access
549+
unsafe {
550+
let flags = &class.slots.flags as *const crate::types::PyTypeFlags
551+
as *mut crate::types::PyTypeFlags;
552+
(*flags).remove(crate::types::PyTypeFlags::IMMUTABLETYPE);
553+
}
547554
class.set_attr(
548555
identifier!(ctx, _fields),
549556
ctx.new_tuple(vec![

‎crates/vm/src/stdlib/ast/python.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@ pub(crate) mod _ast {
2525
impl NodeAst {
2626
#[extend_class]
2727
fn extend_class(ctx: &Context, class: &'static Py<PyType>) {
28+
// AST types are mutable (heap types, not IMMUTABLETYPE)
29+
// Safety: called during type initialization before any concurrent access
30+
unsafe {
31+
let flags = &class.slots.flags as *const crate::types::PyTypeFlags
32+
as *mut crate::types::PyTypeFlags;
33+
(*flags).remove(crate::types::PyTypeFlags::IMMUTABLETYPE);
34+
}
2835
let empty_tuple = ctx.empty_tuple.clone();
2936
class.set_str_attr("_fields", empty_tuple.clone(), ctx);
3037
class.set_str_attr("_attributes", empty_tuple.clone(), ctx);

‎crates/vm/src/stdlib/typing.rs‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// spell-checker:ignore typevarobject funcobj
1+
// spell-checker:ignore typevarobject funcobj typevartuples
22
use crate::{
33
Context, PyResult, VirtualMachine, builtins::pystr::AsPyStr, class::PyClassImpl,
44
function::IntoFuncArgs,
@@ -33,7 +33,7 @@ pub(crate) mod decl {
3333
builtins::{PyGenericAlias, PyStrRef, PyTuple, PyTupleRef, PyType, PyTypeRef, type_},
3434
function::FuncArgs,
3535
protocol::{PyMappingMethods, PyNumberMethods},
36-
types::{AsMapping, AsNumber, Constructor, Hashable, Iterable, Representable},
36+
types::{AsMapping, AsNumber, Constructor, Iterable, Representable},
3737
};
3838
use std::sync::LazyLock;
3939

@@ -95,7 +95,7 @@ pub(crate) mod decl {
9595
module: Option<PyObjectRef>,
9696
}
9797
#[pyclass(
98-
with(Constructor, Representable, Hashable, AsMapping, AsNumber, Iterable),
98+
with(Constructor, Representable, AsMapping, AsNumber, Iterable),
9999
flags(IMMUTABLETYPE)
100100
)]
101101
impl TypeAliasType {
@@ -191,21 +191,22 @@ pub(crate) mod decl {
191191
}
192192

193193
/// Returns the evaluator for the alias value.
194-
/// If compute_value is callable (lazy function from compiler), return it.
195-
/// Otherwise wrap the eagerly-set value in a ConstEvaluator-like callable.
194+
/// Lazy aliases (from compiler) have compute_value as a callable and no cached_value;
195+
/// eager aliases (from constructor) always have cached_value set.
196196
#[pygetset]
197197
fn evaluate_value(&self, vm: &VirtualMachine) -> PyResult {
198-
if self.compute_value.is_callable() {
199-
return Ok(self.compute_value.clone());
198+
let cached = self.cached_value.lock().clone();
199+
if cached.is_some() {
200+
// Eager path: wrap value in a ConstEvaluator
201+
let value = self.compute_value.clone();
202+
return Ok(vm
203+
.new_function("_ConstEvaluator", move |_args: FuncArgs| -> PyResult {
204+
Ok(value.clone())
205+
})
206+
.into());
200207
}
201-
// For eagerly-evaluated aliases (from constructor), create a callable
202-
// that returns the value regardless of format argument
203-
let value = self.compute_value.clone();
204-
Ok(vm
205-
.new_function("_ConstEvaluator", move |_args: FuncArgs| -> PyResult {
206-
Ok(value.clone())
207-
})
208-
.into())
208+
// Lazy path: return the compute function directly
209+
Ok(self.compute_value.clone())
209210
}
210211

211212
/// Check type_params ordering: non-default params must precede default params.
@@ -304,8 +305,11 @@ pub(crate) mod decl {
304305
})?
305306
};
306307

307-
let name = name.downcast::<crate::builtins::PyStr>().map_err(|_| {
308-
vm.new_type_error("typealias() argument 'name' must be str, not int".to_owned())
308+
let name = name.downcast::<crate::builtins::PyStr>().map_err(|obj| {
309+
vm.new_type_error(format!(
310+
"typealias() argument 'name' must be str, not {}",
311+
obj.class().name()
312+
))
309313
})?;
310314

311315
let type_params = if let Some(tp) = args.kwargs.get("type_params") {
@@ -335,12 +339,6 @@ pub(crate) mod decl {
335339
}
336340
}
337341

338-
impl Hashable for TypeAliasType {
339-
fn hash(zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<crate::common::hash::PyHash> {
340-
Ok(zelf.as_object().get_id() as crate::common::hash::PyHash)
341-
}
342-
}
343-
344342
impl AsMapping for TypeAliasType {
345343
fn as_mapping() -> &'static PyMappingMethods {
346344
static AS_MAPPING: LazyLock<PyMappingMethods> = LazyLock::new(|| PyMappingMethods {

0 commit comments

Comments
 (0)