diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 58617a67..6bc7221a 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -516,6 +516,30 @@ 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. +**That fix was canonical within one instance, not across two (improvise-djcn).** +It fixed `Model`'s own maps but not the maps inside its fields. `DataStore` has +`index`, `records` and the symbol table, and `View` has `page_selections`, +`hidden_items` and `collapsed_groups`. All still printed through a derived +`Debug`, in hash order. Each `HashMap` gets its own `RandomState`, so two equal +documents built separately printed differently in 32 of 32 runs. +- The effect-classification tests compare one instance with itself before and + after, so they were unaffected. +- `a_model_command_lands_where_its_keypress_lands` compares **two** `App`s. It + passed only because its fixture had no cells and no multi-entry view maps. + +Now: +- `DataStore`, `SymbolTable` and `View` have hand-written `Debug` impls. Each + sorts wherever its storage is a hash map. +- `DataStore` resolves symbols to their strings. A `Symbol`'s number is interning + order, not content, so printing it would make two equal stores differ again. +- `SymbolTable` prints its interned strings, sorted. +- `cells` keeps its `IndexMap` order, because that order is part of the document + (gotcha 18). +- Every hand-written `Debug`, `Model`'s included, **destructures** `self`. A field + added later is then a compile error there, where a `debug_struct` that names + fields one by one would skip it silently. The pc3b version of `Model`'s impl + had exactly that hole. + `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails `f64::from_str` (overflow saturates to ±inf), but if the two ever drift the diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 70cb9478..50c2dc1e 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -205,6 +205,28 @@ Row, swapping the axes and rotating Row→Column land on the same view, and an equivalence test that cannot distinguish the wrong effect from the right one proves nothing (`doctrine(check_the_checker)`). +**The fixture needs cells and multi-entry view maps too (improvise-djcn).** The +test compares `format!("{:?}", workbook)` across **two** `App`s. It passed only +because the fixture had no cells and every view map had at most one entry, and a +`HashMap` of one entry has only one order. The fixture now carries: +- three cells; +- two page selections; +- two categories with hidden items, one of them with two; +- two categories with collapsed groups. + +That fixture was red in 3 of 3 runs on the old `Debug` impls: the by-key and +by-command fingerprints differed only in hash order. It was green after the fix. + +Each half of the fix was checked by restoring the derived `Debug` on its own: +- `View`'s derived `Debug` alone: 3 of 3 runs red. +- `DataStore` and `SymbolTable`'s derived `Debug` alone: 3 of 3 runs red. + +So neither half is carried by the other. The exhaustive destructure in `Model`'s +`Debug` has no test of its own, because it is a compile-time guard. It was +checked by adding a `probe_field` to `Model` and to `Model::new`. The build then +failed with `E0027: pattern does not mention field probe_field` at the `Debug` +impl, and nowhere else. + ### a_stale_wire_model_command_cannot_be_applied A wire `Command::Model` cannot reach the document without its basis being checked diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 37d7e5db..a598f68b 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -931,15 +931,47 @@ pub(crate) mod tests { /// to the same mutation lands, `Dirty` included. #[test] fn a_model_command_lands_where_its_keypress_lands() { - /// Two categories on different axes: a one-category fixture cannot tell - /// a transpose from a cycle (`doctrine(check_the_checker)`). + /// Two categories on two axes, plus cells and 2+-entry view maps: why each + /// is needed is in `context/test-evidence/command.md` (improvise-djcn). fn pivoted() -> App { + use crate::model::cell::{CellKey, CellValue}; let mut app = app(); - app.model_state.workbook.add_category("Month").unwrap(); - app.model_state.workbook.add_item("Month", "Jan").unwrap(); - let view = app.model_state.workbook.active_view_mut(); + let wb = &mut app.model_state.workbook; + let items = [ + ("Region", ["South", "East", "West"]), + ("Month", ["Jan", "Feb", "Mar"]), + ("Channel", ["Web", "Store", "Phone"]), + ("Year", ["2024", "2025", "2026"]), + ]; + for (category, names) in items { + wb.add_category(category).ok(); + names.iter().for_each(|n| drop(wb.add_item(category, *n))); + } + for (n, (region, month)) in [("North", "Jan"), ("South", "Feb"), ("East", "Jan")] + .into_iter() + .enumerate() + { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("Month".into(), month.into()), + ("Channel".into(), "Web".into()), + ("Year".into(), "2024".into()), + ]); + wb.model.set_cell(key, CellValue::Number(n as f64 + 1.0)); + } + let view = wb.active_view_mut(); view.set_axis("Region", Axis::Row); view.set_axis("Month", Axis::Column); + view.set_axis("Channel", Axis::Page); + view.set_axis("Year", Axis::Page); + view.set_page_selection("Channel", "Web"); + view.set_page_selection("Year", "2024"); + view.hide_item("Region", "West"); + view.hide_item("Region", "South"); + view.hide_item("Month", "Mar"); + view.toggle_group_collapse("Region", "Coastal"); + view.toggle_group_collapse("Region", "Inland"); + view.toggle_group_collapse("Month", "Q1"); app.rebuild_layout(); app } diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index 7a10edf1..9b55a6c2 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -194,7 +194,7 @@ impl RecordEntry { } } -#[derive(Debug, Clone, Default)] +#[derive(Clone, Default)] pub struct DataStore { /// Primary storage: interned keys, insertion-ordered so records mode can /// show rows in entry order. That order is correctness — core.md §8. @@ -208,6 +208,37 @@ pub struct DataStore { records: HashMap, } +/// Resolved through the symbol table and sorted wherever the storage is a +/// `HashMap`, so two equal stores print alike (improvise-djcn). +impl std::fmt::Debug for DataStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let DataStore { + cells, + symbols, + index, + records, + } = self; + let cells: Vec<_> = cells + .iter() + .map(|(key, value)| (self.to_cell_key(key), value)) + .collect(); + let index: std::collections::BTreeMap<_, std::collections::BTreeSet<_>> = index + .iter() + .map(|((cat, item), keys)| { + let pair = (symbols.resolve(*cat), symbols.resolve(*item)); + (pair, keys.iter().map(|k| self.to_cell_key(k).0).collect()) + }) + .collect(); + let records: std::collections::BTreeMap<_, _> = records.iter().collect(); + f.debug_struct("DataStore") + .field("cells", &cells) + .field("symbols", symbols) + .field("index", &index) + .field("records", &records) + .finish() + } +} + impl DataStore { pub fn new() -> Self { Self::default() diff --git a/crates/improvise-core/src/model/symbol.rs b/crates/improvise-core/src/model/symbol.rs index 65d53857..a60ec9c9 100644 --- a/crates/improvise-core/src/model/symbol.rs +++ b/crates/improvise-core/src/model/symbol.rs @@ -5,12 +5,22 @@ use std::collections::HashMap; pub struct Symbol(u64); /// Bidirectional string ↔ Symbol mapping. -#[derive(Debug, Clone, Default)] +#[derive(Clone, Default)] pub struct SymbolTable { to_id: HashMap, to_str: Vec, } +/// The interned strings, sorted: which `Symbol` each got is interning order, +/// not content, so two equal stores print alike (improvise-djcn). +impl std::fmt::Debug for SymbolTable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let SymbolTable { to_id, to_str: _ } = self; + let interned: std::collections::BTreeSet<&String> = to_id.keys().collect(); + f.debug_set().entries(interned).finish() + } +} + impl SymbolTable { #[allow(dead_code)] pub fn new() -> Self { diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index a27460e8..eb1ac3a4 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -317,16 +317,25 @@ pub struct Model { /// 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(); + // Destructured, so a new field is a compile error here (improvise-djcn). + let Model { + name, + categories, + data, + formulas, + next_category_id, + measure_agg, + formula_cache, + } = self; + let aggs: std::collections::BTreeMap<_, _> = measure_agg.iter().collect(); + let mut cache: Vec<_> = 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("name", name) + .field("categories", categories) + .field("data", data) + .field("formulas", formulas) + .field("next_category_id", next_category_id) .field("measure_agg", &aggs) .field("formula_cache", &cache) .finish() diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index 1967bfb8..e9b91961 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -27,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)] +#[derive(Clone)] pub struct View { pub name: String, /// Axis assignment for each category @@ -44,6 +44,38 @@ pub struct View { pub prune_empty: bool, } +/// Sorted wherever the storage is a `HashMap`, so two equal views print alike; +/// destructured, so a new field is a compile error here (improvise-djcn). +impl std::fmt::Debug for View { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::collections::{BTreeMap, BTreeSet}; + let View { + name, + category_axes, + page_selections, + hidden_items, + collapsed_groups, + number_format, + prune_empty, + } = self; + let sorted = |m: &HashMap>| -> BTreeMap> { + m.iter() + .map(|(k, v)| (k.clone(), v.iter().cloned().collect())) + .collect() + }; + let pages: BTreeMap<_, _> = page_selections.iter().collect(); + f.debug_struct("View") + .field("name", name) + .field("category_axes", category_axes) + .field("page_selections", &pages) + .field("hidden_items", &sorted(hidden_items)) + .field("collapsed_groups", &sorted(collapsed_groups)) + .field("number_format", number_format) + .field("prune_empty", prune_empty) + .finish() + } +} + impl View { /// True for the on-demand scratch views, rebuilt rather than persisted. pub fn is_virtual(&self) -> bool {