https://github.com/navicore/patch-seq/issues/393

[navicore]

No description provided.

[claude]

Code Review — Issue #393: Aux Stack Inside Quotations

Overall this is a well-motivated change with clear comments and good test coverage for the typechecker layer. A few things worth addressing before merge:

---

Bug: quotation_id_stack leaked on error paths

In typechecker.rs infer_quotation, the ID is pushed at the top but only popped at the happy path. Two error exits skip the pop:

Early return via ? during body inference:

self.quotation_id_stack.borrow_mut().push(id);
// ...
let (result, subst, effects) =
    self.infer_statements_from(body, &fresh.inputs, false)?;  // ← leaks id on stack

Early return on unbalanced aux check:

if quot_aux != StackType::Empty {
    return Err(format!("Quotation has unbalanced aux stack..."));  // ← leaks id, doesn't restore saved_aux
}

current_aux_stack is also not restored to saved_aux on either error path. This probably doesn't matter for single-shot compilation (errors abort), but it does affect tests that call check_program and inspect the error. If the TypeChecker were ever reused across calls the state corruption could cause mysterious failures.

Suggested fix: use a local RAII guard pattern, or restructure to always run cleanup before returning. The simplest fix is to pop and restore before every return Err(...).

---

Duplicate aux-slot allocation loop

In codegen/words.rs the same loop appears in both the Type::Quotation and Type::Closure branches (lines ~210–218 and ~289–297). Extract into an emit_aux_slots(count) helper:

fn emit_aux_slots(&mut self, count: usize) -> Result<(), CodeGenError> {
    self.current_aux_slots.clear();
    for i in 0..count {
        let slot_name = format!("aux_slot_{}", i);
        writeln!(&mut self.output, "  %{} = alloca %Value", slot_name)?;
        self.current_aux_slots.push(slot_name);
    }
    Ok(())
}

Matches the CONTRIBUTING.md guideline: "Each function does one thing" and avoids the divergence risk if the alloca format ever changes.

---

Missing integration test for the feature

tests/integration/src/test-aux.seq exists but has no cases for aux inside quotations. The example file at examples/language/aux-in-quotations.seq is great documentation but (presumably) isn't run as part of just test. A few cases would catch LLVM-level regressions:

• Basic >aux/aux> round-trip inside a [ ... ] call
• Nested quotations each with independent aux
• Aux inside a list.each / list.fold callback (the motivating use case)

This is the part of the change that has the most surface area (codegen path, LLVM alloca emission, slot indexing) and currently has zero automated coverage.

---

Minor: test_aux_in_quotation_balanced_accepted effect annotation

The word signature in the test is:

effect: Some(Effect::new(
    StackType::Empty,
    StackType::singleton(Type::Quotation(...)),
)),

…but the body is just Statement::Quotation { id: 0, body: vec![...] } which pushes the quotation. That's correct. Just noting that the test quotation body calls >aux then aux> to round-trip an Int, so the effect Int -- Int is right. Readable as-is.

---

Nit: sss.seq comment accuracy

The demo-fold-with-aux example comment says "sum of squares (running)" — the actual computation is acc*acc + elem*elem where acc starts at 0 and accumulates, which is more like a sum of paired squares rather than a running sum-of-squares in the traditional sense. Worth tightening the comment to match what the code actually computes to avoid confusing readers who try to follow the math.

---

Summary

Correctness	Error-path state leak in infer_quotation needs fix
Design	Duplicate alloca loop — minor but worth extracting
Test coverage	No integration test for the LLVM codegen path
Docs/examples	High quality; aux-in-quotations.seq is excellent
Overall	Solid design, clean typechecker expansion; address the error paths and add one integration test before merging

[claude]

Code Review — Issue #393: Aux Stack in Quotations

This is a clean, well-scoped implementation. The design is sound (quotations already compile to independent LLVM functions, so giving each its own aux slot table is a natural fit), and the PR follows existing patterns throughout. A few notes below:

---

What Works Well

IIFE cleanup pattern (typechecker.rs ~L262)
Using an immediately-invoked closure to guarantee scope restoration on all exit paths is smart. The old code left the typechecker with a corrupt quotation_id_stack and stale aux stack if infer_statements or the balance check returned an Err. The new pattern makes correctness obvious.

Per-quotation depth tracking
Replacing the single in_quotation_scope: Cell<bool> with a Vec<usize> ID stack and a per-ID depth map is the right data structure for nested quotations. The old boolean would have silently failed (or silently succeeded) for cases like [ [ >aux ] call aux> ].

emit_aux_slots extraction
Extracting the alloca-emit loop removes duplication between codegen_word, the quotation arm, and the closure arm, and each of those three call sites gets visually lighter.

Test quality
The integration tests use test.assert-eq rather than print-and-eyeball, so seqc test actually catches regressions. The fold-based test is a particularly good choice because the expected result can be verified by hand (Horner's method gives 26 for [1,2,3,4] starting from 0).

---

Minor Issues

Unnecessary .clone() when restoring expected type (typechecker.rs)

*self.expected_quotation_type.borrow_mut() = expected_for_this_quotation.clone();

expected_for_this_quotation is not used after this line. The .clone() keeps it alive but it's immediately dropped. You can just move it:

*self.expected_quotation_type.borrow_mut() = expected_for_this_quotation;

(This is purely cosmetic — zero runtime cost either way.)

emit_aux_slots always clears current_aux_slots

The function doc says "Caller is responsible for resetting current_aux_sp if needed" but doesn't mention the implicit clear of current_aux_slots. In practice the callers either std::mem::take the vec first (quotation path) or simply don't care about prior contents (word path), so this is safe today. Worth a one-line doc note so future callers aren't surprised:

/// Clears `current_aux_slots`, then emits `count` fresh `alloca %Value`
/// instructions and records their names.

---

Considerations

Expected type restoration on error

In the original code, if infer_statements returned Err, expected_quotation_type was left as None (the cleared value). In the new code it's restored to its pre-quotation value. This is strictly better — the typechecker is more robust for callers that inspect errors and continue. Just noting it as an intentional behavior change in case it needs a test.

Quotation ID uniqueness assumption

The implementation implicitly requires parser-assigned quotation IDs to be stable and unique across the whole program. This appears to be an existing guarantee, but quotation_aux_slot_counts is keyed on usize with no validation. A silently reused ID would give the wrong alloca count with no diagnostic. This is probably fine as-is, but it may be worth a debug_assert! somewhere that IDs entering set_quotation_aux_slot_counts are distinct.

No runtime test for repeated invocation

The aux_slot_N allocas live on the LLVM stack frame of each quotation invocation, so calling the same quotation in a loop should work correctly — each call gets a fresh frame. There's no integration test exercising this path (e.g., list.each over a list with more than one element where each iteration uses aux). test-aux-in-list-each-via-fold does hit this path with four elements, so it's actually covered. ✓ (I initially thought it wasn't — ignore this bullet.)

---

Test Correctness Spot-check

• test-quotation-aux-round-trip: 5 >aux 10 aux> i.+ → 15. ✓
• test-nested-quotation-aux: outer stashes 5, inner computes 100 + 10 = 110, outer adds 110 + 5 = 115. ✓
• test-aux-in-list-each-via-fold: Horner on [1,2,3,4] from 0 → 1 → 4 → 11 → 26. ✓
• demo-fold-with-aux (example): binary 1011 via Horner → 11. ✓
• test-aux-in-closure: captured=10, input=5 → stash 5, 10+1=11, restore 5, 11+5=16. ✓

---

Summary

Approved from a design and correctness standpoint. The two items worth a quick follow-up are the unnecessary .clone() (cosmetic) and adding a doc note about the implicit clear in emit_aux_slots. Everything else looks solid.