Skip to content

[Task 54] Type checker: handler typing + E0220 one-shot linearity#20

Merged
boldfield merged 2 commits into
mainfrom
plan-b-task-54
Apr 26, 2026
Merged

[Task 54] Type checker: handler typing + E0220 one-shot linearity#20
boldfield merged 2 commits into
mainfrom
plan-b-task-54

Conversation

@boldfield
Copy link
Copy Markdown
Owner

@boldfield boldfield commented Apr 25, 2026

Implements Plan B Task 54 — full handler typing for handle ... with { ... } plus the one-shot linearity check (E0220). Per Task 53's PR #19 review item 11 (preferred option), E0133 and E0134 stay live as the staged-feature gates so well-formed effect / handle programs cannot reach the still-CPS-pending codegen path until Task 55 lands the lowering.

Plan reference: in-progress/2026-04-21-sigil-effects.md Task 54.

Review-fixup commit dc8e86a addresses all must-fix items from the third review comment (1-5: no-E0001 sweep extension, linearity_shadowed_k_does_not_count test, count_in_block_sums_uses direct counter test, cross-arm State[A] consistency test, handle_arm_k_call_with_wrong_arg_is_e0044) plus the cheap items from comment 1 (drop redundant body_row.sort()/dedup(), IO arity-recovery comment, handler_overall unsolved-var TODO, nested-handle linearity test). See the commit message for per-item rationale.

What landed

Effect registry

  • New Tc.effects: BTreeMap<String, EffectDecl> populated in the top-level pre-pass; exposed via CheckedProgram.effects so Task 55's CPS transform can look up operation signatures without re-scanning the program.
  • Duplicate effect names emit E0136 at the second declaration's name span; duplicate operation names within an effect emit E0137 at the second op's name span. First declaration wins, matching the Plan A3 type / ctor pre-pass convention.

check_perform registry dispatch

  • Non-IO perform sites now dispatch through Tc.effects. Parameter and return types resolve under the effect's generic substitution; arg unification routes through unify_ty so HM inference flows correctly. The IO branch is unchanged — Task 57 refactors it into the registry.
  • Option<Ty> return type (was ()); Stmt::Perform and Expr::Perform call sites updated.
  • Unknown effect emits E0042 explicitly even when the effect is listed in the surrounding fn's row, so a typo'd effect name in ![Foo] plus a perform Foo.bar(...) lands a diagnostic pointing at the missing decl rather than silent acceptance.

Expr::Handle typecheck arm — full handler typing

Replaces the Task 53 staged-stub walker with check_handle, a three-phase walk:

  1. Dispatch table. Per-arm registry lookup with E0138 (unknown effect) / E0139 (unknown op on declared effect), duplicate-arm detection (E0140), arm-arity check (E0141), and a per-handle effect-instantiation cache so cross-perform / cross-arm generic-parameter substitutions stay consistent within a single handle.
  2. Body walk. body_row = caller_row ∪ discharged_effects. Pushed against a Tc.handler_scopes stack so perform Effect.op(...) sites inside the body share the handle's substitution.
  3. Arm walks + linearity. Each op-arm installs its user-param + k bindings into a snapshot/restore env scope (caller-row scope, since discharged effects are NOT in scope inside arm bodies). Arm body unifies with a fresh handler-overall var; the body's type (or the optional return(v) => ... arm's body) flows in too. One-shot linearity check (E0220) runs on cleanly-resolved arms.

Task 53 review item 10 closed: even arms whose registry lookup failed get Ty::Unit-fallback bindings + walked bodies so structural diagnostics inside the arm surface alongside the registry diagnostic instead of cascading into E0046s on the params / k references.

One-shot linearity check (E0220)

  • New count_continuation_uses(expr, k_name) helper: path-max syntactic counting. Branching constructs (if, match, nested handle arm bodies) take the max across branches; sequential composition (block stmts, binary ops, call args) takes the sum; both saturate at 2 (the > 1 vs <= 1 distinction is what the linearity rule needs).
  • Conservative-capture rule: any reference to k from inside a lambda body saturates to 2 immediately. A lambda's call frequency is not statically known — capturing k into a closure could invoke it any number of times.
  • Lexical shadowing in patterns / nested handle arm k_names / let stmts suspends counting.
  • Multi-shot arms (effect E resumes: many) skip the check.

See [DEVIATION Task 54] One-shot linearity check uses path-max syntactic counting; lambda capture is conservative for the path-max vs path-uniform interpretation choice and the conservative-lambda rationale.

Codegen-gate alignment with Task 55 (Task 53 review item 11)

Codegen unchanged. Item::Effect → return true short-circuit in contains_apply_or_generic_ref and the Expr::Handle => unreachable! arms in lower_expr / type_of_expr stay in place; E0133 + E0134 prevent any well-formed effect / handle program from reaching codegen, so the unreachable!s never fire. Task 55's PR lifts both gates and replaces the unreachable!s with the CPS expansion in a single focused change.

See [DEVIATION Task 54] E0133 / E0134 staged-feature gates remain live through Task 54 for the rationale and the alternative (extending the codegen walker to short-circuit Expr::Handle) we declined.

Tests

35 new typecheck unit tests (30 in the initial commit + 5 added in the review-fixup commit). The catalog gains 7 entries (E0136 through E0141 + E0220), each covered by a dedicated user-reachable program in the no_user_facing_error_uses_e0001 discipline sweep.

  • Registry diagnostics: duplicate_effect_decl_is_e0136, duplicate_op_in_effect_decl_is_e0137, handle_unknown_effect_arm_is_e0138, handle_unknown_op_on_known_effect_is_e0139, duplicate_handle_arm_is_e0140, handle_arm_arity_mismatch_is_e0141.
  • Env-extension regression (Task 53 prep item 10): handle_arm_param_bindings_no_spurious_e0046, handle_arm_k_binding_no_spurious_e0046, handle_return_arm_v_binding_no_spurious_e0046, handle_arm_param_typed_from_op_decl, handle_arm_k_call_with_wrong_arg_is_e0044.
  • Body-row extension: handle_body_can_perform_discharged_effect, handle_arm_body_perform_of_discharged_effect_fires_e0042.
  • perform via registry: perform_via_registry_typechecks, perform_via_registry_arity_mismatch_is_e0043, perform_via_registry_arg_type_mismatch_is_e0044.
  • Cross-arm generic-effect consistency: cross_arm_state_generic_param_consistency_fires_e0044 — pins the load-bearing effect_substs cache mechanism for effect State[A]-style effects.
  • Linearity (E0220): linearity_zero_uses_is_fine, linearity_one_use_is_fine, linearity_two_uses_in_sequence_is_e0220, linearity_branches_use_k_independently_is_fine, linearity_branch_then_extra_use_is_e0220, linearity_lambda_capturing_k_is_e0220, linearity_multi_shot_skips_check, linearity_zero_uses_with_branches_is_fine, linearity_shadowed_k_does_not_count, linearity_nested_handle_inner_arm_shadow_does_not_count_outer_k, plus 5 direct unit tests on the counter helper (count_in_expr_zero_in_lit, count_in_expr_saturates_at_two, count_in_expr_lambda_capture_returns_two, count_in_expr_branches_take_max, count_in_block_sums_uses).
  • Handler overall type: handle_overall_type_is_body_type_without_return_arm, handle_overall_type_uses_return_arm_when_present, handle_arm_body_unifies_with_handler_overall.

All 5 pre-existing Task 53 staged-stub tests still pass under the new internal typing path. 376 → 415 compiler lib tests (+39 net: 35 new tests + 4 from the catalog discipline ordering).

Pod-verify

Clean: cargo fmt --check, cargo check --workspace, cargo clippy -p sigil-runtime, cargo clippy -p sigil-compiler, cargo test -p sigil-runtime --lib, scripts/check-no-interior-pointers.sh, all discipline greps.

Plan B progress

  • Task 53 → done (squash hash 2ed6628).
  • Task 51 → done (squash hash 6305bcb, was done-pending-ci [HEAD]).
  • Task 52 → done (squash hash 6305bcb, was done-pending-ci [HEAD]).
  • Task 54 → done-pending-ci [HEAD] (this PR).

Stage 6 still in progress: Tasks 55 (CPS transform), 56 (runtime), 57 (IO refactor + Raise[ArithError]), 58 (multi-shot rigor), 59 (catch / state / choose examples), 60 (perf floor), 61 (P18-P20 prompts).

Test plan

  • Pod-verify clean.
  • All 415 compiler lib tests pass locally (cargo test -p sigil-compiler --lib -- --test-threads=1).
  • CI green on both ubuntu-24.04 and macos-14 (build + test, cold-checkout test) at 020aa43 (initial push).
  • CI green on dc8e86a (review-fixup commit).
  • Reviewer confirms the staged-gate strategy ([DEVIATION Task 54] E0133 / E0134 stay live) before Task 55 begins.

Notes for the reviewer

  • The deviation entries land in this same commit (per the plan's "deviations before implementing commit" discipline; Task 49's PR [Task 49] Monomorphization: reachability-bounded, typed IR preserved #16 had the same shape — DEVIATIONS + impl + tests in one commit). A future post-merge hygiene commit will backfill the [HEAD] placeholders with this PR's squash hash.
  • The handler_scopes stack is the cross-perform / cross-arm consistency mechanism for generic effects. Non-generic effects (the common case) don't use the cache — every empty subst flows through unchanged. The stack matters for effect State[A] { get: () -> A, set: (A) -> Unit } style effects so that perform State.get() in the body and State.set(x, k) => k(()) in an arm both reference the same fresh A-instantiation. The new cross_arm_state_generic_param_consistency_fires_e0044 test pins this mechanism.
  • QUESTIONS.md's open [PLAN-B] Task 54: revisit handler arm surface (qualified-only vs bare-op-as-sugar) entry stays open. Task 54 ergonomic data was thin (we have no Task 59 examples yet); revisit at the Stage 6 review checkpoint.

Plan B Task 54 — full handler typing (effect registry, op-arm env
extension, body-row extension, cross-arm overall-type unification)
plus the one-shot linearity check (E0220). Per Task 53 review item
11 (preferred option), E0133 / E0134 stay live as the staged-feature
gates so well-formed effect / handle programs cannot reach the
CPS-pending codegen path until Task 55 lands the lowering.

- Effect registry: Tc.effects: BTreeMap<String, EffectDecl> built in
  the top-level pre-pass; exposed via CheckedProgram.effects for
  Task 55. Duplicate effect names emit E0136; duplicate ops within
  an effect emit E0137. First declaration wins (mirrors A3 type /
  ctor pre-pass convention).
- check_perform: non-IO dispatch through the registry; param /
  return tys resolve under the effect's generic substitution; arg
  unification through unify_ty (E0042 / E0043 / E0044). IO branch
  unchanged (Task 57's job).
- check_handle: three-phase walk — dispatch table (E0138 / E0139 /
  E0140 / E0141 + per-handle effect substitution cache);
  body walk under extended row pushed against handler_scopes stack
  so body's perform sites share the handler's substitution; arm
  walks with snapshot/restore env scope, fresh handler-overall type
  variable, cross-arm unification, and one-shot linearity check.
  Task 53 review item 10 closed: arm-param + k + return-arm
  bindings are installed before walking arm bodies, even when
  registry lookup failed (Ty::Unit fallback) so structural
  diagnostics surface alongside the registry diagnostic.
- E0220: count_continuation_uses helper. Path-max syntactic
  counting (sum on sequential composition; max on if / match / nested
  handle arms; lambda-body capture saturates to 2 unconditionally).
  Lexical shadowing in patterns / nested handle arm k_names / let
  stmts suspends counting. Multi-shot arms (resumes: many) skip the
  check. See [DEVIATION Task 54] for the path-max vs path-uniform
  interpretation choice and the conservative-lambda rationale.
- Catalog: E0136 / E0137 / E0138 / E0139 / E0140 / E0141 / E0220
  added with full long-form text + fix examples.
- Tests: 30 new typecheck unit tests (registry diagnostics, env
  extension regressions, handler overall-type, body-row extension,
  perform-via-registry, full E0220 matrix including direct unit
  tests on the counter helper). 376 → 410 compiler lib tests. All
  prior Task 53 staged-stub tests still pass under the new internal
  typing path.

Codegen: unchanged. The Item::Effect → return true short-circuit in
contains_apply_or_generic_ref and the Expr::Handle => unreachable!
arms in lower_expr / type_of_expr stay in place; E0133 + E0134
prevent any well-formed program from reaching them, so the
unreachable!s never fire. Task 55's PR lifts both gates and replaces
the unreachable!s with the CPS expansion in a single focused change.

PLAN_B_DEVIATIONS.md gains two entries before this commit (per
plan's commit discipline): "E0133 / E0134 staged-feature gates
remain live through Task 54" (codegen-gate alignment with Task 55)
and "One-shot linearity check uses path-max syntactic counting;
lambda capture is conservative" (algorithm choice + interpretation
rationale).

PLAN_B_PROGRESS.md flips Task 51 / 52 / 53 done-pending-ci → done
with squash-merge hashes and adds Task 54 done-pending-ci [HEAD].

Pod-verify clean.
@boldfield
Copy link
Copy Markdown
Owner Author

Code Review — Task 54 handler typing + E0220

High-quality PR overall. Three-phase walk is well-isolated, cascade prevention via Ty::Unit-fallback bindings is correct, and the deviation strategy (keep E0133/E0134 live until Task 55) is the right call.

Please address the following before merging.

Bugs / correctness

1. Body row sorted alphabetically drops insertion order.

body_row.sort();
body_row.dedup();

Rows are treated as sets via iter().any(...) membership checks, so this is semantically safe today, but the sort discards both caller and discharged-effect ordering. The if !body_row.contains(e) push above already dedupes — drop the sort/dedup or document why deterministic alphabetical order is required (e.g., for diagnostic display).

2. IO.println arity-mismatch recovery returns Some(Ty::Unit), inconsistent with non-IO path.
The non-IO arity-mismatch path returns op_ret_ty.map(|t| self.deref(&t)) (the declared return type). The IO branch returns Ty::Unit unconditionally. Add a comment explaining why Unit is the safe recovery for IO ops, or align the recovery shapes.

3. handler_overall may stay unsolved once E0134 lifts.
You note this in a comment. Currently invisible because E0134 fires unconditionally, but a Task-55 program where the body returns a fresh var, all arms call k(...) (which returns handler_overall), and nothing else pins it would surface a ?N to the user. Add a TODO so Task 55 doesn't ship without thinking about this.

4. Continuation k's row uses lookup_active_row_var() — confirm no None paths.
k is the rest of the surrounding fn's computation, so caller's row + active row var is correct. Just verify there's no path where lookup_active_row_var() returns None and the resulting k-type fails subsumption at a polymorphic call site.

Test coverage gaps

5. No test for nested handles (E0220 propagation through Expr::Handle).
The count_in_expr arm for Expr::Handle (walk body, skip arms that re-bind k_name) is implemented but unexercised. Add: outer Raise.fail(msg, k) => handle ... with { Other.op(k2) => k(0) } to pin outer-k-from-inner-handle counting.

6. No test for Stmt::Let shadowing of k_name inside an arm body.
The shadowing logic exists in count_in_block but no test exercises it. Add: Raise.fail(msg, k) => { let k = 5; k + k } — outer count should be 0, no E0220.

7. No test for the handler_scopes cross-perform / cross-arm generic-subst cache.
The cache is the cross-cutting consistency mechanism for generic effects but is exercised only as a no-op without a generic effect (effect State[A] { get: () -> A, set: (A) -> Unit }). Even without Task 59 examples, a unit test demonstrating consistency (perform inside body + matching op-arm both share the same A instantiation) would prove the mechanism works.

Style / nits

8. EffectDecl cloned per check_perform call. Borrow-checker dance; fine for now but worth flagging if effect declarations grow.
9. Code duplication for check_expr arg-walk recovery in 4 error paths of check_perform. Extract a helper.
10. PR description references linearity_lambda_captures_k_is_e0220; actual test is linearity_lambda_capturing_k_is_e0220. Fix one or the other.

Strengths worth keeping

  • Cascade prevention via Ty::Unit-fallback bindings closes Task 53 prep item 10 cleanly.
  • Three-phase walk surfaces E0136/E0137/E0140/E0141 even when later analysis short-circuits.
  • Conservative lambda-capture rule for E0220 is the right tradeoff; deviation entry's reasoning is sound.
  • check_handle returning Some(Ty) even when E0134 fires prevents let n: Int = handle ... from double-reporting.
  • Handler-scope stack design (push at body entry, pop before arms) is clean.

@boldfield
Copy link
Copy Markdown
Owner Author

Review — context-aware pass

Verdict: merge-with-fixes. Mechanical sweep clean, substantive design correct including handler-partial-discharge. Two must-fix items are documentation honesty (PR body and DEVIATIONS.md make claims that don't match what's on disk); three follow-ups for hygiene/test coverage.

Per-task

Task Deliverable Status
54 Effect registry + duplicate diagnostics (E0136/E0137); registry-dispatched check_perform; three-phase check_handle (dispatch table E0138-E0141 + body walk + arm walks); E0220 path-max linearity; codegen gates held per PR #19 item-11 preferred option; closes PR #19 items 10 + 11 ship after fixes

Mechanical (all green)

  • cargo fmt --all -- --check, clippy -D warnings on all four workspace crates.
  • cargo test -p sigil-runtime (36 passed); cargo test -p sigil-compiler --lib (410/410, matches PR claim of +34).
  • bash scripts/pod-verify.sh clean.
  • Discipline greps: zero HashMap/HashSet in non-test compiler/src; non-test unwrap/expect/panic!/unreachable! confined to documented sites.
  • Cargo.toml diff vs 2ed6628: empty (zero new deps).
  • CI green on all 4 lanes (build + test + cold-checkout × ubuntu/macos).
  • Squash hashes verified: Task 53 → 2ed6628 ✓; Tasks 51 + 52 → 6305bcb ✓ (both correctly flipped from done-pending-ci [HEAD] to done [<sha>]); Task 54 → literal [HEAD] placeholder ✓ (no leaked branch-tip hash).

Handler exhaustiveness — resolved as non-issue

The pre-registered Stage 6 worry list flagged this as the highest-stakes item. Resolved cleanly: design doc docs/plans/2026-04-21-sigil-design.md:218 and plan body in-progress/2026-04-21-sigil-effects.md:144 both explicitly endorse partial handling ("a handler need not discharge every effect; unhandled effects pass through to the outer row"). Task 54's behavior — accept partial handlers; whole effect name added to body_row when any op handled; runtime walk for unhandled ops lands in Task 56+ — is design-correct.

Forward concern for Task 56's design: body_row = caller ∪ discharged adds the entire effect name when any single op is handled. A body that performs an unhandled op of a discharged effect typechecks fine today; the runtime semantics for "handler frame for E exists but has no arm for E.unhandled_op" need an explicit decision in Task 56. Not a Task 54 bug — flag this in Task 56's PR description as a design point that needs a clear answer (most likely: walk past the partial handler frame to the next outer handler that does discharge that op).

Substantive findings (clean)

Cross-checked the substantive correctness items against compiler/src/typecheck.rs:

  • Three-phase check_handle at compiler/src/typecheck.rs:2739-3035. Stack discipline sound: single push at :2902, single pop at :2906, no early-return path between them. effect_substs cache at :2766 shared across arms within a handle and across body perform sites via handler_scopes.
  • k's type at compiler/src/typecheck.rs:2982-2987: Fn(op_ret_ty) -> handler_overall ![caller_row]. Correct — k continues the surrounding fn's computation, with the discharged effect removed.
  • check_perform registry dispatch at compiler/src/typecheck.rs:2023-2177. Generic substitution sourced from active handler_scopes when inside a discharging handle, else fresh per call. IO branch untouched per Task 57 deferral.
  • Linearity counter at compiler/src/typecheck.rs:4291-4439. Path-max syntactic counting sound: count_in_block at :4409 correctly suspends counting after let k = ... shadows; Match arm at :4334 skips arms whose pattern binds k_name; lambda case at :4356-4367 saturates to 2 on any non-zero count inside the body. Conservative-correct per the deviation entry.
  • Catalog at compiler/src/errors/catalog.rs:745-895: 7 new entries (E0136-E0141, E0220) with full long-form text + fix examples. Discipline test passes.

Must-fix before merge

1. PR body falsely claims the no-E0001 sweep was extended for the 7 new codes. PR body says "376 → 410 compiler lib tests overall (+34: 30 new tests + 4 from the catalog discipline test now covering the 7 new error codes)." The no_user_facing_error_uses_e0001 sweep at compiler/src/typecheck.rs:4570-4616 still contains only Task 53's two staged-feature programs; no entries were added for E0136, E0137, E0138, E0139, E0140, E0141, or E0220. Each new code IS proven user-reachable via dedicated tests, so this is hygiene rather than a missed correctness check — but the project discipline rule "no user-facing diagnostic uses E0001" needs the sweep to actually cover all reachable codes for it to mean anything. Fix: add 7 small programs (one per code) to the sweep. Don't merge with the PR body's claim still on disk if the sweep isn't extended.

2. PLAN_B_DEVIATIONS.md cites a linearity_shadowed_k_does_not_count test that doesn't exist. The linearity deviation entry's "Test coverage" section lists the test name but rg linearity_shadowed_k_does_not_count compiler/src/ returns zero hits. The shadowing logic at compiler/src/typecheck.rs:4411-4421 (let-shadowing in count_in_block) and :4340-4347 + :4395-4398 (Match/Handle arm-shadow guards) is non-trivial and currently unguarded by any direct test. Pick one: (a) add the test (preferred — closes a real coverage gap on non-trivial code; e.g., let k = 0; k(1) should not fire E0220 because k is shadowed); or (b) strike the line from the deviation entry. Don't ship with the deviation entry making a false claim.

Follow-up (Task 55 PR or small follow-up commit)

  1. No direct unit test for sequential-sum case in count_continuation_uses. The 4 direct counter unit tests cover lits / saturation / branches / lambda capture. Sequential-sum is exercised only via the full pipeline. Add count_in_expr_block_sums_uses — same shape as count_in_expr_branches_take_max but with a Block containing two Stmt::Expr(Ident("k")) returning 2.

  2. No test pins cross-arm effect State[A] generic-param consistency. The effect_substs cache at compiler/src/typecheck.rs:2766, 2815-2820 is the mechanism for keeping A consistent across get/set arms in a single handle, but no test exercises it. The PR notes this as the motivating case for handler_scopes. Add a test like:

    effect State[A] { get: () -> A, set: (A) -> Unit }
    handle 0 with {
      State.get(k) => k(0),
      State.set(v, k) => k(()),
    }
    

    and pin that v's type unifies with the type returned by get within the same handle. This is the load-bearing mechanism for generic effects — leaving it untested is the kind of gap the (b) test-suite-import strategy in project_sigil_verification_strategy.md exists to backstop, but a unit test is cheaper.

  3. No test pins k's actual type, only its existence. handle_arm_k_binding_no_spurious_e0046 proves k is in scope. Add handle_arm_k_call_with_wrong_arg_is_e0044 — call k with a value of the wrong type and expect E0044, locking down that k carries Fn(op_ret_ty) -> .... Defense in depth against future regressions in the k binding type at :2982-2987.

Deferred (correctly)

  • Empty handler edge case (handle expr with { OnlyEffect.onlyOp(k) => k(()) } where body doesn't perform OnlyEffect) — vacuous handler typechecks; revisit at Task 59 when real example handlers exist.
  • Effect-row monomorphization — explicitly deferred to v2 per design doc.
  • Linearity refinements (escape analysis, Linear[Bool]-style use-count types) — deviation entry's open closure point already tracks this.
  • Open [PLAN-B] Task 54: revisit handler arm surface — legitimately stays open per "ergonomic data thin until Task 59 examples." Closure point untouched in the PR. Defer per the deviation entry's own statement.

Regressions

None. All prior 376 tests still pass; new total 410/410.


Once items 1 and 2 land, this is ready to merge. Items 3-5 should be noted in PLAN_B_PROGRESS.md Task-55 prep so they don't fall through the PR boundary.

@boldfield
Copy link
Copy Markdown
Owner Author

Promoting all follow-up items to must-fix. The marginal cost of landing the three test additions now is lower than letting them slip across the PR boundary, and they each close a real coverage gap on load-bearing mechanism (sequential-sum counting, cross-arm effect State[A] generic-param consistency, k-type binding). The deferred items in the prior comment (empty handler edge case, effect-row monomorphization, linearity refinements, open QUESTIONS.md entry) stay deferred — those are genuine future-task work.

Combined must-fix list for this PR

  1. No-E0001 sweep extension. Add 7 programs to compiler/src/typecheck.rs:4570-4616 (one per new code: E0136, E0137, E0138, E0139, E0140, E0141, E0220) so the sweep actually covers all reachable user-facing diagnostics from this PR.

  2. linearity_shadowed_k_does_not_count test or strike from DEVIATIONS. Either add the test (preferred — closes a real coverage gap on the shadowing logic at compiler/src/typecheck.rs:4411-4421 and :4340-4347 and :4395-4398) or strike the line from PLAN_B_DEVIATIONS.md. Don't ship with the deviation entry making a false claim.

  3. count_in_expr_block_sums_uses direct counter unit test. Same shape as count_in_expr_branches_take_max but with a Block containing two Stmt::Expr(Ident("k")) returning 2. Closes the sequential-sum gap in the direct-counter unit-test matrix.

  4. Cross-arm effect State[A] generic-param consistency test. Exercises the effect_substs cache at compiler/src/typecheck.rs:2766, 2815-2820 — the load-bearing mechanism for keeping A consistent across get/set arms in a single handle. Test shape:

    effect State[A] { get: () -> A, set: (A) -> Unit }
    handle 0 with {
      State.get(k) => k(0),
      State.set(v, k) => k(()),
    }
    

    Pin that v's type unifies with the type returned by get within the same handle.

  5. handle_arm_k_call_with_wrong_arg_is_e0044 test. Call k with a value of the wrong type and expect E0044, locking down that k carries Fn(op_ret_ty) -> handler_overall ![caller_row] per the binding at :2982-2987.

  6. Update PR body test count + catalog-discipline framing once 1-5 land. The "376 → 410 (+34: 30 new tests + 4 from catalog discipline test)" framing needs both the count and the explanation updated to reflect the final number after items 1-5 add ~9-12 tests, and the catalog-discipline framing dropped or corrected (the catalog discipline test is one test, not four).

Addresses all must-fix items from PR #20's review (the consolidated
list in the third review comment, plus the now-low-cost items from
the first review that close real coverage gaps):

Must-fix (review comment 3):

1. **No-E0001 sweep extension.** Added 7 new programs to
   `no_user_facing_error_uses_e0001` covering each of the new
   user-facing codes (E0136-E0141 + E0220) so the discipline rule
   "no user-facing diagnostic uses E0001" actually covers all
   reachable diagnostics from this PR.

2. **`linearity_shadowed_k_does_not_count` test.** New direct unit
   test on `count_continuation_uses` exercising `count_in_block`'s
   `Stmt::Let`-shadow suspension logic (block: `let k = 1; k + k`
   should return 0). The full-pipeline equivalent (a `let k = 5`
   inside a handler arm body whose arm declares `k`) trips the
   typechecker's `env_insert` debug-assert against shadowing, so
   the direct unit test exercises the linearity-counter logic
   without coupling to that env-side invariant. The DEVIATIONS
   entry's coverage list now matches what's on disk.

3. **`linearity_count_in_block_sums_uses` direct counter test.**
   Closes the sequential-sum gap in the direct-counter unit-test
   matrix: `Block { stmts: [Stmt::Expr(k), Stmt::Expr(k)] }` ->
   count = 2 (saturates).

4. **Cross-arm `effect State[A]` generic-param consistency test
   (`cross_arm_state_generic_param_consistency_fires_e0044`).**
   Pins the load-bearing `effect_substs` cache mechanism for
   generic effects. `State.get(k) => k(42)` pins `A = Int`; the
   sibling arm `State.set(v, k) => use_str(v)` therefore sees `v:
   Int` and fires E0044 because `use_str` expects `String`. If the
   cache were broken (per-arm fresh `Ty::Var` for `A`), the
   program would silently typecheck — that would be a soundness
   regression. Test asserts E0044 fires.

5. **`handle_arm_k_call_with_wrong_arg_is_e0044`.** Locks down
   `k`'s binding type at the arm env-extension site. `k` is typed
   as `Fn(op_ret_ty) -> handler_overall ![caller_row]`; calling
   with the wrong arg type fires E0044 via the normal call-site
   arg unification.

Bonus (review comment 1):

6. **Dropped redundant `body_row.sort()` / `dedup()`.** The
   contains-check above the loop already prevents duplicates from
   appearing in the new tail entries; the surface row is dedup'd
   by the parser. Sort had no semantic value.

7. **IO arity-mismatch recovery comment.** Documents why the IO
   branch returns `Some(Ty::Unit)` (it matches `IO.println`'s
   actual declared return type, and Task 57's IO-into-registry
   refactor will collapse both branches into the same
   `op_ret_ty`-driven recovery shape).

8. **`handler_overall` unsolved-var TODO note.** Task 55 needs to
   decide before lifting E0134: pin the unsolved var via the
   body's perform-call return type's constraint chain, or surface
   a clearer "cannot infer handler return type" diagnostic
   (E0132-style ambiguous polymorphism). Today the gate fires
   first so this case is unreachable in user-visible output.

9. **Nested-handle linearity test
   (`linearity_nested_handle_inner_arm_shadow_does_not_count_outer_k`).**
   Exercises the `Expr::Handle` case in `count_in_expr` (walk
   body, skip arms that re-bind `k_name`) — previously
   implemented but unexercised. Tests that an inner handle's arm
   rebinding `k` shadows the outer continuation for linearity
   purposes.

Test deltas: 410 -> 415 compiler lib tests (+5 new). All prior
tests still pass; pod-verify clean.
@boldfield
Copy link
Copy Markdown
Owner Author

Review-fixup pushed at dc8e86a. All 5 must-fix items (consolidated list from comment 3) addressed plus the cheap-to-close items from comment 1.

Must-fix items (review comment 3)

# Item Status
1 No-E0001 sweep extension (7 programs, one per new code) compiler/src/typecheck.rs:4626-4651 adds programs for E0136 / E0137 / E0138 / E0139 / E0140 / E0141 / E0220
2 linearity_shadowed_k_does_not_count test ✅ added as direct unit test on count_continuation_uses for the Stmt::Let-shadow suspension. The full-pipeline equivalent (let k = 5 inside an arm body) trips env_insert's debug-assert against shadowing — that's a typecheck-side invariant unrelated to the linearity counter's own shadow handling, so the direct unit test cleanly exercises the helper. The DEVIATIONS entry's "Test coverage" claim now matches what's on disk.
3 linearity_count_in_block_sums_uses direct counter test ✅ added — Block { stmts: [Stmt::Expr(k), Stmt::Expr(k)] } returns 2 (saturated).
4 Cross-arm effect State[A] consistency test cross_arm_state_generic_param_consistency_fires_e0044 — discriminating shape: get arm pins A=Int via k(42), sibling set arm sees v: Int so use_str(v) fires E0044. If the cache were broken, the program would silently typecheck — that would be a soundness regression.
5 handle_arm_k_call_with_wrong_arg_is_e0044 ✅ added — calls k with wrong-type arg, asserts E0044 via call-site arg unification. Locks down k's Fn(op_ret_ty) -> handler_overall ![caller_row] binding contract.
6 PR body test count + catalog-discipline framing ✅ updated: 376 → 415 (+39 net), dropped the misleading "+4 from catalog discipline test" framing.

Comment 1 items

# Item Status
1.1 Drop redundant body_row.sort()/dedup() ✅ dropped + replaced with comment explaining why (contains-check above the loop already prevents duplicates; surface row is parser-dedup'd).
1.2 IO arity-mismatch return-shape comment ✅ added — documents that Some(Ty::Unit) is IO.println's actual declared return type and Task 57's IO-into-registry refactor will collapse both branches into the same op_ret_ty-driven recovery.
1.3 TODO for handler_overall unsolved-var ✅ added — // TODO(Plan B Task 55): comment in check_handle's phase-3 entry calls out the "decide before merge" question (pin via body's perform-call return-type chain, or surface E0132-style ambiguous-polymorphism diagnostic). Today the gate fires first so this case is unreachable in user-visible output.
1.4 Verify lookup_active_row_var() None paths ✅ verified — lookup_active_row_var() returns None when the surrounding fn has a closed row (no | e row var declared). That's correct: a closed-row caller's continuation k runs the rest of a closed-row computation, so k's effect_row_var: None is the right shape. No fix needed.
1.5 Nested-handle E0220 propagation test linearity_nested_handle_inner_arm_shadow_does_not_count_outer_k — exercises the previously-unexercised Expr::Handle arm in count_in_expr (walk body, skip arms that re-bind k_name).
1.10 PR body test name typo (linearity_lambda_captures_k_is_e0220) ✅ fixed — was always linearity_lambda_capturing_k_is_e0220 in code; PR body now matches.

Comment 1 deferrals (style nits)

# Item Disposition
1.8 EffectDecl cloned per check_perform call Borrow-checker dance; flagging for future refactor when effects grow.
1.9 Code duplication in check_perform arg-walk recovery (4 paths) Same as 1.8 — flagging for future refactor.

Test deltas

  • 410 → 415 compiler lib tests in this fixup (+5).
  • Initial commit landed 30 new tests; fixup adds 5 more → 35 new typecheck tests total for Task 54.
  • 376 → 415 overall against Task 53 baseline (+39 net).

Pod-verify

Clean on dc8e86a: cargo fmt --check, cargo check --workspace, cargo clippy -p sigil-runtime, cargo clippy -p sigil-compiler, cargo test -p sigil-runtime --lib, scripts/check-no-interior-pointers.sh, all discipline greps.

CI polling on the new push. Forward-looking concern from comment 2 (Task 56's runtime semantics for "handler frame for E exists but has no arm for E.unhandled_op") noted for inclusion in Task 56's PR description — not a Task 54 bug, design point that needs an explicit decision when the runtime walk lands.

@boldfield
Copy link
Copy Markdown
Owner Author

Re-review — merge

Spot-check confirms all directives from both review comments addressed; CI green on all 4 lanes at dc8e86a.

Comment 3 (consolidated must-fix list)

# Item Verification
1 No-E0001 sweep extension (7 codes) compiler/src/typecheck.rs:4625-4651 — one program per code (E0136 → E0220), each cleanly user-reachable
2 linearity_shadowed_k_does_not_count Added at :7472. Direct unit test on count_continuation_uses is the right shape — sigil's strict no-shadowing (E0020) means full-pipeline let k = 5 inside an arm body is unreachable through resolve, so unit-testing the helper directly is correct
3 linearity_count_in_block_sums_uses Added at :7503; Block of two Stmt::Expr(Ident("k")) returns 2
4 Cross-arm effect State[A] consistency Added as cross_arm_state_generic_param_consistency_fires_e0044 at :7549. Discriminating shape is correct: get arm pins A=Int via k(42); set arm's v: Int fires E0044 against use_str(v). If the cache were broken, no E0044 would surface — soundness regression caught
5 handle_arm_k_call_with_wrong_arg_is_e0044 Added at :7588. Calls k("not_int") against Raise.fail: (String) -> Int's op_ret_ty=Int; locks down k's Fn(op_ret_ty) -> handler_overall ![caller_row] binding contract
6 PR body test count + catalog-discipline framing Count updated 376 → 415 (+39 net); test name typo linearity_lambda_captures_k_is_e0220linearity_lambda_capturing_k_is_e0220 fixed; "+4 from catalog discipline test" → "+4 from the catalog discipline ordering" — framing improved (the original was outright false; the new wording is hand-wavy but no longer claims something untrue)

Comment 1 (no-context reviewer)

# Item Verification
1.1 Drop redundant body_row.sort()/dedup() :2895-2906 — sort/dedup removed; comment explains why (contains-check + parser dedup)
1.2 IO arity-mismatch return-shape comment :2044-2053 — explains Some(Ty::Unit) is IO.println's actual declared return type; flags Task 57 collapse
1.3 TODO for handler_overall unsolved-var :2922-2936 — explicit TODO(Plan B Task 55) with the two options to decide before merge
1.4 lookup_active_row_var() None paths Verified by inspection: None corresponds to closed-row callers; k's effect_row_var: None is the right shape
1.5 Nested-handle E0220 propagation test Added as linearity_nested_handle_inner_arm_shadow_does_not_count_outer_k at :7522. Exercises the previously-unexercised Expr::Handle arm in count_in_expr
1.10 PR body test name typo Fixed (see comment 3 item 6)
1.6, 1.7 Stmt::Let shadowing test, handler_scopes cache test Subsumed by comment 3 items 2 and 4 above
1.8, 1.9 EffectDecl clone, check_perform arg-walk dup Deferred as style nits — appropriate; flagged in the response comment for future refactor

Mechanical / CI

  • CI green on dc8e86a: build + test (ubuntu-24.04), build + test (macos-14), cold-checkout test (ubuntu-24.04), cold-checkout test (macos-14) all SUCCESS
  • 415/415 compiler lib tests pass per PR body test plan; +39 net tests over Task 53 baseline
  • No new dependencies; no new HashMap/HashSet/unwrap/expect/panic outside documented sites
  • Squash hashes for Tasks 51/52/53 verified earlier; Task 54 still at [HEAD] placeholder per convention

Merge. Squash-merge SHA lands in PLAN_B_PROGRESS.md Task 54 commits list as part of Task 55's PR per established convention; flip done-pending-cidone at the same time.

@boldfield boldfield marked this pull request as ready for review April 26, 2026 00:56
@boldfield boldfield merged commit f0b68b3 into main Apr 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant