Fix typing, typevar, genericalias, and symboltable #7091
Conversation
📝 WalkthroughWalkthroughThis PR introduces selective name mangling for type parameters in generic contexts, refactoring the symbol table to track which names require mangling and conditionally applying mangling during name resolution. It updates TypeVar/ParamSpec variance handling, adds TypeVarTuple unpacking support in generic aliases, and adjusts the NoDefault type's immutability flag. Changes
Sequence DiagramsequenceDiagram
participant Compiler as Compiler<br/>(compile.rs)
participant SymTable as Symbol Table<br/>(symboltable.rs)
participant Resolution as Name Resolution<br/>(maybe_mangle_name)
Compiler->>SymTable: enter_type_param_block(for_class: bool)
SymTable->>SymTable: Initialize mangled_names set
Compiler->>SymTable: register_name(name, usage)
alt TypeParam usage
SymTable->>SymTable: Add name to mangled_names
end
Compiler->>Compiler: Encounter name to mangle
Compiler->>Resolution: maybe_mangle_name(class_name, mangled_names, name)
alt name in mangled_names
Resolution->>Resolution: Apply mangle_name()
Resolution-->>Compiler: Return mangled name
else name not in set or set is None
Resolution-->>Compiler: Return original name
end
Compiler->>SymTable: Insert/lookup resolved name
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
force-pushed
the
typing
branch
2 times, most recently
from
07fbdd0 to
cf798e7
Compare
youknowone
changed the title
- TypeVar/ParamSpec repr: use infer_variance flag - ParamSpec: add type_check on bound argument - ParamSpecArgs/Kwargs: use equality instead of identity - NoDefault: change to IMMUTABLETYPE flag - subscript_generic: wrap TypeVarTuple in Unpack - symboltable: selective name mangling in type param scopes - symboltable: fix double default scanning for non-generic fns - Unmark 4 passing tests in test_type_params
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/concurrent dependencies:
dependent tests: (11 tests)
[x] lib: cpython/Lib/typing.py dependencies:
dependent tests: (14 tests)
Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/symboltable.rs (1)
1279-1322:⚠️ Potential issue | 🔴 CriticalBase class names are mangled incorrectly due to
class_namebeing set when scanning bases (regression).In the old code,
self.class_name = prev_classwas restored immediately afterself.leave_scope()and before scanning bases. The new code moved this restoration to after base scanning, leavingclass_nameset to the current class during base expression evaluation.This breaks name mangling for nested classes with private base names:
Non-generic class: Bases are scanned with
class_name = "Child"andmangled_names = None, so__Privategets mangled as_Child__Private. CPython mangles it as_Outer__Private(bases are evaluated in the enclosing scope).Generic class: Bases are scanned with
class_name = "Child"andmangled_names = Some({T}). Since__Privateis not in the set,maybe_mangle_namereturns it unmangled. CPython would mangle it as_Outer__Private.Root cause: The refactored code moved
self.class_name = prev_class(restoration of the enclosing class name) to line 1320, after bases are scanned (lines 1310–1315), whereas the old code restored it before scanning bases.Fix: Restore
self.class_nametoprev_class(the enclosing class name) immediately afterself.leave_scope()and before scanning base arguments.
🧹 Nitpick comments (2)
crates/vm/src/builtins/genericalias.rs (1)
737-766: Duplicated logic withunpack_typevartuplesintyping.rs.This TypeVarTuple unpacking logic (check
downcast_ref::<TypeVarTuple>, wrap withUnpack.__getitem__) is essentially identical tounpack_typevartuplesincrates/vm/src/stdlib/typing.rs(lines 469–490). Consider extracting a shared helper to avoid maintaining both copies.Additionally,
new_args(line 738) is allocated even whenneeds_unpackisfalseand theelsebranch returnsparamswithout using it. Move the allocation inside theif needs_unpackblock.Suggested refactor
- // Unpack TypeVarTuples: wrap each TypeVarTuple in Unpack[...] - let mut new_args = Vec::with_capacity(params.len()); - let mut needs_unpack = false; - for param in params.iter() { - if param - .downcast_ref::<crate::stdlib::typing::TypeVarTuple>() - .is_some() - { - needs_unpack = true; - break; - } - } - - let args = if needs_unpack { - let unpack_cls = typing_module.get_attr("Unpack", vm)?; - for param in params.iter() { - if param - .downcast_ref::<crate::stdlib::typing::TypeVarTuple>() - .is_some() - { - let unpacked = vm.call_method(&unpack_cls, "__getitem__", (param.clone(),))?; - new_args.push(unpacked); - } else { - new_args.push(param.clone()); - } - } - PyTuple::new_ref(new_args, &vm.ctx) - } else { - params - }; + // Unpack TypeVarTuples: wrap each TypeVarTuple in Unpack[...] + let has_tvt = params + .iter() + .any(|p| p.downcast_ref::<crate::stdlib::typing::TypeVarTuple>().is_some()); + + let args = if has_tvt { + let unpack_cls = typing_module.get_attr("Unpack", vm)?; + let new_args: Vec<PyObjectRef> = params + .iter() + .map(|p| { + if p.downcast_ref::<crate::stdlib::typing::TypeVarTuple>().is_some() { + vm.call_method(&unpack_cls, "__getitem__", (p.clone(),)) + } else { + Ok(p.clone()) + } + }) + .collect::<PyResult<_>>()?; + PyTuple::new_ref(new_args, &vm.ctx) + } else { + params + };This also aligns the style with the existing
unpack_typevartuplesincrates/vm/src/stdlib/typing.rs(line 479–488), which uses the iterator/map/collect pattern.crates/vm/src/stdlib/typevar.rs (1)
670-685: ParamSpec and TypeVar repr implementations are identical — consider extracting a shared helper.The variance-prefix formatting logic (lines 674–684) is a direct copy of TypeVar's
repr_str(lines 273–283). A small shared function likeformat_variance_prefix(name, infer_variance, covariant, contravariant)would eliminate the duplication.
1 participant
Summary by CodeRabbit