diff --git a/.claude/skills/ci-gates/SKILL.md b/.claude/skills/ci-gates/SKILL.md index b63847dc5f..fbef413bcb 100644 --- a/.claude/skills/ci-gates/SKILL.md +++ b/.claude/skills/ci-gates/SKILL.md @@ -5548,3 +5548,58 @@ already disagree with each other. **When a fix-then-verify cycle still fails, ask whether the record has more than one row for the thing you just fixed** before assuming the fix is wrong. + +## 150. When a column moves, add the command that names the rows + +`corpus` reported "Zig accepts it 215" where it had said 217. Two specs had +changed and nothing in the tool could say which. I nearly went hunting with a +hand-rolled harness — the same one that had already reported an implausible zero +and been distrusted. + +The fix was one flag, `--per-spec `: one sorted line per spec with the +binary outcomes behind every number, for `diff` against the same file from +another binary. Three lines differed, all three named, in one command. + +**An aggregate that can move is an aggregate that needs a per-item dump.** Build +it the first time you need it, not the third. + +## 151. Two node shapes, one emitter arm, and only one of them was read + +`gen-c` emitted `int32_t a[3] = { .v = { _ } };` — the array literal's DIMENSION +printed as its element list. Two different parses reach that arm: + + [1, 2, 3] extra_size "1,2,3", no children + [_]i32{1, 2, 3} extra_size "_" (the dimension), elements in CHILDREN + +The arm read `extra_size` unconditionally. Elements were parsed, held in the +node, and never emitted. + +**When one match arm serves two producers, check what each producer actually +filled in.** The comment above the arm described one of them and was accurate +about it, which is why it read as correct for years. + +## 152. Prove a wrapper is dead before removing it + +The same emitter wrapped every array in `{ .v = { ... } }`. Removing that changes +output for hundreds of specs, so removing it on the belief that it looked wrong +would have been a guess. The measurement took one loop: + + of the 156 specs whose generated C `cc` accepts, 0 contain `.v = {` + +Zero. The wrapper had never appeared in a piece of C this compiler produced that +a C compiler would take. **A construct present only in output that is already +rejected cannot be load-bearing** — and now the claim is a number in the commit +rather than an opinion. + +## 153. `sync` should recompute the truth, not pick the newer lie + +First draft of `tri seals sync-twins` copied the seal with the newest +`sealed_at` onto its twins. That settles a disagreement by coin flip: the newer +file is not the true one, it is the recently written one. + +Rewritten to call `t27c seal ` and write THAT to every twin. The rewrite +paid immediately — it refused 31 pairs, and every one turned out to name a spec +file that is not in the tree. My own issue had called those "31 specs where the +record says two different things"; they are 31 pairs of dangling seals about a +file nobody can fetch. **A command that recomputes finds the ones it cannot +recompute, and those are the interesting ones.** diff --git a/bootstrap/src/service.rs b/bootstrap/src/service.rs index 441a613c5f..15e7624801 100644 --- a/bootstrap/src/service.rs +++ b/bootstrap/src/service.rs @@ -780,6 +780,14 @@ pub fn run_path(_repo_root: &Path, spec: &str, to_bitstream: bool) -> anyhow::Re #[derive(Default, Clone)] struct SpecOutcome { + /// W699: how many top-level tokens the parser CONSUMED AND THREW AWAY. + /// + /// The acceptance columns below say "217 specs produce Zig that Zig takes". + /// They have never said "on how much of the spec" -- and 87 specs are + /// accepted while part of their text is discarded, the discarded part being + /// the bodies of invariants, which is to say the assertions. A reader who + /// sees the columns should see this beside them. + discarded: usize, zig_gen: bool, zig_build: bool, /// Does gen-rust produce something rustc accepts? @@ -976,6 +984,13 @@ pub fn run_corpus( } } + // ---- what the parser threw away (W699) ---- + if let Ok(src) = std::fs::read_to_string(p) { + if let Ok((_, d)) = crate::compiler::Compiler::parse_ast_accounted(&src) { + o.discarded = d; + } + } + // ---- Zig ---- if let Some((c, text)) = run_timed(Command::new(&me).args(["gen", &sp]), 15) { if text == "__TIMEOUT__" { @@ -1064,18 +1079,19 @@ pub fn run_corpus( .map(|(rel, o)| { let b = |x: bool| if x { '1' } else { '0' }; format!( - "{}\t{}{}\t{}{}\t{}{}\t{}{}", + "{}\t{}{}\t{}{}\t{}{}\t{}{}\t{}", rel, b(o.zig_gen), b(o.zig_build), b(o.rust_gen), b(o.rust_build), b(o.c_gen), b(o.c_build), b(o.v_gen), b(o.v_build), + o.discarded, ) }) .collect(); rows.sort(); let body = format!( - "# spec\tzig(gen,build)\trust\tc\tverilog\n{}\n", + "# spec\tzig(gen,build)\trust\tc\tverilog\tdropped\n{}\n", rows.join("\n") ); std::fs::write(path, body) @@ -1128,6 +1144,20 @@ pub fn run_corpus( } println!(" {:<26} {:>5} {:>6}", "Zig AND Verilog accept", both, format!("{:.1}%", pct(both))); println!(" {:<26} {:>5} {:>6}", "ALL FOUR accept", all4, format!("{:.1}%", pct(all4))); + + // W699: the columns above are all "how many specs", and none of them is + // "how much of a spec". A number that goes UP when a silent drop is fixed + // is measuring the drop; printing it here is what stops the columns from + // being read as coverage. + let disc: usize = out.iter().map(|(_, o)| o.discarded).sum(); + let disc_specs = out.iter().filter(|(_, o)| o.discarded > 0).count(); + println!(); + println!( + " {:<26} {:>5} {:>6}", + "specs with tokens DROPPED", disc_specs, format!("{:.1}%", pct(disc_specs)) + ); + println!(" {:<26} {:>5}", " ... tokens dropped", disc); + println!(" Accepted is not the same as accepted ON THE WHOLE SPEC."); if to > 0 { println!(" {:<26} {:>5}", "timed out (hang)", to); } diff --git a/docs/PARSER_DISCARD_LANDSCAPE.md b/docs/PARSER_DISCARD_LANDSCAPE.md new file mode 100644 index 0000000000..d33a5b0475 --- /dev/null +++ b/docs/PARSER_DISCARD_LANDSCAPE.md @@ -0,0 +1,107 @@ +# The discard, and how other parsers avoid needing one + +**Status:** design note. Written 2026-08-29 while closing W699 (#2754), after the +parser stopped throwing away 1 292 tokens it had been throwing away silently. + +## What t27 does today + +`parse_bdd_clauses` lowers a braceless clause body into ordinary statements. Any +shape it does not fully understand restores a checkpoint and falls back to +`skip_to_next_top_level()` — the block's tokens are **consumed and dropped**, and +the surrounding file parses as if the block were empty. + +That contract is deliberate and it is written into the function: + +> Safety contract: this may only ADD assertions, never break a file. + +It has a cost the contract does not name. A dropped `invariant` body is a dropped +set of assertions, and every downstream phase then reports success over an empty +block. `t27c parse-complete` exists to count what is dropped: + +``` +specs scanned 650 +parse and consume all 472 +parse but DISCARD 87 (32 485 token(s)) +do not parse 91 +``` + +Eighty-seven specs are accepted while part of their text is thrown away. + +## Three designs that do not have this failure mode + +### 1. Lossless concrete syntax trees — rowan (rust-analyzer), Swift, C#, Kotlin + +Rowan's stated property is that **the original source can always be perfectly +reconstructed from the parse tree, even if it has errors** — comments, +whitespace, and parse errors are all nodes in the tree. Nothing can be dropped, +because dropping something would break the reconstruction invariant. + +The relevant difference is not "better recovery". It is that *discarding is not +representable*. A `parse-complete` command cannot be needed by a parser whose +tree is a bijection with its input. + +Cost: two tree layers (green nodes holding text position-independently, a red +tree modelling the exact source structure on top), and every consumer must +tolerate error nodes rather than assuming a well-formed AST. + +### 2. Explicit ERROR and MISSING nodes — tree-sitter + +Tree-sitter keeps the unparseable region as an `ERROR` node and can also insert +zero-width `MISSING` nodes when insertion is the cheaper repair. The dropped +region is *in the tree*, addressable by a query. + +Worth reading before copying it: `tree-sitter parse` does not report an error +even when an ERROR node is present (tree-sitter/tree-sitter#4049), and MISSING +nodes are not captured by ERROR queries. **Having the representation is not the +same as reporting it** — which is this repository's own recurring lesson, arrived +at from the other direction. + +Tree-sitter's own documentation notes the recovery "costs" are opaque to an +outside observer, and that it currently errs toward skipping subtrees where +inserting would be better. + +### 3. Error productions — yacc/bison, and every LR grammar that uses them + +The grammar names the recovery points itself (`stmt: error ';'`), so what is +skipped is a decision written in the grammar rather than a runtime heuristic. +Predictable, and auditable by reading the grammar — but it must be designed in +per-construct, and it says nothing about how much was skipped at runtime. + +## Where t27 actually sits + +Closer to tree-sitter than to rowan: recovery happens, and the discarded region +is not in the AST. The difference is that t27 **counts** it (`parse-complete`) +and **ratchets** it (`parse-no-discard` is a suite phase), which is more than +tree-sitter's own CLI does today. + +The gap that remains is the same one tree-sitter#4049 describes: the count is +available to someone who runs the command, and the `parse-no-discard` phase sits +in the suite's BLOCKED column for the specs that need it most — so a spec can +lose its assertions and every gate stays green. + +## What this note is not + +It is not a proposal to rewrite the parser as a lossless CST. That is a rewrite +of every backend's assumption about its input, for a repository whose corpus is +650 specs. The cheap half of rowan's property is already available and unused: + +- the discard is counted per spec, so it can be **ratcheted down** (it is) +- a spec that discards can be made to **fail its own phase** rather than be + gated behind an upstream one +- the count can be printed **beside the acceptance columns**, where a reader who + sees "217 accepted" would also see "on 32 485 tokens fewer than were written" + +The third of those was one line, and it is written now — `t27c corpus` ends with + +``` + specs with tokens DROPPED 87 13.4% + ... tokens dropped 32485 + Accepted is not the same as accepted ON THE WHOLE SPEC. +``` + +## Sources + +- rowan, README and design notes — +- tree-sitter, ERROR/MISSING node semantics — +- `tree-sitter parse` does not report ERROR nodes — +- MISSING nodes are not matched by ERROR queries — diff --git a/docs/now/2026-08-29-accepted-is-not-the-same-as-accepted-on-the-whole-spec.md b/docs/now/2026-08-29-accepted-is-not-the-same-as-accepted-on-the-whole-spec.md new file mode 100644 index 0000000000..82c2e80728 --- /dev/null +++ b/docs/now/2026-08-29-accepted-is-not-the-same-as-accepted-on-the-whole-spec.md @@ -0,0 +1,7 @@ +# NOW -- Accepted is not the same as accepted on the whole spec (2026-08-29) + +## Accepted is not the same as accepted on the whole spec (Refs #2754) + +- corpus now ends with the discard beside the acceptance columns: 87 specs, 32485 tokens, and a line saying what that means +- --per-spec gains a dropped column, so the diff that names a moved row also shows what that row throws away +- docs/PARSER_DISCARD_LANDSCAPE.md: rowan makes discarding unrepresentable, tree-sitter represents it and still does not report it (tree-sitter#4049), bison puts it in the grammar