[Plan C Task 81] Fix DONE-path try_pop entry_depth respect#96
Conversation
…lti-perform composition Bug: nested sigil_run_loop (inside sigil_continuation_invoke's Phase 1) consumed outer_post_arm_k entries belonging to the OUTER run_loop. The DONE-path's `outer_post_arm_k_try_pop` at handlers.rs:1869 didn't gate on `outer_post_arm_k_entry_depth`, so the first inner DONE pop succeeded (removed outer's entry, leaving the slot null), and the next inner invoke's DONE pop dereferenced a null fn_ptr → SIGSEGV. Fix: cap try_pop at entry_depth so each run_loop only consumes entries it owns. Mirrors the symmetric DISCHARGED-path drain discipline (Layer 3c from PR #39 review §5). Closes the chained-let-yield + pure-tail multi-perform composition gap under runtime-N dischargers (`std/choose.sigil`'s `all_choices` / `first_choice` over a body like `let a = perform; let b = perform; a + b`). 2 new e2e tests pin: (1) inline single-shot k(0) baseline (#[test]), (2) `all_choices` over chained-let-yield + pure tail (#[test]). Two architectural gaps remain v2 work (#[ignore]'d e2e tests document the closure path): - Nested-if branched tail not classified by is_let_yield_prefix_then_branched_cps_tail_body (only Pure/CpsCall/ Perform leaves, not nested If/Match) - Pure let-bindings between perform let-yields get ANF-lifted, breaking the chain classifier's "every stmt is Perform/Call" rule Both are needed for natural Sudoku-shape bodies; PLAN_C_PROGRESS Task 81 records the architectural finding and v2 follow-up scope. 156 runtime + 288/289 e2e green (1 pre-existing macOS perf-floor failure unrelated). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Body shapes like:
let a = perform A;
let b = perform B;
if a == 1 && b == 1 { 99 } else { perform Choose.fail() }
ANF-elaborate to:
let a = perform A;
let b = perform B;
let _0 = a == 1; // ANF-lifted intermediate
let _1 = b == 1; // ANF-lifted intermediate
if _0 && _1 { 99 } else { perform Choose.fail() }
The chain classifiers (`is_simple_chained_let_yield_then_pure_tail_body`
and `is_let_yield_prefix_then_branched_cps_tail_body`) previously
rejected the lifted body because their per-stmt match required every
`Stmt::Let` value to be `Expr::Perform` or `Expr::Call`. Falling
through to Sync ABI silently broke `first_choice` / `all_choices`
composition: the second perform never made it through the runtime-N
discharger machinery.
Fix:
- Both classifiers now walk stmts and accept `Stmt::Let` with pure
RHS interspersed AFTER the last perform/call yield. Yield count is
returned (= chain_length); pure trailing intermediates are the
closure path for ANF-lifted compound subexpressions.
- New `TailPrefixLet` struct + `tail_prefix_lets: Vec<TailPrefixLet>`
field on `ChainStepRole::Final`. Pre-pass partitions stmts into
yield-bearing (`Perform` / `Call`) chain steps and pure trailing
intermediates; FINAL synth-cont's emit lowers each pure intermediate
in order (binding into env via `lower_expr` + `env.insert`) before
the existing pattern-c-detect / standard-tail dispatch.
- `collect_chained_synth_cont_captures` extended to walk
`tail_prefix_lets` values BEFORE adding their names to the bound
set, so helper params referenced only in lifted intermediates
(e.g. `let _0 = x + threshold` where `threshold` is a helper param)
flow through the closure record.
- Both `compute_user_fn_abi` chain-classification arms updated to
tolerate pure stmts (the chain-step push loop now skips them; the
cap check `captures.len() + chain_length + 1 < MAX_CLOSURE_ENV_SLOTS`
remains correct against `chain_length` as yield count).
- Test-only `collect_chained_synth_cont_captures` callers (7 sites)
updated to pass empty `&[]` for `tail_prefix_lets` (no behavioral
change for those tests).
New e2e test pins the ANF case end-to-end:
- `std_choose_first_choice_two_sequential_performs_anf_intermediates`
— body uses `if a == 1 && b == 1` shape; ANF lifts both compares
into trailing pure lets; chain classifier accepts; FINAL emit
evaluates them; first_choice short-circuits on `a=1, b=1 → 99`.
288/289 e2e green (1 pre-existing macOS perf-floor failure unrelated);
156 runtime + 0 regressions.
Remaining Plan C Task 81 work: nested-If branched-tail
classifier+emit (recursive walk for body shapes like `if a == 1 {
if b == 1 { ... } else { perform fail } } else { perform fail }`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ested
Body shapes like:
let a = perform A;
let b = perform B;
if a == 1 {
if b == 1 { 99 } else { perform Choose.fail() }
} else {
perform Choose.fail()
}
previously rejected by `is_let_yield_prefix_then_branched_cps_tail_body`
because `classify_branched_cps_tail_branch_expr` only accepted Pure /
CpsCall / Perform leaves at one level. Nested If/Match leaves fell to
Sync ABI; first_choice / all_choices then saw only the outer perform's
continuation.
Fix:
- New `BranchedCpsLeaf::Nested` variant. classify_branched_cps_tail_-
branch_expr recursively classifies nested If (and the if-desugar
Match-on-Bool 2-arm shape) when both sub-branches classify. At
least one sub-branch must be Cps-eligible (CpsCall / Perform /
Nested) per a new `leaf_is_cps_eligible` helper; otherwise the
whole tail is wholly pure and the existing Pure path handles it
(lower_expr lowers nested If as a value via internal phi).
- classify_branched_cps_tail_branch (block variant) refactored to
delegate to the bare-expr classifier, sharing the nested
recognition.
- is_let_yield_prefix_then_branched_cps_tail_body's has_cps_call
check uses leaf_is_cps_eligible (covers Nested transitively).
- FINAL synth-cont's branched-tail emit refactored from a 2-element
`for` loop to a `while let Some(work.pop())` loop. When the popped
leaf_kind is Nested, the loop calls detect_pattern_c_dispatch on
the nested tail, lowers the nested cond, brifs into 2 fresh blocks,
and enqueues each sub-branch's (block, expr, kind) tuple. Pure /
CpsCall / Perform leaves dispatch as before. Termination: each
iteration either emits a leaf (return_/jump terminates the block)
or pushes 2 strictly-smaller sub-trees; finite tail expression
bounds the iteration.
Two new e2e tests pin the nested case end-to-end:
- `std_choose_first_choice_two_sequential_performs_nested_if_tail`
— body uses `if a == 1 { if b == 1 { 99 } else { fail } } else
{ fail }`; first_choice short-circuits on a=1, b=1 → 99.
- (also kept `..._anf_intermediates` from the prior commit which
exercises the && form via ANF-lifted intermediates.)
290/291 e2e green (1 pre-existing macOS perf-floor failure unrelated);
156 runtime + 0 regressions.
Remaining Plan C Task 81 work: cross-fn perform composition through
Cps-call branched-tail leaves. The natural Sudoku `solve` body's
recursive `solve(set(...), cell+1)` Cps-call leaf composes via the
existing chain-step machinery (CpsCall leaf forwards caller_k_pair),
but the cross-fn end-to-end behaviour under first_choice needs
verification — a focused e2e test (`pick_outer` calls `pick_inner`
across a branched-tail CpsCall leaf) currently returns -1 (None);
investigating whether the issue is the Cps-call leaf's caller_k_pair
forward arithmetic or the helper-fn's chain caller_k_pair load.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Arm bodies in branched tails frequently contain ANF-lifted pure
intermediates as `Stmt::Let` stmts inside an `Expr::Block`:
if x == 2 {
p + x * 10 // ANF lifts to: { let _0 = x * 10; p + _0 }
} else {
perform Choose.fail()
}
The pre-extension `classify_branched_cps_tail_branch` rejected any
non-empty Block stmts, falling through to Sync ABI. Cross-fn
composition (`pick_outer` → `pick_inner(p)` via Cps-call branched-
tail leaf) silently broke under `first_choice` — both fns dropped to
Sync, the perform-site continuations weren't captured through the
runtime-N discharger machinery, and the discharger saw only the
outer perform's continuation.
Fix:
- `classify_branched_cps_tail_branch` accepts arm-body `Block`s
whose stmts are all `Stmt::Let` with pure RHS. Each pure stmt is
an ANF-lifted intermediate; the leaf-emit walker lowers them
before the leaf-emit dispatch.
- `detect_pattern_c_dispatch` returns 7-tuple: `(cond, then_stmts,
then_leaf, else_stmts, else_leaf, then_kind, else_kind)`. Stmts
are `&[Stmt]` slices into the arm body's Block (or `&[]` for
bare-Expr arm bodies). Per-branch `extract_leaf` returns both
stmts and leaf for both arm-body shapes (Block-wrapped, bare).
- Work-stack item structure extended to carry `&[Stmt]` per branch.
The work-stack iteration lowers each arm-body stmt's RHS via
`lower_expr` and binds the result into env via `lowerer.env.insert`
before dispatching the leaf-emit (Pure / CpsCall / Perform / Nested).
Nested re-dispatch threads its own arm-body stmts through the
recursive `detect_pattern_c_dispatch` call.
- `leaf_is_cps_eligible` (Plan C Task 81 helper) replaces inline
`matches!(.., CpsCall | Perform)` checks at both classifier and
detect-dispatch sites for consistency.
New e2e test pins cross-fn end-to-end:
- `std_choose_first_choice_multi_perform_site_recursive_branched`
— `pick_outer` performs `Choose.choose(2)`, branches on `p == 1`
to either `pick_inner(p)` (Cps-call leaf) or `perform fail`.
`pick_inner` performs `Choose.choose(3)`, branches on `x == 2` to
either `p + x * 10` (Pure leaf with ANF-lifted `_0 = x * 10`) or
`perform fail`. `first_choice` short-circuits at (p=1, x=2) → 21.
290/291 e2e green (1 pre-existing macOS perf-floor failure unrelated);
156 runtime + 0 regressions. The 9×9 std.choose-based Sudoku demo
is now expressible end-to-end via the natural `solve(board, cell)`
recursive shape (classifier accepts; emit composes through
first_choice).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes rustfmt diffs introduced by the Plan C Task 81 commits: - collect_chained_synth_cont_captures call sites at lines 324, 403: multi-line argument layout per rustfmt's width rules - chained-let-yield pre-pass binding_kinds.push at lines 8119, 8137: single-line method-chain - Tail-prefix-let lower at lines 13014, 13022: single-line builder.func.dfg.value_type / builder.ins().uextend - classify_branched_cps_tail_branch_expr nested Match arm at line 22977: tuple-let formatting - e2e.rs trailing whitespace at line 8890 No behavioral changes; cargo fmt --all -- --check now clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ReviewSolid PR. Runtime fix is correct and well-reasoned, the codegen extensions hang together, and test coverage for the interesting branched-tail/cross-fn cases is good. A few items below — nothing blocking, mostly polish + one small coverage gap. Correctness — runtime fix (handlers.rs:1881–1898)The fix is right. Capping Behavioral delta against the old code is null when Correctness — codegen classifier extensions (codegen.rs:22637–22723, 22788+)Both
Correctness — Pattern C work-stack (codegen.rs:13000–13048, ~13150+)Termination is fine — each iteration emits a leaf (terminating the block) or pushes 2 strictly-smaller sub-trees from The classifier↔detect symmetry on Match patterns now matches (both filter to 2 arms with Issues1. Coverage gap — pure-tail variant's tail-prefix-let path has no e2e test (minor). The branched-tail variant ( If ANF doesn't actually lift 2. Doc rot —
3. At
4. PR description is out of sync (minor). Summary says "2 new e2e tests"; the diff has 5 ( 5. Pre-yield pure-let limitation worth a one-line note (very minor). Both classifiers reject Things that look fine
|
…ename, breadcrumb 5 review items addressed: 1. **Coverage gap (item #1)** — added `std_choose_all_choices_- two_perform_then_two_pure_lets_pure_tail` e2e test pinning the pure-tail variant's `seen_pure_after_yield` path with hand-written multi-prefix lets (`let s = a + b; let s2 = s + 1; s2`). The symmetric branched-tail tests don't exercise this independently. 2. **Doc rot (item #2)** — `detect_pattern_c_dispatch` docstring at codegen.rs updated to reflect the 7-tuple return shape (added `then_stmts` / `else_stmts` describing arm-body Block stmts and how the work-stack emit consumes them). 3. **`tail_prefix_lets` widening misnaming (item #3)** — local renamed from `widened` to `env_value`. For non-Int slot kinds the path is no longer disguised as widening; instead each non-Int arm asserts the lower_expr contract (`debug_assert_eq!` against the slot kind's natural Cranelift width: I8 for Bool/ Byte/Unit, I32 for Char, pointer_ty for String/Closure/User). A future lower_expr change that breaks the contract now fails loudly instead of silently bit-mismatching at use sites. 5. **Pre-yield pure-let breadcrumb (item #5)** — branched-tail variant's catch-all `_ => return None` arm now carries the same one-line note the pure-tail variant has, calling out that pure lets BEFORE the first yield require a pre-yield env-prep emit phase that the current Plan C Task 81 extension doesn't ship. (Item #4 — PR description out of sync — addressed via `gh pr edit` in a separate operation since it doesn't touch the diff.) cargo fmt --all clean. 292/293 e2e green (1 pre-existing macOS perf- floor failure unrelated; up from 290 via the new pure-tail test). 156 runtime + 0 regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes 4 architectural composition gaps for
std/choose.sigil'sall_choices/first_choiceruntime-N dischargers, plus a runtime trampoline fix that fell out during diagnosis. Together these unblock natural Sudoku-shape body composition (solve(board, cell)recursive structure with branched-tail Cps-call leaves to itself +perform Choose.fail()leaves on constraint violations).What's pinned
1. Runtime — DONE-path try_pop respects entry_depth (
runtime/src/handlers.rs:1881)Trampoline's
DONE-pathtry_poppreviously ignoredouter_post_arm_k_entry_depth, so a nestedsigil_run_loop(insidesigil_continuation_invoke's Phase 1, when an outer chain step had pushed an entry) consumed entries belonging to the OUTERrun_loop. The first inner DONE pop succeeded (removed outer's entry, leaving the slot null), and the next inner invoke's DONE pop dereferenced a nullfn_ptr→ SIGSEGV. Fix capstry_popatentry_depth. Mirrors the symmetric DISCHARGED-path drain discipline (Layer 3c, PR #39 review §5).2. ANF-tolerant chain classifier (
compiler/src/codegen.rs)Both
is_simple_chained_let_yield_then_pure_tail_bodyandis_let_yield_prefix_then_branched_cps_tail_bodynow acceptStmt::Letwith pure RHS interspersed AFTER the last perform/call yield. ANF-elaborated bodies likelet a = perform; let b = perform; let _0 = a == 1; let _1 = b == 1; if _0 && _1 { ... }now classify (yield_count counted, tail-prefix lets folded into the FINAL synth-cont's preamble).collect_chained_synth_cont_captureswalks tail-prefix lets so helper params referenced only in lifted intermediates flow through the closure record.TailPrefixLetstruct +tail_prefix_lets: Vec<TailPrefixLet>field onChainStepRole::Final.3. Nested-If branched-tail classifier+emit (
compiler/src/codegen.rs)New
BranchedCpsLeaf::Nestedvariant + recursiveclassify_branched_cps_tail_branch_expraccepting nestedExpr::If/Expr::Match(if-desugar shape) when both sub-branches recursively classify. FINAL synth-cont emit's branched-tail dispatch refactored from a 2-elementforloop to awhile let Some(work.pop())loop. When the popped leaf_kind isNested, the loop callsdetect_pattern_c_dispatchon the nested tail, lowers the nested cond, brifs into 2 fresh blocks, and enqueues each sub-branch. Termination: each iteration either emits a leaf (return_/jump terminates the block) or pushes 2 strictly-smaller sub-trees. Newleaf_is_cps_eligiblehelper used at both classifier and detect-dispatch sites.4. Arm-body Block stmts threading (
compiler/src/codegen.rs)classify_branched_cps_tail_branchnow accepts arm-bodyBlocks whose stmts are all pureStmt::Let.detect_pattern_c_dispatchreturns 7-tuple(cond, then_stmts, then_leaf, else_stmts, else_leaf, then_kind, else_kind); per-branchextract_leafreturns both stmts and leaf. Work-stack item structure carries&[Stmt]per branch; the iteration lowers each arm-body stmt's RHS vialower_exprand binds into env before dispatching the leaf. Cross-fn perform composition (e.g.,pick_outer→pick_inner(p)via Cps-call branched-tail leaf) now works end-to-end throughfirst_choice.Test coverage
6 new e2e tests under
compiler/tests/e2e.rs(allstd_choose_*):std_choose_two_chained_let_yields_pure_tail_inline_single_shot— inline single-shot k(0) baseline (sanity check that 2-chained-let-yield codegen itself works for inline handlers; passes pre-fix and post-fix).std_choose_all_choices_two_sequential_performs_pure_tail—all_choicesover chained-let-yield + pure tail (was SIGSEGV pre-fix; passes post-fix with output4= 2*2 branches). Pins the runtime entry_depth fix.std_choose_all_choices_two_perform_then_two_pure_lets_pure_tail— pure-tail variant's hand-written multi-prefix lets (per review item Plan A1 code-review fixes: 6 issues from the post-review audit #1; pins symmetric ANF-tolerance independent of branched-tail).std_choose_first_choice_two_sequential_performs_anf_intermediates—if a == 1 && b == 1 { 99 } else { perform fail }shape with ANF-lifted_0 = a == 1,_1 = b == 1tail-prefix intermediates.std_choose_first_choice_two_sequential_performs_nested_if_tail—if a == 1 { if b == 1 { 99 } else { perform fail } } else { perform fail }nested-If shape; pins single-fn nested-If composition.std_choose_first_choice_multi_perform_site_recursive_branched— cross-fnpick_outer→pick_inner(p)via branched-tail Cps-call leaf with ANF-lifted_0 = x * 10inside arm body Block; mirrors Sudoku's naturalsolverecursive shape.Plan C Task 81 status
PLAN_C_PROGRESS.mdTask 81 documents the in-progress state. The 9×9 std.choose-based Sudoku demo is now expressible end-to-end via the naturalsolve(board, cell)recursive shape; shippingexamples/sudoku.sigil9×9 is the next concrete step.The 4×4 binary-
BranchSudoku from Plan D's smoke gate continues to ship inexamples/sudoku.sigil.Test plan
🤖 Generated with Claude Code