navicore/patch-rexx

Fork 0

⏺ Phase 6 is fully implemented. Here's a summary of all changes: #7

Merged

navicore merged 3 commits from phase6 into main

2026-02-09 21:14:15 +00:00

navicore commented

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

(Migrated from github.com)

Files Modified
File: src/ast.rs
Changes: Added Hash derive to Condition enum
────────────────────────────────────────
File: src/parser.rs
Changes: Added parse_signal() and parse_condition() methods, wired SIGNAL into parse_clause()
────────────────────────────────────────
File: src/eval.rs
Changes: Added ExecSignal::Signal(String) variant, traps/pending_signal fields, exec_signal() method, restart loop in exec(), NOVALUE trap in eval_expr,
SYNTAX trap wrapper exec_clause_outer(), Signal propagation through all DO handlers and call_routine
────────────────────────────────────────
File: src/env.rs
Changes: Added ConditionInfoData struct, condition_info field, set_condition_info() method
────────────────────────────────────────
File: src/builtins.rs
Changes: Added &Environment param to call_builtin, added CONDITION() BIF
────────────────────────────────────────
File: src/main.rs
Changes: Added ExecSignal::Signal handling in run_line()
────────────────────────────────────────
File: tests/phase6.rs
Changes: New — 22 integration tests
Features Implemented

SIGNAL label — goto/transfer-of-control to a label
SIGNAL VALUE expr — computed jump target
SIGNAL ON/OFF condition — condition trap registration/removal
NOVALUE trap — fires when uninitialized variables are accessed
SYNTAX trap — fires on runtime errors, sets RC to error number
CONDITION() BIF — returns trap info (C/D/I/S options)
Traps auto-disable after firing (per REXX spec)

Verification

304 tests pass (282 existing + 22 new)
clippy clean (-D warnings)

Files Modified File: src/ast.rs Changes: Added Hash derive to Condition enum ──────────────────────────────────────── File: src/parser.rs Changes: Added parse_signal() and parse_condition() methods, wired SIGNAL into parse_clause() ──────────────────────────────────────── File: src/eval.rs Changes: Added ExecSignal::Signal(String) variant, traps/pending_signal fields, exec_signal() method, restart loop in exec(), NOVALUE trap in eval_expr, SYNTAX trap wrapper exec_clause_outer(), Signal propagation through all DO handlers and call_routine ──────────────────────────────────────── File: src/env.rs Changes: Added ConditionInfoData struct, condition_info field, set_condition_info() method ──────────────────────────────────────── File: src/builtins.rs Changes: Added &Environment param to call_builtin, added CONDITION() BIF ──────────────────────────────────────── File: src/main.rs Changes: Added ExecSignal::Signal handling in run_line() ──────────────────────────────────────── File: tests/phase6.rs Changes: New — 22 integration tests Features Implemented - SIGNAL label — goto/transfer-of-control to a label - SIGNAL VALUE expr — computed jump target - SIGNAL ON/OFF condition — condition trap registration/removal - NOVALUE trap — fires when uninitialized variables are accessed - SYNTAX trap — fires on runtime errors, sets RC to error number - CONDITION() BIF — returns trap info (C/D/I/S options) - Traps auto-disable after firing (per REXX spec) Verification - 304 tests pass (282 existing + 22 new) - clippy clean (-D warnings)

claude[bot] commented

2026-02-09 04:30:05 +00:00

(Migrated from github.com)

PR #7 Review: SIGNAL Implementation (Phase 6)

Summary

This PR implements SIGNAL control flow and condition traps for the REXX interpreter. The implementation is largely correct with excellent test coverage (22 passing tests). I found 2 critical bugs and several areas for improvement.

🔴 CRITICAL BUGS

1. NOVALUE trap doesn't fire for compound variables

Location: src/eval.rs:998-1001 (Expr::Compound branch)

Bug: The NOVALUE trap only checks simple variables, not compound variables.

signal on novalue
x = arr.1  /* arr.1 is unset - should trap but doesn't */
exit
novalue: say 'TRAPPED'

Root cause: env.is_set() at line 982 only checks the vars HashMap, not stems. The Expr::Compound branch (line 998) has no NOVALUE check at all.

Fix: Add similar trap logic to the Expr::Compound branch, checking if the compound tail is unset.

Test needed: Add test case for NOVALUE on unset compound variable.

2. NOVALUE trap delayed until clause completion

Location: src/eval.rs:982-996 (eval_expr Symbol handling)

Bug: When NOVALUE fires during expression evaluation, pending_signal is set but eval_expr continues and returns the uppercase variable name. The signal doesn't fire until the clause completes.

Example:

signal on novalue
x = unsetvar + 1  /* Should trap BEFORE attempting arithmetic */
exit
novalue: say 'TRAPPED'

Expected: Trap immediately when unsetvar is accessed.
Actual: Returns "UNSETVAR", attempts arithmetic "UNSETVAR" + 1, fails with Error 41, THEN might check pending_signal.

REXX Spec (ANSI X3.274-1996 §7.4.10): SIGNAL ON NOVALUE should transfer control immediately when uninitialized variable is referenced.

Fix: After setting pending_signal (line 994), return immediately instead of continuing to line 996. Consider returning a sentinel value or restructuring to check pending_signal in the caller.

⚠️ MODERATE ISSUES

3. Missing test: SIGNAL VALUE to non-existent label

Gap: No test for signal value 'NONEXISTENT' to verify Error 16 propagation.

Risk: The error is caught at eval.rs:94, but if SIGNAL VALUE is executed inside a function call expression, the pending_signal mechanism might not propagate correctly.

Recommendation: Add test case:

signal value 'BADLABEL'  /* Should fail with Error 16 */

4. CONDITION() with no active trap behavior unclear

Location: src/builtins.rs:1632-1650

Current: Returns empty strings when condition_info is None.

Spec verification needed: Per ANSI X3.274-1996 §9.4.4, verify whether CONDITION('I') and CONDITION('S') should return different values when no trap is active vs. after trap fires. Current implementation might be oversimplified.

Recommendation: Review spec section 9.4.4 and add test for CONDITION() with no active trap.

✅ CORRECTNESS VERIFIED

Parser (`src/parser.rs:1031-1121`)

✅ All four SIGNAL forms parsed correctly
✅ Case-insensitive labels and conditions
✅ Proper error for invalid condition (Error 25)
✅ All seven standard conditions recognized

Signal Propagation

✅ Signals propagate through all DO variants (lines 700, 726, 755, 780, 887)
✅ Loop restart logic correct (eval.rs:89-102)
✅ CALL and exec_body propagate signals
✅ Tests verify nested DO escape (phase6.rs:72-85)

SYNTAX Trap (`src/eval.rs:137-162`)

✅ Sets RC to error number (error.number())
✅ Sets condition info with description
✅ Auto-disables trap after firing (line 155)
✅ Test verifies RC=42 for division by zero (phase6.rs:140-146)

Division by Zero

✅ All three operators check: / (1105), % (1123), // (1138)
✅ Error 42 (ArithmeticOverflow) is correct per REXX spec
✅ Test coverage: phase6.rs:135-147

Trap Auto-Disable (ANSI §7.4.10.4)

✅ NOVALUE: eval.rs:993
✅ SYNTAX: eval.rs:155
✅ Test verifies: phase6.rs:211-219

Hash Derive for Condition Enum

✅ Required for HashMap<Condition, String> at eval.rs:59
✅ Minimal change to ast.rs:227

📊 TEST COVERAGE

22 tests, all passing — Excellent coverage overall.

Missing test cases:

❌ NOVALUE on compound variable (critical bug #1)
❌ SIGNAL VALUE to non-existent label
❌ SIGNAL from within SELECT/WHEN
❌ Multiple nested signals (signal inside signal handler)
❌ CONDITION() when no trap is active

🔍 DETAILED FINDINGS

Error Handling: src/error.rs

✅ All error numbers match ANSI X3.274-1996
✅ Error 16 (LabelNotFound): correct usage
✅ Error 25 (InvalidSubKeyword): correct for bad condition names
✅ Error 42 (ArithmeticOverflow): correct for division by zero and overflow

Environment: src/env.rs

✅ ConditionInfoData struct appropriate (4 string fields)
✅ condition_info: Option<ConditionInfoData> good design
✅ set_condition_info() simple setter (line 175)
⚠️ is_set() only checks simple vars, not compounds (line 180-185) — contributes to bug #1

Built-ins: src/builtins.rs

✅ Added &Environment param to call_builtin (breaking change, all call sites updated)
✅ CONDITION() BIF implementation correct structure
✅ Check_args validates 0 or 1 argument
✅ Option abbreviations supported (C/D/I/S)
✅ Error 40 (IncorrectCall) for invalid option

Main: src/main.rs

✅ Added ExecSignal::Signal handling in run_line (lines 88-91)
✅ Returns Error 16 if signal escapes to top level

🎯 RECOMMENDATIONS

Must Fix Before Merge

Fix bug #1: Add NOVALUE trap check to Expr::Compound branch (eval.rs:998)
Fix bug #2: Make NOVALUE trap fire immediately in eval_expr, not delayed
Add test: NOVALUE on compound variable
Add test: signal value 'NONEXISTENT'

Should Fix

Verify CONDITION() spec behavior for no-trap case
Add test for SIGNAL from SELECT/WHEN

Documentation

Add inline comment at eval.rs:982: why uppercase name is returned
Add comment at error.rs:102: Error 42 covers both overflow AND division by zero

📝 REXX SEMANTICS COMPLIANCE

✅ Correct:

"Everything is a string" — maintained
No reserved words — keywords case-insensitive
Unset variables return uppercase name (except when NOVALUE trap active)
SIGNAL abandons all blocks (tests verify)
Traps auto-disable after firing (per ANSI §7.4.10.4)

⚠️ Needs verification:

NOVALUE firing point (immediate vs. delayed) — spec §7.4.10 suggests immediate

🏆 STRENGTHS

Excellent test coverage — 22 comprehensive tests
Clean separation of concerns — parser, evaluator, traps well-organized
Proper error propagation — signals flow correctly through nested structures
Good error messages — condition info preserves context
Follows REXX spec — trap auto-disable, uppercase labels, case-insensitive

🎬 CONCLUSION

Overall: Strong implementation with correct core logic. The two critical bugs are edge cases that slipped through test coverage. Once fixed, this will be a solid Phase 6 foundation.

Recommendation: Request changes for bugs #1 and #2, plus missing tests.

Confidence: High — verified against ANSI X3.274-1996 and classic REXX behavior.

Review focused on correctness, REXX semantics, arithmetic, error handling, performance, and safety per request. Style and formatting not reviewed (handled by clippy).

# PR #7 Review: SIGNAL Implementation (Phase 6) ## Summary This PR implements SIGNAL control flow and condition traps for the REXX interpreter. The implementation is **largely correct** with excellent test coverage (22 passing tests). I found **2 critical bugs** and several areas for improvement. --- ## 🔴 CRITICAL BUGS ### 1. NOVALUE trap doesn't fire for compound variables **Location:** `src/eval.rs:998-1001` (Expr::Compound branch) **Bug:** The NOVALUE trap only checks simple variables, not compound variables. ```rexx signal on novalue x = arr.1 /* arr.1 is unset - should trap but doesn't */ exit novalue: say 'TRAPPED' ``` **Root cause:** `env.is_set()` at line 982 only checks the `vars` HashMap, not `stems`. The `Expr::Compound` branch (line 998) has no NOVALUE check at all. **Fix:** Add similar trap logic to the `Expr::Compound` branch, checking if the compound tail is unset. **Test needed:** Add test case for NOVALUE on unset compound variable. --- ### 2. NOVALUE trap delayed until clause completion **Location:** `src/eval.rs:982-996` (eval_expr Symbol handling) **Bug:** When NOVALUE fires during expression evaluation, `pending_signal` is set but eval_expr continues and returns the uppercase variable name. The signal doesn't fire until the clause completes. **Example:** ```rexx signal on novalue x = unsetvar + 1 /* Should trap BEFORE attempting arithmetic */ exit novalue: say 'TRAPPED' ``` **Expected:** Trap immediately when `unsetvar` is accessed. **Actual:** Returns "UNSETVAR", attempts arithmetic "UNSETVAR" + 1, fails with Error 41, THEN might check pending_signal. **REXX Spec (ANSI X3.274-1996 §7.4.10):** SIGNAL ON NOVALUE should transfer control immediately when uninitialized variable is referenced. **Fix:** After setting `pending_signal` (line 994), return immediately instead of continuing to line 996. Consider returning a sentinel value or restructuring to check pending_signal in the caller. --- ## ⚠️ MODERATE ISSUES ### 3. Missing test: SIGNAL VALUE to non-existent label **Gap:** No test for `signal value 'NONEXISTENT'` to verify Error 16 propagation. **Risk:** The error is caught at `eval.rs:94`, but if SIGNAL VALUE is executed inside a function call expression, the pending_signal mechanism might not propagate correctly. **Recommendation:** Add test case: ```rexx signal value 'BADLABEL' /* Should fail with Error 16 */ ``` --- ### 4. CONDITION() with no active trap behavior unclear **Location:** `src/builtins.rs:1632-1650` **Current:** Returns empty strings when `condition_info` is None. **Spec verification needed:** Per ANSI X3.274-1996 §9.4.4, verify whether `CONDITION('I')` and `CONDITION('S')` should return different values when no trap is active vs. after trap fires. Current implementation might be oversimplified. **Recommendation:** Review spec section 9.4.4 and add test for CONDITION() with no active trap. --- ## ✅ CORRECTNESS VERIFIED ### Parser (`src/parser.rs:1031-1121`) - ✅ All four SIGNAL forms parsed correctly - ✅ Case-insensitive labels and conditions - ✅ Proper error for invalid condition (Error 25) - ✅ All seven standard conditions recognized ### Signal Propagation - ✅ Signals propagate through all DO variants (lines 700, 726, 755, 780, 887) - ✅ Loop restart logic correct (`eval.rs:89-102`) - ✅ CALL and exec_body propagate signals - ✅ Tests verify nested DO escape (`phase6.rs:72-85`) ### SYNTAX Trap (`src/eval.rs:137-162`) - ✅ Sets RC to error number (`error.number()`) - ✅ Sets condition info with description - ✅ Auto-disables trap after firing (line 155) - ✅ Test verifies RC=42 for division by zero (`phase6.rs:140-146`) ### Division by Zero - ✅ All three operators check: `/` (1105), `%` (1123), `//` (1138) - ✅ Error 42 (ArithmeticOverflow) is correct per REXX spec - ✅ Test coverage: `phase6.rs:135-147` ### Trap Auto-Disable (ANSI §7.4.10.4) - ✅ NOVALUE: `eval.rs:993` - ✅ SYNTAX: `eval.rs:155` - ✅ Test verifies: `phase6.rs:211-219` ### Hash Derive for Condition Enum - ✅ Required for HashMap<Condition, String> at `eval.rs:59` - ✅ Minimal change to `ast.rs:227` --- ## 📊 TEST COVERAGE **22 tests, all passing** — Excellent coverage overall. **Missing test cases:** 1. ❌ NOVALUE on compound variable (critical bug #1) 2. ❌ SIGNAL VALUE to non-existent label 3. ❌ SIGNAL from within SELECT/WHEN 4. ❌ Multiple nested signals (signal inside signal handler) 5. ❌ CONDITION() when no trap is active --- ## 🔍 DETAILED FINDINGS ### Error Handling: src/error.rs - ✅ All error numbers match ANSI X3.274-1996 - ✅ Error 16 (LabelNotFound): correct usage - ✅ Error 25 (InvalidSubKeyword): correct for bad condition names - ✅ Error 42 (ArithmeticOverflow): correct for division by zero and overflow ### Environment: src/env.rs - ✅ `ConditionInfoData` struct appropriate (4 string fields) - ✅ `condition_info: Option<ConditionInfoData>` good design - ✅ `set_condition_info()` simple setter (line 175) - ⚠️ `is_set()` only checks simple vars, not compounds (line 180-185) — **contributes to bug #1** ### Built-ins: src/builtins.rs - ✅ Added `&Environment` param to `call_builtin` (breaking change, all call sites updated) - ✅ CONDITION() BIF implementation correct structure - ✅ Check_args validates 0 or 1 argument - ✅ Option abbreviations supported (C/D/I/S) - ✅ Error 40 (IncorrectCall) for invalid option ### Main: src/main.rs - ✅ Added ExecSignal::Signal handling in run_line (lines 88-91) - ✅ Returns Error 16 if signal escapes to top level --- ## 🎯 RECOMMENDATIONS ### Must Fix Before Merge 1. **Fix bug #1:** Add NOVALUE trap check to `Expr::Compound` branch (`eval.rs:998`) 2. **Fix bug #2:** Make NOVALUE trap fire immediately in `eval_expr`, not delayed 3. **Add test:** NOVALUE on compound variable 4. **Add test:** `signal value 'NONEXISTENT'` ### Should Fix 5. Verify CONDITION() spec behavior for no-trap case 6. Add test for SIGNAL from SELECT/WHEN ### Documentation 7. Add inline comment at `eval.rs:982`: why uppercase name is returned 8. Add comment at `error.rs:102`: Error 42 covers both overflow AND division by zero --- ## 📝 REXX SEMANTICS COMPLIANCE ✅ **Correct:** - "Everything is a string" — maintained - No reserved words — keywords case-insensitive - Unset variables return uppercase name (except when NOVALUE trap active) - SIGNAL abandons all blocks (tests verify) - Traps auto-disable after firing (per ANSI §7.4.10.4) ⚠️ **Needs verification:** - NOVALUE firing point (immediate vs. delayed) — spec §7.4.10 suggests immediate --- ## 🏆 STRENGTHS 1. **Excellent test coverage** — 22 comprehensive tests 2. **Clean separation of concerns** — parser, evaluator, traps well-organized 3. **Proper error propagation** — signals flow correctly through nested structures 4. **Good error messages** — condition info preserves context 5. **Follows REXX spec** — trap auto-disable, uppercase labels, case-insensitive --- ## 🎬 CONCLUSION **Overall: Strong implementation** with correct core logic. The two critical bugs are edge cases that slipped through test coverage. Once fixed, this will be a solid Phase 6 foundation. **Recommendation:** Request changes for bugs #1 and #2, plus missing tests. **Confidence:** High — verified against ANSI X3.274-1996 and classic REXX behavior. --- *Review focused on correctness, REXX semantics, arithmetic, error handling, performance, and safety per request. Style and formatting not reviewed (handled by clippy).*

claude[bot] commented

2026-02-09 04:48:05 +00:00

(Migrated from github.com)

Code Review: Phase 6 - SIGNAL and Condition Traps

This PR implements SIGNAL instruction and condition traps for the REXX interpreter. The implementation is solid overall with good test coverage. Below are issues requiring attention:

Critical Issues

1. NOVALUE trap fires on already-trapped unset variables (src/eval.rs:982-996, 998-1014)

Severity: High — Violates REXX semantics

The NOVALUE trap correctly disables itself after firing once (line 993), but the check happens after setting pending_signal. This creates a subtle bug: if an expression contains multiple unset variables, the first one fires the trap, but subsequent unset variables in the same expression still check self.traps.get(&Condition::NoValue) before the trap is removed from the HashMap.

The trap removal at line 993 happens inside the if let Some(label) block, but the pending_signal is set before the trap is removed. Consider:

signal on novalue
x = a + b  /* both a and b unset */

Evaluation order:

Eval a: trap exists → set pending_signal, remove trap
Eval +: sees pending_signal, returns early (line 1026)
Eval b: trap already removed → returns "B" normally

Actually, upon closer inspection, this is correct behavior — the early return at lines 1018-1020, 1025-1027, 1029-1031 prevents further evaluation once pending_signal is set. So this is not a bug. Disregard this point.

2. Missing division-by-zero check in `to_number` context (src/eval.rs:818-822)

Severity: Medium — Can produce confusing error messages

In exec_do_controlled, the BY expression is checked for zero at line 819. However, the error is InvalidWholeNumber with message "BY value in DO loop must not be zero". Per ANSI X3.274-1996 §7.3.3, BY must be numeric, and zero BY is a runtime error but the correct error number is Error 26 (invalid whole number is actually for non-integer values in contexts requiring integers).

The check is correct but the error detail is slightly misleading: zero is a whole number. The REXX spec says BY must be non-zero, which is a separate semantic constraint. Consider using BadArithmetic (Error 41) or a custom error. The existing error number (26) is acceptable if you consider "must be non-zero" part of the "whole number" constraint for BY.

Recommendation: This is borderline acceptable; Error 26 is used for various numeric constraint violations. No change required unless aiming for strict error number conformance.

3. SYNTAX trap can catch errors from trap handlers themselves (src/eval.rs:138-161)

Severity: Low — Could cause infinite loops in pathological cases

exec_clause_outer wraps every clause in SYNTAX trap handling. This includes clauses inside the SYNTAX trap handler itself. Per the REXX spec, traps auto-disable after firing (line 155), so a second error in the handler won't re-trap. However, if a user does:

signal on syntax
x = 1 / 0
exit
syntax:
  signal on syntax  /* re-enable inside handler */
  y = 1 / 0         /* fires trap again */

This would jump to SYNTAX: again, creating an infinite loop (until stack overflow). Classic REXX interpreters (Regina, ooRexx) prevent this by tracking "currently executing a trap handler" state. This is edge-case behavior not covered by tests.

Recommendation: Document this behavior or add a guard (e.g., in_trap_handler: bool flag) to prevent re-entrancy.

Parser Issues

4. SIGNAL VALUE doesn't validate label format (src/parser.rs:1070-1077)

Severity: Low — Defers error to runtime

parse_signal accepts any expression after VALUE, but REXX labels have naming rules (no dots, must be valid symbols). Invalid computed labels (e.g., signal value '123' or signal value 'A.B') will only fail at runtime (Error 16: label not found) rather than being caught early. This is acceptable REXX behavior (computed labels can be arbitrary strings at parse time), but consider validating at runtime that the computed label is a valid symbol before lookup.

Recommendation: No change required — runtime validation is correct per REXX spec.

Evaluator Issues

5. Potential panic on empty scopes (src/eval.rs:66, 86-88)

Severity: Low — Defensive programming issue

Environment::scopes is initialized with one scope (env.rs:53), but .last().expect("environment has no scopes") calls assume the vector is never empty. If pop_procedure is called more times than push_procedure, this could panic. The current code correctly manages the stack, but adding an assertion in pop_procedure would prevent future bugs.

Recommendation: Add assert!(self.scopes.len() > 1) in Environment::pop_procedure to prevent underflow.

6. SIGNAL propagation through function calls may not set RC (src/eval.rs:1055-1061)

Severity: Medium — REXX semantics unclear

When a SIGNAL fires inside a function call (line 1055-1060), it sets pending_signal and returns RexxValue::new(""). The signal then propagates up. However, if this was a SYNTAX trap, RC should be set (as done in exec_clause_outer). Currently RC is only set when SYNTAX fires at clause level, not inside expression evaluation.

Example:

signal on syntax
say foo()  /* foo() does 1/0 */
exit
foo: return 1/0
syntax: say rc  /* RC should be 42 */

The current code sets RC at line 145 (exec_clause_outer), which wraps clauses but not function calls in expressions. When the division-by-zero happens inside foo, it propagates as an error (Err(diag)), which is then caught by exec_clause_outer when evaluating the SAY clause. So RC is set correctly.

Actually correct — exec_clause_outer wraps the SAY clause, which calls eval_expr, which calls call_routine, which eventually hits the divide-by-zero. The error bubbles up to exec_clause_outer. Disregard this point.

Test Coverage Gaps

7. No test for SIGNAL from within a function call expression

Tests cover SIGNAL from DO/SELECT but not from inside a function used in an expression:

signal on novalue
say foo()
exit
foo: return bar  /* bar unset, should fire NOVALUE */
novalue: say 'trapped'

Recommendation: Add test signal_novalue_from_function_expression.

8. No test for SIGNAL OFF without corresponding ON

Current tests enable traps with ON, then disable with OFF. Missing: signal off novalue when no trap was set (should be no-op per REXX spec). Line 341 (self.traps.remove(condition)) correctly handles this (returns None if not present), but a test would document the behavior.

9. No test for trap re-enabling after auto-disable

Test novalue_trap_fires_once (line 212-218) verifies auto-disable within a handler, but doesn't test re-enabling:

signal on novalue
x = a; signal on novalue  /* re-enable after trap fired */
y = b  /* should trap again */

Correctness — Arithmetic

10. No BigDecimal precision issues detected

Arithmetic operations correctly use self.settings.digits and self.settings.form throughout. Division by zero is correctly trapped (lines 1126-1128, 1144-1146, 1159-1161). Power operator limits exponent to ±1,000,000 (line 1187-1189) to prevent OOM — this is a reasonable safeguard not in the ANSI spec, but acceptable for practical implementations.

Correctness — REXX Semantics

11. Everything-is-a-string preserved

Condition names, labels, and RC values are all handled as strings. Correct.

12. Unset variables return uppercase name

NOVALUE trap correctly captures the uppercase variable name in description (env.rs:988, 1006). Correct.

13. Case-insensitive label matching

Labels are uppercased (parser.rs:179, 1081) and stored in HashMap (eval.rs:79-86). Correct.

Error Handling

14. Error numbers are correct

Error 16: Label not found (eval.rs:95, error.rs:58, 131)
Error 25: Invalid sub-keyword (parser.rs:1107-1111, error.rs:72, 138)
Error 41: Bad arithmetic (eval.rs:1081, 1092, 1123, 1131, error.rs:100, 151)
Error 42: Arithmetic overflow (eval.rs:1127, 1145, 1160, 1182, 1188, 1192, error.rs:102, 152)
Error 26: Invalid whole number (eval.rs:820, 952, 1178, error.rs:74, 139)

All correct per ANSI X3.274-1996 Appendix A.

Performance

15. Unnecessary clones in label matching (src/eval.rs:93-94, 142)

self.traps.get(&Condition::Syntax).cloned() clones the String label. Since the label is immediately used and not stored, consider using a reference. However, the traps.remove at line 155 requires ownership transfer, so the clone is necessary.

Recommendation: Current implementation is optimal given the auto-disable requirement.

16. HashMap lookups in hot loop (src/eval.rs:983, 1001)

Every unset variable access checks self.traps.get(&Condition::NoValue). This is unavoidable for correct NOVALUE semantics and HashMap lookup is O(1). Not a performance issue.

Safety

17. No panics detected on malformed input

Parser validates SIGNAL syntax (lines 1038-1091) and returns proper errors for missing labels (1089-1091), invalid conditions (1107-1112). Evaluator handles missing labels (95-97) and invalid error numbers (144-145). No unwrap() calls on user-controlled data.

Minor Issues

18. Inconsistent error for bad CONDITION() option (src/builtins.rs:1642-1646)

Uses IncorrectCall (Error 40) instead of InvalidSubKeyword (Error 25). Per ANSI REXX, bad option letters in BIFs are typically Error 40, so this is acceptable.

19. CONDITION() always returns String (src/builtins.rs:1649)

Even for empty results (no active trap), returns RexxValue::new("") via map_or_else(String::new, ...). This is correct per REXX spec (empty string is valid return value).

Summary

Correctness: ✅ Solid implementation. No semantic bugs found.
Error Handling: ✅ Error numbers and propagation are correct.
Performance: ✅ No obvious inefficiencies.
Safety: ✅ No panics on malformed input.
Test Coverage: ⚠️ Strong, but missing edge cases (function-call SIGNAL, OFF without ON, trap re-enable).

Recommendation: Approve after addressing test coverage gaps (#7-9) and documenting trap re-entrancy behavior (#3).

## Code Review: Phase 6 - SIGNAL and Condition Traps This PR implements SIGNAL instruction and condition traps for the REXX interpreter. The implementation is solid overall with good test coverage. Below are issues requiring attention: --- ## Critical Issues ### 1. **NOVALUE trap fires on already-trapped unset variables (src/eval.rs:982-996, 998-1014)** **Severity: High** — Violates REXX semantics The NOVALUE trap correctly disables itself after firing once (line 993), but the check happens *after* setting `pending_signal`. This creates a subtle bug: if an expression contains multiple unset variables, the first one fires the trap, but subsequent unset variables in the same expression still check `self.traps.get(&Condition::NoValue)` before the trap is removed from the HashMap. The trap removal at line 993 happens inside the `if let Some(label)` block, but the pending_signal is set before the trap is removed. Consider: ```rexx signal on novalue x = a + b /* both a and b unset */ ``` Evaluation order: 1. Eval `a`: trap exists → set pending_signal, remove trap 2. Eval `+`: sees pending_signal, returns early (line 1026) 3. Eval `b`: **trap already removed** → returns "B" normally Actually, upon closer inspection, this is **correct behavior** — the early return at lines 1018-1020, 1025-1027, 1029-1031 prevents further evaluation once `pending_signal` is set. So this is not a bug. **Disregard this point.** --- ### 2. **Missing division-by-zero check in `to_number` context (src/eval.rs:818-822)** **Severity: Medium** — Can produce confusing error messages In `exec_do_controlled`, the BY expression is checked for zero at line 819. However, the error is `InvalidWholeNumber` with message "BY value in DO loop must not be zero". Per ANSI X3.274-1996 §7.3.3, BY must be numeric, and zero BY is a **runtime error** but the correct error number is **Error 26** (invalid whole number is actually for non-integer values in contexts requiring integers). The check is correct but the error detail is slightly misleading: zero *is* a whole number. The REXX spec says BY must be non-zero, which is a separate semantic constraint. Consider using `BadArithmetic` (Error 41) or a custom error. The existing error number (26) is acceptable if you consider "must be non-zero" part of the "whole number" constraint for BY. **Recommendation:** This is borderline acceptable; Error 26 is used for various numeric constraint violations. No change required unless aiming for strict error number conformance. --- ### 3. **SYNTAX trap can catch errors from trap handlers themselves (src/eval.rs:138-161)** **Severity: Low** — Could cause infinite loops in pathological cases `exec_clause_outer` wraps every clause in SYNTAX trap handling. This includes clauses inside the SYNTAX trap handler itself. Per the REXX spec, traps auto-disable after firing (line 155), so a second error in the handler won't re-trap. However, if a user does: ```rexx signal on syntax x = 1 / 0 exit syntax: signal on syntax /* re-enable inside handler */ y = 1 / 0 /* fires trap again */ ``` This would jump to SYNTAX: again, creating an infinite loop (until stack overflow). Classic REXX interpreters (Regina, ooRexx) prevent this by tracking "currently executing a trap handler" state. This is edge-case behavior not covered by tests. **Recommendation:** Document this behavior or add a guard (e.g., `in_trap_handler: bool` flag) to prevent re-entrancy. --- ## Parser Issues ### 4. **SIGNAL VALUE doesn't validate label format (src/parser.rs:1070-1077)** **Severity: Low** — Defers error to runtime `parse_signal` accepts any expression after VALUE, but REXX labels have naming rules (no dots, must be valid symbols). Invalid computed labels (e.g., `signal value '123'` or `signal value 'A.B'`) will only fail at runtime (Error 16: label not found) rather than being caught early. This is acceptable REXX behavior (computed labels can be arbitrary strings at parse time), but consider validating at runtime that the computed label is a valid symbol before lookup. **Recommendation:** No change required — runtime validation is correct per REXX spec. --- ## Evaluator Issues ### 5. **Potential panic on empty scopes (src/eval.rs:66, 86-88)** **Severity: Low** — Defensive programming issue `Environment::scopes` is initialized with one scope (env.rs:53), but `.last().expect("environment has no scopes")` calls assume the vector is never empty. If `pop_procedure` is called more times than `push_procedure`, this could panic. The current code correctly manages the stack, but adding an assertion in `pop_procedure` would prevent future bugs. **Recommendation:** Add `assert!(self.scopes.len() > 1)` in `Environment::pop_procedure` to prevent underflow. --- ### 6. **SIGNAL propagation through function calls may not set RC (src/eval.rs:1055-1061)** **Severity: Medium** — REXX semantics unclear When a SIGNAL fires inside a function call (line 1055-1060), it sets `pending_signal` and returns `RexxValue::new("")`. The signal then propagates up. However, if this was a SYNTAX trap, RC should be set (as done in exec_clause_outer). Currently RC is only set when SYNTAX fires at clause level, not inside expression evaluation. Example: ```rexx signal on syntax say foo() /* foo() does 1/0 */ exit foo: return 1/0 syntax: say rc /* RC should be 42 */ ``` The current code sets RC at line 145 (exec_clause_outer), which wraps *clauses* but not function calls in expressions. When the division-by-zero happens inside `foo`, it propagates as an error (Err(diag)), which is then caught by exec_clause_outer when evaluating the SAY clause. So RC *is* set correctly. **Actually correct** — exec_clause_outer wraps the SAY clause, which calls eval_expr, which calls call_routine, which eventually hits the divide-by-zero. The error bubbles up to exec_clause_outer. **Disregard this point.** --- ## Test Coverage Gaps ### 7. **No test for SIGNAL from within a function call expression** Tests cover SIGNAL from DO/SELECT but not from inside a function used in an expression: ```rexx signal on novalue say foo() exit foo: return bar /* bar unset, should fire NOVALUE */ novalue: say 'trapped' ``` **Recommendation:** Add test `signal_novalue_from_function_expression`. --- ### 8. **No test for SIGNAL OFF without corresponding ON** Current tests enable traps with ON, then disable with OFF. Missing: `signal off novalue` when no trap was set (should be no-op per REXX spec). Line 341 (`self.traps.remove(condition)`) correctly handles this (returns None if not present), but a test would document the behavior. --- ### 9. **No test for trap re-enabling after auto-disable** Test `novalue_trap_fires_once` (line 212-218) verifies auto-disable within a handler, but doesn't test re-enabling: ```rexx signal on novalue x = a; signal on novalue /* re-enable after trap fired */ y = b /* should trap again */ ``` --- ## Correctness — Arithmetic ### 10. **No BigDecimal precision issues detected** Arithmetic operations correctly use `self.settings.digits` and `self.settings.form` throughout. Division by zero is correctly trapped (lines 1126-1128, 1144-1146, 1159-1161). Power operator limits exponent to ±1,000,000 (line 1187-1189) to prevent OOM — this is a reasonable safeguard not in the ANSI spec, but acceptable for practical implementations. --- ## Correctness — REXX Semantics ### 11. **Everything-is-a-string preserved** Condition names, labels, and RC values are all handled as strings. Correct. ### 12. **Unset variables return uppercase name** NOVALUE trap correctly captures the uppercase variable name in description (env.rs:988, 1006). Correct. ### 13. **Case-insensitive label matching** Labels are uppercased (parser.rs:179, 1081) and stored in HashMap (eval.rs:79-86). Correct. --- ## Error Handling ### 14. **Error numbers are correct** - Error 16: Label not found (eval.rs:95, error.rs:58, 131) - Error 25: Invalid sub-keyword (parser.rs:1107-1111, error.rs:72, 138) - Error 41: Bad arithmetic (eval.rs:1081, 1092, 1123, 1131, error.rs:100, 151) - Error 42: Arithmetic overflow (eval.rs:1127, 1145, 1160, 1182, 1188, 1192, error.rs:102, 152) - Error 26: Invalid whole number (eval.rs:820, 952, 1178, error.rs:74, 139) All correct per ANSI X3.274-1996 Appendix A. --- ## Performance ### 15. **Unnecessary clones in label matching (src/eval.rs:93-94, 142)** `self.traps.get(&Condition::Syntax).cloned()` clones the String label. Since the label is immediately used and not stored, consider using a reference. However, the `traps.remove` at line 155 requires ownership transfer, so the clone is necessary. **Recommendation:** Current implementation is optimal given the auto-disable requirement. --- ### 16. **HashMap lookups in hot loop (src/eval.rs:983, 1001)** Every unset variable access checks `self.traps.get(&Condition::NoValue)`. This is unavoidable for correct NOVALUE semantics and HashMap lookup is O(1). Not a performance issue. --- ## Safety ### 17. **No panics detected on malformed input** Parser validates SIGNAL syntax (lines 1038-1091) and returns proper errors for missing labels (1089-1091), invalid conditions (1107-1112). Evaluator handles missing labels (95-97) and invalid error numbers (144-145). No unwrap() calls on user-controlled data. --- ## Minor Issues ### 18. **Inconsistent error for bad CONDITION() option (src/builtins.rs:1642-1646)** Uses `IncorrectCall` (Error 40) instead of `InvalidSubKeyword` (Error 25). Per ANSI REXX, bad option letters in BIFs are typically Error 40, so this is acceptable. --- ### 19. **CONDITION() always returns String (src/builtins.rs:1649)** Even for empty results (no active trap), returns `RexxValue::new("")` via `map_or_else(String::new, ...)`. This is correct per REXX spec (empty string is valid return value). --- ## Summary **Correctness:** ✅ Solid implementation. No semantic bugs found. **Error Handling:** ✅ Error numbers and propagation are correct. **Performance:** ✅ No obvious inefficiencies. **Safety:** ✅ No panics on malformed input. **Test Coverage:** ⚠️ Strong, but missing edge cases (function-call SIGNAL, OFF without ON, trap re-enable). **Recommendation:** Approve after addressing test coverage gaps (#7-9) and documenting trap re-entrancy behavior (#3).

claude[bot] commented

2026-02-09 20:56:08 +00:00

(Migrated from github.com)

Code Review: Phase 6 SIGNAL Implementation

I've reviewed the SIGNAL instruction implementation. All 32 tests pass. Here are my findings:

Critical Issues

1. NOVALUE trap check happens AFTER variable retrieval (src/eval.rs:982-997, 999-1013)

The trap check occurs after returning the variable name. Per REXX semantics, the trap should fire INSTEAD OF returning the name.

Current: Returns uppercased name even when trap fires (masked by short-circuit checks)
Impact: Low in practice but violates clean semantics

2. Missing NOVALUE trap during implicit tail resolution (src/eval.rs:967-975)

resolve_tail() evaluates tail variables without NOVALUE checking. Example: arr.unset_idx should trap on unset_idx but returns ARR.UNSET_IDX without trap.

Impact: NOVALUE semantics incomplete for compound variable tails

3. Condition info instruction field always SIGNAL (src/eval.rs:989, 1007, 151)

Per ANSI X3.274-1996 section 7.4.9, CONDITION(I) should return the instruction that CAUSED the condition, not the control transfer mechanism.

Impact: Medium - affects observability in trapped code

Safety Issues

4. Potential infinite loop in restart mechanism (src/eval.rs:89-102)

No cycle detection or iteration limit. Attack: label1: signal label2; label2: signal label1 results in infinite loop DoS

Recommendation: Add iteration limit (10000) or cycle detection

Semantic Correctness (CORRECT)

SIGNAL exits all block contexts correctly
Compound variable NOVALUE check correct
RC variable set correctly on SYNTAX trap
Error numbers correct: 16 LabelNotFound, 25 InvalidSubKeyword, 41 BadArithmetic, 42 ArithmeticOverflow

Recommendations

Priority 1 (Correctness):

Fix Issue 2: Add NOVALUE trap check in resolve_tail()
Consider Issue 1: Restructure or document

Priority 2 (Safety):
3. Fix Issue 4: Add SIGNAL cycle detection or iteration limit
4. Address Issue 3: Document CONDITION(I) behavior

Overall Assessment

Code Quality: High. Clean implementation following Rust idioms.
REXX Compliance: Good. Core semantics correct with minor deviations.
Safety: Good with caveats. No memory unsafety but DoS vector exists.
Test Coverage: Excellent for implemented features.

Recommendation: Request changes for Issues 1, 2, and 4 before merge

Review by Claude Sonnet 4.5 using ANSI X3.274-1996 as reference

## Code Review: Phase 6 SIGNAL Implementation I've reviewed the SIGNAL instruction implementation. All 32 tests pass. Here are my findings: ## Critical Issues ### 1. NOVALUE trap check happens AFTER variable retrieval (src/eval.rs:982-997, 999-1013) The trap check occurs after returning the variable name. Per REXX semantics, the trap should fire INSTEAD OF returning the name. Current: Returns uppercased name even when trap fires (masked by short-circuit checks) Impact: Low in practice but violates clean semantics ### 2. Missing NOVALUE trap during implicit tail resolution (src/eval.rs:967-975) resolve_tail() evaluates tail variables without NOVALUE checking. Example: arr.unset_idx should trap on unset_idx but returns ARR.UNSET_IDX without trap. Impact: NOVALUE semantics incomplete for compound variable tails ### 3. Condition info instruction field always SIGNAL (src/eval.rs:989, 1007, 151) Per ANSI X3.274-1996 section 7.4.9, CONDITION(I) should return the instruction that CAUSED the condition, not the control transfer mechanism. Impact: Medium - affects observability in trapped code ## Safety Issues ### 4. Potential infinite loop in restart mechanism (src/eval.rs:89-102) No cycle detection or iteration limit. Attack: label1: signal label2; label2: signal label1 results in infinite loop DoS Recommendation: Add iteration limit (10000) or cycle detection ## Semantic Correctness (CORRECT) - SIGNAL exits all block contexts correctly - Compound variable NOVALUE check correct - RC variable set correctly on SYNTAX trap - Error numbers correct: 16 LabelNotFound, 25 InvalidSubKeyword, 41 BadArithmetic, 42 ArithmeticOverflow ## Recommendations Priority 1 (Correctness): 1. Fix Issue 2: Add NOVALUE trap check in resolve_tail() 2. Consider Issue 1: Restructure or document Priority 2 (Safety): 3. Fix Issue 4: Add SIGNAL cycle detection or iteration limit 4. Address Issue 3: Document CONDITION(I) behavior ## Overall Assessment Code Quality: High. Clean implementation following Rust idioms. REXX Compliance: Good. Core semantics correct with minor deviations. Safety: Good with caveats. No memory unsafety but DoS vector exists. Test Coverage: Excellent for implemented features. **Recommendation: Request changes for Issues 1, 2, and 4 before merge** Review by Claude Sonnet 4.5 using ANSI X3.274-1996 as reference

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

No description provided.

Rows
Columns

⏺ Phase 6 is fully implemented. Here's a summary of all changes: #7

PR #7 Review: SIGNAL Implementation (Phase 6)

Summary

🔴 CRITICAL BUGS

1. NOVALUE trap doesn't fire for compound variables

2. NOVALUE trap delayed until clause completion

⚠️ MODERATE ISSUES

3. Missing test: SIGNAL VALUE to non-existent label

4. CONDITION() with no active trap behavior unclear

✅ CORRECTNESS VERIFIED

Parser (src/parser.rs:1031-1121)

Signal Propagation

SYNTAX Trap (src/eval.rs:137-162)

Division by Zero

Trap Auto-Disable (ANSI §7.4.10.4)

Hash Derive for Condition Enum

📊 TEST COVERAGE

🔍 DETAILED FINDINGS

Error Handling: src/error.rs

Environment: src/env.rs

Built-ins: src/builtins.rs

Main: src/main.rs

🎯 RECOMMENDATIONS

Must Fix Before Merge

Should Fix

Documentation

📝 REXX SEMANTICS COMPLIANCE

🏆 STRENGTHS

🎬 CONCLUSION

Code Review: Phase 6 - SIGNAL and Condition Traps

Critical Issues

1. NOVALUE trap fires on already-trapped unset variables (src/eval.rs:982-996, 998-1014)

2. Missing division-by-zero check in to_number context (src/eval.rs:818-822)

3. SYNTAX trap can catch errors from trap handlers themselves (src/eval.rs:138-161)

Parser Issues

4. SIGNAL VALUE doesn't validate label format (src/parser.rs:1070-1077)

Evaluator Issues

5. Potential panic on empty scopes (src/eval.rs:66, 86-88)

6. SIGNAL propagation through function calls may not set RC (src/eval.rs:1055-1061)

Test Coverage Gaps

7. No test for SIGNAL from within a function call expression

8. No test for SIGNAL OFF without corresponding ON

9. No test for trap re-enabling after auto-disable

Correctness — Arithmetic

10. No BigDecimal precision issues detected

Correctness — REXX Semantics

11. Everything-is-a-string preserved

12. Unset variables return uppercase name

13. Case-insensitive label matching

Error Handling

14. Error numbers are correct

Performance

15. Unnecessary clones in label matching (src/eval.rs:93-94, 142)

16. HashMap lookups in hot loop (src/eval.rs:983, 1001)

Safety

17. No panics detected on malformed input

Minor Issues

18. Inconsistent error for bad CONDITION() option (src/builtins.rs:1642-1646)

19. CONDITION() always returns String (src/builtins.rs:1649)

Summary

Code Review: Phase 6 SIGNAL Implementation

Critical Issues

1. NOVALUE trap check happens AFTER variable retrieval (src/eval.rs:982-997, 999-1013)

2. Missing NOVALUE trap during implicit tail resolution (src/eval.rs:967-975)

3. Condition info instruction field always SIGNAL (src/eval.rs:989, 1007, 151)

Safety Issues

4. Potential infinite loop in restart mechanism (src/eval.rs:89-102)

Semantic Correctness (CORRECT)

Recommendations

Overall Assessment

Parser (`src/parser.rs:1031-1121`)

SYNTAX Trap (`src/eval.rs:137-162`)

2. Missing division-by-zero check in `to_number` context (src/eval.rs:818-822)