⚡ Sangsakala Proxy 🏠 Home
Skip to content

Raise ValueError for None in Expr lists to match CPython validator - #7656

Merged
youknowone merged 2 commits into
RustPython:mainfrom
changjoon-park:fix-ast-none-disallowed
Apr 23, 2026
Merged

youknowone merged 2 commits into
RustPython:mainfrom
changjoon-park:fix-ast-none-disallowed

Conversation

@changjoon-park

@changjoon-park changjoon-park commented Apr 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

CPython raises ValueError(\"None disallowed in expression list\") when compile() encounters None as an element in an Expr list 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.

Fix

One check in Expr::ast_from_object (crates/vm/src/stdlib/_ast/expression.rs) before the catch-all:

} else if vm.is_none(&object) {
    return Err(vm.new_value_error("None disallowed in expression list".to_owned()));
}

Option<Expr> positions (e.g. Dict.keys for **unpack) are unaffected — node.rs Option<T>::ast_from_object already short-circuits None there, so this change only fires when None reaches the non-optional Vec<Expr> iteration path.

Verification

CPython 3.13.4 baseline message:

$ python3 -c "import ast; compile(ast.Expression(ast.Set([None])), '<t>', 'eval')"
ValueError: None disallowed in expression list

After the fix, rustpython -c "import ast; compile(ast.Expression(ast.Set([None])), '<t>', 'eval')" raises the same message.

Full test.test_ast.test_ast suite (227 tests):

Ran 227 tests in 18.144s
OK (skipped=10, expected failures=39)

0 failures, 0 unexpected successes.

Unmasked tests (12)

In Lib/test/test_ast/test_ast.py:

  • test_boolop — was @unittest.skip with stale Option::unwrap() message
  • test_set, test_list, test_tuple — were @unittest.expectedFailure / TypeError
  • test_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 by test_classdef — needs a sibling change in statement.rs. Out of scope here; can be a follow-up PR with the identical pattern.

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling when a missing/empty (None) value is encountered in an expression list. Users will now see a clearer, specific error indicating None is disallowed in expression lists instead of a generic type mismatch, making invalid-expression diagnostics more actionable.

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.
@coderabbitai

coderabbitai Bot commented Apr 22, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 73199a96-6612-4a72-94e0-619aa94ff96c

📥 Commits

Reviewing files that changed from the base of the PR and between 36cbdf5 and 95c6566.

📒 Files selected for processing (1)
  • crates/vm/src/stdlib/_ast/expression.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/stdlib/_ast/expression.rs

📝 Walkthrough

Walkthrough

Explicitly reject Python None in ast::Expr::ast_from_object: after failing known expression node checks the code now calls vm.is_none(&object) and returns a ValueError with message "None disallowed in expression list" instead of a generic type-mismatch error. (50 words)

Changes

Cohort / File(s) Summary
Expression AST Validation
crates/vm/src/stdlib/_ast/expression.rs
After existing node-type checks, ast_from_object now explicitly detects None via vm.is_none(&object) and raises a ValueError("None disallowed in expression list") instead of the previous generic type-mismatch path.

Estimated Code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibble on bytes and guard the nest,
None tried to slip past my careful test.
"Not here," I twitch — a gentle chide,
Now expressions carry no None inside.
thump thump 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a ValueError check for None in expression lists to match CPython's behavior, which is the core objective of this PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Apr 22, 2026 •

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The 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:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5a45d41 and 1a32bfa.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_ast/test_ast.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/stdlib/_ast/expression.rs

Comment thread crates/vm/src/stdlib/_ast/expression.rs Resolved
Comment thread crates/vm/src/stdlib/_ast/expression.rs Resolved

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! tysm!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
crates/vm/src/stdlib/_ast/expression.rs (1)

136-137: ⚠️ Potential issue | 🟠 Major

Keep the None check 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1a32bfa and 36cbdf5.

📒 Files selected for processing (1)
  • crates/vm/src/stdlib/_ast/expression.rs

@changjoon-park
changjoon-park force-pushed the fix-ast-none-disallowed branch from 36cbdf5 to 1a32bfa Compare April 22, 2026 16:53
Matches the other new_value_error call sites in the file.

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
@changjoon-park

Copy link
Copy Markdown
Contributor Author

Amazing! tysm!

thanks for the review !

@youknowone
youknowone merged commit f2e055f into RustPython:main Apr 23, 2026
31 of 40 checks passed
youknowone pushed a commit that referenced this pull request Apr 24, 2026
…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).
@changjoon-park
changjoon-park deleted the fix-ast-none-disallowed branch April 27, 2026 13:24
youknowone pushed a commit that referenced this pull request Sep 16, 2026
…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>
youknowone pushed a commit that referenced this pull request Sep 16, 2026
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants