From 8605fb02f0da1c6fd96a45bf13999f28e157d157 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 12:46:16 +0100 Subject: [PATCH] Charts: follow defined names when deciding a chart is volatile A series like =FreshClock, where the workbook name FreshClock is NOW(), was cached because only the literal series text was searched for volatile calls. The check now also follows defined names the formulas use (workbook or sheet scope, bare or sheet-qualified, LAMBDA names), the way the evaluator looks them up. Nesting deeper than 8, cycles and more than 64 lookups per chart count as volatile, so a chart whose stability isn't established is not cached. Co-Authored-By: Claude Opus 5.5 --- crates/chart/src/resolve.rs | 143 +++++++++++++++++++++++++++----- crates/chart/src/tests.rs | 82 ++++++++++++++++-- crates/ui-egui/src/chartview.rs | 5 +- 3 files changed, 198 insertions(+), 32 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index 92ce0eb23..29158ebc7 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -155,26 +155,131 @@ fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, limit: usize, depth: } } -/// Whether a series formula of `ch` calls a function whose result changes between evaluations -/// (`NOW`, `TODAY`, `RAND`, `RANDBETWEEN`, `RANDARRAY`), so a result resolved earlier is stale. -pub fn has_volatile_series(ch: &Chart) -> bool { - const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; - let calls_volatile = |f: &str| { - // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. - let up = f.to_ascii_uppercase().replace("_XLFN.", ""); - VOLATILE.iter().any(|name| { - up.match_indices(name).any(|(i, _)| { - let before = up[..i].chars().next_back(); - let after = up[i + name.len()..].trim_start(); - before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') - }) - }) - }; +/// Whether a series formula of `ch` (chart of `sheet`) calls a function whose result changes +/// between evaluations (`NOW`, `TODAY`, `RAND`, `RANDBETWEEN`, `RANDARRAY`), so a result resolved +/// earlier is stale. Defined names the formulas use (`=FreshClock`, `Sheet1!Rates`, a LAMBDA name) +/// are followed into their own formulas, the way the evaluator looks them up; each name once per +/// chart, so a cycle ends. Names nested deeper than [`MAX_NAME_DEPTH`], or more than +/// [`MAX_NAMES_FOLLOWED`] distinct names, count as volatile: a chart whose stability isn't +/// established is not cached. +pub fn has_volatile_series(wb: &Workbook, sheet: usize, ch: &Chart) -> bool { + let mut seen = Seen::new(); ch.series.iter().any(|s| { - [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()].into_iter().flatten().any(calls_volatile) + [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()] + .into_iter() + .flatten() + .any(|f| formula_volatile(wb, sheet, f, 0, &mut seen)) }) } +/// Names followed inside names before a formula counts as volatile. +const MAX_NAME_DEPTH: u32 = 8; +/// Distinct defined names followed per chart before the chart counts as volatile. +const MAX_NAMES_FOLLOWED: usize = 64; + +/// Defined names already followed for one chart, each with the sheet its formula is read on. +type Seen<'a> = Vec<(&'a gridcraft_model::DefinedName, usize)>; + +fn formula_volatile<'a>(wb: &'a Workbook, sheet: usize, f: &str, depth: u32, seen: &mut Seen<'a>) -> bool { + if calls_volatile(f) { + return true; + } + name_tokens(f).into_iter().any(|t| { + let Some((n, at)) = defined_name(wb, sheet, t) else { return false }; + // Followed already, or being followed (a cycle): a volatile call in it is found there. + if seen.iter().any(|&(m, s)| std::ptr::eq(m, n) && s == at) { + return false; + } + if depth >= MAX_NAME_DEPTH || seen.len() >= MAX_NAMES_FOLLOWED { + return true; + } + seen.push((n, at)); + formula_volatile(wb, at, &n.formula, depth + 1, seen) + }) +} + +/// Whether formula text `f` calls one of the volatile functions by name. +fn calls_volatile(f: &str) -> bool { + const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; + // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. + let up = f.to_ascii_uppercase().replace("_XLFN.", ""); + VOLATILE.iter().any(|name| { + up.match_indices(name).any(|(i, _)| { + let before = up[..i].chars().next_back(); + let after = up[i + name.len()..].trim_start(); + before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') + }) + }) +} + +fn is_ident_char(c: char) -> bool { + c.is_alphanumeric() || c == '_' || c == '.' || c == '\\' +} + +/// End of the identifier starting at byte `from` of `f`. +fn ident_end(f: &str, from: usize) -> usize { + f[from..].char_indices().find(|&(_, c)| !is_ident_char(c)).map_or(f.len(), |(j, _)| from + j) +} + +/// End (past the closing quote) of the `q`-quoted text opening at byte `from`; a doubled quote +/// is an escaped one. `None` when it is not closed. +fn quoted_end(f: &str, from: usize, q: char) -> Option { + let mut i = from + q.len_utf8(); + loop { + let j = i + f[i..].find(q)?; + if f[j + q.len_utf8()..].starts_with(q) { + i = j + 2 * q.len_utf8(); + } else { + return Some(j + q.len_utf8()); + } + } +} + +/// The words of formula text `f` that may be defined names: `Name`, `Sheet1!Name`, +/// `'Bob''s'!Name`. String literals and numbers are skipped; function names, column letters and +/// the like come along too and simply aren't found as names. +fn name_tokens(f: &str) -> Vec<&str> { + let mut out = Vec::new(); + let mut i = 0; + while let Some(c) = f[i..].chars().next() { + let start = i; + let end = match c { + '"' => { + i = quoted_end(f, i, '"').unwrap_or(f.len()); + continue; + } + '\'' => quoted_end(f, i, '\'').unwrap_or(f.len()), + c if is_ident_char(c) => ident_end(f, i), + c => { + i += c.len_utf8(); + continue; + } + }; + i = end; + if f[end..].starts_with('!') { + let e = ident_end(f, end + 1); + if e > end + 1 { + out.push(&f[start..e]); + } + i = e; + } else if c != '\'' && !c.is_ascii_digit() { + out.push(&f[start..end]); + } + } + out +} + +/// The defined name `t` (`Name` or `Sheet1!Name`) seen from `sheet`, and the sheet its own +/// formula is read on. +fn defined_name<'a>(wb: &'a Workbook, sheet: usize, t: &str) -> Option<(&'a gridcraft_model::DefinedName, usize)> { + // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). + let (at, bare) = match gridcraft_core::split_sheet(t) { + (Some(s), bare) => (wb.sheet_index(&s)?, bare), + (None, bare) => (sheet, bare), + }; + wb.name(bare, at).map(|n| (n, n.scope.unwrap_or(at))) +} + fn to_num(v: &Value) -> Option { match v { Value::Number(n) if n.is_finite() => Some(*n), @@ -278,11 +383,7 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str, limi /// Whether `t` names a defined name seen from `sheet` (`Name`, or `Sheet1!Name` looked up on /// that sheet), the way the evaluator looks names up. fn is_defined_name(wb: &Workbook, sheet: usize, t: &str) -> bool { - // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). - match gridcraft_core::split_sheet(t) { - (Some(s), bare) => wb.sheet_index(&s).is_some_and(|i| wb.name(bare, i).is_some()), - (None, _) => wb.name(t, sheet).is_some(), - } + defined_name(wb, sheet, t).is_some() } /// A series name: a reference/formula or a defined name (`[0]!Title` in a file) is evaluated, diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 1ecabd997..a6d845387 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1683,6 +1683,7 @@ fn a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves() { #[test] fn volatile_series_formulas_are_detected() { + let wb = Workbook::new(); let with = |values: &str, name: Option<&str>| { let mut c = chart(ChartKind::Line); c.series.push(Series { @@ -1699,13 +1700,76 @@ fn volatile_series_formulas_are_detected() { }); c }; - assert!(has_volatile_series(&with("=NOW()", None))); - assert!(has_volatile_series(&with("=SUM(A1:A3)*rand ()", None))); - assert!(has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); - assert!(has_volatile_series(&with("=RANDARRAY(5)", None))); - assert!(has_volatile_series(&with("=_xlfn.RANDARRAY(5)", None))); - assert!(!has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); - assert!(!has_volatile_series(&with("=MYRAND(A1)", None))); - assert!(!has_volatile_series(&with("=Sheet1!$NOW", None))); - assert!(!has_volatile_series(&chart(ChartKind::Line))); + assert!(has_volatile_series(&wb, 0, &with("=NOW()", None))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(A1:A3)*rand ()", None))); + assert!(has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); + assert!(has_volatile_series(&wb, 0, &with("=RANDARRAY(5)", None))); + assert!(has_volatile_series(&wb, 0, &with("=_xlfn.RANDARRAY(5)", None))); + assert!(!has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); + assert!(!has_volatile_series(&wb, 0, &with("=MYRAND(A1)", None))); + assert!(!has_volatile_series(&wb, 0, &with("=Sheet1!$NOW", None))); + assert!(!has_volatile_series(&wb, 0, &chart(ChartKind::Line))); +} + +#[test] +fn volatile_defined_names_are_followed() { + let with = |values: &str| { + let mut c = chart(ChartKind::Line); + c.series.push(Series { + name: None, + categories: None, + values: values.into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + smooth: false, + quartiles: Default::default(), + trendlines: vec![], + }); + c + }; + let name = |name: &str, scope: Option, formula: &str| gridcraft_model::DefinedName { + name: name.into(), + scope, + formula: formula.into(), + comment: String::new(), + hidden: false, + }; + let mut wb = Workbook::new(); + wb.sheet_mut(0).unwrap().name = "Bob's".into(); + wb.names.push(name("FreshClock", None, "NOW()")); + wb.names.push(name("Later", None, "FreshClock+1")); + wb.names.push(name("Dice", Some(0), "RANDBETWEEN(1,6)")); + wb.names.push(name("Rates", None, "'Bob''s'!$A$1:$A$3")); + wb.names.push(name("Loop", None, "Loop+1")); + wb.names.push(name("Spin", None, "Spin+NOW()")); + wb.names.push(name("Roll", None, "LAMBDA(n,n*RAND())")); + // A workbook-scoped name, a name used inside another, and a sheet-scoped one, bare or qualified. + assert!(has_volatile_series(&wb, 0, &with("=FreshClock"))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(Later, 1)"))); + assert!(has_volatile_series(&wb, 0, &with("=Dice*2"))); + assert!(has_volatile_series(&wb, 0, &with("='Bob''s'!Dice"))); + assert!(has_volatile_series(&wb, 0, &with("=Roll(3)"))); + // A self-referencing name ends (it evaluates to an error, not a changing value); one that + // also calls NOW is volatile. + assert!(!has_volatile_series(&wb, 0, &with("=Loop"))); + assert!(has_volatile_series(&wb, 0, &with("=Spin"))); + // Many series over the same stable name stay cacheable: each name is followed once. + let mut many = with("=Rates"); + let first = many.series[0].clone(); + many.series.extend(std::iter::repeat_n(first, 100)); + assert!(!has_volatile_series(&wb, 0, &many)); + // Names nested deeper than the limit are not known to be stable. + for i in 0..10 { + wb.names.push(name(&format!("Deep{i}"), None, &format!("Deep{}", i + 1))); + } + assert!(has_volatile_series(&wb, 0, &with("=Deep0"))); + // A name that refers to cells, a name inside a string and a name of another scope are stable. + assert!(!has_volatile_series(&wb, 0, &with("=Rates"))); + assert!(!has_volatile_series(&wb, 0, &with("=\"FreshClock\""))); + // Dice belongs to sheet 0: seen from another sheet it isn't defined. + wb.sheets.push(Arc::new(gridcraft_model::Sheet::new("Other"))); + assert!(!has_volatile_series(&wb, 1, &with("=Dice"))); + assert!(has_volatile_series(&wb, 1, &with("=FreshClock"))); } diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 6196878b4..a95b97d57 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -258,8 +258,9 @@ fn chart_prims(app: &SheetApp, p: &Painter, z: f32, wb: &Arc, si: usiz let Some(d) = app.session.active().filter(|d| Arc::ptr_eq(&d.wb, wb)) else { return render_chart(p, z, wb, si, ch).into(); }; - // A series that calls NOW or RAND must be evaluated again on every paint. - if gridcraft_chart::has_volatile_series(ch) { + // A series that calls NOW or RAND, directly or through a defined name, must be evaluated + // again on every paint. + if gridcraft_chart::has_volatile_series(wb, si, ch) { return render_chart(p, z, wb, si, ch).into(); } let mut cache = app.chart_cache.lock().unwrap_or_else(std::sync::PoisonError::into_inner);