Skip to content

Commit 0bba529

Browse files
ghaithclaude
andcommitted
fix(parser): review followups — loop recovery, sharper comment, carve-out test
Review followups on PR #1709: - Replace the tail-recursing recovery with a loop drain. On adversarial input like `& & & g` the previous form emitted one diagnostic per `&` and recursed N times; the loop drains the operator run and retries once, producing exactly one diagnostic. Guards against infinite retry on non-operator bad tokens via an `advanced` flag — the reviewer's bare-while suggestion would have looped forever on inputs like `END_VAR` directly. - Sharpen the recovery comment: `to_operator()` also matches unary-eligible operators (`-`, `+`, `NOT`), but those don't reach this fallback because the prefix-operator parser handles them earlier in the cascade. - Add `consecutive_bad_operators_emit_a_single_diagnostic` to lock the N→1 reduction. - Add `empty_parameter_assignment_carve_out_still_fires` to anchor the positive complement of the narrowed `foo(p := )` carve-out. Verified during followup that no `.st` file under tests/lit, tests/integration, or libs relied on the previously-tolerated `&<ident>` shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5f585bb commit 0bba529

2 files changed

Lines changed: 105 additions & 7 deletions

File tree

src/parser/expressions_parser.rs

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -339,14 +339,27 @@ fn parse_atomic_leaf_expression(lexer: &mut ParseSession<'_>) -> Option<AstNode>
339339
lexer.slice(),
340340
lexer.location(),
341341
));
342-
// If the bad token is a binary-only operator misused as a
343-
// prefix (e.g. `&y`, `MOD y`), consume it and retry the leaf
344-
// so the recovered AST captures the operand the user wrote.
345-
// For non-operator tokens (e.g. END_CASE, END_VAR), leave the
346-
// token in the stream so outer parsers can use it for
347-
// synchronization.
348-
if to_operator(&lexer.token).is_some() {
342+
// If the bad token has any operator interpretation, drain
343+
// any consecutive operator tokens and retry the leaf so the
344+
// recovered AST captures the operand the user wrote. In
345+
// practice this fires for *binary-only* operators misused
346+
// as a prefix (e.g. `&y`, `MOD y`); unary-eligible
347+
// operators (`-`, `+`, `NOT`) also match `to_operator()`
348+
// but normally never reach this fallback because the
349+
// prefix-operator parser higher in the cascade handles
350+
// them first. Looping (rather than tail-recursing per
351+
// token) emits one diagnostic per run instead of N, and
352+
// keeps the recovery cost flat on adversarial input like
353+
// `& & & g`. For non-operator tokens (e.g. `END_CASE`,
354+
// `END_VAR`), leave the token in the stream so outer
355+
// parsers can synchronise on it — the `advanced` guard
356+
// prevents an infinite retry loop in that case.
357+
let mut advanced = false;
358+
while to_operator(&lexer.token).is_some() {
349359
lexer.advance();
360+
advanced = true;
361+
}
362+
if advanced {
350363
return parse_atomic_leaf_expression(lexer);
351364
}
352365
None

src/parser/tests/expressions_parser_tests.rs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,91 @@ fn amp_as_unary_prefix_in_assignment_recovers() {
11131113
"#);
11141114
}
11151115

1116+
#[test]
1117+
fn consecutive_bad_operators_emit_a_single_diagnostic() {
1118+
// `& & & g` previously emitted one diagnostic per `&` and recursed N
1119+
// times. The loop drains the run and retries once, so adversarial input
1120+
// produces exactly one diagnostic and one recovered identifier.
1121+
let src = "
1122+
PROGRAM prg
1123+
VAR x : DINT; g : DINT; END_VAR
1124+
x := & & & g;
1125+
END_PROGRAM
1126+
";
1127+
let (unit, diagnostics) = parse_buffered(src);
1128+
assert_snapshot!(diagnostics, @r"
1129+
error[E007]: Unexpected token: expected expression but found &
1130+
┌─ <internal>:4:18
1131+
1132+
4 │ x := & & & g;
1133+
│ ^ Unexpected token: expected expression but found &
1134+
");
1135+
assert_debug_snapshot!(unit.implementations[0].statements, @r#"
1136+
[
1137+
Assignment {
1138+
left: ReferenceExpr {
1139+
kind: Member(
1140+
Identifier {
1141+
name: "x",
1142+
},
1143+
),
1144+
base: None,
1145+
},
1146+
right: ReferenceExpr {
1147+
kind: Member(
1148+
Identifier {
1149+
name: "g",
1150+
},
1151+
),
1152+
base: None,
1153+
},
1154+
},
1155+
]
1156+
"#);
1157+
}
1158+
1159+
#[test]
1160+
fn empty_parameter_assignment_carve_out_still_fires() {
1161+
// The narrowed `foo(p := )` recovery must keep firing when the next token
1162+
// is `)` or `,` — the carve-out is the deliberate complement of the bad-
1163+
// token recovery above. Verifies that tightening the predicate to
1164+
// `KeywordParensClose | KeywordComma` didn't break the positive path.
1165+
let src = "
1166+
FUNCTION foo : DINT VAR_INPUT p : DINT; q : DINT; END_VAR foo := p; END_FUNCTION
1167+
PROGRAM main
1168+
foo(p := );
1169+
foo(p := , q := 1);
1170+
END_PROGRAM
1171+
";
1172+
let (unit, diagnostics) = parse_buffered(src);
1173+
assert!(diagnostics.is_empty(), "expected no parse diagnostics, got:\n{diagnostics}");
1174+
1175+
let main = unit.implementations.iter().find(|i| i.name == "main").unwrap();
1176+
// Both calls should land an Assignment with right == EmptyStatement.
1177+
for stmt in &main.statements {
1178+
let AstStatement::CallStatement(call) = &stmt.stmt else {
1179+
panic!("expected CallStatement, got {:?}", stmt);
1180+
};
1181+
let Some(params) = call.parameters.as_deref() else {
1182+
panic!("expected parameters on call, got {:?}", call);
1183+
};
1184+
// walk into the first Assignment we find and check its RHS
1185+
let first_assignment = match &params.stmt {
1186+
AstStatement::Assignment(a) => a,
1187+
AstStatement::ExpressionList(list) => match &list[0].stmt {
1188+
AstStatement::Assignment(a) => a,
1189+
other => panic!("expected Assignment in list head, got {:?}", other),
1190+
},
1191+
other => panic!("expected Assignment or ExpressionList, got {:?}", other),
1192+
};
1193+
assert!(
1194+
matches!(first_assignment.right.stmt, AstStatement::EmptyStatement(..)),
1195+
"expected RHS to be EmptyStatement, got {:?}",
1196+
first_assignment.right.stmt
1197+
);
1198+
}
1199+
}
1200+
11161201
#[test]
11171202
fn binary_only_operator_as_prefix_recovers() {
11181203
// Generality witness — recovery is not specific to `&`.

0 commit comments

Comments
 (0)