diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 3c9efe8f..eacb06f1 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -495,10 +495,22 @@ compiler named every other consumer. Two remain, and both are the wire: untouched. One test consumer had to move: the effect-classification tests fingerprinted the -model and the views through `serde_json`. They now use `Debug`. That is no weaker: -it renders every field, including the derived `formula_cache` the JSON skipped. -`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. +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/test-evidence/command.md b/context/test-evidence/command.md index 3ffb456a..d9409ff9 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5599,3 +5599,29 @@ 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. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index dbb66317..d9ac1d24 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1408,6 +1408,57 @@ mod tests { 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 /// check the shared state it claims not to touch is genuinely unchanged. #[test] @@ -1445,7 +1496,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-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 09dcad98..5973c1d1 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -237,7 +237,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)] +#[derive(Clone)] pub struct Model { pub name: String, pub categories: IndexMap, @@ -251,6 +251,26 @@ pub struct Model { 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();