A formula is evaluated for the view being projected (improvise-ac2z)
rebuild_layout recomputes formulas for the HOST's none_cats, and grid_in's elsewhere path built another view's GridLayout against that cache. The cache is keyed by the coordinates the recompute used, so a Total keyed without a Product coordinate is not found by a view with Product on Column, and prune_empty drops the formula's columns entirely. Absent columns, not wrong numbers. grid_in::rederived_for clones the model and recomputes for the target view's none_cats. Three conditions gate the clone -- another view, different none_cats, at least one formula -- so a client on the host's own view pays nothing and a formula-free document pays nothing ever. This is an interim and the cost is why: a whole-model clone plus a fixed-point recompute per projection on exactly the broken path. The ticket's first candidate, 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 outside this wave's ground. The characterization test is replaced by the positive one, and the none_cats carve-out is dropped from a_clients_grid_does_not_depend_on_which_view_its_host_is_on -- which is the general property this is an instance of, and which fails under the same injection. Evidence: context/test-evidence/command.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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<crate::model::types::Model> {
|
||||
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<String, Vec<String>> = 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"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user