diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index a3daee641..fef44e047 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -60,6 +60,12 @@ const WIDE: u32 = 64; /// Insertions an [`Intervals`] keeps aside (scanned by every query) before merging them in. const PENDING: usize = 32; +/// Formulas a formula may calculate on demand one inside the other (each needing one another's +/// value before it is done). Each level takes stack, so a chain deeper than this is suspended and +/// calculated from the top instead (see [`PassHost::calculate`]): long dependency chains calculate +/// whatever order they come in, with bounded stack. +const MAX_NESTED: usize = 64; + /// Formula evaluations iterative calculation may spend on one recalculation (passes × formulas /// evaluated again), however high Maximum Iterations is. const MAX_ITERATION_WORK: usize = 10_000_000; @@ -395,6 +401,24 @@ struct PassHost<'a> { cycle: bool, cycles: Vec, depth: usize, + /// A formula needed at [`MAX_NESTED`] levels deep: the evaluations in progress are abandoned + /// and it is calculated first. + suspend: Option, + /// Formulas whose evaluation was abandoned for another one, to be calculated again after it. + /// They all need it, so reading one of them while it is calculated is a circular reference, + /// as reading a formula in progress is. + waiting: HashSet, + /// The formulas abandoned since the last suspension. + abandoned: Vec, + /// Other formulas read at [`MAX_NESTED`] levels deep by the evaluations being abandoned. They + /// are calculated along with the suspended one, rather than one per restart (a wide range + /// read at the limit would restart the abandoned evaluations once per cell in it). + also: Vec, + /// Suspensions so far (tests check that a wide read at the limit needs one, not one per cell). + #[cfg(test)] + suspensions: usize, + /// The formulas in `cycles`. + cycle_seen: HashSet, /// Iterative calculation: a circular reference reads the cell's previous value instead of /// giving `#CIRC!`. iterate: bool, @@ -425,6 +449,13 @@ impl<'a> PassHost<'a> { cycle: false, cycles: vec![], depth: 0, + suspend: None, + waiting: HashSet::default(), + abandoned: Vec::new(), + also: Vec::new(), + #[cfg(test)] + suspensions: 0, + cycle_seen: HashSet::default(), iterate: false, prev: HashMap::default(), stack: Vec::new(), @@ -471,6 +502,50 @@ fn spills_as_before(sheet: &gridcraft_model::Sheet, anchor: CellRef, range: Rang } impl PassHost<'_> { + /// Calculates `k` (and the formulas it reads) with bounded stack: when [`Self::compute`] would + /// go deeper than [`MAX_NESTED`], it suspends what is in progress, the formula it needed is + /// calculated from here, then the suspended ones again (reading the results kept so far). + fn calculate(&mut self, k: Key) { + // Formulas to calculate, innermost last, each with the ones abandoned to calculate the + // next. + let mut work: Vec<(Key, Vec)> = vec![(k, Vec::new())]; + while let Some(&(top, _)) = work.last() { + self.compute(top); + match self.suspend.take() { + Some(next) => { + #[cfg(test)] + { + self.suspensions += 1; + } + let held = std::mem::take(&mut self.abandoned); + self.waiting.extend(held.iter().copied()); + if let Some(entry) = work.last_mut() { + entry.1 = held; + } + // The other formulas read at the limit are calculated after `next`, before the + // held evaluations restart. Each has nothing held of its own, so the held ones + // are released only when the last of them is done. + let mut seen: HashSet = HashSet::default(); + seen.insert(next); + for s in std::mem::take(&mut self.also) { + if seen.insert(s) { + work.push((s, Vec::new())); + } + } + work.push((next, Vec::new())); + } + None => { + work.pop(); + if let Some(entry) = work.last_mut() { + for a in entry.1.drain(..) { + self.waiting.remove(&a); + } + } + } + } + } + } + fn compute(&mut self, k: Key) -> Value { if let Some(v) = self.results.get(&k) { let v = v.clone(); @@ -482,14 +557,29 @@ impl PassHost<'_> { let Some(expr) = self.exprs.get(&k) else { return self.wb.sheet(k.0).map(|s| s.value(k.1)).unwrap_or_default(); }; - if self.in_progress.contains(&k) || self.depth > 2000 { + if self.suspend.is_some() { + // Unwinding to `calculate`: the value is thrown away. A formula read at the limit is + // calculated with the suspended one, so the evaluations are not restarted for each. + if self.depth >= MAX_NESTED && !self.in_progress.contains(&k) && !self.waiting.contains(&k) { + self.also.push(k); + } + return Value::Error(CellError::Calc); + } + if self.in_progress.contains(&k) || self.waiting.contains(&k) { self.cycle = true; if self.iterate { return self.circular_read(k); } - self.cycles.push(k); + // Once each, though a suspended evaluation meets it again. + if self.cycle_seen.insert(k) { + self.cycles.push(k); + } return Value::Error(CellError::Circ); } + if self.depth >= MAX_NESTED { + self.suspend = Some(k); + return Value::Error(CellError::Calc); + } self.in_progress.insert(k); self.depth += 1; let outer_taint = std::mem::replace(&mut self.taint, false); @@ -501,6 +591,17 @@ impl PassHost<'_> { let mut ev = Evaluator::new(self, k.0, k.1); ev.value(&expr) }; + if self.suspend.is_some() { + // Abandoned: `calculate` evaluates this formula again once the one it needs is done. + self.depth -= 1; + self.in_progress.remove(&k); + self.abandoned.push(k); + if self.iterate { + self.stack.pop(); + } + self.taint = outer_taint; + return Value::Error(CellError::Calc); + } // A legacy array formula fills the range it was entered in. let legacy = self.wb.sheet(k.0).and_then(|s| s.cell(k.1)).and_then(|c| c.formula.as_ref()).and_then(|f| f.array); let v = match legacy { @@ -591,7 +692,7 @@ impl PassHost<'_> { self.pending = set.clone(); self.spills.retain(|a, _| !set.contains(a)); for k in &again { - self.compute(*k); + self.calculate(*k); } passes += 1; } @@ -984,7 +1085,7 @@ impl Calc { let mut host = PassHost::new(wb, &exprs, &mut rng, now); host.iterate = wb.calc.iterative; for k in &dirty { - host.compute(*k); + host.calculate(*k); } if host.iterate && !host.members.is_empty() { let passes = host.iterate_loops(&dirty, wb.calc.max_iterations, wb.calc.max_change); @@ -1345,6 +1446,35 @@ pub(crate) mod tests { assert_eq!(deps, vec![at("DA9"), at("DA9")]); } + /// A formula at the nesting limit reading thousands of formulas not calculated yet suspends the + /// evaluations in progress once, not once per formula read. + #[test] + fn wide_read_at_the_nesting_limit_suspends_once() { + const WIDTH: u32 = 3000; + let mut wb = Workbook::new(); + let mut exprs = HashMap::default(); + let mut put = |wb: &mut Workbook, at: String, text: String| { + let at = CellRef::parse(&at).unwrap(); + wb.sheet_mut(0).unwrap().cells.set(at, Cell::formula(Formula::new(&text))); + exprs.insert((0, at), std::sync::Arc::new(gridcraft_formula::parse(&text[1..]).unwrap())); + }; + // A1 reads A2, ..., A64 (the 64th formula deep) reads a wide range of formulas. + for r in 1..64 { + put(&mut wb, format!("A{r}"), format!("=A{}+1", r + 1)); + } + put(&mut wb, "A64".to_string(), format!("=SUM(B1:B{WIDTH})")); + for r in 1..=WIDTH { + put(&mut wb, format!("B{r}"), format!("={r}*2")); + } + let mut rng = 1u64; + let mut host = PassHost::new(&wb, &exprs, &mut rng, 0.0); + host.calculate((0, CellRef::parse("A1").unwrap())); + let total = (WIDTH * (WIDTH + 1)) as f64; + assert_eq!(host.results.get(&(0, CellRef::parse("A1").unwrap())), Some(&Value::Number(total + 63.0))); + assert!(host.cycles.is_empty()); + assert!(host.suspensions <= 3, "{} suspensions", host.suspensions); + } + /// For tests of `PassHost::visit_range`, which can't be reached from outside this module: the /// values it hands over for `range` while the formulas in `pending` (cell, formula) have not been /// calculated yet, or `None` when it declines the block. diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index f5c11b7e6..9763f7aeb 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1286,6 +1286,94 @@ fn offset_extreme_arguments_return_ref() { } } +/// A formula reading one further down (or on a later sheet) is calculated on demand, inside the +/// formula that reads it. Chains of thousands of such formulas, common in financial models, used +/// to overflow the stack (or give `#CIRC!` past 2,000 levels); they now calculate in full. +#[test] +fn long_dependency_chains_against_row_order_calculate() { + const N: u32 = 20_000; + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..N { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c(&format!("A{N}")), Cell::value(Value::Number(1.0))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.num("A1"), N as f64); + assert_eq!(t.num("A2"), (N - 1) as f64); + assert!(t.calc.circular.is_empty()); + // An edit at the far end recalculates the whole chain. + t.set(&format!("A{N}"), "10"); + assert_eq!(t.num("A1"), (N + 9) as f64); + assert!(t.calc.circular.is_empty()); + + // Hopping between two sheets at every step, through a range read (SUM) as well. + let mut t = T::new(); + t.wb.sheets.push(std::sync::Arc::new(gridcraft_model::Sheet::new("Data"))); + for r in 1..N { + t.wb.sheet_mut(0).unwrap().cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=Data!A{r}+1")))); + let next = format!("=SUM(Sheet1!A{}:A{})", r + 1, r + 1); + t.wb.sheet_mut(1).unwrap().cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&next))); + } + t.wb.sheet_mut(0).unwrap().cells.set(c(&format!("A{N}")), Cell::value(Value::Number(0.0))); + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.num("A1"), (N - 1) as f64); + assert_eq!(t.wb.sheet(1).unwrap().value(c("A1")), Value::Number((N - 2) as f64)); + assert!(t.calc.circular.is_empty()); +} + +/// A loop longer than the on-demand nesting limit is still a circular reference (and still ends). +#[test] +fn long_circular_chain_is_circular() { + const N: u32 = 5_000; + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..N { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c(&format!("A{N}")), Cell::formula(Formula::new("=A1+1"))); + sh.cells.set(c("B1"), Cell::formula(Formula::new("=1+2"))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.get("A1"), Value::Error(CellError::Circ)); + assert_eq!(t.get(&format!("A{N}")), Value::Error(CellError::Circ)); + assert_eq!(t.num("B1"), 3.0); + assert!(!t.calc.circular.is_empty()); + // Each formula is listed once at most. + let mut seen = t.calc.circular.clone(); + seen.sort_by_key(|(s, c)| (*s, c.row, c.col)); + seen.dedup(); + assert_eq!(seen.len(), t.calc.circular.len()); + // With iterative calculation the loop settles to numbers instead (no hang, no crash). + t.wb.calc.iterative = true; + t.wb.calc.max_iterations = 3; + t.calc.recalc_all(&mut t.wb); + assert!(t.get("A1").as_f64().is_some()); +} + +/// A loop closing back into the middle of a chain deeper than the on-demand nesting limit is found +/// where calculating it in one go finds it: at the formula read while it was being calculated. +#[test] +fn deep_loop_is_found_where_it_closes() { + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..300 { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c("A300"), Cell::formula(Formula::new("=A30+1"))); + // Reads the loop through IFERROR, so its value shows where the loop was cut. + sh.cells.set(c("B1"), Cell::formula(Formula::new("=IFERROR(A1,-1)"))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.calc.circular, vec![(0, c("A30"))]); + assert_eq!(t.get("A1"), Value::Error(CellError::Circ)); + assert_eq!(t.num("B1"), -1.0); +} + #[test] fn median_large_values_recalculate_dependents() { let mut t = T::new();