From 4e08a9b485ef0ad84e727d2a216e4aac3dca2337 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 15:47:09 -0700 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/formula-and-io.md | 49 +++++- context/test-evidence/formula-and-io.md | 19 +++ crates/improvise-formula/src/parser.rs | 215 ++++++++++++++++++++---- 3 files changed, 244 insertions(+), 39 deletions(-) diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index dd2a0d12..d72be6a9 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -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 diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 30c3b2e9..ce1244de 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -15,6 +15,25 @@ helper under `###` would pass today and fail the moment that is tightened. ## `improvise-formula/src/parser.rs` +### each_primary_alternative_builds_its_own_expr + +The anti-clamp companion to making the four `Rule` dispatches in `parser.rs` +exhaustive. Converting an `if rule == Rule::x { .. } else if .. { .. } else +{ .. }` chain into a `match` is mechanical, and a mechanical conversion can +**transpose two arms and still compile** — the compiler checks that every rule +is named, never that it is named against the right body. This test walks each +`primary` alternative (`number`, `agg_call`, `if_expr`, `paren_expr`, `is_expr`, +`ref_expr`), plus `unary_minus`, and the three `filter_value` alternatives that +share the other dispatch, and asserts each reaches its own `Expr` or its own +unquoting. Proved discriminating by swapping the `string` and `pipe_quoted` +arms: the test fails, and nothing else in the 85-test crate suite does. + +Why the exhaustiveness is worth its noise, and how the defect was demonstrated, +are in `context/code/formula-and-io.md`, "The tree walkers are infallible by +construction" — including the injection trap, that an *atomic* new rule is +caught by accident in `first_inner` and only a **non-atomic** one reaches the +silent path. + ### a_comparison_the_walker_does_not_know_is_not_ge The `cmp_op` half of improvise-492x. `parse_cmp_op` ended `_ => BinOp::Ge`, so diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index f5aced6b..d4256ad1 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -43,17 +43,45 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula let mut expr = None; let mut filter = None; for inner in pair.into_inner() { - let rule = inner.as_rule(); - if rule == Rule::target { + match inner.as_rule() { // Unquoted exactly like `Expr::Ref`: the same spelling must name // the same item, or the output lands in an unreferencable column. - target = Some(identifier_to_string(first_inner(inner))); - } else if rule == Rule::expr { - expr = Some(build_expr(inner)); - } else if rule == Rule::where_clause { - filter = Some(build_filter(inner)); + Rule::target => target = Some(identifier_to_string(first_inner(inner))), + Rule::expr => expr = Some(build_expr(inner)), + Rule::where_clause => filter = Some(build_filter(inner)), + // Ignored as an ARM, so "ignored" is a decision and not a silence. + Rule::EOI => {} + Rule::WHITESPACE + | Rule::formula + | Rule::expr_eoi + | Rule::add_expr + | Rule::add_op + | Rule::mul_expr + | Rule::mul_op + | Rule::pow_expr + | Rule::pow_op + | Rule::unary + | Rule::unary_minus + | Rule::primary + | Rule::is_expr + | Rule::sp + | Rule::paren_expr + | Rule::agg_call + | Rule::agg_func + | Rule::inline_where + | Rule::if_expr + | Rule::comparison + | Rule::cmp_op + | Rule::ref_expr + | Rule::identifier + | Rule::pipe_quoted + | Rule::bare_ident + | Rule::filter_value + | Rule::string + | Rule::number => { + unreachable!("{GRAMMAR_INVARIANT}: formula part {:?}", inner.as_rule()) + } } - // Rule::EOI and any silent rules are ignored. } Formula::new( raw, @@ -107,32 +135,91 @@ fn build_pow_expr(pair: Pair) -> Expr { fn build_unary(pair: Pair) -> Expr { // unary = { unary_minus | primary } let inner = first_inner(pair); - if inner.as_rule() == Rule::unary_minus { - Expr::UnaryMinus(Box::new(build_primary(first_inner(inner)))) - } else { - // primary is the only other alternative. - build_primary(inner) + match inner.as_rule() { + Rule::unary_minus => Expr::UnaryMinus(Box::new(build_primary(first_inner(inner)))), + Rule::primary => build_primary(inner), + Rule::EOI + | Rule::WHITESPACE + | Rule::formula + | Rule::target + | Rule::where_clause + | Rule::expr_eoi + | Rule::expr + | Rule::add_expr + | Rule::add_op + | Rule::mul_expr + | Rule::mul_op + | Rule::pow_expr + | Rule::pow_op + | Rule::unary + | Rule::is_expr + | Rule::sp + | Rule::paren_expr + | Rule::agg_call + | Rule::agg_func + | Rule::inline_where + | Rule::if_expr + | Rule::comparison + | Rule::cmp_op + | Rule::ref_expr + | Rule::identifier + | Rule::pipe_quoted + | Rule::bare_ident + | Rule::filter_value + | Rule::string + | Rule::number => { + unreachable!( + "{GRAMMAR_INVARIANT}: unary alternative {:?}", + inner.as_rule() + ) + } } } fn build_primary(pair: Pair) -> Expr { // primary = { number | agg_call | if_expr | paren_expr | is_expr | ref_expr } let inner = first_inner(pair); - let rule = inner.as_rule(); - if rule == Rule::number { - Expr::Number(inner.as_str().parse().expect(GRAMMAR_INVARIANT)) - } else if rule == Rule::agg_call { - build_agg_call(inner) - } else if rule == Rule::if_expr { - build_if_expr(inner) - } else if rule == Rule::paren_expr { - build_expr(first_inner(inner)) - } else if rule == Rule::is_expr { + match inner.as_rule() { + Rule::number => Expr::Number(inner.as_str().parse().expect(GRAMMAR_INVARIANT)), + Rule::agg_call => build_agg_call(inner), + Rule::if_expr => build_if_expr(inner), + Rule::paren_expr => build_expr(first_inner(inner)), // Same shape as a WHERE clause minus the keyword: identifier ~ filter_value. - Expr::Is(build_filter(inner)) - } else { - // ref_expr is the only remaining alternative. - Expr::Ref(identifier_to_string(first_inner(inner))) + Rule::is_expr => Expr::Is(build_filter(inner)), + Rule::ref_expr => Expr::Ref(identifier_to_string(first_inner(inner))), + // Spelled out, not `_`: a new `primary` alternative must fail the + // BUILD here. context/code/formula-and-io.md, "tree walkers". + Rule::EOI + | Rule::WHITESPACE + | Rule::formula + | Rule::target + | Rule::where_clause + | Rule::expr_eoi + | Rule::expr + | Rule::add_expr + | Rule::add_op + | Rule::mul_expr + | Rule::mul_op + | Rule::pow_expr + | Rule::pow_op + | Rule::unary + | Rule::unary_minus + | Rule::primary + | Rule::sp + | Rule::agg_func + | Rule::inline_where + | Rule::comparison + | Rule::cmp_op + | Rule::identifier + | Rule::pipe_quoted + | Rule::bare_ident + | Rule::filter_value + | Rule::string => { + unreachable!( + "{GRAMMAR_INVARIANT}: primary alternative {:?}", + inner.as_rule() + ) + } } } @@ -205,14 +292,44 @@ fn filter_value_to_string(pair: Pair) -> String { // filter_value = { string | pipe_quoted | bare_ident } let inner = first_inner(pair); let s = inner.as_str(); - let rule = inner.as_rule(); - if rule == Rule::string { - strip_string_quotes(s) - } else if rule == Rule::pipe_quoted { - unquote_pipe(s) - } else { - // bare_ident is the only remaining alternative. - s.to_string() + match inner.as_rule() { + Rule::string => strip_string_quotes(s), + Rule::pipe_quoted => unquote_pipe(s), + Rule::bare_ident => s.to_string(), + Rule::EOI + | Rule::WHITESPACE + | Rule::formula + | Rule::target + | Rule::where_clause + | Rule::expr_eoi + | Rule::expr + | Rule::add_expr + | Rule::add_op + | Rule::mul_expr + | Rule::mul_op + | Rule::pow_expr + | Rule::pow_op + | Rule::unary + | Rule::unary_minus + | Rule::primary + | Rule::is_expr + | Rule::sp + | Rule::paren_expr + | Rule::agg_call + | Rule::agg_func + | Rule::inline_where + | Rule::if_expr + | Rule::comparison + | Rule::cmp_op + | Rule::ref_expr + | Rule::identifier + | Rule::filter_value + | Rule::number => { + unreachable!( + "{GRAMMAR_INVARIANT}: filter_value alternative {:?}", + inner.as_rule() + ) + } } } @@ -271,6 +388,34 @@ mod tests { use super::{parse_agg_func, parse_cmp_op, parse_formula}; use crate::{AggFunc, BinOp, Expr}; + /// Every `primary` alternative routes to its own `Expr` — see + /// context/test-evidence/formula-and-io.md. + #[test] + fn each_primary_alternative_builds_its_own_expr() { + let of = |src: &str| { + parse_formula(&format!("X = {src}"), "_Measure") + .unwrap() + .expr + }; + assert!(matches!(of("42"), Expr::Number(n) if n == 42.0)); + assert!(matches!(of("SUM(Rev)"), Expr::Agg(..))); + assert!(matches!(of("IF(a < b, 1, 2)"), Expr::If(..))); + assert!(matches!(of("(Rev)"), Expr::Ref(ref r) if r == "Rev")); + assert!(matches!(of("Region IS East"), Expr::Is(_))); + assert!(matches!(of("Rev"), Expr::Ref(ref r) if r == "Rev")); + assert!(matches!(of("-Rev"), Expr::UnaryMinus(_))); + // filter_value's three alternatives, which share a dispatch. + let item = |src: &str| { + parse_formula(&format!("X = Y WHERE C = {src}"), "_M") + .unwrap() + .filter + .map(|f| f.item) + }; + assert_eq!(item("\"a b\"").as_deref(), Some("a b")); + assert_eq!(item("|a b|").as_deref(), Some("a b")); + assert_eq!(item("ab").as_deref(), Some("ab")); + } + /// improvise-492x — see context/test-evidence/formula-and-io.md. #[test] fn every_grammar_aggregate_and_comparison_is_spelled_out() {