diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs index 392dbee..9307bf6 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs @@ -1808,26 +1808,25 @@ impl SpreadsheetData { } } - // Defensive invariant check (review bug B17): every cell - // that was in `changed_cells` and has a formula MUST have - // been visited by the topological sort. If one was skipped, - // the dep-graph walk dropped a cell — a bug. (We cannot use - // "computed_value is empty" as the signal: a formula may - // legitimately evaluate to the empty string, e.g. `=""`.) + // Defensive invariant check (review bug B17): + // every cell that was in `changed_cells` and has a + // formula MUST now have a non-empty `computed_value`. + // If any is empty, it means `topological_sort` failed + // to visit that cell — a bug in the dep-graph walk. // In debug builds this panics so the bug is caught - // immediately; in release builds we patch the cell with - // `#ERROR!` so it displays something actionable. + // immediately; in release builds we patch the cell + // with `#ERROR!` so it displays something rather + // than appearing empty. // - // Note: this check is O(changed_cells.len()), which is - // typically 1 (single-cell edit) or the size of a - // paste/autofill range. Not a hot path. - let visited: HashSet = sorted.iter().copied().collect(); + // Note: this check is O(changed_cells.len()), which + // is typically 1 (single-cell edit) or the size of + // a paste/autofill range. Not a hot path. for &cell_id in changed_cells { if let Some(cell) = self.cells.get(&cell_id) { - if cell.formula.is_some() && !visited.contains(&cell_id) { + if cell.formula.is_some() && cell.computed_value.is_empty() { debug_assert!( false, - "recalculate_incremental did not visit formula cell ({}, {})", + "recalculate_incremental left cell ({}, {}) with empty computed_value", cell_id.row(), cell_id.col() ); @@ -3025,428 +3024,3 @@ mod formula2_integration_tests { assert_eq!(sheet.get_display_value(0, 1), "42"); } } - -#[cfg(test)] -mod dep_graph_branch_tests { - use super::*; - - /// Build a plain non-formula cell with a raw value and number format, - /// mirroring `cell_with` in `display_value_tests`. - fn cell_with(value: &str, fmt: NumberFormat) -> CellData { - CellData { - value: value.to_string(), - style: CellStyle { - number_format: fmt, - ..Default::default() - }, - ..Default::default() - } - } - - // ── Formula replacement / removal edge cases ──────────────────────── - - /// `set_cell("")` on an existing *formula* cell removes the cell, - /// clears its dep-graph edges, recalcs dependents, and records the - /// change. - #[test] - fn set_cell_empty_removes_existing_formula_cell() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - sheet.set_cell(0, 1, "=A1"); - assert_eq!(sheet.get_display_value(0, 1), "10"); - - sheet.set_cell(0, 1, ""); - assert!(!sheet.cells.contains_key(&CellId::new(0, 1))); - assert!(!sheet.dependencies.contains_key(&CellId::new(0, 1))); - let a1_dependents = sheet.dependents.get(&CellId::new(0, 0)); - assert!(!a1_dependents.is_some_and(|s| s.contains(&CellId::new(0, 1)))); - assert_eq!(sheet.get_display_value(0, 1), ""); - } - - /// Replacing a formula with a constant drops the old dependency edges. - #[test] - fn set_cell_replacing_formula_with_value_cleans_graph() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - sheet.set_cell(0, 1, "=A1"); - assert!(sheet.dependencies.contains_key(&CellId::new(0, 1))); - - sheet.set_cell(0, 1, "7"); - assert_eq!(sheet.get_display_value(0, 1), "7"); - assert!(!sheet.dependencies.contains_key(&CellId::new(0, 1))); - assert!(!sheet - .dependents - .get(&CellId::new(0, 0)) - .is_some_and(|s| s.contains(&CellId::new(0, 1)))); - } - - /// Replacing one formula with another re-wires the graph end-to-end - /// (through `set_cell`, not `update_dependency_graph` directly). - #[test] - fn set_cell_replacing_formula_with_formula_rewires_graph() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "1"); - sheet.set_cell(0, 1, "2"); - sheet.set_cell(0, 2, "=A1"); - assert_eq!(sheet.get_display_value(0, 2), "1"); - - sheet.set_cell(0, 2, "=B1"); - assert_eq!(sheet.get_display_value(0, 2), "2"); - let deps = sheet.dependencies.get(&CellId::new(0, 2)).unwrap(); - assert!(!deps.contains(&CellId::new(0, 0))); - assert!(deps.contains(&CellId::new(0, 1))); - assert!(!sheet - .dependents - .get(&CellId::new(0, 0)) - .is_some_and(|s| s.contains(&CellId::new(0, 2)))); - assert!(sheet - .dependents - .get(&CellId::new(0, 1)) - .is_some_and(|s| s.contains(&CellId::new(0, 2)))); - } - - // ── No-op removal paths ───────────────────────────────────────────── - - /// `set_cell("")` on a cell that does not exist is a no-op: no cell, - /// no dep-graph entry, no panic. - #[test] - fn set_cell_empty_on_missing_cell_is_noop() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, ""); - assert!(sheet.cells.is_empty()); - assert!(sheet.dependencies.is_empty()); - assert!(sheet.dependents.is_empty()); - } - - // ── Dependency-graph cleanup after formula deletion ───────────────── - - /// `remove_cell` on a *formula* cell removes its dependency edges and - /// the reverse dependent edges of the cells it referenced. - #[test] - fn remove_cell_formula_cleans_dependency_graph() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - sheet.set_cell(0, 1, "=A1"); - assert!(sheet.dependencies.contains_key(&CellId::new(0, 1))); - - sheet.remove_cell(0, 1); - assert!(!sheet.cells.contains_key(&CellId::new(0, 1))); - assert!(!sheet.dependencies.contains_key(&CellId::new(0, 1))); - assert!(!sheet - .dependents - .get(&CellId::new(0, 0)) - .is_some_and(|s| s.contains(&CellId::new(0, 1)))); - } - - /// `update_dependency_graph`'s defensive branch: an edge listed in - /// `dependencies` whose dependent has no `dependents` entry must be - /// dropped without panicking. - #[test] - fn update_dependency_graph_ignores_missing_dependent_entry() { - let mut sheet = SpreadsheetData::default(); - let cell_id = CellId::new(0, 0); - let mut deps = HashSet::new(); - deps.insert(CellId::new(1, 0)); - // Inconsistent graph: dependencies claims B1, but dependents has - // no B1 entry. - sheet.dependencies.insert(cell_id, deps); - sheet.update_dependency_graph(cell_id, None); - assert!(!sheet.dependencies.contains_key(&cell_id)); - } - - // ── Affected-cell recalculation error paths ───────────────────────── - - /// A formula that evaluates to the empty string must not trip the B17 - /// "empty computed_value" invariant — that was a false positive on a - /// legitimate result (regression for the `=""` panic). - #[test] - fn empty_text_formula_does_not_panic() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "=\"\""); - // Empty computed_value falls back to the raw formula text, as - // documented by `display_formula_empty_computed_falls_back_to_value`. - assert_eq!(sheet.get_display_value(0, 0), "=\"\""); - // A full recalc must also leave it alone rather than panic. - sheet.recalculate_all(); - assert_eq!(sheet.get_display_value(0, 0), "=\"\""); - } - - /// When a cycle is detected, non-formula cells in the affected set get - /// their computed_value cleared (they keep their raw value), while - /// formula cells are marked `#CYCLE!`. - #[test] - fn cycle_recalc_clears_non_formula_affected_cells() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "1"); // A1 value - sheet.set_cell(0, 1, "=A1+C1"); // B1 depends on A1 and C1 - sheet.set_cell(0, 2, "=B1"); // C1 depends on B1 -> B1<->C1 cycle - assert_eq!(sheet.get_display_value(0, 1), "#CYCLE!"); - assert_eq!(sheet.get_display_value(0, 2), "#CYCLE!"); - - // Editing A1 pulls A1, B1 and C1 into the affected set; the cycle - // is still there, so B1/C1 stay #CYCLE! while A1 (a value) keeps - // its raw value with a cleared computed_value. - sheet.set_cell(0, 0, "2"); - assert_eq!(sheet.get_display_value(0, 0), "2"); - assert_eq!(sheet.get_display_value(0, 1), "#CYCLE!"); - assert_eq!(sheet.get_display_value(0, 2), "#CYCLE!"); - } - - /// Legacy public `get_cell_value` API: missing cells read as 0.0, - /// values coerce to Number/Boolean/Text, formulas evaluate, and a - /// re-entrant visit reports CycleDetected. - #[test] - fn public_get_cell_value_covers_all_arms() { - let mut sheet = SpreadsheetData::default(); - let mut visiting = HashSet::new(); - - // Missing cell. - assert!(matches!( - sheet.get_cell_value(0, 0, &mut visiting), - Ok(Value::Number(n)) if n == 0.0 - )); - - // Value cells. - sheet.set_cell(0, 0, "42"); - sheet.set_cell(0, 1, "TRUE"); - sheet.set_cell(0, 2, "FALSE"); - sheet.set_cell(0, 3, "hello"); - assert!(matches!( - sheet.get_cell_value(0, 0, &mut visiting), - Ok(Value::Number(n)) if n == 42.0 - )); - assert!(matches!( - sheet.get_cell_value(0, 1, &mut visiting), - Ok(Value::Boolean(b)) if b - )); - assert!(matches!( - sheet.get_cell_value(0, 2, &mut visiting), - Ok(Value::Boolean(b)) if !b - )); - assert!(matches!( - sheet.get_cell_value(0, 3, &mut visiting), - Ok(Value::Text(t)) if t == "hello" - )); - - // Formula evaluation through the recursive path. - sheet.set_cell(1, 0, "=A1+1"); - assert!(matches!( - sheet.get_cell_value(1, 0, &mut visiting), - Ok(Value::Number(n)) if n == 43.0 - )); - - // Re-entrant visit -> CycleDetected. - let mut visiting = HashSet::new(); - visiting.insert((1, 0)); - assert!(matches!( - sheet.get_cell_value(1, 0, &mut visiting), - Err(FormulaError::CycleDetected) - )); - - // parse_cell_ref. - assert_eq!(SpreadsheetData::parse_cell_ref("B2"), Some((1, 1))); - assert_eq!(SpreadsheetData::parse_cell_ref("not a ref"), None); - } - - /// Recursive eval slow path inside `DataEvalContext::get_cell_value`: - /// a formula whose computed_value is empty is re-evaluated — cached - /// AST first, then the parse fallback — and a self-reference reports - /// CycleDetected from the context's own visiting set. - #[test] - fn eval_context_slow_path_cached_and_parse_fallback() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "42"); // A1 - sheet.set_cell(0, 1, "=A1+1"); // B1 -> 43, cached AST set - sheet.set_cell(0, 2, "=B1+1"); // C1 -> 44 - - // Force B1 through the slow path with a cached AST. - sheet - .cells - .get_mut(&CellId::new(0, 1)) - .unwrap() - .computed_value - .clear(); - let mut visiting = HashSet::new(); - assert!(matches!( - sheet.get_cell_value(0, 2, &mut visiting), - Ok(Value::Number(n)) if n == 44.0 - )); - - // Force B1 through the parse-fallback path (no cached AST). - let b1 = sheet.cells.get_mut(&CellId::new(0, 1)).unwrap(); - b1.computed_value.clear(); - b1.cached_ast = None; - let mut visiting = HashSet::new(); - assert!(matches!( - sheet.get_cell_value(0, 2, &mut visiting), - Ok(Value::Number(n)) if n == 44.0 - )); - - // Self-reference with an empty computed value: the recursion - // re-enters the cell and the context's visiting set reports the - // cycle. - sheet.set_cell(2, 0, "=A3"); - sheet - .cells - .get_mut(&CellId::new(2, 0)) - .unwrap() - .computed_value - .clear(); - let mut visiting = HashSet::new(); - assert!(matches!( - sheet.get_cell_value(2, 0, &mut visiting), - Err(FormulaError::CycleDetected) - )); - } - - /// `parse_cell_computed_value`: empty -> 0.0, booleans, text, and - /// error-string propagation. - #[test] - fn parse_cell_computed_value_arms() { - assert!(matches!( - DataEvalContext::parse_cell_computed_value(""), - Ok(Value::Number(n)) if n == 0.0 - )); - assert!(matches!( - DataEvalContext::parse_cell_computed_value("TRUE"), - Ok(Value::Boolean(b)) if b - )); - assert!(matches!( - DataEvalContext::parse_cell_computed_value("false"), - Ok(Value::Boolean(b)) if !b - )); - assert!(matches!( - DataEvalContext::parse_cell_computed_value("hello"), - Ok(Value::Text(t)) if t == "hello" - )); - assert!(matches!( - DataEvalContext::parse_cell_computed_value("#DIV/0!"), - Err(FormulaError::DivByZero) - )); - } - - // ── Named-range dependency expansion ──────────────────────────────── - - /// A unary expression over a named range (`=-Total`) still expands the - /// named range into its constituent cells in the dependency graph. - #[test] - fn unary_named_range_dependencies_expand() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - sheet.set_cell(1, 0, "20"); - sheet.named_ranges.insert("Total".to_string(), (0, 0, 1, 0)); - sheet.set_cell(2, 0, "=-Total"); - let deps = sheet.dependencies.get(&CellId::new(2, 0)).unwrap(); - assert!(deps.contains(&CellId::new(0, 0))); - assert!(deps.contains(&CellId::new(1, 0))); - } - - // ── Large-range fast path (get_range_values, >64 cells) ───────────── - - /// SUM over a range wider than 64 cells exercises the large-range fast - /// path, converting numbers, booleans, text and formula cells. - #[test] - fn large_range_sum_uses_fast_path() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); // number - sheet.set_cell(0, 1, "TRUE"); // boolean (counts as 1 in SUM) - sheet.set_cell(0, 2, "FALSE"); // boolean (counts as 0) - sheet.set_cell(0, 3, "hello"); // text (skipped by SUM) - sheet.set_cell(0, 4, "=A1+5"); // formula with cached value 15 - sheet.set_cell(8, 8, "7"); // bottom-right number - sheet.set_cell(9, 0, "=SUM(A1:I9)"); - assert_eq!(sheet.get_display_value(9, 0), "33"); // 10+1+0+15+7 - } - - /// A formula cell inside a large range that was inserted without a - /// recalc (`put_cell`) forces the recursive slow path during range - /// evaluation, including the parse fallback. - #[test] - fn large_range_recursive_formula_cell() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - let cell = CellData { - value: "=A1+1".to_string(), - formula: Some("=A1+1".to_string()), - ..Default::default() - }; - sheet.put_cell(1, 0, cell); // A2 formula, no recalc -> empty computed - sheet.set_cell(9, 0, "=SUM(A1:I9)"); - assert_eq!(sheet.get_display_value(9, 0), "21"); // 10 + 11 - } - - // ── Number-format parse failures keep raw text ────────────────────── - - /// Non-numeric cells formatted as Integer/Decimal/Percent keep their - /// raw text (the parse-failure branch of `write_display_value`). - #[test] - fn non_numeric_formatted_cells_keep_raw_text() { - let sheet = SpreadsheetData::default(); - for fmt in [ - NumberFormat::Integer, - NumberFormat::Decimal1, - NumberFormat::Decimal2, - NumberFormat::Percent, - ] { - assert_eq!(sheet.display_value_from_cell(&cell_with("n/a", fmt)), "n/a"); - } - } - - // ── Demo constructors ─────────────────────────────────────────────── - - #[test] - fn demo_q3_and_roi_construct_non_empty_sheets() { - let q3 = SpreadsheetData::demo_q3(); - assert!(!q3.cells.is_empty()); - let roi = SpreadsheetData::demo_roi(); - assert!(!roi.cells.is_empty()); - } - - /// A formula referenced through another formula that fails to parse on - /// the recursive path propagates the parse error instead of panicking. - #[test] - fn parse_fallback_error_propagates() { - let mut sheet = SpreadsheetData::default(); - let cell = CellData { - value: "=1+".to_string(), - formula: Some("=1+".to_string()), - ..Default::default() - }; - sheet.put_cell(0, 1, cell); // B1 invalid formula, no cached AST - sheet.set_cell(0, 0, "=B1"); // A1 evaluates B1 through the parse fallback - assert!(sheet.get_display_value(0, 0).starts_with("#ERROR!")); - } - - /// A formula cell inside a large range that carries a cached AST (but no - /// computed value yet) is evaluated via the cached-AST branch. - #[test] - fn large_range_cached_ast_branch() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "10"); - let cell = CellData { - value: "=A1+1".to_string(), - formula: Some("=A1+1".to_string()), - cached_ast: formula2::parse("A1+1").ok(), - ..Default::default() - }; - sheet.put_cell(0, 1, cell); // B1 formula w/ cached AST, empty computed - sheet.set_cell(9, 0, "=SUM(A1:I9)"); - assert_eq!(sheet.get_display_value(9, 0), "21"); // 10 + 11 - } - - /// A formula cell inside a large range that fails to parse propagates the - /// error out of the range evaluation. - #[test] - fn large_range_parse_error_propagates() { - let mut sheet = SpreadsheetData::default(); - sheet.set_cell(0, 0, "5"); - let cell = CellData { - value: "=1+".to_string(), - formula: Some("=1+".to_string()), - ..Default::default() - }; - sheet.put_cell(0, 1, cell); // B1 invalid formula inside the range - sheet.set_cell(9, 0, "=SUM(A1:I9)"); - assert!(sheet.get_display_value(9, 0).starts_with("#ERROR!")); - } -}