Repository navigation
Raise ValueError for None in Expr lists to match CPython validator - #7656
Conversation
CPython raises ValueError("None disallowed in expression list") when
compile() encounters None as an element in a Vec<Expr> position
(Set/List/Tuple.elts, BoolOp.values, Call.args, comprehension generators,
etc.). RustPython reached the catch-all TypeError("expected some sort of
expr, but got {}") branch instead, failing a dozen test_ast validator
tests.
Add an explicit is_none() check in Expr::ast_from_object before the
catch-all so None short-circuits with the CPython-compatible ValueError.
Option<Expr> positions (e.g. Dict.keys for **unpack) are unaffected
since node.rs Option<T>::ast_from_object handles None earlier.
Unmasks 12 skip / expectedFailure markers in
Lib/test/test_ast/test_ast.py:
- test_boolop, test_set, test_list, test_tuple
- test_call, test_delete, test_assign
- test_dict, test_dictcomp, test_generatorexp, test_listcomp, test_setcomp
Addresses part of the skip-test inventory in RustPython#7611.
The Stmt-level analog (body=[None] -> "None disallowed in statement list",
used by test_classdef) needs a sibling change in statement.rs; out of
scope here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExplicitly reject Python Changes
Estimated Code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'ast test_code test_compile test_decorators test_descr test_inspect test_json test_named_expressions test_peepholer' not found) Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/_ast/expression.rs`:
- Around line 136-137: The generic ast::Expr::ast_from_object currently rejects
None with the message "None disallowed in expression list", but that dispatcher
is used for both list elements and required single-expression fields (e.g.,
Lambda.body, IfExp.test, Call.func), so the error message is misleading; move
the None check out of ast::Expr::ast_from_object and into the list-extraction
path (e.g., Vec<ast::Expr>::ast_from_object or extract_elements_with) so only
list element parsing yields "None disallowed in expression list", or
alternatively change ast_from_object to accept a context parameter and make
callers (Lambda, IfExp, Call constructors) pass context to produce
field-specific messages like "field 'body' is required for Lambda".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5426a12d-7940-4ade-a531-0b78c538261d
⛔ Files ignored due to path filters (1)
Lib/test/test_ast/test_ast.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/_ast/expression.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/stdlib/_ast/expression.rs (1)
136-137:⚠️ Potential issue | 🟠 MajorKeep the
Nonecheck scoped to expression-list elements.This generic dispatcher is also used for required single-expression fields, so
Lambda(body=None),Call(func=None), etc. can now receive the list-specific"None disallowed in expression list"error instead of CPython’s field-specific validation error. Move this handling into the expression-sequence conversion path, or pass enough context to distinguish list elements from required fields.#!/bin/bash set -euo pipefail # Compare CPython's behavior for list-element None vs required single-expression None. python3 - <<'PY' import ast args = ast.arguments( posonlyargs=[], args=[], vararg=None, kwonlyargs=[], kw_defaults=[], kwarg=None, defaults=[], ) cases = [ ("list element None", ast.Expression(body=ast.List(elts=[None], ctx=ast.Load()))), ("lambda body None", ast.Expression(body=ast.Lambda(args=args, body=None))), ("call func None", ast.Expression(body=ast.Call(func=None, args=[], keywords=[]))), ] for name, tree in cases: try: compile(ast.fix_missing_locations(tree), "<ast>", "eval") except Exception as exc: print(f"{name}: {type(exc).__name__}: {exc}") else: print(f"{name}: OK") PY # Inspect required single-expression call sites that route through ast::Expr::ast_from_object. rg -n -C2 'get_node_field\(vm, &object, "(body|test|func|value|left|right|target|iter|slice|elt|key)"' crates/vm/src/stdlib/_ast/expression.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_ast/expression.rs` around lines 136 - 137, Remove the generic vm.is_none(&object) check and error return from the shared expression dispatcher (the code that emits vm.new_value_error("None disallowed in expression list")), and instead enforce "None disallowed in expression list" only inside the expression-sequence conversion path (the code that converts AST list elements to Expr nodes); leave required single-expression sites (e.g., Lambda.body, Call.func, etc., which route through ast::Expr::ast_from_object / get_node_field calls) to use their field-specific validation. Concretely: delete or conditionally bypass the vm.is_none check in the shared dispatcher and add the None-check + vm.new_value_error(...) inside the function that converts Vec<Expr> or list elements to expressions (or pass an explicit flag/context from callers to distinguish "list element" vs "single required field"), so list elements trigger the list-specific error while single-expression fields keep their original validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/stdlib/_ast/expression.rs`:
- Around line 136-137: Remove the generic vm.is_none(&object) check and error
return from the shared expression dispatcher (the code that emits
vm.new_value_error("None disallowed in expression list")), and instead enforce
"None disallowed in expression list" only inside the expression-sequence
conversion path (the code that converts AST list elements to Expr nodes); leave
required single-expression sites (e.g., Lambda.body, Call.func, etc., which
route through ast::Expr::ast_from_object / get_node_field calls) to use their
field-specific validation. Concretely: delete or conditionally bypass the
vm.is_none check in the shared dispatcher and add the None-check +
vm.new_value_error(...) inside the function that converts Vec<Expr> or list
elements to expressions (or pass an explicit flag/context from callers to
distinguish "list element" vs "single required field"), so list elements trigger
the list-specific error while single-expression fields keep their original
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 85922a0e-08a4-482e-8938-37ca133c6ca8
📒 Files selected for processing (1)
crates/vm/src/stdlib/_ast/expression.rs
36cbdf5 to
1a32bfa
Compare
Matches the other new_value_error call sites in the file. Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
thanks for the review ! |
…7670) Sibling fix to #7656 (which handled Vec<Expr>). The same catch-all TypeError ("expected some sort of stmt, but got {}") in Stmt::ast_from_object silently swallowed None, so compile() on an AST with None in a statement-list field (ClassDef.body, Try.body, For.body, etc.) raised TypeError where CPython raises ValueError("None disallowed in statement list"). Add an is_none check before the catch-all, matching the Expr-side shape introduced in #7656. Option<Stmt> positions are unaffected — they short-circuit None earlier in node.rs. Unmasks test_classdef (body=[None] sub-case). Full test.test_ast.test_ast run: 227 tests, 0 unexpected successes, 38 expected failures (was 39).
…7656) * Raise ValueError for None in Expr lists to match CPython validator CPython raises ValueError("None disallowed in expression list") when compile() encounters None as an element in a Vec<Expr> position (Set/List/Tuple.elts, BoolOp.values, Call.args, comprehension generators, etc.). RustPython reached the catch-all TypeError("expected some sort of expr, but got {}") branch instead, failing a dozen test_ast validator tests. Add an explicit is_none() check in Expr::ast_from_object before the catch-all so None short-circuits with the CPython-compatible ValueError. Option<Expr> positions (e.g. Dict.keys for **unpack) are unaffected since node.rs Option<T>::ast_from_object handles None earlier. Unmasks 12 skip / expectedFailure markers in Lib/test/test_ast/test_ast.py: - test_boolop, test_set, test_list, test_tuple - test_call, test_delete, test_assign - test_dict, test_dictcomp, test_generatorexp, test_listcomp, test_setcomp Addresses part of the skip-test inventory in #7611. The Stmt-level analog (body=[None] -> "None disallowed in statement list", used by test_classdef) needs a sibling change in statement.rs; out of scope here. * Drop redundant .to_owned() on new_value_error argument Matches the other new_value_error call sites in the file. Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> --------- Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
…7670) Sibling fix to #7656 (which handled Vec<Expr>). The same catch-all TypeError ("expected some sort of stmt, but got {}") in Stmt::ast_from_object silently swallowed None, so compile() on an AST with None in a statement-list field (ClassDef.body, Try.body, For.body, etc.) raised TypeError where CPython raises ValueError("None disallowed in statement list"). Add an is_none check before the catch-all, matching the Expr-side shape introduced in #7656. Option<Stmt> positions are unaffected — they short-circuit None earlier in node.rs. Unmasks test_classdef (body=[None] sub-case). Full test.test_ast.test_ast run: 227 tests, 0 unexpected successes, 38 expected failures (was 39).
Summary
CPython raises
ValueError(\"None disallowed in expression list\")whencompile()encountersNoneas an element in anExprlist position (Set/List/Tuple.elts,BoolOp.values,Call.args, comprehension generators, etc.). RustPython reached the catch-allTypeError(\"expected some sort of expr, but got {}\")branch instead, failing a dozentest_astvalidator tests.Fix
One check in
Expr::ast_from_object(crates/vm/src/stdlib/_ast/expression.rs) before the catch-all:Option<Expr>positions (e.g.Dict.keysfor**unpack) are unaffected —node.rsOption<T>::ast_from_objectalready short-circuitsNonethere, so this change only fires whenNonereaches the non-optionalVec<Expr>iteration path.Verification
CPython 3.13.4 baseline message:
After the fix,
rustpython -c "import ast; compile(ast.Expression(ast.Set([None])), '<t>', 'eval')"raises the same message.Full
test.test_ast.test_astsuite (227 tests):0 failures, 0 unexpected successes.
Unmasked tests (12)
In
Lib/test/test_ast/test_ast.py:test_boolop— was@unittest.skipwith staleOption::unwrap()messagetest_set,test_list,test_tuple— were@unittest.expectedFailure/ TypeErrortest_call,test_delete,test_assign,test_dict,test_dictcomp,test_generatorexp,test_listcomp,test_setcomp— were@unittest.expectedFailure/ "ValueError not raised"Addresses part of the skip-test inventory in #7611.
Scope notes
The Stmt-level analog —
body=[None]raising\"None disallowed in statement list\", used bytest_classdef— needs a sibling change instatement.rs. Out of scope here; can be a follow-up PR with the identical pattern.Summary by CodeRabbit