navicore/patch-rexx

Fork 0

phase 3 #4

Merged

navicore merged 2 commits from phase3 into main

2026-02-08 23:51:37 +00:00

navicore commented

2026-02-08 22:05:57 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-08 22:09:25 +00:00

(Migrated from github.com)

Phase 3 PR Review — REXX Subroutines & Functions

I've reviewed the implementation of CALL/RETURN, function calls, PROCEDURE/EXPOSE, ARG, and DROP instructions. Overall, the implementation is solid and handles the core REXX semantics correctly. Below are specific issues that need attention:

🔴 Critical Issues

1. exec_from runs to end of program after routine completes (src/eval.rs:85-95)

Severity: High — Incorrect semantics

The exec_from method executes from start to the end of the program, not just the routine body. After a routine returns, execution continues through all subsequent clauses in the program until EOF.

Current code:

fn exec_from(&mut self, start: usize) -> RexxResult<ExecSignal> {
    for clause in &self.program.clauses[start..] {  // ⚠️ runs to program end
        let signal = self.exec_clause(clause)?;
        // ...
    }
    Ok(ExecSignal::Normal)
}

Problem: When calling routine1 at line 50 in a 100-line program, exec_from(51) will execute clauses 51-100 instead of stopping at the next label or program end.

Expected REXX semantics: Routine execution should stop at:

An explicit RETURN
An EXIT (which should propagate up)
The next label (start of another routine)
End of program

Test that should fail but might pass:

call sub; exit; sub: say 'in sub'; return; say 'after sub'

Should only print "in sub", but will also print "after sub" because exec_from runs past the RETURN.

Fix: Track routine boundaries. Either:

Stop at the next label clause
Store routine end positions during label building
Use a different approach for routine execution scope

2. ARG parsing doesn't handle missing arguments correctly (src/eval.rs:245-264)

Severity: Medium — Incorrect REXX semantics

Current code:

TemplateElement::Variable(name) => {
    let val = args.get(arg_idx).map_or_else(
        || RexxValue::new(""),  // ⚠️ wrong for missing args
        |v| RexxValue::new(v.as_str().to_uppercase()),
    );
    self.env.set(name, val);
}

Problem: Missing arguments receive empty string "", but per REXX semantics, they should remain unset (so they return their uppercase name when referenced).

Expected REXX behavior:

test: procedure; arg a, b, c
say a b c
return

/* Call with only 1 arg */
call test 'hello'  
/* Should output: "hello B C" not "hello  " */

Fix: Don't call self.env.set() for missing arguments — leave them unset.

3. ARG parser accepts variables without commas between them (src/parser.rs:791-812)

Severity: Medium — Incorrect parsing

Current code:

fn parse_arg(&mut self) -> Clause {
    // ...
    while !self.is_terminator() {
        if matches!(self.peek_kind(), TokenKind::Comma) {
            self.advance();
            elements.push(TemplateElement::Comma);
        } else if let TokenKind::Symbol(s) = self.peek_kind() {
            elements.push(TemplateElement::Variable(s.to_uppercase()));
            self.advance();  // ⚠️ no comma required
        } else {
            break;
        }
    }
}

Problem: This accepts ARG x y z as equivalent to ARG x, y, z. In REXX, comma-separated args map to different argument positions. Without commas, ARG x y should parse x from arg 1, then try to parse pattern y (a literal string pattern, Phase 4 feature).

Impact: Currently harmless since patterns are ignored, but creates wrong AST structure. Will break when Phase 4 implements pattern matching.

Expected parsing:

ARG a, b, c → 3 positional args from positions 0, 1, 2
ARG a b c → parse from position 0, split on pattern 'b', assign remainder to 'c' (Phase 4)

Fix for Phase 3: Insert a comma after each variable if the next token is also a variable, or require commas between positional arguments.

⚠️ Moderate Issues

4. exec_arg doesn't increment arg_idx for missing commas (src/eval.rs:257)

Severity: Medium — Logic error

With the current parser accepting ARG x y z, the evaluator correctly increments arg_idx only on commas. However, if we fix issue #3, this becomes correct. If we don't fix #3, then:

Current code:

TemplateElement::Comma => {
    arg_idx += 1;
}

Without commas in the element list, arg_idx never increments, so ARG x y z would try to assign args[0] to x, args[0] to y, args[0] to z.

This is consistent with the broken parser, but both need fixing together.

5. EXPOSE doesn't search outer scopes for unset variables (src/env.rs:118-139)

Severity: Low — Edge case

Current code:

pub fn push_procedure_expose(&mut self, names: &[String]) {
    // ...
    let caller = self.scopes.last().expect("environment has no scopes");
    for name in names {
        // Only looks in caller scope
        if let Some(val) = caller.vars.get(&upper) {
            new_scope.vars.insert(upper.clone(), val.clone());
        }
    }
}

Problem: Only copies variables from the immediate caller scope. REXX EXPOSE should search all outer scopes (like a closure capture).

Example:

x = 'global'
call level1
exit

level1: procedure
  call level2
  return

level2: procedure expose x  /* should find x from global */
  say x  /* Should print 'global', not 'X' */
  return

Expected: level2 should find x='global' in the outermost scope.

Current behavior: x is unset in level1's scope, so level2 gets no value → prints 'X'.

REXX semantics: EXPOSE searches the caller's scope chain up to the global scope. This is complex because REXX has dynamic scope, not lexical scope.

Fix: In push_procedure_expose, search self.scopes.iter().rev() (all scopes from caller back to global), not just the last scope.

6. Missing bounds check in call_routine (src/eval.rs:191)

Severity: Low — Safety

Current code:

let start_idx = label_idx + 1; // skip the label clause itself
// ...
let has_procedure = matches!(
    self.program.clauses.get(start_idx).map(|c| &c.kind),  // ✓ safely uses get()
    Some(ClauseKind::Procedure(_))
);

Good: Uses .get() for bounds checking.

Potential issue: If a label is the last clause in the program (no instructions after it), start_idx is out of bounds.

Current behavior: get() returns None, has_procedure = false, then exec_from(out_of_bounds) creates an empty slice → returns Normal immediately → correct behavior by accident.

Recommendation: Add explicit check and return error for label-without-body:

if start_idx >= self.program.clauses.len() {
    return Err(RexxDiagnostic::new(RexxError::IncorrectCall)
        .with_detail(format!("routine '{name}' has no instructions")));
}

📊 Test Coverage Observations

The test suite is comprehensive and covers:

✓ Basic calls and returns
✓ RESULT variable handling
✓ Function calls in expressions
✓ Multiple arguments
✓ PROCEDURE isolation
✓ PROCEDURE EXPOSE
✓ Recursion (factorial, fibonacci)
✓ Nested calls
✓ Error cases (Error 43, 44, 17)

Missing test cases:

Routine execution boundaries (my issue #1) — call sub; exit; sub: return; say 'oops'
Missing arguments with ARG (issue #2) — test: arg a,b,c; say b; return; call test 1
EXPOSE with nested procedures (issue #5)
DROP on stem variables — a. = 'default'; a.1 = 'x'; drop a.
Label as last clause in program (issue #6)
Case-insensitive routine names — call HELLO vs hello: return

✅ Strengths

Correct error numbers — Error 17 (UnexpectedProcedure), 43 (RoutineNotFound), 44 (NoReturnData) are correctly used
pending_exit mechanism — Clever design to propagate EXIT through function calls during expression evaluation
EXPOSE write-back — Correctly writes exposed variables back to parent scope on return, including handling DROP
ARG uppercasing — Correctly uppercases argument values per REXX semantics
RESULT handling — Correctly distinguishes between CALL (sets RESULT) and function invocation (requires return value)
Label case-insensitivity — Labels and routine names correctly normalized to uppercase

📝 Recommendations

Priority 1 (must fix):

Issue #1: Fix exec_from to respect routine boundaries
Issue #2: Fix ARG to leave missing arguments unset

Priority 2 (should fix):

Issue #3: Fix ARG parser to handle commas correctly
Issue #5: Fix EXPOSE to search outer scopes

Priority 3 (nice to have):

Issue #6: Add bounds check for empty routines
Add missing test cases listed above

Overall assessment: Strong implementation of REXX subroutine semantics with correct error handling. The critical issues (#1, #2) will cause incorrect behavior on valid REXX programs and should be fixed before merge. Issues #3-6 are edge cases that could be addressed in follow-up PRs.

Great work on the pending_exit mechanism and EXPOSE write-back logic! 🚀

## Phase 3 PR Review — REXX Subroutines & Functions I've reviewed the implementation of CALL/RETURN, function calls, PROCEDURE/EXPOSE, ARG, and DROP instructions. Overall, the implementation is solid and handles the core REXX semantics correctly. Below are specific issues that need attention: --- ## 🔴 Critical Issues ### 1. **exec_from runs to end of program after routine completes** (src/eval.rs:85-95) **Severity: High — Incorrect semantics** The `exec_from` method executes from `start` to the **end of the program**, not just the routine body. After a routine returns, execution continues through all subsequent clauses in the program until EOF. **Current code:** ```rust fn exec_from(&mut self, start: usize) -> RexxResult<ExecSignal> { for clause in &self.program.clauses[start..] { // ⚠️ runs to program end let signal = self.exec_clause(clause)?; // ... } Ok(ExecSignal::Normal) } ``` **Problem:** When calling `routine1` at line 50 in a 100-line program, `exec_from(51)` will execute clauses 51-100 instead of stopping at the next label or program end. **Expected REXX semantics:** Routine execution should stop at: - An explicit RETURN - An EXIT (which should propagate up) - The next label (start of another routine) - End of program **Test that should fail but might pass:** ```rexx call sub; exit; sub: say 'in sub'; return; say 'after sub' ``` Should only print "in sub", but will also print "after sub" because `exec_from` runs past the RETURN. **Fix:** Track routine boundaries. Either: 1. Stop at the next label clause 2. Store routine end positions during label building 3. Use a different approach for routine execution scope --- ### 2. **ARG parsing doesn't handle missing arguments correctly** (src/eval.rs:245-264) **Severity: Medium — Incorrect REXX semantics** **Current code:** ```rust TemplateElement::Variable(name) => { let val = args.get(arg_idx).map_or_else( || RexxValue::new(""), // ⚠️ wrong for missing args |v| RexxValue::new(v.as_str().to_uppercase()), ); self.env.set(name, val); } ``` **Problem:** Missing arguments receive empty string `""`, but per REXX semantics, they should remain **unset** (so they return their uppercase name when referenced). **Expected REXX behavior:** ```rexx test: procedure; arg a, b, c say a b c return /* Call with only 1 arg */ call test 'hello' /* Should output: "hello B C" not "hello " */ ``` **Fix:** Don't call `self.env.set()` for missing arguments — leave them unset. --- ### 3. **ARG parser accepts variables without commas between them** (src/parser.rs:791-812) **Severity: Medium — Incorrect parsing** **Current code:** ```rust fn parse_arg(&mut self) -> Clause { // ... while !self.is_terminator() { if matches!(self.peek_kind(), TokenKind::Comma) { self.advance(); elements.push(TemplateElement::Comma); } else if let TokenKind::Symbol(s) = self.peek_kind() { elements.push(TemplateElement::Variable(s.to_uppercase())); self.advance(); // ⚠️ no comma required } else { break; } } } ``` **Problem:** This accepts `ARG x y z` as equivalent to `ARG x, y, z`. In REXX, comma-separated args map to different argument positions. Without commas, `ARG x y` should parse `x` from arg 1, then try to parse pattern `y` (a literal string pattern, Phase 4 feature). **Impact:** Currently harmless since patterns are ignored, but creates wrong AST structure. Will break when Phase 4 implements pattern matching. **Expected parsing:** - `ARG a, b, c` → 3 positional args from positions 0, 1, 2 - `ARG a b c` → parse from position 0, split on pattern 'b', assign remainder to 'c' (Phase 4) **Fix for Phase 3:** Insert a comma after each variable if the next token is also a variable, or require commas between positional arguments. --- ## ⚠️ Moderate Issues ### 4. **exec_arg doesn't increment arg_idx for missing commas** (src/eval.rs:257) **Severity: Medium — Logic error** With the current parser accepting `ARG x y z`, the evaluator correctly increments `arg_idx` only on commas. However, if we fix issue #3, this becomes correct. If we don't fix #3, then: **Current code:** ```rust TemplateElement::Comma => { arg_idx += 1; } ``` Without commas in the element list, `arg_idx` never increments, so `ARG x y z` would try to assign args[0] to x, args[0] to y, args[0] to z. **This is consistent with the broken parser**, but both need fixing together. --- ### 5. **EXPOSE doesn't search outer scopes for unset variables** (src/env.rs:118-139) **Severity: Low — Edge case** **Current code:** ```rust pub fn push_procedure_expose(&mut self, names: &[String]) { // ... let caller = self.scopes.last().expect("environment has no scopes"); for name in names { // Only looks in caller scope if let Some(val) = caller.vars.get(&upper) { new_scope.vars.insert(upper.clone(), val.clone()); } } } ``` **Problem:** Only copies variables from the immediate caller scope. REXX `EXPOSE` should search **all outer scopes** (like a closure capture). **Example:** ```rexx x = 'global' call level1 exit level1: procedure call level2 return level2: procedure expose x /* should find x from global */ say x /* Should print 'global', not 'X' */ return ``` **Expected:** `level2` should find `x='global'` in the outermost scope. **Current behavior:** `x` is unset in `level1`'s scope, so `level2` gets no value → prints 'X'. **REXX semantics:** EXPOSE searches the **caller's scope chain** up to the global scope. This is complex because REXX has dynamic scope, not lexical scope. **Fix:** In `push_procedure_expose`, search `self.scopes.iter().rev()` (all scopes from caller back to global), not just the last scope. --- ### 6. **Missing bounds check in call_routine** (src/eval.rs:191) **Severity: Low — Safety** **Current code:** ```rust let start_idx = label_idx + 1; // skip the label clause itself // ... let has_procedure = matches!( self.program.clauses.get(start_idx).map(|c| &c.kind), // ✓ safely uses get() Some(ClauseKind::Procedure(_)) ); ``` **Good:** Uses `.get()` for bounds checking. **Potential issue:** If a label is the last clause in the program (no instructions after it), `start_idx` is out of bounds. **Current behavior:** `get()` returns `None`, `has_procedure = false`, then `exec_from(out_of_bounds)` creates an empty slice → returns `Normal` immediately → correct behavior by accident. **Recommendation:** Add explicit check and return error for label-without-body: ```rust if start_idx >= self.program.clauses.len() { return Err(RexxDiagnostic::new(RexxError::IncorrectCall) .with_detail(format!("routine '{name}' has no instructions"))); } ``` --- ## 📊 Test Coverage Observations The test suite is comprehensive and covers: - ✓ Basic calls and returns - ✓ RESULT variable handling - ✓ Function calls in expressions - ✓ Multiple arguments - ✓ PROCEDURE isolation - ✓ PROCEDURE EXPOSE - ✓ Recursion (factorial, fibonacci) - ✓ Nested calls - ✓ Error cases (Error 43, 44, 17) **Missing test cases:** 1. Routine execution boundaries (my issue #1) — `call sub; exit; sub: return; say 'oops'` 2. Missing arguments with ARG (issue #2) — `test: arg a,b,c; say b; return; call test 1` 3. EXPOSE with nested procedures (issue #5) 4. DROP on stem variables — `a. = 'default'; a.1 = 'x'; drop a.` 5. Label as last clause in program (issue #6) 6. Case-insensitive routine names — `call HELLO` vs `hello: return` --- ## ✅ Strengths 1. **Correct error numbers** — Error 17 (UnexpectedProcedure), 43 (RoutineNotFound), 44 (NoReturnData) are correctly used 2. **pending_exit mechanism** — Clever design to propagate EXIT through function calls during expression evaluation 3. **EXPOSE write-back** — Correctly writes exposed variables back to parent scope on return, including handling DROP 4. **ARG uppercasing** — Correctly uppercases argument values per REXX semantics 5. **RESULT handling** — Correctly distinguishes between CALL (sets RESULT) and function invocation (requires return value) 6. **Label case-insensitivity** — Labels and routine names correctly normalized to uppercase --- ## 📝 Recommendations ### Priority 1 (must fix): - Issue #1: Fix `exec_from` to respect routine boundaries - Issue #2: Fix ARG to leave missing arguments unset ### Priority 2 (should fix): - Issue #3: Fix ARG parser to handle commas correctly - Issue #5: Fix EXPOSE to search outer scopes ### Priority 3 (nice to have): - Issue #6: Add bounds check for empty routines - Add missing test cases listed above --- **Overall assessment:** Strong implementation of REXX subroutine semantics with correct error handling. The critical issues (#1, #2) will cause incorrect behavior on valid REXX programs and should be fixed before merge. Issues #3-6 are edge cases that could be addressed in follow-up PRs. Great work on the pending_exit mechanism and EXPOSE write-back logic! 🚀

claude[bot] commented

2026-02-08 23:48:34 +00:00

(Migrated from github.com)

Phase 3 PR Review: Subroutines & Procedure Scoping

Summary

✅ Overall: Strong implementation with correct REXX semantics

All tests pass. The implementation correctly handles subroutine calls, PROCEDURE scoping, PROCEDURE EXPOSE, ARG instruction, and DROP instruction. Error numbers are properly mapped per ANSI X3.274-1996.

Critical Issues Found

1. Execution fall-through bug (src/eval.rs:185-219)

Severity: HIGH — Subroutines that reach EOF continue executing subsequent clauses

Location: call_routine method, line 211

Issue: After calling exec_from(exec_start), execution continues from that point until EOF or an explicit RETURN/EXIT. If a routine doesn't contain a RETURN statement, it will fall through and execute all subsequent clauses in the program (including other label definitions, main code after EXIT, etc.).

REXX Spec: Per ANSI X3.274-1996 §7.3.1, routine execution should stop when reaching:

An explicit RETURN
The next label clause
EOF

Current behavior:

say greet()
exit

greet:
  x = 'hello'
  /* Missing RETURN - falls through */

other_label:
  say 'wrong'
  return 'oops'

This will execute other_label and return "oops" instead of returning the variable name "X".

Fix: Track routine boundaries by stopping execution at the next label:

fn call_routine(&mut self, name: &str, args: Vec<RexxValue>) -> RexxResult<ExecSignal> {
    let &label_idx = self.labels.get(name)...;
    
    // Find the next label to bound execution
    let end_idx = self.program.clauses[label_idx + 1..]
        .iter()
        .position(|c| matches\!(c.kind, ClauseKind::Label(_)))
        .map(|pos| label_idx + 1 + pos)
        .unwrap_or(self.program.clauses.len());
    
    self.arg_stack.push(args);
    
    // Execute only until next label or EOF
    for clause in &self.program.clauses[exec_start..end_idx] {
        let signal = self.exec_clause(clause)?;
        // ... handle signals
    }
    
    // Implicit return with no value if routine ends without RETURN
    if has_procedure {
        self.env.pop_procedure();
    }
    self.arg_stack.pop();
    Ok(ExecSignal::Return(None))
}

Test to add:

#[test]
fn routine_without_return_stops_at_next_label() {
    assert_eq\!(
        run_rexx("x = greet(); say x; exit; greet: y = 'hi'; label2: say 'wrong'; return 'wrong'"),
        "Y"  // Should return unset var name, not execute label2
    );
}

2. EXIT within routines doesn't propagate correctly (src/eval.rs:615-618)

Severity: MEDIUM — Function calls set pending_exit but don't check it before returning

Location: eval_expr FunctionCall arm, line 608-620

Issue: When EXIT is executed inside a function call during expression evaluation, the pending exit is set but the function returns an empty string. The caller continues evaluating the expression instead of immediately propagating the EXIT signal.

Example:

say 'before' cleanup() 'after'
exit

cleanup: procedure
  exit 99

Current behavior: Prints "before after" and exits with 99 (empty string from cleanup concatenates).

Expected behavior: Should exit immediately with code 99 without printing "after".

Root cause: Line 604-606 checks pending_exit during argument evaluation but doesn't check it after call_routine returns on line 608.

Fix:

let signal = self.call_routine(name, evaluated_args)?;
if self.pending_exit.is_pending() {
    return Ok(RexxValue::new(""));  // Already set by call_routine
}
match signal {
    // ... rest of match
}

Actually, the current code does handle this at line 615-617 by setting pending_exit. The real issue is that expression evaluation doesn't check pending_exit after each subexpression in binary operations. Consider:

say func1() + func2()  /* func1 calls EXIT */

If func1() triggers EXIT, func2() should never be called.

Better fix: Check pending_exit in eval_binop after evaluating left operand:

fn eval_binop(...) -> RexxResult<RexxValue> {
    match op {
        BinOp::Add => {
            if self.pending_exit.is_pending() {
                return Ok(RexxValue::new(""));
            }
            self.arithmetic(left, right, |a, b| a + b)
        },
        // ... repeat for all ops
    }
}

Medium Issues

3. PROCEDURE EXPOSE writes back even on error (src/env.rs:143-159)

Severity: MEDIUM — Partial state modification on error

Issue: If an error occurs during routine execution after modifying exposed variables, pop_procedure still writes the modified values back to the parent scope. REXX semantics aren't crystal clear here, but traditional implementations tend to leave parent variables unchanged on error.

Recommendation: Consider wrapping exposed variable writeback in the call_routine method to only write back on successful return:

let result = self.exec_from(exec_start);
if has_procedure {
    if result.is_ok() {
        self.env.pop_procedure();  // Writes back
    } else {
        self.scopes.pop();  // Discard without writeback
    }
}

However, this may not match all REXX implementations. Needs spec clarification.

4. ARG instruction uppercases values, not just variable names (src/eval.rs:252)

Severity: MEDIUM — Incorrect REXX semantics

Location: exec_arg, line 252

Code:

self.env.set(name, RexxValue::new(v.as_str().to_uppercase()));

Issue: The ARG instruction should uppercase the variable name (which is already done by the parser), but it should not uppercase the argument value. The current code uppercases the value itself.

REXX Spec: Per ANSI X3.274-1996 §7.4.1, ARG is shorthand for "PARSE UPPER ARG", which means:

The variable name is case-insensitive (already handled)
The value is uppercased before assignment (this is PARSE UPPER behavior)

Actually, I was wrong: The current implementation is correct. ARG is defined as PARSE UPPER ARG, so values should be uppercased. The test at line 142-146 confirms this:

assert_eq\!(run_rexx("call greet 'hello'; ...; greet: arg msg; say msg; ..."), "HELLO");

This is correct per ANSI spec. Ignore this issue.

Minor Issues / Observations

5. Missing bounds check on label_idx + 1 (src/eval.rs:191-198)

Severity: LOW — Edge case safety

Code:

let start_idx = label_idx + 1; // skip the label clause itself
let has_procedure = matches\!(
    self.program.clauses.get(start_idx).map(|c| &c.kind),
    Some(ClauseKind::Procedure(_))
);

Issue: If a label is the last clause in the program (label_idx == program.clauses.len() - 1), then start_idx will be out of bounds. The get call will return None, which is safe, but it means a label at EOF will successfully "call" and immediately return None (no PROCEDURE, empty body).

Expected behavior: Calling a label at EOF should either:

Execute nothing and return (current behavior — arguably correct)
Raise Error 40 (Incorrect call to routine) if no executable clauses follow

Current behavior is reasonable but consider a comment explaining this edge case.

6. Potential infinite recursion without stack limit (src/eval.rs:185)

Severity: LOW — Missing safeguard

Issue: Recursive calls have no depth limit. Deep recursion (e.g., fact(10000)) will overflow the Rust call stack.

Recommendation: Add a recursion depth counter:

pub struct Evaluator<'a> {
    // ...
    call_depth: usize,
}

fn call_routine(...) {
    if self.call_depth > 1000 {  // Or some reasonable limit
        return Err(RexxDiagnostic::new(RexxError::ResourceExhausted)
            .with_detail("call stack overflow"));
    }
    self.call_depth += 1;
    // ... execute ...
    self.call_depth -= 1;
}

Not critical for Phase 3, but important for production.

7. LEAVE/ITERATE from within functions silently ignored (src/eval.rs:241, 619)

Severity: LOW — Spec compliance edge case

Code:

ExecSignal::Leave(_) | ExecSignal::Iterate(_) => Ok(ExecSignal::Normal),

Issue: If a function executes LEAVE or ITERATE (e.g., nested inside a DO loop that then calls a function), the signal is silently converted to Normal. Per REXX spec, LEAVE/ITERATE should propagate to the enclosing loop, not be swallowed.

Example:

do i = 1 to 10
  if helper(i) then leave
end

helper: procedure
  arg n
  if n > 5 then return 1
  return 0

This is correct — LEAVE is in the main loop, not the function.

But consider:

do i = 1 to 10
  call helper
end

helper: procedure
  leave  /* Should this leave the outer loop? */
  return

Per ANSI spec: LEAVE without a name targets the innermost loop. If executed in a PROCEDURE (isolated scope), there's no enclosing loop, so this should raise Error 28 (Invalid LEAVE or ITERATE).

Current behavior: Silently converts to Normal.

Fix: In call_routine, check for LEAVE/ITERATE in isolated procedures:

match result {
    Ok(ExecSignal::Leave(_)) | Ok(ExecSignal::Iterate(_)) if has_procedure => {
        Err(RexxDiagnostic::new(RexxError::InvalidLeaveIterate)
            .with_detail("LEAVE/ITERATE not valid in isolated procedure"))
    }
    // ... other cases
}

For non-PROCEDURE routines (dynamic scope), LEAVE/ITERATE should propagate.

Code Quality Observations

Positive

✅ Clean separation of concerns (parser, evaluator, environment)
✅ Proper use of REXX error numbers per ANSI spec
✅ Good test coverage for core functionality
✅ Correct handling of PROCEDURE EXPOSE writeback semantics
✅ PendingExit pattern correctly handles deferred EXIT propagation

Suggestions

Add comments explaining REXX-specific semantics (e.g., why ARG uppercases values)
Consider extracting routine boundary detection into a helper method
Add integration tests for nested function calls with EXIT
Document the execution model (fall-through vs. bounded by labels)

Test Coverage Gaps

The existing tests are good but missing:

Routine without RETURN falling through to next label
EXIT within nested function calls during expression eval
LEAVE/ITERATE inside PROCEDURE (should error)
Calling a label at EOF (edge case)
Deep recursion (stack overflow protection)
PROCEDURE EXPOSE with stem variables (tests exist in env.rs but not integration)
Multiple arguments with missing values (e.g., call foo 1,,3)

Correctness per ANSI X3.274-1996

Feature	Status	Notes
CALL instruction	✅	Correct
Function calls in expressions	✅	Correct
PROCEDURE isolation	✅	Correct
PROCEDURE EXPOSE	✅	Correct writeback semantics
ARG uppercasing	✅	Correct (PARSE UPPER ARG)
RESULT variable	✅	Correctly set/dropped
DROP instruction	✅	Correct
Error 43 (routine not found)	✅	Correct
Error 44 (no return data)	✅	Correct
Error 17 (misplaced PROCEDURE)	✅	Correct
Routine termination	❌	Falls through past next label
LEAVE/ITERATE in PROCEDURE	❌	Should error, currently silent

Recommended Fixes (Priority Order)

Fix routine fall-through bug — High priority, breaks correctness
Add LEAVE/ITERATE validation in PROCEDURE — Medium priority, spec compliance
Add recursion depth limit — Low priority, safety feature
Document edge cases — Low priority, maintainability

Conclusion

Strong implementation overall. The critical routine fall-through bug must be fixed before merge. The LEAVE/ITERATE issue is a spec compliance gap but less urgent. Consider adding the suggested tests to prevent regressions.

Great work on Phase 3! 🎉

# Phase 3 PR Review: Subroutines & Procedure Scoping ## Summary ✅ **Overall: Strong implementation with correct REXX semantics** All tests pass. The implementation correctly handles subroutine calls, PROCEDURE scoping, PROCEDURE EXPOSE, ARG instruction, and DROP instruction. Error numbers are properly mapped per ANSI X3.274-1996. --- ## Critical Issues Found ### 1. **Execution fall-through bug** (src/eval.rs:185-219) **Severity: HIGH** — Subroutines that reach EOF continue executing subsequent clauses **Location:** `call_routine` method, line 211 **Issue:** After calling `exec_from(exec_start)`, execution continues from that point **until EOF or an explicit RETURN/EXIT**. If a routine doesn't contain a RETURN statement, it will fall through and execute all subsequent clauses in the program (including other label definitions, main code after EXIT, etc.). **REXX Spec:** Per ANSI X3.274-1996 §7.3.1, routine execution should stop when reaching: 1. An explicit RETURN 2. The next label clause 3. EOF **Current behavior:** ```rexx say greet() exit greet: x = 'hello' /* Missing RETURN - falls through */ other_label: say 'wrong' return 'oops' ``` This will execute `other_label` and return "oops" instead of returning the variable name "X". **Fix:** Track routine boundaries by stopping execution at the next label: ```rust fn call_routine(&mut self, name: &str, args: Vec<RexxValue>) -> RexxResult<ExecSignal> { let &label_idx = self.labels.get(name)...; // Find the next label to bound execution let end_idx = self.program.clauses[label_idx + 1..] .iter() .position(|c| matches\!(c.kind, ClauseKind::Label(_))) .map(|pos| label_idx + 1 + pos) .unwrap_or(self.program.clauses.len()); self.arg_stack.push(args); // Execute only until next label or EOF for clause in &self.program.clauses[exec_start..end_idx] { let signal = self.exec_clause(clause)?; // ... handle signals } // Implicit return with no value if routine ends without RETURN if has_procedure { self.env.pop_procedure(); } self.arg_stack.pop(); Ok(ExecSignal::Return(None)) } ``` **Test to add:** ```rust #[test] fn routine_without_return_stops_at_next_label() { assert_eq\!( run_rexx("x = greet(); say x; exit; greet: y = 'hi'; label2: say 'wrong'; return 'wrong'"), "Y" // Should return unset var name, not execute label2 ); } ``` --- ### 2. **EXIT within routines doesn't propagate correctly** (src/eval.rs:615-618) **Severity: MEDIUM** — Function calls set pending_exit but don't check it before returning **Location:** `eval_expr` FunctionCall arm, line 608-620 **Issue:** When EXIT is executed inside a function call during expression evaluation, the pending exit is set but the function returns an empty string. The caller continues evaluating the expression instead of immediately propagating the EXIT signal. **Example:** ```rexx say 'before' cleanup() 'after' exit cleanup: procedure exit 99 ``` **Current behavior:** Prints "before after" and exits with 99 (empty string from cleanup concatenates). **Expected behavior:** Should exit immediately with code 99 without printing "after". **Root cause:** Line 604-606 checks `pending_exit` during argument evaluation but doesn't check it after `call_routine` returns on line 608. **Fix:** ```rust let signal = self.call_routine(name, evaluated_args)?; if self.pending_exit.is_pending() { return Ok(RexxValue::new("")); // Already set by call_routine } match signal { // ... rest of match } ``` Actually, the current code **does** handle this at line 615-617 by setting pending_exit. The real issue is that expression evaluation doesn't check pending_exit after each subexpression in binary operations. Consider: ```rexx say func1() + func2() /* func1 calls EXIT */ ``` If func1() triggers EXIT, func2() should never be called. **Better fix:** Check pending_exit in `eval_binop` after evaluating left operand: ```rust fn eval_binop(...) -> RexxResult<RexxValue> { match op { BinOp::Add => { if self.pending_exit.is_pending() { return Ok(RexxValue::new("")); } self.arithmetic(left, right, |a, b| a + b) }, // ... repeat for all ops } } ``` --- ## Medium Issues ### 3. **PROCEDURE EXPOSE writes back even on error** (src/env.rs:143-159) **Severity: MEDIUM** — Partial state modification on error **Issue:** If an error occurs during routine execution after modifying exposed variables, `pop_procedure` still writes the modified values back to the parent scope. REXX semantics aren't crystal clear here, but traditional implementations tend to leave parent variables unchanged on error. **Recommendation:** Consider wrapping exposed variable writeback in the `call_routine` method to only write back on successful return: ```rust let result = self.exec_from(exec_start); if has_procedure { if result.is_ok() { self.env.pop_procedure(); // Writes back } else { self.scopes.pop(); // Discard without writeback } } ``` However, this may not match all REXX implementations. Needs spec clarification. --- ### 4. **ARG instruction uppercases values, not just variable names** (src/eval.rs:252) **Severity: MEDIUM** — Incorrect REXX semantics **Location:** `exec_arg`, line 252 **Code:** ```rust self.env.set(name, RexxValue::new(v.as_str().to_uppercase())); ``` **Issue:** The ARG instruction should uppercase the **variable name** (which is already done by the parser), but it should **not** uppercase the **argument value**. The current code uppercases the value itself. **REXX Spec:** Per ANSI X3.274-1996 §7.4.1, ARG is shorthand for "PARSE UPPER ARG", which means: - The variable **name** is case-insensitive (already handled) - The **value** is uppercased before assignment (this is PARSE UPPER behavior) **Actually, I was wrong:** The current implementation is **correct**. ARG is defined as PARSE UPPER ARG, so values **should** be uppercased. The test at line 142-146 confirms this: ```rust assert_eq\!(run_rexx("call greet 'hello'; ...; greet: arg msg; say msg; ..."), "HELLO"); ``` **This is correct per ANSI spec.** Ignore this issue. --- ## Minor Issues / Observations ### 5. **Missing bounds check on label_idx + 1** (src/eval.rs:191-198) **Severity: LOW** — Edge case safety **Code:** ```rust let start_idx = label_idx + 1; // skip the label clause itself let has_procedure = matches\!( self.program.clauses.get(start_idx).map(|c| &c.kind), Some(ClauseKind::Procedure(_)) ); ``` **Issue:** If a label is the last clause in the program (`label_idx == program.clauses.len() - 1`), then `start_idx` will be out of bounds. The `get` call will return None, which is safe, but it means a label at EOF will successfully "call" and immediately return None (no PROCEDURE, empty body). **Expected behavior:** Calling a label at EOF should either: 1. Execute nothing and return (current behavior — arguably correct) 2. Raise Error 40 (Incorrect call to routine) if no executable clauses follow **Current behavior is reasonable** but consider a comment explaining this edge case. --- ### 6. **Potential infinite recursion without stack limit** (src/eval.rs:185) **Severity: LOW** — Missing safeguard **Issue:** Recursive calls have no depth limit. Deep recursion (e.g., `fact(10000)`) will overflow the Rust call stack. **Recommendation:** Add a recursion depth counter: ```rust pub struct Evaluator<'a> { // ... call_depth: usize, } fn call_routine(...) { if self.call_depth > 1000 { // Or some reasonable limit return Err(RexxDiagnostic::new(RexxError::ResourceExhausted) .with_detail("call stack overflow")); } self.call_depth += 1; // ... execute ... self.call_depth -= 1; } ``` Not critical for Phase 3, but important for production. --- ### 7. **LEAVE/ITERATE from within functions silently ignored** (src/eval.rs:241, 619) **Severity: LOW** — Spec compliance edge case **Code:** ```rust ExecSignal::Leave(_) | ExecSignal::Iterate(_) => Ok(ExecSignal::Normal), ``` **Issue:** If a function executes LEAVE or ITERATE (e.g., nested inside a DO loop that then calls a function), the signal is silently converted to Normal. Per REXX spec, LEAVE/ITERATE should propagate to the enclosing loop, not be swallowed. **Example:** ```rexx do i = 1 to 10 if helper(i) then leave end helper: procedure arg n if n > 5 then return 1 return 0 ``` This is correct — LEAVE is in the main loop, not the function. But consider: ```rexx do i = 1 to 10 call helper end helper: procedure leave /* Should this leave the outer loop? */ return ``` **Per ANSI spec:** LEAVE without a name targets the innermost loop. If executed in a PROCEDURE (isolated scope), there's no enclosing loop, so this should raise **Error 28** (Invalid LEAVE or ITERATE). **Current behavior:** Silently converts to Normal. **Fix:** In `call_routine`, check for LEAVE/ITERATE in isolated procedures: ```rust match result { Ok(ExecSignal::Leave(_)) | Ok(ExecSignal::Iterate(_)) if has_procedure => { Err(RexxDiagnostic::new(RexxError::InvalidLeaveIterate) .with_detail("LEAVE/ITERATE not valid in isolated procedure")) } // ... other cases } ``` For non-PROCEDURE routines (dynamic scope), LEAVE/ITERATE should propagate. --- ## Code Quality Observations ### Positive ✅ Clean separation of concerns (parser, evaluator, environment) ✅ Proper use of REXX error numbers per ANSI spec ✅ Good test coverage for core functionality ✅ Correct handling of PROCEDURE EXPOSE writeback semantics ✅ PendingExit pattern correctly handles deferred EXIT propagation ### Suggestions - Add comments explaining REXX-specific semantics (e.g., why ARG uppercases values) - Consider extracting routine boundary detection into a helper method - Add integration tests for nested function calls with EXIT - Document the execution model (fall-through vs. bounded by labels) --- ## Test Coverage Gaps The existing tests are good but missing: 1. **Routine without RETURN falling through to next label** 2. **EXIT within nested function calls during expression eval** 3. **LEAVE/ITERATE inside PROCEDURE (should error)** 4. **Calling a label at EOF (edge case)** 5. **Deep recursion (stack overflow protection)** 6. **PROCEDURE EXPOSE with stem variables** (tests exist in env.rs but not integration) 7. **Multiple arguments with missing values** (e.g., `call foo 1,,3`) --- ## Correctness per ANSI X3.274-1996 | Feature | Status | Notes | |---------|--------|-------| | CALL instruction | ✅ | Correct | | Function calls in expressions | ✅ | Correct | | PROCEDURE isolation | ✅ | Correct | | PROCEDURE EXPOSE | ✅ | Correct writeback semantics | | ARG uppercasing | ✅ | Correct (PARSE UPPER ARG) | | RESULT variable | ✅ | Correctly set/dropped | | DROP instruction | ✅ | Correct | | Error 43 (routine not found) | ✅ | Correct | | Error 44 (no return data) | ✅ | Correct | | Error 17 (misplaced PROCEDURE) | ✅ | Correct | | Routine termination | ❌ | **Falls through past next label** | | LEAVE/ITERATE in PROCEDURE | ❌ | Should error, currently silent | --- ## Recommended Fixes (Priority Order) 1. **Fix routine fall-through bug** — High priority, breaks correctness 2. **Add LEAVE/ITERATE validation in PROCEDURE** — Medium priority, spec compliance 3. **Add recursion depth limit** — Low priority, safety feature 4. **Document edge cases** — Low priority, maintainability --- ## Conclusion Strong implementation overall. The critical routine fall-through bug must be fixed before merge. The LEAVE/ITERATE issue is a spec compliance gap but less urgent. Consider adding the suggested tests to prevent regressions. Great work on Phase 3\! 🎉