⏺ 396 tests pass, clippy clean. Done. #15

Merged
navicore merged 2 commits from span-3 into main 2026-06-17 22:33:15 +00:00
Owner

What I changed in build.rs (stable, no nightly)

The bug was selection, not cargo: build.rs reads the runtime .a off disk
to include_bytes! it, and it preferred the uplifted
target//libplg_runtime.a. That name is only refreshed by a
primary build of plg-runtime; when plgc is built alone, plg-runtime
compiles as its build-dependency into deps/libplg_runtime-.a and
the top-level copy goes stale — so plgc embedded old runtime bytes.

The fix:

  • Drop the top-level preference. Scan deps/ and pick the newest
    libplg_runtime-*.a by mtime — the one cargo wrote for this build
    (build-deps compile before the dependent's build script runs). Ties break
    on path for determinism. The uplifted copy is now only a last-resort
    fallback if deps/ somehow has none.
  • Watch the runtime sources (../runtime/src, ../runtime/Cargo.toml)
    explicitly, as belt-and-suspenders alongside the (reliable)
    build-dependency relink trigger.

You were right that incremental compile is solved — cargo's artifacts in
deps/ were correct all along; the hand-rolled handoff just needed to
point at the right one. The garbage-uplift experiment confirms it now
does, and 396 tests + clippy are green.

What I changed in build.rs (stable, no nightly) The bug was selection, not cargo: build.rs reads the runtime .a off disk to include_bytes! it, and it preferred the uplifted target/<profile>/libplg_runtime.a. That name is only refreshed by a primary build of plg-runtime; when plgc is built alone, plg-runtime compiles as its build-dependency into deps/libplg_runtime-<hash>.a and the top-level copy goes stale — so plgc embedded old runtime bytes. The fix: - Drop the top-level preference. Scan deps/ and pick the newest libplg_runtime-*.a by mtime — the one cargo wrote for this build (build-deps compile before the dependent's build script runs). Ties break on path for determinism. The uplifted copy is now only a last-resort fallback if deps/ somehow has none. - Watch the runtime sources (../runtime/src, ../runtime/Cargo.toml) explicitly, as belt-and-suspenders alongside the (reliable) build-dependency relink trigger. You were right that incremental compile is solved — cargo's artifacts in deps/ were correct all along; the hand-rolled handoff just needed to point at the right one. The garbage-uplift experiment confirms it now does, and 396 tests + clippy are green.
⏺ 396 tests pass, clippy clean. Done.
All checks were successful
CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m6s
78e4e1cf9f
What I changed in build.rs (stable, no nightly)

  The bug was selection, not cargo: build.rs reads the runtime .a off disk
  to include_bytes! it, and it preferred the uplifted
  target/<profile>/libplg_runtime.a. That name is only refreshed by a
  primary build of plg-runtime; when plgc is built alone, plg-runtime
  compiles as its build-dependency into deps/libplg_runtime-<hash>.a and
  the top-level copy goes stale — so plgc embedded old runtime bytes.

  The fix:
  - Drop the top-level preference. Scan deps/ and pick the newest
  libplg_runtime-*.a by mtime — the one cargo wrote for this build
  (build-deps compile before the dependent's build script runs). Ties break
  on path for determinism. The uplifted copy is now only a last-resort
  fallback if deps/ somehow has none.
  - Watch the runtime sources (../runtime/src, ../runtime/Cargo.toml)
  explicitly, as belt-and-suspenders alongside the (reliable)
  build-dependency relink trigger.

  You were right that incremental compile is solved — cargo's artifacts in
  deps/ were correct all along; the hand-rolled handoff just needed to
  point at the right one. The garbage-uplift experiment confirms it now
  does, and 396 tests + clippy are green.
Author
Owner

Review — Spans Layer 3 + build.rs runtime selection

The transport choice is the right one. Side-table + site_id arg, suffix-on-the-message-only, ISO ball untouched: oracle bytes stay byte-identical (verified by query_side_existence_error_has_no_location_suffix and the runtime unit test pinning the exact v1 string). Two sentinels (NO_SITE = u32::MAX in both crates) keep the "no provenance" path explicit instead of relying on empty optionals; the stdlib-via-clause_without_provenance trick — stamp Span::point(u32::MAX, 0), let site_id see an out-of-bounds FileId and return NO_SITE — is clever and correct. Spanned<T> is now actually used (closing the PR #14 observation). The build.rs fix is a real improvement over a real bug.

Issues below, ordered by impact.

Real concerns

1. CodeGen::site_id rebuilds the SourceMap on every call site. codegen/mod.rs:

let (line, col) = SourceMap::new(&src.text).line_col(span.lo);

SourceMap::new walks the entire source byte stream to find line breaks. With N call sites in an M-byte source, that's O(N × M) — quadratic in source size. A 5k-line program with 5k call sites already costs a few hundred million byte iterations just to populate the srcmap. Easy fix: build one SourceMap per CgSource up front (or lazily, via OnceCell<SourceMap> keyed by file_idx) and reuse.

2. site_id doesn't dedupe. Two call sites at the same (file_idx, line, col) — easy to produce because nested goals inherit their enclosing conjunct's span — each append a fresh @plg_srcmap row. The srcmap grows with every emitted call rather than every distinct location. A HashMap<(u32, u32, u32), u32> cache would collapse duplicates and shrink the IR/binary footprint, especially after #1 makes srcmap entries cheap to construct. Not a correctness bug; just bloat that compounds against the "pay-for-what-you-use" footprint claim.

3. The build.rs mtime heuristic is pragmatic, not airtight. Picking the newest libplg_runtime-*.a by mtime correctly handles the bug you describe (uplifted top-level copy goes stale when plg-runtime is a build-dep). Residual risks:

  • Sibling crates touching deps/. A workspace where another crate also depends on plg-runtime (or transitively recompiles it) could leave a newer libplg_runtime-<other_hash>.a from a different rustc invocation. mtime says "newer," but it's not the artifact this plgc build's deps-graph wanted.
  • No-op rebuilds. If plgc is rebuilt without changes, cargo doesn't relink the runtime, mtime stays where it was, and a later-touched unrelated libplg_runtime-*.a could win. Probably impossible because cargo's relink trigger always reruns the build script when this script's last-mod is older — but worth confirming.
  • The principled identifier is the build-dep's metadata hash. Cargo encodes it into the filename suffix; you could pull it from CARGO_MANIFEST_DIR's .fingerprint/plg-runtime-* or, more cleanly, from the build-script env (when a dep has a build script of its own, DEP_<name>_* envvars surface; plg-runtime doesn't have one today, but adding a no-op build script just to publish a cargo:metadata=hash=... would give a positive identifier).

The mtime approach is the right shape for landing now — it fixes the bug; the alternative is a bigger refactor or a deployment-model change (link normally instead of include_bytes!). Worth a short comment in the script noting which guarantee mtime provides ("cargo wrote the build-dep artifact during this build, so it is the newest among artifacts cargo touched") and which it doesn't ("we cannot positively identify it as the one cargo's metadata pinned for this build").

Also: (mtime, &path) > (*t, p) tiebreaks on lexically-larger path. Deterministic but arbitrary — worth a one-liner explaining why this direction (vs. lexically-smaller). Most build scripts I've seen tie-break on lexically-smaller because that's what BTreeMap gives you for free; either is fine, but document the choice.

Design observations

4. PR #14 lexer-span loss is still pending. Tokenizer::tokenize still returns Result<_, String> (tokenizer/mod.rs:167-170 still emits "Unexpected character 'X' at line N col M"), so lexer errors still route through From<String> for ParseError and land at Span::point(0, 0). Out of scope for span-3, but the SPANS.md "Status" line now reads "Layers 1–2 done" while a real failure path in Layer 1 is still broken. Worth a tracking note in the design doc or a separate small follow-up — not a blocker for this PR.

5. Top-level ; collapses every conjunct span into one body-wide span. parse_body_conjuncts's special case is documented ("Top-level disjunction: rebuild what we parsed as the left arm..."), but the consequence is that a body like setup, work, cleanup ; recover. makes the existence_error suffix for an undefined call inside any of those goals point at the whole setup, work, cleanup ; recover range. Not wrong, but a user reading "Undefined procedure: cleanup/1 at file:line:col" expects col to land near cleanup, not at setup. Worth a one-line caveat in the design doc and probably in a test (an existence_error_in_disjunctive_body_carries_coarse_span pin so a future "let's make this granular" refactor knows what it changes).

6. LGoal::Call carries span; other LGoal variants don't. Today only existence_error uses it. The fast-follow list (is/2, type_error, evaluation_error, throw/1) will need spans on whichever LGoal variants lower to those raises — RtDet, Is, ArithCmp, Metacall. The threading you'd need is the same shape as Call, but each new raising builtin is one more lower_goal call site to update. A pub span: Span on a wrapper struct LGoal { kind: LGoalKind, span: Span } would move the bookkeeping into one place rather than per-variant; consider before the next layer of raising builtins lands.

Small observations

7. The two NO_SITE constantsplg_compiler::codegen::NO_SITE and plg_runtime::machine::NO_SITE — are independent definitions that happen to agree at u32::MAX. The doc comments on each link them by reference; a one-line // ABI contract — must match plg_runtime::machine::NO_SITE and a static_assertions::const_assert_eq! (if you already depend on it) or even just a runtime debug_assert_eq!(plg_compiler::NO_SITE, plg_runtime::NO_SITE) somewhere in tests would catch a future "let's renumber" drift.

Same observation for i32 (IR) vs u32 (Rust ABI). The two's-complement reinterpret works (i32 4294967295 is the bit pattern for u32::MAX), and query_side_existence_error_has_no_location_suffix pins it — but the line format!("i32 {site}", site = u32::MAX) reads as suspicious without that context. A one-line comment at the emit site noting "i32 wire / u32 ABI; two's complement: NO_SITE = -1 here = u32::MAX in the runtime" would save a future reader some squinting.

8. Machine::site_location clones the filename String on every raise. Documented as "Owned so the (cold) error path can mutate self.error without a borrow conflict." Fine for existence_error (cold), but as you wire type_error/evaluation_error in tighter loops, consider an append_to_error_msg(&mut self, suffix: &str) helper that does the format-and-append without leaving the borrow window open. Saves the per-raise allocation when the path becomes hot.

9. parse_sources and parse_sources_cg duplicate the read-file/shebang-strip loop. Small duplication (six lines), but if a future shebang-handling change misses one, the LSP lint path and the codegen path will silently diverge. A read_source(path) -> Result<String, String> helper would dedupe and pin the contract.

What's good

  • "Side-table side, message-only suffix" means the ISO ball/Formal/Context stay byte-identical to v1. existence_message_matches_v1_bytes and the new existence_suffix_appears_when_site_resolves pin both halves of the contract cleanly.
  • NO_SITE as an explicit sentinel (rather than an optional) keeps the IR signature flat — no extra present? field, no extra null-check at every raise site. The query-side test query_side_existence_error_has_no_location_suffix directly confirms the contract.
  • Stdlib provenance via Span::point(u32::MAX, 0). Single source of "no provenance," reused across stdlib + future synthetic sources. The clause_without_provenance doc comment explains the design — future contributors won't add their own ad-hoc sentinel.
  • CgClause/parse_program_cg is an additive sibling of parse_program_with_directives. The LSP path stays on the cheaper non-spanned parse; only codegen pays for the conjunct-level span work. Right separation.
  • Build.rs fix replaces a real bug. The old direct_lib.exists() preference would silently embed stale runtime bytes for any non-primary plg-runtime build. The fix is local, documented, and the failure mode is now "panic if deps/ has none" rather than "silent miscompile." Belt-and-suspenders rerun-if-changed watches on ../runtime/src and ../runtime/Cargo.toml are good defense even with the (reliable) build-dep relink trigger.
  • Footprint checkpoint passes empty. The (0, 0) (srcmap_len, files_len) path is exercised — empty tables cost essentially nothing in the IR, so the binary-size ceiling stays inert for provenance-free builds.
  • The "rebuild left arm, parse rest, span-collapse" approach to top-level ; is the right tactical call for v1: it gives one less granularity bucket but keeps the parser code linear. Documented; can be granularized later without an ABI change.

Suggested order

#1 (rebuild SourceMap per call site → cache per file) and #2 (dedup srcmap) are worth doing before merge — both compound against the footprint claim and are small. #3 (build.rs heuristic limits) wants a comment, not a code change. #4 onward are observations or follow-ups.

## Review — Spans Layer 3 + build.rs runtime selection The transport choice is the right one. Side-table + `site_id` arg, suffix-on-the-message-only, ISO ball untouched: oracle bytes stay byte-identical (verified by `query_side_existence_error_has_no_location_suffix` and the runtime unit test pinning the exact v1 string). Two sentinels (`NO_SITE = u32::MAX` in both crates) keep the "no provenance" path explicit instead of relying on empty optionals; the stdlib-via-`clause_without_provenance` trick — stamp `Span::point(u32::MAX, 0)`, let `site_id` see an out-of-bounds `FileId` and return `NO_SITE` — is clever and correct. `Spanned<T>` is now actually used (closing the PR #14 observation). The build.rs fix is a real improvement over a real bug. Issues below, ordered by impact. ### Real concerns **1. `CodeGen::site_id` rebuilds the `SourceMap` on every call site.** `codegen/mod.rs`: ```rust let (line, col) = SourceMap::new(&src.text).line_col(span.lo); ``` `SourceMap::new` walks the entire source byte stream to find line breaks. With N call sites in an M-byte source, that's O(N × M) — quadratic in source size. A 5k-line program with 5k call sites already costs a few hundred million byte iterations just to populate the srcmap. Easy fix: build one `SourceMap` per `CgSource` up front (or lazily, via `OnceCell<SourceMap>` keyed by file_idx) and reuse. **2. `site_id` doesn't dedupe.** Two call sites at the same `(file_idx, line, col)` — easy to produce because nested goals inherit their enclosing conjunct's span — each append a fresh `@plg_srcmap` row. The srcmap grows with every emitted call rather than every distinct location. A `HashMap<(u32, u32, u32), u32>` cache would collapse duplicates and shrink the IR/binary footprint, especially after #1 makes srcmap entries cheap to construct. Not a correctness bug; just bloat that compounds against the "pay-for-what-you-use" footprint claim. **3. The build.rs mtime heuristic is pragmatic, not airtight.** Picking the newest `libplg_runtime-*.a` by mtime correctly handles the bug you describe (uplifted top-level copy goes stale when plg-runtime is a build-dep). Residual risks: - **Sibling crates touching `deps/`.** A workspace where another crate also depends on `plg-runtime` (or transitively recompiles it) could leave a newer `libplg_runtime-<other_hash>.a` from a different rustc invocation. mtime says "newer," but it's not the artifact this plgc build's deps-graph wanted. - **No-op rebuilds.** If `plgc` is rebuilt without changes, cargo doesn't relink the runtime, mtime stays where it was, and a later-touched unrelated `libplg_runtime-*.a` could win. Probably impossible because cargo's relink trigger always reruns the build script when this script's last-mod is older — but worth confirming. - **The principled identifier is the build-dep's metadata hash.** Cargo encodes it into the filename suffix; you could pull it from `CARGO_MANIFEST_DIR`'s `.fingerprint/plg-runtime-*` or, more cleanly, from the build-script env (when a dep has a build script of its own, `DEP_<name>_*` envvars surface; plg-runtime doesn't have one today, but adding a no-op build script just to publish a `cargo:metadata=hash=...` would give a positive identifier). The mtime approach is the right shape for landing now — it fixes the bug; the alternative is a bigger refactor or a deployment-model change (link normally instead of `include_bytes!`). Worth a short comment in the script noting which guarantee mtime provides ("cargo wrote the build-dep artifact during *this* build, so it is the newest among artifacts cargo touched") and which it doesn't ("we cannot positively identify it as the one cargo's metadata pinned for this build"). Also: `(mtime, &path) > (*t, p)` tiebreaks on lexically-larger path. Deterministic but arbitrary — worth a one-liner explaining why this direction (vs. lexically-smaller). Most build scripts I've seen tie-break on lexically-*smaller* because that's what `BTreeMap` gives you for free; either is fine, but document the choice. ### Design observations **4. PR #14 lexer-span loss is still pending.** `Tokenizer::tokenize` still returns `Result<_, String>` (`tokenizer/mod.rs:167-170` still emits `"Unexpected character 'X' at line N col M"`), so lexer errors still route through `From<String> for ParseError` and land at `Span::point(0, 0)`. Out of scope for span-3, but the SPANS.md "Status" line now reads "Layers 1–2 done" while a real failure path in Layer 1 is still broken. Worth a tracking note in the design doc or a separate small follow-up — not a blocker for this PR. **5. Top-level `;` collapses every conjunct span into one body-wide span.** `parse_body_conjuncts`'s special case is documented ("Top-level disjunction: rebuild what we parsed as the left arm..."), but the consequence is that a body like `setup, work, cleanup ; recover.` makes the existence_error suffix for an undefined call inside *any* of those goals point at the whole `setup, work, cleanup ; recover` range. Not wrong, but a user reading "Undefined procedure: cleanup/1 at file:line:col" expects col to land near `cleanup`, not at `setup`. Worth a one-line caveat in the design doc and probably in a test (an `existence_error_in_disjunctive_body_carries_coarse_span` pin so a future "let's make this granular" refactor knows what it changes). **6. `LGoal::Call` carries `span`; other LGoal variants don't.** Today only `existence_error` uses it. The fast-follow list (is/2, type_error, evaluation_error, throw/1) will need spans on whichever LGoal variants lower to those raises — `RtDet`, `Is`, `ArithCmp`, `Metacall`. The threading you'd need is the same shape as `Call`, but each new raising builtin is one more `lower_goal` call site to update. A `pub span: Span` on a wrapper struct `LGoal { kind: LGoalKind, span: Span }` would move the bookkeeping into one place rather than per-variant; consider before the next layer of raising builtins lands. ### Small observations **7. The two `NO_SITE` constants** — `plg_compiler::codegen::NO_SITE` and `plg_runtime::machine::NO_SITE` — are independent definitions that happen to agree at `u32::MAX`. The doc comments on each link them by reference; a one-line `// ABI contract — must match plg_runtime::machine::NO_SITE` and a `static_assertions::const_assert_eq!` (if you already depend on it) or even just a runtime `debug_assert_eq!(plg_compiler::NO_SITE, plg_runtime::NO_SITE)` somewhere in tests would catch a future "let's renumber" drift. Same observation for `i32` (IR) vs `u32` (Rust ABI). The two's-complement reinterpret works (`i32 4294967295` is the bit pattern for `u32::MAX`), and `query_side_existence_error_has_no_location_suffix` pins it — but the line `format!("i32 {site}", site = u32::MAX)` reads as suspicious without that context. A one-line comment at the emit site noting "i32 wire / u32 ABI; two's complement: NO_SITE = -1 here = u32::MAX in the runtime" would save a future reader some squinting. **8. `Machine::site_location` clones the filename `String` on every raise.** Documented as "Owned so the (cold) error path can mutate `self.error` without a borrow conflict." Fine for `existence_error` (cold), but as you wire `type_error`/`evaluation_error` in tighter loops, consider an `append_to_error_msg(&mut self, suffix: &str)` helper that does the format-and-append without leaving the borrow window open. Saves the per-raise allocation when the path becomes hot. **9. `parse_sources` and `parse_sources_cg` duplicate the read-file/shebang-strip loop.** Small duplication (six lines), but if a future shebang-handling change misses one, the LSP lint path and the codegen path will silently diverge. A `read_source(path) -> Result<String, String>` helper would dedupe and pin the contract. ### What's good - **"Side-table side, message-only suffix"** means the ISO ball/Formal/Context stay byte-identical to v1. `existence_message_matches_v1_bytes` and the new `existence_suffix_appears_when_site_resolves` pin both halves of the contract cleanly. - **`NO_SITE` as an explicit sentinel** (rather than an optional) keeps the IR signature flat — no extra `present?` field, no extra null-check at every raise site. The query-side test `query_side_existence_error_has_no_location_suffix` directly confirms the contract. - **Stdlib provenance via `Span::point(u32::MAX, 0)`.** Single source of "no provenance," reused across stdlib + future synthetic sources. The `clause_without_provenance` doc comment explains the design — future contributors won't add their own ad-hoc sentinel. - **`CgClause`/`parse_program_cg` is an additive sibling of `parse_program_with_directives`.** The LSP path stays on the cheaper non-spanned parse; only codegen pays for the conjunct-level span work. Right separation. - **Build.rs fix replaces a real bug.** The old `direct_lib.exists()` preference would silently embed stale runtime bytes for any non-primary plg-runtime build. The fix is local, documented, and the failure mode is now "panic if `deps/` has none" rather than "silent miscompile." Belt-and-suspenders `rerun-if-changed` watches on `../runtime/src` and `../runtime/Cargo.toml` are good defense even with the (reliable) build-dep relink trigger. - **Footprint checkpoint passes empty.** The `(0, 0)` `(srcmap_len, files_len)` path is exercised — empty tables cost essentially nothing in the IR, so the binary-size ceiling stays inert for provenance-free builds. - **The "rebuild left arm, parse rest, span-collapse" approach to top-level `;`** is the right tactical call for v1: it gives one less granularity bucket but keeps the parser code linear. Documented; can be granularized later without an ABI change. ### Suggested order #1 (rebuild SourceMap per call site → cache per file) and #2 (dedup srcmap) are worth doing before merge — both compound against the footprint claim and are small. #3 (build.rs heuristic limits) wants a comment, not a code change. #4 onward are observations or follow-ups.
⏺ Handled. Disposition of the PR #15 review:
All checks were successful
CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m7s
c734b1f53f
Before-merge fixes
  - #1 (quadratic SourceMap rebuild): CodeGen now builds one SourceMap per
  source up front (srcmaps); site_id resolves against it instead of
  re-scanning the file per call. O(N·M) → O(N·log lines).
  - #2 (no srcmap dedup): added a site_cache: HashMap<(file,line,col),
  site_id> so repeated calls at the same location (common when nested goals
  inherit a conjunct's span) share one @plg_srcmap row.

  Cheap clarity/dedup
  - #9: extracted read_source(path) — the read-file/shebang-strip logic is
  now shared by parse_sources and parse_sources_cg, so they can't drift.
  - #7: ABI-contract doc comments on both NO_SITE consts, a value-pin unit
  test in each crate (no_site_sentinel_value_is_pinned) to catch a
  one-sided renumber, and an i32 wire / u32 ABI two's-complement note at
  the emit site.
  - #3: build.rs comment now spells out what mtime does guarantee ("cargo
  just wrote this build's artifact → it's the newest it touched") and
  doesn't (positive metadata identity), plus the tiebreak direction.

  #5 (top-level ; coarse span): pinned with
  existence_error_in_disjunctive_body_carries_coarse_span (suffix reports
  the body's start column, not the call's) and documented in SPANS.md + the
  cg.rs module doc.

  Deferred to the fast-follow (recorded in SPANS.md): #6 (lift span to an
  LGoal { kind, span } wrapper when more variants raise) and #8
  (append_to_error_msg helper to avoid the per-raise filename clone once
  hot paths land) — both explicitly "do as part of the next raising-builtin
  layer."

  #4 (lexer-span loss) — no action: stale. That was fixed in PR #14 and is
  present on this branch — From<String> for ParseError is gone,
  Tokenizer::tokenize returns Result<_, ParseError>, and there's no at line
  in tokenizer source (only test assertions verifying its absence). The
  reviewer was working from a pre-#14 mental model.

  Full workspace suite green (runtime 126, integration 17, frontend 63, …),
  clippy -D warnings clean. Nothing posted to the PR.
navicore deleted branch span-3 2026-06-17 22:33:15 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
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!15
No description provided.