navicore/patch-rexx

Fork 0

rexxfile #19

Merged

navicore merged 3 commits from rexxfile into main

2026-02-12 00:15:45 +00:00

navicore commented

2026-02-11 23:53:45 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-11 23:57:51 +00:00

(Migrated from github.com)

PR Review: Shebang Support and Exit Code Handling

I've reviewed PR #19 which adds shebang line support and proper exit code handling to the REXX interpreter. Overall, the implementation is sound and well-tested. Here are my findings:

✅ What's Good

Shebang Handling (src/lexer.rs:214-222)

Correctly checks for #! at position 0 only
Uses peek_ahead(1) to verify both characters before skipping
Properly stops at newline without consuming it (letting normal EOL handling proceed)
Test coverage includes shebang with comments and code

Exit Code Handling (src/main.rs:86-88)

Correctly converts RexxValue to i32 via to_string().trim().parse()
Uses .unwrap_or(0) for graceful fallback on non-numeric values (matches REXX semantics)
Properly handles EXIT with no value (returns 0)
Exit(Some(val)) correctly propagates from evaluator (src/eval.rs:390-396)

Test Coverage (tests/rexxfile.rs)

Comprehensive: shebang, shebang+comment, exit 0, exit nonzero, exit from variable, exit with no value
All tests pass ✓
Integration tests use actual binary execution for realistic validation

🔍 Potential Issue: Shebang Not Consumed After Break

Location: src/lexer.rs:217-218

if ch == '\n' {
    break;  // ← exits loop without advancing past the newline
}

The shebang skipping logic breaks when it finds \n but doesn't consume it. However, this appears intentional because:

The test expects an Eol token as the first token after shebang (src/lexer.rs:647)
REXX treats newlines as clause terminators, so the newline after shebang should produce an Eol token
This creates an empty first clause, which is harmless

Verdict: Not a bug, but consider adding a comment explaining why the newline isn't consumed:

// Skip shebang line if at start of file
if self.pos == 0 && self.peek() == Some('#') && self.peek_ahead(1) == Some('!') {
    while let Some(ch) = self.peek() {
        if ch == '\n' {
            // Don't consume the newline — it should produce an Eol token
            // for an empty first clause (harmless in REXX)
            break;
        }
        self.advance();
    }
}

🎯 Summary

No blocking issues. The implementation correctly handles:

✅ REXX semantics (everything is a string, graceful numeric conversion)
✅ Edge cases (non-numeric exit values, whitespace, negative numbers)
✅ Platform conventions (i32 exit codes with overflow)
✅ Parser semantics (shebang only at file start, newline handling)

The code is production-ready. The only suggestion is the clarifying comment above.

Recommendation: ✅ Approve and merge

## PR Review: Shebang Support and Exit Code Handling I've reviewed PR #19 which adds shebang line support and proper exit code handling to the REXX interpreter. Overall, the implementation is **sound and well-tested**. Here are my findings: ### ✅ What's Good **Shebang Handling (src/lexer.rs:214-222)** - Correctly checks for `#!` at position 0 only - Uses `peek_ahead(1)` to verify both characters before skipping - Properly stops at newline without consuming it (letting normal EOL handling proceed) - Test coverage includes shebang with comments and code **Exit Code Handling (src/main.rs:86-88)** - Correctly converts RexxValue to i32 via `to_string().trim().parse()` - Uses `.unwrap_or(0)` for graceful fallback on non-numeric values (matches REXX semantics) - Properly handles `EXIT` with no value (returns 0) - `Exit(Some(val))` correctly propagates from evaluator (src/eval.rs:390-396) **Test Coverage (tests/rexxfile.rs)** - Comprehensive: shebang, shebang+comment, exit 0, exit nonzero, exit from variable, exit with no value - All tests pass ✓ - Integration tests use actual binary execution for realistic validation ### 🔍 Potential Issue: Shebang Not Consumed After Break **Location:** src/lexer.rs:217-218 ```rust if ch == '\n' { break; // ← exits loop without advancing past the newline } ``` The shebang skipping logic breaks when it finds `\n` but doesn't consume it. However, this appears **intentional** because: 1. The test expects an `Eol` token as the first token after shebang (src/lexer.rs:647) 2. REXX treats newlines as clause terminators, so the newline after shebang should produce an `Eol` token 3. This creates an empty first clause, which is harmless **Verdict:** Not a bug, but consider adding a comment explaining why the newline isn't consumed: ```rust // Skip shebang line if at start of file if self.pos == 0 && self.peek() == Some('#') && self.peek_ahead(1) == Some('!') { while let Some(ch) = self.peek() { if ch == '\n' { // Don't consume the newline — it should produce an Eol token // for an empty first clause (harmless in REXX) break; } self.advance(); } } ``` ### 🎯 Summary **No blocking issues.** The implementation correctly handles: - ✅ REXX semantics (everything is a string, graceful numeric conversion) - ✅ Edge cases (non-numeric exit values, whitespace, negative numbers) - ✅ Platform conventions (i32 exit codes with overflow) - ✅ Parser semantics (shebang only at file start, newline handling) The code is production-ready. The only suggestion is the clarifying comment above. **Recommendation:** ✅ **Approve and merge**

claude[bot] commented

2026-02-11 23:57:52 +00:00

(Migrated from github.com)

Code Review - PR #19: rexxfile

CRITICAL ISSUE - Exit code overflow behavior (src/main.rs:87):

The line val.to_string().trim().parse::<i32>().unwrap_or(0) silently converts unparseable values to 0.

Test cases that demonstrate the problem:

EXIT 999999999999999999999999 -> returns 0 (should error)
EXIT 3.14 -> returns 3 (loses precision)
EXIT hello -> returns 0 (acceptable)

REXX semantics require EXIT value to be:

A valid REXX number (use val.to_decimal())
A whole number (Error 26: Invalid whole number)
In valid exit code range (0-255 typical)

Recommend: Add proper validation with REXX error codes (BadArithmetic/InvalidWholeNumber/ArithmeticOverflow) instead of silent conversion to 0.

Minor issue - Shebang handler (src/lexer.rs:216-221):
The newline after shebang is not consumed. This is safe but slightly inconsistent with comment handling.

Otherwise: Excellent test coverage, no safety issues, proper error handling. Nice work!

Code Review - PR #19: rexxfile CRITICAL ISSUE - Exit code overflow behavior (src/main.rs:87): The line `val.to_string().trim().parse::<i32>().unwrap_or(0)` silently converts unparseable values to 0. Test cases that demonstrate the problem: - EXIT 999999999999999999999999 -> returns 0 (should error) - EXIT 3.14 -> returns 3 (loses precision) - EXIT hello -> returns 0 (acceptable) REXX semantics require EXIT value to be: 1. A valid REXX number (use val.to_decimal()) 2. A whole number (Error 26: Invalid whole number) 3. In valid exit code range (0-255 typical) Recommend: Add proper validation with REXX error codes (BadArithmetic/InvalidWholeNumber/ArithmeticOverflow) instead of silent conversion to 0. Minor issue - Shebang handler (src/lexer.rs:216-221): The newline after shebang is not consumed. This is safe but slightly inconsistent with comment handling. Otherwise: Excellent test coverage, no safety issues, proper error handling. Nice work!

claude[bot] commented

2026-02-12 00:14:24 +00:00

(Migrated from github.com)

PR Review: Shebang Support and EXIT Code Handling

I've reviewed this PR focusing on REXX semantics, error handling, arithmetic operations, and edge cases.

Summary

✅ Overall: Clean implementation with good test coverage

The PR adds shebang support and proper EXIT code propagation to the process exit status. The implementation is sound with one minor observation.

Detailed Analysis

1. Shebang Implementation (`src/lexer.rs:214-222`)

✅ Correct

Only triggers at pos == 0 (file start)
Properly checks for #! sequence using peek_ahead
Stops at newline (or EOF via while let Some)
Leaves the newline for normal tokenization → produces EOL token
Test confirms: TokenKind::Eol followed by first statement tokens

Edge cases handled:

Shebang-only file (no newline): loop exits when peek() == None
Shebang mid-file: won't trigger due to pos == 0 guard
No shebang: no processing overhead

2. EXIT Code Handling (`src/main.rs:86-100`)

✅ Correct REXX semantics

Validation chain:

to_decimal() → Error 41 (BadArithmetic) if non-numeric
round(0) → round to integer
d != rounded → Error 26 (InvalidWholeNumber) if fractional
parse::<i32>().unwrap_or(1) → handle overflow

Error numbers:

✅ Error 41 (BadArithmetic) for non-numeric values
✅ Error 26 (InvalidWholeNumber) for fractional values
✅ Both match ANSI X3.274-1996 specification

Minor observation (not a bug):

Line 99: rounded.to_string().parse::<i32>() could theoretically fail if BigDecimal emits scientific notation (e.g., 1e10), but:
- After round(0), BigDecimal typically emits plain integers
- unwrap_or(1) gracefully handles parse failures
- Tests confirm this works correctly for normal cases
- Extreme cases (e.g., EXIT 999999999999999) will map to exit code 1 via unwrap_or

REXX compliance:

✅ EXIT with no value → 0 (line 102)
✅ EXIT with integer → that code
✅ EXIT with non-number → Error 41
✅ EXIT with fraction → Error 26

3. Test Coverage (`tests/rexxfile.rs`)

✅ Comprehensive

Shebang tests:

Basic shebang + code
Shebang + comment + code
No shebang (regression check)

EXIT tests:

Zero exit code
Nonzero codes (2, 42)
Exit from variable
No value (defaults to 0)
Non-numeric (Error 41)
Fractional (Error 26)
Negative code (OS-specific wrapping)

Not tested (probably not needed):

Very large exit codes (>2³¹−1) — covered by unwrap_or(1)
Scientific notation edge case — extremely unlikely

4. Safety & Correctness

✅ No panics

All unwrap() calls are on infallible operations
Line 99 uses unwrap_or(1) for fallback

✅ No undefined behavior

Shebang handling uses safe peek() / peek_ahead()
No out-of-bounds access

✅ Arithmetic operations (verified via separate analysis)

Division by zero → Error 42 (ArithmeticOverflow) ✅
Non-numeric operands → Error 41 (BadArithmetic) ✅
NUMERIC DIGITS compliance → all results formatted via from_decimal() ✅

Recommendation

✅ Approve and merge

This is a clean, well-tested implementation. The minor observation about scientific notation is not a practical concern and is already handled defensively.

Files Reviewed

src/lexer.rs: Shebang handling
src/main.rs: EXIT code propagation
tests/rexxfile.rs: Integration tests
examples/rexxfile: Practical usage example (verified syntax)

No issues with REXX semantics, error handling, or safety.

## PR Review: Shebang Support and EXIT Code Handling I've reviewed this PR focusing on REXX semantics, error handling, arithmetic operations, and edge cases. ### Summary ✅ **Overall: Clean implementation with good test coverage** The PR adds shebang support and proper EXIT code propagation to the process exit status. The implementation is sound with one minor observation. --- ### Detailed Analysis #### 1. Shebang Implementation (`src/lexer.rs:214-222`) ✅ **Correct** - Only triggers at `pos == 0` (file start) - Properly checks for `#!` sequence using `peek_ahead` - Stops at newline (or EOF via `while let Some`) - Leaves the newline for normal tokenization → produces EOL token - Test confirms: `TokenKind::Eol` followed by first statement tokens **Edge cases handled:** - Shebang-only file (no newline): loop exits when `peek() == None` - Shebang mid-file: won't trigger due to `pos == 0` guard - No shebang: no processing overhead --- #### 2. EXIT Code Handling (`src/main.rs:86-100`) ✅ **Correct REXX semantics** **Validation chain:** 1. `to_decimal()` → Error 41 (BadArithmetic) if non-numeric 2. `round(0)` → round to integer 3. `d != rounded` → Error 26 (InvalidWholeNumber) if fractional 4. `parse::<i32>().unwrap_or(1)` → handle overflow **Error numbers:** - ✅ Error 41 (BadArithmetic) for non-numeric values - ✅ Error 26 (InvalidWholeNumber) for fractional values - ✅ Both match ANSI X3.274-1996 specification **Minor observation (not a bug):** - Line 99: `rounded.to_string().parse::<i32>()` could theoretically fail if BigDecimal emits scientific notation (e.g., `1e10`), but: - After `round(0)`, BigDecimal typically emits plain integers - `unwrap_or(1)` gracefully handles parse failures - Tests confirm this works correctly for normal cases - Extreme cases (e.g., EXIT 999999999999999) will map to exit code 1 via `unwrap_or` **REXX compliance:** - ✅ EXIT with no value → 0 (line 102) - ✅ EXIT with integer → that code - ✅ EXIT with non-number → Error 41 - ✅ EXIT with fraction → Error 26 --- #### 3. Test Coverage (`tests/rexxfile.rs`) ✅ **Comprehensive** **Shebang tests:** - Basic shebang + code - Shebang + comment + code - No shebang (regression check) **EXIT tests:** - Zero exit code - Nonzero codes (2, 42) - Exit from variable - No value (defaults to 0) - Non-numeric (Error 41) - Fractional (Error 26) - Negative code (OS-specific wrapping) **Not tested (probably not needed):** - Very large exit codes (>2³¹−1) — covered by `unwrap_or(1)` - Scientific notation edge case — extremely unlikely --- #### 4. Safety & Correctness ✅ **No panics** - All `unwrap()` calls are on infallible operations - Line 99 uses `unwrap_or(1)` for fallback ✅ **No undefined behavior** - Shebang handling uses safe `peek()` / `peek_ahead()` - No out-of-bounds access ✅ **Arithmetic operations** (verified via separate analysis) - Division by zero → Error 42 (ArithmeticOverflow) ✅ - Non-numeric operands → Error 41 (BadArithmetic) ✅ - NUMERIC DIGITS compliance → all results formatted via `from_decimal()` ✅ --- ### Recommendation **✅ Approve and merge** This is a clean, well-tested implementation. The minor observation about scientific notation is not a practical concern and is already handled defensively. --- ### Files Reviewed - `src/lexer.rs`: Shebang handling - `src/main.rs`: EXIT code propagation - `tests/rexxfile.rs`: Integration tests - `examples/rexxfile`: Practical usage example (verified syntax) No issues with REXX semantics, error handling, or safety.