⏺ Stage 1 is complete and green. Summary of this PR (behavior-preserving #16

Merged

navicore merged 2 commits from staged-pr-1-span into main

2026-06-18 00:28:23 +00:00

navicore commented

2026-06-18 00:14:29 +00:00

Owner

refactor — no runtime behavior change):

Stage 1: LGoal = Spanned

Renamed enum LGoal → enum LGoalKind; the goal IR is now type LGoal =
Spanned, reusing plg_shared::Spanned. Every goal carries a span
uniformly — the span moved off the Call variant onto the wrapper, and
codegen reads goal.span at the existence-error site.
Threaded the change through lower.rs (construction +
flatten_conj/lower_body now share a splice_lowered helper), body.rs
(compile_seq matches &goal.node), and clause.rs (emit_inline_builtin
matches &g.node).
Spanned now has real in-tree consumers (doc updated; this closes the
"defined-but-unused" note from PR#14's review). SPANS.md's fast-follow note
updated — the wrapper is done; only the append_to_error_msg helper remains
as in-line work for the next stage.

Verification: 399 tests pass, clippy -D warnings clean, and crucially
golden IR is byte-identical + existence_error provenance still works —
confirming it's a pure refactor. The payoff: adding a raising kind now
costs zero span plumbing (just read g.span), and finer granularity later is
a parser change, not an IR one.

Ready for you to branch/PR. Stage 2 (next PR) wires arithmetic provenance
on top of this: site_id through plg_rt_b_is + plg_rt_b_arith_cmp, the
runtime error constructors (evaluation/type_error) gain a site_id param,
codegen passes goal.span at those call sites (spans are already there via
the wrapper), plus the append_to_error_msg helper. Then later stages: the
type-checking det builtins, and the diff-test suffix stripping.

refactor — no runtime behavior change): Stage 1: LGoal = Spanned<LGoalKind> - Renamed enum LGoal → enum LGoalKind; the goal IR is now type LGoal = Spanned<LGoalKind>, reusing plg_shared::Spanned. Every goal carries a span uniformly — the span moved off the Call variant onto the wrapper, and codegen reads goal.span at the existence-error site. - Threaded the change through lower.rs (construction + flatten_conj/lower_body now share a splice_lowered helper), body.rs (compile_seq matches &goal.node), and clause.rs (emit_inline_builtin matches &g.node). - Spanned<T> now has real in-tree consumers (doc updated; this closes the "defined-but-unused" note from PR#14's review). SPANS.md's fast-follow note updated — the wrapper is done; only the append_to_error_msg helper remains as in-line work for the next stage. Verification: 399 tests pass, clippy -D warnings clean, and crucially golden IR is byte-identical + existence_error provenance still works — confirming it's a pure refactor. The payoff: adding a raising kind now costs zero span plumbing (just read g.span), and finer granularity later is a parser change, not an IR one. Ready for you to branch/PR. Stage 2 (next PR) wires arithmetic provenance on top of this: site_id through plg_rt_b_is + plg_rt_b_arith_cmp, the runtime error constructors (evaluation/type_error) gain a site_id param, codegen passes goal.span at those call sites (spans are already there via the wrapper), plus the append_to_error_msg helper. Then later stages: the type-checking det builtins, and the diff-test suffix stripping.

navicore added 1 commit

2026-06-18 00:14:29 +00:00

⏺ Stage 1 is complete and green. Summary of this PR (behavior-preserving

CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m40s

Details

6c89f34d9e

refactor — no runtime behavior change):

  Stage 1: LGoal = Spanned<LGoalKind>

  - Renamed enum LGoal → enum LGoalKind; the goal IR is now type LGoal =
  Spanned<LGoalKind>, reusing plg_shared::Spanned. Every goal carries a span
  uniformly — the span moved off the Call variant onto the wrapper, and
  codegen reads goal.span at the existence-error site.
  - Threaded the change through lower.rs (construction +
  flatten_conj/lower_body now share a splice_lowered helper), body.rs
  (compile_seq matches &goal.node), and clause.rs (emit_inline_builtin
  matches &g.node).
  - Spanned<T> now has real in-tree consumers (doc updated; this closes the
  "defined-but-unused" note from PR#14's review). SPANS.md's fast-follow note
  updated — the wrapper is done; only the append_to_error_msg helper remains
  as in-line work for the next stage.

  Verification: 399 tests pass, clippy -D warnings clean, and crucially
  golden IR is byte-identical + existence_error provenance still works —
  confirming it's a pure refactor. The payoff: adding a raising kind now
  costs zero span plumbing (just read g.span), and finer granularity later is
  a parser change, not an IR one.

  Ready for you to branch/PR. Stage 2 (next PR) wires arithmetic provenance
  on top of this: site_id through plg_rt_b_is + plg_rt_b_arith_cmp, the
  runtime error constructors (evaluation/type_error) gain a site_id param,
  codegen passes goal.span at those call sites (spans are already there via
  the wrapper), plus the append_to_error_msg helper. Then later stages: the
  type-checking det builtins, and the diff-test suffix stripping.

navicore commented

2026-06-18 00:18:03 +00:00

Author

Owner

Review — Stage 1: `LGoal = Spanned<LGoalKind>`

Small, focused, behavior-preserving refactor. Right call to do this before wiring arithmetic provenance — the next-stage diff becomes "read g.span at the emit site" instead of "thread span into seven variants and update every constructor." The fact that golden IR comes out byte-identical is the test result that matters; it's load-bearing evidence that the refactor is purely structural.

What's right

Wrapper-once instead of field-per-variant. Putting span on Spanned<LGoalKind> rather than LGoalKind::Call is the maintainable shape: each new raising kind reads g.span with zero plumbing, and the type system stops being something to re-thread for each addition.
splice_lowered deduplicates the conj-splicing logic. The drop-True + flatten-Conj + push-default branch existed in both flatten_conj and lower_body; one helper now owns it. If a future kind wants to be spliced (e.g. Disj of one element collapses), there's now one place to teach.
Spanned<T> doc updated. Closes the dangling "no in-tree consumer" note from PR #14's review. The new comment names both consumers (parser body conjuncts, IR goals) instead of leaving it as "reserved for Layer 3."
SPANS.md fast-follow list shrinks by one. Honest progress markers — "Stage 1 done; only append_to_error_msg remains" — let a future contributor pick up the slack without rereading the whole doc.
Per-construct spans are now available, not just call sites. LGoalKind::Disj, IfThen, IfThenElse, Naf, Once each take Box<LGoal> = Box<Spanned<LGoalKind>>, so each branch already carries its leaf span. Stage 2 + later raising kinds (type_error in arithmetic, evaluation_error in is/2) get precise leaf provenance for free.

Small observations

1. splice_lowered drops the outer Conj wrapper's span. When (b, c) lowers to Spanned<Conj([Spanned<b>, Spanned<c>])>, the outer span (covering the parenthesized expression) is silently discarded as we extend with inner. Today this is a no-op: flatten_conj propagates the same span to every inner leaf via the recursion, so the inner goals already carry the parent's span. But the wrapper span itself is gone — if a future feature wants "this Conj appeared at file:line:col" for, say, nested-goal stack traces, that information is no longer reachable from the spliced output. Worth a one-line comment in splice_lowered documenting "we drop the wrapper span; per-leaf spans (set by flatten_conj's propagation) are the source of truth after splicing."

2. No regression test asserts "every lowered goal has a non-default span." The type system guarantees the field exists, but a Span::point(0, 0) slipping in from a construction path that forgets span would compile clean. A small lowering test that walks Vec<LGoal> for a representative body and asserts each g.span != Span::point(0, 0) (or span.hi > span.lo for non-empty inputs) would pin the contract — useful when Stage 2 starts using g.span in tight loops. Not a blocker; the existence_error provenance test from PR #15 indirectly covers Call.

3. splice_lowered's Spanned::new(kind, lowered.span) reconstruction is the cost of partial-moving lowered.node in the match. Acceptable as written. A destructure-first form would slightly cleaner:

let Spanned { node, span } = lowered;
match node {
    LGoalKind::True => {}
    LGoalKind::Conj(inner) => out.extend(inner),
    kind => out.push(Spanned::new(kind, span)),
}

Reads as "split the wrapper, decide what to do with the kind." Stylistic; same compiled output.

4. goals_of now uses _ instead of other as the wildcard. Old code: other => vec![other.clone()]. New: _ => vec![g.clone()]. Equivalent behavior — the wildcard is fine since we already have g in scope — but the prior form was slightly more self-documenting ("the non-Conj arm"). Trivial.

What's not in scope (and shouldn't be)

The append_to_error_msg(&mut self, &str) helper is the right thing to land alongside Stage 2 (arithmetic provenance) rather than here — it only pays off once a hot raising path exists. Same for the lexer-span loss carried over from PR #14: out of scope for the IR refactor.

Net

Pure structural cleanup that pays for itself the moment Stage 2 wires g.span into plg_rt_b_is and plg_rt_b_arith_cmp. Ready to merge as-is; the wrapper-span-drop note (#1) is the only thing I'd want documented before moving on.

## Review — Stage 1: `LGoal = Spanned<LGoalKind>` Small, focused, behavior-preserving refactor. Right call to do this *before* wiring arithmetic provenance — the next-stage diff becomes "read `g.span` at the emit site" instead of "thread `span` into seven variants and update every constructor." The fact that golden IR comes out byte-identical is the test result that matters; it's load-bearing evidence that the refactor is purely structural. ### What's right - **Wrapper-once instead of field-per-variant.** Putting `span` on `Spanned<LGoalKind>` rather than `LGoalKind::Call` is the maintainable shape: each new raising kind reads `g.span` with zero plumbing, and the type system stops being something to re-thread for each addition. - **`splice_lowered` deduplicates the conj-splicing logic.** The drop-`True` + flatten-`Conj` + push-default branch existed in both `flatten_conj` and `lower_body`; one helper now owns it. If a future kind wants to be spliced (e.g. `Disj` of one element collapses), there's now one place to teach. - **`Spanned<T>` doc updated.** Closes the dangling "no in-tree consumer" note from PR #14's review. The new comment names both consumers (parser body conjuncts, IR goals) instead of leaving it as "reserved for Layer 3." - **SPANS.md fast-follow list shrinks by one.** Honest progress markers — "Stage 1 done; only `append_to_error_msg` remains" — let a future contributor pick up the slack without rereading the whole doc. - **Per-construct spans are now available, not just call sites.** `LGoalKind::Disj`, `IfThen`, `IfThenElse`, `Naf`, `Once` each take `Box<LGoal> = Box<Spanned<LGoalKind>>`, so each branch already carries its leaf span. Stage 2 + later raising kinds (type_error in arithmetic, evaluation_error in `is/2`) get precise leaf provenance for free. ### Small observations **1. `splice_lowered` drops the outer `Conj` wrapper's span.** When `(b, c)` lowers to `Spanned<Conj([Spanned<b>, Spanned<c>])>`, the outer span (covering the parenthesized expression) is silently discarded as we extend with `inner`. Today this is a no-op: `flatten_conj` propagates the same `span` to every inner leaf via the recursion, so the inner goals already carry the parent's span. But the wrapper span itself is gone — if a future feature wants "this Conj appeared at file:line:col" for, say, nested-goal stack traces, that information is no longer reachable from the spliced output. Worth a one-line comment in `splice_lowered` documenting "we drop the wrapper span; per-leaf spans (set by `flatten_conj`'s propagation) are the source of truth after splicing." **2. No regression test asserts "every lowered goal has a non-default span."** The type system guarantees the field exists, but a `Span::point(0, 0)` slipping in from a construction path that forgets `span` would compile clean. A small lowering test that walks `Vec<LGoal>` for a representative body and asserts each `g.span != Span::point(0, 0)` (or `span.hi > span.lo` for non-empty inputs) would pin the contract — useful when Stage 2 starts using `g.span` in tight loops. Not a blocker; the existence_error provenance test from PR #15 indirectly covers `Call`. **3. `splice_lowered`'s `Spanned::new(kind, lowered.span)` reconstruction** is the cost of partial-moving `lowered.node` in the match. Acceptable as written. A destructure-first form would slightly cleaner: ```rust let Spanned { node, span } = lowered; match node { LGoalKind::True => {} LGoalKind::Conj(inner) => out.extend(inner), kind => out.push(Spanned::new(kind, span)), } ``` Reads as "split the wrapper, decide what to do with the kind." Stylistic; same compiled output. **4. `goals_of` now uses `_` instead of `other` as the wildcard.** Old code: `other => vec![other.clone()]`. New: `_ => vec![g.clone()]`. Equivalent behavior — the wildcard is fine since we already have `g` in scope — but the prior form was slightly more self-documenting ("the non-Conj arm"). Trivial. ### What's not in scope (and shouldn't be) The `append_to_error_msg(&mut self, &str)` helper is the right thing to land alongside Stage 2 (arithmetic provenance) rather than here — it only pays off once a hot raising path exists. Same for the lexer-span loss carried over from PR #14: out of scope for the IR refactor. ### Net Pure structural cleanup that pays for itself the moment Stage 2 wires `g.span` into `plg_rt_b_is` and `plg_rt_b_arith_cmp`. Ready to merge as-is; the wrapper-span-drop note (#1) is the only thing I'd want documented before moving on.

navicore referenced this pull request from a commit

2026-06-18 00:26:20 +00:00

⏺ Built clean. Disposition of the PR #16 review (reviewer's verdict was

navicore added 1 commit

2026-06-18 00:26:20 +00:00

⏺ Built clean. Disposition of the PR #16 review (reviewer's verdict was

CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m5s

Details

6ff24693a7

merge-ready):

  Documented — #1 (the one thing the reviewer wanted documented before moving
  on): added a note in splice_lowered explaining that splicing a Conj
  intentionally discards the wrapper's span, and why that's lossless for
  provenance — flatten_conj already propagated the same span to every inner
  leaf, so per-leaf spans are the source of truth after splicing; the only
  thing lost is "this whole parenthesized group appeared at X," which no
  current consumer needs (a nested-goal stack trace would be the first that
  might).

  Folded in — #3 (destructure-first): rewrote the match as let Spanned {
  node, span } = lowered; so the span-drop is self-evident in the code (span
  is visibly used only in the default arm). Same compiled output; it just
  makes #1's point legible.

  Not done, deliberately:
  - #2 (test asserting every lowered goal has a non-default span): it's a
  test, not documentation, and the reviewer themselves noted it's "useful
  when Stage 2 starts using g.span in tight loops." I'd land it with Stage 2
  — that's where g.span actually starts getting consumed in the arithmetic
  paths, so the pin is most meaningful (and least likely to be a lonely test)
  there.
  - #4 (_ vs other wildcard in goals_of): trivial; _ with g already in scope
  reads fine. Skipping.

  So just the one doc note + the stylistic destructure this pass; nothing
  structural. Ready for you to merge, and Stage 2 picks up arithmetic
  provenance (plus the deferred #2 pin) on top.