parser(step 40): statements & control flow #9
Loading…
Reference in a new issue
No description provided.
Delete branch "parser/stmt"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements parser step 40 — Statements & control flow:
parse_stmt()and the recursive-descent layer above the step-30 expression core. Turns a function body intoStmts.What's here
parse_stmt(single-token dispatch),parse_block,parse_var_decl(named + tuple-destructure),parse_expr_stmt,parse_return,parse_if/parse_while,parse_for(ForIn vs C-style ForC),parse_switch/parse_case/parse_default, plusparse_stmt_textharness.stmt_span+ span-clamped statement construction; 14 shapes assert0..lenspan coverage.ast.rs,lexer, and the ignored step-70 fixture driver untouched.Verification
cargo fmt --check— cleancargo clippy --all --all-targets -- -D warnings— zero warningscargo test --all— 51 parser tests pass, no regressionsOpen items flagged (not silently decided)
forslots are refused.language-grammar.mdwritesinit/updateasexpr ("," expr)*, butStmt::ForCholds one init + one update.for (i = 0, j = 1;;)errors at the comma rather than dropping the second expression. EitherForCneeds list slots or the grammar should drop the comma list — no fixture uses it.defaultarm is wrapped in a synthesizedStmt::Block(theSwitchdefault slot holds one stmt).ast.rsdump cosmetics:ForC/Switch/Caseuseformat!(notsexp), so absent parts leave literal spaces ((for ; ; (block))). Cosmetic, not a behavior bug — worth a follow-up onast.rsif those should read like the rest.40-statements.md:89shows(for (assign i 0) (< i n) (++ i)); the implemented (AST) dump is(for (expr (assign i = 0)) ;(< i n) ;(++ i) (block)). The dump is the contract; tests follow it.Prior-commit adversarial review of parser(step 40). I traced every Stmt branch, computed spans by hand, cross-checked the golden strings against ast.rs dump() and the source-of-truth grammar (docs/prd/language-grammar.md), and ran the suite:
cargo test -p parser= 51 passed,cargo clippy --all --all-targets -- -D warningsclean,cargo fmt --checkclean. The golden assertions are genuine dump() output, not fudged.Verdict: COMMENT (concerns), no approval. Findings ranked 0-5, top first.
[3] Comma lists in
forinit/update are refused, contradicting the binding grammar.no_comma_list(parser.rs:1412) hard-rejectsfor (i = 0, j = 0; ...)andfor (;; i++, j++), but language-grammar.md:363-364 (the source of truth) writes these slots asexpr ("," expr)*. Stmt::ForC holds one init stmt and one update expr, and the author chose refusal over truncation - defensible as a local call, but per AGENTS.md the grammar is authoritative and divergences are meant to be "resolved in docs/prd/ first." As landed, the parser rejects a documented, derivable production to-spec user code will hit. Fix: grow ForC to hold lists (init: Vec / update: Vec, as 40-statements.md's own case-body machinery suggests) - or amend language-grammar.md to a single-expression slot and update ForC's AST comment.[1] for-in lookahead hijacks a grammar-valid C-style init built on the
inbinary.parser.rs:1353 routes
for (<ident> in ...)to ForIn unconditionally.inis a BP_COMPARE binary (infix_op:420), sofor (a in b; a < n; ++a)is a derivable for-c whose init is a membership expr - but it is misread as for-in and dies on the;. It fails hard (no silent wrong tree) and for-in is the idiomatic reading, hence low severity, but the ambiguity is real and undocumented in 40-statements.md; at minimum a note in the step doc is owed.[1] Synthesized Block around every
defaultarm.parse_default (parser.rs:1498) wraps the arm in Stmt::Block because Switch.default is Option<Box>, asymmetric with Case which holds Vec directly. The block is real and spans the arm (no invented position), so tolerable - but a single-statement default gains a block it never wrote, and the dump renders
default:as(block ...). If Case may hold a list, Switch.default should too.[1] Dump cosmetics + stale step doc (open items #3/#4, both real).
ast.rs renders ForC/Switch/Case with format! rather than sexp, so absent parts leave literal doubled/trailing spaces the goldens now guarantee:
(for ; ; (block))(parser.rs:2129),(switch x (case 1 ) )(2165),(switch x )(2166). And docs/parser/40-statements.md:89 still writes(for (assign i 0) (< i n) (++ i))- no(expr ...)wrapper, no=, no;separators - which the implementation provably does not emit (2114-2121). Following the dump is right (dump is the contract, README locked decision #4); the step doc just needs reconciling.[1] Qualified binding rejected with the wrong message.
let A::b = x;is correctly refused at byte 5 (parser.rs:2236), but it fails through parse_var_decl'sexpect(';')at 1220, so the user sees "expected ';' after aletdeclaration" pointed at the::- when the real rule is "binding positions may not be qualified." Offsets are right; the message names the wrong production. parse_ident_name already errors on a non-identifier; surfacing the qualification case there would be more honest.Note (not a finding, out of the 0-5 rank): step 40 claims to complete array-iteration.cat, array-c-style-loop.cat and the function-*.cat fixtures, but tests/fixtures.rs is
#[ignore]d, so those binding fixtures are never actually parsed by this PR - acceptance rests on hand-written inline strings of the same shape. Confirm the fixtures genuinely parse when the step-70 driver is enabled.Top finding: for-c init/update reject the comma list the source-of-truth grammar promises. Severity 3.
[Severity 3] Comma lists in for-c init/update refused — contradicts the binding grammar.
Anchor: parser/src/parser.rs:1412 (fn no_comma_list), raised via parser.rs:1374 & 1396.
The source-of-truth grammar (docs/prd/language-grammar.md:363-364) writes
init = var-decl | expr ("," expr)*andupdate = expr ("," expr)*, but Stmt::ForC holds a single init stmt and a single update expr, so this hard-refusesfor (i = 0, j = 0; ...)andfor (;; i++, j++). Refusal beats silent truncation — agreed — but the grammar is authoritative and AGENTS.md says resolve such gaps in docs/prd/ first. As landed, a documented, derivable production is unparseable. Either grow ForC to carry lists (init: Vec, update: Vec) or amend the grammar to a single-expression slot and update the ForC AST comment.[Severity 1] for-in lookahead hijacks a grammar-valid C-style init built on the
inbinary.Anchor: parser/src/parser.rs:1353.
parser.rs:1353 routes
for (<ident> in ...)to ForIn unconditionally.inis also a BP_COMPARE binary (parser.rs:420), sofor (a in b; a < n; ++a)is a derivable for-c whose init is a membership expression — but it is misread as for-in and dies on the;. It fails hard (no silent wrong tree) and for-in is the idiomatic reading, hence low severity, but the ambiguity is real and undocumented in 40-statements.md; at minimum a note in the step doc is owed.[Severity 1] Synthesized Block wraps every
defaultarm.Anchor: parser/src/parser.rs:1498 (parse_default), see also 1487-1490.
Switch.default is Option<Box> while Case holds Vec directly, so parse_default wraps each default arm in a Stmt::Block. The block spans the arm correctly (no invented position) so it is safe, but a single-statement
default: g();gains a block it never wrote and renders as(block (expr (g)))instead of a symmetric(case ...)-style list. If Case may hold a list, the default slot should too — or the asymmetry is worth a written rationale.[Severity 1] Dump cosmetics now part of the test contract + stale step doc (open items #3/#4, both real).
Anchor: parser/src/parser.rs:2121 (and 2129, 2165-2166).
ast.rs renders ForC/Switch/Case with format! rather than sexp, so absent parts leave literal doubled/trailing spaces the goldens now guarantee:
(for ; ; (block))(2129),(switch x (case 1 ) )(2165),(switch x )(2166). And docs/parser/40-statements.md:89 still promises(for (assign i 0) (< i n) (++ i))— no(expr ...)wrapper, no=, no;separators — which the implementation provably does not emit (2114-2121). ForC renders the init slot wrapped in an ExprStmt and separates slots with;. Following the dump is correct (dump = contract, README locked decision #4); the step doc simply needs reconciling to the real dumps.[Severity 1] Qualified binding refused with the wrong diagnostic.
Anchor: parser/src/parser.rs:2236 (test), root cause parse_var_decl:1220.
let A::b = x;is correctly refused at byte 5, but it fails through parse_var_decl'sexpect(';')at 1220, so the user sees "expected ';' after aletdeclaration" pointed at the::— when the real rule is that binding positions are never::-qualified. The rejection offset and the test are right; the message names the wrong production. parse_ident_name already errors on a non-identifier; surfacing the qualification case there would be more honest about what actually went wrong.[Note — not ranked 0-5] The step's fixture-acceptance claim is unverified.
Step 40's acceptance section says it completes array-iteration.cat, array-c-style-loop.cat and the function-*.cat fixtures, but tests/fixtures.rs is
#[ignore]d (the step-70 driver), so none of those binding fixtures is actually parsed by this PR. Acceptance rests on hand-written inline strings of the same shape (which do pass — 51 tests green), not on the manifest's real fixtures. Out of scope for this PR per its own note; worth confirming the fixtures genuinely parse when the step-70 driver is enabled, since that is the binding acceptance surface per AGENTS.md.- ForC init/update hold comma lists (grammar init/update = expr ("," expr)*): init is Vec<Stmt> (one ExprStmt per expr), update is Vec<Expr>; drop the no_comma_list refusal and its tests, add positive comma-list goldens. - Switch.default becomes Option<DefaultArm> (new node, symmetric with Case) instead of a synthesized Stmt::Block; dumps as (default ...). - parse_ident_name rejects a trailing '::' with an honest diagnostic ("binding positions may not be ::'-qualified") at the correct byte, so let A::b = x; no longer surfaces as a missing ';'. - docs/parser/40-statements.md: reconcile the for/switch dump examples to the real output and document the for-in lookahead ambiguity (disambiguating is deliberately deferred; it fails hard, never a wrong tree).Addressed the prior-commit review (addresses sev 1 & sev 3) in
27ef7c1:Stmt::ForCinit/update now hold comma lists (grammarinit/update = expr (\",\" expr)*): init isVec<Stmt>(oneExprStmtper expr, only the last reaching the shared;in its span), update isVec<Expr>;no_comma_listdropped, with positive comma-list goldens.Switch.defaultis nowOption<DefaultArm>(new node symmetric withCase), dumps(default ...); the synthesizedStmt::Blockis gone.parse_ident_namerejects a trailing::with an honest diagnostic at byte 5;let A::b = x;no longer reads as a missing;.for/switchdumps and documents thefor (<ident> in ...)lookahead ambiguity (deferred: fails hard, never a wrong tree).Verification:
cargo fmt --check,cargo clippy --all --all-targets -- -D warnings, andcargo test --allall pass (53 parser tests green, 0 failures).A header like "for (a in b; a < n; ++a)" is a grammar-valid C-style loop whose init is the membership expression "a in b", but parse_for's two-token lookahead routed it to for-in and it died on the ';'. The cursor is a position into a fixed token vector, so the for-in read is now speculative: parse the iterable, then let the header terminator decide. ')' completes the for-in; a ';' or ',' means the C-style slots, so rewind to just after '(' and re-parse "a in b" as the init. Anything else stays a for-in "expected ')'" diagnostic — the likely intent. Cursor gains Clone to make the save/restore cheap and side-effect-free.Follow-up: the for-in / C-style overlap is no longer merely documented — it is now resolved by backtracking (
c753609).for (a in b; a < n; ++a)is a grammar-valid C-style loop whose init is the membership expressiona in b; the two-token for-in lookahead used to hijack it and fail at the;. Since the Cursor is just a position into a fixed token vector (it now derives Clone, which is the whole point), parse_for parses the for-in speculatively and lets the header terminator decide:)→ for-in;;or,→ rewind to just after(and re-parse as the C-style init (no hand-spliced nodes). Anything else stays the for-in "expected ')'" diagnostic, so a broken for-in is not re-reported as a bad C-style init.New tests:
an_in_init_takes_the_c_style_branch(incl. a,init-list variant) anda_broken_for_in_keeps_its_own_diagnostic. Gates still pass: fmt clean, clippy -D warnings clean, parser 55/55. The step doc now documents the disambiguation instead of the old workaround.