fix issue 93

[navicore]

fix #93

Summary of fixes for bug #93:

Location	Issue	Fix
Statement::If (line 711-762)	Used abstract inference + apply_effect	Changed to infer_statements_from(&branch, &stack_after_cond)
Statement::Match (line 654-658)	Used abstract inference + apply_effect	Changed to infer_statements_from(&arm.body, &arm_stack)

Other locations checked:

• Statement::Quotation (line 793) - Uses abstract inference but this is correct because quotations are deferred execution; they don't execute with the current stack.

Tests added:

1. test_roll_inside_conditional_with_concrete_stack - Bug Type checker bug: n roll inside if/else causes phantom 'roll_input' stack item #93 reproduction case
2. test_roll_inside_match_arm_with_concrete_stack - Same pattern for match
3. test_roll_with_row_polymorphic_input - Ensures stdlib still works
4. test_pick_with_row_polymorphic_input - Ensures stdlib still works

The core issue was that infer_statements creates a fresh row variable, so n roll/n pick would generate unconstrained phantom types. By using infer_statements_from with the actual concrete stack, operations can see what types are available.

[claude]

Pull Request Review: Fix Issue #93

Summary

This PR fixes a critical type checker bug where n roll and n pick operations inside conditional branches (if/else and match arms) were generating phantom row variable types instead of using the concrete stack types available at runtime.

---

✅ Strengths

1. Correct Root Cause Analysis

The fix properly identifies that the issue was using infer_statements (which creates a fresh row variable) instead of infer_statements_from (which uses the actual concrete stack). This is exactly right.

2. Comprehensive Test Coverage

The PR adds 4 well-designed tests:

• test_roll_inside_conditional_with_concrete_stack - Direct reproduction of bug Type checker bug: n roll inside if/else causes phantom 'roll_input' stack item #93
• test_roll_inside_match_arm_with_concrete_stack - Same pattern for match expressions
• test_roll_with_row_polymorphic_input - Ensures polymorphic stdlib words still work
• test_pick_with_row_polymorphic_input - Ensures pick works polymorphically

These tests cover both the bug fix AND regression prevention for legitimate polymorphic use cases.

3. Excellent Documentation

The added comments clearly explain:

• Why the change was necessary
• How it preserves correctness for polymorphic words
• The conceptual model (row variables only appear when the word itself is polymorphic)

4. Minimal, Focused Changes

The fix touches only what's necessary:

• Statement::If (lines 737-770)
• Statement::Match (lines 654-658)
• Correctly leaves Statement::Quotation unchanged (as noted in the PR description)

5. Proper Substitution Composition

The fix in Statement::If properly composes ALL substitutions (lines 767-770):

let total_subst = cond_subst
    .compose(&then_subst)
    .compose(&else_subst)
    .compose(&branch_subst);

This was missing before and could have caused subtle bugs.

---

🔍 Observations & Minor Concerns

1. Substitution Handling in Match Arms

Looking at lines 654-661, I notice that arm_subst is composed into combined_subst, but unlike the if/else case, it doesn't seem to compose substitutions from intermediate steps. This is likely fine since each arm is independent, but worth verifying.

2. Test Assertions Could Be Stronger

The new tests check that type checking succeeds (is_ok()), but don't verify the inferred types are correct. Consider adding assertions like:

let result = checker.check_program(&program).unwrap();
// Verify the inferred effect matches expectations

3. Edge Case: Empty Else Branch

Line 746-747 handles the case when else_branch is None. This is correct, but a test case for n roll in a then-branch with no else would be valuable:

: test ( Int Int Int -- Int Int Int )
  dup 0 > if
    2 roll
  then ;

---

🔒 Security Considerations

✅ No security concerns. This is a type checker fix that improves correctness without introducing vulnerabilities.

---

⚡ Performance Considerations

✅ Neutral to slightly positive:

• The change from infer_statements + apply_effect to direct infer_statements_from eliminates the intermediate effect abstraction
• Should be marginally faster since it avoids unnecessary row variable creation/unification

---

🏗️ Code Quality

Excellent:

• Clear variable names (arm_stack, stack_after_cond, etc.)
• Consistent with codebase style
• Well-structured error messages with helpful hints

Minor Nitpick:

Line 655-656 comment could be more concise:

// Type check arm body from concrete stack (needed for n roll/pick)

---

📋 Recommendations

Must Have:

✅ All critical items are already addressed

Nice to Have:

1. Add a test for n roll in if-without-else
2. Consider testing deeply nested conditionals: if ... if ... 3 roll ... then ... then
3. Add a test that verifies the inferred effect type matches expectations (not just that it succeeds)

Questions for Author:

1. Did you run the full stdlib test suite to ensure no regressions?
2. Are there any other control flow constructs (loops, etc.) that might have similar issues?

---

🎯 Final Verdict

Recommendation: ✅ APPROVE with minor suggestions

This is a high-quality fix that:

• Correctly addresses the root cause
• Includes excellent test coverage
• Improves code correctness without breaking existing functionality
• Is well-documented and minimal in scope

The bug was a genuine type system flaw, and this PR fixes it the right way. The added comments will help future maintainers understand this subtle aspect of the type checker.

Suggested next steps:

1. Run full test suite to confirm no regressions
2. Consider adding the "nice to have" edge case tests
3. Merge with confidence!

---

Great work tracking down this tricky type system bug! The fix demonstrates a solid understanding of row polymorphism and stack effect inference. 🎉