From c567511cf1e5335e4078e5bdd2036e14e72293c6 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Mon, 26 Oct 2026 22:21:15 +0100 Subject: [PATCH 1/2] calc: rebuild the dependency graph from shared syntax trees Calc::rebuild and Calc::cells_changed called Formula::expr(), which deep-copies each formula's syntax tree only to read its references. Use expr_arc() (already used when formulas are calculated), which hands out the cached Arc without copying. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/recalc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 17a0e6649..6cd1551fe 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -1010,7 +1010,7 @@ impl Calc { for (si, sheet) in wb.sheets.iter().enumerate() { for (c, cell) in sheet.cells.iter() { if let Some(f) = &cell.formula - && let Some(e) = f.expr() + && let Some(e) = f.expr_arc() { let (areas, dynamic) = precedents(wb, si, &e); self.graph.insert((si, c), Node { areas, dynamic }); @@ -1088,7 +1088,7 @@ impl Calc { let mut cleared: Vec<(usize, RangeRef)> = Vec::new(); for &k in changed { let formula = wb.sheet(k.0).and_then(|s| s.cell(k.1)).and_then(|c| c.formula.clone()); - match formula.and_then(|f| f.expr()) { + match formula.and_then(|f| f.expr_arc()) { Some(e) => { let (areas, dynamic) = precedents(wb, k.0, &e); self.graph.insert(k, Node { areas, dynamic }); From b6bbdcd5cf23626171768d0cf061e4b0319a67c1 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 29 Oct 2026 00:51:45 +0100 Subject: [PATCH 2/2] calc: build precedents from a borrowed syntax tree, no Arc for text-only formulas Formula::with_expr runs a closure on the cached tree, or on a freshly parsed one that is dropped afterwards, so rebuild and cells_changed need neither a tree copy nor an Arc allocation / reference count. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/recalc.rs | 11 +++++------ crates/calc/src/tests.rs | 39 +++++++++++++++++++++++++++++++++++++++ crates/model/src/cell.rs | 10 ++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 34c712ddc..9886ca9bd 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -128,9 +128,8 @@ impl Calc { for (si, sheet) in wb.sheets.iter().enumerate() { for (c, cell) in sheet.cells.iter() { if let Some(f) = &cell.formula - && let Some(e) = f.expr_arc() + && let Some((areas, dynamic)) = f.with_expr(|e| precedents(wb, si, e)) { - let (areas, dynamic) = precedents(wb, si, &e); self.graph.insert((si, c), Node { areas, dynamic }); if cell.value == Value::Error(CellError::Circ) { circ.push((si, c)); @@ -205,10 +204,10 @@ impl Calc { pub fn cells_changed(&mut self, wb: &mut Workbook, changed: &[Key]) { let mut cleared: Vec<(usize, RangeRef)> = Vec::new(); for &k in changed { - let formula = wb.sheet(k.0).and_then(|s| s.cell(k.1)).and_then(|c| c.formula.clone()); - match formula.and_then(|f| f.expr_arc()) { - Some(e) => { - let (areas, dynamic) = precedents(wb, k.0, &e); + let precedent = + wb.sheet(k.0).and_then(|s| s.cell(k.1)).and_then(|c| c.formula.as_ref()).and_then(|f| f.with_expr(|e| precedents(wb, k.0, e))); + match precedent { + Some((areas, dynamic)) => { self.graph.insert(k, Node { areas, dynamic }); } None => { diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 5b9e90085..0d4c973d4 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -2274,3 +2274,42 @@ fn loan_schedule_residue_keeps_excels_sign() { } assert_eq!(t.get("P7"), Value::Text("".into())); } + +/// `rebuild` and `cells_changed` read formulas through `Formula::with_expr`, which borrows the +/// cached tree or parses on the fly. The dependency graph must come out the same for formulas that +/// carry their tree and for formulas that only carry their text (as after loading a file). +#[test] +fn graph_built_from_text_only_formulas_matches_cached() { + fn run(cached: bool) -> Vec { + let mut t = T::new(); + t.set("A1", "2"); + for (at, f) in [("B1", "=A1*10"), ("C1", "=B1+A1"), ("D1", "=SUM(A1:C1)"), ("E1", "=NOPE(")] { + // E1 does not parse, so it has no tree either way and only covers the `None` branch. + let mut formula = Formula::new(f); + if !cached { + formula.expr = None; + } + t.wb.sheet_mut(0).unwrap().cells.set(c(at), Cell::formula(formula)); + } + t.calc.rebuild(&t.wb); + t.calc.recalc_all(&mut t.wb); + let mut out: Vec = ["B1", "C1", "D1", "E1"].iter().map(|a| t.get(a)).collect(); + // Editing the input must reach the dependents through the rebuilt graph. + t.set("A1", "5"); + out.extend(["B1", "C1", "D1"].iter().map(|a| t.get(a))); + // A text-only formula that is rewritten through `cells_changed` joins the graph too. + let mut formula = Formula::new("=A1+100"); + if !cached { + formula.expr = None; + } + t.wb.sheet_mut(0).unwrap().cells.set(c("F1"), Cell::formula(formula)); + t.calc.cells_changed(&mut t.wb, &[(0, c("F1"))]); + t.set("A1", "7"); + out.push(t.get("F1")); + out + } + let cached = run(true); + assert_eq!(cached, run(false)); + assert_eq!(cached[4], Value::Number(50.0)); + assert_eq!(cached[7], Value::Number(107.0)); +} diff --git a/crates/model/src/cell.rs b/crates/model/src/cell.rs index d4466365f..7d3a7715e 100644 --- a/crates/model/src/cell.rs +++ b/crates/model/src/cell.rs @@ -46,6 +46,16 @@ impl Formula { pub fn expr_arc(&self) -> Option> { self.expr.clone().or_else(|| gridcraft_formula::parse(&self.text).ok().map(Arc::new)) } + /// Runs `f` on the parsed expression, borrowing the cached tree or, after deserialization, + /// a freshly parsed one that is dropped afterwards. Unlike [`expr`](Self::expr) it never copies + /// the tree, and unlike [`expr_arc`](Self::expr_arc) it needs no reference count or `Arc` + /// allocation. `None` when the text does not parse. + pub fn with_expr(&self, f: impl FnOnce(&Expr) -> R) -> Option { + match &self.expr { + Some(e) => Some(f(e)), + None => gridcraft_formula::parse(&self.text).ok().map(|e| f(&e)), + } + } } /// A static picture stored as cell content, independent of floating drawing anchors.