⏺ Phase 12B Complete #16

Merged

navicore merged 2 commits from phase12b into main 2026-02-11 13:38:48 +00:00

Conversation 2 Commits 2 Files changed 4 +906 -6

navicore commented 2026-02-11 13:03:19 +00:00 (Migrated from github.com)

Copy link

Summary

Created: tests/conformance.rs — 75 ANSI X3.274-1996 conformance tests organized by spec section:

• Section 6 (Lexical): 10 tests — hex/binary strings, comments, quoting, empty programs
• Section 7 (Clauses & Instructions): 10 tests — semicolons, NOP, compound variables, abuttal, stem defaults
• Section 8 (Expressions & Operators): 20 tests — power assoc, comparisons, strict ops, logical ops, negative div/remainder, precision
• Section 9 (PARSE): 8 tests — comma args, backward reposition, relative zero, patterns, PARSE VALUE
• Section 10 (Conditions & SIGNAL): 7 tests — computed SIGNAL VALUE, trap re-enable, CONDITION BIF, NOVALUE, nested DO exit
• Section 11 (BIF Edge Cases): 20 tests — SUBSTR padding, LEFT/RIGHT, TRANSLATE pad, DATATYPE, FORMAT, COMPARE, VERIFY, STRIP

Bugs Found and Fixed

1. Parser: compound variable lookups (src/parser.rs:1632-1644) — parse_primary() now creates Expr::Compound for symbols containing ., matching the existing assignment-side handling. Previously arr.3.5 was treated as a simple symbol, returning its name instead of the stored value.
2. Evaluator: stem default assignment (src/eval.rs:360-366) — Assignment to arr. (empty tail) now calls set_stem_default() instead of set_compound() with an empty tail. This fixes arr.='default'; say arr.99.
3. Test update (tests/phase9.rs:354-361) — Updated trace_compound_via_symbol to expect >C> tag (correct for compound variable) instead of >V> (simple variable).

Verification

• 534 tests pass (87 unit + 75 conformance + 372 integration)
• Clippy clean in both default and --features lsp modes
• Zero regressions

Summary Created: tests/conformance.rs — 75 ANSI X3.274-1996 conformance tests organized by spec section: - Section 6 (Lexical): 10 tests — hex/binary strings, comments, quoting, empty programs - Section 7 (Clauses & Instructions): 10 tests — semicolons, NOP, compound variables, abuttal, stem defaults - Section 8 (Expressions & Operators): 20 tests — power assoc, comparisons, strict ops, logical ops, negative div/remainder, precision - Section 9 (PARSE): 8 tests — comma args, backward reposition, relative zero, patterns, PARSE VALUE - Section 10 (Conditions & SIGNAL): 7 tests — computed SIGNAL VALUE, trap re-enable, CONDITION BIF, NOVALUE, nested DO exit - Section 11 (BIF Edge Cases): 20 tests — SUBSTR padding, LEFT/RIGHT, TRANSLATE pad, DATATYPE, FORMAT, COMPARE, VERIFY, STRIP Bugs Found and Fixed 1. Parser: compound variable lookups (src/parser.rs:1632-1644) — parse_primary() now creates Expr::Compound for symbols containing ., matching the existing assignment-side handling. Previously arr.3.5 was treated as a simple symbol, returning its name instead of the stored value. 2. Evaluator: stem default assignment (src/eval.rs:360-366) — Assignment to arr. (empty tail) now calls set_stem_default() instead of set_compound() with an empty tail. This fixes arr.='default'; say arr.99. 3. Test update (tests/phase9.rs:354-361) — Updated trace_compound_via_symbol to expect >C> tag (correct for compound variable) instead of >V> (simple variable). Verification - 534 tests pass (87 unit + 75 conformance + 372 integration) - Clippy clean in both default and --features lsp modes - Zero regressions

claude[bot] commented 2026-02-11 13:12:13 +00:00 (Migrated from github.com)

Copy link

PR #16 Review: Phase 12B - ANSI X3.274-1996 Conformance Tests

Reviewed all changes with focus on correctness, REXX semantics, arithmetic, and error handling.

Critical Issues Found

1. ❌ CRITICAL: Power operator is right-associative, should be left-associative

Location: src/parser.rs:1575-1588

The parser implements ** as right-associative (via right-recursion), but ANSI X3.274-1996 REXX specifies left-associative evaluation for all operators including power.

// Current (WRONG):
fn parse_power(&mut self) -> RexxResult<Expr> {
    let base = self.parse_unary()?;
    if matches!(self.peek_kind(), TokenKind::Power) {
        self.advance();
        let exp = self.parse_power()?; // right-recursive = right-assoc
        // ...
    }
}

Impact:

• 2 ** 3 ** 2 evaluates to 512 (2^9) but should be 64 ((2^3)^2)
• Test sec8_power_right_assoc_execution (line 239) expects 512 but should expect 64
• Parser unit test parse_power_right_assoc() (line 1760) also needs updating

Fix: Change to left-associative loop like parse_multiplication():

fn parse_power(&mut self) -> RexxResult<Expr> {
    let mut left = self.parse_unary()?;
    while matches!(self.peek_kind(), TokenKind::Power) {
        self.advance();
        let right = self.parse_unary()?;
        left = Expr::BinOp {
            left: Box::new(left),
            op: BinOp::Power,
            right: Box::new(right),
        };
    }
    Ok(left)
}

---

2. ❌ CRITICAL: Numeric-starting compound tail elements treated as variables

Location: src/parser.rs:1691-1702 (parse_tail_elements)

// Current (WRONG):
if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) {
    TailElement::Const(part.to_uppercase())
} else {
    TailElement::Var(part.to_uppercase())  // ❌ Wrong for "3abc"
}

REXX Semantics: Any tail element starting with a digit is a constant, even if it contains letters.

Impact:

• arr.3abc would incorrectly try to look up variable 3ABC instead of using constant "3ABC"
• arr.123xyz would fail similarly

Fix:

if part.is_empty() 
    || part.chars().all(|c| c.is_ascii_digit())
    || part.chars().next().is_some_and(|c| c.is_ascii_digit()) {
    TailElement::Const(part.to_uppercase())
} else {
    TailElement::Var(part.to_uppercase())
}

---

3. ⚠️ MODERATE: Empty stem not validated

Location: src/parser.rs:1639-1644

The parser accepts symbols like .foo (if tokenized as a single symbol) and creates Expr::Compound { stem: "", tail: [Var("FOO")] }.

REXX Semantics: A stem must have at least one character before the dot.

Impact: Could lead to invalid compound variables with empty stems, causing undefined behavior in the environment.

Recommendation: Add validation:

if name.contains('.') {
    let parts: Vec<&str> = name.splitn(2, '.').collect();
    if parts[0].is_empty() {
        return Err(RexxDiagnostic::new(RexxError::InvalidExpression)
            .at(self.loc())
            .with_detail("compound variable stem cannot be empty"));
    }
    // ...
}

---

Correct Implementations ✅

Assignment to stem default (arr.)

Location: src/eval.rs:360-366

Correctly distinguishes between stem default assignment and compound assignment:

if resolved_tail.is_empty() {
    self.env.set_stem_default(stem, val);
} else {
    self.env.set_compound(stem, &resolved_tail, val);
}

✅ Test sec7_stem_default_assignment (line 268) validates this correctly.

Integer division (%)

Location: src/eval.rs (via trunc_div)

Correctly implements truncation toward zero using RoundingMode::Down.

• ✅ -7 % 2 = -3 (test line 437)

Remainder (//)

Location: src/eval.rs:1871-1886

Correctly implements REXX remainder: a // b = a - (a % b) * b

• ✅ -7 // 2 = -1 (test line 447)

Division precision

Correctly implements NUMERIC DIGITS with half-up rounding.

• ✅ 2/3 with NUMERIC DIGITS 5 = 0.66667 (test line 456)

FORMAT overflow detection

Location: src/builtins.rs:962-971

Correctly errors when integer part exceeds requested width.

• ✅ format(123,2,0) correctly errors (test line 883)

TRANSLATE with pad char

Location: src/builtins.rs:534-577

Correctly pads output when output table is shorter than input table.

• ✅ translate('abcde','12','abcde') = "12 " (test line 765)
• ✅ translate('abcde','12','abcde','*') = "12***" (test line 774)

---

Test Coverage Assessment

The new conformance test suite is comprehensive and well-organized:

• 75 ANSI X3.274-1996 tests across 6 sections
• Good coverage of edge cases (empty strings, backwards PARSE, negative arithmetic)
• Proper use of stdout() vs stdout_raw() for whitespace-sensitive tests
• Clear test names indicating spec section and behavior

Only issue: Two tests have wrong expectations due to the power operator bug (#1 above).

---

Other Observations

Updated trace test (tests/phase9.rs:354-361)

✅ Correctly changed expected trace tag from >V> to >C> for compound variable access, matching the fix that compounds are now properly parsed as Expr::Compound.

No safety issues

• No panics on malformed input
• No unbounded allocations
• Proper error handling throughout

No performance concerns

• splitn(2, '.') appropriately stops after first split
• No unnecessary clones in hot paths
• String operations are linear

---

Summary

Must fix before merge:

1. Power operator associativity (parser + 2 tests)
2. Numeric-starting tail elements (parser)

Should fix:
3. Empty stem validation

Zero regressions confirmed - the fixes are targeted and test coverage is excellent.

Reviewed with reference to ANSI X3.274-1996 REXX specification.

# PR #16 Review: Phase 12B - ANSI X3.274-1996 Conformance Tests Reviewed all changes with focus on correctness, REXX semantics, arithmetic, and error handling. ## Critical Issues Found ### 1. ❌ CRITICAL: Power operator is right-associative, should be left-associative **Location:** `src/parser.rs:1575-1588` The parser implements `**` as right-associative (via right-recursion), but **ANSI X3.274-1996 REXX specifies left-associative evaluation** for all operators including power. ```rust // Current (WRONG): fn parse_power(&mut self) -> RexxResult<Expr> { let base = self.parse_unary()?; if matches!(self.peek_kind(), TokenKind::Power) { self.advance(); let exp = self.parse_power()?; // right-recursive = right-assoc // ... } } ``` **Impact:** - `2 ** 3 ** 2` evaluates to `512` (2^9) but should be `64` ((2^3)^2) - Test `sec8_power_right_assoc_execution` (line 239) expects `512` but should expect `64` - Parser unit test `parse_power_right_assoc()` (line 1760) also needs updating **Fix:** Change to left-associative loop like `parse_multiplication()`: ```rust fn parse_power(&mut self) -> RexxResult<Expr> { let mut left = self.parse_unary()?; while matches!(self.peek_kind(), TokenKind::Power) { self.advance(); let right = self.parse_unary()?; left = Expr::BinOp { left: Box::new(left), op: BinOp::Power, right: Box::new(right), }; } Ok(left) } ``` --- ### 2. ❌ CRITICAL: Numeric-starting compound tail elements treated as variables **Location:** `src/parser.rs:1691-1702` (`parse_tail_elements`) ```rust // Current (WRONG): if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) { TailElement::Const(part.to_uppercase()) } else { TailElement::Var(part.to_uppercase()) // ❌ Wrong for "3abc" } ``` **REXX Semantics:** Any tail element **starting with a digit** is a constant, even if it contains letters. **Impact:** - `arr.3abc` would incorrectly try to look up variable `3ABC` instead of using constant `"3ABC"` - `arr.123xyz` would fail similarly **Fix:** ```rust if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) || part.chars().next().is_some_and(|c| c.is_ascii_digit()) { TailElement::Const(part.to_uppercase()) } else { TailElement::Var(part.to_uppercase()) } ``` --- ### 3. ⚠️ MODERATE: Empty stem not validated **Location:** `src/parser.rs:1639-1644` The parser accepts symbols like `.foo` (if tokenized as a single symbol) and creates `Expr::Compound { stem: "", tail: [Var("FOO")] }`. **REXX Semantics:** A stem must have at least one character before the dot. **Impact:** Could lead to invalid compound variables with empty stems, causing undefined behavior in the environment. **Recommendation:** Add validation: ```rust if name.contains('.') { let parts: Vec<&str> = name.splitn(2, '.').collect(); if parts[0].is_empty() { return Err(RexxDiagnostic::new(RexxError::InvalidExpression) .at(self.loc()) .with_detail("compound variable stem cannot be empty")); } // ... } ``` --- ## Correct Implementations ✅ ### Assignment to stem default (`arr.`) **Location:** `src/eval.rs:360-366` Correctly distinguishes between stem default assignment and compound assignment: ```rust if resolved_tail.is_empty() { self.env.set_stem_default(stem, val); } else { self.env.set_compound(stem, &resolved_tail, val); } ``` ✅ Test `sec7_stem_default_assignment` (line 268) validates this correctly. ### Integer division (`%`) **Location:** `src/eval.rs` (via `trunc_div`) Correctly implements truncation toward zero using `RoundingMode::Down`. - ✅ `-7 % 2` = `-3` (test line 437) ### Remainder (`//`) **Location:** `src/eval.rs:1871-1886` Correctly implements REXX remainder: `a // b = a - (a % b) * b` - ✅ `-7 // 2` = `-1` (test line 447) ### Division precision Correctly implements `NUMERIC DIGITS` with half-up rounding. - ✅ `2/3` with `NUMERIC DIGITS 5` = `0.66667` (test line 456) ### FORMAT overflow detection **Location:** `src/builtins.rs:962-971` Correctly errors when integer part exceeds requested width. - ✅ `format(123,2,0)` correctly errors (test line 883) ### TRANSLATE with pad char **Location:** `src/builtins.rs:534-577` Correctly pads output when output table is shorter than input table. - ✅ `translate('abcde','12','abcde')` = `"12 "` (test line 765) - ✅ `translate('abcde','12','abcde','*')` = `"12***"` (test line 774) --- ## Test Coverage Assessment The new conformance test suite is comprehensive and well-organized: - 75 ANSI X3.274-1996 tests across 6 sections - Good coverage of edge cases (empty strings, backwards PARSE, negative arithmetic) - Proper use of `stdout()` vs `stdout_raw()` for whitespace-sensitive tests - Clear test names indicating spec section and behavior **Only issue:** Two tests have wrong expectations due to the power operator bug (#1 above). --- ## Other Observations ### Updated trace test (tests/phase9.rs:354-361) ✅ Correctly changed expected trace tag from `>V>` to `>C>` for compound variable access, matching the fix that compounds are now properly parsed as `Expr::Compound`. ### No safety issues - No panics on malformed input - No unbounded allocations - Proper error handling throughout ### No performance concerns - `splitn(2, '.')` appropriately stops after first split - No unnecessary clones in hot paths - String operations are linear --- ## Summary **Must fix before merge:** 1. Power operator associativity (parser + 2 tests) 2. Numeric-starting tail elements (parser) **Should fix:** 3. Empty stem validation **Zero regressions confirmed** - the fixes are targeted and test coverage is excellent. Reviewed with reference to ANSI X3.274-1996 REXX specification.

claude[bot] commented 2026-02-11 13:33:51 +00:00 (Migrated from github.com)

Copy link

Code Review — Phase 12B

Reviewed for correctness, REXX semantics, arithmetic, error handling, performance, and safety. Overall the PR looks solid with comprehensive test coverage. Found one critical issue with compound variable tail parsing.

---

❌ Critical Issue: Incorrect tail element classification

Location: src/parser.rs:1700

Problem: The change from .chars().all(|c| c.is_ascii_digit()) to .starts_with(|c: char| c.is_ascii_digit()) is incorrect.

// BEFORE (correct):
if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) {
    TailElement::Const(part.to_uppercase())
} else {
    TailElement::Var(part.to_uppercase())
}

// AFTER (incorrect):
if part.is_empty() || part.starts_with(|c: char| c.is_ascii_digit()) {
    TailElement::Const(part.to_uppercase())
} else {
    TailElement::Var(part.to_uppercase())
}

Impact: This breaks the REXX spec. According to ANSI X3.274-1996, tail elements that are purely numeric should be constants, while alphanumeric symbols should be variables.

• arr.3 → tail "3" should be Const (✓ works before and after)
• arr.3a → tail "3a" should be Var (✓ works before, ❌ broken after — now classified as Const)
• arr.a3 → tail "a3" should be Var (✓ works both)

Example failure:

i = 3
arr.3a = 'STORED_VALUE'
say arr.i  /* Should look up arr.3, get "ARR.3" (unset) */
           /* But now looks up arr.3a incorrectly if i='3a' */

Why it matters: When i="3a", with the new logic arr.i will incorrectly resolve to the constant tail "3A" instead of treating it as a variable that would need substitution.

Fix: Revert line 1700 to:

if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) {

The original logic was correct. The change appears to have been made as part of the compound variable fix but wasn't necessary and introduces a bug.

---

✅ Correct Changes

1. Compound variable parsing in expressions (src/parser.rs:1638-1649)

• Correctly adds Expr::Compound for symbols containing . in expression position
• Matches existing assignment-side handling
• Empty stem check is appropriate
• Fixes the bug where arr.3.5 returned "ARR.3.5" instead of stored value

2. Stem default assignment (src/eval.rs:362-366)

• Correctly distinguishes arr.="value" (stem default) from arr.key="value" (compound)
• Proper use of set_stem_default() vs set_compound()
• Fixes the bug where stem defaults weren't working

3. Test update (tests/phase9.rs:354-357)

• Correctly updates trace expectation from >V> (variable) to >C> (compound)
• Reflects the parser change accurately

---

✅ Test Coverage

The 75 new conformance tests are well-organized and comprehensive:

• Lexical: Good coverage of hex/binary strings, comments, quoting
• Expressions: Excellent operator precedence, comparison, and arithmetic tests
• PARSE: Good edge cases for patterns, repositioning, comma args
• Conditions: Good SIGNAL and trap handling tests
• BIFs: Thorough edge case coverage for SUBSTR, LEFT, RIGHT, TRANSLATE, etc.

All tests pass (534 total), which is excellent.

---

Minor Observations (not blockers)

1. Performance note (src/parser.rs:1640): splitn(2, '.') is efficient — ✓
2. No panic paths: Empty stem check at line 1642 prevents panics — ✓
3. Error handling: Proper RexxError::InvalidExpression usage — ✓
4. REXX semantics: All changes respect case-insensitive keywords, uppercase normalization — ✓

---

Recommendation

Do not merge until the parse_tail_elements logic is fixed. The .starts_with() change must be reverted to .chars().all() to maintain spec compliance.

After that fix:

• All 534 tests should still pass
• Compound variables will work correctly for all tail element patterns
• Stem defaults will work as expected

Great work on the comprehensive test suite and the bug fixes! Just need to address the tail element classification issue.

---

Reviewed: src/{parser.rs, eval.rs}, tests/{conformance.rs, phase9.rs}
Compiler: ✓ clean
Tests: ✓ 534 passing
Spec compliance: ⚠️ needs tail element fix

## Code Review — Phase 12B Reviewed for correctness, REXX semantics, arithmetic, error handling, performance, and safety. Overall the PR looks solid with comprehensive test coverage. Found **one critical issue** with compound variable tail parsing. --- ### ❌ Critical Issue: Incorrect tail element classification **Location:** src/parser.rs:1700 **Problem:** The change from `.chars().all(|c| c.is_ascii_digit())` to `.starts_with(|c: char| c.is_ascii_digit())` is incorrect. ```rust // BEFORE (correct): if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) { TailElement::Const(part.to_uppercase()) } else { TailElement::Var(part.to_uppercase()) } // AFTER (incorrect): if part.is_empty() || part.starts_with(|c: char| c.is_ascii_digit()) { TailElement::Const(part.to_uppercase()) } else { TailElement::Var(part.to_uppercase()) } ``` **Impact:** This breaks the REXX spec. According to ANSI X3.274-1996, tail elements that are purely numeric should be constants, while alphanumeric symbols should be variables. - `arr.3` → tail `"3"` should be **Const** (✓ works before and after) - `arr.3a` → tail `"3a"` should be **Var** (✓ works before, ❌ broken after — now classified as Const) - `arr.a3` → tail `"a3"` should be **Var** (✓ works both) **Example failure:** ```rexx i = 3 arr.3a = 'STORED_VALUE' say arr.i /* Should look up arr.3, get "ARR.3" (unset) */ /* But now looks up arr.3a incorrectly if i='3a' */ ``` **Why it matters:** When `i="3a"`, with the new logic `arr.i` will incorrectly resolve to the constant tail `"3A"` instead of treating it as a variable that would need substitution. **Fix:** Revert line 1700 to: ```rust if part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) { ``` The original logic was correct. The change appears to have been made as part of the compound variable fix but wasn't necessary and introduces a bug. --- ### ✅ Correct Changes **1. Compound variable parsing in expressions (src/parser.rs:1638-1649)** - Correctly adds Expr::Compound for symbols containing `.` in expression position - Matches existing assignment-side handling - Empty stem check is appropriate - Fixes the bug where `arr.3.5` returned "ARR.3.5" instead of stored value **2. Stem default assignment (src/eval.rs:362-366)** - Correctly distinguishes `arr.="value"` (stem default) from `arr.key="value"` (compound) - Proper use of `set_stem_default()` vs `set_compound()` - Fixes the bug where stem defaults weren't working **3. Test update (tests/phase9.rs:354-357)** - Correctly updates trace expectation from `>V>` (variable) to `>C>` (compound) - Reflects the parser change accurately --- ### ✅ Test Coverage The 75 new conformance tests are well-organized and comprehensive: - **Lexical:** Good coverage of hex/binary strings, comments, quoting - **Expressions:** Excellent operator precedence, comparison, and arithmetic tests - **PARSE:** Good edge cases for patterns, repositioning, comma args - **Conditions:** Good SIGNAL and trap handling tests - **BIFs:** Thorough edge case coverage for SUBSTR, LEFT, RIGHT, TRANSLATE, etc. All tests pass (534 total), which is excellent. --- ### Minor Observations (not blockers) 1. **Performance note (src/parser.rs:1640):** `splitn(2, '.')` is efficient — ✓ 2. **No panic paths:** Empty stem check at line 1642 prevents panics — ✓ 3. **Error handling:** Proper RexxError::InvalidExpression usage — ✓ 4. **REXX semantics:** All changes respect case-insensitive keywords, uppercase normalization — ✓ --- ### Recommendation **Do not merge** until the `parse_tail_elements` logic is fixed. The `.starts_with()` change must be reverted to `.chars().all()` to maintain spec compliance. After that fix: - All 534 tests should still pass - Compound variables will work correctly for all tail element patterns - Stem defaults will work as expected Great work on the comprehensive test suite and the bug fixes\! Just need to address the tail element classification issue. --- Reviewed: src/{parser.rs, eval.rs}, tests/{conformance.rs, phase9.rs} Compiler: ✓ clean Tests: ✓ 534 passing Spec compliance: ⚠️ needs tail element fix

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

dependencies

Pull requests that update a dependency file

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

rust

Pull requests that update rust code

  

wontfix

This will not be worked on

No labels

bug

dependencies

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

rust

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-rexx!16

No description provided.