From a5d240b0d4ebcc8c25d4b9c10e418c566931f1e6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 09:49:34 -0700 Subject: [PATCH] docs(formula): move the quoting and grammar arguments into context/ improvise-formula's comments carried three arguments worth keeping and several restatements of the code. The arguments move to context/code/formula-and-io.md (pipe quoting's one home, the keyword guard and why PEG rule ordering is not a rule, the unquoted formula target, and why a new grammar production needs generator coverage) and the bug narratives to context/test-evidence/formula-and-io.md, keyed by test name. No behaviour change: comments only. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/formula-and-io.md | 424 ++++++++++++++++ context/test-evidence/formula-and-io.md | 612 ++++++++++++++++++++++++ crates/improvise-formula/src/ast.rs | 6 +- crates/improvise-formula/src/parser.rs | 103 ++-- crates/improvise-formula/src/quote.rs | 60 +-- 5 files changed, 1083 insertions(+), 122 deletions(-) create mode 100644 context/code/formula-and-io.md create mode 100644 context/test-evidence/formula-and-io.md diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md new file mode 100644 index 00000000..b08bd431 --- /dev/null +++ b/context/code/formula-and-io.md @@ -0,0 +1,424 @@ +# `improvise-formula` and `improvise-io` + +The arguments that used to live in the comments of these two crates. Written +for a reader with the code open; the code says *what*, this says *why*, and +why the obvious alternative is wrong. Bug narratives keyed by test name are in +`context/test-evidence/formula-and-io.md`. + +--- + +## Pipe quoting has exactly one home + +`improvise_formula::quote` owns **both halves** of the CL-style `|…|` +convention — writing and reading — for both grammars. `formula.pest`'s +`bare_ident`/`pipe_quoted` and `improv.pest`'s `bare_name`/`pipe_quoted` +mirror each other character for character, and `quote.rs` is the one Rust +statement of that rule. + +It lives in `improvise-formula` because that crate owns the grammar that says +what a bare name *is*, and because every emitter needs it: a name that came +from a user's data has to be spelled so the parser hands it back unchanged. +`improvise-gp` re-exports it, `improvise-io` calls it from +`persistence/{format,parse}.rs`, and `parser.rs::unquote_pipe` is a thin shim +over `unescape_pipe_body`. + +Before improvise-i2m5 there were **three** copies and they had drifted, over +the keyword rule below. **`improvise-io` no longer has one** — its +`persistence/quoting.rs` is deleted. A stale line in `context/repo-map.md` +claimed io "still has its own unguarded copy" for months after the deletion; +if you find a comment or doc saying that, it is wrong. + +Three entry points, because the callers do not want the same thing: + +- `quote_name` — a name as the grammar wants it: bare when `is_bare_name` + allows, else pipe-quoted. +- `pipe_quote` — unconditional. A `.improv` text cell goes through this, not + through `quote_name`, because a bare `42` would read back as a number. +- `unescape_pipe_body` — the inverse, over the body between the delimiters + (what both grammars' `pipe_inner` matches). The escape table is written + once, in the last two. + +### The keyword guard is canonical, user-confirmed — do not revert it + +An item literally named `SUM` is written `|SUM|`. `is_bare_name` is +deliberately **stricter than the grammar strictly requires**: bare keywords +(`WHERE SUM AVG MIN MAX COUNT IF`) *do* parse today, but only because +`agg_call` needs a `(` and PEG backtracking then falls through to `ref_expr` +— an emergent property of **rule ordering**, not a stated rule. A pipe-quoted +name is an identifier in every position, with nothing to fall through, so a +name colliding with a keyword is quoted and stops depending on that ordering. + +The cost is worth paying because a `## Formulas` line carries formula text +**verbatim**: a name `format_md` writes is later read by `parse_formula`. One +namespace, one spelling rule. + +### The escape table, and the one input outside both grammars + +`\|` → `|`, `\\` → `\`, `\n` → newline; any other `\X` is preserved verbatim. +The trailing arms of `unescape_pipe_body` are unreachable from either grammar +(`pipe_inner` is `("\\" ~ ANY | !"|" ~ ANY)*`, so a backslash is always +followed by a character). They are written as a **total function** rather than +a panic so a caller arriving by some other route gets its bytes back. The two +pre-i2m5 unquoting copies differed *only* on a trailing lone backslash, where +io returned it and `parser.rs` panicked; the survivor returns it. + +--- + +## A formula target is unquoted, exactly as a reference is + +`parse_formula` strips the pipes off the target the same way it does off an +`Expr::Ref`, because the target **names a `_Measure` item**. A target that +kept its delimiters made `Model::add_formula` create an item literally called +`|Total Revenue|` — a ghost column the user's real item never matched, and +that `Expr::Ref("Total Revenue")` never resolved to. + +Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through +that one call, so both were affected. The writer emits `Formula::raw`, never +`target`, so the file text is unaffected either way — **which is exactly why +the round-trip tests never saw it**, and why the fix repairs existing files on +next load with no migration. + +--- + +## The `.improv` grammar is the single source of truth + +`persistence/improv.pest` defines the format; `parse.rs` is a tree-walker over +pest's parse tree, not a line scanner. Adding a format feature means changing +the grammar first. + +The test generator (`parse.rs::generator`, and its twin in +`parser.rs::generator` for `formula.pest`) reads the grammar's AST at test time +via `pest_meta` and emits random valid input. **A new production therefore +needs generator coverage**, or the proptests quietly stop describing the +format: an `Expr` variant the emitter does not handle produces nothing, and +`generated_file_parses` then passes over input that never exercised the new +rule. `formula.pest`'s generator is explicit about this — lookaheads emit +nothing on purpose, and any unused `Expr` variant falls through the `_ => {}` +arm; if the grammar starts using one, generated formulas fail to parse and the +gap surfaces. + +Two generator details worth not re-deriving. The `atomic` flag in the formula +generator controls whether a `Seq` inserts a space between children, so +non-atomic rules produce the whitespace-separated tokens pest's implicit +`WHITESPACE` accepts. And `Choice` is 50/50 between *two* branches, so rules +with three or more alternatives (nested `Choice`s) pick deeper branches less +often — good enough for random generation, not a uniform distribution. + +### The tree walkers are infallible by construction + +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`). + +`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 +hand-editable and a failure there is a user's problem, not a programmer's. +`parse_tree` renders pest's error as a located message (line number plus the +offending line), because "Parse error: {pest dump}" is unusable to somebody +fixing a file by hand. + +### Two passes, because sections may appear in any order + +Pass 1 turns each `## ` section into a plain collector (`PCategory`, `PView`) +or a flat list; pass 2 builds the `Workbook`. That split is what makes the +format order-independent — a `## View:` may name a category defined further +down. `PCategory::items` is a `Vec` in file order because `IndexMap` insertion +order is load-bearing (gotcha 18), so pass 2 must replay the list as written. + +### A section header takes its name as the whole line, totally + +`header_name = { pipe_quoted ~ &NEWLINE | line_name }` (improvise-pczv). The +`&NEWLINE` lookahead is load-bearing: without it, `## Category: |A|B` parses as +`A` and **silently drops `B`**. `line_name` catches everything else, so a +header name can no longer fail to parse. Four rules share it — `model_name`, +`initial_view`, `category_section`, `view_section` — which is why it is one +rule and not four copies. + +The writing half is `format.rs::quote_header`, and it is **deliberately not +`quote_name`**: that rule is for a name embedded among other tokens, and +applying it to a header would pipe-quote every ordinary spaced title the line +carries perfectly well, rewriting the header of every existing document for no +correctness gain. `quote_header` quotes exactly four things, each one thing the +*reader* does: + +- the empty name, and any name with leading or trailing whitespace — the + reader trims, so the category comes back renamed while every data coordinate + still names the untrimmed original: silent orphaning; +- a name containing `\n` or `\r` — it would start a second line, and + `Region\n## Data` a second *section*; +- a name already spelled `|…|` — the reader would take the pipes off. + +So a header is *written* raw where a reference is quoted. The grammar accepts +both arms, so that asymmetry is a **writer convention, not a format rule**. + +--- + +## `Storage` is whole-document read/write, and that closed the SQLite question + +Saving or loading a workbook only ever touches one whole document, so a backend +needs no seeking, metadata or partial reads (improvise-6mq). It does need to +enumerate and remove documents, because a browser build has no OS file manager +behind it — hence `list` and `delete` beside `read` and `write` (improvise-i34). + +Paths are **opaque document names, not a directory tree**: a backend may treat +them as filesystem paths, OPFS keys or map entries. The trait doc says +"backends that are not trees treat `dir` as a name prefix" — keep that sentence +reachable. It is the sentence that turned out to be *right while the `OpfsFs` +impl was wrong*, and the bug was found by reading it. + +Because the trait is two ops over whole documents, an `sqlite3_vfs` — which +needs `xRead(offset,len)`, `xWrite`, `xTruncate`, `xLock` — has nothing to bind +to. See gotcha 41 for the measurements that declined improvise-9ix. + +### `storage_contract` is a public function, not a test helper + +It reads like a test helper and it *was* one: `#[cfg(test)]` in `storage.rs`, +run against `MemoryFs` and `PhysicalFs` so they could not drift. The one +backend that most needed it could never be handed to it — `OpfsFs` is +`cfg(target_arch = "wasm32")` and its sync access handles exist in no host +harness — so for that backend the contract lived only in a doc comment. + +Moving it out of `cfg(test)` is what lets a browser run **this** sequence +rather than a second transcription of it (improvise-vvl3/xf1). A second copy of +a contract is the disease this codebase keeps curing. It returns `Result` +rather than asserting because a failure has to travel out of a Web Worker as a +string; the host tests `.unwrap()` it and get the same message. + +**Two bugs fell out of its first actual execution** — see the `dir` parameter +and the index rules below. + +`dir` is a parameter, not a constant, because the three backends must agree at +**every** `dir`, not only at the flat root the browser happens to use +(improvise-vhcz). A backend whose `list` answered in a different path shape +passes at `Path::new("")` — where joining is the identity — and fails at +anything else. + +### `impl Storage for Rc` lives in `improvise-io` because of the orphan rule + +`OpfsFs` keeps four genuinely async operations — `open`, `attach`, `refresh`, +`commit_deletes` — off the trait, so `Storage` can stay synchronous (gotcha +28). But `App::storage` is a `Box`, and a trait object can name +none of the four: **handing the backend to the `App` puts the async seam +permanently out of reach**. The host must hold the concrete backend *and* give +the `App` a `Storage`, which means sharing one. + +`impl Storage for Rc` is two foreign types in any other crate, so +improvise-9x6 wrote a forwarding newtype in the worker and every future worker +would have written it again. Here the trait is local, so the impl is allowed, +and it is **blanket** rather than `OpfsFs`-specific because nothing about it is +OPFS-shaped: it is the general statement that a shared handle to a backend is +usable as that backend (improvise-hmws). `Rc` and not `Arc` because the one +caller is a Web Worker — a single thread, and `wasm32-unknown-unknown` has no +others. An `Arc` impl would be speculative. + +--- + +## `OpfsFs`: why a browser backend can be synchronous, and what the index holds + +OPFS has two APIs. The promise-based one (`getFileHandle`, `createWritable`) is +async, but `FileSystemFileHandle::createSyncAccessHandle()` yields a handle +whose `read`/`write`/`truncate`/`flush`/`close` are ordinary **blocking** calls. +It exists so wasm can do blocking file I/O — it is what SQLite's wasm build uses +— and is available **only inside a dedicated Web Worker**, which is where +improvise-9x6 puts the whole app. So `Storage` never had to become async. + +*Acquiring* a handle is still async, as are directory listing and removal, so +the backend splits in two: + +- **async, explicit**: `open`, `attach`, `refresh`, `commit_deletes`, called by + the worker from its message loop where awaiting is free; +- **sync, via the trait**: `read`/`write` use an already-attached handle; `list` + answers from an index; `delete` drops the name from that index and queues the + removal. + +So a `delete` is visible to `read` and `list` immediately and lands in OPFS at +the end of the batch. That ordering is the price of a synchronous trait and is +invisible to a single worker. `write` therefore **un-queues** any pending +delete of the same name: otherwise delete-then-write would be listed and +readable and then removed by `commit_deletes` at the end of the batch — the one +ordering the deferral can get wrong. + +### The index holds documents, not entries + +`Storage::list` promises *document* names, and documents and entries stopped +being the same thing the moment `attach` had to create a file to get a handle +for a document that does not exist yet (improvise-vvl3). Three writers, and the +split between them is the whole rule: + +- **`attach` indexes nothing.** What it creates is zero bytes. Indexing there + made a fresh origin list `demo.improv` before anything had been written, so + the seed was skipped as "already there" and `load_from` got an empty buffer. +- **`Storage::write` indexes the name** (and un-queues a pending delete). A + written document is the first thing there is to list. +- **`refresh` rebuilds from OPFS, skipping zero-byte entries** — and counting an + entry it cannot *measure* as a document, since the one cause of that is + another tab holding the exclusive handle. + +`holds_bytes` is where the last clause lives, and its two failure modes get +**opposite answers**, which is the whole function: + +- `getFileHandle` fails → the name is not a file. `keys()` yields directories + too, and a directory rejects with `TypeMismatchError`; an entry removed + between `keys()` and here rejects as well. Neither is a document and neither + can ever be attached or read, so listing one is the same picker bug this + filter exists to remove (Copilot, PR #24). +- `getFile` fails on a real file handle → another context of the same origin + holds the exclusive sync access handle (two tabs). An entry somebody else has + open is certainly a document, so this **fails open**. Dropping it would hide a + real document; failing the whole `refresh` would let a second tab break the + first one's picker. Over-reporting costs one row — the direction `Dirty::of` + is deliberately sloppy in. + +Handles are exclusive per file, so `attach` closes any handle it replaces and +`detach` releases the lock. + +### `list` joins `dir` like the other backends + +The join is not cosmetic (improvise-vhcz). `MemoryFs` and `PhysicalFs` answer +with `dir`-joined paths — paths a caller can hand straight back to `read` — and +`OpfsFs` used to answer with bare names. At `Path::new("")` the two shapes +coincide, which is the only `dir` the worker ever passes, so the disagreement +was invisible: a worker that organised documents under `docs/` would have got a +**full picker in every host test and an empty one in a browser**, with nothing +saying why. + +`dir` is decoration rather than a filter, because this backend is flat by +construction: `entry_name` reduces every path to its file name, so +`docs/a.improv` and `a.improv` are the same OPFS entry and an entry name may not +contain a separator at all. That is the trait's own "treat `dir` as a name +prefix" — the doc was right and the impl was wrong. + +--- + +## Loading: decompression and decoding are two failures with two answers + +`load_from` decompresses, *then* decodes UTF-8, with a separate `Context` on +each. `read_to_string` would fuse them, and a valid gzip stream holding invalid +UTF-8 would then be reported as unreadable gzip — a wrong diagnosis of a +document that decompressed perfectly well. Decoding once, afterwards, also +leaves **one** answer to "is this UTF-8" rather than one per branch. + +The `.gz` suffix selects gzip regardless of backend: that is a property of the +document name, not of the filesystem (improvise-6mq). Legacy JSON is detected by +a leading `{` and is never written. + +`parse_number_cell` returns `CellValue::Error` naming the offending text rather +than a fabricated `0.0`. No token the current grammar admits actually fails +`f64::from_str` (overflow saturates to ±inf), but if the two ever drift the +mismatch must **surface** — never silently store a wrong number. + +--- + +## Persistence write order and the virtual views + +`format_md` writes Views → Formulas → Categories → Data: views first because +they are typically small and orient the reader, data last because it is the +largest section. The parser tolerates any order (see the two-pass note above). + +`_Records` and `_Drill` are scratch views the app rebuilds on demand. They put +`_Index`/`_Dim` on axes, and those categories are never written — so persisting +the views would produce axis lines naming categories the loader will not find. +They are skipped on write, and **dropped on load** if an older file carries +them, rather than reinstating a view the app would rebuild anyway; `Initial +View` then falls through to the default, since the name it pointed at is gone. +`persisted_initial_view` is the writing half: saving from records or drill mode +falls back to the first real view rather than naming one the file will not +contain (improvise-60z). + +`_Measure` persists only its non-formula items — formula targets are rebuilt +from the `## Formulas` section, so `format_md` collects the targets and excludes +them. + +`Page` and `Filter` both carry the selected item after the axis keyword; the +other axes have nothing to select. `Axis::Filter` round-trips even though it +occurs only inside the never-written `_Drill` view, because the alternative was +silently writing a Filter axis as some other axis (improvise-dz8, gotcha 27). + +--- + +## Import: order is a correctness property, not tidiness + +`analyzer::analyze_records` emits distinct values in **first-seen order**, never +`HashSet` order (improvise-7yx, gotcha 18). `build_model` adds items in exactly +that order and `IndexMap` preserves it, so a hash order made the *same CSV* +import to a different document on every run — different `.improv` bytes, +different row order on screen, and a different detected date format, since +`detect_date_format` only inspects the first ten samples and *which* ten it saw +varied. The `HashSet` in `analyze_records` is there to deduplicate and **never +to iterate**. + +### The ambiguous-date heuristic reads American-first, deliberately + +`01/02/2025` parses under both `%m/%d/%Y` and `%d/%m/%Y` and nothing in the data +can tell them apart. `detect_date_format` is first-match over `DATE_FORMATS`, so +the American reading wins. That is *a* defensible answer and not obviously the +right one: a European CSV imports with days and months swapped, **silently**, +and the derived `_Month`/`_Quarter` categories inherit the error. What makes it +tolerable is that the wizard shows the detected format and lets the user change +it — the same argument two-digit years already rested on. It is pinned by a test +so that changing the answer is a decision somebody makes rather than a side +effect of reordering the table. Filed as **improvise-l8rh**. + +### CSV parsing + +Short rows keep every header column, missing trailing fields filled with +`Value::Null`, and each affected column is reported as a warning with a row +count. The reader is `.flexible(true)`, so without this a short row parsed +cleanly and the trailing columns were simply *absent* from the record map — no +error, no warning, and skewed value counts in field-kind analysis +(improvise-k8i). + +A header name becomes a **category name** and survives into the `.improv` file, +so anything riding along invisibly in one — a byte-order mark, a stray carriage +return — is silent corruption rather than cosmetics. That is why the adversarial +inputs are tested with *controls*: a check that cannot see a BOM where there is +one proves nothing. + +`parse_csv_bytes_with_warnings` exists so CSV import can come from any `Storage` +backend rather than only a file (improvise-fuj); the path-taking wrappers remain +for the native CLI. + +### The wizard's back path is a history stack + +`advance` pushes the step it leaves; `previous` pops it. Keeping the path +actually taken, rather than re-deriving the reverse of each conditional +transition, means back can never disagree with how the user really got here +(improvise-907). + +Preconditions live in `validation_error()` and are checked **on the step where +the user can still fix them** — never deferred to `build_model` after the final +confirm. Two gates earn that: too many categories (improvise-mzv) and no +category at all (improvise-gfq); both are fixable with `c` at `ReviewProposals`, +and both used to refuse only at the last keystroke. `MAX_CATEGORIES` here mirrors +the model's private limit and counts regular categories only. + +`NameModel` is terminal for `advance`: leaving it is a **build**, not a step +change. `build()` parks the workbook in `built` on `Done`; a build failure leaves +the user on `NameModel` with the error. Going back from `Done` discards `built`, +so accepting an import always reflects the latest field decisions. + +`scalar_name` decides what a JSON scalar contributes as an item name. +`Value::Null` — which is also what the CSV parser produces for a blank field — +and a blank string are **absences, not names**; stringifying them used to invent +an item literally called "null" (improvise-ych). Every other value keeps its +printed form. + +A record missing a value for an accepted Category field has no place in the grid +and is dropped; `build_model_with_warnings` counts it against the *first* +category it is missing and reports the total, because without a warning that +loss is invisible in a thousand-row import (improvise-5j9). + +--- + +## CSV export + +`render_csv` is separated from writing so the bytes can go to any `Storage` +backend, not just a file (improvise-fuj). It goes through the `csv` crate rather +than joining with commas, and it honours the view's number format — a value of +1200 under a comma-grouping format renders `1,200`, and a label like +`Smith, Jr.` splits into two fields, corrupting every row (improvise-1cz). diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md new file mode 100644 index 00000000..978acb47 --- /dev/null +++ b/context/test-evidence/formula-and-io.md @@ -0,0 +1,612 @@ +# Test evidence — `improvise-formula` and `improvise-io` + +What each of these tests is evidence *of*: the bug it reproduced, the fault +that was injected, and why it can fail. Keyed by exact test name — a rename +orphans the entry, and `scripts/check-comment-runs.py --check-evidence` +catches that. The mechanism arguments are in +`context/code/formula-and-io.md`. + +--- + +## `improvise-formula/src/parser.rs` + +### parse_sum_with_inline_where_filter + +Regression: a `WHERE` inside the aggregate's parens must parse as the +aggregate's *inline* filter, not as a top-level `WHERE` clause on the formula. +The two land in different places on the AST — `Expr::Agg(_, _, Some(filter))` +against `Formula::filter` — and the difference is silent at parse time. + +### hyphen_in_bare_identifier_reference + +Regression. The hand-rolled tokenizer that preceded the pest grammar did not +allow `-` inside bare identifiers, so a reference to a persistence-legal name +like `east-coast` was silently parsed as `east - coast`: a **subtraction of two +unrelated `Expr::Ref`s**, which evaluates rather than erroring. The pest grammar +mirrors `improv.pest`'s `bare_name`, which does allow `-`. + +### hyphen_in_bare_identifier_inside_aggregate + +The same fix through the aggregate path, because `SUM(east-coast)` reaches the +identifier rule by a different route. + +### pipe_quoted_target_is_unquoted_like_a_reference + +Regression, and a data bug rather than a cosmetic one. The tree walker took the +target's **raw span**, so a pipe-quoted target came back with its delimiters +still attached while an `Expr::Ref` for the very same name came back unquoted. +Downstream, `Model::add_formula` then created a `_Measure` item literally named +`|My Target|`, which never matched the user's item `My Target`: the formula +wrote into a ghost column and nothing referring to it by name resolved. + +The test asserts both halves — the stripped target *and* that the reference +spelled identically names the same item — because asserting only the target +would pass under a walker that unquoted both wrongly in the same way. + +Why no existing test caught it: the writer emits `Formula::raw`, not `target`, +so the file text is identical either way and every round-trip test was blind to +it. See `context/code/formula-and-io.md` § "A formula target is unquoted". + +### pipe_quoted_target_applies_escapes + +The escape table applies to the target too, not only to `Expr::Ref` — the +companion that stops the fix above from being "strip the outer pipes". + +### bare_target_is_unchanged + +The negative arm: a bare target has nothing to strip. Without it, "always strip +the first and last character" would pass the two tests above. + +--- + +## `improvise-formula/src/quote.rs` + +### unescaping_inverts_quoting + +The reading half inverts the writing half over every escape both grammars +define, plus the `\X` passthrough neither of them assigns a meaning to. The +`"42"` case is there because `pipe_quote` is unconditional: a `.improv` text +cell holding `42` must not read back as a number. + +### a_trailing_lone_backslash_is_kept + +Before improvise-i2m5 the unescape table existed **twice** — here (as +`parser.rs::unquote_pipe`) and as +`improvise-io/src/persistence/quoting.rs::unescape_pipe`. The two agreed on +every input either grammar's `pipe_inner` can produce, and differed *only* on a +trailing lone backslash, which `pipe_inner` cannot emit: io returned it, +`parser.rs` panicked. This pins the surviving answer for that unreachable input, +so the surviving function is total. + +### a_quoted_name_round_trips_through_the_parser + +The property the whole module exists for: a name `quote_name` spells parses, and +the parser hands back the name that went in — as a **target** and as a +**reference**, which after the target-unquoting fix are the same rule. The +fixture list is the interesting part: keywords (`WHERE`, `SUM`), a leading digit +(`9lives`), the escapes, and the empty name. + +### a_keyword_is_quoted_even_though_it_looks_bare + +Pins the keyword guard, which is deliberately stricter than the grammar +requires and is **user-confirmed** — do not revert it as over-caution. The +argument (PEG rule ordering, and `## Formulas` carrying formula text verbatim) +is in `context/code/formula-and-io.md`. + +--- + +## `improvise-io/src/storage.rs` + +### a_shared_backend_is_a_backend_and_a_clone_is_the_same_one + +improvise-hmws. A worker must hold the concrete backend — for the four async +methods the trait does not carry — *and* hand the `App` a `Storage`. Before the +blanket `impl Storage for Rc`, `Rc` was not a `Storage` and could not +be made one outside this crate (orphan rule), so `improvise-worker-server` wrote +a forwarding newtype, and so would every worker after it. + +**The regression guard is the signature**: this does not compile without the +impl. What it asserts *beyond* compiling is the part a newtype could still get +wrong — that the clone the `App` holds and the handle the message loop keeps are +one backend and not two. + +### memory_fs_satisfies_the_storage_contract + +### physical_fs_satisfies_the_storage_contract + +Both run `contract::storage_contract` at a **non-empty `dir`**, which is the +whole point of `dir` being a parameter: at `Path::new("")` a backend that +answers `list` in the wrong path shape passes, because joining is the identity +there. `OpfsFs` did exactly that and the disagreement was invisible until +improvise-vhcz (see `context/code/formula-and-io.md`). + +--- + +## `improvise-io/src/persistence/mod.rs` + +### load_corrupt_gzip + +Not a test — the harness the two gzip-corruption tests share. It asserts two +**preconditions** before handing the damaged bytes to the loader: that the +corruption actually changed the bytes, and that the result still looks like a +gzip document by its magic bytes. Without the second, a corruption that also +broke the routing would make the loader fail for a different reason and the test +would pass while proving nothing. + +### a_truncated_gzip_document_fails_as_an_unexpected_eof + +A `.gz` document cut short — the disk filled, or the process died mid-save. The +**kind** is asserted (`io::ErrorKind::UnexpectedEof`), not merely that an error +occurred, because "an error occurred" would also pass if the loader had stopped +decompressing altogether and choked on the compressed bytes as text. + +### a_gzip_document_with_a_flipped_byte_fails_its_checksum + +The other way a `.gz` arrives damaged, and it is a different kind +(`InvalidInput`), which is the reason to test both: one test against one +corruption cannot tell "the decoder refuses damaged input" from "the decoder +refuses short input". The first is *incomplete*, the second is *wrong* — "you +lost the tail" versus "you cannot trust any of it". + +The byte flipped is in the **CRC32 field**, the first four of the eight-byte +gzip trailer, and not in the DEFLATE payload (Copilot, PR #22). A payload byte +also yields `InvalidInput`, but it can do so for malformed compressed data +rather than for the mismatch the test is named after, and the name would then be +a guess about which. Corrupting the trailer leaves the stream decodable, so the +only thing left to fail is the checksum — hence the assertion on the *source* +error's message, since anyhow's `to_string` shows only the outermost context. + +### a_valid_gzip_of_invalid_utf8_is_reported_as_not_utf8 + +The third failure, and the one that made the other two worth keeping apart in +the code as well as in the tests (Copilot, PR #22). A gzip stream can be +perfectly well-formed and still hold bytes that are not UTF-8; `read_to_string` +decodes as it decompresses, so a single `Context` around it called that document +*unreadable gzip* — a wrong diagnosis of a document that decompressed without +complaint. + +The precondition asserts the payload really is invalid UTF-8; without it the +test would also pass on a document that merely failed to decompress, which is +the case the other two already own. It also asserts the message does **not** +mention gzip: the gzip stream was fine, and blaming it is the bug. + +### storage_fixture + +A workbook with a category, items, a formula and data — enough that a round-trip +exercises every section of the format. The gzip tests reuse it so that "the +document survived" is a claim about a real document rather than about an empty +one. + +### round_trips_through_memory_storage + +improvise-6mq: persistence goes through `Storage`, so a workbook can be saved and +loaded with **no filesystem at all** — no `tempfile`, no disk. This is the shape +the browser backend plugs into. + +### gzip_round_trips_through_memory_storage + +The `.gz` suffix selects gzip regardless of backend: a property of the document +*name*, not of the filesystem. + +### physical_and_memory_backends_write_identical_bytes + +Swapping the backend must not change the bytes written. This is what stops a +backend from quietly normalising or re-encoding what it is handed. + +--- + +## `improvise-io/src/persistence/parse.rs` + +### parse_md_formula_category_suffix_without_space + +Bug improvise-zgb (b). The formulas walker located the `[Category]` suffix with +`raw.rfind(" [")`, so a hand-written line with no space before the bracket — +`- Total = Food + Gas[Type]` — was not recognised and the formula was **silently +retargeted to `_Measure`**. The suffix is now part of the grammar +(`formula_line`) and walked structurally. + +### parse_md_formula_ending_in_bracketed_pipe_name_keeps_raw + +Companion to improvise-zgb: a formula *ending* in a pipe-quoted identifier that +contains `" ["` must not have the quoted name mistaken for a category suffix. It +stays a `_Measure` formula with its raw text intact. Without this arm, "look for +the last `[`" would pass the test above. + +### unparseable_number_token_yields_error_not_zero + +Bug improvise-4yc. The `Rule::number` arm used `as_str().parse().unwrap_or(0.0)`, +so any numeric token the grammar admitted but `f64::from_str` rejected silently +became `Number(0.0)` — a fabricated value in a spreadsheet. + +No token the *current* grammar admits actually fails `f64` parsing (overflow +saturates to inf), so the test exercises `parse_number_cell` **directly**, with +the forms the grammar could plausibly drift toward admitting. That is the point: +the guard exists for the drift, not for today. + +### malformed_numbers_in_data_never_silently_zero + +The same bug through the grammar, covering all three routings: `--5` fails the +`number` rule entirely and becomes `bare_value` → `Text`; `3.14.15` matches the +`3.14` prefix, commits, and the rest fails → a whole-file parse error naming the +text; `1e999` is admitted and saturates to `+inf`. None of the three is `0.0`. + +### parse_error_names_line_number_and_contents + +Bug improvise-6kj: parse failures were wrapped as `Parse error: {e}`, leaning on +pest's raw rendering instead of stating the location in plain words. A user +fixing a hand-edited file needs the line number and the offending line. + +### roundtrip_preserves_an_awkward_category_name + +improvise-pczv as a property rather than a list. `quote_header` decides which +names the header line cannot carry, and that decision is a small set of clauses +— so the property is what earns its place: for *any* name, save-then-load must +give the name back. It runs against `populated_model` rather than a fresh one +because a workbook with no data cannot show a coordinate losing its category, +which is the silent half of the bug. + +### populated_model + +A workbook that already holds categories, items and cells. Named here because +the *reason* it is not a default-constructed one is the evidence: a +default-constructed workbook makes the round-trip test vacuous for the +data-coordinate half. + +### awkward_name + +The strategy behind the property above. The generated shapes are the whole-line +contract's stress points: edge whitespace, embedded line breaks, pipes, and text +that looks like another section marker (`\n## Data`). + +--- + +## `improvise-io/src/persistence/format.rs` + +### virtual_views_are_not_written + +improvise-60z. `_Records` and `_Drill` are scratch views the app builds on +demand. They put `_Index`/`_Dim` on axes, and those categories are never written +to the file — so persisting the views produces axis lines referring to +categories the loader will not find. The test also asserts a real view *is* +written, so "write no views at all" cannot pass it. + +--- + +## `improvise-io/src/persistence/roundtrip_tests.rs` + +### initial_view_skips_a_virtual_active_view + +improvise-60z's writing half: saving while in records mode must not leave +`Initial View: _Records` pointing at a view the file no longer contains. It +asserts the loaded workbook's active view **exists in the loaded workbook**, not +merely that the string changed. + +### virtual_views_in_an_existing_file_are_dropped_on_load + +The reading half: a file written by an older build may already carry the scratch +views, and loading one must not reinstate them. + +### filter_axis_round_trips_with_its_selection + +improvise-dz8. `Axis::Filter` arises only inside the `_Drill` view, which is +never written, so this is defensive — but the alternative to supporting it in +the format is **silently writing a Filter axis as something else**. + +### parse_md_round_trips_formula_category_with_special_chars + +Bug improvise-zgb (a). `rfind(" [")` mis-split a suffix whose category name +itself contains `" ["`: the writer emitted `[Margin [%]]` unquoted and the parser +then split inside the name, producing garbage raw text and a category called +`%]`. Category names in the suffix are now pipe-quoted on write and parsed via +the grammar's `name` rule. + +### a_keyword_named_category_is_pipe_quoted + +improvise-i2m5. `.improv` now spells names through +`improvise_formula::quote_name`, which pipe-quotes a formula keyword. Before the +switch, io's own copy of the rule had no keyword guard and wrote +`## Category: SUM`, `- WHERE` and `SUM=WHERE = 1` bare — that was the **only** +output difference the whole persistence suite showed. + +The names round-trip either way (`improv.pest` accepts `pipe_quoted` wherever a +`name` goes), so this pins the *spelling*, not the survival. It also pins the +header exception: `category_section` takes the whole line and never goes through +`quote_name` on either side, so only *references* are quoted. + +### a_keyword_named_category_round_trips_everywhere_it_appears + +The round trip the quoting exists for, at every position a keyword-named +category or item reaches the file: the axis line, the page selection, +`hidden:`, the category section and the data coordinates. A guard that covered +only some of them would pass a single-position test. + +### a_formula_targeting_a_keyword_named_measure_round_trips + +Why the keyword guard is worth its cosmetic cost even though bare keywords +parse: a `## Formulas` line carries formula text **verbatim**, so a name written +by `format_md` is later read by `parse_formula`. The two grammars share one +namespace, and `quote_name` is the one rule spelling names for both. + +### a_category_name_the_header_line_cannot_carry_round_trips + +improvise-pczv. A section header was the one name position that never went +through a quoting rule, so a name the header *line* could not carry was written +raw into it. The table `NAMES_THE_LINE_CANNOT_CARRY` is the whole rule, and each +entry is a different failure: + +- `Region\nName` — the newline ended the header and the rest became a stray + line: the file would not parse at all. +- `Region\n## Data` — worse: the remainder looked like another section marker. +- `"Region "` / `" Region"` — the reader trims, so the category came back + renamed while every data coordinate still named the untrimmed original: + **silent orphaning**. +- `""` and `"|Region|"` — would be taken for a quoted name and unquoted. + +This is reachable and not theoretical — see +`a_csv_header_with_an_embedded_newline_survives_a_save`. + +### a_model_and_view_name_the_header_line_cannot_carry_round_trip + +The same rule at the other three header positions: the model title, the view +title, and the `Initial View:` reference that must still resolve to the view. + +### an_ordinary_spaced_header_stays_bare + +The negative arm, and the one that stops `quote_header` from quietly becoming +`quote_name`. The header carries its name as the whole line — that is the +contract — and quoting a name the line carries fine would rewrite the header of +every existing document for nothing. It also asserts that a *reference* to the +same name is still quoted, so the two rules stay distinguishable. + +### pipes_that_do_not_close_the_header_line_are_part_of_the_name + +The one genuinely ambiguous line, decided by the grammar's `&NEWLINE`: +`## Category: |a| and |b|` is a category called `|a| and |b|`. Without the +lookahead the pipe-quoted arm would match `a` and **silently drop the rest of +the line**. + +### a_header_written_by_an_older_build_still_loads + +A header written before improvise-pczv is a bare line and must still load as one +— spaces, pipes in the middle and all. The compatibility direction of the same +grammar change. + +### a_csv_header_with_an_embedded_newline_survives_a_save + +The bug as a user meets it, and the reason improvise-pczv is a corruption bug +rather than tidiness. An RFC 4180 CSV may quote a header field containing a +newline; nothing on the import path forbids it, so the category carried one — +and the `.improv` file the user then saved **could not be reopened**. The test +asserts the precondition (the import still admits the header verbatim) so it +cannot pass by the import having quietly started rejecting it. + +### an_item_name_the_item_line_cannot_carry_round_trips + +improvise-odbc. `bank-info.improv` in the repo root failed to load at +`- JORDAN MARTINEZ` (line 293) with `expected pipe_quoted`. The ticket named two +readings — a stale file, or a writer/reader gap in which improvise emits an item +line the grammar rejects — and said to decide by asking whether improvise can +*write* that line today. + +It cannot. `item_list`'s `name` is `pipe_quoted | bare_name`, and `bare_name` is +`[A-Za-z_][A-Za-z0-9_-]*` with no space; `quote_name` mirrors it character for +character, so every one of those item names is emitted pipe-quoted and read +straight back. The file was hand-edited in `c48a5cd` — the `v2025-04-09` +migration quoted the `Date` items and left the `Entity` ones bare — and never +round-tripped. So the reading is "stale file", and the standing finding was that +nothing exercised it. This test is what that finding turned into: the item names +from the file that motivated the ticket, driven through the writer and back. It +asserts both directions — the spaced name is quoted, the bare-legal name stays +bare. + +### a_bare_item_name_with_a_space_is_rejected + +The other half of the same finding: the exact line the file carries is *not* +something the reader accepts, so the file really was broken rather than the +error message being misleading. Asserting the failure keeps the test above +honest — without it, "the writer quotes it" would be consistent with the reader +also accepting the unquoted form. The trailing precondition (the same line +quoted *does* parse) stops it passing because of some unrelated syntax error. + +--- + +## `improvise-io/src/persistence/export.rs` + +### export_csv_quotes_commas_per_rfc4180 + +Bug improvise-1cz. `export_csv` joined labels and values with bare commas — no +RFC 4180 quoting — and ignored the view's number format. Under a comma-grouping +format a value of 1200 renders as `1,200` and a label like `Smith, Jr.` splits +into two fields, **corrupting every row**. The test reads the file back with a +real CSV reader and asserts the field *count*, which is the thing a string +`contains` check cannot see. + +--- + +## `improvise-io/src/import/analyzer.rs` + +### detect_short_year_format + +Two-digit years are ambiguous with the four-digit format, so `%m/%d/%Y` matches +first. Expected, not a defect: the wizard shows the detected format and the user +can override it. The same argument as the ambiguity below. + +### an_ambiguous_date_is_read_american_first + +`01/02/2025` and `02/03/2025` parse under `%m/%d/%Y` *and* `%d/%m/%Y`, and +nothing in the data can tell them apart. The heuristic is first-match over +`DATE_FORMATS`, so the American reading wins — a European CSV imports with its +days and months swapped, silently, and the derived `_Month`/`_Quarter` +categories inherit the error. Filed as **improvise-l8rh**; pinned here so that +changing the answer is a decision somebody makes rather than a side effect of +reordering the table. + +The loop at the end is the precondition that makes this a statement about +*ambiguity* rather than about one format merely fitting: the other reading has +to fit too, or there is nothing to resolve. + +### a_day_first_only_value_settles_the_ambiguity + +Where the two readings genuinely can be told apart, the unambiguous value +decides — first match still, but `%m/%d/%Y` now fails. Without this, "always +American" would pass the test above. + +### the_distinct_values_are_in_first_seen_order + +improvise-7yx, found while pinning the date heuristic. `analyze_records` built +its distinct values by collecting into a `HashSet` and iterating it, so the order +was different in every process. This is gotcha 18's bug in a second place, and it +reaches further than a tidy-up: `build_model` adds items in exactly this order, +`IndexMap` preserves insertion order, and persistence relies on it — so the +**same CSV imported twice produced two different `.improv` files** and two +different row orders on screen. + +The answer is first-seen order: deterministic, and the order the records +themselves are in, which is the one a user can predict. Five runs is enough to +see it, because each `HashSet` gets its own `RandomState`; the pre-fix code +produced five different orders here. + +### the_detected_date_format_does_not_depend_on_the_hash_seed + +The consequence of the same bug that a user would have found hardest to explain. +`detect_date_format` tests only the first ten samples, so when the sample order +was a hash order, *which* ten it saw varied — and with it the detected format, +whenever the eleventh-and-later values are the ones that rule a format out. + +Twelve values are ambiguous and one (`25/12/2025`) is day-first only. In hash +order it landed inside the first ten about half the time, so the column imported +as `%d/%m/%Y` on some runs and `%m/%d/%Y` on others — every date in it off by the +difference. In first-seen order it is last, deliberately: the answer is +`%m/%d/%Y` **stably**, which is the property asserted. The ten-sample truncation +itself is unchanged and is still a real limit. + +--- + +## `improvise-io/src/import/csv_parser.rs` + +### short_rows_pad_missing_trailing_columns_with_null + +Bug improvise-k8i. The reader is configured `.flexible(true)`, so rows shorter +than the header parsed without error — but the missing trailing columns were +simply **absent** from the record map. No error, no warning, and field-kind +analysis saw skewed value counts. Short rows must keep every header column, +missing ones as `Null`. + +### short_rows_produce_warning_naming_column_and_count + +Companion to improvise-k8i: the warning must name the affected column and say how +many rows were short. A warning that said only "some rows were short" would pass +a weaker test and be useless to the user. + +### full_rows_produce_no_warnings + +The negative arm: padding must not fire on a complete file. + +### a_leading_utf8_bom_is_not_part_of_the_first_header + +A header name becomes a **category name** and survives into the `.improv` file, +so a byte-order mark riding along invisibly is silent corruption rather than +cosmetics. Excel writes one; the `csv` crate strips a *leading* one, so the first +header is `Region` and not `\u{feff}Region`. + +The control at the end is what makes this an assertion rather than a tautology: +a BOM in the *second* header is not leading, is not stripped, and shows up in the +key — so the check demonstrably can see a BOM where there is one. + +### crlf_line_endings_leave_no_carriage_return_in_the_data + +The `\r` belongs to the terminator, not to the last field of the row: a `Revenue` +of `1\r` would be a string rather than a number, and every row would import as +text. + +The control: a **lone** `\r` is itself a record terminator to this reader, so +`Ea\rst` splits one row into two. That is what a stray carriage return does when +it is not part of a CRLF pair, and it is why "two records, clean values" is a +real observation rather than a coincidence. + +### an_empty_file_is_no_records_and_no_warnings + +An empty file is no records and no complaint. The wizard's own preconditions +(`wizard.rs::validation_error`) are what refuse to build a model from nothing; +the parser's job is to say truthfully that there was nothing there. + +### a_header_with_no_rows_is_no_records + +The same answer, and the case a user actually hits — an export filtered down to +nothing. + +### a_single_column_file_yields_one_field_per_record + +The degenerate shape for everything downstream that splits a row into +coordinates and a measure: one field, present, and **not** mistaken for a short +row. + +--- + +## `improvise-io/src/import/wizard.rs` + +### wizard_previous_reverses_each_advance + +Bug improvise-907 (from improvise-o1c): no step bound a back key, so reaching a +later step and wanting to change a field kind meant Esc — discarding every +proposal toggle, formula and name. `previous` walks back through the path +**actually taken**, including the steps a conditional transition skipped on the +way forward, which is why it is a history stack and not a reverse table. + +### wizard_build_parks_the_workbook_on_done + +Bug improvise-907 (from improvise-c6j): `advance` mapped `NameModel` → `Done` +but nothing ever called it there, so `Done` was reachable **only from tests**. +`build` now parks the workbook on `Done` for confirmation. + +### wizard_build_failure_stays_on_name_model + +A build that fails despite the earlier gates leaves the user on `NameModel` with +the error, rather than moving to a `Done` step with nothing to confirm. + +### wizard_blocks_advance_when_over_category_limit + +Bug improvise-mzv: with more than `MAX_CATEGORIES` (12) accepted Category +proposals, the wizard let the user advance through every remaining step and +`build_model()` only failed at the final confirm. Advancing past +`ReviewProposals` must be blocked, with a message telling the user to mark +fields as Measure or toggle them off — the actions still available on that step. + +### wizard_blocks_advance_when_no_category_accepted + +Bug improvise-907 (from improvise-gfq): `build_model` raised "At least one +category must be accepted" only on Enter at `NameModel`, the final step. An +import whose fields all classify as Measure walked the user through +`DefineFormulas` and `NameModel` before refusing. The gate belongs at +`ReviewProposals`, where `c` can still change a field's kind — the same treatment +improvise-mzv gave the too-many-categories case. + +### build_model_null_category_value_is_missing_not_the_item_null + +improvise-ych. A category value of JSON `null` used to be stringified by the +`v.to_string()` fallback, **inventing an item literally named "null"** and +placing the record under it. A CSV empty cell parses to `Value::Null` too, so +every blank column value in a CSV import grew a phantom "null" item. Null means +*absent*: the record must be skipped and counted, exactly as if the key were +missing. + +### build_model_literal_string_null_is_a_real_category_item + +The other side of improvise-ych: the *string* `"null"` is an ordinary value — a +region really called "null" — and must survive as its own item. Only +`Value::Null` means absent. Without this arm, "drop anything that stringifies to +null" would pass the test above. + +### build_model_empty_string_category_value_is_missing + +improvise-ych, third arm. An empty string is the JSON spelling of a blank cell — +the CSV parser already turns a blank field into `Value::Null`, so the two inputs +must agree. An item with an empty name is unnameable in the grid, so it is +treated as missing rather than created. + +### build_model_warns_about_records_skipped_for_missing_category + +improvise-5j9: skipping a record for a missing category value used to be silent, +so a 1000-row import could drop rows with no user-visible sign. The warning must +name the count, the total, and the offending field(s) — a bare "some records were +skipped" is not actionable. diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 30095ec1..aaf8341a 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -9,10 +9,8 @@ pub enum AggFunc { Count, } -/// Arithmetic and comparison operators used in binary expressions. -/// Having an enum (rather than a raw String) means the parser must -/// produce a valid operator; invalid operators are caught at parse -/// time rather than silently returning Empty at eval time. +/// Arithmetic and comparison operators. An enum rather than a `String`, so an +/// invalid operator is caught at parse time — design-principles §2. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum BinOp { Add, diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 2acc1e38..9e68dda9 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -9,9 +9,8 @@ use super::ast::{AggFunc, BinOp, Expr, Filter, Formula}; #[grammar = "formula.pest"] struct FormulaParser; -/// Message used by `.expect()` calls on invariants that the grammar -/// guarantees. If one of these ever panics, the grammar and the tree -/// walker are out of sync — it's a bug, not a runtime condition. +/// Bug marker for `.expect()` on invariants the grammar guarantees: if one +/// fires, the grammar and the tree walker are out of sync. const GRAMMAR_INVARIANT: &str = "grammar invariant violated: parser out of sync with formula.pest"; /// Parse a formula string like "Profit = Revenue - Cost" @@ -36,14 +35,8 @@ pub fn parse_expr(s: &str) -> Result { Ok(build_expr(first_inner(expr_eoi_pair))) } -// ---- tree walkers ------------------------------------------------------- -// -// Every `build_*` function below operates on a pest Pair that has already -// been validated by the grammar. Invariants like "a `formula` has exactly -// one `target`" or "a `comparison` has an lhs, op, and rhs" are guaranteed -// by pest before the tree walker sees the Pair, so these functions are -// infallible. Any `.expect(GRAMMAR_INVARIANT)` in here represents a bug -// marker — if it ever fires, the grammar and the walker have diverged. +// ---- tree walkers: infallible, because pest validated the pair first ---- +// See `context/code/formula-and-io.md` for why every .expect is a bug marker. fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula { let mut target = None; @@ -52,9 +45,8 @@ fn build_formula(pair: Pair, raw: &str, target_category: &str) -> Formula for inner in pair.into_inner() { let rule = inner.as_rule(); if rule == Rule::target { - // Unquoted exactly like `Expr::Ref`: a target and a reference that - // spell the same name must produce the same item name, or the - // formula's own output lands in a column nothing can refer to. + // 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)); @@ -219,10 +211,8 @@ fn filter_value_to_string(pair: Pair) -> String { } } -/// Convert an identifier pair (identifier, pipe_quoted, or bare_ident) to -/// its content string. Pipe-quoted identifiers have their delimiters -/// stripped and backslash escapes applied; bare identifiers are returned -/// verbatim. +/// An identifier pair (identifier, pipe_quoted or bare_ident) as its content: +/// pipes stripped and escapes applied, or returned verbatim. fn identifier_to_string(pair: Pair) -> String { let s = pair.as_str(); if is_pipe_quoted(s) { @@ -241,9 +231,8 @@ fn strip_string_quotes(s: &str) -> String { s[1..s.len() - 1].to_string() } -/// Strip surrounding pipes and apply the escape table `quote::pipe_quote` -/// writes — the one copy of that rule, shared with `improvise-io`'s -/// `.improv` walker (improvise-i2m5). +/// Strip the pipes and apply `quote::pipe_quote`'s escape table — the one copy +/// of that rule, shared with `improvise-io`'s `.improv` walker (improvise-i2m5). fn unquote_pipe(s: &str) -> String { debug_assert!(is_pipe_quoted(s)); crate::quote::unescape_pipe_body(&s[1..s.len() - 1]) @@ -255,10 +244,8 @@ fn first_inner(pair: Pair<'_, Rule>) -> Pair<'_, Rule> { pair.into_inner().next().expect(GRAMMAR_INVARIANT) } -/// Fold a left-associative binary-operator rule of the shape -/// `rule = { child ~ (op ~ child)* }` into a left-leaning BinOp tree. -/// The `match_op` closure is infallible — the grammar guarantees the -/// operator token is one of the expected alternatives. +/// Fold `rule = { child ~ (op ~ child)* }` into a left-leaning BinOp tree. +/// `match_op` is infallible: the grammar guarantees the operator token. fn fold_left_binop(pair: Pair, mut build_child: F, match_op: M) -> Expr where F: FnMut(Pair) -> Expr, @@ -605,9 +592,8 @@ mod tests { #[test] fn pipe_quoted_unknown_escape_preserved() { - // Any `\X` where X isn't |, \, or n is preserved verbatim as - // backslash-plus-character. The grammar's `"\\" ~ ANY` allows - // any following character; we just don't interpret it. + // Any other escape is preserved verbatim: the grammar allows any + // character after a backslash; we just don't interpret it. let f = parse_formula("X = |A\\zB|", "Cat").unwrap(); assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\zB")); } @@ -664,9 +650,8 @@ mod tests { #[test] fn unary_minus_before_pow() { - // `-A ^ B` must parse as Pow(UnaryMinus(A), B). The `-` binds - // through `unary_minus = { "-" ~ primary }`, producing a unary - // node that is then used as the pow_expr's base. + // `-A ^ B` is Pow(UnaryMinus(A), B): `-` binds through + // `unary_minus = { "-" ~ primary }`, giving the pow_expr's base. let f = parse_formula("X = -A ^ B", "Cat").unwrap(); assert!(matches!( &f.expr, @@ -804,12 +789,8 @@ mod tests { assert_eq!(f.target, "my_target-name"); } - /// Regression: the hand-rolled tokenizer didn't allow `-` inside bare - /// identifiers, so references to persistence-legal names like - /// `east-coast` were silently parsed as `east - coast` (a subtraction - /// of two unrelated Refs). The pest grammar mirrors improv.pest's - /// `bare_name`, which does allow `-`, so a hyphenated name now parses - /// as a single identifier. + /// Regression: `east-coast` used to parse as `east - coast`. Evidence: + /// `context/test-evidence/formula-and-io.md`. #[test] fn hyphen_in_bare_identifier_reference() { let f = parse_formula("Total = east-coast + west-coast", "Cat").unwrap(); @@ -832,13 +813,8 @@ mod tests { )); } - /// Regression: the tree walker took the target's raw span, so a - /// pipe-quoted target came back with its delimiters still attached while - /// an `Expr::Ref` for the very same name came back unquoted. Downstream - /// that is a data bug, not a cosmetic one: `Model::add_formula` creates a - /// `_Measure` item literally named `|My Target|`, which never matches the - /// user's item `My Target`, so the formula writes into a ghost column and - /// nothing referring to it by name resolves. + /// Regression: a pipe-quoted target kept its delimiters and so created a + /// ghost item. Evidence: `context/test-evidence/formula-and-io.md`. #[test] fn pipe_quoted_target_is_unquoted_like_a_reference() { let f = parse_formula("|My Target| = |My Target|", "Cat").unwrap(); @@ -865,11 +841,8 @@ mod tests { } } -// ---- grammar-driven proptests ------------------------------------------ -// -// Uses pest_meta to read formula.pest at test time and walks the grammar -// AST to generate random valid formulas. Mirrors the pattern used by the -// persistence parser tests in src/persistence/parse.rs. +// ---- grammar-driven proptests: pest_meta reads formula.pest at test time +// and walks its AST. A new production needs generator coverage (context doc). #[cfg(test)] mod generator { @@ -890,11 +863,8 @@ mod generator { .collect() } - /// Recursive string generator driven by a pest `Expr`. `choices` is - /// consumed left-to-right at every decision point; the `atomic` flag - /// controls whether sequences insert a space between their children, - /// so that non-atomic grammar rules produce whitespace-separated - /// tokens that pest's implicit WHITESPACE handling accepts. + /// Recursive string generator driven by a pest `Expr`; `choices` is consumed + /// left-to-right. The `atomic` flag is explained in the context doc. pub struct Gen<'g> { rules: &'g HashMap, choices: Vec, @@ -929,11 +899,8 @@ mod generator { self.emit(b, out, atomic); } Expr::Choice(a, b) => { - // 50/50 between the two branches of each Choice. - // Nested Choices (used by rules with 3+ alternatives) - // recurse into themselves, so deeper branches are - // chosen less often than shallower ones — good - // enough for the random-generation use case. + // 50/50 per Choice; nested Choices make deeper branches + // rarer — good enough for random generation. if self.pick().is_multiple_of(2) { self.emit(a, out, atomic); } else { @@ -956,19 +923,15 @@ mod generator { } } Expr::RepOnce(inner) => { - // formula.pest only uses `+` inside atomic rules - // (ASCII_DIGIT+), so inner repetitions never need - // whitespace separation. + // `+` occurs only inside atomic rules here (ASCII_DIGIT+), + // so inner repetitions never need whitespace separation. let count = 1 + self.pick() % 3; for _ in 0..count { self.emit(inner, out, atomic); } } - // Lookaheads (NegPred, PosPred) don't emit output. Any - // other Expr variant (Range, RepExact, Push, PeekSlice, - // Skip, …) is unused by formula.pest and silently - // produces nothing; if the grammar starts using one, - // generated formulas will fail to parse and we'll know. + // Lookaheads emit nothing, as does any Expr variant formula.pest + // does not use — generated formulas then fail to parse, loudly. _ => {} } } @@ -991,10 +954,8 @@ mod generator { "NEWLINE" => out.push('\n'), "SOI" | "EOI" => {} _ => { - // Every Ident in formula.pest refers to a rule that - // exists in the grammar file — if it doesn't, the - // grammar itself failed to compile and we wouldn't be - // running tests, so `expect` here is a bug marker. + // Every Ident names a rule in the grammar file, or the + // grammar would not compile: `expect` is a bug marker. let (ty, inner) = self .rules .get(name) diff --git a/crates/improvise-formula/src/quote.rs b/crates/improvise-formula/src/quote.rs index 3b687d22..968080b1 100644 --- a/crates/improvise-formula/src/quote.rs +++ b/crates/improvise-formula/src/quote.rs @@ -1,33 +1,12 @@ -//! CL-style `|…|` quoting of names — both halves of the convention -//! `formula.pest`'s `bare_ident` / `pipe_quoted` defines, and the one -//! `improv.pest`'s `bare_name` / `pipe_quoted` mirrors character for -//! character. -//! -//! This lives here because `improvise-formula` owns the grammar that says what -//! a bare name is, and because it is what every emitter needs: a name that came -//! from a user's data has to be spelled so the parser hands it back unchanged. -//! `improvise-gp` and `improvise-io` each carried their own copy, and all -//! three had drifted over the keyword rule below (improvise-i2m5). -//! -//! The two crates do not want the same entry point, so there are three: -//! [`quote_name`] for a name, [`pipe_quote`] for a value that must be quoted -//! whatever it looks like (a `.improv` text cell, so it cannot be mistaken for -//! a number), and [`unescape_pipe_body`] for reading either back. The escape -//! table is written once, in the last two. +//! CL-style `|…|` quoting of names: the one home of both halves, shared by +//! `formula.pest` and `improv.pest`. See `context/code/formula-and-io.md`. /// Words the formula grammar gives a meaning of their own. A name spelled like /// one is pipe-quoted even though it is otherwise a legal bare identifier. const KEYWORDS: [&str; 7] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF"]; -/// Whether `name` may be written without pipes: it matches -/// `[A-Za-z_][A-Za-z0-9_-]*` and is not one of the [`KEYWORDS`]. -/// -/// The keyword clause is deliberately stricter than the grammar strictly -/// requires. `SUM` does parse bare today, but only because `agg_call` needs a -/// `(` and PEG backtracking then falls through to `ref_expr` — an emergent -/// property of rule ordering, not a stated rule. A pipe-quoted name is an -/// identifier in every position with nothing to fall through, so a name that -/// collides with a keyword is quoted and stops depending on that. +/// Bare-writable: matches `[A-Za-z_][A-Za-z0-9_-]*` and is not a [`KEYWORDS`] +/// entry. The keyword clause is canonical — `context/code/formula-and-io.md`. pub fn is_bare_name(name: &str) -> bool { let mut chars = name.chars(); let starts = chars @@ -47,9 +26,8 @@ pub fn quote_name(name: &str) -> String { pipe_quote(name) } -/// `s` pipe-quoted whatever it looks like. `.improv` text cell values go -/// through this rather than [`quote_name`], because a bare `42` there would -/// read back as a number. +/// `s` pipe-quoted whatever it looks like. `.improv` text cell values go here +/// and not through [`quote_name`]: a bare `42` would read back as a number. pub fn pipe_quote(s: &str) -> String { let mut out = String::with_capacity(s.len() + 2); out.push('|'); @@ -65,14 +43,8 @@ pub fn pipe_quote(s: &str) -> String { out } -/// The inverse of [`pipe_quote`], over the body between the delimiters — -/// what both grammars' `pipe_inner` matches. `\|` → `|`, `\\` → `\`, -/// `\n` → newline; any other `\X` is preserved verbatim. -/// -/// The trailing arms are unreachable from either grammar: `pipe_inner` is -/// `("\\" ~ ANY | !"|" ~ ANY)*`, so a backslash is always followed by a -/// character. They are written as a total function rather than a panic so -/// that a caller reaching this by some other route gets its bytes back. +/// The inverse of [`pipe_quote`] over the body between the delimiters. Total +/// by design; its trailing arms are unreachable from either grammar. pub fn unescape_pipe_body(body: &str) -> String { let mut out = String::with_capacity(body.len()); let mut chars = body.chars(); @@ -130,8 +102,7 @@ mod tests { } /// The reading half inverts the writing half over every escape the - /// grammars define, plus the `\X` passthrough neither of them assigns a - /// meaning. + /// grammars define, plus the passthrough neither of them assigns a meaning. #[test] fn unescaping_inverts_quoting() { for s in [ @@ -151,20 +122,15 @@ mod tests { assert_eq!(unescape_pipe_body(r"\q"), r"\q"); } - /// Before improvise-i2m5 the unescape table existed twice — here (as - /// `parser.rs::unquote_pipe`) and as - /// `improvise-io/src/persistence/quoting.rs::unescape_pipe`. The two - /// agreed on every input either grammar's `pipe_inner` can produce; they - /// differed only on a trailing lone backslash, which `pipe_inner` cannot - /// emit. This pins the surviving answer for that input. + /// Pins the surviving answer for an input neither grammar can emit — see + /// `context/test-evidence/formula-and-io.md` (improvise-i2m5). #[test] fn a_trailing_lone_backslash_is_kept() { assert_eq!(unescape_pipe_body(r"a\"), r"a\"); } - /// The property the whole module exists for: a quoted name parses, and the - /// parser hands back the name that went in — as a target and as a - /// reference, which after the target-unquoting fix are the same rule. + /// The property the module exists for: a quoted name parses, and the parser + /// hands back the name that went in — as a target and as a reference. #[test] fn a_quoted_name_round_trips_through_the_parser() { for name in [