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);