fix(formula): every Rule dispatch in the walker is exhaustive

Integration review, cross-branch against #108. My asymmetry argument was right
-- a &str dispatch can only have GRAMMAR_INVARIANT, a Rule dispatch can have a
compile error -- and I then applied it to the two &str sites and NOT to the
Rule site where the stronger technique was available. build_primary kept its
unlabelled `else`.

Demonstrated, not argued. With `pct_literal = { "#" ~ identifier }` first in
`primary`: the tree built clean, all 85 crate tests passed including my three
new 492x guards, and parse_formula("X = #Revenue") returned Ref("Revenue") --
the `#` discarded, the formula silently wrong. The injection must be
NON-ATOMIC: an atomic rule is caught by accident in first_inner's own
GRAMMAR_INVARIANT and would have reported the code safe.

Sweeping for the shape found three more in the same file, two genuinely silent:
build_formula's if-chain-in-a-loop dropped an unhandled part (its comment said
"Rule::EOI and any silent rules are ignored"; EOI is now an ARM, so ignoring is
a decision), filter_value_to_string returned unprocessed text, and build_unary
was caught only incidentally downstream. All four now match over Rule with an
or-pattern over the rules that cannot appear, so ANY new rule in formula.pest
is a build failure at all four until someone decides what it means. Verified by
injecting three more probe rules: every site names all three.

The greppable invariant: `== Rule::` now appears nowhere in parser.rs.

each_primary_alternative_builds_its_own_expr is the anti-clamp guard -- a
mechanical if/else-to-match conversion can transpose two arms and still
compile, since the compiler checks that every rule is NAMED, never that it is
named against the right body. Proved by swapping the string and pipe_quoted
arms: it fails and nothing else in the suite does.

Also corrects context/code/formula-and-io.md, which still called the final
`else` arms "unlabelled-but-safe" on the grounds that COUNT was the only
remaining agg_func and >= the only remaining cmp_op. Two of those three clauses
were made false by my own earlier commit. Each was true OF THE GRAMMAR AS IT
STOOD -- a snapshot silently re-read as an invariant, which is the whole defect
in one sentence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-21 15:47:09 -07:00
parent e3e89907ea
commit 4e08a9b485
3 changed files with 244 additions and 39 deletions
+45 -4
View File
@@ -160,10 +160,51 @@ Every `build_*` in `parser.rs` operates on a `Pair` pest has already
validated. "A `formula` has exactly one `target`", "a `comparison` has lhs, op
and rhs" — pest guarantees these before the walker sees the pair. So every
`.expect(GRAMMAR_INVARIANT)` is a **bug marker**, not a runtime condition: if
one fires, the grammar and the walker have diverged. The same reasoning makes
the final `else` arm of each dispatch unlabelled-but-safe (`ref_expr` is the
only remaining alternative of `primary`, `COUNT` the only remaining `agg_func`,
`>=` the only remaining `cmp_op`).
one fires, the grammar and the walker have diverged.
**That reasoning used to be extended to the final `else` arm of each dispatch,
and the extension was wrong.** The sentence here said the unlabelled arm was
safe because `ref_expr` is the only remaining alternative of `primary`, `COUNT`
the only remaining `agg_func` and `>=` the only remaining `cmp_op`. Each clause
was true *of the grammar as it stood*, which is exactly the property a grammar
edit changes -- so what the arm really encoded was a snapshot, silently
re-interpreted as an invariant. "pest validated the pair" says the pair matches
**some** alternative; it never said which, and an unlabelled arm reads the
answer off a count the next commit is free to change.
Two different repairs, and **which one is available is a property of the
input**:
- **A `&str`, where no compile error exists.** `parse_agg_func` and
`parse_cmp_op` take the matched text, because `agg_func` and `cmp_op` are
each *one* pest rule over string alternatives and `pest_derive` generates no
per-alternative `Rule`. Nothing for a `match` to be exhaustive over, so they
spell every alternative and `unreachable!(GRAMMAR_INVARIANT)` on anything
else -- loud at the first parse rather than silent forever (improvise-492x).
- **A `Rule`, where one does.** The four dispatches on `Pair::as_rule` --
`build_formula`, `build_unary`, `build_primary`, `filter_value_to_string` --
match over an enum, so they name every rule, including a final or-pattern
over the ones that cannot appear. Adding **any** rule to `formula.pest` is
now a build failure at all four until someone decides what it means.
The greppable form: **`== Rule::` appears nowhere in `parser.rs`.** An
`if rule == Rule::x` chain cannot be exhaustive, and it is how the snapshot got
written down as an invariant in the first place.
The cost is four or-patterns of ~28 enum names, which is genuinely noisy, and
that is the price of the guarantee -- an invariant made unrepresentable rather
than validated, applied to a grammar instead of to a type. The noise is also
the tell that a walker has grown a dispatch, which is worth noticing.
`build_primary` is where this was demonstrated rather than argued, and **the
injection has to be chosen carefully**: a new alternative that is an *atomic*
rule is caught by accident, because `first_inner` then finds no child and
panics on its own `GRAMMAR_INVARIANT`. Only a **non-atomic** alternative
reaches the silent path. With `pct_literal = { "#" ~ identifier }` placed first
in `primary`, the pre-fix tree built clean, the whole crate suite passed, and
`parse_formula("X = #Revenue")` returned `Ref("Revenue")` -- the `#` discarded
and the formula quietly wrong. An injection into an atomic rule would have
reported the code safe when it was not.
`parse.rs`'s walkers are the opposite and deliberately so: they return
`Result` and name the section in the error, because the `.improv` file is