Update _ast_unparse and fix unparse, type_comments#6961
Update _ast_unparse and fix unparse, type_comments#6961youknowone merged 8 commits intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughConvert internal panics/asserts into explicit syntax errors in codegen; guard truncated string-literal parsing; add t-string unparse support; restrict type subscripting to non-None Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuiltinsCompile as builtins::compile
participant Parser
participant ASTNode as ParsedAST
Caller->>BuiltinsCompile: compile(source, filename, mode, flags)
BuiltinsCompile->>BuiltinsCompile: extract PyCF_ONLY_AST from flags
BuiltinsCompile->>Parser: parse(source) -> ParsedAST
alt PyCF_ONLY_AST set
BuiltinsCompile->>BuiltinsCompile: mode_type_and_name(mode) -> expected_type
BuiltinsCompile->>ASTNode: inspect node type
alt node.type == expected_type
BuiltinsCompile-->>Caller: return AST node (short-circuit)
else mismatch
BuiltinsCompile-->>Caller: raise TypeError (expected vs actual)
end
else normal compile
BuiltinsCompile->>BuiltinsCompile: continue compile pipeline
BuiltinsCompile-->>Caller: return compiled result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin unparse |
6eea5dc to
017ae40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/ast/type_parameters.rs (1)
116-120:⚠️ Potential issue | 🟡 MinorChange
default_valuedeserialization from required to optional for TypeVar, ParamSpec, and TypeVarTuple.In Python 3.13+, the
default_valuefield ofTypeVar,ParamSpec, andTypeVarTupleAST nodes is optional and can beNone. Currently, the code usesget_node_field()which requires the field to be present, causing errors whendefault_valueis absent from an input AST object.Apply the same pattern used for the
boundfield to all three locations:
- Line 116-120 (TypeVar)
- Line 163-167 (ParamSpec)
- Line 213-217 (TypeVarTuple)
Replace
get_node_field(_vm, &_object, "default_value", "TypeVar")?with:+ default: get_node_field_opt(_vm, &_object, "default_value")? + .map(|obj| Node::ast_from_object(_vm, source_file, obj)) + .transpose()?,
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/builtins.rs`:
- Around line 144-166: The AST-only branch currently returns early when
ast::PY_COMPILE_FLAG_AST_ONLY is set without validating the rest of the flags;
update the block around flags/args.flags and ast::PY_COMPILE_FLAG_AST_ONLY to
run the same unrecognized-flag validation used in the parser branch (i.e.,
detect and reject any unsupported flag bits and raise vm.new_value_error for
invalid flags) before checking mode_str/expected_type and returning
Ok(args.source); reference the existing symbols flags, args.flags,
ast::PY_COMPILE_FLAG_AST_ONLY, mode_str and the early return to locate and
insert this validation.
🧹 Nitpick comments (1)
crates/vm/src/protocol/object.rs (1)
710-713: Useexpect()for consistency with codebase conventions.The downcast is safe here since the outer condition at line 699 guarantees
selfis a type object. However, elsewhere in this file (lines 525 and 618-619),expect()with an explanatory message is used when an invariant is guaranteed by prior checks.Suggested change
return Err(vm.new_type_error(format!( "type '{}' is not subscriptable", - self.downcast_ref::<PyType>().unwrap().name() + self.downcast_ref::<PyType>().expect("self is verified to be a type").name() )));
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/ast/type_parameters.rs (1)
116-120:⚠️ Potential issue | 🟠 MajorUse
get_node_field_optwith.map().transpose()pattern for optionaldefault_valuefield.The deserialization treats
default_valueas required usingget_node_field, but Python 3.13's AST specification definesdefault_valueas optional (can beNonefor type parameters without defaults). This causes aTypeErrorwhen constructing AST nodes without explicitly providingdefault_value.The pattern for optional fields is already correctly implemented for
bound(lines 113-115) usingget_node_field_opt. Apply the same pattern todefault_valueinTypeParamTypeVar(lines 116-120),TypeParamParamSpec(lines 163-167), andTypeParamTypeVarTuple(lines 213-217):Example fix for TypeParamTypeVar
-default: Node::ast_from_object( - _vm, - source_file, - get_node_field(_vm, &_object, "default_value", "TypeVar")?, -)?, +default: get_node_field_opt(_vm, &_object, "default_value")? + .map(|obj| Node::ast_from_object(_vm, source_file, obj)) + .transpose()?,
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/ast/python.rs`:
- Around line 99-137: The defaulting logic uses a global LIST_FIELDS to decide
list-vs-none which causes Lambda.args to be set to an empty list; change the
loop that assigns defaults (the block referencing LIST_FIELDS, fields, and
zelf.set_attr) to consult the node-specific _field_types metadata instead of the
global LIST_FIELDS: for each field in &fields, look up its declared type in
self._field_types (or equivalent per-node field-type map) and only create
vm.ctx.new_list(vec![]) when that field’s declared type is a sequence/list type;
otherwise use vm.ctx.none() so fields like Lambda.args (declared as an arguments
node) are not defaulted to an empty list. Ensure you still use
vm.ctx.intern_str(field.as_str()) and set the attribute via
zelf.set_attr(vm.ctx.intern_str(...), default, vm)?.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/builtins.rs`:
- Around line 148-164: The code currently compares the source AST class name
string to expected_type (using cls_name != expected_type), which rejects
subclasses; change this to map mode_str to the actual AST type class object
(e.g., the Module/Expression/Interactive/FunctionType class from the VM/ctx
types) and use fast_issubclass(args.source.class(), expected_type_class) to
validate inheritance; if fast_issubclass returns false, return the same
vm.new_type_error with a message like "expected {expected_type} node, got
{cls_name}" so subclasses are accepted while still failing on unrelated types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/ast/python.rs`:
- Around line 90-149: The body-field list detection in
crates/vm/src/stdlib/ast/python.rs incorrectly treats IfExp.body as a list;
update the conditional that computes is_list_field (which uses class_name,
field_name, and LIST_FIELDS) so the "body" branch also excludes "IfExp" (i.e.,
ensure class_name != "Lambda" && class_name != "Expression" && class_name !=
"IfExp") before choosing the default and calling zelf.set_attr.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/ast/python.rs`:
- Around line 129-154: The code incorrectly uses exact class name string
comparisons (class_name == "Call", "Lambda", "Expression", "IfExp") to decide
list defaults, which misclassifies subclasses; change those comparisons to
subclass checks using vm.ctx.fast_issubclass against the canonical AST base
classes so subclasses inherit the same defaults. Specifically, in the
is_list_field logic replace checks like class_name == "Call" or class_name ==
"arguments" and the matches!(...) for "Lambda"|"Expression"|"IfExp" with calls
to vm.ctx.fast_issubclass(zelf.class(), <Call_class_obj>) etc.; obtain the base
class objects once (e.g., look up the Call, Lambda, Expression, IfExp class
objects from the VM context) and use those in fast_issubclass checks, keeping
LIST_FIELDS and the rest of the default/list logic intact and falling back to
the string-based behavior only if the type lookup fails.
🧹 Nitpick comments (2)
crates/vm/src/protocol/object.rs (1)
708-715: Logic looks good; preferexpect()overunwrap()for clarity.The change correctly handles the case where
__class_getitem__is explicitly set toNone, producing a clearer error message than the previous "NoneType is not callable".However, the
unwrap()on line 714 doesn't communicate the safety invariant to readers. While it's safe here (we're inside thefast_issubclass(type_type)check, soselfmust be aPyType), usingexpect()would document this assumption and provide a better panic message if the invariant is ever violated.📝 Suggested improvement for clarity
return Err(vm.new_type_error(format!( "type '{}' is not subscriptable", - self.downcast_ref::<PyType>().unwrap().name() + self.downcast_ref::<PyType>() + .expect("self is a type object (checked by fast_issubclass)") + .name() )));crates/codegen/src/unparse.rs (1)
630-641: Optional: consolidate f/t-string rendering to avoid drift.
unparse_tstringmirrorsunparse_fstringclosely; consider extracting a small shared helper (prefix + body builder) to keep escaping behavior consistent as string kinds evolve. Also please confirmcargo fmtandcargo clippyrun clean for this change.As per coding guidelines, follow the default rustfmt code style by running
cargo fmtto format Rust code; always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes.
| let class_name = zelf.class().name().to_string(); | ||
|
|
||
| for field in &fields { | ||
| if !set_fields.contains(field.as_str()) { | ||
| let field_name = field.as_str(); | ||
| // Some field names have different ASDL types depending on the node. | ||
| // For example, "args" is `expr*` in Call but `arguments` in Lambda. | ||
| // "body" and "orelse" are `stmt*` in most nodes but `expr` in IfExp. | ||
| let is_list_field = if field_name == "args" { | ||
| class_name == "Call" || class_name == "arguments" | ||
| } else if field_name == "body" || field_name == "orelse" { | ||
| !matches!( | ||
| class_name.as_str(), | ||
| "Lambda" | "Expression" | "IfExp" | ||
| ) | ||
| } else { | ||
| LIST_FIELDS.contains(&field_name) | ||
| }; | ||
|
|
||
| let default: PyObjectRef = if is_list_field { | ||
| vm.ctx.new_list(vec![]).into() | ||
| } else { | ||
| vm.ctx.none() | ||
| }; | ||
| zelf.set_attr(vm.ctx.intern_str(field_name), default, vm)?; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/vm && head -200 src/stdlib/ast/python.rs | tail -100Repository: RustPython/RustPython
Length of output: 3868
🏁 Script executed:
sed -n '100,160p' crates/vm/src/stdlib/ast/python.rsRepository: RustPython/RustPython
Length of output: 2601
🏁 Script executed:
# Search for fast_issubclass usage in the codebase to understand the pattern
rg "fast_issubclass" crates/vm/src/stdlib/ast/ -A 2 -B 2Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look for NodeExprCall, NodeArguments, NodeExprLambda types in the codebase
rg "NodeExprCall|NodeArguments|NodeExprLambda|NodeModExpression|NodeExprIfExp" crates/vm/src/stdlib/ast/Repository: RustPython/RustPython
Length of output: 1907
🏁 Script executed:
# Check what node types are available in the pyast module
fd "pyast" crates/vm/src/stdlib/ast/ && ls -la crates/vm/src/stdlib/ast/pyast* 2>/dev/null || find crates/vm/src/stdlib/ast -name "*pyast*" -type fRepository: RustPython/RustPython
Length of output: 182
🏁 Script executed:
rg "fast_issubclass" crates/vm --type rustRepository: RustPython/RustPython
Length of output: 6337
🏁 Script executed:
# Search for issubclass patterns in the codebase
rg "\.is\(.*::static_type" crates/vm/src/stdlib/ast/ -A 1 -B 1Repository: RustPython/RustPython
Length of output: 25507
🏁 Script executed:
# Check what methods are available on PyClass for subclass checking
rg "impl.*PyClass" crates/vm/src/builtins/ -A 10 | head -50Repository: RustPython/RustPython
Length of output: 3469
🏁 Script executed:
# Look for subclass checking methods in the PyClass implementation
fd "class.rs" crates/vm/src && head -100 "$(fd 'class.rs' crates/vm/src)"Repository: RustPython/RustPython
Length of output: 3451
🏁 Script executed:
# Search for how .is() method is used on PyClass
rg "\.is\(" crates/vm/src/stdlib/ast/python.rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 47
Use subclass checking instead of exact class name matching for field defaults.
When _field_types is inherited by subclasses, string comparison like class_name == "Call" misclassifies subclasses and defaults fields incorrectly. Use fast_issubclass against the base node types so subclasses follow the same defaults as their parents.
🛠️ Suggested fix
- let class_name = zelf.class().name().to_string();
+ let class = zelf.class();
+ let class_name = class.name().to_string();
...
- let is_list_field = if field_name == "args" {
- class_name == "Call" || class_name == "arguments"
- } else if field_name == "body" || field_name == "orelse" {
- !matches!(
- class_name.as_str(),
- "Lambda" | "Expression" | "IfExp"
- )
- } else {
- LIST_FIELDS.contains(&field_name)
- };
+ let is_list_field = if field_name == "args" {
+ class.fast_issubclass(super::pyast::NodeExprCall::static_type())
+ || class.fast_issubclass(super::pyast::NodeArguments::static_type())
+ } else if field_name == "body" || field_name == "orelse" {
+ !(class.fast_issubclass(super::pyast::NodeExprLambda::static_type())
+ || class.fast_issubclass(super::pyast::NodeModExpression::static_type())
+ || class.fast_issubclass(super::pyast::NodeExprIfExp::static_type()))
+ } else {
+ LIST_FIELDS.contains(&field_name)
+ };🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/ast/python.rs` around lines 129 - 154, The code
incorrectly uses exact class name string comparisons (class_name == "Call",
"Lambda", "Expression", "IfExp") to decide list defaults, which misclassifies
subclasses; change those comparisons to subclass checks using
vm.ctx.fast_issubclass against the canonical AST base classes so subclasses
inherit the same defaults. Specifically, in the is_list_field logic replace
checks like class_name == "Call" or class_name == "arguments" and the
matches!(...) for "Lambda"|"Expression"|"IfExp" with calls to
vm.ctx.fast_issubclass(zelf.class(), <Call_class_obj>) etc.; obtain the base
class objects once (e.g., look up the Call, Lambda, Expression, IfExp class
objects from the VM context) and use those in fast_issubclass checks, keeping
LIST_FIELDS and the rest of the default/list logic intact and falling back to
the string-based behavior only if the type lookup fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@DEVELOPMENT.md`:
- Around line 67-69: The fenced code block contains a shell prompt character ("$
") before the command string "cargo test --workspace --exclude rustpython_wasm
--exclude rustpython-venvlauncher" which triggers markdownlint MD014; edit the
fenced block to remove the leading "$ " so it contains just the plain command
line (i.e., "cargo test --workspace --exclude rustpython_wasm --exclude
rustpython-venvlauncher") to satisfy the lint rule.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/ast/python.rs (1)
154-158: Minor:ImportFrom.levelgets set twice.When
levelis not provided, the loop at lines 131-152 first sets it toNone, then lines 155-157 immediately overwrite it to0. This results in twoset_attrcalls for the same field.Consider handling this inside the loop to avoid the redundant attribute assignment:
♻️ Suggested consolidation
let default: PyObjectRef = if is_list_field { vm.ctx.new_list(vec![]).into() + } else if field_name == "level" && class_name == "ImportFrom" { + vm.ctx.new_int(0).into() } else { vm.ctx.none() }; zelf.set_attr(vm.ctx.intern_str(field_name), default, vm)?; } } - - // Special defaults that are not None or empty list - if class_name == "ImportFrom" && !set_fields.contains("level") { - zelf.set_attr("level", vm.ctx.new_int(0), vm)?; - }
| ```shell | ||
| $ cargo test --workspace --exclude rustpython_wasm | ||
| $ cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher | ||
| ``` |
There was a problem hiding this comment.
Remove the shell prompt to satisfy markdownlint MD014.
The $ prompt in a fenced block without output triggers MD014. Consider removing it.
Suggested edit
-$ cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher
+cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 68-68: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
In `@DEVELOPMENT.md` around lines 67 - 69, The fenced code block contains a shell
prompt character ("$ ") before the command string "cargo test --workspace
--exclude rustpython_wasm --exclude rustpython-venvlauncher" which triggers
markdownlint MD014; edit the fenced block to remove the leading "$ " so it
contains just the plain command line (i.e., "cargo test --workspace --exclude
rustpython_wasm --exclude rustpython-venvlauncher") to satisfy the lint rule.
- Extract empty_arguments_object helper from expression.rs - Fix LIST_FIELDS ambiguity: "args" and "body" have different ASDL types per node (e.g. Lambda.args is `arguments`, not `expr*`; Lambda.body is `expr`, not `stmt*`) - Replace class name string comparison in compile() with fast_isinstance to accept AST subclasses
📦 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_exceptions.py (TODO: 31) dependencies: dependent tests: (no tests depend on exception) [x] lib: cpython/Lib/typing.py dependencies:
dependent tests: (9 tests)
[ ] test: cpython/Lib/test/test_class.py (TODO: 16) dependencies: dependent tests: (no tests depend on class) Legend:
|
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Serialization
Documentation