Raise ValueError for None in required AST scalar fields (#7680)

Match CPython's "field 'X' is required for Y" error format when
required scalar AST fields receive None during ast_from_object
conversion. Previously these produced TypeError or a generic
"None disallowed in expression list" ValueError.

Fields covered (Alias.name, Arg.arg, Comprehension.target/iter,
Keyword.value, MatchCase.pattern, WithItem.context_expr,
YieldFrom.value): all now raise the CPython-compatible message
verbatim.

Add a get_node_field_required helper alongside the existing
get_node_field / get_node_field_opt and switch the eight call
sites that read these required scalar fields. The helper rejects
None with the proper ValueError before the value flows into the
type converter where Rust's strong typing would otherwise force a
generic catch-all error. Optional fields (read via
get_node_field_opt) and fields where None is valid (e.g.
Constant.value) are unaffected.

This complements the existing AST validator pass at
crates/vm/src/stdlib/_ast/validate.rs (hooked at _ast.rs:763):
the validator handles post-conversion semantic invariants
(ExprContext, empty body, cross-field rules) but cannot handle
required-scalar-field None because Rust's non-Option types reject
None at conversion time, before validation runs.

Verified byte-identical with CPython 3.14.4 for all seven probe
cases. test.test_ast.test_ast: 227 tests, expected failures
38 -> 36 (test_empty_yield_from and test_none_checks unmasked,
no regressions). test.test_compile: 0 regressions.
This commit is contained in:
Changjoon
2026-04-25 21:53:37 +09:00
committed by GitHub
parent 7df0801db3
commit b427f31164
6 changed files with 35 additions and 10 deletions

View File

@@ -604,7 +604,6 @@ class AST_Tests(unittest.TestCase):
):
compile(e, "<test>", "eval")
@unittest.expectedFailure # TODO: RUSTPYTHON; TypeError: expected some sort of expr, but got None
def test_empty_yield_from(self):
# Issue 16546: yield from value is not optional.
empty_yield_from = ast.parse("def f():\n yield from g()")
@@ -1039,7 +1038,6 @@ class AST_Tests(unittest.TestCase):
with self.assertRaisesRegex(ValueError, f"^{e}$"):
compile(tree, "<test>", "exec")
@unittest.expectedFailure # TODO: RUSTPYTHON; TypeError: expected some sort of expr, but got None
def test_none_checks(self) -> None:
tests = [
(ast.alias, "name", "import spam as SPAM"),

View File

@@ -73,6 +73,33 @@ fn get_node_field(vm: &VirtualMachine, obj: &PyObject, field: &'static str, typ:
.ok_or_else(|| vm.new_type_error(format!(r#"required field "{field}" missing from {typ}"#)))
}
/// Read a required scalar field, rejecting both attribute absence and `None` value
/// with CPython-compatible error messages. Pairs with `get_node_field_opt` (which
/// returns `Option::None` for the same conditions): both filter `None`, but diverge
/// on whether to raise or return `None`.
///
/// Errors:
/// - Attribute absent: `TypeError("required field \"X\" missing from Y")` (via `get_node_field`).
/// - Attribute present but `None`: `ValueError("field 'X' is required for Y")`,
/// matching CPython's `Python/ast.c` validator output.
///
/// Use for required scalar fields where `None` is invalid (e.g. `comprehension.target`,
/// `keyword.value`, `match_case.pattern`). Do NOT use for fields where `None` is
/// legitimate (e.g. `Constant.value` representing the `None` literal — use plain
/// `get_node_field`); or for optional fields (use `get_node_field_opt`).
fn get_node_field_required(
vm: &VirtualMachine,
obj: &PyObject,
field: &'static str,
typ: &str,
) -> PyResult {
let value = get_node_field(vm, obj, field, typ)?;
if vm.is_none(&value) {
return Err(vm.new_value_error(format!("field '{field}' is required for {typ}")));
}
Ok(value)
}
fn get_node_field_opt(
vm: &VirtualMachine,
obj: &PyObject,

View File

@@ -802,7 +802,7 @@ impl Node for ast::ExprYieldFrom {
value: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "value", "YieldFrom")?,
get_node_field_required(vm, &object, "value", "YieldFrom")?,
)?,
range: range_from_object(vm, source_file, object, "YieldFrom")?,
})
@@ -1316,12 +1316,12 @@ impl Node for ast::Comprehension {
target: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "target", "comprehension")?,
get_node_field_required(vm, &object, "target", "comprehension")?,
)?,
iter: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "iter", "comprehension")?,
get_node_field_required(vm, &object, "iter", "comprehension")?,
)?,
ifs: Node::ast_from_object(
vm,

View File

@@ -92,7 +92,7 @@ impl Node for ast::Alias {
name: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "name", "alias")?,
get_node_field_required(vm, &object, "name", "alias")?,
)?,
asname: get_node_field_opt(vm, &object, "asname")?
.map(|obj| Node::ast_from_object(vm, source_file, obj))
@@ -140,7 +140,7 @@ impl Node for ast::WithItem {
context_expr: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "context_expr", "withitem")?,
get_node_field_required(vm, &object, "context_expr", "withitem")?,
)?,
optional_vars: get_node_field_opt(vm, &object, "optional_vars")?
.map(|obj| Node::ast_from_object(vm, source_file, obj))

View File

@@ -143,7 +143,7 @@ impl Node for ast::Parameter {
name: Node::ast_from_object(
_vm,
source_file,
get_node_field(_vm, &_object, "arg", "arg")?,
get_node_field_required(_vm, &_object, "arg", "arg")?,
)?,
annotation: get_node_field_opt(_vm, &_object, "annotation")?
.map(|obj| Node::ast_from_object(_vm, source_file, obj))
@@ -189,7 +189,7 @@ impl Node for ast::Keyword {
value: Node::ast_from_object(
_vm,
source_file,
get_node_field(_vm, &_object, "value", "keyword")?,
get_node_field_required(_vm, &_object, "value", "keyword")?,
)?,
range: range_from_object(_vm, source_file, _object, "keyword")?,
})

View File

@@ -34,7 +34,7 @@ impl Node for ast::MatchCase {
pattern: Node::ast_from_object(
vm,
source_file,
get_node_field(vm, &object, "pattern", "match_case")?,
get_node_field_required(vm, &object, "pattern", "match_case")?,
)?,
guard: get_node_field_opt(vm, &object, "guard")?
.map(|obj| Node::ast_from_object(vm, source_file, obj))