diff --git a/context/code/command.md b/context/code/command.md index 7f3e5b9a..a9e007e5 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1541,6 +1541,29 @@ The `LayoutGeometry` arm is spelled out rather than written as `EVERYTHING`: the two coincide today, but they are derived from different effects and an entry added for one must not silently arrive for the other. +### `grid_in`'s elsewhere path and the inputs it must rebuild (improvise-ac2z) + +`grid_in` projects a view the host may not be on, and +`doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild)` +says to enumerate the inputs and ask of **each** whether it was derived for the +caller or for the subject. Two have been found that way. `prune_empty` was the +first (improvise-t6db): `GridLayout::new` does not apply it, so the fix was +`with_frozen_records`. `Model::formula_cache` is the second, and it was worse, +because it cannot be fixed where it is observed — `grid_in` takes `&App` and +`recompute_formulas` needs `&mut Model`. + +`rederived_for` answers it by **copying**: where the target view's `none_cats` +differ from the host's and the document has formulas, it clones the model and +recomputes for that view. The three conditions are what keep the clone rare — +the common case is a client on the host's own view, which pays nothing, and a +formula-free document pays nothing ever. It is an interim: keying +`formula_cache` by `none_cats`, so one `Model` holds more than one view's +derived values, is the durable answer and is an `improvise-core` change. + +The list is not closed. Anything else `rebuild_layout` computes for the host's +view and `grid_in` then reads is the next instance, and nothing enumerates them +— which is the doctrine's whole point. + ### `grid_of` — the extent is the whole grid `GridExtent` carries the whole grid, not the window: it is what a client clamps its diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index b064cdbd..752bdad8 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3471,31 +3471,46 @@ are identical by construction, which is why the whole suite passed unchanged across the change. The evidence is the benchmark, before and after, in `context/code/command.md`: about 3x, the number of passes removed. -### a_hosts_none_cats_leak_into_another_views_formula_columns +### a_formulas_columns_survive_a_host_that_aggregates_differently -improvise-ac2z, and a **characterization** test: it asserts the bug, so it fails -when the bug is fixed and says so in its own message. +improvise-ac2z, **fixed**. This replaces the characterization test +`a_hosts_none_cats_leak_into_another_views_formula_columns`, whose message said +that failing meant the bug was fixed; it did, and it is gone. `App::rebuild_layout` calls `recompute_formulas(&none_cats)` for the **host's** -view, populating `Model::formula_cache`. `project::grid_in` on the elsewhere path -builds the other view's `GridLayout` against that same, stale cache. The cache is -keyed by the coordinates the recompute used, so with `Product` on None in the -host's view the `Total` cells are keyed *without* a `Product` coordinate; a view -with `Product` on Column asks for `(Region, Product, _Measure=Total)`, finds -nothing, and — `prune_empty` being on — drops both `Total` columns. Not a wrong -number: absent columns. +view, populating `Model::formula_cache`. `project::grid_in` on the elsewhere +path built the other view's `GridLayout` against that same cache, which is keyed +by the coordinates the recompute used: with `Product` on None in the host's view +the `Total` cells are keyed *without* a `Product` coordinate, a view with +`Product` on Column asks for `(Region, Product, _Measure=Total)`, finds nothing, +and — `prune_empty` being on — drops both `Total` columns. Not a wrong number: +absent columns. host on `Second`, projecting `Second` 6 columns, `Total` present host on `Default`, projecting `Second` 4 columns, no `Total` at all -It asserts the **precondition** — that `Second` on its own really does draw the -formula's columns — before asserting their absence, so a fixture that lost the -formula would fail rather than pass on two empties. +The fix is `grid_in::rederived_for`: where the target view aggregates over +different categories than the host's, it clones the model and recomputes the +formulas for **that** view's `none_cats`. Three conditions gate the clone — +another view, different `none_cats`, and at least one formula — so the common +case pays nothing and a formula-free document pays nothing ever. -The fix is not local and none is chosen: `grid_in` takes `&App` and -`recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the -view it is projecting. Candidates are on the ticket. +**This is an interim, and the cost is the reason.** A clone of the whole model +plus a fixed-point recompute, per projection, on exactly the path that is broken +today. The ticket's first candidate — keying `formula_cache` by `none_cats` so +one `Model` can hold more than one view's derived values — is the durable answer +and is filed separately; it is an `improvise-core` change and did not belong in +this wave's ground. What is bought meanwhile is correctness on a path that was +silently wrong, `doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild)`. +**Fault injected**: `rederived_for`'s condition forced to `false`. Two tests +fail — this one, and +`a_clients_grid_does_not_depend_on_which_view_its_host_is_on`, whose `none_cats` +carve-out was removed at the same time and which is the general property this +one is an instance of. The test asserts its two **preconditions** first — that +`Second` on its own really draws the formula's columns, and that the two views +really do aggregate over different categories — so a fixture that lost either +fails rather than passing on two empties. ### a_view_named_command_lands_on_the_view_it_names diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 5e6ddd20..eb2def0a 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -132,24 +132,21 @@ pub fn grid_of(app: &App, viewport: Viewport) -> GridCache { pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { let workbook = &app.model_state.workbook; let (name, view) = app.resolve_view(view_name); + // `formula_cache` holds the HOST's derivation, keyed by the coordinates ITS + // `none_cats` aggregated over — improvise-ac2z, `context/code/command.md`. + let derived = rederived_for(app, name, view); + let model = derived.as_ref().unwrap_or(&workbook.model); // `with_frozen_records`, not `new`: `new` skips `prune_empty`, so a client on // another view was projected rows its own config prunes — `context/code/command.md`. - let elsewhere = (name != app.view_name()) - .then(|| GridLayout::with_frozen_records(&workbook.model, view, None)); + let elsewhere = + (name != app.view_name()).then(|| GridLayout::with_frozen_records(model, view, None)); let layout = elsewhere.as_ref().unwrap_or(&app.layout); let (comma, decimals) = parse_number_format(&view.number_format); let rows = viewport.rows_in(layout.row_count()); let cols = viewport.cols_in(layout.col_count()); // ONE walk: the widths, the totals and this window's values all come off the // same evaluation — `context/code/command.md` (improvise-jmjz). - let body = grid_body( - &workbook.model, - layout, - comma, - decimals, - rows.clone(), - cols.clone(), - ); + let body = grid_body(model, layout, comma, decimals, rows.clone(), cols.clone()); GridCache { row_frame: axis_frame(&layout.row_cats, &layout.row_items, view, rows.clone()), @@ -182,6 +179,22 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { } } +/// A copy of the model whose formulas were evaluated for **this** view, where the +/// host's own derivation does not answer for it — `context/code/command.md`. +fn rederived_for(app: &App, name: &str, view: &View) -> Option { + let workbook = &app.model_state.workbook; + // Three conditions, and each one is what makes the clone rare: another view, + // a different aggregation, and something derived to get wrong. + let owed = name != app.view_name() + && view.none_cats() != app.view().none_cats() + && !workbook.model.formulas().is_empty(); + owed.then(|| { + let mut model = workbook.model.clone(); + model.recompute_formulas(&view.none_cats()); + model + }) +} + /// One axis's frame, windowed to the data indices `window` names. Three edge rules, /// all about group headers — see `context/code/command.md`. fn axis_frame( @@ -2497,20 +2510,13 @@ mod tests { below is between two answers that had nothing to disagree about" ); - let none_cats: std::collections::HashMap> = names - .iter() - .map(|n| (n.clone(), app.model_state.workbook.views[n].none_cats())) - .collect(); + // The `none_cats` carve-out is GONE with improvise-ac2z: every pair is + // compared, including the ones that aggregate over different categories. let mut compared = 0usize; for host in &names { app.switch_to_view(host); app.rebuild_layout(); for (target, expected) in names.iter().zip(&authoritative) { - // The exception is a BUG and not a carve-out — improvise-ac2z, - // pinned by the characterization test below. - if none_cats[host] != none_cats[target] { - continue; - } compared += (host != target) as usize; assert_eq!( &grid_in(&app, target, full), @@ -2525,34 +2531,40 @@ mod tests { ); } - /// improvise-ac2z, found by correcting this fixture: the host's `formula_cache` - /// leaks into another view's grid. Evidence: `context/test-evidence/command.md`. + /// improvise-ac2z: a formula is evaluated for the view being PROJECTED, not for + /// whichever one the host stands on. Evidence: `context/test-evidence/command.md`. #[test] - fn a_hosts_none_cats_leak_into_another_views_formula_columns() { + fn a_formulas_columns_survive_a_host_that_aggregates_differently() { let mut app = app_with_three_views_and_an_empty_row(); let full = full_viewport(); + let totals = |grid: &GridCache| { + grid.col_frame + .items() + .filter(|levels| levels.contains(&"Total".to_string())) + .count() + }; app.switch_to_view("Second"); app.rebuild_layout(); let authoritative = grid_in(&app, "Second", full); assert!( - authoritative - .col_frame - .items() - .any(|levels| levels.contains(&"Total".to_string())), - "precondition: Second really does draw the formula's columns" + totals(&authoritative) > 0, + "precondition: Second really does draw the formula's columns, or \ + this compares two empties" ); app.switch_to_view("Default"); app.rebuild_layout(); - let elsewhere = grid_in(&app, "Second", full); - assert!( - !elsewhere - .col_frame - .items() - .any(|levels| levels.contains(&"Total".to_string())), - "IF THIS FAILS, improvise-ac2z IS FIXED: delete this test and drop \ - the none_cats guard from the property above" + assert_ne!( + app.view().none_cats(), + app.model_state.workbook.views["Second"].none_cats(), + "precondition: the two views must aggregate over different \ + categories, which is the whole input to the hazard" + ); + assert_eq!( + grid_in(&app, "Second", full), + authoritative, + "the host's formula_cache decided what another view's grid draws" ); }