navicore/patch-rexx

Fork 0

⏺ Phase 1 is complete. Here's a summary of what was implemented: #1

Merged

navicore merged 4 commits from phase1 into main

2026-02-08 13:55:59 +00:00

navicore commented

2026-02-08 06:19:54 +00:00

(Migrated from github.com)

Files modified

src/lexer.rs — Added space_before: bool to Token struct; tokenize() now tracks whether whitespace preceded each token

Files created

src/parser.rs — Recursive descent parser (530 lines) with:
- Clause disambiguation: labels (name:), assignments (name =), SAY instruction, NOP, and command clauses
- Full expression precedence: logical OR/XOR → AND → comparison → concatenation → add/sub → mul/div → power (right-assoc) → unary → primary
- Function call detection via name( (no space) vs name ( (concat)
- Abuttal vs blank-concat via space_before
- 14 unit tests
src/eval.rs — Tree-walking evaluator (340 lines) with:
- All arithmetic ops (+ - * / % // **) using BigDecimal
- String concatenation (abuttal, ||, blank-concat)
- Normal and strict comparison operators
- Logical operators (& | && )
- Variable assignment/lookup with REXX semantics (unset → uppercase name)
- Division by zero → Error 42, non-numeric → Error 41
- 21 unit tests
src/main.rs — Wired lexer → parser → evaluator pipeline; REPL now persists Environment across lines
tests/phase1.rs — 8 integration tests running the binary with -e

Verification

cargo build ✓
cargo test ✓ (67 tests pass)
cargo clippy ✓ (zero warnings)
say 2 + 3 → 5
say 2 + 3 * 4 → 14
x = 10; say x + 5 → 15
say 'hello' 'world' → hello world

Files modified - src/lexer.rs — Added space_before: bool to Token struct; tokenize() now tracks whether whitespace preceded each token Files created - src/parser.rs — Recursive descent parser (530 lines) with: - Clause disambiguation: labels (name:), assignments (name =), SAY instruction, NOP, and command clauses - Full expression precedence: logical OR/XOR → AND → comparison → concatenation → add/sub → mul/div → power (right-assoc) → unary → primary - Function call detection via name( (no space) vs name ( (concat) - Abuttal vs blank-concat via space_before - 14 unit tests - src/eval.rs — Tree-walking evaluator (340 lines) with: - All arithmetic ops (+ - * / % // **) using BigDecimal - String concatenation (abuttal, ||, blank-concat) - Normal and strict comparison operators - Logical operators (& | && \) - Variable assignment/lookup with REXX semantics (unset → uppercase name) - Division by zero → Error 42, non-numeric → Error 41 - 21 unit tests - src/main.rs — Wired lexer → parser → evaluator pipeline; REPL now persists Environment across lines - tests/phase1.rs — 8 integration tests running the binary with -e Verification cargo build ✓ cargo test ✓ (67 tests pass) cargo clippy ✓ (zero warnings) say 2 + 3 → 5 say 2 + 3 * 4 → 14 x = 10; say x + 5 → 15 say 'hello' 'world' → hello world

navicore commented

2026-02-08 06:35:57 +00:00

(Migrated from github.com)

@claude where is our PR review?

👀 1

claude[bot] commented

2026-02-08 06:36:09 +00:00

(Migrated from github.com)

Claude finished @navicore's task in 2m 0s —— View job

Code Review - Phase 1 Implementation

I've completed a comprehensive review of the Phase 1 implementation. This is solid work with good architectural decisions and clean code. Here are my findings:

Todo List

Check for repository guidelines (CLAUDE.md)
Review lexer changes (src/lexer.rs)
Review parser implementation (src/parser.rs)
Review evaluator implementation (src/eval.rs)
Review main.rs changes
Review integration tests (tests/phase1.rs)
Provide final summary and recommendations

Overall Assessment

Excellent work. The Phase 1 implementation is well-architected, follows REXX semantics correctly, and has good test coverage. The code is clean, idiomatic Rust, and the architecture sets up well for future phases.

Strengths

Architecture

Clean separation of concerns: Lexer → Parser → Evaluator pipeline is well-defined
Persistent REPL environment (src/main.rs:88): The REPL correctly maintains an Environment across lines, which is essential for REXX semantics
Space tracking for abuttal (src/lexer.rs:68): The space_before flag elegantly solves the distinction between name( (function call) and name ( (concatenation with parentheses)
Right-associative power operator (src/parser.rs:398): Correctly implemented via right recursion

REXX Semantics

Normal comparison (src/eval.rs:315-329): Proper implementation of REXX's comparison rules (numeric if both are numbers, otherwise string comparison with padding)
Unset variables return their name (src/env.rs:56): Correct REXX behavior
BigDecimal arithmetic: Appropriate choice for arbitrary-precision decimal math
Error codes match ANSI spec: Error 41 (non-numeric), Error 42 (arithmetic overflow)

Code Quality

Good test coverage: 67 tests passing (14 parser, 21 evaluator, 8 integration)
No clippy warnings: Clean code that follows Rust best practices
Clear documentation: Module-level docs explain design decisions

Issues Found

Critical Issues

None found. The implementation is sound.

Moderate Issues

1. Precedence ambiguity in `is_binary_op` (src/parser.rs:321-348)

Location: src/parser.rs:321-348

Issue: The is_binary_op function is used to prevent implicit concatenation when the next token is a real binary operator. However, TokenKind::Plus and TokenKind::Minus appear in this list, but these can also start unary expressions (handled in can_start_term). This creates ambiguity.

Example:

say 'x' + 5    -- Should be: concat('x', ' '), then +5 or 'x' + 5?

The current implementation would correctly parse this as 'x' + 5 (addition), but the logic should be clearer about the intent.

Recommendation: Add a comment explaining that + and - as binary operators take precedence over unary operators in the concatenation lookahead. This is correct behavior but could confuse future maintainers.

2. Hex/Binary string conversion uses `char` instead of bytes (src/lexer.rs:486-513)

Location: src/lexer.rs:496, 511

Issue: The hex and binary string decoders convert u8 bytes directly to char via byte as char. This only works correctly for ASCII values (0-127). For bytes 128-255, this creates invalid Unicode characters.

Example:

'FF'x  -- Should produce byte 255, but creates char U+00FF (ÿ)

While this happens to work for Latin-1 characters, it's semantically incorrect for REXX, which treats hex/binary strings as byte sequences, not Unicode.

Recommendation: Consider using a byte buffer representation internally, or document that the current implementation assumes Latin-1 encoding.

3. Integer division implementation may have precision issues (src/eval.rs:171-176)

Location: src/eval.rs:171-176

Issue: The integer division (%) implementation converts BigDecimal to string, splits on ., then parses as BigInt. This will fail if the number is in exponential notation (e.g., 1E10).

Example:

say 1E10 % 3  -- May fail or give wrong result

Recommendation: Use BigDecimal::round(0) to convert to integer, then extract the BigInt representation. Or use to_bigint() if available in your version of bigdecimal.

Minor Issues

4. Incomplete numeric formatting (src/value.rs:132-147)

Location: src/value.rs:132-147

Issue: The format_rexx_number function has a TODO comment and doesn't implement full REXX NUMERIC DIGITS truncation or ENGINEERING form. The form parameter is explicitly ignored.

Impact: Low for Phase 1 (works for basic cases), but will need to be addressed before Phase 5 (built-in functions) or when users set NUMERIC FORM.

Recommendation: Track this as a Phase 5 task. Add a test that expects the current behavior so you'll know when to update it.

5. Parser uses `unreachable!()` without a panic message (src/parser.rs:158)

Location: src/parser.rs:158

Issue: In parse_assignment, there's an unreachable!() without a message. While this is provably unreachable due to the prior if let, it's better practice to include a message.

Recommendation:

unreachable!("parse_assignment called on non-symbol token")

6. Test helper `eval_and_capture` returns empty string (src/eval.rs:362-372)

Location: src/eval.rs:362-372

Issue: The eval_and_capture helper is documented to "capture what was printed to stdout" but always returns an empty string because capturing println! output isn't trivial in Rust.

Impact: Currently unused (only called in one test), but misleading.

Recommendation: Either remove this helper and use the pattern from other tests, or implement actual stdout capturing using a solution like gag crate or manual file descriptor redirection.

Code Style & Consistency

7. Inconsistent error detail messages

Some error messages use full sentences with periods (src/eval.rs:151 "division by zero"), while others don't (src/lexer.rs:372 "unexpected character 'x'").

Recommendation: Pick one style and apply consistently. I suggest no periods for short error details, periods for longer explanations.

Architecture Recommendations

For Future Phases

Consider adding source locations to AST nodes: Currently only Clause has a SourceLoc, but expressions don't. When you implement error messages for runtime errors in expressions (e.g., "division by zero at line 5"), you'll want the location of the / operator.
Plan for INTERPRET: The current architecture is perfect for INTERPRET (just call run_line with the string). Make sure future phases don't introduce mutable state that would make INTERPRET complicated.
Function call resolution: Currently function calls return a "routine not found" error (src/eval.rs:94). Phase 3 will need a function registry. Consider the lookup order: built-in functions vs. internal labels vs. external .rexx files.

Test Coverage Observations

What's Well-Tested

Basic arithmetic precedence ✓
String concatenation (abuttal, blank, explicit) ✓
Power operator right-associativity ✓
Division by zero error handling ✓
Variable assignment and lookup ✓
Unset variable behavior ✓

Test Gaps

No tests for:
- Compound variables (stem.tail) — AST supports them, but no tests
- Multiple forms of division (/, %, //) in combination
- Logical operator short-circuiting (though REXX doesn't short-circuit, good to test)
- Comparison operator chaining (is 3 > 2 > 1 an error or does it evaluate?)
- Hex/binary string literals with the X and B suffixes

Recommendation: Add tests for compound variables before Phase 2, since they're already partially implemented.

Documentation

The code is generally well-documented with clear module-level comments. A few suggestions:

Add examples to public API docs: Functions like Environment::get and Parser::parse would benefit from usage examples in their doc comments.
Document REXX-specific design decisions: Comments like "REXX remainder: a - (a%b)*b" (src/eval.rs:185) are excellent. More of these would help future maintainers understand why the code does things a particular way.

Performance Notes

For future optimization (not needed now):

String allocations in concatenation (src/eval.rs:217, 221): Every concat creates a new String via format!. For long chains, this could be optimized with a rope data structure or a concat-specific AST node that flattens at evaluation time.
BigDecimal operations: Currently creating new BigDecimals for every arithmetic operation. Consider reusing or implementing in-place operations where possible.

Not relevant for Phase 1, but keep in mind for the performance pass in Phase 12.

Final Verdict

Approve with minor follow-up items.

This is a solid Phase 1 implementation that correctly implements the core REXX expression evaluation and SAY instruction. The architecture is clean, the code is idiomatic Rust, and the tests give good confidence in correctness.

Recommended Follow-up Actions

Fix integer division for exponential notation (issue #3 above) — only if you want to support 1E10 % 3 in Phase 1
Add compound variable tests — the code is already there, just needs tests
Document hex/binary string encoding assumption — clarify that 'FF'x produces Latin-1, not raw bytes

Everything else can wait for later phases. The implementation is production-ready for Phase 1 scope.

Excellent work on Phase 1! The foundation is solid and sets up well for Phase 2 (control flow).

**Claude finished @navicore's task in 2m 0s** —— [View job](https://github.com/navicore/patch-rexx/actions/runs/21793752725) --- ### Code Review - Phase 1 Implementation I've completed a comprehensive review of the Phase 1 implementation. This is solid work with good architectural decisions and clean code. Here are my findings: #### Todo List - [x] Check for repository guidelines (CLAUDE.md) - [x] Review lexer changes (src/lexer.rs) - [x] Review parser implementation (src/parser.rs) - [x] Review evaluator implementation (src/eval.rs) - [x] Review main.rs changes - [x] Review integration tests (tests/phase1.rs) - [x] Provide final summary and recommendations --- ## Overall Assessment **Excellent work.** The Phase 1 implementation is well-architected, follows REXX semantics correctly, and has good test coverage. The code is clean, idiomatic Rust, and the architecture sets up well for future phases. ## Strengths ### Architecture - **Clean separation of concerns**: Lexer → Parser → Evaluator pipeline is well-defined - **Persistent REPL environment** (src/main.rs:88): The REPL correctly maintains an `Environment` across lines, which is essential for REXX semantics - **Space tracking for abuttal** (src/lexer.rs:68): The `space_before` flag elegantly solves the distinction between `name(` (function call) and `name (` (concatenation with parentheses) - **Right-associative power operator** (src/parser.rs:398): Correctly implemented via right recursion ### REXX Semantics - **Normal comparison** (src/eval.rs:315-329): Proper implementation of REXX's comparison rules (numeric if both are numbers, otherwise string comparison with padding) - **Unset variables return their name** (src/env.rs:56): Correct REXX behavior - **BigDecimal arithmetic**: Appropriate choice for arbitrary-precision decimal math - **Error codes match ANSI spec**: Error 41 (non-numeric), Error 42 (arithmetic overflow) ### Code Quality - **Good test coverage**: 67 tests passing (14 parser, 21 evaluator, 8 integration) - **No clippy warnings**: Clean code that follows Rust best practices - **Clear documentation**: Module-level docs explain design decisions --- ## Issues Found ### Critical Issues None found. The implementation is sound. ### Moderate Issues #### 1. **Precedence ambiguity in `is_binary_op`** (src/parser.rs:321-348) **Location**: src/parser.rs:321-348 **Issue**: The `is_binary_op` function is used to prevent implicit concatenation when the next token is a real binary operator. However, `TokenKind::Plus` and `TokenKind::Minus` appear in this list, but these can also start unary expressions (handled in `can_start_term`). This creates ambiguity. **Example**: ```rexx say 'x' + 5 -- Should be: concat('x', ' '), then +5 or 'x' + 5? ``` The current implementation would correctly parse this as `'x' + 5` (addition), but the logic should be clearer about the intent. **Recommendation**: Add a comment explaining that `+` and `-` as binary operators take precedence over unary operators in the concatenation lookahead. This is correct behavior but could confuse future maintainers. #### 2. **Hex/Binary string conversion uses `char` instead of bytes** (src/lexer.rs:486-513) **Location**: src/lexer.rs:496, 511 **Issue**: The hex and binary string decoders convert `u8` bytes directly to `char` via `byte as char`. This only works correctly for ASCII values (0-127). For bytes 128-255, this creates invalid Unicode characters. **Example**: ```rexx 'FF'x -- Should produce byte 255, but creates char U+00FF (ÿ) ``` While this happens to work for Latin-1 characters, it's semantically incorrect for REXX, which treats hex/binary strings as byte sequences, not Unicode. **Recommendation**: Consider using a byte buffer representation internally, or document that the current implementation assumes Latin-1 encoding. #### 3. **Integer division implementation may have precision issues** (src/eval.rs:171-176) **Location**: src/eval.rs:171-176 **Issue**: The integer division (`%`) implementation converts `BigDecimal` to string, splits on `.`, then parses as `BigInt`. This will fail if the number is in exponential notation (e.g., `1E10`). **Example**: ```rexx say 1E10 % 3 -- May fail or give wrong result ``` **Recommendation**: Use `BigDecimal::round(0)` to convert to integer, then extract the `BigInt` representation. Or use `to_bigint()` if available in your version of `bigdecimal`. --- ### Minor Issues #### 4. **Incomplete numeric formatting** (src/value.rs:132-147) **Location**: src/value.rs:132-147 **Issue**: The `format_rexx_number` function has a TODO comment and doesn't implement full REXX NUMERIC DIGITS truncation or ENGINEERING form. The `form` parameter is explicitly ignored. **Impact**: Low for Phase 1 (works for basic cases), but will need to be addressed before Phase 5 (built-in functions) or when users set NUMERIC FORM. **Recommendation**: Track this as a Phase 5 task. Add a test that expects the current behavior so you'll know when to update it. #### 5. **Parser uses `unreachable!()` without a panic message** (src/parser.rs:158) **Location**: src/parser.rs:158 **Issue**: In `parse_assignment`, there's an `unreachable!()` without a message. While this is provably unreachable due to the prior `if let`, it's better practice to include a message. **Recommendation**: ```rust unreachable!("parse_assignment called on non-symbol token") ``` #### 6. **Test helper `eval_and_capture` returns empty string** (src/eval.rs:362-372) **Location**: src/eval.rs:362-372 **Issue**: The `eval_and_capture` helper is documented to "capture what was printed to stdout" but always returns an empty string because capturing `println!` output isn't trivial in Rust. **Impact**: Currently unused (only called in one test), but misleading. **Recommendation**: Either remove this helper and use the pattern from other tests, or implement actual stdout capturing using a solution like `gag` crate or manual file descriptor redirection. --- ### Code Style & Consistency #### 7. **Inconsistent error detail messages** Some error messages use full sentences with periods (src/eval.rs:151 "division by zero"), while others don't (src/lexer.rs:372 "unexpected character 'x'"). **Recommendation**: Pick one style and apply consistently. I suggest no periods for short error details, periods for longer explanations. --- ## Architecture Recommendations ### For Future Phases 1. **Consider adding source locations to AST nodes**: Currently only `Clause` has a `SourceLoc`, but expressions don't. When you implement error messages for runtime errors in expressions (e.g., "division by zero at line 5"), you'll want the location of the `/` operator. 2. **Plan for INTERPRET**: The current architecture is perfect for INTERPRET (just call `run_line` with the string). Make sure future phases don't introduce mutable state that would make INTERPRET complicated. 3. **Function call resolution**: Currently function calls return a "routine not found" error (src/eval.rs:94). Phase 3 will need a function registry. Consider the lookup order: built-in functions vs. internal labels vs. external .rexx files. --- ## Test Coverage Observations ### What's Well-Tested - Basic arithmetic precedence ✓ - String concatenation (abuttal, blank, explicit) ✓ - Power operator right-associativity ✓ - Division by zero error handling ✓ - Variable assignment and lookup ✓ - Unset variable behavior ✓ ### Test Gaps - No tests for: - Compound variables (stem.tail) — AST supports them, but no tests - Multiple forms of division (`/`, `%`, `//`) in combination - Logical operator short-circuiting (though REXX doesn't short-circuit, good to test) - Comparison operator chaining (is `3 > 2 > 1` an error or does it evaluate?) - Hex/binary string literals with the `X` and `B` suffixes **Recommendation**: Add tests for compound variables before Phase 2, since they're already partially implemented. --- ## Documentation The code is generally well-documented with clear module-level comments. A few suggestions: 1. **Add examples to public API docs**: Functions like `Environment::get` and `Parser::parse` would benefit from usage examples in their doc comments. 2. **Document REXX-specific design decisions**: Comments like "REXX remainder: a - (a%b)*b" (src/eval.rs:185) are excellent. More of these would help future maintainers understand why the code does things a particular way. --- ## Performance Notes For future optimization (not needed now): 1. **String allocations in concatenation** (src/eval.rs:217, 221): Every concat creates a new String via `format!`. For long chains, this could be optimized with a rope data structure or a concat-specific AST node that flattens at evaluation time. 2. **BigDecimal operations**: Currently creating new `BigDecimal`s for every arithmetic operation. Consider reusing or implementing in-place operations where possible. Not relevant for Phase 1, but keep in mind for the performance pass in Phase 12. --- ## Final Verdict **Approve with minor follow-up items.** This is a solid Phase 1 implementation that correctly implements the core REXX expression evaluation and SAY instruction. The architecture is clean, the code is idiomatic Rust, and the tests give good confidence in correctness. ### Recommended Follow-up Actions 1. **Fix integer division** for exponential notation (issue #3 above) — only if you want to support `1E10 % 3` in Phase 1 2. **Add compound variable tests** — the code is already there, just needs tests 3. **Document hex/binary string encoding assumption** — clarify that `'FF'x` produces Latin-1, not raw bytes Everything else can wait for later phases. The implementation is production-ready for Phase 1 scope. --- **Excellent work on Phase 1!** The foundation is solid and sets up well for Phase 2 (control flow). ---

claude[bot] commented

2026-02-08 13:22:58 +00:00

(Migrated from github.com)

Code Review: Phase 1 Parser & Evaluator

I've reviewed the implementation focusing on REXX semantics, correctness, arithmetic handling, and error management. Overall, this is a solid foundation with proper error numbering and good test coverage. Here are the issues I found:

Critical Issues

1. Integer Division Semantics — src/eval.rs:171

Location: eval.rs:171-173

The implementation uses .round(0) for integer division, but REXX's % operator requires truncation toward zero, not rounding.

Current code:

let ai = a.round(0);
let bi = b.round(0);
let result = (ai / bi).round(0);

Issue: For -7 % 2, REXX expects -3 (truncate toward zero), but .round(0) would give -4 (round to nearest).

Fix: Use .with_scale(0) or implement proper truncation:

let ai = a.with_scale(0);  // truncates toward zero
let bi = b.with_scale(0);
let result = (ai / bi).with_scale(0);

2. Remainder Calculation Matches IntDiv Bug — src/eval.rs:188

Location: eval.rs:187-189

The remainder operation depends on integer division. Since IntDiv uses .round(0), this will produce incorrect results for negative operands.

Formula: REXX remainder is a - (a%b)*b

This is correctly implemented, but it will give wrong answers when a%b is wrong.

Fix: After fixing integer division, ensure this uses the corrected truncation.

3. Power Operation: Division by Zero for 0^n where n<0 — src/eval.rs:338-340

Location: eval.rs:338-340

The code handles negative exponents via 1 / pow_bigdecimal(base, -exp).

Issue: When base = 0 and exp < 0, this performs 1 / 0^|exp| = 1 / 0, which should raise Error 42 (ArithmeticOverflow) but instead returns infinity or panics.

Missing check:

if exp < 0 {
    if base.is_zero() {
        return Err(RexxDiagnostic::new(RexxError::ArithmeticOverflow)
            .with_detail("0 cannot be raised to a negative power"));
    }
    let pos_result = pow_bigdecimal(base, -exp);
    return BigDecimal::from(1) / pos_result;
}

Moderate Issues

4. NUMERIC DIGITS Not Applied in Arithmetic — src/value.rs:137

Location: value.rs:137 and eval.rs:280-287

The format_rexx_number function uses .round(i64::from(digits)), which rounds to a number of decimal places, not significant digits.

Example: With NUMERIC DIGITS 5:

123.456 should become 123.46 (5 significant digits)
Current code: .round(5) → 123.45600 (5 decimal places) ❌

REXX spec: NUMERIC DIGITS controls total significant digits, not decimal places.

Impact: All arithmetic results will have incorrect precision. This doesn't break current tests (which use default DIGITS=9), but violates REXX semantics.

TODO comment exists (line 133-135) acknowledging this, but the placeholder is incorrect.

5. Missing Handling of Compound Variables in Parser — src/parser.rs:451-457

Location: parser.rs:451-457 (parse_primary)

When parsing a Symbol, the code checks for function calls but never produces Expr::Compound.

Current: Compound variables like arr.i.j are treated as simple symbols with dots in the name.

Expected: These should parse to Expr::Compound { stem, tail } during expression parsing, not just in assignments.

Result: Reading compound variables in expressions will fail or produce incorrect behavior:

arr.1 = "hello"
say arr.1    /* Should work but may fail */

The parser only handles compounds in assignments (line 163), not in symbol expressions.

6. Comparison Operators Don't Check for Non-Chaining — src/parser.rs:237-264

Location: parser.rs:237-264

The comparison parsing uses a loop, allowing chains like a = b = c, which REXX does not support (comparison operators are non-associative).

Issue: say 1 = 1 = 1 would parse as (1 = 1) = 1 → 1 = 1 → 1, instead of raising a syntax error.

REXX behavior: Comparison operators cannot be chained. Only one comparison per precedence level.

Fix: Replace the loop with a single optional comparison:

fn parse_comparison(&mut self) -> RexxResult<Expr> {
    let mut left = self.parse_concat()?;
    if let Some(op) = self.match_comparison_op() {
        self.advance();
        let right = self.parse_concat()?;
        left = Expr::BinOp { left: Box::new(left), op, right: Box::new(right) };
    }
    Ok(left)
}

Minor Issues

7. Tail Element Classification in Compound Variables — src/parser.rs:506

Location: parse_tail_elements (line 506)

The function treats parts that are all digits as Const, but empty parts should also be constants.

Issue: The condition part.is_empty() || part.chars().all(|c| c.is_ascii_digit()) is correct, but the comment/documentation doesn't clarify that empty parts arise from arr..i (two consecutive dots).

Not a bug, but edge case handling should be tested.

8. Normal Comparison Padding — src/eval.rs:327-330

Location: eval.rs:327-330

The padding uses format!("{ls:<max_len$}"), allocating new strings even when lengths are equal.

Optimization: Check lengths first:

match ls.len().cmp(&rs.len()) {
    std::cmp::Ordering::Equal => ls.cmp(rs),
    std::cmp::Ordering::Less => {
        let lp = format!("{ls:<max_len$}");
        lp.cmp(rs)
    }
    std::cmp::Ordering::Greater => {
        let rp = format!("{rs:<max_len$}");
        ls.cmp(&rp)
    }
}

Impact: Minor performance issue, not a correctness bug.

9. Exponentiation by Squaring: Potential Stack Overflow — src/eval.rs:334-353

Location: pow_bigdecimal function

The algorithm is iterative (good), but for extremely large exponents, the loop could run for a very long time or the intermediate b * b squaring could overflow available precision.

Not a safety issue (no recursion), but consider clamping exponents to a reasonable range (e.g., |exp| < 999999999) per REXX spec limits.

Test Coverage Gaps

Missing test cases:

Negative operands in % and //: Need -7 % 2, -7 // 2, 7 % -2
Zero to negative power: say 0 ** -1 should error
Chained comparisons: say 1 = 1 = 1 should error (if fixed)
Compound variable reads: arr.1 = 5; say arr.1
NUMERIC DIGITS edge cases: Numbers with more digits than DIGITS setting
Whitespace in numbers: say " 3 " + " 4 " (should work per REXX)

Summary

Correctness issues to fix before merge:

Integer division truncation (critical)
Remainder depends on #1 (critical)
0 ** -n division by zero (critical)
Compound variable parsing in expressions (moderate, likely already failing tests)
NUMERIC DIGITS significance vs decimal places (moderate, spec violation)
Comparison operator chaining (moderate, spec violation)

Non-blocking issues:

String comparison padding optimization (#8)
Test coverage gaps

Good things:
✅ Error numbers are correct (41, 42, etc.)
✅ Division by zero is properly caught
✅ Operator precedence is correct
✅ Power right-associativity is correct
✅ "Everything is a string" semantics are followed
✅ Unset variables return uppercase name
✅ Good unit test coverage for basic cases

Let me know if you'd like me to elaborate on any of these issues or help with the fixes.

## Code Review: Phase 1 Parser & Evaluator I've reviewed the implementation focusing on REXX semantics, correctness, arithmetic handling, and error management. Overall, this is a solid foundation with proper error numbering and good test coverage. Here are the issues I found: --- ## Critical Issues ### 1. **Integer Division Semantics** — src/eval.rs:171 **Location:** `eval.rs:171-173` The implementation uses `.round(0)` for integer division, but REXX's `%` operator requires **truncation toward zero**, not rounding. **Current code:** ```rust let ai = a.round(0); let bi = b.round(0); let result = (ai / bi).round(0); ``` **Issue:** For `-7 % 2`, REXX expects `-3` (truncate toward zero), but `.round(0)` would give `-4` (round to nearest). **Fix:** Use `.with_scale(0)` or implement proper truncation: ```rust let ai = a.with_scale(0); // truncates toward zero let bi = b.with_scale(0); let result = (ai / bi).with_scale(0); ``` --- ### 2. **Remainder Calculation Matches IntDiv Bug** — src/eval.rs:188 **Location:** `eval.rs:187-189` The remainder operation depends on integer division. Since `IntDiv` uses `.round(0)`, this will produce incorrect results for negative operands. **Formula:** REXX remainder is `a - (a%b)*b` This is correctly implemented, but it will give wrong answers when `a%b` is wrong. **Fix:** After fixing integer division, ensure this uses the corrected truncation. --- ### 3. **Power Operation: Division by Zero for 0^n where n<0** — src/eval.rs:338-340 **Location:** `eval.rs:338-340` The code handles negative exponents via `1 / pow_bigdecimal(base, -exp)`. **Issue:** When `base = 0` and `exp < 0`, this performs `1 / 0^|exp| = 1 / 0`, which should raise Error 42 (ArithmeticOverflow) but instead returns infinity or panics. **Missing check:** ```rust if exp < 0 { if base.is_zero() { return Err(RexxDiagnostic::new(RexxError::ArithmeticOverflow) .with_detail("0 cannot be raised to a negative power")); } let pos_result = pow_bigdecimal(base, -exp); return BigDecimal::from(1) / pos_result; } ``` --- ## Moderate Issues ### 4. **NUMERIC DIGITS Not Applied in Arithmetic** — src/value.rs:137 **Location:** `value.rs:137` and `eval.rs:280-287` The `format_rexx_number` function uses `.round(i64::from(digits))`, which rounds to a number of **decimal places**, not **significant digits**. **Example:** With `NUMERIC DIGITS 5`: - `123.456` should become `123.46` (5 significant digits) - Current code: `.round(5)` → `123.45600` (5 decimal places) ❌ **REXX spec:** NUMERIC DIGITS controls total significant digits, not decimal places. **Impact:** All arithmetic results will have incorrect precision. This doesn't break current tests (which use default DIGITS=9), but violates REXX semantics. **TODO comment exists (line 133-135)** acknowledging this, but the placeholder is incorrect. --- ### 5. **Missing Handling of Compound Variables in Parser** — src/parser.rs:451-457 **Location:** `parser.rs:451-457` (parse_primary) When parsing a `Symbol`, the code checks for function calls but **never produces `Expr::Compound`**. **Current:** Compound variables like `arr.i.j` are treated as simple symbols with dots in the name. **Expected:** These should parse to `Expr::Compound { stem, tail }` during expression parsing, not just in assignments. **Result:** Reading compound variables in expressions will fail or produce incorrect behavior: ```rexx arr.1 = "hello" say arr.1 /* Should work but may fail */ ``` The parser only handles compounds in assignments (line 163), not in symbol expressions. --- ### 6. **Comparison Operators Don't Check for Non-Chaining** — src/parser.rs:237-264 **Location:** `parser.rs:237-264` The comparison parsing uses a loop, allowing chains like `a = b = c`, which REXX does not support (comparison operators are non-associative). **Issue:** `say 1 = 1 = 1` would parse as `(1 = 1) = 1` → `1 = 1` → `1`, instead of raising a syntax error. **REXX behavior:** Comparison operators cannot be chained. Only one comparison per precedence level. **Fix:** Replace the loop with a single optional comparison: ```rust fn parse_comparison(&mut self) -> RexxResult<Expr> { let mut left = self.parse_concat()?; if let Some(op) = self.match_comparison_op() { self.advance(); let right = self.parse_concat()?; left = Expr::BinOp { left: Box::new(left), op, right: Box::new(right) }; } Ok(left) } ``` --- ## Minor Issues ### 7. **Tail Element Classification in Compound Variables** — src/parser.rs:506 **Location:** `parse_tail_elements` (line 506) The function treats parts that are all digits as `Const`, but empty parts should also be constants. **Issue:** The condition `part.is_empty() || part.chars().all(|c| c.is_ascii_digit())` is correct, but the comment/documentation doesn't clarify that empty parts arise from `arr..i` (two consecutive dots). **Not a bug**, but edge case handling should be tested. --- ### 8. **Normal Comparison Padding** — src/eval.rs:327-330 **Location:** `eval.rs:327-330` The padding uses `format!("{ls:<max_len$}")`, allocating new strings even when lengths are equal. **Optimization:** Check lengths first: ```rust match ls.len().cmp(&rs.len()) { std::cmp::Ordering::Equal => ls.cmp(rs), std::cmp::Ordering::Less => { let lp = format!("{ls:<max_len$}"); lp.cmp(rs) } std::cmp::Ordering::Greater => { let rp = format!("{rs:<max_len$}"); ls.cmp(&rp) } } ``` **Impact:** Minor performance issue, not a correctness bug. --- ### 9. **Exponentiation by Squaring: Potential Stack Overflow** — src/eval.rs:334-353 **Location:** `pow_bigdecimal` function The algorithm is iterative (good), but for extremely large exponents, the loop could run for a very long time or the intermediate `b * b` squaring could overflow available precision. **Not a safety issue** (no recursion), but consider clamping exponents to a reasonable range (e.g., `|exp| < 999999999`) per REXX spec limits. --- ## Test Coverage Gaps ### Missing test cases: 1. **Negative operands in `%` and `//`**: Need `-7 % 2`, `-7 // 2`, `7 % -2` 2. **Zero to negative power**: `say 0 ** -1` should error 3. **Chained comparisons**: `say 1 = 1 = 1` should error (if fixed) 4. **Compound variable reads**: `arr.1 = 5; say arr.1` 5. **NUMERIC DIGITS edge cases**: Numbers with more digits than DIGITS setting 6. **Whitespace in numbers**: `say " 3 " + " 4 "` (should work per REXX) --- ## Summary **Correctness issues to fix before merge:** 1. Integer division truncation (critical) 2. Remainder depends on #1 (critical) 3. `0 ** -n` division by zero (critical) 4. Compound variable parsing in expressions (moderate, likely already failing tests) 5. NUMERIC DIGITS significance vs decimal places (moderate, spec violation) 6. Comparison operator chaining (moderate, spec violation) **Non-blocking issues:** - String comparison padding optimization (#8) - Test coverage gaps **Good things:** ✅ Error numbers are correct (41, 42, etc.) ✅ Division by zero is properly caught ✅ Operator precedence is correct ✅ Power right-associativity is correct ✅ "Everything is a string" semantics are followed ✅ Unset variables return uppercase name ✅ Good unit test coverage for basic cases --- Let me know if you'd like me to elaborate on any of these issues or help with the fixes.

claude[bot] commented

2026-02-08 13:49:04 +00:00

(Migrated from github.com)

Code Review — Phase 1 REXX Interpreter

I've reviewed the parser, evaluator, and lexer implementation. Here are my findings:

Critical Issues

1. Arithmetic: Truncation Logic in trunc_div is Flawed — src/eval.rs:336-344

The current implementation truncates by string manipulation which does not properly truncate toward zero for negative numbers. For -7 / 3, this may give incorrect results for remainder calculations.

Better approach: Use proper toward-zero truncation with BigDecimal methods.

2. Parser: Implicit Concatenation Creates Infinite Loop Risk — src/parser.rs:268-303

In parse_concat, the implicit concatenation logic at lines 285-298 may loop infinitely if parse_addition returns without consuming tokens.

Recommendation: Add a position check to ensure forward progress.

3. Evaluator: pow_bigdecimal Lacks Overflow Protection — src/eval.rs:348-367

The exponentiation function uses repeated squaring but does not check for overflow. say 999 ** 999 will consume unbounded memory without returning Error 42 (ArithmeticOverflow).

Fix: Check result magnitude after each multiplication and return Error 42 if it exceeds a reasonable threshold.

4. Parser: Assignment Parsing Incorrectly Handles Compound Variables — src/parser.rs:163-169

The parser assumes the lexer will always produce a single symbol token for compound variables like arr.i.j, but the lexer's dot-handling logic (lexer.rs:222-232) is ambiguous.

Fix: Either ensure the lexer always produces compound symbols as single tokens, or parse compound assignment by lookahead for dots after the initial symbol.

Medium-Severity Issues

5. Evaluator: Normal Comparison Does Not Respect NUMERIC FUZZ — src/eval.rs:319-332

The normal_compare function compares numbers directly without respecting NUMERIC FUZZ. Per ANSI X3.274-1996 section 7.1.5, if FUZZ is 2 and you compare 1.001 and 1.002, they should be considered equal.

6. Parser: peek_at Returns Reference to Temporary — src/parser.rs:89-96

The peek_at method returns a reference to TokenKind::Eof which is a temporary value. While Rust allows this due to lifetime extension, it is fragile.

Fix: Store an EOF token in the Parser struct or return an owned value.

7. Lexer: Hex/Binary String Conversion Assumes Single-Byte Chars — src/lexer.rs:486-513

Hex strings like C3A9 X (UTF-8 for é) will produce two separate characters instead of one UTF-8 codepoint. This is technically correct for REXX's 1-byte-per-character model, but may surprise users with multi-byte encodings.

Performance Issues

8. Evaluator: Unnecessary String Allocations in Concatenation — src/eval.rs:220-226

For repeated concatenation like a || b || c || d, this creates intermediate strings for each operation, leading to O(n²) behavior. Not critical for phase 1 but will become problematic for long concatenations.

9. Parser: clone() on TokenKind in parse_primary — src/parser.rs:442

Clones string data from tokens unnecessarily. For StringLit and Symbol tokens, this copies the entire string.

Test Coverage Gaps

10. Missing Test: Negative Remainder — src/eval.rs:543-547

The test suite includes eval_remainder but only tests positive operands. REXX remainder with negative operands has specific behavior that should be tested.

11. Missing Test: Deeply Nested Expressions

No test verifies the parser handles deeply nested expressions without stack overflow.

12. Missing Test: Exponent Overflow

No test for say 10 ** 9999999 which should return Error 42, not hang or crash.

Minor Issues

13. Evaluator: unwrap_or_default in trunc_div Silently Returns Zero — src/eval.rs:341

If BigDecimal::from_str fails, this returns zero silently instead of propagating an error.

14. Lexer: Comment Depth Counter Uses u32 — src/lexer.rs:170

Allows 4 billion nested comments. A more reasonable limit would catch runaway nesting faster.

Positive Observations

✅ Operator precedence is correctly implemented with proper precedence climbing (parser.rs:181-407)

✅ Division by zero correctly returns Error 42 with proper checks in all division operations

✅ Unset variables correctly return uppercase name (eval.rs:78, test at line 514)

✅ Error numbers match ANSI X3.274-1996 specification (error.rs:119-160)

✅ Right-associative power operator correctly implemented recursively (parser.rs:394-407)

✅ Space tracking for function call vs concatenation disambiguation is well-designed (lexer.rs:65-68, parser.rs:454)

Summary

Critical: Issues 1, 3, and 4 should be fixed before merge.

Recommended: Issues 2, 5, 6, and 7 should be addressed for robustness.

Future work: Performance issues 8-9 and test coverage gaps 10-12 can be deferred to later phases.

The implementation demonstrates solid understanding of REXX semantics and good use of Rust idioms. The architecture is clean and the test coverage for core functionality is strong.

## Code Review — Phase 1 REXX Interpreter I've reviewed the parser, evaluator, and lexer implementation. Here are my findings: --- ## Critical Issues ### 1. Arithmetic: Truncation Logic in trunc_div is Flawed — src/eval.rs:336-344 The current implementation truncates by string manipulation which does not properly truncate toward zero for negative numbers. For -7 / 3, this may give incorrect results for remainder calculations. Better approach: Use proper toward-zero truncation with BigDecimal methods. ### 2. Parser: Implicit Concatenation Creates Infinite Loop Risk — src/parser.rs:268-303 In parse_concat, the implicit concatenation logic at lines 285-298 may loop infinitely if parse_addition returns without consuming tokens. Recommendation: Add a position check to ensure forward progress. ### 3. Evaluator: pow_bigdecimal Lacks Overflow Protection — src/eval.rs:348-367 The exponentiation function uses repeated squaring but does not check for overflow. say 999 ** 999 will consume unbounded memory without returning Error 42 (ArithmeticOverflow). Fix: Check result magnitude after each multiplication and return Error 42 if it exceeds a reasonable threshold. ### 4. Parser: Assignment Parsing Incorrectly Handles Compound Variables — src/parser.rs:163-169 The parser assumes the lexer will always produce a single symbol token for compound variables like arr.i.j, but the lexer's dot-handling logic (lexer.rs:222-232) is ambiguous. Fix: Either ensure the lexer always produces compound symbols as single tokens, or parse compound assignment by lookahead for dots after the initial symbol. --- ## Medium-Severity Issues ### 5. Evaluator: Normal Comparison Does Not Respect NUMERIC FUZZ — src/eval.rs:319-332 The normal_compare function compares numbers directly without respecting NUMERIC FUZZ. Per ANSI X3.274-1996 section 7.1.5, if FUZZ is 2 and you compare 1.001 and 1.002, they should be considered equal. ### 6. Parser: peek_at Returns Reference to Temporary — src/parser.rs:89-96 The peek_at method returns a reference to TokenKind::Eof which is a temporary value. While Rust allows this due to lifetime extension, it is fragile. Fix: Store an EOF token in the Parser struct or return an owned value. ### 7. Lexer: Hex/Binary String Conversion Assumes Single-Byte Chars — src/lexer.rs:486-513 Hex strings like C3A9 X (UTF-8 for é) will produce two separate characters instead of one UTF-8 codepoint. This is technically correct for REXX's 1-byte-per-character model, but may surprise users with multi-byte encodings. --- ## Performance Issues ### 8. Evaluator: Unnecessary String Allocations in Concatenation — src/eval.rs:220-226 For repeated concatenation like a || b || c || d, this creates intermediate strings for each operation, leading to O(n²) behavior. Not critical for phase 1 but will become problematic for long concatenations. ### 9. Parser: clone() on TokenKind in parse_primary — src/parser.rs:442 Clones string data from tokens unnecessarily. For StringLit and Symbol tokens, this copies the entire string. --- ## Test Coverage Gaps ### 10. Missing Test: Negative Remainder — src/eval.rs:543-547 The test suite includes eval_remainder but only tests positive operands. REXX remainder with negative operands has specific behavior that should be tested. ### 11. Missing Test: Deeply Nested Expressions No test verifies the parser handles deeply nested expressions without stack overflow. ### 12. Missing Test: Exponent Overflow No test for say 10 ** 9999999 which should return Error 42, not hang or crash. --- ## Minor Issues ### 13. Evaluator: unwrap_or_default in trunc_div Silently Returns Zero — src/eval.rs:341 If BigDecimal::from_str fails, this returns zero silently instead of propagating an error. ### 14. Lexer: Comment Depth Counter Uses u32 — src/lexer.rs:170 Allows 4 billion nested comments. A more reasonable limit would catch runaway nesting faster. --- ## Positive Observations ✅ Operator precedence is correctly implemented with proper precedence climbing (parser.rs:181-407) ✅ Division by zero correctly returns Error 42 with proper checks in all division operations ✅ Unset variables correctly return uppercase name (eval.rs:78, test at line 514) ✅ Error numbers match ANSI X3.274-1996 specification (error.rs:119-160) ✅ Right-associative power operator correctly implemented recursively (parser.rs:394-407) ✅ Space tracking for function call vs concatenation disambiguation is well-designed (lexer.rs:65-68, parser.rs:454) --- ## Summary **Critical**: Issues 1, 3, and 4 should be fixed before merge. **Recommended**: Issues 2, 5, 6, and 7 should be addressed for robustness. **Future work**: Performance issues 8-9 and test coverage gaps 10-12 can be deferred to later phases. The implementation demonstrates solid understanding of REXX semantics and good use of Rust idioms. The architecture is clean and the test coverage for core functionality is strong.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

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!1

No description provided.

Rows
Columns

⏺ Phase 1 is complete. Here's a summary of what was implemented: #1

Code Review - Phase 1 Implementation

Todo List

Overall Assessment

Strengths

Architecture

REXX Semantics

Code Quality

Issues Found

Critical Issues

Moderate Issues

1. Precedence ambiguity in is_binary_op (src/parser.rs:321-348)

2. Hex/Binary string conversion uses char instead of bytes (src/lexer.rs:486-513)

3. Integer division implementation may have precision issues (src/eval.rs:171-176)

Minor Issues

4. Incomplete numeric formatting (src/value.rs:132-147)

5. Parser uses unreachable!() without a panic message (src/parser.rs:158)

6. Test helper eval_and_capture returns empty string (src/eval.rs:362-372)

Code Style & Consistency

7. Inconsistent error detail messages

Architecture Recommendations

For Future Phases

Test Coverage Observations

What's Well-Tested

Test Gaps

Documentation

Performance Notes

Final Verdict

Recommended Follow-up Actions

Code Review: Phase 1 Parser & Evaluator

Critical Issues

1. Integer Division Semantics — src/eval.rs:171

2. Remainder Calculation Matches IntDiv Bug — src/eval.rs:188

3. Power Operation: Division by Zero for 0^n where n<0 — src/eval.rs:338-340

Moderate Issues

4. NUMERIC DIGITS Not Applied in Arithmetic — src/value.rs:137

5. Missing Handling of Compound Variables in Parser — src/parser.rs:451-457

6. Comparison Operators Don't Check for Non-Chaining — src/parser.rs:237-264

Minor Issues

7. Tail Element Classification in Compound Variables — src/parser.rs:506

8. Normal Comparison Padding — src/eval.rs:327-330

9. Exponentiation by Squaring: Potential Stack Overflow — src/eval.rs:334-353

Test Coverage Gaps

Missing test cases:

Summary

Code Review — Phase 1 REXX Interpreter

Critical Issues

1. Arithmetic: Truncation Logic in trunc_div is Flawed — src/eval.rs:336-344

2. Parser: Implicit Concatenation Creates Infinite Loop Risk — src/parser.rs:268-303

3. Evaluator: pow_bigdecimal Lacks Overflow Protection — src/eval.rs:348-367

4. Parser: Assignment Parsing Incorrectly Handles Compound Variables — src/parser.rs:163-169

Medium-Severity Issues

5. Evaluator: Normal Comparison Does Not Respect NUMERIC FUZZ — src/eval.rs:319-332

6. Parser: peek_at Returns Reference to Temporary — src/parser.rs:89-96

7. Lexer: Hex/Binary String Conversion Assumes Single-Byte Chars — src/lexer.rs:486-513

Performance Issues

8. Evaluator: Unnecessary String Allocations in Concatenation — src/eval.rs:220-226

9. Parser: clone() on TokenKind in parse_primary — src/parser.rs:442

Test Coverage Gaps

10. Missing Test: Negative Remainder — src/eval.rs:543-547

11. Missing Test: Deeply Nested Expressions

12. Missing Test: Exponent Overflow

Minor Issues

13. Evaluator: unwrap_or_default in trunc_div Silently Returns Zero — src/eval.rs:341

14. Lexer: Comment Depth Counter Uses u32 — src/lexer.rs:170

Positive Observations

Summary

1. Precedence ambiguity in `is_binary_op` (src/parser.rs:321-348)

2. Hex/Binary string conversion uses `char` instead of bytes (src/lexer.rs:486-513)

5. Parser uses `unreachable!()` without a panic message (src/parser.rs:158)

6. Test helper `eval_and_capture` returns empty string (src/eval.rs:362-372)