SWG-INF-04: replace ProgramSpans with a full SourceMap - #194
Merged
Conversation
Tests only; no `src/` change. `swang/tests/source_map_contract.rs` states the
contract against the public surface, and cannot compile:
error[E0432]: unresolved imports `griff_swang::syntax::parse_with_source_map`,
`griff_swang::syntax::AstId`, `griff_swang::syntax::FieldKind`,
`griff_swang::syntax::FieldRef`
error[E0425]: cannot find type `Parsed` in module `griff_swang::syntax`
error: could not compile `griff-swang` (test "source_map_contract")
`ProgramSpans` locates four words because four were all the expansion
frontend needed. That is a special case wearing a struct: every other value
in a program is unlocatable, and each new one means another field. What
replaces it:
source -> parse_with_source_map -> Parsed<Program> { value, source_map }
The witnesses, and what each is for:
- **the reference census** — seven nodes, eighteen fields, each slicing back
to its exact author value. `classify` destructures every level-1 AST
struct exhaustively with no `..`, so a new AST field stops this compiling
and forces the choice — field span, or located by a child's node span —
rather than letting it default to unlocatable;
- **node containment** — every field span lies inside its owner's node span,
which catches a node span built from the wrong construct;
- **optionality** — no prune and no corpus is fifteen fields, and `density`,
the pruning `seed` and `corpus` have no location at all. No phantom span
for syntax nobody wrote. The generation `seed` survives, because the
owning `AstId` is what tells the two seeds apart;
- **legal reordering** — two sources whose word order differs and whose ASTs
are equal must produce the same node and field key sets, with *different*
bytes for the words that moved. A map built from canonical field order
instead of the author's tokens gives identical spans and fails here;
- **UTF-8** — multibyte literals, every span checked in-bounds and on a
`char` boundary. A map computed in `chars` slices mid-character;
- **one parser** — `parse` and `parse_with_source_map` accept the same
programs and refuse with the same codes, messages, spans and order, over
five flawed sources. Two implementations could drift; there must be one;
- **formatter independence** — a `Program` built directly in Rust, with no
source text anywhere, still formats and reparses. If the formatter ever
required a map this would not compile;
- **frozen ownership** — the four locations §3.5 already released still
slice to the same bytes. The map now knows `bars`, `candidates` and
`strategy` too; that is editor capability, not permission to move a
diagnostic someone's tooling already parses;
- **determinism** — two walks agree and both come out in key order, which is
what a `BTreeMap` is for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vdRzztKXy8bA16tEzwgLE
A general side table in place of a four-field special case.
source -> parse_with_source_map -> Parsed<Program> { value, source_map }
`SourceMap` holds two `BTreeMap`s — `AstId -> Span` and `FieldRef -> Span` —
both private, with read-only queries and key-ordered iteration. `AstId`
carries an occurrence ordinal (always `0` at level 1, because level 1 has one
of each construct) so that level 2's repeated nodes need no new identity
model. `FieldKind` is deliberately not unique on its own: `Seed` names both
the pruning and the generation seed, and the owning `AstId` tells them apart.
A variant per (construct, word) pair would grow quadratically and say nothing
the pair does not.
Both enums are `#[non_exhaustive]`, so level 2 appends variants without
breaking a caller that matches today's set. There is no `FieldRef::Node`
variant: `node_span` already owns that relation, and two ways to ask one
question is one too many.
**The AST did not change.** No spans, no ids, no `Parsed` in `Program`,
`PatternDef` or `ExactScoreDocument`; `format` still takes `&Program` alone.
A lifter that builds a program in memory formats and reparses it with no
source text anywhere, which the contract suite proves by doing exactly that.
**One parser, not two.** `parse` is `parse_with_source_map` with the map
dropped, so acceptance and diagnostics cannot drift. `ProgramSpans` and
`parse_with_spans` are removed rather than wrapped — a compatibility shim
would have kept the four-field model alive indefinitely, which is the thing
this task exists to end.
`CompiledProgram` now carries the whole map instead of a four-word
projection, and `source_map()` exposes it. What that does **not** license is
moving a diagnostic: `flaw_to_diagnostic` and `lower_diagnostic` resolve
exactly the four locations §3.5 released — kernel, unit, tail, source — and
`released()` says so at the call site. The map now knows `bars`,
`candidates` and `strategy` too; that is editor capability, not permission
to relocate something a frontend already parses.
`released()` falls back rather than panicking. The map is total over a
well-formed program — the contract suite proves all eighteen level-1 fields
are present — so the fallback is unreachable; it exists because a location
bug should degrade to a worse message, not to a crash in someone's editor.
The two frozen level-1 span tests migrate to the new API with their
assertions intact, as characterization rather than rewrite.
Prior art, and the departure worth naming: byte offsets with no line/column
state is rustc's; the structural-identity/position split is
rust-analyzer's `AstIdMap`; a lossless CST is rowan's and is *not* adopted,
because SWG-UI-07 owns that gate. Unlike rust-analyzer, these ids are
parse-local — deterministic for an AST topology, blind to whitespace and
legal word reordering, and explicitly not stable across edits. Persistent
identity is Phase 4C's question and is not pre-decided here.
`LANGUAGE_LEVEL` stays 1. No dispatch, no level-2 parsing, no recovery, no
resource limits, no token API, no new dependency.
Verified: 1430 tests green across core, swang, pattern, cli, ui-core;
`cargo fmt --all --check`, `cargo clippy --workspace --all-targets` and
`cargo check --workspace --all-targets` clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vdRzztKXy8bA16tEzwgLE
Marks INF-04 done, records the acceptance actually met, and moves the "next" marker to INF-06 — which is a hard prerequisite for 4A-06, not a neighbour: §5.11 requires level 2's input bounds to be declared before its first accepted program. Two departures from this entry's own sketch, recorded rather than quietly implemented differently. `AstId` gets one variant per construct instead of a shared `PipelineStep`, so a field's owner is named rather than inferred. And `FieldRef` has no `Node` variant, because `SourceMap::node_span` already owns that relation. One correction in the direction of claiming less: the entry said "the four existing span tests pass through the new model". There were **two**. They are carried across as characterization, and the count is fixed rather than left flattering. The decision log gains the two records this task owes: - **what an `AstId` promises.** It is parse-local. Equal ASTs give equal node-key and field-key sets with differing spans — deterministic under whitespace and under §3.2's legal word reordering, which is the stability an editor needs to re-anchor after a reformat. It is *not* stable across AST-changing edits, and is not a patch identity, a serialized id, or a semantic-hash input. Phase 4C owns persistent identity; a side table that never addressed that problem should not look like it solved it. - **the prior art, adopted and refused.** Byte offsets with no line/column state, from `rustc`. The structural-identity/position split, from `rust-analyzer`'s `AstIdMap`. A lossless CST is refused: `rowan`'s `SyntaxNodePtr` is sound prior art, but SWG-UI-07 owns that gate and its three demonstrated needs have not been demonstrated. Level 2 is not released or frozen, Phase 4A is not closed, v2 parsing does not exist, and neither do multi-error recovery, resource limits, or persistent selector identity. Verified: 16 census witnesses still green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vdRzztKXy8bA16tEzwgLE
…ownership
Tests only. No production line changes: independent review found two gaps in
the *evidence*, and the behaviour they describe was already correct.
**Gap 1 — frozen ownership was checked on the map, not end to end.**
`the_four_released_diagnostic_locations_are_unchanged` proves the source map
holds the right kernel/unit/tail/source spans. Between that map and a
rendered diagnostic sits `flaw_to_diagnostic`, which decides *which* span
each flaw class gets, and no source-map test can see a mistake there. Swap
two arms of that match and every existing witness stays green while a unit
error starts underlining the seed path.
`swang/tests/diagnostic_ownership.rs` drives the evaluator instead: it
builds seed scores that trigger each of the four released classes, takes the
`EvalDiagnostic` actually produced, and slices the source with the span it
actually chose. A meter change gives `SWG0304` at `"seed.gp5"`; a 1/4 unit
against a 1680-tick 7/8 bar gives `SWG0301` at `1/4`; nine cells into
sixteen-slot bars under `tail reject` gives `SWG0302` at `reject`; a
seventeen-cell kernel with one onset and `bars 1` gives `SWG0306` at the
quoted literal. A fifth test asserts the four resolve to four *distinct*
words, which is what makes an arm swap visible rather than merely possible.
**Gap 2 — node spans were checked for containment, not extent.**
`SourceMap` documents a node span as the *smallest* contiguous range
covering the construct. The only structural witness asked that field spans
lie inside their node span — and `0..source.len()` contains every field it
owns while being wrong about all of them.
`every_node_span_covers_exactly_its_construct` slices all seven level-1
nodes and states what each must be.
Both gaps are confirmed rather than argued. The three mutations were run
against the suite as it stood **before** this commit and against it after:
stretch every pipeline node back to byte 0 SURVIVED -> CAUGHT
swap the unit and source diagnostic owners SURVIVED -> CAUGHT
point an incomplete final bar at the unit SURVIVED -> CAUGHT
each now caught by exactly the witness written for it. A guard that would
have passed anyway is decoration; these were not.
That takes the falsification round to eleven probes, none surviving.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vdRzztKXy8bA16tEzwgLE
…ount Docs only. **The dates were wrong by three days.** Both new decision-log entries were written `2026-08-26`; the whole INF-04 run is `2026-08-29` — RED at 11:09:01Z, closure at 11:20:42Z. In an append-only log the date is most of what a later reader has to order events by, so a three-day drift for no reason at all is worth one commit to remove. Corrected in place rather than appended to: the entries themselves are unchanged, and a correction note about a typo would be longer than the typo. **The falsification count moves from eight to eleven.** The closure said "eight mutations of the implementation, none survived", which was true of the round as run and stopped being the whole story when independent review found two evidence gaps. The three probes added since — a node span stretched back to byte 0, and two swapped arms of the evaluator's flaw-to-location match — all survived the suite as it stood at closure. The backlog now says so, because "eleven caught" reads like a stronger round than it was, and what actually happened is that three holes were found and closed. Verified: 16 census witnesses still green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vdRzztKXy8bA16tEzwgLE
Owner
Author
|
@coderabbitai full review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe parser now returns a ChangesSourceMap contract and public API
Parser source-map population
Evaluator diagnostic resolution
Source-map and diagnostic validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR replaces specialized source spans with a read-only source map for parser and diagnostic locations without changing accepted language behavior or evaluation authority. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Source as Source text
participant Parser as parse_with_source_map
participant Map as SourceMap
participant Evaluator as compile_program
participant Diagnostic as diagnostic lowering
Source->>Parser: parse level-1 program
Parser->>Map: record node and field spans
Parser-->>Evaluator: return Parsed<Program>
Evaluator->>Diagnostic: pass compiled program and SourceMap
Diagnostic->>Map: resolve FieldRef span
Map-->>Diagnostic: return diagnostic location
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 83.61% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 7 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@codex review |
✅ Action performedFull review finished. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
SWG-INF-04 only: replace the level-1
ProgramSpansfour-field special case with a general, span-free-ASTSourceMapside table.Base:
main @ 722a70f549fdd37b123bfbe6e6e7cc29520459cb.Head:
ff802fe32156f339e04e9093e0903beb02763c52.No level-2 parsing, dispatch, recovery, resource limits, token API, persistent selector identity, new dependency, or canonical-score-model change.
LANGUAGE_LEVELremains 1.Commit evidence
fec2a17parse_with_source_map,AstId,FieldKind,FieldRef, andParsed2dbce7aSourceMapreplacesProgramSpans; one parser; evaluator migrates to the full mapca91210efaa80eff802feThe first three commits were not rewritten by review.
Shape
SourceMapowns privateBTreeMap<AstId, Span>andBTreeMap<FieldRef, Span>tables with read-only queries and deterministic iteration.Two deliberate departures from the backlog sketch are recorded in-tree:
AstIdvariant per construct instead of a sharedPipelineStep, so field ownership is named rather than inferred;FieldRef::Node, becauseSourceMap::node_spanalready owns that relation.FieldKind::Seedis shared by pruning and generation and disambiguated by the owningAstId.AstIdandFieldKindare#[non_exhaustive].AstIdis explicitly parse-local structural identity. It is stable for equal AST topology across whitespace/legal word reordering, but is not persistent across AST-changing edits, not serialized, not a patch identity, and not a semantic-hash input. Phase 4C still owns persistent selector identity.Acceptance evidence
The level-1 reference maps 7 nodes and 18 fields; without pruning/corpus it maps 15 fields. The contract suite pins:
..destructuring;parseandparse_with_source_mapsharing acceptance and diagnostics;ProgramSpansandparse_with_spansare removed rather than wrapped.Falsification
Initial implementation round: 8 probes, 0 survivors.
Independent review found two evidence gaps. Three additional mutations were first run against the suite exactly as it stood at closure and survived:
flaw_to_diagnostic;IncompleteFinalBaratunitinstead oftailinlower_diagnostic.efaa80eadds witnesses for those gaps. The same three mutations are then caught. Final accounting: 11 probes, 0 surviving after the review strengthening, without pretending the three review probes had been caught by the original suite.No production line changed after
2dbce7a.Validation
Locally verified on the head:
cargo fmt --all --checkclean;cargo clippy --workspace --all-targetsclean;cargo check --workspace --all-targetsclean, including cockpit/preview/plugin.cargo test --workspacecannot link in the current development container becauserust-lldcannot find-lasound. The same command fails identically in a clean worktree at the base commit722a70f; this is a pre-existing environment limitation, not introduced by this branch. CI is the acceptance source for the full workspace test run and fuzz matrix.State after merge
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests