⏺ Stage 2 complete and green. Summary (git is yours — ready to branch/PR): #17

Merged

navicore merged 2 commits from ff-stage-2-span into main 2026-06-18 03:47:57 +00:00

Conversation 1 Commits 2 Files changed 11 +228 -85

navicore commented 2026-06-18 02:01:54 +00:00

Owner

Copy link

Stage 2: arithmetic provenance

Mechanism evolution (the interesting part): instead of threading a site_id
param down through the recursive arithmetic evaluator to each error
constructor, I added a Machine::error_site field. The raising builtin sets
it at its ABI boundary and clears it on exit; set_formal — the single
function every error constructor routes through — appends at file:line:col
from it. So:

• is/2 and the comparisons (plg_rt_b_is, plg_rt_b_arith_cmp) gain a site_id
  arg, set/reset error_site around the eval, and every error they can raise
  (evaluation_error, type_error, instantiation) gets provenance with zero
  evaluator changes.
• existence_error was migrated to the same field (one append point instead
  of an explicit param + helper). This supersedes the planned
  append_to_error_msg helper from PR#15 — the centralization in set_formal is
  the cleaner form, and NO_SITE (the default) allocates nothing on the
  no-provenance path.
• Codegen: emit_inline_builtin passes g.span's site_id for Is/ArithCmp; the
  two RUNTIME_DECLS updated.

Verified end-to-end on real binaries: compiled-body _ is 1 // 0 →
…evaluation_error(zero_divisor)… at prog.pl:2:5; _ is foo + 1 and 1 < foo →
type_error(evaluable, foo) at …:2:5; query-side _ is 1 // 0 → no suffix
(byte-identical to v1).

Tests: 4 new integration tests (eval/type/comparison provenance +
query-side no-suffix), plus the deferred PR#16 #2 pin
(lowered_goals_have_non_degenerate_spans). The byte-exact v1 unit tests and
golden IR are preserved. 404 pass, clippy clean.

SPANS.md updated to record Stage 2 and the error_site mechanism.

Stage 2: arithmetic provenance Mechanism evolution (the interesting part): instead of threading a site_id param down through the recursive arithmetic evaluator to each error constructor, I added a Machine::error_site field. The raising builtin sets it at its ABI boundary and clears it on exit; set_formal — the single function every error constructor routes through — appends at file:line:col from it. So: - is/2 and the comparisons (plg_rt_b_is, plg_rt_b_arith_cmp) gain a site_id arg, set/reset error_site around the eval, and every error they can raise (evaluation_error, type_error, instantiation) gets provenance with zero evaluator changes. - existence_error was migrated to the same field (one append point instead of an explicit param + helper). This supersedes the planned append_to_error_msg helper from PR#15 — the centralization in set_formal is the cleaner form, and NO_SITE (the default) allocates nothing on the no-provenance path. - Codegen: emit_inline_builtin passes g.span's site_id for Is/ArithCmp; the two RUNTIME_DECLS updated. Verified end-to-end on real binaries: compiled-body _ is 1 // 0 → …evaluation_error(zero_divisor)… at prog.pl:2:5; _ is foo + 1 and 1 < foo → type_error(evaluable, foo) at …:2:5; query-side _ is 1 // 0 → no suffix (byte-identical to v1). Tests: 4 new integration tests (eval/type/comparison provenance + query-side no-suffix), plus the deferred PR#16 #2 pin (lowered_goals_have_non_degenerate_spans). The byte-exact v1 unit tests and golden IR are preserved. 404 pass, clippy clean. SPANS.md updated to record Stage 2 and the error_site mechanism.

navicore added 1 commit 2026-06-18 02:01:54 +00:00

⏺ Stage 2 complete and green. Summary (git is yours — ready to branch/PR): ... 8a0fdd6e21

Stage 2: arithmetic provenance

  Mechanism evolution (the interesting part): instead of threading a site_id
  param down through the recursive arithmetic evaluator to each error
  constructor, I added a Machine::error_site field. The raising builtin sets
  it at its ABI boundary and clears it on exit; set_formal — the single
  function every error constructor routes through — appends  at file:line:col
  from it. So:
  - is/2 and the comparisons (plg_rt_b_is, plg_rt_b_arith_cmp) gain a site_id
  arg, set/reset error_site around the eval, and every error they can raise
  (evaluation_error, type_error, instantiation) gets provenance with zero
  evaluator changes.
  - existence_error was migrated to the same field (one append point instead
  of an explicit param + helper). This supersedes the planned
  append_to_error_msg helper from PR#15 — the centralization in set_formal is
  the cleaner form, and NO_SITE (the default) allocates nothing on the
  no-provenance path.
  - Codegen: emit_inline_builtin passes g.span's site_id for Is/ArithCmp; the
  two RUNTIME_DECLS updated.

  Verified end-to-end on real binaries: compiled-body _ is 1 // 0 →
  …evaluation_error(zero_divisor)… at prog.pl:2:5; _ is foo + 1 and 1 < foo →
  type_error(evaluable, foo) at …:2:5; query-side _ is 1 // 0 → no suffix
  (byte-identical to v1).

  Tests: 4 new integration tests (eval/type/comparison provenance +
  query-side no-suffix), plus the deferred PR#16 #2 pin
  (lowered_goals_have_non_degenerate_spans). The byte-exact v1 unit tests and
  golden IR are preserved. 404 pass, clippy clean.

  SPANS.md updated to record Stage 2 and the error_site mechanism.

navicore commented 2026-06-18 02:36:53 +00:00

Author

Owner

Copy link

Review — Stage 2: arithmetic provenance via Machine::error_site

The mechanism evolution is the part worth stopping on. Threading site_id through arith::eval and every error constructor it can reach (evaluation_error, type_error, instantiation, …) would have been an ugly recursive plumbing job. Instead, moving the suffix-append into set_formal — the one function every error constructor already routes through — and gating it on a Machine::error_site field set at the raising builtin's ABI boundary is a real architectural insight: it exploits an existing structural property of the runtime to make "add provenance to a new raising builtin" a two-line change. The migration of existence_error to the same mechanism is the right move; collapsing two patterns into one shrinks the surface a future contributor has to learn.

Issues below, ordered by impact.

Real concerns

1. The set/clear discipline is not RAII-enforced. plg_rt_b_is and plg_rt_b_arith_cmp both do:

m.error_site = site_id;
let r = match ... { ... };
m.error_site = crate::machine::NO_SITE;
r

This works for these two, but it's a try { ... } finally { reset } pattern with no compiler help. The failure modes for future raising builtins:

• Forgot to set: errors in that builtin silently lose provenance — quality regression, hard to notice in tests unless someone writes the matching integration test.
• Forgot to clear: a subsequent raise from a different code path picks up this builtin's site_id. Silent wrong provenance — much worse, and indistinguishable from a real raise from the user's standpoint.

The fast-follow list is long (functor/3, arg/3, =../2, the atom_* family, number_chars/codes, msort/sort, succ, plus). Each one is one more chance to miss a reset. A small RAII guard would make this self-enforcing:

struct ErrorSiteGuard<'a> { m: &'a mut Machine }
impl<'a> ErrorSiteGuard<'a> {
    fn enter(m: &'a mut Machine, site_id: u32) -> Self {
        m.error_site = site_id;
        ErrorSiteGuard { m }
    }
}
impl Drop for ErrorSiteGuard<'_> {
    fn drop(&mut self) { self.m.error_site = NO_SITE; }
}

Each raising builtin then does let _g = ErrorSiteGuard::enter(m, site_id); at the top and the reset is impossible to forget. It also kills the 'cmp: labeled block (#4 below) and the let r = ...; reset; r shuffle. The borrow-checker pain with mref(m) may be why the current shape avoids the guard — worth seeing if it works before the next stage's six builtins land each with their own set/clear pair.

2. set_formal now has a hidden dependency on m.error_site. Any future code path that calls set_formal (directly or transitively via resource, throw_term, etc.) and runs while m.error_site happens to be set will silently append a suffix from that site. This is the failure mode of #1 from the consumer side. Worth a load-bearing comment near set_formal:

// SAFETY: every caller must guarantee m.error_site is either NO_SITE or the
// site of the raise in flight. Stale values here = wrong-provenance bugs.
// Raising builtins MUST set + clear around their work (or use ErrorSiteGuard).

3. error_site assumes raising builtins are leaf operations. Today they are: arith::eval is a pure expression walker that never recurses back into compiled code or into another raising builtin. But the fast-follow list includes type-checking det builtins, and one of them may eventually want to call back into the goal walker (or invoke try_builtin — control.rs already calls back into pred::plg_rt_b_is). If a nested raise ever happens — outer raise sets error_site = A, inner raise sets error_site = B, returns, outer continues with B — the outer's raise gets the inner's site. Worth a // INVARIANT comment on the field saying "raising builtins are assumed leaf; if a meta-builtin makes nested raises possible, this needs a save/restore stack." The guard pattern in #1 actually solves this for free if the guard saves the previous value rather than resetting to NO_SITE.

Small observations

4. The 'cmp: labeled block in plg_rt_b_arith_cmp exists solely to hop past the m.error_site = NO_SITE cleanup at the end. It works, but it's an unusual idiom in this codebase — and it's exactly the pattern RAII would erase. If the guard from #1 lands, this collapses to a plain function body.

5. The PR #15 follow-ups don't appear in SPANS.md's "Remaining" section. Specifically, CodeGen::site_id still rebuilds SourceMap::new per call (O(N×M) over source) and the srcmap still doesn't dedupe. Out of scope here, but the doc reads as if Stage 2 is the last codegen-side item; worth a one-line "deferred from PR #15 review" note so they don't fall off the radar.

6. Two allocations on the hot path. errors.rs:

message.push_str(&format!(" at {file}:{line}:{col}"));

This formats into a temporary String and then copies it into message. A direct write avoids the temporary:

use std::fmt::Write;
let _ = write!(message, " at {file}:{line}:{col}");

Cold path today; matters once a tight loop (arithmetic in a recursive predicate) hits a type error repeatedly. Trivial.

7. The existence_suffix_appears_when_site_resolves unit test now sets m.error_site = 0; inline. Cleaner than threading the param. But: each test that wants provenance now has to remember to set it before AND reset after (the same discipline as #1, at the test layer). With per-test machine() construction the cross-test contamination risk is zero, but a single test that does multiple raises in sequence could trip on it. Mention only if you grow that test pattern.

What's good

• Centralizing append at set_formal exploits an existing choke point. Every error constructor already routes through it; piggybacking the suffix-append on that property turns "each new raising builtin threads site_id through every constructor" into "each new raising builtin sets/clears one field at its boundary." Real architectural insight, and it's the right call.
• arith::eval is untouched. That's the actual payoff — the recursive evaluator stays a pure arithmetic walker, ignorant of provenance. A future arith refactor (constant folding, bignum optimization) doesn't have to know spans exist.
• existence_error migrated to the same field. Two mechanisms collapse to one. The append_to_error_msg helper from PR #15's review is correctly superseded — the centralized field is the cleaner form, and SPANS.md records why.
• NO_SITE as the default field value preserves byte-identical v1 behavior. Runtime-internal raises (query-side, solve.rs dispatch) leave error_site untouched at NO_SITE → site_location returns None → no suffix. query_side_arith_error_has_no_location_suffix pins this contract directly.
• Four new integration tests cover all three error classes. evaluation_error, type_error (via is/2), type_error (via <), and the query-side no-suffix case. The contract is pinned at the binary-level integration layer, not just at the unit-test layer — that's the right place because it exercises the full ABI dance.
• The PR #16 review #2 deferred pin landed (lowered_goals_have_non_degenerate_spans). Good follow-through; the g.span invariant Stage 2 reads from is now defended.
• SPANS.md is honest about the design evolution. "How the suffix is applied (Stage 2 evolution)" calls out the divergence from the original sketch and explains why. Future contributors trying to add an append_to_error_msg helper will see the rationale for not doing so.
• control.rs callers pass NO_SITE explicitly. Query-side try_builtin doesn't accidentally inherit a previous raise's site; the explicit NO_SITE argument documents the intent.

Suggested order

#1 (the RAII guard) is worth doing as part of this PR — it's small, and the next round of raising builtins each gets to use it instead of reinventing the set/clear. #2 (the set_formal invariant comment) is a one-line addition. #3 (the leaf assumption) is a comment on the field. The rest are observations.

## Review — Stage 2: arithmetic provenance via `Machine::error_site` The mechanism evolution is the part worth stopping on. Threading `site_id` through `arith::eval` and every error constructor it can reach (evaluation_error, type_error, instantiation, …) would have been an ugly recursive plumbing job. Instead, moving the suffix-append into `set_formal` — the one function every error constructor already routes through — and gating it on a `Machine::error_site` field set at the raising builtin's ABI boundary is a real architectural insight: it exploits an existing structural property of the runtime to make "add provenance to a new raising builtin" a two-line change. The migration of `existence_error` to the same mechanism is the right move; collapsing two patterns into one shrinks the surface a future contributor has to learn. Issues below, ordered by impact. ### Real concerns **1. The set/clear discipline is not RAII-enforced.** `plg_rt_b_is` and `plg_rt_b_arith_cmp` both do: ```rust m.error_site = site_id; let r = match ... { ... }; m.error_site = crate::machine::NO_SITE; r ``` This works for these two, but it's a `try { ... } finally { reset }` pattern with no compiler help. The failure modes for future raising builtins: - **Forgot to set**: errors in that builtin silently lose provenance — quality regression, hard to notice in tests unless someone writes the matching integration test. - **Forgot to clear**: a subsequent raise from a *different* code path picks up this builtin's site_id. **Silent wrong provenance** — much worse, and indistinguishable from a real raise from the user's standpoint. The fast-follow list is long (`functor/3`, `arg/3`, `=../2`, the `atom_*` family, `number_chars/codes`, `msort`/`sort`, `succ`, `plus`). Each one is one more chance to miss a reset. A small RAII guard would make this self-enforcing: ```rust struct ErrorSiteGuard<'a> { m: &'a mut Machine } impl<'a> ErrorSiteGuard<'a> { fn enter(m: &'a mut Machine, site_id: u32) -> Self { m.error_site = site_id; ErrorSiteGuard { m } } } impl Drop for ErrorSiteGuard<'_> { fn drop(&mut self) { self.m.error_site = NO_SITE; } } ``` Each raising builtin then does `let _g = ErrorSiteGuard::enter(m, site_id);` at the top and the reset is impossible to forget. It also kills the `'cmp:` labeled block (#4 below) and the `let r = ...; reset; r` shuffle. The borrow-checker pain with `mref(m)` may be why the current shape avoids the guard — worth seeing if it works before the next stage's six builtins land each with their own set/clear pair. **2. `set_formal` now has a hidden dependency on `m.error_site`.** Any future code path that calls `set_formal` (directly or transitively via `resource`, `throw_term`, etc.) and runs while `m.error_site` happens to be set will silently append a suffix from that site. This is the failure mode of #1 from the consumer side. Worth a load-bearing comment near `set_formal`: ```rust // SAFETY: every caller must guarantee m.error_site is either NO_SITE or the // site of the raise in flight. Stale values here = wrong-provenance bugs. // Raising builtins MUST set + clear around their work (or use ErrorSiteGuard). ``` **3. `error_site` assumes raising builtins are leaf operations.** Today they are: `arith::eval` is a pure expression walker that never recurses back into compiled code or into another raising builtin. But the fast-follow list includes type-checking det builtins, and one of them may eventually want to call back into the goal walker (or invoke `try_builtin` — `control.rs` already calls back into `pred::plg_rt_b_is`). If a nested raise ever happens — outer raise sets `error_site = A`, inner raise sets `error_site = B`, returns, outer continues with `B` — the outer's raise gets the inner's site. Worth a `// INVARIANT` comment on the field saying "raising builtins are assumed leaf; if a meta-builtin makes nested raises possible, this needs a save/restore stack." The guard pattern in #1 actually solves this for free if the guard saves the previous value rather than resetting to `NO_SITE`. ### Small observations **4. The `'cmp:` labeled block in `plg_rt_b_arith_cmp`** exists solely to hop past the `m.error_site = NO_SITE` cleanup at the end. It works, but it's an unusual idiom in this codebase — and it's exactly the pattern RAII would erase. If the guard from #1 lands, this collapses to a plain function body. **5. The PR #15 follow-ups don't appear in SPANS.md's "Remaining" section.** Specifically, `CodeGen::site_id` still rebuilds `SourceMap::new` per call (O(N×M) over source) and the srcmap still doesn't dedupe. Out of scope here, but the doc reads as if Stage 2 is the last *codegen-side* item; worth a one-line "deferred from PR #15 review" note so they don't fall off the radar. **6. Two allocations on the hot path.** `errors.rs`: ```rust message.push_str(&format!(" at {file}:{line}:{col}")); ``` This formats into a temporary `String` and then copies it into `message`. A direct write avoids the temporary: ```rust use std::fmt::Write; let _ = write!(message, " at {file}:{line}:{col}"); ``` Cold path today; matters once a tight loop (arithmetic in a recursive predicate) hits a type error repeatedly. Trivial. **7. The `existence_suffix_appears_when_site_resolves` unit test now sets `m.error_site = 0;` inline.** Cleaner than threading the param. But: each test that wants provenance now has to remember to set it before AND reset after (the same discipline as #1, at the test layer). With per-test `machine()` construction the cross-test contamination risk is zero, but a single test that does multiple raises in sequence could trip on it. Mention only if you grow that test pattern. ### What's good - **Centralizing append at `set_formal` exploits an existing choke point.** Every error constructor already routes through it; piggybacking the suffix-append on that property turns "each new raising builtin threads site_id through every constructor" into "each new raising builtin sets/clears one field at its boundary." Real architectural insight, and it's the right call. - **`arith::eval` is untouched.** That's the actual payoff — the recursive evaluator stays a pure arithmetic walker, ignorant of provenance. A future arith refactor (constant folding, bignum optimization) doesn't have to know spans exist. - **`existence_error` migrated to the same field.** Two mechanisms collapse to one. The `append_to_error_msg` helper from PR #15's review is correctly superseded — the centralized field is the cleaner form, and SPANS.md records why. - **`NO_SITE` as the default field value preserves byte-identical v1 behavior.** Runtime-internal raises (query-side, `solve.rs` dispatch) leave `error_site` untouched at `NO_SITE` → `site_location` returns `None` → no suffix. `query_side_arith_error_has_no_location_suffix` pins this contract directly. - **Four new integration tests cover all three error classes.** `evaluation_error`, `type_error` (via `is/2`), `type_error` (via `<`), and the query-side no-suffix case. The contract is pinned at the binary-level integration layer, not just at the unit-test layer — that's the right place because it exercises the full ABI dance. - **The PR #16 review #2 deferred pin landed** (`lowered_goals_have_non_degenerate_spans`). Good follow-through; the `g.span` invariant Stage 2 reads from is now defended. - **SPANS.md is honest about the design evolution.** "How the suffix is applied (Stage 2 evolution)" calls out the divergence from the original sketch and explains why. Future contributors trying to add an `append_to_error_msg` helper will see the rationale for not doing so. - **`control.rs` callers pass `NO_SITE` explicitly.** Query-side `try_builtin` doesn't accidentally inherit a previous raise's site; the explicit NO_SITE argument documents the intent. ### Suggested order #1 (the RAII guard) is worth doing as part of this PR — it's small, and the next round of raising builtins each gets to use it instead of reinventing the set/clear. #2 (the `set_formal` invariant comment) is a one-line addition. #3 (the leaf assumption) is a comment on the field. The rest are observations.

navicore referenced this pull request from a commit 2026-06-18 03:42:39 +00:00

⏺ 404 pass, clippy clean — the guard refactor is behavior-preserving (the

navicore added 1 commit 2026-06-18 03:42:39 +00:00

⏺ 404 pass, clippy clean — the guard refactor is behavior-preserving (the ... 0a954bc044

arith/existence provenance integration tests are in that 404). Disposition
  of the PR #17 review:

  Done this PR (the reviewer's recommended set):
  - #1 RAII guard: added ErrorSiteGuard to machine.rs and used it in
  plg_rt_b_is, plg_rt_b_arith_cmp, and plg_rt_existence_error. Set/clear is
  now impossible to forget — each upcoming Stage 3 builtin is just let _site
  = ErrorSiteGuard::enter(m, site_id); at the top.
  - #3 nesting: the guard restores the previous value (not NO_SITE), so a
  future meta-builtin that makes nested raises possible is correct for free —
  the field behaves like a save/restore stack without one. Documented as an
  INVARIANT on error_site.
  - #4 labeled block: gone. With the guard's Drop doing the cleanup, both
  functions are plain early-return bodies again; the 'cmp: block is removed.
  - #2 set_formal contract: added the load-bearing comment ("reads
  m.error_site; stale = wrong-provenance bug; enforced by ErrorSiteGuard").
  - #6 hot-path alloc: replaced push_str(&format!(...)) with write!(message,
  ...) — no temporary String.

  No action — #5 is stale. The reviewer flagged CodeGen::site_id as still
  rebuilding SourceMap per call and not deduping the srcmap — but both were
  fixed in the PR#15 review and are on this branch: srcmaps: Vec<SourceMap>
  is built once per source, and site_cache: HashMap<(file,line,col), site_id>
  dedupes rows. So there's nothing deferred to note in SPANS.md.

  #7 (test set/clear discipline): observation only; the existing test does a
  single raise per machine(), so no contamination risk. If that test ever
  grows multiple sequential raises, it can use ErrorSiteGuard too. Left
  as-is.

  Byte-exact v1 messages and golden IR are preserved; nothing posted to the
  PR. Ready for you to merge — Stage 3 (the type-checking det builtins) now
  gets the guard for free.

navicore merged commit b88aa92e43 into main 2026-06-18 03:47:57 +00:00

navicore deleted branch ff-stage-2-span 2026-06-18 03:47:57 +00:00

navicore referenced this pull request from a commit 2026-06-18 03:47:58 +00:00

Merge pull request '⏺ Stage 2 complete and green. Summary (git is yours — ready to branch/PR):' (#17) from ff-stage-2-span into main

navicore referenced this pull request 2026-06-18 04:39:48 +00:00

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

navicore referenced this pull request 2026-06-20 06:12:48 +00:00

⏺ Confirmed clean: the only @plg_rt_metacall emission left is #24

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!17

No description provided.