The two generator strategies re-parsed the grammar inside their
prop_map, once per generated case: the same defect improvise-8shu fixed
in improvise-io. A OnceLock takes the lib test binary from ~0.9s to
~0.06s on the same machine and load.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
tt22 removed the formula crate's serde dependency with the legacy JSON loader,
keeping serde only where a type crosses the wire. FormulaId crosses the wire, so
it is the third such type. No derive goes back on the formula AST.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
Enter or e in the formula panel opens FormulaEdit on the formula under the
cursor, seeded with its text. The editor names the formula by a FormulaId
captured when it opens, and commits through a new ReplaceFormula effect and
ModelCommand. The edit keeps the formula's position, renames it in place when
the target changes, and refuses out loud if the formula was removed or
rewritten meanwhile, if the new target belongs to another formula, or if the
text does not parse. A refusal leaves the editor open over the user's text.
FormulaCache now carries each formula's id, so the browser composes the same
edit, and a formula-row click opens the editor on the clicked row.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded
`,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause
is the format, not the arithmetic; exact decimal math would not change the
rendering by one character, so nothing here touches the evaluator and gotcha 8
is unchanged.
Two halves, per the user's ruling:
* `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The
noise on whole numbers is the cheaper error — a currency column with two
redundant zeroes is legible, a ratio with none is wrong — and a format cannot
be guessed from a value.
* A formula may carry its own format, which wins for its target; a raw measure
keeps the view's. `formula.pest` gains
`format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s
reason (a predicate is invisible to the grammar-walking generator) and
`FORMAT` added to `quote::KEYWORDS`.
`improv.pest` is deliberately unchanged: `formula_raw` already carries the
`## Formulas` line verbatim, so the format rides in the formula text with
nothing to add — and the generator, which reads `improv.pest`, sees no new
spelling to violate.
Resolution has one home. `FormatPlan::of(model, view)` replaces the
`(comma, decimals)` pair every render path threaded — `GridLayout::display_text`,
`viewport::grid_body`/`compute_col_widths`, `project::grid_in` and
`persistence/export.rs::render_csv` — rather than being added beside it, which
would be two answers to "what format is this cell". It is asked of the cell's
`CellKey`, so a cell aggregating over `_Measure` keeps the view's format by
construction. `GridBody::col_formats` carries the answer to the totals strip,
which is formatted on its own path.
Persistence is untouched except for metadata: `format_f64` is display,
`persistence/format.rs::format_number` writes round-trip precision, and the two
have no call between them. Newly created views write `format: ,.2`; existing
documents are unaffected, because `format_md` has always written the line
explicitly. `web/demo.improv` moves to `,.2` on both views.
The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a
`FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip
test therefore asserts the parsed field — injecting a walker that drops the
clause fails that one test and nothing else in `improvise-io`, 242 tests
including the whole characterization suite and the grammar properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Integration review, cross-branch against #108. My asymmetry argument was right
-- a &str dispatch can only have GRAMMAR_INVARIANT, a Rule dispatch can have a
compile error -- and I then applied it to the two &str sites and NOT to the
Rule site where the stronger technique was available. build_primary kept its
unlabelled `else`.
Demonstrated, not argued. With `pct_literal = { "#" ~ identifier }` first in
`primary`: the tree built clean, all 85 crate tests passed including my three
new 492x guards, and parse_formula("X = #Revenue") returned Ref("Revenue") --
the `#` discarded, the formula silently wrong. The injection must be
NON-ATOMIC: an atomic rule is caught by accident in first_inner's own
GRAMMAR_INVARIANT and would have reported the code safe.
Sweeping for the shape found three more in the same file, two genuinely silent:
build_formula's if-chain-in-a-loop dropped an unhandled part (its comment said
"Rule::EOI and any silent rules are ignored"; EOI is now an ARM, so ignoring is
a decision), filter_value_to_string returned unprocessed text, and build_unary
was caught only incidentally downstream. All four now match over Rule with an
or-pattern over the rules that cannot appear, so ANY new rule in formula.pest
is a build failure at all four until someone decides what it means. Verified by
injecting three more probe rules: every site names all three.
The greppable invariant: `== Rule::` now appears nowhere in parser.rs.
each_primary_alternative_builds_its_own_expr is the anti-clamp guard -- a
mechanical if/else-to-match conversion can transpose two arms and still
compile, since the compiler checks that every rule is NAMED, never that it is
named against the right body. Proved by swapping the string and pipe_quoted
arms: it fails and nothing else in the suite does.
Also corrects context/code/formula-and-io.md, which still called the final
`else` arms "unlabelled-but-safe" on the grounds that COUNT was the only
remaining agg_func and >= the only remaining cmp_op. Two of those three clauses
were made false by my own earlier commit. Each was true OF THE GRAMMAR AS IT
STOOD -- a snapshot silently re-read as an invariant, which is the whole defect
in one sentence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-492x. parse_agg_func ended `_ => AggFunc::Count` and parse_cmp_op
ended `_ => BinOp::Ge`, so a new alternative in formula.pest would have parsed
as COUNT or as >= with no error: MEDIAN(Revenue) evaluating as a count.
One of three beads that are the same defect (with improvise-vyrx and
improvise-4lgj); design-principles §2 is the rule all three break. A compile
error is not on offer here, because agg_func is one pest rule over five string
alternatives and pest_derive generates no per-alternative Rule — so the walker
gets the idiom this file already uses for a grammar/walker desync,
GRAMMAR_INVARIANT, and the accepted set is written down in Rust as a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
The parameter was none_cats: &[String] and the body consulted it exactly once,
for emptiness -- a boolean wearing a list's type, since the aggregation runs
over key.0 through matching_values. git log -S back to the commit that
introduced Axis::None shows it never examined an element, so the name promised a
filter the function has never performed.
Aggregation::{Exact, OverHidden} admits only the two states the body
distinguishes, and Aggregation::of reduces the list once at the boundary that
holds it. The regression guard is the signature: two lists with the same
emptiness and different contents is no longer a constructible call.
recompute_formulas and aggregate_raw keep &[String] -- they walk the elements.
improvise-5jm5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
where_clause and inline_where were ordinary rules, so pest auto-inserted
WHITESPACE after the WHERE keyword and SUM(Rev WHEREAS x = y) parsed as a WHERE
on a category AS -- a silently wrong filter where it should have been a syntax
error. Both rules are now compound-atomic with sp+, exactly as is_expr.
A !bare_char lookahead with sp* would have preserved one more spelling, a
quoted category abutting the keyword, but the grammar-walking generator cannot
honour a predicate: it emits a bare identifier straight after the keyword and
generated_formula_parses goes red. sp+ is generatable and costs that one
spelling.
improvise-r08u
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Review of #54 found the fold reintroducing exactly the divergence the ruling
exists to remove, and the same line turned out to be the 244 ms stall. One
defect, two symptoms.
`eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at
`context.with(cat, name)`. Inside the fold `context` is a MEMBER-RECORD key
carrying the none_cat coordinates, while `formula_cache` is keyed by stems with
none_cats STRIPPED — so the lookup could never hit, fell through to
`aggregate_raw`, found no raw cell behind a formula target, and answered
Err("ref:…"). A routing formula over a derived measure gave Number(11.0)
uncached and Error("ref:Margin") cached, and the cached path is the one a pivot
grid runs whenever any category sits on Axis::None.
`eval_expr`/`eval_bool` are hoisted to module level and BOTH evaluators now hand
the fold the same record-level `eval_expr`. The two agree by construction rather
than by a test. Note the repair not taken: teaching the fold to hit the cache by
stripping none_cats off the member key returns the GROUP's value to every
member — aggregate-then-route through a cache lookup, and silent.
That same per-member `aggregate_raw` was one poorly-narrowed scan per member, an
O(n^2) shape. Measured with the new `examples/fold_bench.rs`, five runs a point,
at 18,050 cells: 51.1-53.3 ms before, 18.1-19.2 ms after, against 7.1-9.0 ms for
a linear formula. Scaling went from superlinear (45x cells, 85x time) to linear
(45x cells, 45x time). Debug 2.75 s -> 132.9-135.1 ms. The fixed point runs
exactly 2 passes, measured by counting, so hoisting the member set has a 2x
ceiling and is declined on improvise-9iyv — a memo keyed by less than the whole
`partial` is a second answer to what a cell depends on.
`Expr::Is` has an arm in each evaluator and the cached one was untested: the
earlier fault was injected into the shared `eval_is`, which both arms call.
Replacing the cached arm alone left the suite green. The first replacement test
ALSO stayed green, which is the interesting part — once the fold routes through
`eval_expr`, an `Is` inside a SUM never reaches the cached arm, so that arm is
reachable only by a bare `Is` outside an aggregate. That is what the new test
uses, and injecting into that arm alone now fails it.
`IS` gained a word boundary: `Region ISLAND` parsed as Is(Region, "LAND"),
silently wrong where it was a parse error. `is_expr` is compound-atomic with
`sp+` written out, because a negative lookahead in an ordinary rule tests the
character AFTER pest's auto-inserted whitespace and fails backwards. The
lookahead was then redundant and removed — deleting it left the suite green,
while relaxing `sp+` to `sp*` fails the test, so `sp+` is the mechanism. `WHERE`
has the same hole, is far less exposed, and is filed as improvise-r08u.
Also: `ambiguous.sort_unstable()` before `dedup`, which only removes consecutive
duplicates, so the refusal could name a column twice.
Every fault re-injected after the refactor: no-fold 10 failures, shared eval_is
5, record-level Is arm 4, cached Is arm 1, feature_roles order 2, context-vs-
partial 1, the new cached-fold divergence 1, `sp+` relaxed 1. Baseline 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-4zl7. A tree is fitted on individual records; an improvise formula is
evaluated at a CellKey, and an aggregating cell used to aggregate each Expr::Ref
first and evaluate the IF once on the totals. That is the same answer for a
linear formula and a different one for anything that routes, so every fitted
tree meant one thing on a record row and something else one row up. The user
ruled: aggregate the rule over the members.
The rule needed no new aggregate vocabulary. SUM(e) already means "fold e over
the members" and simply did not: eval_agg specialised Expr::Ref and, for every
other inner expression, summed the raw matching cells and ignored the expression
entirely. It now folds — one entry per member record, the expression evaluated
at that record's own coordinates. eval_agg is shared by eval_expr and
eval_expr_cached, so the fold has exactly one home; each evaluator passes its
own recursion in.
Expr::Is(Filter) is the new arm in both evaluators: `Cat IS Item` is 1.0 when
the cell's own Cat coordinate is Item, keeping the evaluator f64-only and
reusing Formula::filter's own context.get/unwrap_or(false). A grammar production
in primary, before ref_expr, with both operands through
improvise_formula::quote's one home; IS joins the keyword guard.
The emitter reads feature index -> role off preprocess's plan rather than
reconstructing it: gp_tree::data::feature_roles is the one statement of the
expansion order and encode_row now drives off it too. improvise_gp::Features
carries one Spelling per feature — Measure, Coordinate or Artefact — decided by
role and column, never by the name's shape. A one-hot over a real category is
emitted as `Region IS North` and accepted; col.missing and the date parts stay
Artefact and stay refused (improvise-wd6n).
fit-measure on a document a category explains now lands instead of refusing, and
the refusal no longer depends on which features the search happened to pick.
Evidence: eight hand-computed tests in improvise-core over a four-record fixture
whose Cost straddles the threshold inside each region, so route-then-aggregate
and aggregate-then-route differ at every level. Two faults injected and watched
to fail — reverting the fold fails 7 of 8, and a constant-false eval_is fails
exactly the 3 coordinate tests. Reordering feature_roles fails both it and
encode_row_replays_the_training_expansion.
The fold is O(members) per evaluation per fixed-point pass and unbenchmarked:
improvise-9iyv.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
improvise-io/src/persistence/quoting.rs was a third copy of the quoting
convention: is_bare_name/escape_pipe/quote_name/pipe_quote WITHOUT the
keyword guard, plus a second copy of the unquoting rule. Delete it and
route format.rs and parse.rs through improvise_formula::quote.
The keyword guard is now canonical, so a category or item literally named
SUM/WHERE/IF is written |SUM| rather than SUM. It is not a correctness
fix — bare keywords parse in every position — but a `## Formulas` line
carries formula text verbatim, so names written by format_md are read
back by parse_formula: the two grammars share one namespace and now one
spelling rule. The cost is cosmetic and the names round-trip either way,
improv.pest accepting pipe_quoted wherever a `name` goes.
Behaviour delta, measured rather than assumed: with the guard in, the
entire persistence suite stayed green except the characterization written
for this change. The only difference in emitted .improv is keyword-named
references. Section headers are untouched on purpose — `## Category: `
is followed by rest_of_line in the grammar, so a header never went
through quote_name on either side.
Unquoting is centralized too. The two copies agreed on every input either
grammar's pipe_inner can produce; they differed only on a trailing lone
backslash, which pipe_inner cannot emit (io returned the backslash,
parser.rs panicked on .expect(GRAMMAR_INVARIANT)). The surviving
unescape_pipe_body is total, and a_trailing_lone_backslash_is_kept pins
the answer.
Tests: three in roundtrip_tests (the spelling, the round trip across
every place a keyword name reaches the file, a formula targeting a
keyword-named measure) and two in quote.rs (unescaping inverts quoting,
the trailing-backslash divergence).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
The rule had three copies and two of them had already drifted:
improvise-io/src/persistence/quoting.rs has no keyword guard, the
improvise-gp copy does. improvise-formula owns the grammar that defines
what a bare name is and both other crates already depend on it, so the
writing half now lives beside the reading half as `quote::{quote_name,
is_bare_name}`. improvise-gp re-exports it and keeps its public surface.
Canonical behaviour: the keyword guard stays, so `SUM` is written
`|SUM|`. Probed first — the pest grammar does accept bare keywords in
every position (target, ref, aggregate argument, IF condition), so this
is not a correctness fix. It is accepted only because `agg_call` needs a
`(` and PEG backtracking falls through to `ref_expr`; that is rule
ordering, not a stated rule, and a pipe-quoted name is an identifier
everywhere with nothing to fall through. Cost is cosmetic.
improvise-io's copy is untouched — out of scope here, and switching it
would change `.improv` output for a keyword-named category. Reported as
a follow-up.
Faults injected to prove the new tests fire: dropping the keyword clause
fails a_keyword_is_quoted_even_though_it_looks_bare (and nothing else —
the parser round trip stays green, which is the probe's finding again);
dropping the `\\` escape fails the_escapes_match_the_grammar.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
`build_formula` took the target's raw span, so `|Total Revenue| = ...`
parsed to `Formula { target: "|Total Revenue|" }` — delimiters included —
while an `Expr::Ref` for the same name came back unquoted. The target
names a `_Measure` item, so `Model::add_formula` created an item named
literally `|Total Revenue|`: the formula wrote into a ghost column, the
user's own `Total Revenue` item stayed empty, and nothing referring to
the target by name resolved.
Both user-facing entry points hit this: the TUI formula minibuffer
(ui/effect.rs AddFormula) and `.improv` load (persistence/parse.rs).
No caller relied on the raw span — the writer emits `Formula::raw`, so
file round-tripping is unaffected, and the `_Measure` exclusion set in
persistence/format.rs now matches the real item instead of the ghost.
Fixed in improvise-formula rather than at the three call sites: the
target and a reference are the same syntactic identifier, so unquoting
them differently is the defect itself, and three copies of unquote_pipe
would be three chances to drift.
The grammar comment that documented the raw span is updated, and the two
tests that asserted the pipes (parser.rs, improvise-gp) now assert the
semantic round trip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
Pure relocation, no behaviour change. 2783 lines in one file became:
mod.rs the document lifecycle (save/load/gzip/legacy-JSON over
Storage) plus the pub facade re-exporting the rest
quoting.rs CL-style |…| quoting, the one thing format and parse share
format.rs format_md + format_number, persisted_initial_view, coord_str
parse.rs parse_md + ImprovParser/Rule, parse_number_cell, and the
parser property/edge-case tests and grammar generator
export.rs export_csv / export_csv_to / render_csv
roundtrip_tests.rs the format_md <-> parse_md characterization suite, which
belongs to neither side on its own
Every function moved with its tests where ownership was clear; the 34 tests
that exercise both directions went to roundtrip_tests.rs. `coord` and
`two_cat_model`, previously copied per test module, are now one `test_helpers`
module in mod.rs.
No test changed meaning: all 97 persistence tests still pass, and each moved
function body is identical modulo indentation and the visibility keyword on the
five quoting helpers (now pub(super)).
The public API is unchanged — save, load, save_to, load_from, autosave_path,
format_md, parse_md, export_csv and export_csv_to all still resolve through
`crate::persistence`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
model/types.rs carried four copy-pasted blocks across its two evaluation
paths (cached and uncached). All four copies were byte-identical — no
drift yet, but any change to aggregation or item lookup had to be made
twice to stay correct.
- apply_agg(values, func) — cell aggregation, shared by
evaluate_aggregated and aggregate_raw. Absorbs the callers' emptiness
guards so "empty means absent, not zero" is stated once.
- eval_agg(...) — formula-level SUM/AVG/MIN/MAX/COUNT, shared by
eval_expr_cached and eval_expr. This pair was a second instance of the
same defect, not named in either ticket but the same 27 duplicated
lines. It keeps its own emptiness rule (0 where an identity exists,
error otherwise) and now expresses that by delegating to apply_agg.
- Model::find_item_category — was a nested fn defined identically in
both eval paths.
Filter joins the improvise-formula re-exports so eval_agg can name it.
Tests cover each helper directly, including the deliberate emptiness
asymmetry between the two aggregation paths.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refactor formula parser tests to use more concise assert!(matches!(...))
syntax. Simplify the formula generator implementation by removing unused
expression variants and using expect() for mandatory grammar rules. Add a
regression test for hyphenated identifiers in bare names.
Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)
Refactor the formula parser to assume grammar invariants, replacing
Result-based error handling in tree walkers with infallible functions and
.expect(GRAMMAR_INVARIANT) calls.
This simplification is based on the guarantee that the Pest grammar already
validates the input structure. To verify these invariants and ensure
correctness, extensive new unit tests and property-based tests using
proptest have been added.
Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-31B-it-UD-Q4_K_XL.gguf)
Replace the manual tokenizer and recursive descent parser with a PEG
grammar using the pest library.
This migration involves introducing a formal grammar in formula.pest and
updating the parser implementation to utilize the generated Pest parser
with a tree-walking approach to construct the AST.
The change introduces a stricter requirement for identifiers: multi-word
identifiers must now be enclosed in pipe quotes (e.g., |Total Revenue|) and
are no longer accepted as bare words.
Tests have been updated to reflect the new parsing logic, remove
tokenizer-specific tests, and verify the new pipe-quoting and escape
semantics.
BREAKING CHANGE: Multi-word identifiers now require pipe-quoting (e.g. |Total Revenue|) and
are no longer accepted as bare words.
Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-31B-it-UD-Q4_K_XL.gguf)
Extract the formula AST and parser into a dedicated `improvise-formula`
crate and convert the project into a Cargo workspace.
The root crate now re-exports `improvise-formula` as `crate::formula` to
maintain backward compatibility for internal callers. The repository map is
updated to reflect the new crate structure.
Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-31B-it-UD-Q4_K_XL.gguf)
Edward Langley