# `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).