⏺ Phase 2 is complete. Here's a summary of what was implemented: #3
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
rust
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
navicore/patch-rexx!3
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "phase2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Changes Made
src/parser.rs — Control flow parsing
src/eval.rs — Control flow execution
src/main.rs — Signal handling
tests/phase2.rs — 26 integration tests
PR #3 Review: Phase 2 Control Flow Implementation
Summary:
This PR implements REXX control flow (IF/THEN/ELSE, DO variants, SELECT/WHEN, LEAVE, ITERATE, EXIT, RETURN). The implementation adds ~1200 lines across parser, evaluator, and tests.
Overall Assessment: Strong implementation with excellent test coverage. I identified 4 correctness issues that need fixing before merge.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
CRITICAL ISSUES (MUST FIX)
Controlled loop allows zero BY step - causes infinite loop
Location: src/eval.rs:295-301
Severity: HIGH
Problem: Zero step should raise Error 26, not loop forever.
REXX ANSI X3.274-1996 §7.3.5 requires BY ≠ 0.
Fix: Add check before loop:
if by_num.is_zero() {
return Err(RexxDiagnostic::new(RexxError::InvalidWholeNumber)
.with_detail("BY value must be non-zero"));
}
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
to_integer allows negative values for loop counts
Location: src/eval.rs:408-416
Severity: HIGH
Problems:
Fix: Add negative check and use ArithmeticOverflow for parse failure
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Power operator lacks exponent bounds checking
Location: src/eval.rs:548-570
Severity: MEDIUM
Problem: No range validation. Input like 10 ** 1000000000 will hang.
Fix: Add bounds check after parsing exp_i64:
if exp_i64.abs() > 999999999 {
return Err(Error 42 with "exponent exceeds limits");
}
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
trunc_div string parsing may fail on scientific notation
Location: src/eval.rs:690-699
Severity: MEDIUM
Problem: BigDecimal::to_string() may use scientific notation.
Parsing "1.5e10" will fail to find period.
Fix: Use BigDecimal rounding instead of string parsing
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
STRENGTHS
✅ Signal propagation is correct
ExecSignal enum correctly implements REXX semantics
(LEAVE/ITERATE match by name, EXIT/RETURN always propagate)
✅ Cross-line IF/THEN/ELSE works properly
Parser correctly handles THEN/ELSE across line boundaries
with position backtracking (src/parser.rs:323-335)
✅ Error numbers match ANSI X3.274-1996
All error codes correct: 7, 8, 10, 28
✅ Test coverage is excellent
26 integration tests including FizzBuzz, factorial, nested loops,
error cases. Only gap: no test for negative BY step.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
QUESTIONS
• UNTIL variable behavior (src/eval.rs:346-363)
Code increments control variable when UNTIL becomes true.
Is this standard REXX? Most implementations leave variable
at value that satisfied UNTIL, not incremented.
Verify against ANSI §7.3.5.
• Parser keyword checks (src/parser.rs:145)
is_do_header_keyword() calls to_uppercase() on every check.
Could use eq_ignore_ascii_case. Not critical.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
RECOMMENDATIONS
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
VERDICT
This is a well-architected implementation of REXX control flow.
The parser context flags for keyword suppression are clever,
and the signal propagation is clean.
The critical issues are all arithmetic edge cases that need
fixing to prevent errors on unusual input. Once addressed,
this is ready to merge.
Estimated fix time: 1-2 hours + tests
Code Review: Phase 2 Control Flow Implementation
This PR implements IF/THEN/ELSE, DO loops (all variants), SELECT/WHEN/OTHERWISE, LEAVE/ITERATE, EXIT/RETURN, and signal propagation. I've reviewed for correctness, REXX semantics, arithmetic, error handling, performance, and safety.
✅ What's Working Well
Parser Architecture:
do_header_depth,condition_depth,if_depth) correctly prevent keyword consumption by implicit concatenation (src/parser.rs:17-25, 787-803)skip_terminators()and position restoration (src/parser.rs:326-338)Controlled Loop Semantics:
Signal Propagation:
Arithmetic:
trunc_divfixed to usewith_scale_round(0, Down)instead of string parsing (src/eval.rs:701-706) - this avoids scientific notation bugsTest Coverage:
🔴 Critical Issues
1. Division by Zero Returns Wrong Error Number (src/eval.rs:510, 528, 543)
Issue: Division by zero returns
RexxError::ArithmeticOverflow(Error 42), but per ANSI X3.274-1996, Error 42 is "Arithmetic overflow/underflow" which typically means result exceeds NUMERIC DIGITS. Division by zero should arguably be Error 41 ("Bad arithmetic conversion") or a distinct error.Location: src/eval.rs:510, 528, 543
Impact: Incorrect error reporting for a common arithmetic error.
Recommendation: Check ANSI spec for correct error number for division by zero. If Error 42 is correct for REXX, add clarifying comment; otherwise, use Error 41.
2. Iteration Counter Overflow Risk in Controlled Loops (src/eval.rs:295, 363)
Issue:
iterationsisi64and incremented unconditionally (line 363). A malicious or buggy program with a massive FOR value or infinite TO range could overflowiterations, causing silent wraparound or panic in debug mode.Location: src/eval.rs:295, 363
Proof of Concept:
Impact: Potential panic (debug) or silent wraparound (release).
Recommendation:
iterationsapproachingi64::MAXbefore incrementi64::MAXiterations = iterations.saturating_add(1)3.
to_integerAllows Fractional Loop Counts Without Error (src/eval.rs:410-423)Issue:
to_integerrounds fractional values withd.round(0)(line 412) and silently accepts them. Per REXX semantics, loop counts and FOR values should be whole numbers. If a user writesdo 3.7, REXX should raise Error 26 ("Invalid whole number"), not silently round to 4.Location: src/eval.rs:410-423
Test Case:
Impact: Silent data loss; user may not realize their fractional count was rounded.
Recommendation: Check if
d \!= roundedand return Error 26 if fractional part exists. Example:4.
pow_bigdecimalCan Produce Massive Intermediate Values (src/eval.rs:709-729)Issue: Exponentiation by squaring (line 720-727) with large exponents can create intermediate
BigDecimalvalues that consume unbounded memory. Example:2 ** 999999999passes the exponent limit check (line 568-571) but will still allocate enormous memory during computation.Location: src/eval.rs:709-729
Proof of Concept:
Impact: Out-of-memory crash or DoS.
Recommendation:
result.digits() > MAX_DIGITS)5. Parser Does Not Check for Duplicate TO/BY/FOR/WHILE/UNTIL in Controlled DO (src/parser.rs:463-483)
Issue:
parse_controlled_doallows duplicate keywords:The last occurrence silently overwrites earlier ones. Per REXX, duplicate keywords should raise Error 27 ("Invalid DO syntax").
Location: src/parser.rs:463-483
Impact: Silent acceptance of malformed syntax.
Recommendation: Track which keywords have been seen and return Error 27 if any are repeated.
⚠️ Moderate Issues
6. SELECT Without OTHERWISE Can Execute Zero Branches (src/eval.rs:389)
Issue: If no WHEN matches and there's no OTHERWISE, the code correctly returns Error 7, but the error message says "no WHEN matched" which could be clearer.
Location: src/eval.rs:389-390
Recommendation: Error message is adequate but could add "(add OTHERWISE branch or ensure a WHEN matches)" for user guidance.
7. Controlled Loop Re-evaluates WHILE on Every Iteration (src/eval.rs:323-329)
Issue: WHILE condition is evaluated before each iteration. This is correct per REXX semantics, but the code does not cache
while_expr. If the expression is expensive, this could be a performance issue. However, this is the correct behavior per ANSI REXX, so not a bug.Location: src/eval.rs:323-329
Recommendation: No change needed; this is correct REXX behavior. Just noting for completeness.
8.
trunc_divTruncates Toward Zero, But Comment Could Be Clearer (src/eval.rs:701-706)Issue: The fix to use
with_scale_round(0, Down)is correct, butDownin BigDecimal means "toward zero" (not "toward negative infinity"). The comment "Truncate toward zero" is correct. However, it could clarify thatDownis the correct rounding mode for this.Location: src/eval.rs:705
Recommendation: Add comment:
// Note: RoundingMode::Down truncates toward zero (not toward negative infinity)🟡 Minor Issues / Style
9.
exec_do_untilITERATE Logic Has Subtle Control Flow (src/eval.rs:236-242)Issue: When ITERATE matches, the code "falls through to condition check" (line 238). This is correct, but the logic is slightly hard to follow because the ITERATE arm has a comment but no explicit control flow. Consider restructuring for clarity.
Location: src/eval.rs:236-242
Recommendation:
10.
#[allow(clippy::cmp_owned)]in Controlled Loop (src/eval.rs:300)Issue: Line 309 compares
¤t > limitwherecurrentisBigDecimalandlimitis&BigDecimal. This triggersclippy::cmp_owned. The allow is correct, but the comparison could be rewritten ascurrent > *limitto avoid the lint.Location: src/eval.rs:300, 309
Recommendation: Minor - the allow is fine, or rewrite as
current > *limit.11.
leave_matchesanditerate_matchesAre Identical (src/eval.rs:393-407)Issue: Both functions have identical logic. Consider a single
signal_matcheshelper.Location: src/eval.rs:393-407
Recommendation: Refactor to DRY:
Use for both LEAVE and ITERATE.
🔍 Edge Cases to Test
The following cases are not covered by current tests but should work correctly (or fail gracefully):
do i = 1 by 2; if i > 100 then leave; end(should run forever until LEAVE)do foo=1 to 5; leave bar; end(should propagate signal upward)📊 Performance Notes
Vecallocations:parse_do_bodyandexec_bodyallocate vectors, but this is unavoidable for the AST structurecurrentevery iteration (line 362). For very large loops, this is unavoidable but could be slow. Acceptable for a REXX interpreter.🛡️ Safety
Summary
This is a high-quality implementation of REXX control flow with correct semantics and excellent test coverage. The critical issues are:
Address these before merge. The moderate/minor issues are nice-to-haves.
Test Coverage: ✅ Excellent (30 tests, edge cases covered)
Error Handling: ⚠️ Good, but division by zero and loop count validation need fixes
REXX Compliance: ✅ Correct (UNTIL semantics, final variable value, named LEAVE/ITERATE)
Safety: ✅ Good (no panics on bad input, but watch for overflow/OOM)
Great work overall! The architecture (depth counters, signal propagation) is clean and correct. Fix the critical issues and this will be a solid implementation.
Code Review: Phase 2 Control Flow Implementation
I've reviewed this PR implementing control flow for the REXX interpreter. The implementation demonstrates solid understanding of REXX semantics and proper error handling. Below are my findings:
✅ Correctness — STRONG
Parser Edge Cases:
do_header_depth,condition_depth,if_depth)skip_terminators()and position restorationREXX Semantics:
✅ Arithmetic — EXCELLENT
BigDecimal Usage:
2**999999999— great addition (src/eval.rs:568)trunc_divnow useswith_scale_round(0, Down)instead of fragile string parsing (src/eval.rs:706)BigDecimal::to_string()Loop Count Validation:
✅ Error Handling — CORRECT
All error numbers match ANSI X3.274-1996:
Propagation:
RexxResult<ExecSignal>— signal and error paths cleanly separatedmain.rscorrectly accepts Normal/Exit/Return, rejects Leave/Iterate with Error 28⚠️ ISSUE 1: ITERATE in DO UNTIL — Potential Bug (src/eval.rs:236-249)
Location: src/eval.rs:236-249
Problem: When ITERATE matches the loop name, control falls through to the UNTIL check. But per ANSI REXX, ITERATE should restart the loop from the top, checking the UNTIL condition after the next iteration's body.
Test Case:
Expected: Output
1 3 4(body runs 4 times, ITERATE skips SAY on iteration 2)Current behavior: Likely correct by accident, but logic suggests UNTIL is checked immediately after ITERATE
Fix: After matched ITERATE in DO UNTIL, use
continueto restart loop:Actually, reviewing the code more carefully: the current implementation may be correct if the intent is that ITERATE in UNTIL immediately checks the condition. I recommend adding an integration test to verify the exact behavior matches a reference REXX interpreter.
⚠️ ISSUE 2: Controlled Loop UNTIL Semantics (src/eval.rs:350-357)
Location: src/eval.rs:350-357
Comment says "loop ends without incrementing", but the code then increments at line 361.
Ambiguity: Does "without incrementing" mean:
Recommendation: Verify against ANSI spec section on controlled DO with UNTIL. If current behavior is correct, rephrase comment to "loop ends, then variable is incremented and set to final value" to clarify that the increment still happens after the break for final value assignment.
Test case to verify:
Expected: If UNTIL satisfied on iteration where i=5, should final value be 5 or 7?
✅ Performance — GOOD
Minimal Allocations:
RexxValueclones limited to leaf nodes (StringLit, Number) — acceptablePotential Optimizations (not blockers):
name.clone()in LEAVE/ITERATE — could useRc<String>or lifetime annotation, but premature optimizationbase.clone()inpow_bigdecimal— necessary for exponentiation by squaring✅ Safety — EXCELLENT
✅ Test Coverage — COMPREHENSIVE
tests/phase2.rs provides 26 tests covering:
Missing edge case tests (recommend adding):
Summary
Overall: This is high-quality, production-ready code with only two minor ambiguities to clarify:
Strengths:
Recommendation: ✅ Approve with minor follow-up — address the two semantic questions above in a follow-up commit or confirm current behavior is correct.
🤖 Generated with Claude Code
Code Review: Phase 2 Control Flow Implementation
I've completed a thorough review of PR #3, focusing on correctness, REXX semantics, arithmetic, error handling, and safety. Overall, this is solid work with excellent attention to REXX spec compliance. All 36 tests pass.
Critical Issues
1. Controlled loop with WHILE: incorrect final variable value (src/eval.rs:318-371)
Location:
exec_do_controlledlines 318-371When a controlled loop exits due to a WHILE condition failure, the control variable is set BEFORE checking WHILE (line 318-321), then WHILE is checked (line 324-329), and if it fails we break. However, the final assignment at line 368-371 unconditionally overwrites the variable with
currentagain.Problem: When WHILE causes early termination, the variable gets set twice with the same value. This is correct by accident but semantically wrong — the comment at line 366-367 says "may be one-past-limit for TO termination", but WHILE termination should leave the variable at the value that failed the test, not re-set it.
Test case:
Expected output:
1 2 3 4 final=5(i=5 fails WHILE, so loop ends with i=5)Actual output:
1 2 3 4 final=5✓ (works but for wrong reason)Root cause: The loop sets
i, body runs, increment happens (line 362), then next iteration setsiagain. When WHILE fails at i=5, we break, and the final assignment (line 368-371) sets it to 5 again. This works but the logic is convoluted.Fix: The final assignment should be conditional or the loop should track which exit path was taken.
2. Incorrect error number for BY zero (src/eval.rs:280-281)
Location: Line 280-281
Problem: REXX spec uses Error 26 (InvalidWholeNumber) for "value is not a whole number". Zero IS a whole number, so this is the wrong error. Per ANSI X3.274-1996 §7.3.5, a zero BY step should be Error 27 (InvalidDoSyntax).
Fix: Use
RexxError::InvalidDoSyntaxinstead.Correctness Issues
3.
trunc_divuses wrong rounding mode (src/eval.rs:708)Location: Line 708
Problem:
RoundingMode::Downrounds toward negative infinity (floor), not toward zero. For negative quotients, this gives wrong results.Test case:
Actual output:
-3✓ (I tested this — so either BigDecimal's docs are unclear or this is lucky)Note: The comment says "Down truncates toward zero" but that's not what RoundingMode::Down does in most contexts. You want
RoundingMode::ToZeroif it exists, or explicit sign handling.Verification needed: Confirm BigDecimal's RoundingMode::Down semantics match REXX's truncation rules.
REXX Semantics
4. Good: Error 28 for LEAVE/ITERATE outside loop (src/main.rs:499-502)
Correctly catches stray LEAVE/ITERATE at top level. ✓
5. Good: UNTIL semantics (src/eval.rs:350-359)
Correctly implements UNTIL: body executes first, then condition is checked. ITERATE skips to UNTIL check. Variable retains current value when UNTIL succeeds (no increment). This matches ANSI spec §7.3.5. ✓
6. Good: Final variable value for TO loops (tests/phase2.rs:1310, 1525)
Tests confirm that after
DO i = 1 TO 5,iis 6 (one-past-limit). This is correct REXX behavior. ✓Arithmetic & BigDecimal
7. Good: Division by zero checks (src/eval.rs:509, 527, 542)
All three division operators (/, %%, %) check for zero divisor and raise Error 42. ✓
8. Good: Exponent overflow protection (src/eval.rs:570-573)
Limits exponents to ±1,000,000 to prevent OOM from
2 ** 9999999999. This is a pragmatic safeguard. ✓9. Good: Zero to negative power (src/eval.rs:574-577)
Correctly rejects
0 ** -1with Error 42. ✓10. Good: Loop count validation (src/eval.rs:406-431)
to_integercorrectly:All appropriate. ✓
Error Handling
11. Good: All error numbers are correct
All match ANSI spec. ✓
Safety & Edge Cases
12. Good: Parser depth guards prevent keyword capture
The
do_header_depth,condition_depth, andif_depthflags (src/parser.rs:526-546, 810-1110) correctly prevent implicit concatenation from consuming keywords like TO, BY, THEN, ELSE. This is a subtle and important correctness issue. ✓13. Good: Signal propagation
EXIT and RETURN always propagate up (src/eval.rs:175, 347). LEAVE/ITERATE only match by name (src/eval.rs:164-173, 335-346). Correct per spec. ✓
14. Good: No panics on malformed input
I tested several edge cases:
DO 9999999999999999999→ Error 42 (not panic)SELECT; WHEN 0 THEN SAY 'x'; END→ Error 7 (not panic)All handled gracefully. ✓
15. Good: Cross-line IF/ELSE parsing (tests/phase2.rs:1436-1444)
Tests confirm that THEN and ELSE can appear on separate lines from IF. The parser's
skip_terminators()calls handle this correctly. ✓Minor Observations
16. Unused return value (src/eval.rs:86)
Line 86:
let _val = self.eval_expr(expr)?;— the underscore prefix is good Rust style for intentionally ignored values. ✓17. Exponentiation by squaring (src/eval.rs:712-732)
pow_bigdecimaluses the standard fast exponentiation algorithm. Efficient and correct. ✓18. Comparison operators (src/eval.rs:686-700)
normal_comparecorrectly implements REXX comparison:This matches ANSI §2.5. ✓
Test Coverage
The test suite (
tests/phase2.rs) is excellent:All pass. ✓
Recommendations
trunc_divrounding mode (issue #3) — verify BigDecimal behavior or switch to explicit handling-10 % 3to verify integer division/remainder with negative operandsSummary
This is high-quality work with strong attention to REXX semantics. The parser's keyword suppression mechanism is clever and necessary. The error handling is thorough and matches the spec. The test coverage is comprehensive.
The two critical issues (#1 and #2) should be fixed before merge, but they don't affect current test results. Issue #3 needs verification.
Excellent job on Phase 2! 🚀
Code Review: Phase 2 Control Flow Implementation
I've completed a thorough review of PR #3 focusing on correctness, REXX semantics, arithmetic handling, error handling, performance, and safety. Here are my findings:
Critical Issues
1. Controlled Loop Increment Overflow (eval.rs:362-363)
Issue: While iterations uses saturating addition, current can grow unbounded. With a large FOR value or wide TO range, this allows unlimited BigDecimal growth, potentially causing memory exhaustion.
REXX Spec: ANSI X3.274-1996 §7.3.5 requires implementations to detect exponent overflow (Error 42).
Fix: Add overflow detection for the control variable. Check if the exponent exceeds a reasonable limit (e.g., 1_000_000) and raise ArithmeticOverflow.
Location: src/eval.rs:362
2. Missing TO Limit Validation (eval.rs:262-268)
Issue: The TO limit is converted to BigDecimal but never validated. An extremely large value like 1E999999999 will be accepted, leading to potential memory issues during loop execution.
Fix: Add validation similar to the exponent check in BinOp::Power (eval.rs:570). Limit the exponent of TO/BY values to prevent resource exhaustion.
Location: src/eval.rs:262-268
Correctness Issues
3. Incorrect SELECT Error When No WHEN Matched (eval.rs:390-391)
Issue: Uses ExpectedWhenOtherwise (Error 7) which is a parsing error for "WHEN or OTHERWISE expected". The correct error for runtime failure to match is not defined in the basic REXX error set, but Error 7 is misleading.
REXX Semantics: When no WHEN matches and there's no OTHERWISE, REXX typically raises a condition (historically this was treated as a syntax error in some implementations, but it's a runtime issue).
Suggestion: Consider using Error::InvalidExpressionResult (33) or documenting why Error 7 is appropriate here. The current error message is good, but the number is semantically wrong.
Location: src/eval.rs:390
4. Controlled Loop Final Value After UNTIL (eval.rs:354-358)
Observation: The implementation correctly avoids incrementing after UNTIL succeeds (test at line 307-315 confirms). However, the comment at lines 351-353 could be clearer.
Suggestion: Clarify that the break occurs before the increment at line 362, ensuring the control variable retains its current value. The code is correct but the comment is slightly ambiguous.
Location: src/eval.rs:351-358
REXX Semantics
5. Parser Depth Counters Prevent Keywords in Expressions (parser.rs:18-25)
Observation: The do_header_depth, condition_depth, and if_depth mechanism correctly prevents implicit concatenation from consuming control flow keywords.
Verification:
Correctness: This is the correct approach for REXX's "no reserved words" semantics. Well done.
6. LEAVE/ITERATE Name Matching (eval.rs:397-401)
Observation: Correctly implements REXX semantics where unnamed LEAVE/ITERATE match the innermost loop, and named ones match only the specified loop name.
REXX Spec Compliance: Per ANSI X3.274-1996 §7.4.6 (LEAVE) and §7.4.7 (ITERATE). ✓
Arithmetic & Error Handling
7. Division by Zero Error Number (eval.rs:510, 528, 543)
Issue: Division by zero raises ArithmeticOverflow (Error 42), which is technically correct per ANSI X3.274-1996 Appendix A ("Arithmetic overflow/underflow"), but most REXX implementations historically use Error 42 for exponent overflow and Error 41 ("Bad arithmetic conversion") for division by zero.
Recommendation: Consider documenting this choice or switching to Error 41 for consistency with classic REXX interpreters. The current choice is defensible but may surprise users.
Locations: src/eval.rs:510, src/eval.rs:528, src/eval.rs:543
8. Exponent Validation (eval.rs:570)
Observation: Good safety check. Prevents 2 ** 9999999999 from causing OOM. The limit of 1M is generous and practical. ✓
Test Coverage: Verified by large_exponent_error test. ✓
9. Zero BY Step Detection (eval.rs:279-282)
Issue: Uses InvalidWholeNumber (Error 26), but zero is a whole number. The correct error is InvalidDoSyntax (Error 27).
REXX Spec: ANSI X3.274-1996 §7.3.5 states that BY must be non-zero, and violations are syntax errors (Error 27).
Fix: Change to RexxError::InvalidDoSyntax.
Location: src/eval.rs:280
10. NUMERIC DIGITS Compliance (value.rs:69-73, eval.rs:29)
Observation: NumericSettings::default() sets digits: 9, which is correct per ANSI X3.274-1996 §9.3.
Issue: format_rexx_number at value.rs:137 uses d.round(i64::from(digits)), which rounds to digits decimal places, not significant digits.
Example: With NUMERIC DIGITS 9:
Impact: Arithmetic results may not comply with NUMERIC DIGITS for values with exponents or many leading digits.
Location: src/value.rs:137
Performance
11. Unnecessary Clones in Parser (parser.rs:163, 270, 990)
Observations:
Impact: Each clone allocates a new String. For expressions with many symbols, this adds up.
Fix: Use matches! to check the kind, then extract the string without cloning the enum first.
Priority: Low (parser runs once; evaluator runs hot).
Locations: src/parser.rs:163, 270, 990
12. Quadratic String Concatenation in Tests (tests/phase2.rs:116, 124, etc.)
Observation: Tests use repeated s = s || i which reallocates strings on each iteration.
Impact: None (only in tests), but worth noting for production code guidance.
Safety
13. No Panics on Malformed Input
Verification:
Deep Nesting: Parser is recursive but REXX programs are typically shallow. No explicit depth limit, but stack overflow is mitigated by platform limits.
Unterminated Strings: Handled by lexer (out of scope for this PR).
Test Coverage
Strengths:
Gaps:
Summary
Overall Assessment: This is a solid, well-structured implementation of REXX control flow. The parser's use of depth counters to handle REXX's "no reserved words" rule is elegant. The evaluator correctly implements loop semantics including LEAVE/ITERATE name matching and final variable values.
Must-Fix Issues:
Should-Fix Issues:
5. SELECT error number (Issue #3)
6. Division by zero error number (Issue #7)
Nice-to-Have:
7. Parser clone optimizations (Issue #11)
8. Comment clarity for UNTIL behavior (Issue #4)
All tests pass. The implementation demonstrates strong understanding of REXX semantics and Rust best practices.
Files Reviewed:
Testing: All 36 phase2 integration tests pass. ✓
Code Review - PR #3: Phase 2 Control Flow Implementation
Reviewed the REXX interpreter control flow implementation focusing on correctness, REXX semantics compliance (ANSI X3.274-1996), arithmetic handling, and error propagation.
Summary
Overall assessment: Excellent implementation with strong attention to REXX semantics. One critical error number fix required, otherwise ready to merge.
🔴 CRITICAL ISSUE
1. Incorrect Error Number for BY=0
Location:
src/eval.rs:286-288Issue: Zero BY value raises Error 26 (InvalidWholeNumber) but should raise Error 27 (InvalidDoSyntax).
Reasoning:
Current code:
Should be:
Test verification:
The test
zero_by_step_errorintests/phase2.rs:1672currently checks for "BY value" substring but should verify Error 27 specifically.🟡 MINOR ISSUES
2. Comment Inaccuracy in trunc_div
Location:
src/eval.rs:458-461Issue: Comment says "RoundingMode::Down truncates toward zero" but
RoundingMode::Downactually rounds toward negative infinity (floor). The code works correctly becausewith_scale_round(0, RoundingMode::Down)gives the right behavior for integer division, but the comment could mislead future maintainers.Current comment:
More accurate:
✅ CORRECTNESS VALIDATION
Verified all critical REXX semantics:
Control Flow ✓
Loop Semantics ✓
src/eval.rs:303-378)do i = 1 to 5; end; say i→6✓do i = 1 to 10 by 2 until i > 5; end; say i→7(not 9) ✓Keyword Suppression ✓
do_header_depthguardcondition_depthguardif_depthguardArithmetic ✓
src/eval.rs:438-443) ✓Error Handling ✓
All error numbers verified correct (except BY=0 issue above):
Safety ✓
💪 STRENGTHS
📋 RECOMMENDATIONS
MUST FIX:
src/eval.rs:287)SHOULD IMPROVE:
src/eval.rs:458)OPTIONAL:
🎯 VERDICT
Approve with required fix - This is high-quality work that demonstrates sophisticated understanding of REXX's parsing challenges and control flow semantics. The BY=0 error number is a trivial fix. Otherwise, the implementation is excellent.
After fixing the BY=0 error number, this PR is ready to merge.