From fdba6832460541339337ac2eb8dc6fc91d5da815 Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Sat, 14 Nov 2026 10:53:03 -0500 Subject: [PATCH] calc: linear dependency extraction for range-operator chains (#2183) (#2198) * calc: linear dependency extraction for range-operator chains (#2183) precedents() handled a range operator with non-reference operands by recursing into both operands and then letting Expr::walk descend into them again, so a left-associative chain of k operands cost 2^k (a 60-operand INDEX(...):INDEX(...) chain would hang on open). It is now a hand-written collector that visits every node once, growing the per-sheet bounding boxes of the enclosing range operator as it goes. Past a depth (4096) or node (1,000,000) bound it marks the formula dynamic (always recalculated) instead of recursing further. Co-Authored-By: Claude Opus 5.5 * calc: drop wall-clock timing from precedents tests (clippy) std::time::Instant::now is a disallowed method (clippy.toml: traps on wasm). The regression test only needs to finish; the old 2^k walk would never return. Co-Authored-By: Claude Opus 5.5 * calc: bound precedents to 100 ms and box plain-reference range chains (#2183) - range_operator_chain_precedents_are_linear times the direct precedents call against the 100 ms bound in the issue. - A Range node with two plain Ref operands (A1:B2:C3, which the parser can't fold) now collects its bounding box like any other range node, so an edit to C1 recalculates. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- crates/calc/src/eval/special.rs | 183 ++++++++++++++++++++++++-------- crates/calc/src/tests.rs | 79 ++++++++++++++ 2 files changed, 216 insertions(+), 46 deletions(-) diff --git a/crates/calc/src/eval/special.rs b/crates/calc/src/eval/special.rs index 0151ea2f9..2e7083457 100644 --- a/crates/calc/src/eval/special.rs +++ b/crates/calc/src/eval/special.rs @@ -27,62 +27,153 @@ pub fn resized_sum_range(sum: RangeRef, height: u32, width: u32) -> RangeRef { RangeRef::new(sum.start, CellRef::new(row, col)) } +/// Most expression nodes [`precedents`] visits before giving up and marking the formula dynamic. +const MAX_PRECEDENT_NODES: usize = 1_000_000; + /// Collects the static references of an expression with their sheets resolved; `dynamic` is /// set when the formula uses INDIRECT/OFFSET/names/tables whose targets can change. +/// +/// Every node is visited once (#2183: range-operator chains used to re-walk shared operands, +/// costing 2^k for k operands), with an explicit stack, so a flat chain as long as Excel allows +/// (`1+1+…`, about 4,000 levels deep) never recurses. Past [`MAX_PRECEDENT_NODES`] the result +/// is conservative: `dynamic` is set so the formula recalculates every time. pub fn precedents(wb: &Workbook, sheet: usize, e: &Expr) -> (Vec, bool) { - let mut out = Vec::new(); - let mut dynamic = false; - let push = |out: &mut Vec, r: &Reference, range: RangeRef| match &r.sheet { - SheetSel::Current => out.push(Area { sheet, range }), - SheetSel::Named(n) => { - if let Some(i) = wb.sheet_index(n) { - out.push(Area { sheet: i, range }); - } + let mut c = Precedents { wb, sheet, out: Vec::new(), dynamic: false, boxes: Vec::new() }; + c.collect(e); + (c.out, c.dynamic) +} + +/// Grows the per-sheet bounding box in `boxes` to cover `area`. +fn union_box(boxes: &mut Vec, area: Area) { + match boxes.iter_mut().find(|x| x.sheet == area.sheet) { + Some(x) => x.range = x.range.union(&area.range), + None => boxes.push(area), + } +} + +/// One entry of [`Precedents::collect`]'s explicit stack. `boxes` indexes +/// [`Precedents::boxes`]: the bounding boxes of the innermost enclosing range operator, if any. +enum PrecedentStep<'e> { + /// Visit an expression and queue its children. + Visit(&'e Expr, Option), + /// Both operands of a range operator have been visited: record the bounding boxes they + /// collected in `inner`, growing the `outer` operator's boxes too. + Close { inner: usize, outer: Option }, +} + +struct Precedents<'w> { + wb: &'w Workbook, + sheet: usize, + out: Vec, + dynamic: bool, + /// Per-sheet bounding boxes, one list per range operator (`A1:INDEX(…)`) met so far. + boxes: Vec>, +} + +impl Precedents<'_> { + /// Records `area`, also growing the bounding boxes of the enclosing range operator, if any. + fn add(&mut self, area: Area, boxes: Option) { + if let Some(b) = boxes.and_then(|i| self.boxes.get_mut(i)) { + union_box(b, area); } - SheetSel::Span(a, b) => { - if let (Some(i), Some(j)) = (wb.sheet_index(a), wb.sheet_index(b)) { - for s in i.min(j)..=i.max(j) { - out.push(Area { sheet: s, range }); + self.out.push(area); + } + + fn push(&mut self, r: &Reference, range: RangeRef, boxes: Option) { + match &r.sheet { + SheetSel::Current => self.add(Area { sheet: self.sheet, range }, boxes), + SheetSel::Named(n) => { + if let Some(i) = self.wb.sheet_index(n) { + self.add(Area { sheet: i, range }, boxes); + } + } + SheetSel::Span(a, b) => { + if let (Some(i), Some(j)) = (self.wb.sheet_index(a), self.wb.sheet_index(b)) { + for s in i.min(j)..=i.max(j) { + self.add(Area { sheet: s, range }, boxes); + } } } } - }; - e.walk(&mut |x| match x { - Expr::Ref(r) => push(&mut out, r, r.range()), - // The cells SUMIF/AVERAGEIF really read from the sum range (see `resized_sum_range`); when - // the criteria range's size isn't known statically, recalculate the formula every time. - Expr::Call(n, args) if resizes_sum_range(n, args.len()) => match (args.first(), args.get(2)) { - (Some(Expr::Ref(c)), Some(Expr::Ref(s))) => { - let (crit, sum) = (c.range(), s.range()); - push(&mut out, s, resized_sum_range(sum, crit.height(), crit.width())); - } - (_, Some(Expr::Number(_) | Expr::Text(_) | Expr::Bool(_) | Expr::Error(_) | Expr::Array(_) | Expr::Missing)) => {} - _ => dynamic = true, - }, - Expr::Call(n, _) - if matches!(n.as_str(), "INDIRECT" | "OFFSET" | "NOW" | "TODAY" | "RAND" | "RANDBETWEEN" | "RANDARRAY" | "CELL" | "INFO") => - { - dynamic = true - } - Expr::Name(_) | Expr::Struct(_) => dynamic = true, - Expr::Call(n, _) if gridcraft_functions::lookup(n).is_none() && !is_special(n) => dynamic = true, - // A range ending in a function's result (`A1:INDEX(B1:B9,n)`) covers the cells between - // the references inside it, not only those references. - Expr::Binary(BinOp::Range, a, b) if !matches!((&**a, &**b), (Expr::Ref(_), Expr::Ref(_))) => { - let mut inner = precedents(wb, sheet, a).0; - inner.extend(precedents(wb, sheet, b).0); - let mut boxes: Vec = Vec::new(); - for area in inner { - match boxes.iter_mut().find(|x| x.sheet == area.sheet) { - Some(x) => x.range = x.range.union(&area.range), - None => boxes.push(area), + } + + /// Visits every node of `root` once, parents before children, left to right. Areas found + /// inside a range operator (`A1:INDEX(…)`) also grow that operator's per-sheet bounding boxes. + fn collect(&mut self, root: &Expr) { + let mut pending = vec![PrecedentStep::Visit(root, None)]; + let mut nodes = 0usize; + while let Some(step) = pending.pop() { + let (x, boxes) = match step { + PrecedentStep::Visit(x, boxes) => (x, boxes), + PrecedentStep::Close { inner, outer } => { + let areas = self.boxes.get_mut(inner).map(std::mem::take).unwrap_or_default(); + for area in areas { + self.add(area, outer); + } + continue; } + }; + nodes = nodes.saturating_add(1); + if nodes > MAX_PRECEDENT_NODES { + self.dynamic = true; + return; } - out.extend(boxes); + self.visit(x, boxes, &mut pending); } - _ => {} - }); - (out, dynamic) + } + + /// Handles one node for [`Precedents::collect`], queueing the children still to visit. + fn visit<'e>(&mut self, x: &'e Expr, boxes: Option, pending: &mut Vec>) { + match x { + Expr::Ref(r) => self.push(r, r.range(), boxes), + // The cells SUMIF/AVERAGEIF really read from the sum range (see `resized_sum_range`); when + // the criteria range's size isn't known statically, recalculate the formula every time. + Expr::Call(n, args) if resizes_sum_range(n, args.len()) => match (args.first(), args.get(2)) { + (Some(Expr::Ref(c)), Some(Expr::Ref(s))) => { + let (crit, sum) = (c.range(), s.range()); + self.push(s, resized_sum_range(sum, crit.height(), crit.width()), boxes); + } + (_, Some(Expr::Number(_) | Expr::Text(_) | Expr::Bool(_) | Expr::Error(_) | Expr::Array(_) | Expr::Missing)) => {} + _ => self.dynamic = true, + }, + Expr::Call(n, _) + if matches!(n.as_str(), "INDIRECT" | "OFFSET" | "NOW" | "TODAY" | "RAND" | "RANDBETWEEN" | "RANDARRAY" | "CELL" | "INFO") => + { + self.dynamic = true + } + Expr::Name(_) | Expr::Struct(_) => self.dynamic = true, + Expr::Call(n, _) if gridcraft_functions::lookup(n).is_none() && !is_special(n) => self.dynamic = true, + // A range ending in a function's result (`A1:INDEX(B1:B9,n)`), or a chain of plain + // references the parser can't fold into one (`A1:B2:C3`), covers the cells between the + // references inside it, not only those references. Both operands are visited once, + // collecting their bounding boxes on the way, which `Close` then records. + Expr::Binary(BinOp::Range, a, b) => { + let inner = self.boxes.len(); + self.boxes.push(Vec::new()); + pending.push(PrecedentStep::Close { inner, outer: boxes }); + pending.push(PrecedentStep::Visit(b, Some(inner))); + pending.push(PrecedentStep::Visit(a, Some(inner))); + return; + } + _ => {} + } + // Children are pushed in reverse so they pop left to right. + let visit = |e: &'e Expr| PrecedentStep::Visit(e, boxes); + match x { + Expr::Array(rows) => pending.extend(rows.iter().flatten().rev().map(visit)), + Expr::Unary(_, e) | Expr::Paren(e) => pending.push(visit(e)), + Expr::Binary(_, a, b) => { + pending.push(visit(b)); + pending.push(visit(a)); + } + Expr::Call(_, args) => pending.extend(args.iter().rev().map(visit)), + Expr::Invoke(c, args) => { + pending.extend(args.iter().rev().map(visit)); + pending.push(visit(c)); + } + _ => {} + } + } } fn is_special(n: &str) -> bool { diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 7f6e753de..5ab61a8b8 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -2331,6 +2331,85 @@ fn median_large_values_recalculate_dependents() { } } +/// #2183: `INDEX(…):INDEX(…):…` used to re-walk each operand of the left-associative range chain, +/// costing 2^k for k operands (28 took 40 s; 60 would take millennia), on open and on every edit. +#[test] +fn range_operator_chain_precedents_are_linear() { + let operands: Vec = (1..=60).map(|i| format!("INDEX(A{i},1)")).collect(); + let src = format!("=SUM({})", operands.join(":")); + let e = gridcraft_formula::parse(&src).unwrap(); + let wb = Workbook::new(); + // With the old 2^k walk this call would never return; #2183 asks for under 100 ms. + #[allow(clippy::disallowed_methods)] // a native test: the clock is there + let t0 = std::time::Instant::now(); + let (areas, dynamic) = crate::eval::precedents(&wb, 0, &e); + let took = t0.elapsed(); + assert!(took < std::time::Duration::from_millis(100), "precedents took {took:?}"); + assert!(!dynamic); + // The chain covers A1:A60, not only the 60 cells named in it. + let whole = gridcraft_core::RangeRef::new(c("A1"), c("A60")); + assert!(areas.iter().any(|a| a.sheet == 0 && a.range == whole), "{areas:?}"); + // Each node is visited once: 60 references plus one box per range operator. + assert!(areas.len() < 200, "{} areas", areas.len()); + + // Through the recalculation engine: setting it finishes and editing a cell in the box recalculates. + let mut t = T::new(); + for i in 1..=60 { + t.set(&format!("A{i}"), "1"); + } + t.set("B1", &src); + assert_eq!(t.num("B1"), 60.0); + t.set("A30", "5"); + assert_eq!(t.num("B1"), 64.0); +} + +/// A chained range of plain references (`A1:B2:C3`, which the parser can't fold into one +/// reference) covers their bounding box, as the evaluator expands it: an edit inside the box but +/// outside each operand still recalculates. +#[test] +fn chained_plain_range_precedents_cover_the_bounding_box() { + let e = gridcraft_formula::parse("=SUM(A1:B2:C3)").unwrap(); + let (areas, dynamic) = crate::eval::precedents(&Workbook::new(), 0, &e); + assert!(!dynamic); + let whole = gridcraft_core::RangeRef::new(c("A1"), c("C3")); + assert!(areas.iter().any(|a| a.sheet == 0 && a.range == whole), "{areas:?}"); + + let mut t = T::new(); + t.set("A1", "1"); + t.set("E1", "=SUM(A1:B2:C3)"); + assert_eq!(t.num("E1"), 1.0); + t.set("C1", "4"); + assert_eq!(t.num("E1"), 5.0); +} + +/// `precedents` walks with an explicit stack: an expression nested deeper than any formula +/// within the length limit can build still finds its references, on a small stack. +#[test] +fn precedents_of_deep_nesting_do_not_recurse() { + let (areas, dynamic) = std::thread::Builder::new() + .stack_size(256 * 1024) + .spawn(|| { + let mut e = gridcraft_formula::parse("=A1").unwrap(); + for _ in 0..10_000 { + e = gridcraft_formula::Expr::Paren(Box::new(e)); + } + let found = crate::eval::precedents(&Workbook::new(), 0, &e); + // Dropping the deep tree recurses; leak it so only `precedents` is under test. + std::mem::forget(e); + found + }) + .unwrap() + .join() + .unwrap(); + assert!(!dynamic); + assert_eq!(areas.len(), 1); + // Shallow nesting stays static. + let e = gridcraft_formula::parse("=((((A1))))").unwrap(); + let (areas, dynamic) = crate::eval::precedents(&Workbook::new(), 0, &e); + assert!(!dynamic); + assert_eq!(areas.len(), 1); +} + #[test] fn cell_filename_names_the_workbook_file() { // Excel returns `/[]`, and the classic sheet-name formula splits it on