navicore/patch-rexx

Fork 0

Pass 1 of src/parser.rs audit complete. #27

Merged

navicore merged 3 commits from parser-refactor into main

2026-04-28 01:47:06 +00:00

navicore commented

2026-04-27 22:34:37 +00:00

(Migrated from github.com)

Applied:

G1 — parse_clause's 22-arm if-else keyword chain replaced with one match name.to_ascii_uppercase().as_str(). Drops #[allow(clippy::too_many_lines)].
G2 — finish_do(loc, kind, name) -> RexxResult helper. All six DO variants (Simple, Forever, While, Until, Count, Controlled) now share the skip-terminators + parse-body + Clause-build tail.
G3 — consume_unique_keyword(kw, slot) helper. The TO/BY/FOR/WHILE/UNTIL block in parse_controlled_do shrank from ~45 lines to 5.
G4 — Deleted is_then_keyword, is_else_keyword, is_with_keyword. Four call sites switched to self.is_keyword("THEN" | "ELSE" | "WITH") directly.
G5 — Removed redundant // consume KEYWORD and similar restate-the-token comments throughout. Kept the ones that explain non-obvious things (depth-counter purpose, save/restore-pos rationale, named-END syntax, DO-variant disambiguation, precedence ladder).
G6 — parse_optional_expression() helper applied to parse_exit, parse_return, parse_push, parse_queue, and as a bonus to parse_numeric's DIGITS/FUZZ arms.

Result:

1981 → 1792 L (−189).
#[allow(clippy::too_many_lines)] removed from parse_clause.
Net function count unchanged (added 3 helpers, removed 3 wrapper checks).
just ci green: fmt-check ✓ (one auto-fmt nudge on the new helper signature, applied via just fmt), clippy ✓, all 500+ tests ✓, release build ✓.
Public API unchanged.

Applied: - G1 — parse_clause's 22-arm if-else keyword chain replaced with one match name.to_ascii_uppercase().as_str(). Drops #[allow(clippy::too_many_lines)]. - G2 — finish_do(loc, kind, name) -> RexxResult<Clause> helper. All six DO variants (Simple, Forever, While, Until, Count, Controlled) now share the skip-terminators + parse-body + Clause-build tail. - G3 — consume_unique_keyword(kw, slot) helper. The TO/BY/FOR/WHILE/UNTIL block in parse_controlled_do shrank from ~45 lines to 5. - G4 — Deleted is_then_keyword, is_else_keyword, is_with_keyword. Four call sites switched to self.is_keyword("THEN" | "ELSE" | "WITH") directly. - G5 — Removed redundant // consume KEYWORD and similar restate-the-token comments throughout. Kept the ones that explain non-obvious things (depth-counter purpose, save/restore-pos rationale, named-END syntax, DO-variant disambiguation, precedence ladder). - G6 — parse_optional_expression() helper applied to parse_exit, parse_return, parse_push, parse_queue, and as a bonus to parse_numeric's DIGITS/FUZZ arms. Result: - 1981 → 1792 L (−189). - #[allow(clippy::too_many_lines)] removed from parse_clause. - Net function count unchanged (added 3 helpers, removed 3 wrapper checks). - just ci green: fmt-check ✓ (one auto-fmt nudge on the new helper signature, applied via just fmt), clippy ✓, all 500+ tests ✓, release build ✓. - Public API unchanged.

claude[bot] commented

2026-04-27 22:42:15 +00:00

(Migrated from github.com)

Code Review — PR #27 (parser.rs audit pass 1)

The refactoring is sound: the six helpers (G1–G6) are semantically equivalent to the code they replace, all 500+ tests pass, and the structural improvements are real. One performance regression was introduced by G1, and two pre-existing correctness issues became more visible.

Bug introduced — G1: per-clause `String` allocation in `parse_clause`

File: src/parser.rs, line 167

match name.to_ascii_uppercase().as_str() {

to_ascii_uppercase() heap-allocates a new String on every parse_clause call, including all the common paths (assignments, command clauses, label definitions) where name is not a keyword at all. Before G1, the dispatch used Self::check_keyword(name, kw) which resolves to eq_ignore_ascii_case — a zero-allocation in-place comparison.

This is a measurable regression for scripts with many clauses. The fix is to either store the uppercase form once before the match, or apply eq_ignore_ascii_case comparisons directly:

// Option A: compute once, allocate once
let upper = name.to_ascii_uppercase();
match upper.as_str() { ... }

// Option B: keep zero-alloc comparisons, restructure as a helper
// that returns a KeywordKind enum before calling the dispatch fn

Option A is the smallest mechanical fix and keeps the match readable.

Pre-existing — `parse_do_body` silently accepts wrong END name

File: src/parser.rs, lines 462–467

if self.is_keyword("END") {
    self.advance();
    // Optionally consume a loop name after END (e.g., `END i`).
    if let TokenKind::Symbol(_) = self.peek_kind()
        && !self.is_terminator()
    {
        self.advance();  // consumes any symbol, not just the matching name
    }
    break;
}

REXX spec §7.4.6: "The name in an END instruction, if specified, must be the same as the name of the control variable in the corresponding DO instruction." The code accepts end j for a do i = 1 to 10 without error. The loop variable name is available via DoBlock::name; the fix is to thread it into parse_do_body and compare. Not introduced here, but worth tracking.

Pre-existing — dead predicate in `parse_do_body`

File: src/parser.rs, line 463

if let TokenKind::Symbol(_) = self.peek_kind()
    && !self.is_terminator()

is_terminator() returns true only for Semicolon | Eol | Eof. A TokenKind::Symbol can never be any of those, so !self.is_terminator() is always true when the outer pattern matches. The guard is dead and should be removed to avoid misleading future readers. Also not introduced here.

Everything else looks correct

G2 finish_do: all six DO variants call it with the same skip_terminators → parse_do_body sequence as the original. ✓
G3 consume_unique_keyword: duplicate-keyword detection is preserved; error messages are string-identical. ✓
G4 is_keyword("THEN"|"ELSE"|"WITH"): identical to the deleted helpers since both call check_keyword → eq_ignore_ascii_case. ✓
G6 parse_optional_expression: EXIT / RETURN / PUSH / QUEUE / NUMERIC DIGITS / FUZZ all behave identically. ✓
do_header_depth is still active while consume_unique_keyword parses sub-expressions (TO/BY/FOR/WHILE/UNTIL), matching the original scoping. ✓
No new panics on malformed input; error paths in consume_unique_keyword propagate after the keyword is consumed, same as the original. ✓

## Code Review — PR #27 (parser.rs audit pass 1) The refactoring is sound: the six helpers (G1–G6) are semantically equivalent to the code they replace, all 500+ tests pass, and the structural improvements are real. One performance regression was introduced by G1, and two pre-existing correctness issues became more visible. --- ### Bug introduced — G1: per-clause `String` allocation in `parse_clause` **File:** `src/parser.rs`, line 167 ```rust match name.to_ascii_uppercase().as_str() { ``` `to_ascii_uppercase()` heap-allocates a new `String` on **every** `parse_clause` call, including all the common paths (assignments, command clauses, label definitions) where `name` is not a keyword at all. Before G1, the dispatch used `Self::check_keyword(name, kw)` which resolves to `eq_ignore_ascii_case` — a zero-allocation in-place comparison. This is a measurable regression for scripts with many clauses. The fix is to either store the uppercase form once before the match, or apply `eq_ignore_ascii_case` comparisons directly: ```rust // Option A: compute once, allocate once let upper = name.to_ascii_uppercase(); match upper.as_str() { ... } // Option B: keep zero-alloc comparisons, restructure as a helper // that returns a KeywordKind enum before calling the dispatch fn ``` Option A is the smallest mechanical fix and keeps the match readable. --- ### Pre-existing — `parse_do_body` silently accepts wrong END name **File:** `src/parser.rs`, lines 462–467 ```rust if self.is_keyword("END") { self.advance(); // Optionally consume a loop name after END (e.g., `END i`). if let TokenKind::Symbol(_) = self.peek_kind() && !self.is_terminator() { self.advance(); // consumes any symbol, not just the matching name } break; } ``` REXX spec §7.4.6: *"The name in an END instruction, if specified, must be the same as the name of the control variable in the corresponding DO instruction."* The code accepts `end j` for a `do i = 1 to 10` without error. The loop variable name is available via `DoBlock::name`; the fix is to thread it into `parse_do_body` and compare. Not introduced here, but worth tracking. --- ### Pre-existing — dead predicate in `parse_do_body` **File:** `src/parser.rs`, line 463 ```rust if let TokenKind::Symbol(_) = self.peek_kind() && !self.is_terminator() ``` `is_terminator()` returns true only for `Semicolon | Eol | Eof`. A `TokenKind::Symbol` can never be any of those, so `!self.is_terminator()` is always `true` when the outer pattern matches. The guard is dead and should be removed to avoid misleading future readers. Also not introduced here. --- ### Everything else looks correct - G2 `finish_do`: all six DO variants call it with the same `skip_terminators → parse_do_body` sequence as the original. ✓ - G3 `consume_unique_keyword`: duplicate-keyword detection is preserved; error messages are string-identical. ✓ - G4 `is_keyword("THEN"|"ELSE"|"WITH")`: identical to the deleted helpers since both call `check_keyword` → `eq_ignore_ascii_case`. ✓ - G6 `parse_optional_expression`: EXIT / RETURN / PUSH / QUEUE / NUMERIC DIGITS / FUZZ all behave identically. ✓ - `do_header_depth` is still active while `consume_unique_keyword` parses sub-expressions (TO/BY/FOR/WHILE/UNTIL), matching the original scoping. ✓ - No new panics on malformed input; error paths in `consume_unique_keyword` propagate after the keyword is consumed, same as the original. ✓

claude[bot] commented

2026-04-28 01:38:54 +00:00

(Migrated from github.com)

Code Review — Pass 1 Audit (PR #27)

This is a clean refactoring pass. The six changes (G1–G6) are mechanically correct, and I found no behavioral regressions in the expression parser or the DO/IF/SELECT parsers. Comments below are issues I'm confident about; the refactoring itself is sound.

1. `finish_do` receives `name` but doesn't pass it to `parse_do_body` — Error 21 never fires (CORRECTNESS)

src/parser.rs:401

fn finish_do(&mut self, loc: SourceLoc, kind: DoKind, name: Option<String>) -> RexxResult<Clause> {
    self.skip_terminators();
    let body = self.parse_do_body()?;   // ← name is dropped here
    Ok(Clause { kind: ClauseKind::Do(Box::new(DoBlock { kind, body, name })), loc })
}

src/parser.rs:539–546

if self.is_keyword("END") {
    self.advance();
    if let TokenKind::Symbol(_) = self.peek_kind() {
        self.advance();  // silently consumed, never compared
    }
    break;
}

Per ANSI X3.274-1996 §6.6.6, end j inside do i = 1 to 5 must raise Error 21 (InvalidDataOnEnd). The RexxError::InvalidDataOnEnd variant already exists (error.rs:68, error code 21 at error.rs:136). The design doc (docs/design/end-name-validation.md) describes the exact fix — thread name.as_deref() into parse_do_body — but it isn't applied in this PR.

The name parameter was carefully added to finish_do's signature, so the scaffolding is in place. The missing step is just wiring it through, and the design doc already shows the code. Risk: this can be inadvertently merged as-is since just ci is green and the gap is silent (no test currently catches it).

Missing tests (also called out in the design doc's checkpoints):

do i = 1 to 5; nop; end i — should parse OK (regression guard)
do i = 1 to 5; nop; end I — should parse OK (case-insensitive)
do i = 1 to 5; nop; end j — should fail with Error 21
do; nop; end x — should fail (Simple DO has no control variable)
do forever; leave; end x — should fail (Forever DO has no control variable)

2. `parse_select`'s END handler doesn't guard against a trailing Symbol (CORRECTNESS)

src/parser.rs:573–576

if self.is_keyword("END") {
    self.advance();
    break;  // no symbol-after-END handling
}

Per ANSI, SELECT's closing END takes no name (unlike DO). select; when 1 then say 'a'; end foo silently leaves foo in the token stream, which is then parsed as a stray command clause rather than raising Error 21. The fix is symmetric with parse_do_body: after advancing past END, if the next token is a Symbol, raise RexxError::InvalidDataOnEnd.

This was pre-existing but worth fixing in the same pass since parse_do_body's END handling was explicitly redesigned here.

3. Test coverage gaps for `consume_unique_keyword` error path

tests/phase2.rs:276,282 — duplicate TO and BY are covered. Duplicate FOR (do i = 1 for 3 for 2), duplicate WHILE, and duplicate UNTIL have no tests. Since consume_unique_keyword is the new helper that consolidates this logic, tests for all five keywords would fully exercise it.

Minor observation (not a blocker)

src/parser.rs:228 — self.peek_kind().clone() allocates a String for every parse_clause call (cloning the Symbol's inner String to bind name). The doc comment on KeywordKind::lookup says "zero-alloc" which is true for lookup itself but the call site still clones. Not introduced by this PR, but the comment is slightly misleading in context.

Summary

#	Location	Severity	Status
1	`parser.rs:401,539`	Bug — Error 21 never fires	Needs fix before merge
2	`parser.rs:573`	Bug — stray token after SELECT END	Pre-existing; fix alongside #1
3	`tests/phase2.rs`	Missing coverage for FOR/WHILE/UNTIL duplicates	Low risk

The G1–G6 refactoring is correct and clean. The main blocker is #1: finish_do's name parameter needs to be threaded into parse_do_body per the design doc before this lands.

## Code Review — Pass 1 Audit (PR #27) This is a clean refactoring pass. The six changes (G1–G6) are mechanically correct, and I found no behavioral regressions in the expression parser or the DO/IF/SELECT parsers. Comments below are issues I'm confident about; the refactoring itself is sound. --- ### 1. `finish_do` receives `name` but doesn't pass it to `parse_do_body` — Error 21 never fires (CORRECTNESS) **`src/parser.rs:401`** ```rust fn finish_do(&mut self, loc: SourceLoc, kind: DoKind, name: Option<String>) -> RexxResult<Clause> { self.skip_terminators(); let body = self.parse_do_body()?; // ← name is dropped here Ok(Clause { kind: ClauseKind::Do(Box::new(DoBlock { kind, body, name })), loc }) } ``` **`src/parser.rs:539–546`** ```rust if self.is_keyword("END") { self.advance(); if let TokenKind::Symbol(_) = self.peek_kind() { self.advance(); // silently consumed, never compared } break; } ``` Per ANSI X3.274-1996 §6.6.6, `end j` inside `do i = 1 to 5` must raise Error 21 (`InvalidDataOnEnd`). The `RexxError::InvalidDataOnEnd` variant already exists (`error.rs:68`, error code 21 at `error.rs:136`). The design doc (`docs/design/end-name-validation.md`) describes the exact fix — thread `name.as_deref()` into `parse_do_body` — but it isn't applied in this PR. The `name` parameter was carefully added to `finish_do`'s signature, so the scaffolding is in place. The missing step is just wiring it through, and the design doc already shows the code. Risk: this can be inadvertently merged as-is since `just ci` is green and the gap is silent (no test currently catches it). **Missing tests** (also called out in the design doc's checkpoints): - `do i = 1 to 5; nop; end i` — should parse OK (regression guard) - `do i = 1 to 5; nop; end I` — should parse OK (case-insensitive) - `do i = 1 to 5; nop; end j` — should fail with Error 21 - `do; nop; end x` — should fail (Simple DO has no control variable) - `do forever; leave; end x` — should fail (Forever DO has no control variable) --- ### 2. `parse_select`'s END handler doesn't guard against a trailing Symbol (CORRECTNESS) **`src/parser.rs:573–576`** ```rust if self.is_keyword("END") { self.advance(); break; // no symbol-after-END handling } ``` Per ANSI, SELECT's closing `END` takes no name (unlike DO). `select; when 1 then say 'a'; end foo` silently leaves `foo` in the token stream, which is then parsed as a stray command clause rather than raising Error 21. The fix is symmetric with `parse_do_body`: after advancing past END, if the next token is a Symbol, raise `RexxError::InvalidDataOnEnd`. This was pre-existing but worth fixing in the same pass since `parse_do_body`'s END handling was explicitly redesigned here. --- ### 3. Test coverage gaps for `consume_unique_keyword` error path **`tests/phase2.rs:276,282`** — duplicate TO and BY are covered. Duplicate FOR (`do i = 1 for 3 for 2`), duplicate WHILE, and duplicate UNTIL have no tests. Since `consume_unique_keyword` is the new helper that consolidates this logic, tests for all five keywords would fully exercise it. --- ### Minor observation (not a blocker) **`src/parser.rs:228`** — `self.peek_kind().clone()` allocates a `String` for every `parse_clause` call (cloning the Symbol's inner `String` to bind `name`). The doc comment on `KeywordKind::lookup` says "zero-alloc" which is true for `lookup` itself but the call site still clones. Not introduced by this PR, but the comment is slightly misleading in context. --- ### Summary | # | Location | Severity | Status | |---|----------|----------|--------| | 1 | `parser.rs:401,539` | Bug — Error 21 never fires | Needs fix before merge | | 2 | `parser.rs:573` | Bug — stray token after SELECT END | Pre-existing; fix alongside #1 | | 3 | `tests/phase2.rs` | Missing coverage for FOR/WHILE/UNTIL duplicates | Low risk | The G1–G6 refactoring is correct and clean. The main blocker is #1: `finish_do`'s `name` parameter needs to be threaded into `parse_do_body` per the design doc before this lands.

claude[bot] commented

2026-04-28 01:53:21 +00:00

(Migrated from github.com)

PR #27 Review — Pass 1 parser.rs audit

This is a clean, well-executed refactor. All six changes (G1–G6) preserve semantics faithfully. No regressions were found. The notes below are purely about gaps and one subtle pre-existing issue that became more visible through the refactor.

Correctness

No new bugs introduced. Each of the six refactors is a mechanical equivalence transform:

G1 (KeywordKind dispatch): KeywordKind::lookup uses eq_ignore_ascii_case, matching the old check_keyword calls exactly. The label (symbol + colon) and assignment (symbol + =) checks still come first, so then = 5, then:, etc. correctly reach those branches before the keyword table is consulted. REXX's "no reserved words" rule is preserved for assignment and label positions.
G6 (removed !self.is_terminator() guard in parse_do_body): The claim that this was dead code is correct. is_terminator returns true only for Semicolon | Eol | Eof. The guard if let TokenKind::Symbol(_) = self.peek_kind() && !self.is_terminator() — since Symbol is never any of those three variants, the !is_terminator() sub-expression was always true when the outer pattern matched. Removal is safe.
finish_do / consume_unique_keyword / parse_optional_expression: All are behaviorally identical to the inlined code they replace.

Test Coverage (missing, not introduced by this PR)

src/parser.rs unit test gaps created by the refactor becoming more visible:

consume_unique_keyword error path for FOR, WHILE, UNTIL (lines 501–508): phase2.rs has duplicate_to_keyword_error and duplicate_by_keyword_error, but no tests for do i = 1 for 3 for 3 (duplicate FOR), do i = 1 while x while x (duplicate WHILE), or do i = 1 until x until x (duplicate UNTIL). These paths now flow through the same helper as TO/BY, so a single helper test would cover all five, but the FOR/WHILE/UNTIL branches remain untested at the unit level.
DO WHILE and DO UNTIL parser unit tests (src/parser.rs): parse_do already has unit tests for Simple (line 1811), Count (line 1822), Controlled (line 1833), and Forever (line 1850), but While and Until variants have no unit tests in parser.rs. Integration-level coverage exists in the phase tests, but adding two #[test] entries alongside the existing DO tests would fill the gap cheaply.
END name matching — unit tests deferred with the feature (parse_do_body, lines 540–547): The design doc (docs/design/end-name-validation.md) lists 8 test cases that should accompany the eventual implementation. Since the validation itself is deferred, the tests are too — but it's worth ensuring those 8 cases land together with the implementation in the follow-on PR, not after.

Pre-existing issues (not introduced by this PR, flagged for completeness)

do_header_depth not decremented on early error exit (src/parser.rs, lines 484–513):

self.do_header_depth += 1;
let start = self.parse_expression()?;  // ← if this errors, depth leaks
// ...
if self.consume_unique_keyword("TO", &mut to)? { ... }  // ← same
// ...
self.do_header_depth -= 1;

Same for if_depth (lines 366–381) and condition_depth (lines 350–352 in parse_select). In practice this is benign — Parser is single-use per parse() call (INTERPRET creates a fresh Parser::new()), so leaked counters are discarded with the parser on error. But if Parser were ever reused or the depth counters were checked_sub'd, this would surface. Worth a comment or a drop-guard pattern if the parser ever becomes multi-invocable.

end used as a variable name inside DO body (parse_do_body, line 540): end = 5 inside a DO block hits is_keyword("END") and is misread as the block terminator. This is unchanged from before the PR and is a known limitation of context-free is_keyword checks.

Summary

Category	Finding	Severity	New in PR?
Correctness	Dead `!is_terminator()` guard removal	Safe (dead code)	Yes
Correctness	KeywordKind dispatch equivalence	Correct	Yes
Test coverage	Duplicate `FOR`/`WHILE`/`UNTIL` not unit-tested	Low	Pre-existing
Test coverage	DO WHILE / DO UNTIL no parser unit tests	Low	Pre-existing
Test coverage	END-name validation tests deferred with feature	Low	Pre-existing
Safety	Depth counter leak on error (benign, single-use parser)	Informational	Pre-existing
Correctness	`end = 5` inside DO body mis-parsed	Pre-existing bug	Pre-existing

The refactor achieves its stated goals cleanly. The three helpers and the KeywordKind table are solid additions. Suggest adding the two missing DO WHILE/UNTIL parser unit tests in src/parser.rs before or alongside the follow-on END-name validation PR.

## PR #27 Review — Pass 1 parser.rs audit This is a clean, well-executed refactor. All six changes (G1–G6) preserve semantics faithfully. No regressions were found. The notes below are purely about gaps and one subtle pre-existing issue that became more visible through the refactor. --- ### Correctness **No new bugs introduced.** Each of the six refactors is a mechanical equivalence transform: - **G1 (`KeywordKind` dispatch):** `KeywordKind::lookup` uses `eq_ignore_ascii_case`, matching the old `check_keyword` calls exactly. The label (`symbol + colon`) and assignment (`symbol + =`) checks still come first, so `then = 5`, `then:`, etc. correctly reach those branches before the keyword table is consulted. REXX's "no reserved words" rule is preserved for assignment and label positions. - **G6 (removed `!self.is_terminator()` guard in `parse_do_body`):** The claim that this was dead code is correct. `is_terminator` returns `true` only for `Semicolon | Eol | Eof`. The guard `if let TokenKind::Symbol(_) = self.peek_kind() && !self.is_terminator()` — since `Symbol` is never any of those three variants, the `!is_terminator()` sub-expression was always `true` when the outer pattern matched. Removal is safe. - **`finish_do` / `consume_unique_keyword` / `parse_optional_expression`:** All are behaviorally identical to the inlined code they replace. --- ### Test Coverage (missing, not introduced by this PR) **`src/parser.rs` unit test gaps created by the refactor becoming more visible:** 1. **`consume_unique_keyword` error path for `FOR`, `WHILE`, `UNTIL`** (lines 501–508): `phase2.rs` has `duplicate_to_keyword_error` and `duplicate_by_keyword_error`, but no tests for `do i = 1 for 3 for 3` (duplicate `FOR`), `do i = 1 while x while x` (duplicate `WHILE`), or `do i = 1 until x until x` (duplicate `UNTIL`). These paths now flow through the same helper as `TO`/`BY`, so a single helper test would cover all five, but the `FOR`/`WHILE`/`UNTIL` branches remain untested at the unit level. 2. **DO WHILE and DO UNTIL parser unit tests** (`src/parser.rs`): `parse_do` already has unit tests for `Simple` (line 1811), `Count` (line 1822), `Controlled` (line 1833), and `Forever` (line 1850), but `While` and `Until` variants have no unit tests in `parser.rs`. Integration-level coverage exists in the phase tests, but adding two `#[test]` entries alongside the existing DO tests would fill the gap cheaply. 3. **END name matching — unit tests deferred with the feature** (`parse_do_body`, lines 540–547): The design doc (`docs/design/end-name-validation.md`) lists 8 test cases that should accompany the eventual implementation. Since the validation itself is deferred, the tests are too — but it's worth ensuring those 8 cases land together with the implementation in the follow-on PR, not after. --- ### Pre-existing issues (not introduced by this PR, flagged for completeness) **`do_header_depth` not decremented on early error exit** (`src/parser.rs`, lines 484–513): ```rust self.do_header_depth += 1; let start = self.parse_expression()?; // ← if this errors, depth leaks // ... if self.consume_unique_keyword("TO", &mut to)? { ... } // ← same // ... self.do_header_depth -= 1; ``` Same for `if_depth` (lines 366–381) and `condition_depth` (lines 350–352 in `parse_select`). In practice this is benign — `Parser` is single-use per `parse()` call (INTERPRET creates a fresh `Parser::new()`), so leaked counters are discarded with the parser on error. But if `Parser` were ever reused or the depth counters were `checked_sub`'d, this would surface. Worth a comment or a drop-guard pattern if the parser ever becomes multi-invocable. **`end` used as a variable name inside DO body** (`parse_do_body`, line 540): `end = 5` inside a DO block hits `is_keyword("END")` and is misread as the block terminator. This is unchanged from before the PR and is a known limitation of context-free `is_keyword` checks. --- ### Summary | Category | Finding | Severity | New in PR? | |---|---|---|---| | Correctness | Dead `!is_terminator()` guard removal | Safe (dead code) | Yes | | Correctness | KeywordKind dispatch equivalence | Correct | Yes | | Test coverage | Duplicate `FOR`/`WHILE`/`UNTIL` not unit-tested | Low | Pre-existing | | Test coverage | DO WHILE / DO UNTIL no parser unit tests | Low | Pre-existing | | Test coverage | END-name validation tests deferred with feature | Low | Pre-existing | | Safety | Depth counter leak on error (benign, single-use parser) | Informational | Pre-existing | | Correctness | `end = 5` inside DO body mis-parsed | Pre-existing bug | Pre-existing | The refactor achieves its stated goals cleanly. The three helpers and the `KeywordKind` table are solid additions. Suggest adding the two missing DO WHILE/UNTIL parser unit tests in `src/parser.rs` before or alongside the follow-on END-name validation PR.