⏺ 396 tests pass, clippy clean. Done. #15
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "span-3"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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:
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.
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.
Review — Spans Layer 3 + build.rs runtime selection
The transport choice is the right one. Side-table +
site_idarg, suffix-on-the-message-only, ISO ball untouched: oracle bytes stay byte-identical (verified byquery_side_existence_error_has_no_location_suffixand the runtime unit test pinning the exact v1 string). Two sentinels (NO_SITE = u32::MAXin both crates) keep the "no provenance" path explicit instead of relying on empty optionals; the stdlib-via-clause_without_provenancetrick — stampSpan::point(u32::MAX, 0), letsite_idsee an out-of-boundsFileIdand returnNO_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_idrebuilds theSourceMapon every call site.codegen/mod.rs:SourceMap::newwalks 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 oneSourceMapperCgSourceup front (or lazily, viaOnceCell<SourceMap>keyed by file_idx) and reuse.2.
site_iddoesn'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_srcmaprow. The srcmap grows with every emitted call rather than every distinct location. AHashMap<(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-*.aby mtime correctly handles the bug you describe (uplifted top-level copy goes stale when plg-runtime is a build-dep). Residual risks:deps/. A workspace where another crate also depends onplg-runtime(or transitively recompiles it) could leave a newerlibplg_runtime-<other_hash>.afrom a different rustc invocation. mtime says "newer," but it's not the artifact this plgc build's deps-graph wanted.plgcis rebuilt without changes, cargo doesn't relink the runtime, mtime stays where it was, and a later-touched unrelatedlibplg_runtime-*.acould win. Probably impossible because cargo's relink trigger always reruns the build script when this script's last-mod is older — but worth confirming.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 acargo: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 whatBTreeMapgives you for free; either is fine, but document the choice.Design observations
4. PR #14 lexer-span loss is still pending.
Tokenizer::tokenizestill returnsResult<_, String>(tokenizer/mod.rs:167-170still emits"Unexpected character 'X' at line N col M"), so lexer errors still route throughFrom<String> for ParseErrorand land atSpan::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 likesetup, work, cleanup ; recover.makes the existence_error suffix for an undefined call inside any of those goals point at the wholesetup, work, cleanup ; recoverrange. Not wrong, but a user reading "Undefined procedure: cleanup/1 at file:line:col" expects col to land nearcleanup, not atsetup. Worth a one-line caveat in the design doc and probably in a test (anexistence_error_in_disjunctive_body_carries_coarse_spanpin so a future "let's make this granular" refactor knows what it changes).6.
LGoal::Callcarriesspan; other LGoal variants don't. Today onlyexistence_erroruses 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 asCall, but each new raising builtin is one morelower_goalcall site to update. Apub span: Spanon a wrapper structLGoal { 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_SITEconstants —plg_compiler::codegen::NO_SITEandplg_runtime::machine::NO_SITE— are independent definitions that happen to agree atu32::MAX. The doc comments on each link them by reference; a one-line// ABI contract — must match plg_runtime::machine::NO_SITEand astatic_assertions::const_assert_eq!(if you already depend on it) or even just a runtimedebug_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) vsu32(Rust ABI). The two's-complement reinterpret works (i32 4294967295is the bit pattern foru32::MAX), andquery_side_existence_error_has_no_location_suffixpins it — but the lineformat!("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_locationclones the filenameStringon every raise. Documented as "Owned so the (cold) error path can mutateself.errorwithout a borrow conflict." Fine forexistence_error(cold), but as you wiretype_error/evaluation_errorin tighter loops, consider anappend_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_sourcesandparse_sources_cgduplicate 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. Aread_source(path) -> Result<String, String>helper would dedupe and pin the contract.What's good
existence_message_matches_v1_bytesand the newexistence_suffix_appears_when_site_resolvespin both halves of the contract cleanly.NO_SITEas an explicit sentinel (rather than an optional) keeps the IR signature flat — no extrapresent?field, no extra null-check at every raise site. The query-side testquery_side_existence_error_has_no_location_suffixdirectly confirms the contract.Span::point(u32::MAX, 0). Single source of "no provenance," reused across stdlib + future synthetic sources. Theclause_without_provenancedoc comment explains the design — future contributors won't add their own ad-hoc sentinel.CgClause/parse_program_cgis an additive sibling ofparse_program_with_directives. The LSP path stays on the cheaper non-spanned parse; only codegen pays for the conjunct-level span work. Right separation.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 ifdeps/has none" rather than "silent miscompile." Belt-and-suspendersrerun-if-changedwatches on../runtime/srcand../runtime/Cargo.tomlare good defense even with the (reliable) build-dep relink trigger.(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.;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.