I 230 closure and quotation safety

[navicore]

#230
#231

[claude]

PR Review: Closure and Quotation Safety Tests

Thank you for this comprehensive test suite! This PR adds excellent coverage for closure nesting and quotation effect verification. Here's my detailed review:

---

✅ Strengths

1. Excellent Test Coverage

• 21 new tests covering closure captures, nested quotations (2-3 levels deep), and combinator interactions
• Tests both positive cases (valid usage) and negative cases (type mismatches, wrong arities)
• Clear mapping to issues Deep closure nesting - type checking coverage #230 and Quotation effect verification - inferred vs declared #231 with thorough documentation

2. Outstanding Documentation

• Each test has descriptive comments explaining the expected behavior
• Seq pseudo-code comments make tests readable even without deep Rust knowledge
• Section headers clearly organize tests by issue and purpose
• Examples like test_closure_use_after_creation demonstrate real-world usage patterns

3. Transparency About Known Gaps

• Two known gaps are explicitly documented with "KNOWN GAP" comments:
  • test_while_body_stack_neutral_gap (line 4185): Row variable absorbs pushed values instead of failing
  • test_until_cond_return_type_gap (line 4228): Int not rejected when Bool expected
• Tests correctly use assert\!(result.is_ok()) for gaps with clear TODO comments
• This is excellent engineering practice - documenting limitations rather than hiding them

4. Good Test Design Patterns

• Tests are focused and test one concept each
• Assertion messages include helpful context ({:?} for error details)
• Tests follow existing codebase patterns (manual AST construction)
• Proper use of row variables (RowVar("r")) for polymorphic effects

---

🔍 Code Quality Observations

Minor: Test Complexity vs. Behavior

In test_closure_multiple_captures (line 3782), the comment says:

// : make-between ( Int Int -- Quot )
//   [ dup rot i.>= swap rot i.<= and ] ;

But the actual test body is simplified:

body: vec\![Statement::WordCall {
    name: "i.>=".to_string(),
    span: None,
}],

Recommendation: Either update the comment to match the simplified test or implement the full "between" logic. The current test still validates multi-capture handling, but the mismatch could confuse future maintainers.

Minor: Assertion in test_closure_multiple_captures

Line 3814:

assert\!(
    result.is_ok() || result.is_err(),
    "Multiple captures should be handled (pass or fail gracefully)"
);

Issue: This assertion always passes (tautology).

Recommendation: Decide whether multiple captures should succeed or fail, then assert that specific outcome. If the behavior is genuinely implementation-dependent, add a comment explaining why and consider splitting into two tests (one for success case, one for failure case).

---

🎯 Test Coverage Analysis

Excellent Coverage For:

✅ Basic closure capture
✅ 2-3 level nesting
✅ Closure usage after creation
✅ Type mismatches (Int vs String)
✅ Quotation type preservation
✅ Stack-neutral quotations in loops
✅ Combinator integration (times, while, until)

Potential Gaps (Future Work):

Based on issue #230's original scope:

• ⚠️ Closures with Yield effects (mentioned in issue but not tested)
• ⚠️ Closure propagation through bi/dip combinators (mentioned in issue Deep closure nesting - type checking coverage #230)
• ⚠️ Transitive captures (outer closure capturing from multiple scopes)

These aren't blockers for this PR - they could be follow-up work.

---

🔒 Security & Performance

✅ No security concerns: Tests only add test code, no production changes
✅ No performance concerns: Tests run at build time only
✅ No backwards compatibility issues: Pure test additions

---

📝 Best Practices Check

✅ Single file changed (focused PR)
✅ Only test additions (no production code changes)
✅ Clear commit messages with detailed descriptions
✅ Links to relevant issues
⚠️ Minor: No updates to documentation about known gaps (consider updating TYPE_SYSTEM_GUIDE.md or adding a KNOWN_ISSUES.md)

---

🎓 What I Learned From This PR

This PR demonstrates excellent test-driven development:

1. Test edge cases explicitly - The "wrong call type" test (line 3709) catches subtle type mismatches
2. Document known limitations - The KNOWN GAP pattern is valuable for future maintainers
3. Test both ways - Valid cases prove features work; invalid cases prove error detection works

The Quotation vs Closure distinction (comment at line 3630) is particularly well-explained:

• Quotation = stateless, body effect matches declared effect
• Closure = captures values, must be explicitly declared

---

✨ Final Recommendation

Approve with minor suggestions.

This PR significantly improves test coverage for a critical part of the type system. The two known gaps are properly documented and don't block the value of having these tests in place.

Suggested follow-up issues:

1. Fix row variable unification for quotation type matching (addresses both KNOWN GAPs)
2. Add tests for closures with Yield effects
3. Add tests for closure propagation through bi/dip combinators
4. Update TYPE_SYSTEM_GUIDE.md to document closure vs quotation semantics

Nice work! 🎉

---

Review generated by Claude Code - feedback welcome at https://github.com/anthropics/claude-code/issues