⏺ 407 pass, clippy clean. Stage 3 complete. #18

Merged

navicore merged 2 commits from ff-stage-3-span into main 2026-06-18 04:57:15 +00:00

Conversation 1 Commits 2 Files changed 11 +331 -119

navicore commented 2026-06-18 04:10:59 +00:00

Owner

Copy link

Stage 3: provenance for the type-checking det builtins

The guard from Stage 2 paid off — each of the 12 raising builtins was a
mechanical two-line change.

• DET_BUILTINS gains a raises flag ((name, arity, sym, raises)). It drives
  both the decl-gen (declare … , i32 for raising ones) and the codegen RtDet
  arm (lower::det_builtin_raises(sym) → append g.span's site_id). Non-raising
  type checks (var/1, I/O, copy_term/2, …) carry no site_id — provenance
  only where raises happen.
• 12 runtime fns (functor/3, arg/3, =../2, atom_length/2, atom_concat/3,
  atom_chars/2, number_chars/2, number_codes/2, msort/2, sort/2, succ/2,
  plus/3) gained site_id: u32 + let _site = ErrorSiteGuard::enter(m,
  site_id);. control.rs's query-side dispatch passes NO_SITE; the unit tests
  use thin per-module wrappers so they stay site-free.

Verified end-to-end: atom_length(123,) → type_error(atom, 123) … at
prog.pl:2:5; sort(foo,) → type_error(list, foo) … :2:5; succ(-1,_) →
domain_error(not_less_than_zero, -1) … :4:5; query-side → no suffix
(byte-identical v1). Three new integration tests pin these; 407 pass,
clippy clean, golden IR + byte-exact v1 messages preserved.

SPANS.md updated: Layer 3 provenance is now complete for all raising
builtins. The only remaining item is the (non-CI) diff-test
suffix-stripping for checkpoint 5; throw/1 stays excluded. Ready for you to
branch/PR — once the diff-test helper lands, SPANS.md can retire to done/.

Stage 3: provenance for the type-checking det builtins The guard from Stage 2 paid off — each of the 12 raising builtins was a mechanical two-line change. - DET_BUILTINS gains a raises flag ((name, arity, sym, raises)). It drives both the decl-gen (declare … , i32 for raising ones) and the codegen RtDet arm (lower::det_builtin_raises(sym) → append g.span's site_id). Non-raising type checks (var/1, I/O, copy_term/2, …) carry no site_id — provenance only where raises happen. - 12 runtime fns (functor/3, arg/3, =../2, atom_length/2, atom_concat/3, atom_chars/2, number_chars/2, number_codes/2, msort/2, sort/2, succ/2, plus/3) gained site_id: u32 + let _site = ErrorSiteGuard::enter(m, site_id);. control.rs's query-side dispatch passes NO_SITE; the unit tests use thin per-module wrappers so they stay site-free. Verified end-to-end: atom_length(123,_) → type_error(atom, 123) … at prog.pl:2:5; sort(foo,_) → type_error(list, foo) … :2:5; succ(-1,_) → domain_error(not_less_than_zero, -1) … :4:5; query-side → no suffix (byte-identical v1). Three new integration tests pin these; 407 pass, clippy clean, golden IR + byte-exact v1 messages preserved. SPANS.md updated: Layer 3 provenance is now complete for all raising builtins. The only remaining item is the (non-CI) diff-test suffix-stripping for checkpoint 5; throw/1 stays excluded. Ready for you to branch/PR — once the diff-test helper lands, SPANS.md can retire to done/.

navicore added 1 commit 2026-06-18 04:10:59 +00:00

⏺ 407 pass, clippy clean. Stage 3 complete. ... e27a3873e3

Stage 3: provenance for the type-checking det builtins

  The guard from Stage 2 paid off — each of the 12 raising builtins was a
  mechanical two-line change.

  - DET_BUILTINS gains a raises flag ((name, arity, sym, raises)). It drives
  both the decl-gen (declare … , i32 for raising ones) and the codegen RtDet
  arm (lower::det_builtin_raises(sym) → append g.span's site_id). Non-raising
  type checks (var/1, I/O, copy_term/2, …) carry no site_id — provenance
  only where raises happen.
  - 12 runtime fns (functor/3, arg/3, =../2, atom_length/2, atom_concat/3,
  atom_chars/2, number_chars/2, number_codes/2, msort/2, sort/2, succ/2,
  plus/3) gained site_id: u32 + let _site = ErrorSiteGuard::enter(m,
  site_id);. control.rs's query-side dispatch passes NO_SITE; the unit tests
  use thin per-module wrappers so they stay site-free.

  Verified end-to-end: atom_length(123,_) → type_error(atom, 123) … at
  prog.pl:2:5; sort(foo,_) → type_error(list, foo) … :2:5; succ(-1,_) →
  domain_error(not_less_than_zero, -1) … :4:5; query-side → no suffix
  (byte-identical v1). Three new integration tests pin these; 407 pass,
  clippy clean, golden IR + byte-exact v1 messages preserved.

  SPANS.md updated: Layer 3 provenance is now complete for all raising
  builtins. The only remaining item is the (non-CI) diff-test
  suffix-stripping for checkpoint 5; throw/1 stays excluded. Ready for you to
  branch/PR — once the diff-test helper lands, SPANS.md can retire to done/.

navicore commented 2026-06-18 04:39:48 +00:00

Author

Owner

Copy link

Review — Stage 3: provenance for the type-checking det builtins

Mechanical, focused, and the payoff from the prior stages is now visible: each of the 12 raising builtins really is a two-line change (a trailing site_id: u32 and let _site = ErrorSiteGuard::enter(m, site_id);). The raises flag in DET_BUILTINS is the right place to put the metadata — it drives both the IR decl-gen (crates/compiler/src/codegen/program.rs) and the RtDet emit (crates/compiler/src/codegen/clause.rs) from a single bit, so they can't drift against each other by construction.

Issues below, ordered by impact.

Real concerns

1. End-to-end provenance is tested for 2 of 12 newly raising builtins. The new integration tests cover atom_length (type_check_builtin_error_carries_source_location) and sort (sort_type_error_carries_source_location). Wired but never verified end-to-end with a suffix assertion:

• functor/3, arg/3, =../2 (termops)
• atom_concat/3, atom_chars/2, number_chars/2, number_codes/2 (atomops)
• msort/2 (sortops)
• succ/2, plus/3 (miscops)

The PR description claims "the golden-IR / integration tests catch a mismatch" — true for the IR shape (the decl-gen / RtDet wire-up is generated from the same raises flag, so they agree by construction), but the IR doesn't catch a runtime ABI mismatch. If a future refactor flips ("functor", 3, ..., true) to false while leaving plg_rt_b_functor_3's signature carrying site_id, you get one i32 of garbage on the stack/register read as site_id with no test firing. The 10 missing tests are each one line of "compile a body that hits the builtin with bad input, assert out.contains(" at prog.pl:")." A table-driven test that walks every raises: true row and pins a known-bad invocation would close this — and would automatically extend to the next stage of raising builtins (if any).

2. det_builtin_raises(sym) is O(N) per emit site. crates/compiler/src/codegen/lower.rs:

pub fn det_builtin_raises(sym: &str) -> bool {
    DET_BUILTINS.iter().any(|&(_, _, s, raises)| s == sym && raises)
}

Called once per RtDet goal in the program from emit_inline_builtin. The (name, arity) lookup already happened in lower_goal when populating LGoalKind::RtDet { sym, args } — the natural place to also store raises is right there, alongside sym:

LGoalKind::RtDet {
    sym: &'static str,
    args: Vec<Term>,
    raises: bool,
}

Populated once at lower time, read O(1) at emit time, and the fact "does this raise?" is co-located with the symbol it applies to instead of looked up by string at the emit site. Small refactor; meaningful in a 10k-RtDet program (27 × 10k string compares per build).

Small observations

3. let _site = ErrorSiteGuard::enter(m, site_id); has a silent footgun. If someone writes let _ = ErrorSiteGuard::enter(m, site_id); instead (easy mistake — let _ looks idiomatic), the guard drops immediately and the function body runs with m.error_site restored to whatever it was before. No raise → no observable bug; raise → wrong site (or none). #[must_use] doesn't catch let _ binding. Two cheap defenses:

• Doc this on ErrorSiteGuard: "Bind to a named variable (e.g. let _site); let _ drops the guard immediately and the function runs with the wrong site."
• Macro-ize: entered!(m, site_id) expanding to let _site = ErrorSiteGuard::enter(m, site_id); makes the pattern unambiguous and self-documenting. With 12 call sites — and more if the fast-follow stretches further — the macro pays for itself.

4. The raises flag and the runtime fn signature have no compile-time agreement check. Same root cause as #1: the table's metadata claims the wire shape, and the runtime fn signature is the wire shape, but nothing forces them to agree at build time. A static_assertions::const_assert! on the count of raises: true rows is hard without a procedural macro. A pragmatic alternative is a unit test that, at runtime, scans the table and asserts each raising builtin's first call site receives the i32 — but that's what #1's end-to-end coverage already proves indirectly, so closing #1 closes most of this. Tracking only.

5. The DET_BUILTINS 3-tuple → 4-tuple migration leaks into the vocab_invariant tests. Both det_builtins_are_det_rows_in_shared and recognized_names_equal_shared_vocabulary already updated to the new shape. Fine — the cost is two destructures — but if a future addition (say, an arity_min/arity_max flag for variadic builtins) adds another field, every consumer updates. A named struct DetBuiltin { name, arity, sym, raises } would future-proof this and double as documentation at the call site. Stylistic.

What's good

• ErrorSiteGuard is used uniformly across all 12 sites. No hand-rolled set/restore lurking anywhere; the PR 17 disposition got the foundation right and Stage 3 leans on it instead of inventing variants. The pattern is self-enforcing exactly where the surface grew most.
• Save/restore semantics (Drop restores the previous site, not NO_SITE) quietly solve the nesting concern from PR #17 review #3. If plg_rt_b_sort_2 ever calls back into another raising builtin (today: leaf, but the architecture survives the change), the outer raise sees its own site again on inner-guard-Drop. Right-shaped now, right-shaped later.
• The INVARIANT: comment on Machine::error_site addresses PR 17 review #2 directly. Future contributors who reach for m.error_site = ... see the contract.
• NO_SITE's ABI contract note ("MUST equal plg_compiler::codegen::NO_SITE") addresses PR 15 review #7. Independent consts, same value, documented as a contract — won't drift silently.
• Per-test wrappers (fn alen, fn acat, …) keep existing behavior tests site-free. The intent is obvious to a reader: "this test exercises behavior, not provenance; site is elided." A single-line wrapper instead of inline NO_SITE at every call site reads cleaner.
• control.rs's query-side dispatch passes NO_SITE explicitly. Documented at the comment, mechanical at each call. Query-side raises stay byte-identical to v1; query_side_type_check_error_has_no_location_suffix pins it.
• throw/1 explicitly excluded with rationale (user-thrown ball isn't a system error). Right call — appending source location to a user's deliberate throw would be wrong.
• SPANS.md "Adding a new raising builtin is now two lines" documents the recipe. Future contributors get a one-paragraph onboarding instead of having to read the four PRs that built up to it.
• The raises boolean drives decl-gen and emit from one bit. Drift between IR shape and codegen lookup is impossible by construction. The single source of truth for "this builtin has provenance" is exactly the row that names the builtin.

Suggested order

#1 (the 10 missing end-to-end pins) is worth doing before merge — it's the only thing standing between "wired" and "verified," and it's a single table-driven test. #2 (carry raises in RtDet) is small, mechanical, and avoids fan-out lookups. #3+ are observations.

## Review — Stage 3: provenance for the type-checking det builtins Mechanical, focused, and the payoff from the prior stages is now visible: each of the 12 raising builtins really is a two-line change (a trailing `site_id: u32` and `let _site = ErrorSiteGuard::enter(m, site_id);`). The `raises` flag in `DET_BUILTINS` is the right place to put the metadata — it drives both the IR decl-gen (`crates/compiler/src/codegen/program.rs`) and the RtDet emit (`crates/compiler/src/codegen/clause.rs`) from a single bit, so they can't drift against each other by construction. Issues below, ordered by impact. ### Real concerns **1. End-to-end provenance is tested for 2 of 12 newly raising builtins.** The new integration tests cover `atom_length` (`type_check_builtin_error_carries_source_location`) and `sort` (`sort_type_error_carries_source_location`). Wired but **never verified end-to-end with a suffix assertion**: - `functor/3`, `arg/3`, `=../2` (termops) - `atom_concat/3`, `atom_chars/2`, `number_chars/2`, `number_codes/2` (atomops) - `msort/2` (sortops) - `succ/2`, `plus/3` (miscops) The PR description claims "the golden-IR / integration tests catch a mismatch" — true for the IR shape (the decl-gen / RtDet wire-up is generated from the same `raises` flag, so they agree by construction), but **the IR doesn't catch a runtime ABI mismatch**. If a future refactor flips `("functor", 3, ..., true)` to `false` while leaving `plg_rt_b_functor_3`'s signature carrying `site_id`, you get one i32 of garbage on the stack/register read as site_id with no test firing. The 10 missing tests are each one line of "compile a body that hits the builtin with bad input, assert `out.contains(" at prog.pl:")`." A table-driven test that walks every `raises: true` row and pins a known-bad invocation would close this — and would automatically extend to the next stage of raising builtins (if any). **2. `det_builtin_raises(sym)` is O(N) per emit site.** `crates/compiler/src/codegen/lower.rs`: ```rust pub fn det_builtin_raises(sym: &str) -> bool { DET_BUILTINS.iter().any(|&(_, _, s, raises)| s == sym && raises) } ``` Called once per RtDet goal in the program from `emit_inline_builtin`. The `(name, arity)` lookup already happened in `lower_goal` when populating `LGoalKind::RtDet { sym, args }` — the natural place to also store `raises` is right there, alongside `sym`: ```rust LGoalKind::RtDet { sym: &'static str, args: Vec<Term>, raises: bool, } ``` Populated once at lower time, read O(1) at emit time, and the fact "does this raise?" is co-located with the symbol it applies to instead of looked up by string at the emit site. Small refactor; meaningful in a 10k-RtDet program (27 × 10k string compares per build). ### Small observations **3. `let _site = ErrorSiteGuard::enter(m, site_id);` has a silent footgun.** If someone writes `let _ = ErrorSiteGuard::enter(m, site_id);` instead (easy mistake — `let _` looks idiomatic), the guard drops immediately and the function body runs with `m.error_site` restored to whatever it was before. No raise → no observable bug; raise → wrong site (or none). `#[must_use]` doesn't catch `let _` binding. Two cheap defenses: - **Doc this on `ErrorSiteGuard`**: "Bind to a named variable (e.g. `let _site`); `let _` drops the guard immediately and the function runs with the wrong site." - **Macro-ize**: `entered!(m, site_id)` expanding to `let _site = ErrorSiteGuard::enter(m, site_id);` makes the pattern unambiguous and self-documenting. With 12 call sites — and more if the fast-follow stretches further — the macro pays for itself. **4. The `raises` flag and the runtime fn signature have no compile-time agreement check.** Same root cause as #1: the table's metadata claims the wire shape, and the runtime fn signature is the wire shape, but nothing forces them to agree at build time. A `static_assertions::const_assert!` on the count of `raises: true` rows is hard without a procedural macro. A pragmatic alternative is a unit test that, at runtime, scans the table and asserts each raising builtin's first call site receives the i32 — but that's what #1's end-to-end coverage already proves indirectly, so closing #1 closes most of this. Tracking only. **5. The `DET_BUILTINS` 3-tuple → 4-tuple migration leaks into the `vocab_invariant` tests.** Both `det_builtins_are_det_rows_in_shared` and `recognized_names_equal_shared_vocabulary` already updated to the new shape. Fine — the cost is two destructures — but if a future addition (say, an `arity_min`/`arity_max` flag for variadic builtins) adds another field, every consumer updates. A named struct `DetBuiltin { name, arity, sym, raises }` would future-proof this and double as documentation at the call site. Stylistic. ### What's good - **`ErrorSiteGuard` is used uniformly across all 12 sites.** No hand-rolled set/restore lurking anywhere; the PR 17 disposition got the foundation right and Stage 3 leans on it instead of inventing variants. The pattern is self-enforcing exactly where the surface grew most. - **Save/restore semantics (Drop restores the *previous* site, not `NO_SITE`)** quietly solve the nesting concern from PR #17 review #3. If `plg_rt_b_sort_2` ever calls back into another raising builtin (today: leaf, but the architecture survives the change), the outer raise sees its own site again on inner-guard-Drop. Right-shaped now, right-shaped later. - **The `INVARIANT:` comment on `Machine::error_site`** addresses PR 17 review #2 directly. Future contributors who reach for `m.error_site = ...` see the contract. - **`NO_SITE`'s ABI contract note** ("MUST equal `plg_compiler::codegen::NO_SITE`") addresses PR 15 review #7. Independent consts, same value, documented as a contract — won't drift silently. - **Per-test wrappers (`fn alen`, `fn acat`, …)** keep existing behavior tests site-free. The intent is obvious to a reader: "this test exercises behavior, not provenance; site is elided." A single-line wrapper instead of inline `NO_SITE` at every call site reads cleaner. - **`control.rs`'s query-side dispatch passes `NO_SITE` explicitly.** Documented at the comment, mechanical at each call. Query-side raises stay byte-identical to v1; `query_side_type_check_error_has_no_location_suffix` pins it. - **`throw/1` explicitly excluded** with rationale (user-thrown ball isn't a system error). Right call — appending source location to a user's deliberate throw would be wrong. - **SPANS.md "Adding a new raising builtin is now two lines"** documents the recipe. Future contributors get a one-paragraph onboarding instead of having to read the four PRs that built up to it. - **The `raises` boolean drives decl-gen and emit from one bit.** Drift between IR shape and codegen lookup is impossible by construction. The single source of truth for "this builtin has provenance" is exactly the row that names the builtin. ### Suggested order #1 (the 10 missing end-to-end pins) is worth doing before merge — it's the only thing standing between "wired" and "verified," and it's a single table-driven test. #2 (carry `raises` in `RtDet`) is small, mechanical, and avoids fan-out lookups. #3+ are observations.

navicore referenced this pull request from a commit 2026-06-18 04:55:54 +00:00

⏺ 408 pass (+1 table-driven test), clippy clean. Disposition of the PR #18

navicore added 1 commit 2026-06-18 04:55:54 +00:00

⏺ 408 pass (+1 table-driven test), clippy clean. Disposition of the PR #18 ... 549d7f41af

review:

  Done before merge (the reviewer's recommended set):
  - #1 — end-to-end coverage for all 12 raising builtins: added
  every_raising_det_builtin_carries_provenance — one program with 12 clauses
  (one per raising builtin: functor/3, arg/3, =../2, atom_length/2,
  atom_concat/3, atom_chars/2, number_chars/2, number_codes/2, msort/2,
  sort/2, succ/2, plus/3), each hit with bad input, asserting each names a
  source location. This is exactly the table-driven pin the reviewer wanted —
  it catches a future raises-flag/runtime-signature mismatch that the IR
  generation can't (since both IR sides derive from the same flag, but the
  runtime ABI is independent). Coverage went 2/12 → 12/12.
  - #2 — raises in RtDet: LGoalKind::RtDet { sym, args, raises }; populated
  once in lower_goal (the find already produced the bit — it was being
  discarded), read O(1) at emit. Removed det_builtin_raises (the per-emit
  O(N) string scan).
  - #3 — the let _site footgun: added a FOOTGUN: doc note and #[must_use] to
  ErrorSiteGuard. #[must_use] catches the bare-statement form
  (ErrorSiteGuard::enter(...); → immediate drop); the doc covers the let _
  form it can't catch (which the reviewer correctly noted). I skipped the
  entered! macro — the doc + #[must_use] is enough without hiding a let
  binding behind a macro.

  Not done, deliberately:
  - #4 (compile-time table↔signature agreement): closed indirectly — #1's
  table-driven coverage is what proves the agreement at the runtime layer, as
  the reviewer noted.
  - #5 (named DetBuiltin struct vs the 4-tuple): stylistic, and #2 already
  removed the awkward consumer (det_builtin_raises). The tuple now has only
  the decl-gen, the lower_goal find, and the vocab tests as consumers — clean
  enough. Left to keep the PR focused; noting it as a future option if the
  table grows a fifth field.

  408 pass, clippy clean, byte-exact v1 + golden IR preserved. Nothing posted
  to the PR — ready for you to merge.

navicore merged commit 507e078b0f into main 2026-06-18 04:57:15 +00:00

navicore deleted branch ff-stage-3-span 2026-06-18 04:57:15 +00:00

navicore referenced this pull request from a commit 2026-06-18 04:57:17 +00:00

Merge pull request '⏺ 407 pass, clippy clean. Stage 3 complete.' (#18) from ff-stage-3-span into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-prolog!18

No description provided.