From a88fb68eab6037325e680e80bd5ea5ad1939b0b7 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 04:39:22 +0000 Subject: [PATCH 1/2] fix(spreadsheet): B17 recalc invariant must not panic on empty-string results `recalculate_incremental` treated "empty computed_value on a changed formula cell" as proof that the topological sort dropped the cell, and debug_asserted on it. But a formula may legitimately evaluate to the empty string (`=""`, `=IF(FALSE, "x", "")`), leaving computed_value empty through no fault of the dep-graph walk. In a debug build that edit panicked the engine. The invariant now checks what it actually meant to check: whether the topological sort *visited* every changed formula cell, using the `visited` set built from `sorted`. Emptiness is no longer used as the error signal, so legitimate empty-string results flow through (the display falls back to the raw formula text, as already documented for empty computed values). --- .../spreadsheet-engine/src/data.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs index 9307bf6..2561b29 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs @@ -1808,25 +1808,26 @@ impl SpreadsheetData { } } - // 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. + // 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. `=""`.) // In debug builds this panics so the bug is caught - // immediately; in release builds we patch the cell - // with `#ERROR!` so it displays something rather - // than appearing empty. + // immediately; in release builds we patch the cell with + // `#ERROR!` so it displays something actionable. // - // 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. + // 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(); for &cell_id in changed_cells { if let Some(cell) = self.cells.get(&cell_id) { - if cell.formula.is_some() && cell.computed_value.is_empty() { + if cell.formula.is_some() && !visited.contains(&cell_id) { debug_assert!( false, - "recalculate_incremental left cell ({}, {}) with empty computed_value", + "recalculate_incremental did not visit formula cell ({}, {})", cell_id.row(), cell_id.col() ); From a859053bc676e912a1e36b084fa295046d194c44 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 04:39:34 +0000 Subject: [PATCH 2/2] test(spreadsheet): cover remaining data.rs dependency-graph branches Targets the uncovered branches the coverage report named, around the formula dependency graph and incremental recalculation: - Formula replacement/removal edge cases: set_cell("") removes the cell and its edges, formula->value and formula->formula rewiring drop stale dependency edges. - No-op removal paths: set_cell("") on a missing cell records nothing. - Dependency-graph cleanup after formula deletion: remove_cell on a formula cell, and update_dependency_graph's defensive path when a dependent has no dependents entry. - Affected-cell recalculation error paths: non-formula cells inside a cycle-affected set keep their raw value; the ="" empty-result regression; the recursive eval slow path (cached AST, parse fallback, and CycleDetected); parse_cell_computed_value arms; parse errors propagating through a dependent and through a large range. Also covers named-range unary expansion (=-Total), the >64-cell range fast path, non-numeric number-format fallbacks, and the demo_q3 / demo_roi constructors. Engine line coverage: data.rs 91.17% -> 97.90%; engine total 90.09% -> 93.24%. Unit tests 260 -> 279 (19 new); 9 integration tests unchanged. --- .../spreadsheet-engine/src/data.rs | 425 ++++++++++++++++++ 1 file changed, 425 insertions(+) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs index 2561b29..392dbee 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs @@ -3025,3 +3025,428 @@ 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!")); + } +}