- parse_do_body(loop_name: Option<&str>) — now validates that an op… #28

Merged

navicore merged 2 commits from end into main

2026-04-28 02:07:40 +00:00

navicore commented

2026-04-28 01:55:48 +00:00

(Migrated from github.com)

…tional

END name matches the DO's control variable (case-insensitive). Mismatched
name on a controlled DO, or any name on a non-controlled DO (Simple /
Forever / Count / While / Until), raises RexxError::InvalidDataOnEnd (ANSI
Error 21). finish_do threads name.as_deref() through.

parse_select — now rejects a trailing symbol after END with the same Error 21. SELECT's END takes no name per ANSI.
Tests — 8 DO checkpoints from the design doc (4 positive, 4 negative), plus 2 SELECT checkpoints (1 positive bare-end, 1 negative trailing-name), plus 3 new duplicate-keyword cases (FOR / WHILE / UNTIL) in tests/phase2.rs to fully exercise consume_unique_keyword.
No public API change. No existing test or example used END name form, so nothing latent surfaced.

…tional END name matches the DO's control variable (case-insensitive). Mismatched name on a controlled DO, or any name on a non-controlled DO (Simple / Forever / Count / While / Until), raises RexxError::InvalidDataOnEnd (ANSI Error 21). finish_do threads name.as_deref() through. - parse_select — now rejects a trailing symbol after END with the same Error 21. SELECT's END takes no name per ANSI. - Tests — 8 DO checkpoints from the design doc (4 positive, 4 negative), plus 2 SELECT checkpoints (1 positive bare-end, 1 negative trailing-name), plus 3 new duplicate-keyword cases (FOR / WHILE / UNTIL) in tests/phase2.rs to fully exercise consume_unique_keyword. - No public API change. No existing test or example used END name form, so nothing latent surfaced.

claude[bot] commented

2026-04-28 02:02:26 +00:00

(Migrated from github.com)

Code Review — END name validation (PR #28)

The core logic is correct: loop_name is threaded through finish_do → parse_do_body via name.as_deref(), the eq_ignore_ascii_case comparison is right, and the borrow split (end_symbol: Option<String> before calling self.advance()) is necessary and clean. RexxError::InvalidDataOnEnd => 21 is confirmed correct in error.rs. No correctness bugs found in the implementation itself.

Three gaps worth addressing:

1. No integration test verifying "Error 21" appears in stderr

tests/phase2.rs

Every other parse-error test in this file uses the run_rexx_fail → stderr.contains("Error N") pattern, e.g.:

fn duplicate_by_keyword_error() {
    let stderr = run_rexx_fail("do i = 1 for 3 for 2; say i; end");
    assert!(stderr.contains("Error 27"));
}

The new END name tests live only in the parser.rs unit-test module. They verify err.error == RexxError::InvalidDataOnEnd (the Rust enum variant) but not what the interpreter actually prints. If the display impl for InvalidDataOnEnd regresses — wrong number, wrong format — it would be invisible. Suggested additions to phase2.rs:

#[test]
fn end_name_mismatch_error_number() {
    let stderr = run_rexx_fail("do i = 1 to 5; nop; end j");
    assert!(stderr.contains("Error 21"));
}

#[test]
fn end_name_on_simple_do_error_number() {
    let stderr = run_rexx_fail("do; nop; end x");
    assert!(stderr.contains("Error 21"));
}

#[test]
fn end_name_on_select_error_number() {
    let stderr = run_rexx_fail("select; when 1 then nop; end x");
    assert!(stderr.contains("Error 21"));
}

2. Missing test: nested controlled DOs with END names

src/parser.rs (test module)

No test covers nested loops where both ENDs carry names. The recursive call chain is:

finish_do(name=Some("i")) → parse_do_body(loop_name=Some("i"))
  └─ finish_do(name=Some("j")) → parse_do_body(loop_name=Some("j"))

A regression that passed the outer name down instead of the inner one would silently pass the current suite. Suggested tests:

#[test]
fn end_name_nested_both_match() {
    // Each END name matches its own DO's control variable.
    assert!(parse_result(
        "do i = 1 to 5; do j = 1 to 3; nop; end j; end i"
    ).is_ok());
}

#[test]
fn end_name_inner_mismatch_in_nested_do() {
    // Inner END has the outer name — must fail.
    let err = parse_result(
        "do i = 1 to 5; do j = 1 to 3; nop; end i; end i"
    ).unwrap_err();
    assert_eq!(err.error, RexxError::InvalidDataOnEnd);
}

3. Missing test: controlled DO with WHILE/UNTIL guard + END name

src/parser.rs (test module)

bare_end_on_while_do_ok tests do while 1 (no control variable, name = None). There is no test for a controlled DO that also has a WHILE or UNTIL guard — a case where name = Some("i") but the loop kind includes a guard:

#[test]
fn end_name_matches_controlled_do_with_while_guard() {
    // Control variable present despite WHILE — END i is valid.
    assert!(parse_result(
        "do i = 1 to 10 while 1; leave; end i"
    ).is_ok());
}

#[test]
fn end_name_mismatch_on_controlled_do_with_until_guard() {
    let err = parse_result(
        "do i = 1 to 10 until 0; nop; end j"
    ).unwrap_err();
    assert_eq!(err.error, RexxError::InvalidDataOnEnd);
}

This exercises the path where finish_do is called with name = Some("i") for a loop that also has a WHILE/UNTIL clause, confirming the name is correctly threaded through regardless of the loop's kind.

No other issues found. The implementation is sound — these are test-coverage gaps only.

## Code Review — END name validation (PR #28) The core logic is correct: `loop_name` is threaded through `finish_do` → `parse_do_body` via `name.as_deref()`, the `eq_ignore_ascii_case` comparison is right, and the borrow split (`end_symbol: Option<String>` before calling `self.advance()`) is necessary and clean. `RexxError::InvalidDataOnEnd => 21` is confirmed correct in `error.rs`. No correctness bugs found in the implementation itself. Three gaps worth addressing: --- ### 1. No integration test verifying \"Error 21\" appears in stderr **`tests/phase2.rs`** Every other parse-error test in this file uses the `run_rexx_fail` → `stderr.contains("Error N")` pattern, e.g.: ```rust fn duplicate_by_keyword_error() { let stderr = run_rexx_fail("do i = 1 for 3 for 2; say i; end"); assert!(stderr.contains("Error 27")); } ``` The new END name tests live only in the `parser.rs` unit-test module. They verify `err.error == RexxError::InvalidDataOnEnd` (the Rust enum variant) but not what the interpreter actually prints. If the display impl for `InvalidDataOnEnd` regresses — wrong number, wrong format — it would be invisible. Suggested additions to `phase2.rs`: ```rust #[test] fn end_name_mismatch_error_number() { let stderr = run_rexx_fail("do i = 1 to 5; nop; end j"); assert!(stderr.contains("Error 21")); } #[test] fn end_name_on_simple_do_error_number() { let stderr = run_rexx_fail("do; nop; end x"); assert!(stderr.contains("Error 21")); } #[test] fn end_name_on_select_error_number() { let stderr = run_rexx_fail("select; when 1 then nop; end x"); assert!(stderr.contains("Error 21")); } ``` --- ### 2. Missing test: nested controlled DOs with END names **`src/parser.rs` (test module)** No test covers nested loops where both ENDs carry names. The recursive call chain is: ``` finish_do(name=Some("i")) → parse_do_body(loop_name=Some("i")) └─ finish_do(name=Some("j")) → parse_do_body(loop_name=Some("j")) ``` A regression that passed the *outer* name down instead of the inner one would silently pass the current suite. Suggested tests: ```rust #[test] fn end_name_nested_both_match() { // Each END name matches its own DO's control variable. assert!(parse_result( "do i = 1 to 5; do j = 1 to 3; nop; end j; end i" ).is_ok()); } #[test] fn end_name_inner_mismatch_in_nested_do() { // Inner END has the outer name — must fail. let err = parse_result( "do i = 1 to 5; do j = 1 to 3; nop; end i; end i" ).unwrap_err(); assert_eq!(err.error, RexxError::InvalidDataOnEnd); } ``` --- ### 3. Missing test: controlled DO with WHILE/UNTIL guard + END name **`src/parser.rs` (test module)** `bare_end_on_while_do_ok` tests `do while 1` (no control variable, `name = None`). There is no test for a controlled DO that *also* has a WHILE or UNTIL guard — a case where `name = Some("i")` but the loop kind includes a guard: ```rust #[test] fn end_name_matches_controlled_do_with_while_guard() { // Control variable present despite WHILE — END i is valid. assert!(parse_result( "do i = 1 to 10 while 1; leave; end i" ).is_ok()); } #[test] fn end_name_mismatch_on_controlled_do_with_until_guard() { let err = parse_result( "do i = 1 to 10 until 0; nop; end j" ).unwrap_err(); assert_eq!(err.error, RexxError::InvalidDataOnEnd); } ``` This exercises the path where `finish_do` is called with `name = Some("i")` for a loop that also has a WHILE/UNTIL clause, confirming the name is correctly threaded through regardless of the loop's `kind`. --- No other issues found. The implementation is sound — these are test-coverage gaps only.

claude[bot] commented

2026-04-28 02:12:37 +00:00

(Migrated from github.com)

Code Review — PR #28: END-name validation

Summary: The implementation is correct. Error 21 is the right ANSI number, the case-insensitive match (eq_ignore_ascii_case) is properly applied, nested-DO recursion handles each END [name] at the right stack frame, and LEAVE/ITERATE name comparison is sound by construction (both try_consume_symbol_name at parser.rs:698 and parse_controlled_do at parser.rs:478 uppercase their symbols before storing, so signal_matches's ln == name at eval.rs:1663 always compares two uppercase strings).

Issue 1 — Missing negative test coverage for `DO WHILE` / `DO UNTIL` + END name (test gap, not a code bug)

src/parser.rs:531–577 — The None arm of the match correctly fires Error 21 when a name follows END on a WHILE or UNTIL DO (both arrive via finish_do(..., None) so loop_name is always None). However the PR's eight DO checkpoints omit the two negative cases for these variants:

do while 1; leave; end x   /* should be Error 21 */
do until 0; leave; end x   /* should be Error 21 */

There's a bare_end_on_while_do_ok positive test but no corresponding negative, and no UNTIL tests at all. If someone later changes parse_do to pass a name for these variants the logic would silently invert with no regression signal. The integration suite in tests/phase2.rs also has no coverage for these two paths.

Suggested additions (unit tests in src/parser.rs, mirroring the existing pattern):

#[test]
fn end_name_on_while_do_errors() {
    let err = parse_result("do while 1; leave; end x").unwrap_err();
    assert_eq!(err.error, RexxError::InvalidDataOnEnd);
}

#[test]
fn end_name_on_until_do_errors() {
    let err = parse_result("do until 0; leave; end x").unwrap_err();
    assert_eq!(err.error, RexxError::InvalidDataOnEnd);
}

And two matching run_rexx_fail checks in tests/phase2.rs asserting "Error 21".

Issue 2 — `peek_kind()` borrow lifetime exposes a latent inconsistency between DO body and SELECT

src/parser.rs:544–548 vs src/parser.rs:600–601 — The DO-body path extracts the symbol name via a clone into end_symbol: Option<String> before the if let Some(s) = end_symbol block, releasing the peek_kind() borrow in time for the subsequent self.loc() and self.advance() calls. The SELECT path clones inside the if let block (let s = s.clone()), also correct.

Both patterns compile correctly today, but the DO-body version has an extra heap allocation for the Option<String> that's immediately matched away. This is not a bug, just a note that the two adjacent code paths use different idioms for the same pattern.

Verified correct

Error number: InvalidDataOnEnd → 21 (error.rs:136) matches ANSI X3.274-1996 §A.
REXX semantics: controlled-DO name stored uppercase at parser.rs:478; eq_ignore_ascii_case at parser.rs:552 handles END I matching do i = 1 to 5 and vice-versa.
SELECT: END takes no name per spec §6.6.5; the check at parser.rs:599–605 is correct and consistent with the DO check.
Nested DOs: parse_clause() recursion means each inner DO's parse_do_body(inner_name) consumes its own END [name] before returning, so outer contexts see a clean token stream.
No panics: all error paths return Err(RexxDiagnostic::new(...)). The unreachable! at parser.rs:476 is guarded by the caller's TokenKind::Symbol(_) check at line 444.
Duplicate-keyword tests (duplicate_for_keyword_error, _while_, _until_): the consume_unique_keyword slot check triggers correctly for all three newly covered keywords.

## Code Review — PR #28: END-name validation **Summary**: The implementation is correct. Error 21 is the right ANSI number, the case-insensitive match (`eq_ignore_ascii_case`) is properly applied, nested-DO recursion handles each `END [name]` at the right stack frame, and LEAVE/ITERATE name comparison is sound by construction (both `try_consume_symbol_name` at `parser.rs:698` and `parse_controlled_do` at `parser.rs:478` uppercase their symbols before storing, so `signal_matches`'s `ln == name` at `eval.rs:1663` always compares two uppercase strings). --- ### Issue 1 — Missing negative test coverage for `DO WHILE` / `DO UNTIL` + END name (test gap, not a code bug) `src/parser.rs:531–577` — The `None` arm of the match correctly fires Error 21 when a name follows END on a WHILE or UNTIL DO (both arrive via `finish_do(..., None)` so `loop_name` is always `None`). However the PR's eight DO checkpoints omit the two negative cases for these variants: ```rexx do while 1; leave; end x /* should be Error 21 */ do until 0; leave; end x /* should be Error 21 */ ``` There's a `bare_end_on_while_do_ok` positive test but no corresponding negative, and no UNTIL tests at all. If someone later changes `parse_do` to pass a name for these variants the logic would silently invert with no regression signal. The integration suite in `tests/phase2.rs` also has no coverage for these two paths. **Suggested additions** (unit tests in `src/parser.rs`, mirroring the existing pattern): ```rust #[test] fn end_name_on_while_do_errors() { let err = parse_result("do while 1; leave; end x").unwrap_err(); assert_eq!(err.error, RexxError::InvalidDataOnEnd); } #[test] fn end_name_on_until_do_errors() { let err = parse_result("do until 0; leave; end x").unwrap_err(); assert_eq!(err.error, RexxError::InvalidDataOnEnd); } ``` And two matching `run_rexx_fail` checks in `tests/phase2.rs` asserting `"Error 21"`. --- ### Issue 2 — `peek_kind()` borrow lifetime exposes a latent inconsistency between DO body and SELECT `src/parser.rs:544–548` vs `src/parser.rs:600–601` — The DO-body path extracts the symbol name via a clone into `end_symbol: Option<String>` *before* the `if let Some(s) = end_symbol` block, releasing the `peek_kind()` borrow in time for the subsequent `self.loc()` and `self.advance()` calls. The SELECT path clones inside the `if let` block (`let s = s.clone()`), also correct. Both patterns compile correctly today, but the DO-body version has an extra heap allocation for the `Option<String>` that's immediately matched away. This is not a bug, just a note that the two adjacent code paths use different idioms for the same pattern. --- ### Verified correct - **Error number**: `InvalidDataOnEnd` → 21 (`error.rs:136`) matches ANSI X3.274-1996 §A. - **REXX semantics**: controlled-DO name stored uppercase at `parser.rs:478`; `eq_ignore_ascii_case` at `parser.rs:552` handles `END I` matching `do i = 1 to 5` and vice-versa. - **SELECT**: END takes no name per spec §6.6.5; the check at `parser.rs:599–605` is correct and consistent with the DO check. - **Nested DOs**: `parse_clause()` recursion means each inner DO's `parse_do_body(inner_name)` consumes its own `END [name]` before returning, so outer contexts see a clean token stream. - **No panics**: all error paths return `Err(RexxDiagnostic::new(...))`. The `unreachable!` at `parser.rs:476` is guarded by the caller's `TokenKind::Symbol(_)` check at line 444. - **Duplicate-keyword tests** (`duplicate_for_keyword_error`, `_while_`, `_until_`): the `consume_unique_keyword` slot check triggers correctly for all three newly covered keywords.