Implement more ast features - #6986
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughThis PR refactors the AST system to add comprehensive validation and representation helpers, changes how builtins are stored from PyDictRef to PyObjectRef throughout the frame and function system, expands documentation entries, and enhances string handling for f-strings and template strings with normalization and escape-sequence warnings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VM
participant Frame
participant Module
participant Builtins
Client->>VM: run_code_obj(code_obj, scope)
VM->>Module: resolve builtins from __builtins__
alt Module has PyModule __builtins__
Module->>Builtins: dict()
else Module has direct builtins
Module->>Builtins: use as-is
else No __builtins__
VM->>Builtins: use vm.builtins.dict()
end
VM->>Frame: new(builtins: PyObjectRef)
Frame->>Client: Frame ready (builtins: PyObjectRef)
Client->>Frame: load_global_or_builtin(name)
alt builtins is dict
Frame->>Builtins: fast path dict lookup
else builtins is non-dict
Frame->>Builtins: generic path getattr
end
Builtins->>Client: value or error
sequenceDiagram
participant Compiler
participant Parser
participant Validator
participant AST_Module
Compiler->>Parser: parse(source, mode, feature_version)
Parser->>Parser: apply target_version defaults
Parser->>AST_Module: emit AST nodes
alt mode == "single"
Validator->>AST_Module: wrap_interactive(Module → Interactive)
end
Validator->>Validator: validate_mod(module)
Validator->>Validator: check context consistency, body non-empty, patterns
alt validation fails
Validator->>Compiler: PyResult error
else validation succeeds
Validator->>Compiler: validated AST
end
Compiler->>Compiler: compile to code object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 1 | ❌ 2 ❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin ast |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/ast.py dependencies:
dependent tests: (48 tests)
[x] test: cpython/Lib/test/test_builtin.py (TODO: 25) dependencies: dependent tests: (no tests depend on builtin) [ ] test: cpython/Lib/test/test_funcattrs.py (TODO: 5) dependencies: dependent tests: (no tests depend on funcattrs) [ ] lib: cpython/Lib/traceback.py dependencies:
dependent tests: (32 tests)
Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agentsIn `@crates/vm/src/frame.rs`: - Around line 1209-1234: The LOAD_BUILD_CLASS handler currently assumes __builtins__ is a mapping and calls get_item, which causes an uncaught TypeError when __builtins__ is a module; change the else branch of the Instruction::LoadBuildClass handling (the branch after self.builtins.downcast_ref::<PyDict>()) to detect module-style builtins (downcast_ref::<PyModule>() or equivalent) and call get_attr(identifier!(vm, __build_class__), vm) for modules, translating AttributeError into a NameError the same way you handle KeyError for mappings; keep the existing get_item path for dict/mapping objects. Apply the same fix to the slow-path global/builtin name lookup routine (the name-lookup code referenced in the review) so it uses get_attr for module builtins and preserves existing error translation logic for mapping builtins. In `@crates/vm/src/stdlib/ast/string.rs`: - Around line 50-134: The merge logic currently keeps the first constant's TextRange when collapsing adjacent string literals, so location info becomes the first-span-only; update normalize_joined_str_parts and normalize_template_str_parts so that when you append a new constant (in the branches handling JoinedStrPart::Constant and TemplateStrPart::Constant) you also extend the pending range to cover both spans (e.g., set pending.2 = TextRange::new(pending.start(), constant.range.end()) or use the project's helper like TextRange::cover/join to set the pending range to span from the original pending start to constant.range.end), and ensure push_joined_str_literal and push_template_str_literal use that extended range when building Constant::new_str; apply the identical change for both joined and template paths so the merged literal range extends to the last fragment. In `@crates/vm/src/stdlib/ast/validate.rs`: - Around line 349-354: The NamedExpr handler only checks that named.target is an ast::Expr::Name but doesn't validate it in Store context; after the existing type check for ast::Expr::Named (and the matches!(&*named.target, ast::Expr::Name(_)) check), call validate_expr(vm, &named.target, ast::ExprContext::Store) to validate the target as an assignment target and propagate any error, then validate the value with validate_expr(vm, &named.value, ast::ExprContext::Load) as already done; keep the existing type error message if the target isn't a Name.
crates/vm/src/stdlib/ast.rs (2)crates/vm/src/stdlib/ast/python.rs (1)398-470: Consider handling edge cases in func_type parsing.
The parse_func_type function has good structure but a few observations:
💡 Optional: Add validation for unexpected expression types
Lines 405-406: optimize and target_version are unused (explicitly ignored with let _ =). This is intentional but worth noting.
Line 437: When extracting right, the offset split_at + 2 assumes -> is always 2 bytes, which is correct for ASCII but the comment/documentation should clarify this.
Lines 457-461: The match arm for other expression types (other => vec![other]) is a catch-all that may accept invalid func_type syntax silently.
let argtypes: Vec<ast::Expr> = match arg_expr { ast::Expr::Tuple(tup) => tup.elts, ast::Expr::Name(_) | ast::Expr::Subscript(_) | ast::Expr::Attribute(_) => vec![arg_expr], - other => vec![other], + // CPython accepts any expression as a type annotation + other => vec![other], };
472-503: Minor: redundant assignment on Line 487.
The line let tag = if tag.is_empty() { "" } else { tag }; is a no-op since tag already equals itself in both branches.
♻️ Simplify redundant conditionallet tag = rest.trim_start(); - let tag = if tag.is_empty() { "" } else { tag }; let node = NodeAst433-513: Redundant setup in module_exec.
The module_exec function (lines 440-477) repeats the same setup that extend_class already performs in NodeAst. This appears to be setting up the AST class that's registered in the module, which may be a different type object than the one defined via #[pyattr].
If this duplication is intentional (to handle both the pyattr-registered type and the module-registered type), consider adding a comment explaining why.
💡 Add clarifying commentpub(crate) fn module_exec( vm: &VirtualMachine, module: &Py<crate::builtins::PyModule>, ) -> PyResult<()> { __module_exec(vm, module); super::super::pyast::extend_module_nodes(vm, module); + // The AST class registered in extend_module_nodes needs its attributes + // and methods configured separately from the #[pyattr] NodeAst type. let ast_type = module .get_attr("AST", vm)?
Sorry, something went wrong.
| Instruction::LoadBuildClass => { | ||
| self.push_value(vm.builtins.get_attr(identifier!(vm, __build_class__), vm)?); | ||
| let build_class = | ||
| if let Some(builtins_dict) = self.builtins.downcast_ref::<PyDict>() { | ||
| builtins_dict | ||
| .get_item_opt(identifier!(vm, __build_class__), vm)? | ||
| .ok_or_else(|| { | ||
| vm.new_name_error( | ||
| "__build_class__ not found".to_owned(), | ||
| identifier!(vm, __build_class__).to_owned(), | ||
| ) | ||
| })? | ||
| } else { | ||
| self.builtins | ||
| .get_item(identifier!(vm, __build_class__), vm) | ||
| .map_err(|e| { | ||
| if e.fast_isinstance(vm.ctx.exceptions.key_error) { | ||
| vm.new_name_error( | ||
| "__build_class__ not found".to_owned(), | ||
| identifier!(vm, __build_class__).to_owned(), | ||
| ) | ||
| } else { | ||
| e | ||
| } | ||
| })? | ||
| }; | ||
| self.push_value(build_class); |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🏁 Script executed:
cd crates/vm && wc -l src/frame.rsRepository: RustPython/RustPython
Length of output: 82
🏁 Script executed:
cd crates/vm && sed -n '1209,1234p' src/frame.rsRepository: RustPython/RustPython
Length of output: 1413
🏁 Script executed:
cd crates/vm && sed -n '2151,2169p' src/frame.rsRepository: RustPython/RustPython
Length of output: 927
🏁 Script executed:
cd crates/vm && rg "get_attr" --type rust -B2 -A2 | head -50Repository: RustPython/RustPython
Length of output: 2773
🏁 Script executed:
cd crates/vm && rg "module_type" --type rust | head -20Repository: RustPython/RustPython
Length of output: 680
🏁 Script executed:
cd crates/vm && rg "\.class\(\)" --type rust -B1 -A1 | head -30Repository: RustPython/RustPython
Length of output: 1642
🏁 Script executed:
cd crates/vm && rg "builtins.*module" --type rust -B2 -A2 | head -40Repository: RustPython/RustPython
Length of output: 2296
🏁 Script executed:
cd crates/vm && rg "get_item" src/frame.rs -B3 -A3 | head -60Repository: RustPython/RustPython
Length of output: 2874
🏁 Script executed:
cd crates/vm && grep -n "attribute_error\|key_error" src/frame.rs | head -20Repository: RustPython/RustPython
Length of output: 1161
🏁 Script executed:
cd crates && find . -name "*.rs" -type f -exec grep -l "PyModule" {} \; | grep -E "builtins|module" | head -5Repository: RustPython/RustPython
Length of output: 205
🏁 Script executed:
cd crates/vm && sed -n '1,100p' src/builtins/module.rsRepository: RustPython/RustPython
Length of output: 3283
🏁 Script executed:
cd crates/vm && rg "__getitem__" src/builtins/module.rs -B3 -A5Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates/vm && rg "get_item\|get_attr" src/builtins/module.rs -B2 -A2 | head -40Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates/vm && sed -n '100,300p' src/builtins/module.rsRepository: RustPython/RustPython
Length of output: 7033
🏁 Script executed:
cd crates/vm && rg "GetAttr\|Initializer" src/builtins/module.rs -B2 -A8 | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates/vm && python3 << 'EOF'
import sys
# In Python, modules don't support __getitem__
# They support __getattr__ via attribute access
# Check the behavior:
mod = sys
try:
mod["path"] # This will fail
except TypeError as e:
print(f"__getitem__ on module raises: {type(e).__name__}: {e}")
try:
getattr(mod, "path") # This works
print("__getattr__ on module works")
except AttributeError as e:
print(f"__getattr__ raises: {e}")
try:
getattr(mod, "nonexistent_attr")
except AttributeError as e:
print(f"__getattr__ for missing attr raises AttributeError: {type(e).__name__}")
EOFRepository: RustPython/RustPython
Length of output: 237
🏁 Script executed:
cd crates/vm && rg "fn get_item" src/builtins/*.rs -A5 | head -60Repository: RustPython/RustPython
Length of output: 1137
🏁 Script executed:
cd crates/vm && rg "object.get_item\|obj.get_item" src/ --type rust -B2 -A3 | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates && find . -name "*.rs" -type f -exec grep -l "fn get_item" {} \; | head -10Repository: RustPython/RustPython
Length of output: 205
🏁 Script executed:
cd crates/vm && sed -n '1,100p' src/protocol/object.rsRepository: RustPython/RustPython
Length of output: 3552
🏁 Script executed:
cd crates/vm && rg "pub fn get_item" src/protocol/object.rs -A10 | head -40Repository: RustPython/RustPython
Length of output: 530
🏁 Script executed:
cd crates/vm && grep -n "get_item" src/protocol/object.rs | head -20Repository: RustPython/RustPython
Length of output: 294
🏁 Script executed:
cd crates/vm && sed -n '686,720p' src/protocol/object.rsRepository: RustPython/RustPython
Length of output: 1504
🏁 Script executed:
cd crates/vm && rg "try_mapping\|try_sequence" src/protocol/object.rs -B2 -A8 | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates/vm && rg "\.class\(\)\.is\(vm\.ctx\.types\." src/ --type rust -B1 -A1 | head -40Repository: RustPython/RustPython
Length of output: 2395
🏁 Script executed:
cd crates/vm && rg "get_attr.*vm\)" src/frame.rs -B2 -A2 | head -30Repository: RustPython/RustPython
Length of output: 1579
🏁 Script executed:
cd crates/vm && cargo clippy --lib 2>&1 | grep -A5 "frame.rs" | head -30Repository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
cd crates/vm && cargo fmt --check -- src/frame.rs 2>&1 | head -20Repository: RustPython/RustPython
Length of output: 604
Handle module-style __builtins__ lookup to avoid TypeError.
When __builtins__ is a module (the common case), calling get_item raises TypeError, which is not caught by the code expecting KeyError. This breaks LOAD_BUILD_CLASS and global/builtin name resolution.
For modules, use get_attr instead (which raises AttributeError on missing attributes, translatable to NameError). Keep get_item for dict/mapping objects.
This applies to both the LOAD_BUILD_CLASS instruction (lines 1209-1234) and the slow-path name lookup (lines 2151-2169).
Suggested fix@@
- } else {
- self.builtins
- .get_item(identifier!(vm, __build_class__), vm)
- .map_err(|e| {
- if e.fast_isinstance(vm.ctx.exceptions.key_error) {
- vm.new_name_error(
- "__build_class__ not found".to_owned(),
- identifier!(vm, __build_class__).to_owned(),
- )
- } else {
- e
- }
- })?
- };
+ } else if self.builtins.class().is(vm.ctx.types.module_type) {
+ self.builtins
+ .get_attr(identifier!(vm, __build_class__), vm)
+ .map_err(|e| {
+ if e.fast_isinstance(vm.ctx.exceptions.attribute_error) {
+ vm.new_name_error(
+ "__build_class__ not found".to_owned(),
+ identifier!(vm, __build_class__).to_owned(),
+ )
+ } else {
+ e
+ }
+ })?
+ } else {
+ self.builtins
+ .get_item(identifier!(vm, __build_class__), vm)
+ .map_err(|e| {
+ if e.fast_isinstance(vm.ctx.exceptions.key_error) {
+ vm.new_name_error(
+ "__build_class__ not found".to_owned(),
+ identifier!(vm, __build_class__).to_owned(),
+ )
+ } else {
+ e
+ }
+ })?
+ };
@@
- } else {
- // Slow path: builtins is not a dict, use generic __getitem__
- if let Some(value) = self.globals.get_item_opt(name, vm)? {
- return Ok(value);
- }
- self.builtins.get_item(name, vm).map_err(|e| {
- if e.fast_isinstance(vm.ctx.exceptions.key_error) {
- vm.new_name_error(format!("name '{name}' is not defined"), name.to_owned())
- } else {
- e
- }
- })
- }
+ } else {
+ // Slow path: builtins is not a dict
+ if let Some(value) = self.globals.get_item_opt(name, vm)? {
+ return Ok(value);
+ }
+ if self.builtins.class().is(vm.ctx.types.module_type) {
+ self.builtins.get_attr(name, vm).map_err(|e| {
+ if e.fast_isinstance(vm.ctx.exceptions.attribute_error) {
+ vm.new_name_error(format!("name '{name}' is not defined"), name.to_owned())
+ } else {
+ e
+ }
+ })
+ } else {
+ self.builtins.get_item(name, vm).map_err(|e| {
+ if e.fast_isinstance(vm.ctx.exceptions.key_error) {
+ vm.new_name_error(format!("name '{name}' is not defined"), name.to_owned())
+ } else {
+ e
+ }
+ })
+ }
+ }In `@crates/vm/src/frame.rs` around lines 1209 - 1234, The LOAD_BUILD_CLASS handler currently assumes __builtins__ is a mapping and calls get_item, which causes an uncaught TypeError when __builtins__ is a module; change the else branch of the Instruction::LoadBuildClass handling (the branch after self.builtins.downcast_ref::<PyDict>()) to detect module-style builtins (downcast_ref::<PyModule>() or equivalent) and call get_attr(identifier!(vm, __build_class__), vm) for modules, translating AttributeError into a NameError the same way you handle KeyError for mappings; keep the existing get_item path for dict/mapping objects. Apply the same fix to the slow-path global/builtin name lookup routine (the name-lookup code referenced in the review) so it uses get_attr for module builtins and preserves existing error translation logic for mapping builtins.
Sorry, something went wrong.
| fn push_joined_str_literal( | ||
| output: &mut Vec<JoinedStrPart>, | ||
| pending: &mut Option<(String, StringLiteralPrefix, TextRange)>, | ||
| ) { | ||
| if let Some((value, prefix, range)) = pending.take() | ||
| && !value.is_empty() | ||
| { | ||
| output.push(JoinedStrPart::Constant(Constant::new_str( | ||
| value, prefix, range, | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| fn normalize_joined_str_parts(values: Vec<JoinedStrPart>) -> Vec<JoinedStrPart> { | ||
| let mut output = Vec::with_capacity(values.len()); | ||
| let mut pending: Option<(String, StringLiteralPrefix, TextRange)> = None; | ||
|
|
||
| for part in values { | ||
| match part { | ||
| JoinedStrPart::Constant(constant) => { | ||
| let ConstantLiteral::Str { value, prefix } = constant.value else { | ||
| push_joined_str_literal(&mut output, &mut pending); | ||
| output.push(JoinedStrPart::Constant(constant)); | ||
| continue; | ||
| }; | ||
| let value: String = value.into(); | ||
| if let Some((pending_value, _, _)) = pending.as_mut() { | ||
| pending_value.push_str(&value); | ||
| } else { | ||
| pending = Some((value, prefix, constant.range)); | ||
| } | ||
| } | ||
| JoinedStrPart::FormattedValue(value) => { | ||
| push_joined_str_literal(&mut output, &mut pending); | ||
| output.push(JoinedStrPart::FormattedValue(value)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| push_joined_str_literal(&mut output, &mut pending); | ||
| output | ||
| } | ||
|
|
||
| fn push_template_str_literal( | ||
| output: &mut Vec<TemplateStrPart>, | ||
| pending: &mut Option<(String, StringLiteralPrefix, TextRange)>, | ||
| ) { | ||
| if let Some((value, prefix, range)) = pending.take() | ||
| && !value.is_empty() | ||
| { | ||
| output.push(TemplateStrPart::Constant(Constant::new_str( | ||
| value, prefix, range, | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| fn normalize_template_str_parts(values: Vec<TemplateStrPart>) -> Vec<TemplateStrPart> { | ||
| let mut output = Vec::with_capacity(values.len()); | ||
| let mut pending: Option<(String, StringLiteralPrefix, TextRange)> = None; | ||
|
|
||
| for part in values { | ||
| match part { | ||
| TemplateStrPart::Constant(constant) => { | ||
| let ConstantLiteral::Str { value, prefix } = constant.value else { | ||
| push_template_str_literal(&mut output, &mut pending); | ||
| output.push(TemplateStrPart::Constant(constant)); | ||
| continue; | ||
| }; | ||
| let value: String = value.into(); | ||
| if let Some((pending_value, _, _)) = pending.as_mut() { | ||
| pending_value.push_str(&value); | ||
| } else { | ||
| pending = Some((value, prefix, constant.range)); | ||
| } | ||
| } | ||
| TemplateStrPart::Interpolation(value) => { | ||
| push_template_str_literal(&mut output, &mut pending); | ||
| output.push(TemplateStrPart::Interpolation(value)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| push_template_str_literal(&mut output, &mut pending); | ||
| output | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Merged literal ranges should extend to the last fragment.
When adjacent constants are collapsed, the resulting range keeps only the first span; this can skew location info for AST consumers. Consider extending the range while merging.
- if let Some((pending_value, _, _)) = pending.as_mut() {
- pending_value.push_str(&value);
- } else {
- pending = Some((value, prefix, constant.range));
- }
+ if let Some((pending_value, _, pending_range)) = pending.as_mut() {
+ pending_value.push_str(&value);
+ *pending_range = TextRange::new(pending_range.start(), constant.range.end());
+ } else {
+ pending = Some((value, prefix, constant.range));
+ }
@@
- if let Some((pending_value, _, _)) = pending.as_mut() {
- pending_value.push_str(&value);
- } else {
- pending = Some((value, prefix, constant.range));
- }
+ if let Some((pending_value, _, pending_range)) = pending.as_mut() {
+ pending_value.push_str(&value);
+ *pending_range = TextRange::new(pending_range.start(), constant.range.end());
+ } else {
+ pending = Some((value, prefix, constant.range));
+ }In `@crates/vm/src/stdlib/ast/string.rs` around lines 50 - 134, The merge logic currently keeps the first constant's TextRange when collapsing adjacent string literals, so location info becomes the first-span-only; update normalize_joined_str_parts and normalize_template_str_parts so that when you append a new constant (in the branches handling JoinedStrPart::Constant and TemplateStrPart::Constant) you also extend the pending range to cover both spans (e.g., set pending.2 = TextRange::new(pending.start(), constant.range.end()) or use the project's helper like TextRange::cover/join to set the pending range to span from the original pending start to constant.range.end), and ensure push_joined_str_literal and push_template_str_literal use that extended range when building Constant::new_str; apply the identical change for both joined and template paths so the merged literal range extends to the last fragment.
Sorry, something went wrong.
| ast::Expr::Named(named) => { | ||
| if !matches!(&*named.target, ast::Expr::Name(_)) { | ||
| return Err(vm.new_type_error("NamedExpr target must be a Name".to_owned())); | ||
| } | ||
| validate_expr(vm, &named.value, ast::ExprContext::Load) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🌐 Web query:
Python AST NamedExpr target Store context validation requirement
💡 Result:
In CPython’s ast, a NamedExpr (the walrus operator) is treated like an assignment target:
- NamedExpr.target must be an ast.Name node (not Attribute, Subscript, etc.). The compiler’s AST validator explicitly errors otherwise: “NamedExpr target must be a Name”. [3]
- That ast.Name must be in Store context (because it’s being assigned to). This is what the parser produces, and what you must build if you construct the AST yourself: Name(id='x', ctx=Store()). [1]
- More generally, assignment targets use Store context (vs Load for reading a value, Del for del). [2]
So, when creating a walrus AST manually, do:
ast.NamedExpr(
target=ast.Name(id="x", ctx=ast.Store()),
value=...
)Validate NamedExpr targets in Store context.
Python's AST validator requires that NamedExpr.target (the walrus operator) be a Name node in Store context, since it represents an assignment target. The current check only enforces the target is a Name, but does not validate the context. Add context validation to reject invalid ASTs:
Proposed fix ast::Expr::Named(named) => {
if !matches!(&*named.target, ast::Expr::Name(_)) {
return Err(vm.new_type_error("NamedExpr target must be a Name".to_owned()));
}
+ validate_expr(vm, &named.target, ast::ExprContext::Store)?;
validate_expr(vm, &named.value, ast::ExprContext::Load)
}In `@crates/vm/src/stdlib/ast/validate.rs` around lines 349 - 354, The NamedExpr handler only checks that named.target is an ast::Expr::Name but doesn't validate it in Store context; after the existing type check for ast::Expr::Named (and the matches!(&*named.target, ast::Expr::Name(_)) check), call validate_expr(vm, &named.target, ast::ExprContext::Store) to validate the target as an assignment target and propagate any error, then validate the value with validate_expr(vm, &named.value, ast::ExprContext::Load) as already done; keep the existing type error message if the target isn't a Name.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation