andodeki/nigig-org
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 8

Compare commits

base: andodeki:a859053bc676e912a1e36b084fa295046d194c44
Branches Tags
andodeki:main
..
compare: andodeki:bd97e68af06d356071aab2d5dba8c843f1191172
Branches Tags
andodeki:main

No commits in common. "a859053bc676e912a1e36b084fa295046d194c44" and "bd97e68af06d356071aab2d5dba8c843f1191172" have entirely different histories.
a859053bc6 ... bd97e68af0

1 changed files with 13 additions and 439 deletions
Download patch file Download diff file Expand all files Collapse all files

452
crates/apps/spreadsheet/spreadsheet-engine/src/data.rs
View file

@ -1808,26 +1808,25 @@ impl SpreadsheetData {
} }
} }
// Defensive invariant check (review bug B17): every cell // Defensive invariant check (review bug B17):
// that was in `changed_cells` and has a formula MUST have // every cell that was in `changed_cells` and has a
// been visited by the topological sort. If one was skipped, // formula MUST now have a non-empty `computed_value`.
// the dep-graph walk dropped a cell — a bug. (We cannot use // If any is empty, it means `topological_sort` failed
// "computed_value is empty" as the signal: a formula may // to visit that cell — a bug in the dep-graph walk.
// legitimately evaluate to the empty string, e.g. `=""`.)
// In debug builds this panics so the bug is caught // In debug builds this panics so the bug is caught
// immediately; in release builds we patch the cell with // immediately; in release builds we patch the cell
// `#ERROR!` so it displays something actionable. // with `#ERROR!` so it displays something rather
// than appearing empty.
// //
// Note: this check is O(changed_cells.len()), which is // Note: this check is O(changed_cells.len()), which
// typically 1 (single-cell edit) or the size of a // is typically 1 (single-cell edit) or the size of
// paste/autofill range. Not a hot path. // a paste/autofill range. Not a hot path.
let visited: HashSet<CellId> = sorted.iter().copied().collect();
for &cell_id in changed_cells { for &cell_id in changed_cells {
if let Some(cell) = self.cells.get(&cell_id) { 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!( debug_assert!(
false, false,
"recalculate_incremental did not visit formula cell ({}, {})", "recalculate_incremental left cell ({}, {}) with empty computed_value",
cell_id.row(), cell_id.row(),
cell_id.col() cell_id.col()
); );
@ -3025,428 +3024,3 @@ mod formula2_integration_tests {
assert_eq!(sheet.get_display_value(0, 1), "42"); 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!"));
}
}