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() {