span-1-and-2 #14

Merged

navicore merged 3 commits from span-1-and-2 into main 2026-06-17 21:04:48 +00:00

Conversation 1 Commits 3 Files changed 22 +566 -326

navicore commented 2026-06-17 17:44:32 +00:00

Owner

Copy link

No description provided.

navicore added 2 commits 2026-06-17 17:44:32 +00:00

⏺ Layers 1 and 2 are complete, single-buffer, and fully green. Here's where ... ca2c7d5439

things landed:

  What shipped

  Layer 1 — frontend carries spans
  - plg-shared: Span { file, lo, hi }, Spanned<T>, FileId — value types,
  never embedded in Term (so its PartialEq is untouched).
  - Tokenizer: every Token now carries byte offsets (lo/hi), stamped at one
  point in next_token.
  - ParseError { message, span } replaces the Result<_, String> parser API;
  the … at line N col M trailer is gone. All ~20 parser methods + every
  error site flipped; SourceMap (byte offset → line/col, and UTF-16 for
  LSP) resolves position at format time.
  - Parser records atom-functor occurrences (CallSite { functor, arity,
  span }), exposed via parse_program_with_spans.

  Layer 2 — consumers use spans directly
  - LSP: parse_at_line_col and call_site_ranges both deleted. Parse errors
  map span → Range; undefined-call warnings filter the parser's recorded
  occurrences by the lint's (name, arity) set — so squiggles land on real
  calls and a name in a comment is invisible (it's never parsed).
  - plgc: format_parse_error resolves file:line:col from the span +
  SourceMap (scraper extract_line_col deleted).
  - repl: renders line/col from the span.

  Tests (all 23 binaries green, clippy -D warnings clean): updated the
  parser/LSP assertions off the old string; added span-byte-range pins
  (unexpected token, EOF) and the checkpoint-2 comment-vs-call test.

  Two scope notes, as agreed

  - Option A held: plgc and the repl took only mechanical error-rendering
  edits; codegen/runtime untouched.
  - For the lint squiggle you chose the occurrence-list approach over a
  fully spanned AST. One known over-approximation, logged in SPANS.md: a
  data term sharing a functor/arity with an undefined-called predicate
  would also squiggle (rare; the doc's checkpoint doesn't exercise it).

  SPANS.md now reads "Layers 1–2 implemented; Layer 3 pending" — Layer 3
  (runtime error provenance: the @plg_srcmap side-table + site_id ABI) is
  the remaining piece and stays out of this session's scope.

ci 7cd4abffcc

navicore commented 2026-06-17 20:29:41 +00:00

Author

Owner

Copy link

Review — Spans, Layers 1 & 2

The shape of this is right. The "Span never in Term" decision is the load-bearing one — Term's PartialEq stays untouched, the AST stays the same value type the codegen and tests already work against, and spans annotate from the outside via Spanned<T> / CallSite. The SourceMap resolved on demand from a &str the consumer already holds keeps positions off the parser's return signatures. Single-point byte-offset stamping in Tokenizer::next_token keeps the per-kind helpers ignorant of offsets. These are all the calls I'd have wanted made.

Issues below, ordered by impact.

Real bugs

1. Lexer-level errors lose their span. Tokenizer::tokenize still returns Result<Vec<Token>, String>, and every parser entry point does let tokens = Tokenizer::tokenize(input)? — the ? runs From<String> for ParseError, which discards position and stamps Span::point(0, 0). Consequences:

• LSP: parse_error_to_diagnostic underlines bytes 0..0 (start of file) for any lexer error — invalid character, unterminated quote — instead of the actual offending byte.
• plgc: format_parse_error prints file.pl:1:1: Unexpected character 'X' at line N col M — the file-prefix line/col is wrong, AND the old trailer (preserved in the message) duplicates the position from the real location. Two positions, one wrong.

The fix wants Tokenizer::tokenize to return Result<_, ParseError> directly, with the lexer building spans at the offending byte (it has pos/line/col in scope at every error site — tokenizer/mod.rs:167-170). The From<String> impl on ParseError then becomes the temporary scaffold to delete once that flips. As-is, the SPANS.md claim that the "... at line N col M" trailer is gone is true only for parser errors — the tokenizer still emits it and the parser propagates it verbatim.

2. Tokenizer still emits the old trailer. tokenizer/mod.rs:167-170:

Err(format!(
    "Unexpected character '{}' at line {} col {}",
    ch as char, line, col
))

This is the source of the duplication in #1. The grep for "at line" in error strings should be empty after this PR — it isn't.

Design observations

3. Spanned<T> is defined, exported, and unused. git grep Spanned returns four hits: the definition, its doc comment, the re-export, and a doc reference. Per the commit's scope note ("occurrence-list approach over a fully spanned AST"), this is anticipatory infrastructure for Layer 3 — but as it stands, a future contributor will see a pub type with no consumer and ask the same question. Either:

• inline-doc that Spanned<T> is reserved for the Layer 3 runtime-provenance path (link the design doc), or
• drop it from this PR and add it back when something actually uses it.

I lean toward the first — the doc on the type already explains the design rationale; one extra line "no in-tree consumer yet — see docs/design/SPANS.md Layer 3" would prevent the confusion.

4. CallSite over-approximation is broader than the SPANS.md note suggests. The doc note says "a data term sharing a functor/arity with an undefined-called predicate would also squiggle." True. But the parser records every atom-functor occurrence in parse_primary — including atoms used as constants (X = foo), atoms inside data terms (assertz(p(foo, bar))), atoms in operator specs (dynamic(foo/1) records dynamic, foo, and /), even [] recorded as []/0. The lint filter (must match a (name, arity) reported as undefined) keeps the false-positive surface small in practice, but the over-approximation is wider than the doc reads. Worth a one-line widening of the note, or a record_call_site callsite annotation noting which paths intentionally over-record vs. which could be filtered (e.g. don't record atoms in dynamic specs).

Small observations

5. SourceMap::line_col counts characters, utf16_position counts UTF-16 units. Both are correct for their consumers (humans / LSP). Worth a one-line doc note on line_col that it's char-based — emacs/CI tooling that expects byte columns will mismatch on emoji/multibyte source. The split (human display vs LSP wire format) is right; the discoverability isn't.

6. LSP undefined-call loop is O(callees × call_sites). diagnostics.rs:60-75 — for each undefined callee, scan all recorded call_sites. Editor-buffer sized; fine. A HashMap<(AtomId, usize), Vec<usize>> index over call_sites would flatten it to O(N + K). Not a near-term concern; flag for when a 5k-line buffer makes this visible.

7. From<String> for ParseError is a temporary bridge. Right shape for landing without rewriting the tokenizer in the same PR — but it's the mechanism by which #1 happens. Once the tokenizer returns ParseError directly, this impl should go too. The doc comment ("Lexer-level errors surface as plain Strings (rare)") undersells it: every parse path that uses the tokenizer hits this. "Rare" is misleading — the path is hit every lexer error.

What's good

• "Span never in Term" keeps PartialEq stable and avoids codegen ripple. The file header makes the decision explicit; future contributors will see the constraint.
• SourceMap is built on demand at format time from the source string a consumer already has. No positions threaded through return types; the parser API stays small.
• Single-point byte-offset stamping in Tokenizer::next_token. Per-kind helpers don't have to track offsets. The construction site is one place to break it; no scattered drift.
• Separate line_col and utf16_position. LSP needs UTF-16 code units; humans need char columns. The emoji test (utf16_columns_count_code_units) pins the case most LSPs get wrong.
• Display for ParseError only renders the message. Position is the caller's concern via SourceMap. Discipline pays off: format_parse_error in plgc collapses to two lines.
• Comment-vs-call test pins the checkpoint-2 contract directly. comment_mention_does_not_squiggle_only_the_real_call is exactly the test the design promises, and it asserts both severity and the precise character range.
• Byte-range pins for spans (parse_error_span_covers_unexpected_token, parse_error_span_points_at_end_of_input) catch off-by-one regressions cheaply.
• Lint filter on (name, arity) is the right narrowing — call_sites by themselves are too broad, but combined with the lint they squiggle the right cells.
• Layer 3 explicitly deferred. Codegen/runtime untouched, oracle bytes preserved, PR stays reviewable. The commit message's "Option A held" is the right scope discipline.
• Honest over-approximation note in SPANS.md. Better than a "perfect lint" claim that would have been wrong.

Suggested order

#1 + #2 (lexer span loss + leftover trailer) are the only items I'd want fixed before merge — together they undermine the SPANS.md claim that line/col is fully positional. Both are local fixes; the rest are observations and follow-up surface.

## Review — Spans, Layers 1 & 2 The shape of this is right. The "**`Span` never in `Term`**" decision is the load-bearing one — `Term`'s `PartialEq` stays untouched, the AST stays the same value type the codegen and tests already work against, and spans annotate from the outside via `Spanned<T>` / `CallSite`. The `SourceMap` resolved on demand from a `&str` the consumer already holds keeps positions off the parser's return signatures. Single-point byte-offset stamping in `Tokenizer::next_token` keeps the per-kind helpers ignorant of offsets. These are all the calls I'd have wanted made. Issues below, ordered by impact. ### Real bugs **1. Lexer-level errors lose their span.** `Tokenizer::tokenize` still returns `Result<Vec<Token>, String>`, and every parser entry point does `let tokens = Tokenizer::tokenize(input)?` — the `?` runs `From<String> for ParseError`, which discards position and stamps `Span::point(0, 0)`. Consequences: - LSP: `parse_error_to_diagnostic` underlines bytes 0..0 (start of file) for any lexer error — invalid character, unterminated quote — instead of the actual offending byte. - `plgc`: `format_parse_error` prints `file.pl:1:1: Unexpected character 'X' at line N col M` — the file-prefix line/col is wrong, AND the old trailer (preserved in the message) duplicates the position from the *real* location. Two positions, one wrong. The fix wants `Tokenizer::tokenize` to return `Result<_, ParseError>` directly, with the lexer building spans at the offending byte (it has `pos`/`line`/`col` in scope at every error site — `tokenizer/mod.rs:167-170`). The `From<String>` impl on `ParseError` then becomes the temporary scaffold to delete once that flips. As-is, the SPANS.md claim that the "`... at line N col M`" trailer is gone is true only for parser errors — the tokenizer still emits it and the parser propagates it verbatim. **2. Tokenizer still emits the old trailer.** `tokenizer/mod.rs:167-170`: ```rust Err(format!( "Unexpected character '{}' at line {} col {}", ch as char, line, col )) ``` This is the source of the duplication in #1. The grep for `"at line"` in error strings should be empty after this PR — it isn't. ### Design observations **3. `Spanned<T>` is defined, exported, and unused.** `git grep Spanned` returns four hits: the definition, its doc comment, the re-export, and a doc reference. Per the commit's scope note ("occurrence-list approach over a fully spanned AST"), this is anticipatory infrastructure for Layer 3 — but as it stands, a future contributor will see a pub type with no consumer and ask the same question. Either: - inline-doc that `Spanned<T>` is reserved for the Layer 3 runtime-provenance path (link the design doc), or - drop it from this PR and add it back when something actually uses it. I lean toward the first — the doc on the type already explains the design rationale; one extra line "no in-tree consumer yet — see `docs/design/SPANS.md` Layer 3" would prevent the confusion. **4. `CallSite` over-approximation is broader than the SPANS.md note suggests.** The doc note says "a data term sharing a functor/arity with an undefined-called predicate would also squiggle." True. But the parser records *every* atom-functor occurrence in `parse_primary` — including atoms used as constants (`X = foo`), atoms inside data terms (`assertz(p(foo, bar))`), atoms in operator specs (`dynamic(foo/1)` records `dynamic`, `foo`, and `/`), even `[]` recorded as `[]/0`. The lint filter (must match a `(name, arity)` reported as undefined) keeps the false-positive surface small in practice, but the over-approximation is wider than the doc reads. Worth a one-line widening of the note, or a `record_call_site` callsite annotation noting which paths intentionally over-record vs. which could be filtered (e.g. don't record atoms in `dynamic` specs). ### Small observations **5. `SourceMap::line_col` counts *characters*, `utf16_position` counts UTF-16 units.** Both are correct for their consumers (humans / LSP). Worth a one-line doc note on `line_col` that it's char-based — emacs/CI tooling that expects byte columns will mismatch on emoji/multibyte source. The split (human display vs LSP wire format) is right; the discoverability isn't. **6. LSP undefined-call loop is O(callees × call_sites).** `diagnostics.rs:60-75` — for each undefined callee, scan all recorded call_sites. Editor-buffer sized; fine. A `HashMap<(AtomId, usize), Vec<usize>>` index over call_sites would flatten it to O(N + K). Not a near-term concern; flag for when a 5k-line buffer makes this visible. **7. `From<String> for ParseError` is a temporary bridge.** Right shape for landing without rewriting the tokenizer in the same PR — but it's the mechanism by which #1 happens. Once the tokenizer returns `ParseError` directly, this impl should go too. The doc comment ("Lexer-level errors surface as plain `String`s (rare)") undersells it: every parse path that uses the tokenizer hits this. "Rare" is misleading — the path is hit *every* lexer error. ### What's good - **"`Span` never in `Term`"** keeps `PartialEq` stable and avoids codegen ripple. The file header makes the decision explicit; future contributors will see the constraint. - **`SourceMap` is built on demand at format time** from the source string a consumer already has. No positions threaded through return types; the parser API stays small. - **Single-point byte-offset stamping in `Tokenizer::next_token`.** Per-kind helpers don't have to track offsets. The construction site is one place to break it; no scattered drift. - **Separate `line_col` and `utf16_position`.** LSP needs UTF-16 code units; humans need char columns. The emoji test (`utf16_columns_count_code_units`) pins the case most LSPs get wrong. - **`Display for ParseError` only renders the message.** Position is the caller's concern via `SourceMap`. Discipline pays off: `format_parse_error` in `plgc` collapses to two lines. - **Comment-vs-call test pins the checkpoint-2 contract directly.** `comment_mention_does_not_squiggle_only_the_real_call` is exactly the test the design promises, and it asserts both severity *and* the precise character range. - **Byte-range pins for spans** (`parse_error_span_covers_unexpected_token`, `parse_error_span_points_at_end_of_input`) catch off-by-one regressions cheaply. - **Lint filter on `(name, arity)`** is the right narrowing — call_sites by themselves are too broad, but combined with the lint they squiggle the right cells. - **Layer 3 explicitly deferred.** Codegen/runtime untouched, oracle bytes preserved, PR stays reviewable. The commit message's "Option A held" is the right scope discipline. - **Honest over-approximation note in SPANS.md.** Better than a "perfect lint" claim that would have been wrong. ### Suggested order #1 + #2 (lexer span loss + leftover trailer) are the only items I'd want fixed before merge — together they undermine the SPANS.md claim that line/col is fully positional. Both are local fixes; the rest are observations and follow-up surface.

navicore added 1 commit 2026-06-17 21:03:12 +00:00

⏺ All 23 test binaries pass, clippy clean. Here's how I handled the review. ... e861079cad

Must-fix items (both done)

  #1 lexer errors lose their span + #2 leftover trailer — the root cause
  was my shortcut of keeping Tokenizer::tokenize on Result<_, String> and
  wrapping coarsely. Fixed properly:
  - tokenize/next_token/read_*/sym_* now return Result<_, ParseError> with
  real spans built from self.pos (the offending byte) — a precise char span
  for the unexpected-character case, a point span where the scanner stalls
  (EOF/unterminated, bad operator).
  - All "... at line N col M" strings are gone from the tokenizer. A grep
  for at line/: expected in non-test source now returns only doc comments.
  - Added two regression tests: lexer_error_carries_span_not_a_trailer
  (pins byte range, asserts no trailer) and
  unterminated_quote_points_at_end_of_input.

  #7 the From<String> bridge — deleted, since the parser entry points now ?
  directly on a ParseError-returning tokenizer.

  Observations folded in

  - #3 Spanned<T>: doc note that it has no in-tree consumer yet and is
  reserved for the Layer 3 provenance path (links SPANS.md).
  - #4 CallSite breadth: widened both the type's doc-comment and the
  SPANS.md note to spell out that it over-records every atom/compound
  (constants, data terms, operator/directive functors, []/0), narrowed by
  the lint intersection.
  - #5 line_col: doc note that it's character-based (byte-column tooling
  mismatches on multibyte/emoji); LSP uses utf16_position.
  - #6 O(callees × call_sites): left as-is per your "not near-term" call,
  with a one-line comment pointing at the (functor, arity) index fix for
  when a large buffer makes it visible.

  One regression I caught from the fix

  Removing the trailer broke plgc's exit-code heuristic in main.rs:185,
  which keyed on the "at line" substring to map program parse errors to
  exit 2. Parse errors like unexpected ] would have silently dropped to
  exit 3. Replaced it with is_parse_error, which detects the path:line:col:
  shape format_parse_error produces (and which non-parse failures lack).

navicore merged commit d68c7ada9a into main 2026-06-17 21:04:48 +00:00

navicore deleted branch span-1-and-2 2026-06-17 21:04:49 +00:00

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

Merge pull request 'span-1-and-2' (#14) from span-1-and-2 into main

navicore referenced this pull request 2026-06-17 22:13:06 +00:00

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

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

⏺ Handled. Disposition of the PR #15 review:

navicore referenced this pull request 2026-06-18 00:18:03 +00:00

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

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

No description provided.