Plan A2 Stage 1.5: scaffolding + pod-verify + cold-checkout fix + debug_assert#2
Merged
Conversation
- PLAN_A2_PROGRESS.md: task tracker for Stage 1.5 (1.5.1-1.5.6) and all Stage 2 (20-28) / Stage 3 (29-35) tasks; carries the acceptance-is-CI reminder from the plan's Local verification strategy. - PLAN_A2_DEVIATIONS.md: empty template, same format rules as A1. - QUESTIONS.md: append a plan-tagging convention to the header so entries from A2+ are grep-able by plan origin (A1 entries implicitly pre-tag-era). Three tasks land in one commit because the scaffolding is genuinely atomic - each file is a few lines and none makes sense without the others.
…ring - scripts/pod-verify.sh: memory-safe local verification wrapper. Runs cargo fmt --check, cargo check --workspace, cargo clippy one crate at a time, cargo test -p sigil-runtime --lib, scripts/check-no-interior-pointers.sh, and the discipline greps. Does NOT run cargo test --workspace, cargo build --release, or the repro/smoke scripts - those are CI's responsibility. Defensive PATH extension for $HOME/.cargo/bin so non-login shells and constrained-CI contexts find cargo. - README.md: "Local verification on memory-constrained hosts" section explaining the pod-vs-CI split; updates the Status block to reflect A1 done / A2 current. - .github/workflows/ci.yml: invoke pod-verify.sh as an additional step before the full build+test matrix. Intent is to keep the pod-safe path continuously exercised on both hosts, not to replace the full matrix. Verified green locally on the Talos pod with CARGO_BUILD_JOBS=1: 23 runtime lib tests pass, clippy clean on both crates, fmt clean, check clean, interior-pointer check green. Discipline-grep warnings for unwrap/expect/panic are advisory; clippy -D warnings above them is the authority, and the false positives (parser.rs `self.expect(...)` method calls, test-module allowances) do not fail the script.
Problem: on a cold `cargo test --workspace`, cargo builds sigil-runtime only as an rlib (what sigil-compiler's dep graph needs) and does not reliably materialise `target/<profile>/libsigil_runtime.a` before the e2e test binary runs. The e2e test invokes the `sigil` compiler, which links against the staticlib — if missing, link fails. Fix (deviation: plan's option-a artifact deps needs nightly `bindeps`; option-b CI restructure alone doesn't fix local cold builds): - compiler/build.rs (new): checks for the staticlib at `target/<profile>/libsigil_runtime.a`; if missing, invokes `cargo build -p sigil-runtime` before sigil-compiler's build finishes. Env opt-out `SIGIL_SKIP_RUNTIME_STATICLIB_BUILD=1`. Clippy-disallowed panic/expect are allowed via inner attribute — build scripts are build-time infrastructure, not user-facing paths. - runtime/README.md: new "Cold-checkout build ordering" section documenting the approach and the opt-out. - .github/workflows/ci.yml: new `cold-checkout-test` job that runs `rm -rf target && cargo test --workspace` twice in succession on both hosts (plan's acceptance criterion). Separate from `build-test` because that job caches `target/` and cannot prove cold behaviour. - PLAN_A2_DEVIATIONS.md: full deviation entry with rationale (why option-a and option-b each fall short) and forward implications. Cold path cannot be validated on the headless pod without OOM risk. CI's cold-checkout-test is the authoritative gate.
…variant) Extract a `Tc::env_insert(name, ty)` helper that wraps `BTreeMap::insert` with `debug_assert!(prev.is_none(), ...)`. Both insertion sites use it: - `check_fn`: parameter bindings on entry to each function. - `check_block`: `let` bindings at declaration. The invariant: `resolve.rs` catches shadowing upstream (E0020), so duplicate inserts are impossible on a resolved AST. If a future caller (fuzzer harness, IDE integration, experimental pipeline) ever invokes `typecheck` on an un-resolved AST, the debug assertion fires instead of silently preferring the last insertion and emitting diagnostics against the wrong type. No behaviour change in release builds — the helper expands to a bare insert. All 14 typecheck lib tests still pass on the pod-safe path.
…to e2e test Revises the approach from commit f0a6212. That commit put the staticlib-materialisation rebuild in compiler/build.rs, which deadlocked under `cargo test --workspace` on a cold target directory: PR #2's first CI run sat on "cold run 1 of 2" for 47+ minutes on both Linux and macOS before being cancelled. The outer cargo holds per-build-unit locks while build.rs runs, so the nested `cargo build -p sigil-runtime` blocked trying to acquire compatible locks. Revised approach: - compiler/build.rs: simplified to rerun-if-changed hints only. No nested cargo invocation. No longer panics/expects, so the clippy-disallowed-macros/methods allow is no longer needed (attribute removed). - compiler/tests/e2e.rs: new `ensure_runtime_staticlib(root, sigil_bin)` helper called at the top of the `hello` test. Detects profile from env!("CARGO_BIN_EXE_sigil")'s path; invokes `cargo build -p sigil-runtime` (with --release if applicable) if the staticlib is missing. Runs at test-run time, after outer cargo has released its build-unit locks — no deadlock. The `SIGIL_SKIP_RUNTIME_STATICLIB_BUILD` env var is gone — callers that pre-build the staticlib get the same short-circuit behaviour via the existence check in ensure_runtime_staticlib. Documentation updates: - runtime/README.md "Cold-checkout build ordering" section rewritten. - PLAN_A2_DEVIATIONS.md: new [Task 1.5.5 revision] entry at top; original f0a6212 entry preserved for history. - PLAN_A2_PROGRESS.md: notes updated with the deadlock history and revised approach. Pod-verify clean locally; CI cold-checkout job will be the authoritative gate.
- Keywords: true, false, if, else, match. - Binary/unary operators: Plus, Minus, Star, Slash, Percent, EqEq, NotEq, Lt, Gt, LtEq, GtEq, AndAnd, OrOr, FatArrow (=>). Bang is unchanged; parser decides unary-not vs effect-row-bang from context. - Char literals: `'x'`, `'\n'`, `'\t'`, `'\r'`, `'\\'`, `'\''`. The lexer does not distinguish unary from binary `-` per plan — both emit TokenKind::Minus and the parser routes via precedence. Two-char lookahead wins over single-char for `->`, `==`, `=>`, `!=`, `<=`, `>=`, `&&`, `||`; bare `=`/`<`/`>` fall through to their single-char tokens. Errors: empty char literal `''`, unterminated `'x`, multi-char `'ab'`, and unknown escape `'\q'` all produce E0010 with spans at the offending bytes. `/` alone is Slash; `//`/`/*` are consumed as comments by the existing whitespace/comment skipper before reaching the operator matcher, so no ambiguity. Tests: 6 original lexer tests preserved; 9 new covering the Stage-2 surface. Parser/typecheck tests unchanged (the new tokens aren't yet parsed — task 21).
…nstant-fold
AST (compiler/src/ast.rs):
- New Expr variants: BoolLit, CharLit, Binary, Unary, If, Match.
- Supporting types: BinOp (Add/Sub/Mul/Div/Mod/Eq/NotEq/Lt/Gt/LtEq/GtEq/
And/Or), UnOp (Neg/Not), MatchArm, Pattern (IntLit/BoolLit/CharLit/
Wildcard).
Parser (compiler/src/parser.rs):
- Pratt-style `parse_expr_prec(min_prec)` with left-associative binary
operators. Precedence ladder: `||` (1) < `&&` (2) < `== !=` (3) <
`< > <= >=` (4) < `+ -` (5) < `* / %` (6). `parse_unary` handles
prefix `-` and `!` above all binary ops. `parse_postfix` preserves
the existing call-chain logic.
- `parse_primary` now recognises `true`/`false`/char-literal atoms,
parenthesized expressions, `if`, and `match`.
- `parse_if_expr`: `if cond block 'else' block` (else branch required).
- `parse_match_expr`: `match expr { pat => expr (',' pat => expr)*
','? }` with `parse_pattern` handling int-lit, bool-lit, char-lit,
and `_` wildcard. Negative int patterns `-N` supported via the same
constant-fold trick.
- `-<int-literal>` constant-folds at parse time into a single
`Expr::IntLit(-n, ..)` rather than `Unary(Neg, IntLit(n, ..))`.
`wrapping_neg` is used so `-i64::MIN` does not panic on debug.
- Grammar docblock updated to reflect Stage 2.
Typecheck (compiler/src/typecheck.rs):
- New Expr variants are accepted but rejected with E0043
"Stage-2 expression shapes are parsed but not yet typed (plan A2
task 22)". Task 22 replaces with real typing rules (Bool/Char/Byte,
binop rules, if-unification, match exhaustiveness).
Tests: 15 parser lib tests (12 new) cover precedence pairs (Mul>Add,
Lt>Add, Eq>Lt, Or<And), left-associativity, paren override,
constant-fold of `-5`, Unary Neg on non-literal, Unary Not on bool,
`if/else` round-trip, multi-arm match with wildcard, and char-lit
expression. All 15 lexer tests still pass.
No behaviour change for Stage-1 hello-world: parser produces the same
AST it did before and runtime pipeline is untouched.
… match Add `Bool`, `Char`, `Byte` to `Ty`; wire `ty_from_type_expr` and `type_matches` for each. `check_block` now returns `Option<Ty>` so `if` branch unification can see block types. Typing rules: - `+ - * / %`: Int -> Int -> Int - `< > <= >=`: Int -> Int -> Bool - `&& ||`: Bool -> Bool -> Bool - `== !=`: T -> T -> Bool where T is a primitive; operands must agree - Unary `-`: Int -> Int; Unary `!`: Bool -> Bool - `if` condition must be Bool; branches must unify - `match` pattern/scrutinee must match; arm bodies must unify; arms must be exhaustive (Bool: both polarities or wildcard; other primitives: wildcard required; Unit: non-empty arm list) Catalog additions: E0060 (binop operand type), E0061 (unary operand type), E0062 (if-cond not Bool), E0063 (if-branch disunion), E0064 (pattern/scrutinee mismatch), E0065 (match-arm disunion), E0066 (non-exhaustive match). QUESTIONS.md logs the PLAN-A2 Byte-ordering discrepancy between the plan's explicit `< > <= >=`: Int -> Int -> Bool rule and its Byte- feature paragraph that mentions Byte ordering. Implementor chose strict form (Int-only). Reviewer may override. 25 new typecheck tests added; 88 compiler lib tests all pass locally. Also backfills the existing E0044 message to use `ty_display` for consistency with the new diagnostics.
… ANF Desugars `Expr::If` into `Expr::Match` with `true`/`false` literal pattern arms. Pure-expression branches unwrap to their tail expression; branches with statements are wrapped in the new `Expr::Block(Box<Block>)` AST variant so they survive as an arm body without changing `MatchArm::body`'s type. Flattens `Expr::Binary` and `Expr::Unary` operands into ANF: any non-trivial operand is hoisted into a synthetic `let $elab_tN: <ty> = <expr>;` binding in the enclosing block, and the operator site references it by name. Synthetic names use `$` as a prefix — the surface lexer rejects `$` in identifiers, so no user-name collision is possible. Declared `TypeExpr` on each synthetic let is inferred from the operator tag directly (arith/Neg -> Int; comparison/logic/Not -> Bool), matching task 22's typing rules without re-running typecheck. Scope kept to task 23's one-line spec: match scrutinees, perform args, and call args are NOT flattened here. Future tasks can extend the recursion with `need_trivial=true` at those sites if codegen prefers a trivial-scrutinee invariant. `Expr::Block` is handled in typecheck by delegation to `check_block` for exhaustiveness even though the parser never produces it — defensive, since elaborate runs after typecheck in the Plan A2 pipeline. 9 new elaborate unit tests (97 compiler lib tests total) all pass locally. Pod-verify script green (fmt, check, per-crate clippy -D warnings, runtime lib tests, interior-pointer check, discipline greps). CI-authoritative per plan.
Owner
Author
Review verdict: merge-with-minor-fixesAll Stage 1.5 tasks (1.5.1–1.5.6) and Stage 2 tasks 20–23 pass verification. CI green on both hosts (Run 24843174375). 122 tests pass locally (23 runtime + 97 compiler lib + 2 e2e). No regressions from Plan A1 — Directives for the headless agent below. Items are grouped by when they need to land. Required before merge1. Update the PR body to disclose the full shipped scopeThe PR body currently lists Stage 1.5 tasks only, but the branch also ships Tasks 20–23 (lexer/parser/typechecker/elaborate for Stage 2). A reviewer reading only the body would miss half the diff. Required new sections in the body: Command: `gh pr edit 2 --body ""`. 2. Close the Byte-ordering QUESTIONS.md itemThe implementer flagged a plan discrepancy (normative typing rules say Reviewer decision: keep the strict form (a). Reasons:
Byte equality ( Action: update the Defer to the Task 26 PR (codegen + examples)
|
boldfield
added a commit
that referenced
this pull request
Apr 23, 2026
**Task 26** (scope revised post PR #3 review at designs commit `8e75c43` — factorial deferred to Stage 3's Task 33): - `examples/arith.sigil` — Stage-2 arithmetic showcase (`+`, `-`, `*`, `/`, `%`, comparison, unary negation, `if`/`else`). Invariant documented in the file header: exit code 26. - `examples/div_by_zero.sigil` — test-only program that triggers the runtime's division-by-zero trap. File header documents the expected stderr banner (`sigil: arithmetic error: division by zero`) and exit status (2). - Two new file-based e2e tests (`arith_example_exits_26`, `div_by_zero_example_traps`) in `compiler/tests/e2e.rs` replace the Task-24 inline `arith_integer_ops` / `div_by_zero_traps` coverage; the canonical Task-26 examples are now the source of truth. - **PR #2 deferrals picked up** per the Task 26 scope note: - `fn sigil_binary() -> PathBuf` wraps `env!("CARGO_BIN_EXE_sigil")` + `ensure_runtime_staticlib` behind `std::sync::Once`. Every e2e test (incl. pre-existing `hello` and `stackmap_section_parses_v0_ placeholder`) migrated to the helper. Serialises the nested cargo-build call and makes the staticlib-check syntactically unskippable. - `compile_file_and_run(&Path, &str)` shared implementation; `compile_and_run(&str, &str)` thin wrapper for inline-source tests. - QUESTIONS.md `[PLAN-A2] factorial-in-Stage-2` entry resolved to option (a) with full reviewer rationale (normative rules outrank descriptive paragraphs; Stage 3 is the right home for recursion; fibonacci is a better benchmark oracle anyway). **Task 28** (scope revised — P04 + P06 moved to Task 35): - `spec/validation-prompts.md` gains **P05** (parity check via `%` and `if`/`else`) and **P07** (safe divide with explicit divisor check; Unix exit-code truncation documented in the oracle). - P04 (sum-to-n via recursion) and P06 (multiplication table via nested recursion) require user function calls and will land in Task 35 alongside P08–P10. **`PLAN_A2_DEVIATIONS.md`** — two post-hoc deviations formalised per the PR #3 review: - **[Task 25]** `std::process::exit(2)` vs plan's `libc::exit(2)`. Equivalent on Unix (delegates to `exit(3)`; flushes stdio; runs atexit handlers — load-bearing for the counter-dump). Avoids adding `libc` to the runtime's dep allow-list. Swap trivially if Plan B adds `libc` for other reasons. - **[CHORE]** CI `rm -f target/{debug,release}/libsigil_runtime.a` before `cargo build -p sigil-runtime` in the `build + test` job. Workaround for cargo declaring a cached staticlib fresh when a prior pod-verify step had updated adjacent per-unit fingerprints. Surfaced concretely on PR #3 as `undefined reference to \`sigil_panic_arith_error\``. Proper fix (swap to `Swatinem/rust-cache@v2`) is deferred; the `rm -f` is load-bearing until then, and the workflow comment cross-references this entry. **Drive-by codegen cleanup** — the three identical `IntLit` / `BoolLit` / `CharLit` branches in `lower_match` collapsed through a `pattern_as_immediate(&Pattern) -> Option<i64>` helper. ~40 lines of duplication removed; Plan A3's constructor patterns will change the signature when they land (noted in the helper's doc). **Task 27 is closed out**: Stage 2 has no recursive program to benchmark (factorial moved to Stage 3), so the original `factorial(10) < 100ms` perf floor is dropped. Verification for Stage 2 shrinks to "`cargo test --workspace` completes within CI limits on both hosts" — satisfied by every CI run. Verification: `cargo fmt --check`, `cargo check --workspace`, per-crate `cargo clippy -D warnings`, `cargo test -p sigil-runtime --lib` (37 green), `scripts/check-no-interior-pointers.sh`, full `scripts/pod-verify.sh`. Local smoke: `arith.sigil` exits 26, `div_by_zero.sigil` prints the banner and exits 2. Full e2e test suite deferred to CI per plan discipline.
boldfield
added a commit
that referenced
this pull request
Apr 23, 2026
* [Task 26][Task 28] Examples + prompt bank + PR #2 deferrals + deviations **Task 26** (scope revised post PR #3 review at designs commit `8e75c43` — factorial deferred to Stage 3's Task 33): - `examples/arith.sigil` — Stage-2 arithmetic showcase (`+`, `-`, `*`, `/`, `%`, comparison, unary negation, `if`/`else`). Invariant documented in the file header: exit code 26. - `examples/div_by_zero.sigil` — test-only program that triggers the runtime's division-by-zero trap. File header documents the expected stderr banner (`sigil: arithmetic error: division by zero`) and exit status (2). - Two new file-based e2e tests (`arith_example_exits_26`, `div_by_zero_example_traps`) in `compiler/tests/e2e.rs` replace the Task-24 inline `arith_integer_ops` / `div_by_zero_traps` coverage; the canonical Task-26 examples are now the source of truth. - **PR #2 deferrals picked up** per the Task 26 scope note: - `fn sigil_binary() -> PathBuf` wraps `env!("CARGO_BIN_EXE_sigil")` + `ensure_runtime_staticlib` behind `std::sync::Once`. Every e2e test (incl. pre-existing `hello` and `stackmap_section_parses_v0_ placeholder`) migrated to the helper. Serialises the nested cargo-build call and makes the staticlib-check syntactically unskippable. - `compile_file_and_run(&Path, &str)` shared implementation; `compile_and_run(&str, &str)` thin wrapper for inline-source tests. - QUESTIONS.md `[PLAN-A2] factorial-in-Stage-2` entry resolved to option (a) with full reviewer rationale (normative rules outrank descriptive paragraphs; Stage 3 is the right home for recursion; fibonacci is a better benchmark oracle anyway). **Task 28** (scope revised — P04 + P06 moved to Task 35): - `spec/validation-prompts.md` gains **P05** (parity check via `%` and `if`/`else`) and **P07** (safe divide with explicit divisor check; Unix exit-code truncation documented in the oracle). - P04 (sum-to-n via recursion) and P06 (multiplication table via nested recursion) require user function calls and will land in Task 35 alongside P08–P10. **`PLAN_A2_DEVIATIONS.md`** — two post-hoc deviations formalised per the PR #3 review: - **[Task 25]** `std::process::exit(2)` vs plan's `libc::exit(2)`. Equivalent on Unix (delegates to `exit(3)`; flushes stdio; runs atexit handlers — load-bearing for the counter-dump). Avoids adding `libc` to the runtime's dep allow-list. Swap trivially if Plan B adds `libc` for other reasons. - **[CHORE]** CI `rm -f target/{debug,release}/libsigil_runtime.a` before `cargo build -p sigil-runtime` in the `build + test` job. Workaround for cargo declaring a cached staticlib fresh when a prior pod-verify step had updated adjacent per-unit fingerprints. Surfaced concretely on PR #3 as `undefined reference to \`sigil_panic_arith_error\``. Proper fix (swap to `Swatinem/rust-cache@v2`) is deferred; the `rm -f` is load-bearing until then, and the workflow comment cross-references this entry. **Drive-by codegen cleanup** — the three identical `IntLit` / `BoolLit` / `CharLit` branches in `lower_match` collapsed through a `pattern_as_immediate(&Pattern) -> Option<i64>` helper. ~40 lines of duplication removed; Plan A3's constructor patterns will change the signature when they land (noted in the helper's doc). **Task 27 is closed out**: Stage 2 has no recursive program to benchmark (factorial moved to Stage 3), so the original `factorial(10) < 100ms` perf floor is dropped. Verification for Stage 2 shrinks to "`cargo test --workspace` completes within CI limits on both hosts" — satisfied by every CI run. Verification: `cargo fmt --check`, `cargo check --workspace`, per-crate `cargo clippy -D warnings`, `cargo test -p sigil-runtime --lib` (37 green), `scripts/check-no-interior-pointers.sh`, full `scripts/pod-verify.sh`. Local smoke: `arith.sigil` exits 26, `div_by_zero.sigil` prints the banner and exits 2. Full e2e test suite deferred to CI per plan discipline. * [CHORE] Backfill Tasks 26, 28 commit hashes in PLAN_A2_PROGRESS * [CHORE] Tasks 26, 28 CI green both hosts; mark done
This was referenced Apr 24, 2026
5 tasks
boldfield
added a commit
that referenced
this pull request
Apr 25, 2026
…tests Addresses PR #16 review (boldfield): - Comment 4318163326 #1 (mangling format ambiguity, soundness hazard): switch separator from `_`/`__` to `$`/`$$`. `$` is lexer-rejected per Plan A2's `$lambda_N` precedent, so user identifiers cannot contain it. `type List_Option[A]` instantiated at `Int` no longer collides with `List[Option[Int]]` — the former mangles to `List_Option$$Int`, the latter to `List$$Option$Int`. Codegen's `mangle_user_fn` rewrites `$` to `__` for ELF/Mach-O compat; AST-level uniqueness is preserved. - Comment 4318163326 #2 (E0132 ambiguous polymorphism): new diagnostic in error catalog plus end-of-typecheck check. After resolving pending instantiations through `subst.apply_ty`, any type-arg that is still `Ty::Var(_)` AND not an outer-fn's generic-param id (i.e. not legitimately deferred to clone time) fires E0132 with the affected param name. Three tests pin the diagnostic: fires on unconstrained `nothing[A]()`, doesn't fire on constrained `id[A](42)`, doesn't fire on outer-fn-bound `use_id[B](y) { id(y) }`. - Comment 4318161359 #2 (canon_ty placeholder collision): the `Ty::Var(_)` and `Ty::Fn(_)` arms in `canon_ty` and `ty_to_type_expr` now `unreachable!` instead of emitting placeholder strings that two distinct vars would collide at. Reachable only when the upstream invariant breaks — E0132 catches ambiguous-poly cases at typecheck, and `Ty::Fn` requires a `TypeExpr::Fn` surface that v1 deliberately omits. - Comment 4318161359 #1 (call-site ctor lookup span fragility): the `Expr::Call` arm now explicitly checks `ctor_sites.get(span)` by the Call's span before falling through to the recursive callee rewrite. No longer relies on the parser's incidental `Call.span == callee_ident.span` for ctor calls. - Comment 4318161359 #3 (pattern sub-ctor unimplemented! guard): `rewrite_pattern`'s `Pattern::Ctor` arm now `unreachable!`s when the ctor's owning type is generic but doesn't match the scrutinee's User type. v1 surface can't construct this case; surfacing loudly closes a silent-miscompile path. - Comment 4318163326 #3 (test additions/strengthening): - `match_against_generic_scrutinee_rewrites_pattern_ctors`: strengthened to assert *both* `Nada$$Int` and `Just$$Int` appear, not just `>= 1`. - `recursive_generic_type_termination_one_clone_per_arg_tuple`: new test pinning that a recursive generic type (`type List[A] = | Nil | Cons(A, List[A])`) produces exactly one `List$$Int` clone given recursive use. - `self_recursive_generic_fn_terminates`: new test on a self-recursive generic fn. - `end_to_end_nested_generic_clone_name_matches_pinned_format`: new e2e variant of `mangle_fn_handles_nested_generics` that runs through the full pipeline. - `ctor_call_site_callee_ident_is_rewritten`: new test asserting the Call's callee Ident text is the mangled name post-mono (closes Comment 1 #1's gap). - `generic_fn_calling_generic_fn_resolves_var_chain`: new test exercising the `Ty::Var → Ty::Var → concrete` chain through `Substitution::by_var`. - `two_fn_clones_with_nested_generic_type_args`: new test confirming two clones for two distinct nested instantiations. - `canon_ty_disambiguates_underscore_named_user_types`: new unit test pinning the soundness fix. - Comment 4318163326 #4 (Deviation #2 reframe): effect-rows-not- monomorphized is reframed from open-ended ("v2 might revisit") to permanent v1 design choice; informational. Closure-path = none. - Comment 4318161359 #5 (apply_to_ty Ty::Fn comment): added inline comment pointing at Deviation #2 so the unchanged `effect_row_var` copy isn't mistaken for a substitution bug. - Pattern::Var nullary-ctor promotion in monomorph: typecheck's Plan A3 task 38.3 nullary-ctor promotion treats `Pattern::Var(name)` semantically as `Pattern::Ctor` when the name matches a Unit variant of the scrutinee's user type, but doesn't rewrite the AST. Monomorph now performs that rewrite explicitly (with mangled name `Nada$$Int` etc) so codegen's `ctor_index` lookup succeeds post-mono. This was the root cause of the strengthened match-pattern test failing initially. DEVIATIONS Deviation #1 updated to reflect the `$` separator and the hardening rationale. Deviation #2 reframed. Test deltas: 309 → 319 compiler lib tests (+10). - 28 → 35 monomorphize tests (+7 new). - 281 → 284 typecheck tests (+3 E0132 tests). Pod-verify green: cargo check workspace, cargo fmt --check, cargo clippy -p sigil-runtime -p sigil-compiler --all-targets -D warnings, cargo test -p sigil-runtime --lib, scripts/check-no-interior-pointers.sh.
This was referenced Apr 30, 2026
boldfield
added a commit
that referenced
this pull request
Apr 30, 2026
…tdlib deferral (#57) * [Plan D Stage 12 review checkpoint] Sign-off + closure-path edits + stdlib migration deferral Plan D Stage 12 ships four type-system surface lifts (Tasks 113 tuples, 114 type-parameterized effect rows, 115 per-op generic params, 116 row-polymorphic Fn parameters). Per the plan body, the Stage 12 review checkpoint requires human review of: 1. AST shape consistency 2. Diagnostic quality 3. Closure-path edits to [DEVIATION Task 71/72/73] 4. Stdlib updates to use the now-expressible generic shapes This commit records sign-off across all four items. **Item 1 (AST)**: ✅ - EffectRef/EffectInst split mirrors Tuple's TypeExpr/Ty pattern - EffectOp.generic_params parallels FnDecl's existing field - FnTypeExpr.effect_row_var binding wired through current_row_var_subst **Item 2 (diagnostics)**: ✅ - E0117 (tuple-pattern arity) — span at pattern - E0143 (row-arg arity, renamed from E0140 due to dup-arm collision) - E0144 (per-op generic shadow) — span at per-op param decl - E0137 (narrowed: only fires on unbound row vars) - All carry consistent source spans **Item 3 (closure-path edits)**: - PLAN_C_DEVIATIONS [DEVIATION Task 71] constraints #1/#2/#3 → Closed by Plan D Tasks 114/115/116 - [DEVIATION Task 72] constraints #1/#2/#4/#5 → Closed; #3 (wrapper-fn-frame) stays Deferred per Task 112's deferral - [DEVIATION Task 73] constraints #1/#5/#6 → Closed; #2/#3/#4 (multi-shot codegen) stay open for Tasks 117/118 - New stdlib-migration deferral notes added to all three **Item 4 (stdlib migration deferred to Plan C completion)**: Migration of std/raise.sigil to `effect Raise[E] { fail[A]: (E) -> A }` + row-poly catch was attempted in this session and surfaced an additional handler-discharge type-arg propagation gap not flagged by the Tasks 114-116 typecheck-level test corpus: - `discharged: Vec<String>` in check_handle pushes bare-name EffectInst entries into body_row (no args). When body() is called inside a handle, body's row `![Raise[E] | e]` doesn't subsume body_row's `![Raise + e_caller]` because the structural EffectInst diff (Task 114) sees `Raise[E]` ≠ `Raise` (no args). - Closure path: typecheck refinement to walk handle body's expected type for discharged effect's instantiation and use those args. Estimated 1-2 typecheck.rs sites + regression test corpus. Per the plan-body's Plan-D-vs-Plan-C-completion separation, stdlib migrations belong to Plan C completion. The handler-discharge gap is a fresh finding that wasn't on Plan D's task list; closing it requires its own design + implementation cycle. Plan C completion's stdlib migration PR will batch (a) handler-discharge fix, (b) std/raise migration, (c) std/state migration to (A, S) tuple-return + generic E + row-poly run_state, (d) std/result verification. **Stage 12 sign-off**: ✅ Tasks 113/114/115/116 + closure-path edits land; stdlib migration deferred per the plan-overview separation. Stage 12 complete from Plan D's perspective. pod-verify clean (doc-only changes). * [Plan D Stage 12] std/raise.sigil migration to generic Raise[E] / fail[A]: (E) -> A / row-poly catch User pushback on prior commit's stdlib deferral. Re-attempted the migration after fixing two underlying typecheck gaps that surfaced during the first attempt: **Gap 1 — `subsume_row` / `unify_row` did structural-equality diff on EffectInst, not name-match-with-arg-unify.** When a row carries `Raise[Var(N)]` (Ty::Var) and another carries `Raise[String]` (concrete), the structural diff sees them as distinct effects and fires E0042 / E0128. The fix: both fns now match by name; if names collide they unify args pairwise via `unify_ty` (E0044 fires on arg-type mismatch). This is the correct semantics — two rows containing `Raise[?N]` and `Raise[Concrete]` should unify ?N with Concrete, not error out. **Gap 2 — `Tc::rename_ty`'s Ty::Fn arm cloned `effects` without renaming Ty::Var ids.** At scheme instantiation, `raise[A, E](e: E) -> A ![Raise[E]]`'s row's `Var(E_decl_id)` wasn't renamed to `Var(E_fresh)`. So calling `raise(42)` would bind E_fresh := Int via arg unification, but the row's E_decl_id stayed unbound, and subsume_row's name-match unify_ty(Var(E_decl_id), String) bound E_decl_id := String without conflict. The fix: rename_ty now walks each EffectInst's args with the same ty_map. **Stdlib migration**: - `std/raise.sigil`: `effect Raise[E] { fail[A]: (E) -> A }`, `raise[A, E](e: E) -> A ![Raise[E]]`, `catch[A, E](body: () -> A ![Raise[E] | e]) -> Result[A, E] ![| e]`. - 5 e2e tests + 3 typecheck tests updated from `![Raise]` to `![Raise[String]]`. - `raise_int_return_in_string_returning_fn_fires_e0044_v1_gap_pin` inverted to `raise_in_string_returning_fn_typechecks_post_task_115` — the v1 gap is closed by per-op generics. - `raise_with_int_arg_fires_e0044` updated: now exercises Int arg vs row-instantiated String E (E0044 from arg-unification). - `cross_fn_row_with_distinct_type_args_fires_e0042` → `cross_fn_row_with_distinct_type_args_fires_e0044` — the semantics fix changed which code surfaces; arg-unification diagnostic is more precise than "row mismatch". **std/state.sigil + std/result.sigil migration** continues to defer to Plan C completion's stdlib batch — both involve their own runtime-correctness considerations (state's discharge-with-lambda pattern under generic E + tuple return; result's Apply→Generic verification). pod-verify clean. typecheck unit tests: 635 pass. * [Plan D Stage 12 fix] Migrate examples/catch.sigil + examples/interpreter.sigil to ![Raise[String]] * [Stage 12 R3] Discharge-site args + apply_ty/apply_row args + return-bool threading + docs reconcile R3 reviewer surfaced four real items + several smaller follow-ups. All addressed. **Item 1 — docs out of sync with diff**: PR description and deviation entries said "stdlib migration deferred to Plan C completion", but std/raise.sigil + 2 examples + 8 tests had already migrated in commit 5880e97 / fc36cb3. Updated: - PLAN_D_DEVIATIONS.md `[DEVIATION Stage 12 review]` Item 4 fully rewritten — std/raise shipped, std/state + std/result still deferred, three architectural gaps documented as fixed. - PLAN_D_PROGRESS.md Stage 12 sign-off summary updated. **Item 2 — handler-discharge gap fix at the documented closure path** (was: papered over via silent-skip in unify_row). `Tc::check_handle` now pushes EffectInst-with-args at the discharge site by looking up the effect-decl's generic_params and reading the active handler subst (`effect_substs[name]`). The handler arm's existing op-typing already allocates these substs; reusing them ensures body row's `Raise[E_var]` matches body's expected `Raise[E_body_var]` via subsume_row's arg unification. Falls back to bare-name (no args) for non-generic effects (`IO`, `Mem`) — preserves pre-Stage-12 behavior. **Item 3 — apply_ty/apply_row didn't walk EffectInst args** (was: parallel to `rename_ty` Gap 2 but unfixed). `apply_ty_inner`'s Ty::Fn arm and `apply_row_inner` now walk each EffectInst's args via `apply_ty_inner`. Without this, a row carrying `Raise[Var(N)]` survived subst application even after Var(N) was bound elsewhere — bug just moved to a different surface. Symmetric with rename_ty's Stage 12 fix. **Item 4 — silent-skip on arg-arity mismatch was a soundness hole**. unify_row / subsume_row's name-match-with-arg-unify path silently skipped when arg-counts differed — accepted invalid shapes without binding anything. Now both fire diagnostics: - unify_row: E0128 with explicit "consistent arity" message. - subsume_row: E0042 with explicit "same arity" message. This is graceful-recovery only; the discharge-site fix (Item 2) ensures well-formed rows always have the right arity, so the diagnostic path is unreachable from valid programs. **unify_ty bool return now threaded through**: `args_ok` flag ANDed into unify_row / subsume_row's overall return. A false from arg-unification (E0044) now fails the row check, not just pushes the error and returns true. Caller bool-branches no longer cascade under E0044. **Regression test**: `subsume_row_arg_arity_mismatch_fires_e0042` pins the (now-error) arg-arity-mismatch case so a future contributor doesn't accidentally restore the silent skip. **Stale code refs in PLAN_C_DEVIATIONS Task 73**: refreshed line numbers (3665 → 3744, 1505-1518 → 1569, 1591-1603 → 1652) since this PR explicitly stamps Closed/Open status on those constraints. pod-verify clean. typecheck unit tests: 636 pass.
boldfield
added a commit
that referenced
this pull request
May 1, 2026
…e Ty::Fn Same 10 e2e tests still failed on 3ae703f, this time with a different panic (codegen-walker reject): has arm `Choose.flip` body that references continuation `k` as a value (not as the callee of a call) — Sigil v1 supports invoking `k` as a callee but not passing it as a first-class value; first-class continuations are deferred to v2 Root cause: closure_convert::rewrite_expr's Lambda arm at the ArmKPairCapture detection site (closure_convert.rs:628 prior) matched on `matches!(cty, Ty::Fn(_))`. With k now bound as Ty::Continuation, the match misses, k stays in regular `caps`, the lambda's ClosureRecord lifts k as a normal Expr::Ident(k) capture in env_exprs, and the codegen-entry walker (which checks every Expr::Ident(k_name) inside the arm body, including ClosureRecord env_exprs) fires the "k as a value" reject. Surgical fix: widen the match to `Ty::Fn(_) | Ty::Continuation(_)` and refactor the post-match unpack into a single `k_pair_tys` extraction step that pulls `(op_ret_ty, handler_overall_ty)` from either variant. ArmKPairCapture machinery is otherwise untouched — the synth-fn-record layout, the trailing-pair convention, and codegen's lower_k_pair_call dispatch all remain. This is fix #2 of the iteration budget. Same affected test set as the prior fix (slot_kind_for_ty), expected back to green on the same discharge-with-lambda + run_state suites. Pod-verify clean.
boldfield
added a commit
that referenced
this pull request
May 1, 2026
…d::Var asserts + RELINK_STACK debug_assert PR #60 review (boldfield, 06:04 UTC) flagged 3 quality/consistency issues + 4 smaller notes to fold into in-flight work. This addresses issues #1, #2, #3 + the RELINK_STACK note. **Issue #1 — E0145 coverage gap.** The Continuation-vs-Continuation arm in `unify_ty` already fires E0145 (scope_id mismatch); but Continuation-vs-non-Continuation (record field whose declared type is Fn / fn-arg whose param is Fn / fn-return whose return type is Fn) fell through to the generic E0044 catchall, surfacing as "type mismatch" with no escape-barrier framing. Add a broad arm `(Ty::Continuation(_), _) | (_, Ty::Continuation(_))` between the Continuation-vs-Continuation arm and the catchall, firing E0145 with a uniform fix message ("keep `k` inside the handle's arm body — do not store it in a record/ctor field, pass it to a function expecting a non-continuation parameter, or return it from a function whose declared return type is not a continuation"). Var-vs-Continuation is intentionally left to the (Var, other) bind_ty_var arm earlier in the match — `let f = k` legitimately binds f's var to Continuation; the escape only manifests when that bound var later unifies against a non-Continuation target, which re-enters the new arm with both sides resolved. **Issue #2 — `unreachable!()` messages.** Folded into issue #1: now that E0145 actually fires uniformly in `unify_ty`, the existing "E0145 should have rejected" messages in `monomorphize::canon_ty` / `Substitution::apply_to_ty` / `ty_to_type_expr` and `codegen::cranelift_ty_of_ty` (tuple element / user-type field) are accurate as written. No edits needed at those sites. **Issue #3 — ScopeId::Var unreachability assertion.** Replaced the `_ => true` silent-pass-through in three walkers (`unify_ty`, `rename_ty`, `apply_ty_inner`) with explicit `(ScopeId::Var(_), _) | (_, ScopeId::Var(_)) => unreachable!(...)`. Today only `check_handle` produces ScopeIds and always allocates `Concrete(N)` via `fresh_scope_id()`; no scheme stores Var. The unreachable!() surfaces the integration point loudly when region- polymorphism work (Task 117 follow-up) introduces Var producers, preventing a silent miscompile. **RELINK_STACK debug_assert.** `sigil_handle_pop`'s `pop().unwrap_or(true)` soft fallback masks integration bugs where a codegen path emits a pop without a matching push. Added `debug_assert!(!stack.is_empty(), "...")` before the pop so debug builds (cargo test) trip loudly on missing push; release builds keep the soft fallback unchanged so any legitimate caller that bypasses RELINK_STACK still works. Pod-verify clean. Handle (45) / linearity (15) / runtime lib (149) tests all green. Smaller notes deferred (PR description checklist refresh — separate; row-var subsumption regression test + tests for positive `let f = k; f(arg)` paths and negative E0145 escape shapes land alongside the codegen-walker delta + closure_convert let-bound k work, where the surface admits exercising them).
boldfield
added a commit
that referenced
this pull request
May 1, 2026
…ELINK_STACK frame-keyed + mono defensive forwarding + DEVIATIONS clarification Review 2 (boldfield, 06:11 UTC) flagged 5 quality/architectural issues beyond review 1. This addresses 4 of them directly + surfaces the 5th (bind_ty_var bypass) for Brian's design call in the PR thread. **Issue #5 — `ty_display` leaks `<scope=N>` to user.** Dropped scope_id from `ty_display`'s Continuation rendering — users have no mental model for "scope N" and can't remediate. Continuations now surface as `Continuation(op_ret) -> ret`. Scope numbers stay in the E0145 cross-handle-mismatch message where they explain the violation ("continuation from handle scope 3 cannot unify with continuation from handle scope 4 — `k` cannot escape its originating handle's arm body"); the broad-arm E0145 message also stays scope-free since the violation is escape-shape, not cross-handle-confusion. **Issue #3 — RELINK_STACK pop on empty.** 503308d used `debug_assert!`; reviewer wanted release-build catch too. Replaced with `match ... { None => { eprintln!(...); std::process::abort(); } }` matching the codebase's existing underflow-handling idiom (see adjacent `if head.is_null() { eprintln!; abort(); }`). `panic!()` is disallowed in the runtime crate per clippy::disallowed-macros. Both debug and release now hard-abort on desync. **Issue #1 — DEVIATIONS.md drift.** PLAN_D_DEVIATIONS.md scope #2 claimed `Scheme.scope_vars` / `Tc.current_scope_subst` / `apply_scope_id` parallel infrastructure exists; grep confirms none of these names appear in code. Updated the entry to make the deferral explicit: "Task 117 (a) is Concrete-only; the Var(u32) variant exists in the enum but is structurally dead — no Scheme.scope_vars / Tc.current_scope_subst / apply_scope_id; walkers unreachable!() on Var so a stray Var leaks loudly when region- polymorphism work begins. The Plan B Stage 5 row-var-infrastructure parallel is deferred to Task 117 (b)." **Issue #4 — RELINK_STACK frame-keyed.** Upgraded `RefCell<Vec<bool>>` to `RefCell<Vec<(*mut HandlerFrame, bool)>>`. At pop time, `debug_assert_eq!(recorded_frame, head)` catches COUNT-balanced unbalanced pairs (push X 2× / pop X 2× across different frames) at the actual desync site rather than three handler levels later. GC reasoning documented at the static declaration: Vec buffer is on Rust's allocator (not Boehm's heap), so frame pointers in it are NOT conservatively scanned. Sound by invariant — every frame in RELINK_STACK is also reachable from HANDLER_STACK because push/pop pairs balance in lockstep across both stacks; a violation trips the abort or debug_assert before GC could observe the desync. **Issue #2 — mono unreachable!()s.** Reviewer's concern: until E0145 covers all paths, `unreachable!()` on user code is uncatchable. Defensive-forwarded the two clean cases: - `Substitution::apply_to_ty(Ty::Continuation)` recurses into op_ret/ret + rebuilds (mirrors the Ty::Fn arm). - `canon_ty(Ty::Continuation)` mangles to a stable `Cont${scope}${op_ret}$Ret${ret}` string (mirrors Ty::Fn). `ty_to_type_expr` is the one site where defensive forwarding has no clean target — Continuation has no syntactic representation, so a synthetic TypeExpr would just shift the panic to codegen-side "unknown symbol." Pinned `unreachable!()` here with explicit acknowledgment in the comment that the bind_ty_var-via-generic- instantiation path (`id(k)` for generic `id[A](x: A) -> A`) reaches mono today; canon_ty + apply_to_ty defensive-forward, but ty_to_type_expr cannot. Closing requires either E0145-on-bind for generic instantiation (restrictive — `let f = k` uses the same bind_ty_var arm and is the intended capability) OR threading Continuation through TypeExpr via a synthetic named form (codegen needs to learn to resolve it). Both are PR (b) territory; surfaced to Brian via the PR thread for direction. Pod-verify clean. Handle (45) / runtime lib (149) tests green. Iteration budget: this is 0 fixes against the in-flight landing budget — review responses count separately per Brian's framing ("quality / consistency issues to fold into the work in progress").
This was referenced May 1, 2026
Merged
This was referenced May 9, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Plan A2's Stage 1.5 (scaffolding) and Stage 2 Tasks 20–23 per
boldfield/designs:in-progress/2026-04-21-sigil-core-a2.md.Stage 1.5 tasks
a18876e) — CreatePLAN_A2_PROGRESS.md,empty
PLAN_A2_DEVIATIONS.md, append[PLAN-A2]prefix conventionto
QUESTIONS.md.215ef8a) —scripts/pod-verify.shmemory-safe localverification wrapper;
README.md"Local verification onmemory-constrained hosts" section; CI invokes the script as a new
step before the full matrix.
f0a6212, DEVIATION; revised indb3ae5e) —Cold-checkout staticlib ordering fix. Original approach:
compiler/build.rsthat invokescargo build -p sigil-runtimewhentarget/<profile>/libsigil_runtime.ais missing. Revised after thebuild.rs approach deadlocked under
cargo test --workspaceon coldtargets (47+ min hang before CI cancelled both hosts): the
materialisation now lives in
compiler/tests/e2e.rs::ensure_runtime_staticlib, called at testrun-time after cargo releases its build-unit locks. The
SIGIL_SKIP_RUNTIME_STATICLIB_BUILDenv var is gone — not neededonce the rebuild is at test-run time.
cold-checkout-testCI jobvalidates the fix with two successive cold builds on both hosts.
See
PLAN_A2_DEVIATIONS.mdfor full rationale.00739d3) —Tc::env_insert(name, ty)helper withdebug_assert!(prev.is_none(), ...)replaces bareenv.insertatboth param and let-binding sites in
compiler/src/typecheck.rs. Norelease-build behaviour change.
75733ba,7d05183) — Backfill commit hashes in progress /deviations files.
Stage 2 tasks also shipped
b838a9c) — lexer extended for Stage 2:true/false/if/else/matchkeywords, binary/unary operators, char literals.964a83c) — parser extended for Stage 2: Pratt-style precedence,if/else,match, unary-/!, constant-folded negative literals.1de46b4) — typechecker extended for Stage 2:Bool/Char/Bytetypes, binop/unary/if/match typing rules, exhaustiveness check, new catalog codes E0060–E0066.0714454) — elaboration extended:if/else→match-on-Bool; arithmetic operands flattened into ANF with$-prefixed synthetics.Tasks 24–28 (Cranelift codegen, runtime primitives, example programs, performance floor, prompt bank) are not in this PR; they land in subsequent PRs once Stage-1.5 + 20–23 merge.
Pod-safe verification (already green locally)
cargo fmt --all --check✓cargo check --workspace✓cargo clippy -p sigil-runtime --all-targets -- -D warnings✓cargo clippy -p sigil-compiler --all-targets -- -D warnings✓cargo test -p sigil-runtime --lib— 23 passedcargo test -p sigil-compiler --lib -- --test-threads=1— 97 passedscripts/check-no-interior-pointers.sh✓CI-authoritative acceptance (what this PR gates on)
build-testmatrix green on both hosts (ubuntu-24.04,macos-14).cold-checkout-testmatrix green on both hosts — proves the task 1.5.5fix: two successive
rm -rf target && cargo test --workspaceruns pass.not local-green.
Scope guardrails
Tasks 24–28 are out of scope for this PR — codegen, runtime primitives,
examples, performance floor, and prompt-bank expansion land in follow-up
PRs. Plan A3 does not start until all of Plan A2 is merged and reviewed.
Draft because this PR bundles Stage 1.5 plus four Stage-2 tasks whose
acceptance still depends on CI; Plan A2 completion (Tasks 24–28)
continues after this branch merges.