diff --git a/Cargo.lock b/Cargo.lock index d1f6969c..b80ea5bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,7 +897,6 @@ dependencies = [ "pest_derive", "pest_meta", "proptest", - "serde", ] [[package]] @@ -997,8 +996,6 @@ checksum = "cc4e190f5d26ca7051642629da2c52fc03bde85a03197c99408dcd291734c855" dependencies = [ "equivalent", "hashbrown 0.17.1", - "serde", - "serde_core", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index bda00df3..259d8478 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,7 +45,7 @@ serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["float_roundtrip"] } anyhow = "1" thiserror = "2" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" chrono = { version = "0.4", features = ["serde"] } flate2 = "1" unicode-width = "^0.2" diff --git a/README.md b/README.md index 03ae07e1..76c15f5a 100644 --- a/README.md +++ b/README.md @@ -169,8 +169,9 @@ Measure: column format: ,.2f ``` -Gzip-compressed `.improv.gz` is also supported. Legacy JSON is auto-detected -for backward compatibility. +Gzip-compressed `.improv.gz` is also supported. `.improv` text is the only +native document format: the old JSON document format is no longer read, and +opening one fails with an error that says so. JSON *import* is unaffected. ## Import and scripting diff --git a/context/code/command.md b/context/code/command.md index c7985893..47c41a59 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3566,3 +3566,52 @@ On success the command's optimistic "Formula updated" stands, except on a rename, where the effect rewrites the status to name the old and new targets and any formula that still mentions the old one (`context/code/core.md` §18). +## A wire write reaches only a cell the host projects as a Record (improvise-ciw8) + +`App::resolve_record` answers "which record does this surrogate name". Its +fallback, `record_in_layout`, accepts any key the current layout names, and that +includes a collapsed group's fold and a key the None axis aggregates over. It +has to stay that permissive: it is also the basis check's question, and a record +nobody has stored yet (the empty cell a client is about to fill) is found only +there. But resolving is not the same as being writable. A hand-rolled frame +`SetCell { record: RecordHash::of({Month=Q2}), .. }` was answered `Accepted` and +stored a cell at `Month=Q2`, while the host's own projection called that +position `CellId::Aggregate`. A write under a formula (`Derived`) was accepted +the same way. + +So `model_effects` asks `App::unwritable` before a wire `SetCell` or `ClearCell` +builds its effect, and refuses through `effect::refuse`. That is the #181 path: +the refusal marks the batch, `Host::mutating_frame` undoes the frame, the asker +gets `Rejected(MutationRefused)`, and the reason is on the host's status line. +The question it asks is the projection's own, never a second derivation of it: + +- A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule, + `formula_for(&key)`. It is asked directly because a stored record may sit on + no position in the host's current view. +- A record found **in the store** is a record, whatever view anyone is on. + `RecordHash` is document-scoped (gotcha 44). +- A record found **only through the layout** is asked of `project::cell_id` at + the position of **its cell**: the one whose key is the whole key the write + names, record and measure together. `Aggregate`, a fold or a sum over a + None-axis category, is refused. It must be the whole key, not the first + position carrying the record (improvise-sjdb). `RecordHash` drops `_Measure`, + so every column of a row carries the same record, and with a collapsed + `_Measure` group the first of them can be the fold. That refused the empty + `Units` cell beside a `Money` fold, which the host itself projects as a + `Record`. A write whose whole key the host's grid does not draw is not + refused here, as it was not before this rule existed. + +**Why the `vec![]` arm stays.** A reviewer asked whether a `SetCell` whose cell +does not resolve at all, which `model_effects` turns into `vec![]` and so +applies nothing, should also be a `Rejection`. Through a `Host`, it already is, +before that arm is reached. A lone write naming a record that never existed gets +`RecordGone` up front, whether or not the basis claims it, because the create +exemption covers only a *later* command in a frame +(`a_wire_write_to_a_record_that_never_existed_is_rejected`). A later write whose +record nothing in the frame made resolvable gets `ChainBroken` from +`broken_chain` +(`a_frame_whose_write_would_land_nowhere_is_refused_rather_than_dropped`). So +the arm is unreachable from the wire. Making it a refusal would add a second +answer to "does this resolve" beside `broken_chain`, and that answer could only +ever fire after the first had already refused. It is left as the mutation +path's own behaviour for callers that do not go through the session door. diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 00f0835b..eacb06f1 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -471,8 +471,46 @@ 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. +document name, not of the filesystem (improvise-6mq). + +**`.improv` text is the only native format (improvise-tt22).** A legacy JSON +workbook used to be detected by a leading `{` and loaded read-only through +`Workbook`'s serde derives. That path is gone, on the user's direction on PR #210. +A document that still starts with `{` is refused with an error that names the +format and says it is no longer supported. The test is +`a_legacy_json_workbook_is_refused_by_name`. The error is decided rather than +defaulted, because otherwise `parse_md` would report a grammar error at line 1, +which diagnoses the wrong thing: the file is not malformed `.improv`. The check +is on the decompressed text, so `.improv.gz` is covered too. + +The serde derives that existed only for that path went with it: `Workbook`, +`Model`, `View`, `Category`, `Item`, `Group`, `CategoryKind`, `CellKey`, +`DataStore`'s manual impls and the formula AST. So did indexmap's `serde` feature +in `improvise-core`, `improvise-io` and the root crate, and `improvise-formula`'s +serde dependency. Each was removed and the whole workspace rebuilt, so the +compiler named every other consumer. Two remain, and both are the wire: +`Axis` (whose `rename_all = "lowercase"` is its wire spelling) and `PageAxis`. +`CellValue` was never a candidate, since it is carried on the wire too. JSON +*import* (`StartImportWizard`, `ImportJsonHeadless`) is a separate feature and is +untouched. + +One test consumer had to move: the effect-classification tests fingerprinted the +model and the views through `serde_json`. They now use `Debug`. `format_md` was +rejected as the replacement, because it skips the virtual views and sorts the +data, so it would have been a weaker fingerprint. + +The first version of this paragraph said `Debug` was "no weaker" because it +renders every field, including the derived `formula_cache` the JSON skipped. +**That was wrong (improvise-pc3b).** A derived `Debug` prints a `HashMap` in hash +order, and `recompute_formulas` clears and refills the cache. So a correct +`ViewOnly` effect such as `RebuildLayout` changed the fingerprint in 90 of 200 +runs. The suite passed only because the fixture had no formulas, so the cache +was empty. `Model` now has a hand-written `Debug` that renders `formula_cache` +and `measure_agg` in key order. The fingerprint keeps every field, including the +derived one, and a refill in a different hash order is no longer read as a +change. `recomputing_formulas_leaves_the_model_fingerprint_alone` pins this. +`view_only_effects_leave_shared_state_alone` now runs over a formula-bearing +fixture, for the same reason. `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails diff --git a/context/code/protocol.md b/context/code/protocol.md index b71cd8a3..a05c8999 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -863,7 +863,7 @@ others to inherit it works in `cargo test` and nowhere else. **Why "can parse" and not "parses a frame".** The first version of the rule covered only the three crates that parse frames and listed them by hand. Frames are not the only place a user's number crosses a JSON parser. -`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import +`improvise-io`'s legacy-JSON `.improv` load (removed since, improvise-tt22), `improvise-command`'s JSON import (`StartImportWizard`, `ImportJsonHeadless`) and the root binary's `improvise import` all parsed with the lossy default, and in the TUI binary nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285` diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 156de170..ac2a89b8 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5833,6 +5833,69 @@ on `Food/Jan/Profit`, where there is nothing to clear, and `Clothing/Jan/Profit = 5` survives. Injected alone into a full workspace run, the rival fails this test and nothing else. +### a_wire_write_to_a_collapsed_fold_is_rejected_and_stores_nothing + +improvise-ciw8, through a real `Host` frame. Q2 is collapsed, and a hand-rolled +client sends `SetCell` (then, separately, `ClearCell`) for `{Month=Q2, _Measure=Cost}` +with a caught-up basis. At `ea119bd9` both were answered `Accepted`, and the +`SetCell` stored `Month=Q2, _Measure=Cost = 7`, the non-item coordinate +improvise-gw18 exists to prevent. The test asserts `Rejected(MutationRefused)` +and an unchanged document for both. Rival: the aggregate refusal disabled. This +test and the None-axis test fail. The control is +`a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted`, so a host that refused +every wire write cannot pass. + +### a_wire_write_under_a_formula_is_rejected_and_stores_nothing + +improvise-ciw8's `Derived` arm: `SetCell` on Feb × Profit, where +`Profit = Revenue - Cost`. At `ea119bd9` it was accepted and stored a value under +the formula. Rival: the formula refusal disabled. Only this test fails. + +### a_wire_write_to_a_none_axis_aggregate_is_rejected_and_stores_nothing + +improvise-ciw8's other `Aggregate`. With `Product` on `Axis::None`, the grid's +key `{Region=South, _Measure=Revenue}` is a partial key that no stored record +has, so it resolves only through the layout, where `cell_id` says `Aggregate`. +At `ea119bd9` it was accepted and stored the partial key. It fails under the +same rival as the fold test. + +### a_wire_write_to_a_record_that_never_existed_is_rejected + +The evidence for keeping `model_effects`' `vec![]` arm rather than turning it +into a refusal (`context/code/command.md`, "A wire write reaches only a cell the +host projects as a Record"). A lone write naming a record that never existed is +answered `RecordGone` before anything runs, both when a caught-up basis claims +it and when a hand-rolled basis omits it. The second half was expected to be +`ChainBroken`, and the test said otherwise: the create exemption covers only +commands after the first in a frame. That surprise is why it is asserted rather +than argued. + +### an_empty_cell_beside_a_collapsed_measure_group_is_writable + +improvise-sjdb, a regression PR #217 introduced and its review found. `_Measure` +is on Column with Revenue and Cost in a collapsed group `Money` and `Units` +ungrouped, and the Feb row is empty. The host projects Feb × Units as a +`Record`. A caught-up `SetCell` on it was answered `Rejected(MutationRefused)` +at `bc060f67`. It was accepted before #217. `aggregate_refusal` found the +unstored record by `RecordHash` alone, which drops `_Measure`, so it judged the +record at the first column on Feb's row, the `Money` fold, which `cell_id` calls +`Aggregate`. The setup asserts that the fold comes before `Units` on the row and +that the target projects as `Record`, so a fixture where the first column is +harmless cannot pass. The fix looks the cell up by its whole key. The rival is +the old lookup by record, which is this test's red commit, `6e021699`. + +### a_stored_record_the_hosts_view_aggregates_is_writable + +improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is +added on `Axis::None` in the host's view, so the host projects South's position +as `Aggregate`. A stored record is a record wherever it is drawn, because +`RecordHash` is document-scoped (gotcha 44). A records-mode client, or one on +another view, may write it. The setup asserts the host really does project +`Aggregate` there, so the test cannot pass on a view that draws South plainly. +Rival: stored records sent through `cell_id` like unstored ones. That rival +fails this test and nothing else in `improvise-command`. Before this test, +nothing in the suite saw it. + ### a_fresh_sessions_slice_is_the_hosts_startup_slice improvise-v8ab. A fresh session's slice is built twice: `App::with_storage` @@ -5857,6 +5920,33 @@ that is deliberate. Its helpers seed a client *after* mutating the host (mode, buffers, cursor), so a `Snapshot` would drop exactly the state those tests stage. This test is what makes the copy faithful at startup. + +### ui::effect::tests::recomputing_formulas_leaves_the_model_fingerprint_alone + +improvise-pc3b, found by the review of PR #224 at `e62a289`. That PR moved the +effect-classification fingerprints from `serde_json` to `Debug`. The derived +`Debug` for `Model` prints `formula_cache`, a `HashMap` that `recompute_formulas` +clears and refills, so it prints in hash order. The reviewer measured a correct +`ViewOnly` effect (`RebuildLayout`) changing the fingerprint in 90 of 200 runs +once formulas were present. `test_app` has none, so the suite was green over a +latent flake. + +The test builds `formula_app` (two measures across a 2×2 grid, plus +`Profit = Revenue - Cost` and `Margin = Profit / Revenue`) 32 times. Each build +is a fresh `App`, so each gets fresh hash states. Each time, it asserts that +`RebuildLayout` leaves `model_fingerprint` unchanged. One flip in about two makes +32 independent tries fail near-certainly. + +**Red before the fix**: the two renderings differed only in `formula_cache`'s +entry order. The fix is a hand-written `Debug` for `Model` that renders +`formula_cache` and `measure_agg` in key order. + +**Rival, run:** keep the hand-written `Debug` but print `&self.formula_cache` +unsorted. This test fails. `view_only_effects_leave_shared_state_alone` now runs +over `formula_app` too, so every `ViewOnly` case is checked against a populated +cache. The two earlier injections (a `SetStatus` that writes the model, and one +that writes a view's `number_format`) still fail it. + ### a_document_opening_on_a_records_view_opens_the_host_in_records_normal improvise-y5eo. `App::with_storage` set `view_state.mode` to `Normal` and never diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index d60ca91f..26c16963 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -476,12 +476,6 @@ cell in the row). The pair `live_records` yields is not two facts that could drift apart: the surrogate is the record's own, so it is the one that resolves it back. -### a_deserialized_store_can_resolve_its_records - -The record index is derived state and is `#[serde(skip)]`-equivalent — it must -come back *with* the data, rebuilt through `set`, rather than being written -alongside it. - ## `model/record.rs` ### a_records_surrogate_is_pinned_to_a_literal @@ -1481,6 +1475,33 @@ because the depth bound the fold path used before is gone. Without the marker the evaluation recurses until the stack runs out, which is why the marker is there rather than being an optimisation. +### one_formula_at_two_fold_keys_is_evaluated_at_each + +improvise-d7aj, found by the adversarial re-review of PR #203 at `b3379d1a`. The +fold memo in `fold_formula_once` is keyed by the whole `CellKey`, and nothing +pinned that: both tests above keep one key context throughout, so a memo keyed +by the formula alone passed the whole workspace. A reference to an item of +**another** category moves the key while `_Measure` names the same formula. +Stores A (Jan 3, Feb 4) and B (Jan 100, Feb 200), `G = Revenue * 2`, +`F = IF(Revenue > 50, G, G + B)`. At `(Q1, A, F)` Revenue is 7, so F is +`G + B`: G at A is 14, and `B` reads F at `(Q1, B)`, where Revenue is 300, so +that F is G at B, 600. The answer is **614**. Rival: the memo keyed by +`formula.target` alone. F is still in progress at `(Q1, A)` when F at `(Q1, B)` +is asked, so that key reads `circular`. Injected into a full workspace run, it +fails this test and nothing else. The reviewer's probe, now committed. + +### a_diamond_at_a_fold_is_not_circular + +improvise-d7aj's second case, which passed at `b3379d1a` and was pinned by +nothing. `A1 = B2 + C3`, `B2 = D * 2`, `C3 = D * 3`, `D = Revenue` reaches D by +two paths in one fold evaluation, so the second reach must find D *finished* +and not *in progress*. It asserts **35** over D = 7 and exactly four +evaluations, so a memo that re-evaluates D passes neither. Rival: never store a +finished value, so only the in-progress marker stays. This test fails with +`circular`, and so does `a_fold_evaluates_each_formula_it_reaches_once`, whose +doubling chain is also a diamond at every step. So this test is a named pin of +that shape rather than the only guard against it. + ### a_fold_answers_is_and_where_on_its_category_as_the_none_axis_does improvise-lnkb, ruled by the orchestrator: on a fold, the folded category reads diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 48e8bd5a..536effb1 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1435,16 +1435,34 @@ reintroduce the iot6 divergence between what the user sees and what they export. On the pre-6qet tree the body is empty (the group had no row, and pruning left nothing), so the test fails there. -## Legacy JSON numbers (improvise-6yvg) +## Legacy JSON documents (improvise-6yvg, improvise-tt22) -### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit` +`a_legacy_json_workbook_loads_its_numbers_bit_for_bit` (improvise-6yvg) pinned +that the legacy JSON load path read numbers exactly. improvise-tt22 removed that +path, so the test went with it. Its other role was to witness `improvise-io`'s own +`float_roundtrip` declaration. The manifest rule in +`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`) +still covers that, because `improvise-io` keeps a normal `serde_json` +dependency for its import types (`context/test-evidence/tooling.md`). -A workbook with Food/Jan holding `0.24815714285714285` is serialized with -`serde_json` and loaded through `load_from`. A leading `{` sends it down the -legacy JSON path. The test first asserts that the text holds the exact decimal, -so what it measures is the parse. It compares `f64::to_bits`. Found by the -review of PR #190, not by the ticket. +### `a_legacy_json_workbook_is_refused_by_name` -**Red at `1158c23`** in `cargo test -p improvise-io`: loaded -`0.24815714285714283`. Green once `improvise-io`'s own manifest enables -`float_roundtrip`, and red again with only that line reverted. +improvise-tt22, on the user's direction on PR #210: `.improv` text is the only +native format. A document starting with `{` used to be deserialized as a +`Workbook`. The test writes a JSON workbook in the shape the old loader read, +both plain and gzipped, and asserts the error names "legacy JSON" and says "no +longer supported". + +**Red at `52c6a05`**: `Failed to deserialize workbook: missing field +`next_category_id``. That is a parse error, which is exactly what the ticket said +the refusal must not be. A complete legacy document instead loaded successfully, +which the since-deleted bit-for-bit test above demonstrated. + +**Rivals, each run:** + +- **Deleting the refusal.** The text falls through to `parse_md`, and the test + fails on `legacy.improv` with "Parse error at line 1, column 1 (expected + version_line)". A parse error does not name the format. +- **Refusing only when the path is not gzipped.** The plain case passes and the + test fails on `legacy.improv.gz` with the same grammar error. So the gzip + case is what makes the check prove it runs on decompressed text. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 820e622c..c6845bd5 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -696,8 +696,9 @@ every target-specific entry, whose **normal** serde_json dependency lacks **Faults injected, each run:** - **The feature removed from `improvise-io` alone.** The check fails naming - `improvise-io (all targets)`, and so does - `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`. + `improvise-io (all targets)`. So did + `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`, until improvise-tt22 + removed the legacy load path and that test with it. `a_json_import_stores_its_numbers_bit_for_bit` stays green, because `improvise-command` enables the feature and unification reaches io in its build. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 080a27b0..48c7165e 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -428,9 +428,31 @@ inputs. This test is aimed at exactly that gap. **It is deliberately a directory walk rather than a list of names**: a list is a second place to remember, and the file that gets forgotten is the one nobody -added to it. The scope is the two directories `.gitignore` un-ignores — -`examples/` and `web/`; everything else matching `*.improv` is somebody's -working file. +added to it. The scope is the directories `.gitignore` un-ignores `*.improv` +in. Everything else matching `*.improv` is somebody's working file. + +**The directory list is read from `.gitignore`, not written in the test +(improvise-qlrh).** The test used to hard-code `["examples", "web"]`, a list of +exactly the kind the paragraph above warns against. When improvise-plcx +un-ignored `harness/fixtures/*.improv`, nothing added that directory to the +walk. Its two fixtures were then loaded only by the Playwright job, where a +missing seed degrades to an empty model and surfaces as a missing-label +failure, not a load error. `shipped_directories` now takes every +`!/*.improv` line, so un-ignoring a directory is what enlists it. + +The precondition is per directory: each derived directory must yield at least +one document. The old whole-walk `>= 2` could not see a directory silently lost. + +**Red, measured at `705c92d`:** `harness/fixtures/tall.improv` was replaced with +`{"legacy": true}`, and the old test still passed. **Rivals, each run against +the new test:** + +- **That same broken fixture.** Fails with "…/harness/fixtures/tall.improv does + not load". +- **A walk that drops every `harness` path.** Fails the per-directory + precondition, naming `harness/fixtures`. +- **A derivation that matches no `.gitignore` line.** Fails "…must un-ignore at + least one *.improv directory". **It matches on the full name, not on `Path::extension()`**: `.improv.gz` is the same document gzipped and `extension()` calls it `gz`, so matching on the diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 00123c52..9a8a7353 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -27,7 +27,7 @@ use crate::ui::viewport::{compute_col_widths, compute_row_header_width, compute_ use crate::view::{GridLayout, View}; use crate::workbook::Workbook; use improvise_core::model::record::{AggregateHash, RecordHash}; -use improvise_protocol::RecordCell; +use improvise_protocol::{CellId, RecordCell}; /// A cell's coordinate tuple with `_Measure` dropped: the unit of atomicity /// ADR 001 settles on, and not the cell — see `context/code/command.md`. @@ -576,16 +576,56 @@ impl App { .map(|key| record_of(&key)) } + /// Why a wire write to `cell` is refused: the host itself projects it as no + /// `CellId::Record` (improvise-ciw8, ADR 002). `None` for a writable cell. + pub fn unwritable(&self, cell: &RecordCell) -> Option { + let key = self.resolve_cell(cell)?; + let model = &self.model_state.workbook.model; + let derived = model.formula_for(&key).map(|f| { + format!( + "{} is computed by a formula — edit the formula, not the cell", + f.target + ) + }); + derived.or_else(|| self.aggregate_refusal(cell.record, &key)) + } + + /// A record the store does not hold is known only by its layout position, and + /// is a record only where the projection at ITS CELL says so (improvise-sjdb). + fn aggregate_refusal(&self, hash: RecordHash, key: &CellKey) -> Option { + let stored = self.model_state.workbook.model.data.resolve_record(hash); + let (row, col) = stored.map_or_else(|| self.cell_position(key), |_| None)?; + let projected = + crate::ui::project::cell_id(&self.model_state.workbook.model, &self.layout, row, col); + matches!(projected, Some(CellId::Aggregate(_))).then(|| { + "an aggregate combines several records, so there is no one cell to write".into() + }) + } + + /// Where in the current layout the cell `key` sits, if anywhere. By the whole + /// key: a `RecordHash` drops `_Measure`, so a row's columns all share one. + fn cell_position(&self, key: &CellKey) -> Option<(usize, usize)> { + self.layout_cells() + .find(|(_, _, at)| at == key) + .map(|(row, col, _)| (row, col)) + } + /// Every cell key the current grid names. Records mode is excluded: its /// `cell_key` carries *positions*, the trap ADR 002 rejected `_Index` for. fn layout_cell_keys(&self) -> impl Iterator + '_ { + self.layout_cells().map(|(_, _, key)| key) + } + + /// [`Self::layout_cell_keys`] with each key's position. + fn layout_cells(&self) -> impl Iterator + '_ { let layout = &self.layout; let rows = match layout.is_records_mode() { true => 0, false => layout.row_count(), }; - (0..rows) - .flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c))) + (0..rows).flat_map(move |r| { + (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c).map(|k| (r, c, k))) + }) } /// Point **this client's** slice at `name`, if the document has such a view, diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 87838a4a..e8fe85d3 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1458,16 +1458,67 @@ mod tests { // ── Effect classification (improvise-mae) ─────────────────────────── - /// Serialized model data — everything a connected client renders. - /// Excludes the view slice, which is per-client. + /// Every field of the model, rendered — everything a connected client + /// renders. `Debug`, not serde: the model has no serde form (improvise-tt22). fn model_fingerprint(app: &App) -> String { - serde_json::to_string(&app.model_state.workbook.model).expect("model serializes") + format!("{:?}", app.model_state.workbook.model) } - /// Serialized view configuration. Kept for the NEGATIVE arm only, where a + /// Every field of every view. Kept for the NEGATIVE arm only, where a /// hash stricter than `document_config` is wanted (improvise-1xf4). fn view_config_fingerprint(app: &App) -> String { - serde_json::to_string(&app.model_state.workbook.views).expect("views serialize") + format!("{:?}", app.model_state.workbook.views) + } + + /// `test_app` plus measures, data and two chained formulas, so the derived + /// formula cache holds several entries for a recompute to refill. + fn formula_app() -> App { + let mut app = test_app(); + let wb = &mut app.model_state.workbook; + for m in ["Revenue", "Cost"] { + wb.add_item("_Measure", m).unwrap(); + } + let mut n = 1.0; + for t in ["Food", "Clothing"] { + for mo in ["Jan", "Feb"] { + for (m, scale) in [("Revenue", 10.0), ("Cost", 3.0)] { + let key = CellKey::new(vec![ + ("Type".into(), t.into()), + ("Month".into(), mo.into()), + ("_Measure".into(), m.into()), + ]); + wb.model.set_cell(key, CellValue::Number(n * scale)); + } + n += 1.0; + } + } + for raw in ["Profit = Revenue - Cost", "Margin = Profit / Revenue"] { + AddFormula { + raw: raw.into(), + target_category: "_Measure".into(), + } + .apply(&mut app); + } + app.rebuild_layout(); + app + } + + /// improvise-pc3b: the fingerprint must not read a derived cache's hash + /// order. Evidence: `context/test-evidence/command.md`. + #[test] + fn recomputing_formulas_leaves_the_model_fingerprint_alone() { + // Each App gets fresh hash states; one run observed ~45% flips, so 32 + // independent tries make a hash-ordered fingerprint fail near-certainly. + for _ in 0..32 { + let mut app = formula_app(); + let before = model_fingerprint(&app); + RebuildLayout.apply(&mut app); + assert_eq!( + model_fingerprint(&app), + before, + "a ViewOnly recompute changed the model fingerprint" + ); + } } /// improvise-mae: rather than restate each declaration, apply the effect and @@ -1507,7 +1558,8 @@ mod tests { EffectKind::ViewOnly, "{effect:?} is in the ViewOnly case list" ); - let mut app = test_app(); + // With formulas, so `RebuildLayout` refills a non-empty cache (pc3b). + let mut app = formula_app(); let model_before = model_fingerprint(&app); let views_before = view_config_fingerprint(&app); effect.apply(&mut app); diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 78aac189..86cd8769 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1358,6 +1358,260 @@ mod tests { ); } + fn month_cell(month: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Month".to_string(), month.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]) + } + + /// Months in two quarters on Row, Revenue/Cost and `Profit = Revenue - Cost` on + /// Column, every month funded, Q2 collapsed: rows `[Jan, Feb, Q2]`. + fn host_on_a_fold() -> Host { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + for measure in ["Revenue", "Cost"] { + wb.add_item("_Measure", measure).unwrap(); + let value = CellValue::Number(10.0); + wb.model.set_cell(month_cell(month, measure), value); + } + } + let profit = crate::formula::parse_formula("Profit = Revenue - Cost", "_Measure"); + wb.model.add_formula(profit.unwrap()); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("Month", "Q2"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert!( + app.layout.folds_a_group(2, 1), + "setup: row 2 is the Q2 fold" + ); + Host::new(app) + } + + /// One wire write, as a hand-rolled client composes it: a surrogate built from + /// a key, and a basis claiming whatever the host says is current. + fn wire_write(host: &mut Host, key: &CellKey, clear: bool) -> Vec { + use improvise_protocol::ModelCommand; + let cell = RecordCell { + record: RecordHash::of(key), + measure: key.get("_Measure").map(String::from), + }; + let cmd = match clear { + true => ModelCommand::ClearCell(cell), + false => ModelCommand::SetCell { + cell, + value: CellValue::Number(7.0), + }, + }; + let (asker, _) = host.connect(10, 10); + let frame = crate::ui::reduce::caught_up(host.app(), cmd); + host.handle(asker, ClientFrame::Commands(vec![frame])) + } + + fn refused(answer: &[Addressed]) -> bool { + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::MutationRefused))) + } + + fn document_text(host: &Host) -> String { + crate::persistence::format_md(&host.app().model_state.workbook) + } + + /// improvise-ciw8 — see context/test-evidence/command.md. + #[test] + fn a_wire_write_to_a_collapsed_fold_is_rejected_and_stores_nothing() { + for clear in [false, true] { + let mut host = host_on_a_fold(); + let before = document_text(&host); + let answer = wire_write(&mut host, &month_cell("Q2", "Cost"), clear); + assert!(refused(&answer), "clear={clear}: not refused: {answer:?}"); + assert_eq!( + document_text(&host), + before, + "clear={clear}: the fold was written" + ); + } + } + + /// improvise-ciw8: a formula's result is `CellId::Derived`, no write's target. + #[test] + fn a_wire_write_under_a_formula_is_rejected_and_stores_nothing() { + let mut host = host_on_a_fold(); + let before = document_text(&host); + let answer = wire_write(&mut host, &month_cell("Feb", "Profit"), false); + assert!(refused(&answer), "not refused: {answer:?}"); + assert_eq!( + document_text(&host), + before, + "a value was stored under the formula" + ); + } + + /// improvise-ciw8: a key the None axis aggregates over is `CellId::Aggregate`. + #[test] + fn a_wire_write_to_a_none_axis_aggregate_is_rejected_and_stores_nothing() { + let mut host = host(); + host.app + .model_state + .workbook + .add_category("Product") + .unwrap(); + host.app + .model_state + .workbook + .add_item("Product", "Widget") + .unwrap(); + let view = host.app.model_state.workbook.active_view_mut(); + view.set_axis("Product", Axis::None); + host.app.rebuild_layout(); + let before = document_text(&host); + let answer = wire_write(&mut host, &cell("South"), false); + assert!(refused(&answer), "not refused: {answer:?}"); + assert_eq!(document_text(&host), before, "the partial key was written"); + } + + /// improvise-ciw8's second question: a write naming a record that never existed + /// is refused by the frame, not dropped — see context/test-evidence/command.md. + #[test] + fn a_wire_write_to_a_record_that_never_existed_is_rejected() { + use improvise_protocol::ModelCommand; + let mut host = host_on_a_fold(); + let before = document_text(&host); + // Claimed by a caught-up basis: structural staleness, answered up front. + let answer = wire_write(&mut host, &month_cell("Nowhere", "Cost"), false); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))), + "claimed: {answer:?}" + ); + // Unclaimed: the create exemption covers only LATER commands in a frame, + // so a lone write is answered up front too. + let key = month_cell("Nowhere", "Cost"); + let cmd = ModelCommand::SetCell { + cell: RecordCell { + record: RecordHash::of(&key), + measure: Some("Cost".into()), + }, + value: CellValue::Number(7.0), + }; + let basis = improvise_protocol::Basis { + document_generation: host.app().document_generation(), + layout_version: host.app().layout_version(), + records: vec![], + }; + let (asker, _) = host.connect(10, 10); + let frame = ClientFrame::Commands(vec![Command::Model { basis, cmd }]); + let answer = host.handle(asker, frame); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))), + "unclaimed: {answer:?}" + ); + assert_eq!(document_text(&host), before); + } + + /// The control for the three above: the stored cell beside the fold is written. + #[test] + fn a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted() { + let mut host = host_on_a_fold(); + let answer = wire_write(&mut host, &month_cell("Feb", "Cost"), false); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)), + "{answer:?}" + ); + let model = &host.app().model_state.workbook.model; + let value = model.get_cell(&month_cell("Feb", "Cost")); + assert_eq!(value, Some(&CellValue::Number(7.0))); + } + + /// improvise-sjdb: an empty record is judged at its own cell, not at the first + /// column carrying it — see context/test-evidence/command.md. + #[test] + fn an_empty_cell_beside_a_collapsed_measure_group_is_writable() { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for month in ["Jan", "Feb"] { + wb.add_item("Month", month).unwrap(); + } + for measure in ["Revenue", "Cost"] { + wb.add_item_in_group("_Measure", measure, "Money").unwrap(); + } + wb.add_item("_Measure", "Units").unwrap(); + let jan = month_cell("Jan", "Revenue"); + wb.model.set_cell(jan, CellValue::Number(10.0)); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut() + .toggle_group_collapse("_Measure", "Money"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + let target = month_cell("Feb", "Units"); + let units = (0..app.layout.col_count()) + .find(|&c| app.layout.cell_key(1, c).as_ref() == Some(&target)) + .expect("setup: Feb x Units is drawn"); + let fold = (0..app.layout.col_count()) + .find(|&c| app.layout.folds_a_group(1, c)) + .expect("setup: a folded measure column"); + assert!(fold < units, "setup: the fold comes first on Feb's row"); + let id = + crate::ui::project::cell_id(&app.model_state.workbook.model, &app.layout, 1, units); + let projected = matches!(id, Some(improvise_protocol::CellId::Record(_))); + assert!(projected, "setup: Feb x Units projects as {id:?}"); + let mut host = Host::new(app); + let answer = wire_write(&mut host, &target, false); + let accepted = answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)); + assert!(accepted, "a projected Record was refused: {answer:?}"); + let model = &host.app().model_state.workbook.model; + assert_eq!(model.get_cell(&target), Some(&CellValue::Number(7.0))); + } + + /// A STORED record is writable whatever the host's view does to it — see + /// context/test-evidence/command.md (improvise-sjdb). + #[test] + fn a_stored_record_the_hosts_view_aggregates_is_writable() { + let mut wb = workbook(); + wb.model.set_cell(cell("South"), CellValue::Number(1.0)); + wb.add_category("Product").unwrap(); + wb.add_item("Product", "Widget").unwrap(); + wb.active_view_mut().set_axis("Product", Axis::None); + let mut app = App::new(wb, None); + app.rebuild_layout(); + let south = (0..app.layout.row_count()) + .flat_map(|r| (0..app.layout.col_count()).map(move |c| (r, c))) + .find(|&(r, c)| app.layout.cell_key(r, c).as_ref() == Some(&cell("South"))) + .expect("setup: South's cell is drawn"); + let id = crate::ui::project::cell_id( + &app.model_state.workbook.model, + &app.layout, + south.0, + south.1, + ); + let aggregate = matches!(id, Some(improvise_protocol::CellId::Aggregate(_))); + assert!(aggregate, "setup: the host's view aggregates South: {id:?}"); + let mut host = Host::new(app); + let answer = wire_write(&mut host, &cell("South"), false); + let accepted = answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)); + assert!(accepted, "a stored record was refused: {answer:?}"); + let model = &host.app().model_state.workbook.model; + assert_eq!( + model.get_cell(&cell("South")), + Some(&CellValue::Number(7.0)) + ); + } + /// Clause 4 must not fire on a frame that merely names a record it is /// creating. See `context/test-evidence/command.md`. #[test] diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index a8658c9d..37d7e5db 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -135,18 +135,20 @@ pub fn apply_command(app: &mut App, cmd: Checked<'_>) -> Dirty { /// was derived from (improvise-kmgs). Exhaustive on purpose. pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { let one: Box = match cmd { + // A cell the host projects as no `Record` is refused, so the frame is + // Rejected rather than written (improvise-ciw8, ADR 002). ModelCommand::SetCell { cell, value } => { - let Some(key) = app.resolve_cell(cell) else { - return vec![]; - }; - Box::new(effect::SetCell(key, value.clone())) - } - ModelCommand::ClearCell(cell) => { - let Some(key) = app.resolve_cell(cell) else { - return vec![]; - }; - Box::new(effect::ClearCell(key)) + match (app.unwritable(cell), app.resolve_cell(cell)) { + (Some(why), _) => effect::refuse(why), + (None, None) => return vec![], + (None, Some(key)) => Box::new(effect::SetCell(key, value.clone())), + } } + ModelCommand::ClearCell(cell) => match (app.unwritable(cell), app.resolve_cell(cell)) { + (Some(why), _) => effect::refuse(why), + (None, None) => return vec![], + (None, Some(key)) => Box::new(effect::ClearCell(key)), + }, ModelCommand::AddItem { category, item } => Box::new(effect::AddItem { category: category.clone(), item: item.clone(), diff --git a/crates/improvise-core/Cargo.toml b/crates/improvise-core/Cargo.toml index 886ece4a..790fb647 100644 --- a/crates/improvise-core/Cargo.toml +++ b/crates/improvise-core/Cargo.toml @@ -9,7 +9,7 @@ repository = "https://github.com/fiddlerwoaroof/improvise" [dependencies] improvise-formula = { path = "../improvise-formula" } anyhow = "1" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" serde = { version = "1", features = ["derive"] } # SipHash-2-4 under a constant key: the record surrogate must not move # between processes, which rules out anything built on `RandomState` diff --git a/crates/improvise-core/src/model/category.rs b/crates/improvise-core/src/model/category.rs index 500ad680..e915e37f 100644 --- a/crates/improvise-core/src/model/category.rs +++ b/crates/improvise-core/src/model/category.rs @@ -1,10 +1,9 @@ use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; pub type CategoryId = usize; pub type ItemId = usize; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Item { pub id: ItemId, pub name: String, @@ -27,7 +26,7 @@ impl Item { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Group { pub name: String, /// Parent group name for nested hierarchies @@ -50,7 +49,7 @@ impl Group { /// Regular categories store their items explicitly; virtual ones are /// synthesized at query time by the layout layer. context/code/core.md §9. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] pub enum CategoryKind { #[default] Regular, @@ -125,7 +124,7 @@ impl CategoryKind { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Category { pub id: CategoryId, pub name: String, @@ -136,7 +135,6 @@ pub struct Category { /// Next item id counter next_item_id: ItemId, /// Whether this is a regular or virtual category - #[serde(default)] pub kind: CategoryKind, } diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index 0b143831..7a10edf1 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -7,7 +7,7 @@ use super::symbol::{Symbol, SymbolTable}; /// A cell key: (category, item) pairs, always sorted by category name. The /// constructors are the only way to build one — context/code/core.md §1. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct CellKey(pub Vec<(String, String)>); impl CellKey { @@ -194,8 +194,6 @@ impl RecordEntry { } } -/// Serialized as a list of (key, value) pairs, so `CellKey` need not -/// serialize as a string for JSON object keys. #[derive(Debug, Clone, Default)] pub struct DataStore { /// Primary storage: interned keys, insertion-ordered so records mode can @@ -210,29 +208,6 @@ pub struct DataStore { records: HashMap, } -impl Serialize for DataStore { - fn serialize(&self, s: S) -> Result { - use serde::ser::SerializeSeq; - let mut seq = s.serialize_seq(Some(self.cells.len()))?; - for (k, v) in &self.cells { - let cell_key = self.to_cell_key(k); - seq.serialize_element(&(cell_key, v))?; - } - seq.end() - } -} - -impl<'de> Deserialize<'de> for DataStore { - fn deserialize>(d: D) -> Result { - let pairs: Vec<(CellKey, CellValue)> = Vec::deserialize(d)?; - let mut store = DataStore::default(); - for (key, value) in pairs { - store.set(key, value); - } - Ok(store) - } -} - impl DataStore { pub fn new() -> Self { Self::default() @@ -1181,15 +1156,4 @@ mod record_index { assert_eq!(store.resolve_record(hash), Some(record)); } } - - /// Derived state must come back *with* the data, rebuilt through `set`, - /// rather than be written alongside it. - #[test] - fn a_deserialized_store_can_resolve_its_records() { - let json = serde_json::to_string(&store()).expect("serializes"); - let back: DataStore = serde_json::from_str(&json).expect("deserializes"); - let east = key(&[("Product", "Shirts"), ("Region", "East")]); - assert_eq!(back.resolve_record(RecordHash::of(&east)), Some(&east)); - assert_eq!(back.live_records().count(), 2); - } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index b6f8da5f..a27460e8 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use anyhow::{Result, anyhow}; use indexmap::{IndexMap, IndexSet}; -use serde::{Deserialize, Serialize}; use super::category::{Category, CategoryId, CategoryKind, MEASURE_CATEGORY}; use super::cell::{CellKey, CellValue, DataStore}; @@ -300,7 +299,7 @@ impl Aggregation { /// Pure-data document model: categories, cells, formulas. It knows nothing /// about views — see [`crate::workbook::Workbook`] and core.md §10. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone)] pub struct Model { pub name: String, pub categories: IndexMap, @@ -309,13 +308,31 @@ pub struct Model { next_category_id: CategoryId, /// Per-measure aggregation, used when collapsing `Axis::None` /// categories. Defaults to `Sum`. - #[serde(default)] pub measure_agg: HashMap, /// Cached formula evaluation results. Recomputed by `recompute_formulas`. - #[serde(skip)] formula_cache: HashMap, } +/// Every field, with the two `HashMap`s in key order: a recompute refills the +/// cache in hash order, which is not a change (improvise-pc3b, gotcha 18). +impl std::fmt::Debug for Model { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut aggs: Vec<_> = self.measure_agg.iter().collect(); + aggs.sort_by(|a, b| a.0.cmp(b.0)); + let mut cache: Vec<_> = self.formula_cache.iter().collect(); + cache.sort_by(|a, b| a.0.0.cmp(&b.0.0)); + f.debug_struct("Model") + .field("name", &self.name) + .field("categories", &self.categories) + .field("data", &self.data) + .field("formulas", &self.formulas) + .field("next_category_id", &self.next_category_id) + .field("measure_agg", &aggs) + .field("formula_cache", &cache) + .finish() + } +} + impl Model { pub fn new(name: impl Into) -> Self { let name = name.into(); @@ -1747,6 +1764,43 @@ mod model_tests { ); } + /// improvise-d7aj: the memo is per KEY, not per formula — one formula met at two + /// fold keys in one evaluation. Evidence: context/test-evidence/core.md. + #[test] + fn one_formula_at_two_fold_keys_is_evaluated_at_each() { + let mut m = quarter_model(&[ + ("Jan", "A", 3.0), + ("Feb", "A", 4.0), + ("Jan", "B", 100.0), + ("Feb", "B", 200.0), + ]); + for text in ["G = Revenue * 2", "F = IF(Revenue > 50, G, G + B)"] { + m.add_formula(crate::formula::parse_formula(text, "_Measure").unwrap()); + } + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "F")]); + assert_eq!( + m.evaluate_covering(&key, Aggregation::Exact, &|_, _| true), + Some(CellValue::Number(614.0)), + "G at A (14) plus F at B, which is G at B (600)" + ); + } + + /// improvise-d7aj: a formula reached twice by two paths is finished, not in + /// progress, the second time. Evidence: context/test-evidence/core.md. + #[test] + fn a_diamond_at_a_fold_is_not_circular() { + let mut m = quarter_model(&[("Jan", "A", 3.0), ("Feb", "A", 4.0)]); + for text in ["A1 = B2 + C3", "B2 = D * 2", "C3 = D * 3", "D = Revenue"] { + m.add_formula(crate::formula::parse_formula(text, "_Measure").unwrap()); + } + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "A1")]); + super::FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); + let value = m.evaluate_covering(&key, Aggregation::Exact, &|_, _| true); + let evaluations = super::FOLD_FORMULA_EVALUATIONS.with(|n| n.get()); + assert_eq!(value, Some(CellValue::Number(35.0)), "14 + 21 over D = 7"); + assert_eq!(evaluations, 4, "A1, B2, C3 and D once each"); + } + /// Evidence: context/test-evidence/core.md. #[test] fn a_groups_stored_values_fold_once_not_per_member() { diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index 140061f7..1967bfb8 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -1,14 +1,9 @@ use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; use super::axis::Axis; use crate::model::category::CategoryKind; -fn default_prune() -> bool { - true -} - /// Scratch views the app rebuilds on demand rather than persisting, because /// they put the never-persisted `_Index`/`_Dim` on axes (improvise-60z). pub const VIRTUAL_VIEW_NAMES: [&str; 2] = ["_Records", "_Drill"]; @@ -32,7 +27,7 @@ pub fn unused_view_name<'a>(existing: impl IntoIterator) -> Stri .expect("a name is free below usize::MAX, there being fewer views") } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct View { pub name: String, /// Axis assignment for each category @@ -46,7 +41,6 @@ pub struct View { /// Number format string (e.g. ",.0f" for comma-separated integer) pub number_format: String, /// When true, empty rows/columns are pruned from the display. - #[serde(default = "default_prune")] pub prune_empty: bool, } diff --git a/crates/improvise-core/src/workbook.rs b/crates/improvise-core/src/workbook.rs index 718e9067..0df64dc3 100644 --- a/crates/improvise-core/src/workbook.rs +++ b/crates/improvise-core/src/workbook.rs @@ -3,13 +3,12 @@ use anyhow::{Result, anyhow}; use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; use crate::model::Model; use crate::model::category::{CategoryId, CategoryKind, ItemId}; use crate::view::{Axis, DEFAULT_VIEW, View}; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Workbook { pub model: Model, pub views: IndexMap, diff --git a/crates/improvise-formula/Cargo.toml b/crates/improvise-formula/Cargo.toml index 1912219f..d7459be8 100644 --- a/crates/improvise-formula/Cargo.toml +++ b/crates/improvise-formula/Cargo.toml @@ -10,7 +10,6 @@ repository = "https://github.com/fiddlerwoaroof/improvise" anyhow = "1" pest = "2.9.2" pest_derive = "2.9.2" -serde = { version = "1", features = ["derive"] } [dev-dependencies] pest_meta = "2.9.2" diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 1022ea25..b8028b56 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -1,6 +1,4 @@ -use serde::{Deserialize, Serialize}; - -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub enum AggFunc { Sum, Avg, @@ -11,7 +9,7 @@ pub enum AggFunc { /// 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)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BinOp { Add, Sub, @@ -26,13 +24,13 @@ pub enum BinOp { Ge, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Filter { pub category: String, pub item: String, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub enum Expr { Number(f64), Ref(String), @@ -45,7 +43,7 @@ pub enum Expr { Is(Filter), } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Formula { /// The raw formula text, e.g. "Profit = Revenue - Cost" pub raw: String, @@ -59,7 +57,6 @@ pub struct Formula { pub filter: Option, /// How this formula's target is drawn, overriding the view's `format:` for /// that one measure. `None` means the view answers — `context/code/core.md`. - #[serde(default)] pub format: Option, } diff --git a/crates/improvise-io/Cargo.toml b/crates/improvise-io/Cargo.toml index 1ec96762..bb8d367f 100644 --- a/crates/improvise-io/Cargo.toml +++ b/crates/improvise-io/Cargo.toml @@ -13,7 +13,7 @@ anyhow = "1" chrono = { version = "0.4", features = ["serde"] } csv = "1" flate2 = "1" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" pest = "2.9.2" pest_derive = "2.9.2" serde = { version = "1", features = ["derive"] } diff --git a/crates/improvise-io/src/persistence/mod.rs b/crates/improvise-io/src/persistence/mod.rs index 505dfa2c..ee8d579a 100644 --- a/crates/improvise-io/src/persistence/mod.rs +++ b/crates/improvise-io/src/persistence/mod.rs @@ -1,4 +1,4 @@ -//! `.improv` document lifecycle — save, load, gzip, legacy JSON — over a +//! `.improv` document lifecycle — save, load, gzip — over a //! [`Storage`] backend. The text format is in `format`, `parse` and `export`. mod export; @@ -96,11 +96,16 @@ pub fn load_from(storage: &dyn Storage, path: &Path) -> Result { }; let text = String::from_utf8(bytes).with_context(|| format!("{} is not UTF-8", path.display()))?; + // `.improv` text is the only native format; a JSON workbook is named, not + // handed to the grammar to fail on its first byte (improvise-tt22). if text.trim_start().starts_with('{') { - serde_json::from_str(&text).context("Failed to deserialize workbook") - } else { - parse_md(&text) + anyhow::bail!( + "{} is a legacy JSON workbook, which is no longer supported: \ + only .improv text can be opened", + path.display() + ); } + parse_md(&text) } pub fn autosave_path(path: &Path) -> std::path::PathBuf { @@ -194,37 +199,34 @@ mod tests { ); } + /// improvise-tt22: `.improv` text is the only native format, so a legacy + /// JSON workbook is refused by name. Evidence: `context/test-evidence/formula-and-io.md`. #[test] - fn load_dispatcher_detects_legacy_json_by_brace() { - // The load() function routes to JSON deserializer when text starts with '{' - let m = two_cat_model(); - let json = serde_json::to_string_pretty(&m).unwrap(); - assert!(json.trim_start().starts_with('{'), "sanity check"); - // Deserialise via the JSON path - let m2: Workbook = serde_json::from_str(&json).unwrap(); - assert_eq!(m2.model.name, "Budget"); - } - - #[test] - fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() { + fn a_legacy_json_workbook_is_refused_by_name() { use crate::storage::{MemoryFs, Storage}; - const LOSSY: f64 = 0.248_157_142_857_142_85; - let key = coord(&[("Type", "Food"), ("Month", "Jan")]); - let mut wb = two_cat_model(); - wb.model.set_cell(key.clone(), CellValue::Number(LOSSY)); - let json = serde_json::to_string(&wb).unwrap(); - assert!( - json.contains("0.24815714285714285"), - "the file holds the exact text" - ); - let fs = MemoryFs::new(); - let path = std::path::Path::new("legacy.improv"); - fs.write(path, json.as_bytes()).unwrap(); - let loaded = super::load_from(&fs, path).unwrap(); - let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else { - panic!("the cell must load as a number"); - }; - assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}"); + // The shape the removed loader read: `Workbook`'s serde form. + let legacy = r#" + {"model": {"name": "Budget", "categories": {}, "data": [], "formulas": []}, + "views": {}, "active_view": "Default"}"#; + for name in ["legacy.improv", "legacy.improv.gz"] { + let fs = MemoryFs::new(); + let path = std::path::Path::new(name); + let bytes = if name.ends_with(".gz") { + use std::io::Write; + let mut enc = + flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + enc.write_all(legacy.as_bytes()).unwrap(); + enc.finish().unwrap() + } else { + legacy.as_bytes().to_vec() + }; + fs.write(path, &bytes).unwrap(); + let err = format!("{:#}", super::load_from(&fs, path).unwrap_err()); + assert!( + err.contains("legacy JSON") && err.contains("no longer supported"), + "{name}: expected the format named and refused, got {err:?}" + ); + } } // ── save/load roundtrip via file ──────────────────────────────────── diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index d20a9d11..385b3e29 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -196,7 +196,6 @@ dependencies = [ "anyhow", "pest", "pest_derive", - "serde", ] [[package]] @@ -228,8 +227,6 @@ checksum = "cc4e190f5d26ca7051642629da2c52fc03bde85a03197c99408dcd291734c855" dependencies = [ "equivalent", "hashbrown", - "serde", - "serde_core", ] [[package]] diff --git a/tests/shipped_documents_load.rs b/tests/shipped_documents_load.rs index 5b4d5467..deb725e4 100644 --- a/tests/shipped_documents_load.rs +++ b/tests/shipped_documents_load.rs @@ -3,11 +3,21 @@ use std::path::{Path, PathBuf}; -fn shipped_documents() -> Vec { - let root = Path::new(env!("CARGO_MANIFEST_DIR")); - let mut found: Vec = ["examples", "web"] - .iter() - .flat_map(|dir| std::fs::read_dir(root.join(dir)).expect("a shipped-document directory")) +/// The directories `.gitignore` un-ignores `*.improv` in: the definition of +/// "shipped", read from the one place that states it (improvise-qlrh). +fn shipped_directories(root: &Path) -> Vec { + let gitignore = std::fs::read_to_string(root.join(".gitignore")).expect("a .gitignore"); + gitignore + .lines() + .filter_map(|line| line.trim().strip_prefix('!')) + .filter_map(|pattern| pattern.strip_suffix("/*.improv")) + .map(|dir| root.join(dir)) + .collect() +} + +fn documents_in(dir: &Path) -> Vec { + let mut found: Vec = std::fs::read_dir(dir) + .expect("a shipped-document directory") .map(|entry| entry.expect("a readable directory entry").path()) // `extension()` of `x.improv.gz` is `gz`, so matching on the extension // alone would skip it and put back the hole this test closes. @@ -22,21 +32,31 @@ fn shipped_documents() -> Vec { #[test] fn every_shipped_document_loads() { - let documents = shipped_documents(); + let root = Path::new(env!("CARGO_MANIFEST_DIR")); + let dirs = shipped_directories(root); assert!( - documents.len() >= 2, - "precondition: the walk must find the documents that exist, got {documents:?}" + !dirs.is_empty(), + "precondition: .gitignore must un-ignore at least one *.improv directory" ); - for path in documents { - let workbook = improvise::persistence::load(&path) - .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); + for dir in dirs { + let documents = documents_in(&dir); + // Per directory, so a walk that silently loses one cannot pass (qlrh). assert!( - workbook.views.contains_key(&workbook.active_view), - "{} loads but its initial view {:?} is not one of {:?}", - path.display(), - workbook.active_view, - workbook.views.keys().collect::>() + !documents.is_empty(), + "precondition: {} is un-ignored for *.improv but the walk found none there", + dir.display() ); + for path in documents { + let workbook = improvise::persistence::load(&path) + .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); + assert!( + workbook.views.contains_key(&workbook.active_view), + "{} loads but its initial view {:?} is not one of {:?}", + path.display(), + workbook.active_view, + workbook.views.keys().collect::>() + ); + } } }