⏺ Phase 2c is complete. Here's a summary:

[navicore]

Phase 2c: Effect-based tracking

Added a compile-time check that quotations with Yield effect cannot be called directly with call - they must be wrapped with strand.weave.

Changes made:

1. crates/compiler/src/typechecker.rs:
   - Added check_call_yield_effect() method (~20 lines)
   - Integrated check into infer_word_call() for call
   - Added 3 new tests:
   • test_call_yield_quotation_error - calling yield quotation fails
   • test_strand_weave_yield_quotation_ok - strand.weave on yield quotation succeeds - test_call_pure_quotation_ok - calling pure quotation succeeds

Error message:
Cannot call quotation with Yield effect directly.
Quotations that yield values must be wrapped with strand.weave.
Example: [ yielding-code ] strand.weave instead of [ yielding-code ] call

Issue #142 is now substantially complete:

• ✅ Phase 2a: Single-word data flow analysis
• ✅ Phase 2b: Cross-word analysis
• ✅ Phase 2c: Effect-based tracking (Yield → strand.weave requirement)

All 224 Rust tests and 123 integration tests pass.

[claude]

PR Review: Phase 2c Effect-based Tracking

Summary

This PR adds a compile-time check that prevents calling quotations with Yield effects directly with call. Such quotations must be wrapped with strand.weave. The implementation is clean, focused, and well-tested.

✅ Strengths

1. Clean, Minimal Implementation

The core logic in check_call_yield_effect() (crates/compiler/src/typechecker.rs:964-982) is concise and clear:

• Properly checks both Quotation and Closure types
• Provides helpful error messages with examples
• Integrates seamlessly into existing infer_word_call() flow at the right location (before applying effects)

2. Excellent Test Coverage

Three well-designed tests cover the key scenarios:

• test_call_yield_quotation_error: Verifies the error case
• test_strand_weave_yield_quotation_ok: Confirms strand.weave works correctly
• test_call_pure_quotation_ok: Ensures pure quotations aren't affected

Each test includes clear comments explaining the scenario and expected behavior.

3. User-Friendly Error Messages

The error message at line 976-979 is exemplary:

• Explains why it's wrong
• Shows how to fix it with a concrete example
• Uses consistent terminology

4. Proper Integration

• Correctly placed check before effect application (line 950)
• Uses existing has_yield() infrastructure from the type system
• Doesn't break existing functionality (all 224 Rust tests + 123 integration tests pass)

🔍 Observations & Questions

1. Stack Cloning (Line 968)

if let Some((_rest, top_type)) = stack.clone().pop() {

Minor performance consideration: The stack.clone() is necessary for peeking without mutation, but could be optimized if StackType had a peek() method. This is not a blocker—the performance impact is negligible for compile-time checks, but worth noting for future refactoring.

2. Consistency with Other Call-like Operations

The check is applied specifically to "call". Should similar checks apply to other words that might execute quotations? Looking at builtins.rs:

• times: Takes a quotation with effect (a -- a) - should this also reject Yield quotations?
• cond: Variable arity conditional that executes quotations
• Any user-defined words that might call quotations

Question: Is the intent that:

1. Only direct call is restricted (current implementation)
2. OR all quotation execution should be checked?

If #1, this is correct. If #2, we might need additional checks. Based on issue #142's goals, restricting just call seems correct—other operations likely have their own semantics.

3. Potential Edge Cases

What happens in these scenarios?

Case A: Indirect call through closure

: indirect ( -- ) 
  [ [ yield ] ] call call ;  # Outer quotation contains yield quotation

Case B: Yield quotation stored and called later

: store-yielder ( -- Quot ) [ yield ] ;
: use-it ( -- ) store-yielder call ;  # Error caught here?

I suspect Case B might not be caught since the Yield effect may not propagate through the returned quotation type from store-yielder. This might be acceptable (caller responsibility), but worth documenting.

4. Error Reporting Location

The error doesn't reference a span for source location. While the error message is clear, users might benefit from knowing where in their code the problematic call appears. This isn't critical since the current implementation is consistent with other type errors in the codebase.

🎯 Suggestions (Optional Enhancements)

1. Add Documentation Comment

Consider expanding the doc comment on check_call_yield_effect to explain the rationale:

/// Check if `call` is being applied to a quotation with Yield effects.
/// 
/// Quotations with Yield must be wrapped in strand.weave to handle the yields.
/// This prevents runtime errors where yielding code is executed in a non-strand context.
///
/// # Rationale
/// Direct calls to yielding quotations would attempt to yield outside of a strand,
/// which is undefined behavior. strand.weave sets up the necessary runtime context.

2. Consider Additional Test Cases

To ensure robustness:

// Test: Closure with Yield (not just Quotation)
#[test]
fn test_call_yield_closure_error() {
    // Verify that Closures with Yield effects are also caught
}

// Test: Empty stack case
#[test] 
fn test_call_empty_stack() {
    // Ensure graceful handling when call is used with empty stack
    // (Should fail with type error, not panic)
}

3. Integration Test

Add a .seq file in the integration tests directory to verify the error message appears correctly in end-to-end compilation:

# test-files/phase2c-yield-call-error.seq
: bad-example ( -- )
  [ yield ] call ;  # Should produce compile error

🔒 Security & Safety

✅ No security concerns identified
✅ No unsafe code introduced
✅ Type-safe implementation using existing type system infrastructure

🚀 Performance

✅ Compile-time check only—zero runtime overhead
✅ Minimal performance impact during type checking (single stack peek per call)

📝 Code Quality

• Style: Consistent with existing codebase ✅
• Comments: Clear and helpful ✅
• Naming: Descriptive function names ✅
• Error handling: Appropriate use of Result ✅

Final Verdict

✅ APPROVE - This PR successfully implements Phase 2c as specified in issue #142. The code is clean, well-tested, and integrates smoothly with the existing type system. The suggested enhancements are minor improvements, not blockers.

Recommendation: Merge after considering whether the edge cases in observation #3 need to be addressed (they may be out of scope for this phase).

---

Great work completing Phase 2c! The progression from pattern-based linting to effect-based compile-time checking is a significant improvement in safety guarantees. 🎉