From 572bec289bdf5caaced596a11e272033cf7891b6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 16:10:10 -0500 Subject: [PATCH] engine: work limits for Advanced Filter, Remove Duplicates, filters, AutoFit, macro footprints; range sizes saturate (#3295) * engine: bound Advanced Filter, Remove Duplicates, AutoFilter, Circle Invalid Data; one-pass AutoFit; saturating range dimensions * address review: AutoFit scans each run of adjacent selected columns separately * address review: report a truncated circle-invalid scan, no spill scan for plain cells in Replace, one pass for many AutoFit runs * address review: warn in the app when Circle Invalid Data stopped early; Replace test requires success * address review: the scan note joins an open-time notice; say cell visits --- crates/core/src/addr.rs | 11 ++++- crates/engine/src/cmd/data.rs | 2 +- crates/engine/src/cmd/data/filter.rs | 3 +- crates/engine/src/cmd/data/tools.rs | 7 +++ crates/engine/src/cmd/data/validation.rs | 10 ++++- crates/engine/src/cmd/edit/find.rs | 6 ++- crates/engine/src/cmd/format.rs | 15 +++++++ crates/engine/src/cmd/format/sizes.rs | 43 +++++++++++++------ .../engine/src/cmd/whatif/advanced_filter.rs | 7 +++ crates/engine/src/cmd/whatif/tests.rs | 9 ++++ crates/engine/src/macros/run.rs | 15 +++++-- crates/engine/src/tests.rs | 26 +++++++++++ crates/ui-egui/src/app/commands.rs | 17 +++++++- crates/ui-egui/tests/it/interaction.rs | 22 ++++++++++ 14 files changed, 170 insertions(+), 23 deletions(-) diff --git a/crates/core/src/addr.rs b/crates/core/src/addr.rs index 498c3f068..dfe32498a 100644 --- a/crates/core/src/addr.rs +++ b/crates/core/src/addr.rs @@ -147,10 +147,10 @@ impl RangeRef { RangeRef::new(CellRef::new(0, 0), CellRef::new(MAX_ROWS - 1, MAX_COLS - 1)) } pub fn height(&self) -> u32 { - self.end.row - self.start.row + 1 + self.end.row.saturating_sub(self.start.row).saturating_add(1) } pub fn width(&self) -> u32 { - self.end.col - self.start.col + 1 + self.end.col.saturating_sub(self.start.col).saturating_add(1) } pub fn count(&self) -> u64 { self.height() as u64 * self.width() as u64 @@ -321,6 +321,13 @@ pub fn split_areas(reference: &str) -> Vec<&str> { mod tests { use super::*; + #[test] + fn hostile_range_dimensions_saturate_instead_of_wrapping() { + let r = RangeRef { start: CellRef::new(0, 0), end: CellRef::new(u32::MAX, u32::MAX) }; + assert_eq!((r.height(), r.width()), (u32::MAX, u32::MAX)); + assert_eq!(r.count(), u64::from(u32::MAX) * u64::from(u32::MAX)); + } + #[test] fn areas_split_only_at_commas_outside_quoted_sheet_names() { assert_eq!(split_areas("B2:B4,D1"), ["B2:B4", "D1"]); diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 0906f4893..c87906dee 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -98,7 +98,7 @@ pub fn specs() -> Vec { validation ), cmd!(query "data.validate", "Check Value Against Validation", [], None, "{cell?, input} → {ok, message?}", has_doc, validate_cmd), - cmd!(noundo "data.circleInvalid", "Circle Invalid Data", ["Data", "Data Tools", "Data Validation"], None, "{} → invalid cells", has_doc, circle_invalid), + cmd!(noundo "data.circleInvalid", "Circle Invalid Data", ["Data", "Data Tools", "Data Validation"], None, "{} → {invalid: [cell], truncated: bool (true when the 100,000-cell scan allowance ended before every range was checked)}", has_doc, circle_invalid), CommandSpec { shortcut_mac: Some("Cmd+Shift+K"), ..cmd!( diff --git a/crates/engine/src/cmd/data/filter.rs b/crates/engine/src/cmd/data/filter.rs index cda2160bf..179dc04a1 100644 --- a/crates/engine/src/cmd/data/filter.rs +++ b/crates/engine/src/cmd/data/filter.rs @@ -184,7 +184,8 @@ pub(crate) fn grow_autofilter(sh: &mut Sheet, at: CellRef) { pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) { let Some(sh) = wb.sheet(sheet) else { return }; let Some(af) = sh.autofilter.clone() else { return }; - let r = af.range; + // An imported range can name rows past the grid; none of those exist to hide. + let r = RangeRef::new(af.range.start, CellRef::new(af.range.end.row.min(gridcraft_core::MAX_ROWS - 1), af.range.end.col)); let mut hide = Vec::new(); // Precompute per-criterion top10 threshold and average once, not per row. let mut col_stats: std::collections::HashMap, f64)> = std::collections::HashMap::new(); diff --git a/crates/engine/src/cmd/data/tools.rs b/crates/engine/src/cmd/data/tools.rs index c4ae43ef4..e4b9f028f 100644 --- a/crates/engine/src/cmd/data/tools.rs +++ b/crates/engine/src/cmd/data/tools.rs @@ -5,6 +5,9 @@ use super::{ // ---------------------------------------------------------------- tools +/// Most cells (rows times compared columns) one Remove Duplicates call reads. +const MAX_COMPARED_CELLS: u64 = 50_000_000; + pub(super) fn remove_duplicates(s: &mut Session, p: &Json) -> Result { let (sheet, r) = data_target(s, p, "data.removeDuplicates")?; let d = s.doc()?; @@ -24,6 +27,10 @@ pub(super) fn remove_duplicates(s: &mut Session, p: &Json) -> Result { if cols.is_empty() || cols.iter().any(|c| !(r.start.col..=r.end.col).contains(c)) { return Err(bad("data.removeDuplicates", "`columns` must name at least one column of the range")); } + // Rows are compared one by one, so a sparse sheet's enclosing rectangle is bounded like a list. + if r.count() > MAX_COMPARED_CELLS { + return Err(bad("data.removeDuplicates", "the range is too large")); + } let start = r.start.row + header as u32; let mut removed = 0; edit(s, |cx| { diff --git a/crates/engine/src/cmd/data/validation.rs b/crates/engine/src/cmd/data/validation.rs index e02ef06b3..0d86fbc36 100644 --- a/crates/engine/src/cmd/data/validation.rs +++ b/crates/engine/src/cmd/data/validation.rs @@ -337,11 +337,17 @@ pub(super) fn circle_invalid(s: &mut Session, _: &Json) -> Result { let sheet = d.wb.active_sheet(); let Some(sh) = d.wb.sheet(sheet) else { return Err(EngineError::NoDocument) }; let mut bad_cells = Vec::new(); + // One scan allowance for the whole command: duplicate or overlapping ranges don't multiply it. + let mut budget = 100_000usize; + // Cells were left unchecked because the allowance ran out: said in the result. + let mut truncated = false; for dv in &sh.validations { for r in &dv.ranges { let r = sh.used_range().and_then(|u| r.intersection(&u)); let Some(r) = r else { continue }; - for c in r.iter().take(100_000) { + truncated |= r.count() > budget as u64; + for c in r.iter().take(budget) { + budget -= 1; let text = sh.input_text(c); if !text.is_empty() && check_validation(&d.wb, sheet, c, &text).is_some() { bad_cells.push(c.a1()); @@ -349,5 +355,5 @@ pub(super) fn circle_invalid(s: &mut Session, _: &Json) -> Result { } } } - Ok(json!({"invalid": bad_cells})) + Ok(json!({"invalid": bad_cells, "truncated": truncated})) } diff --git a/crates/engine/src/cmd/edit/find.rs b/crates/engine/src/cmd/edit/find.rs index 3efcdaaf4..94ab15041 100644 --- a/crates/engine/src/cmd/edit/find.rs +++ b/crates/engine/src/cmd/edit/find.rs @@ -199,7 +199,11 @@ pub(super) fn replace(s: &mut Session, p: &Json) -> Result { let mut total = 0usize; for si in sheets { let cells: Vec<(CellRef, String)> = - cx.wb.sheet(si).map(|sh| sh.cells.iter().map(|(c, _)| (c, sh.input_text(c))).collect()).unwrap_or_default(); + // A hidden formula's source is not searched: a count or error would reveal its text. + cx.wb + .sheet(si) + .map(|sh| sh.cells.iter().filter(|(c, cell)| cell.formula.is_none() && cell.py.is_none() || !crate::display::formula_hidden(&cx.wb, sh, *c)).map(|(c, _)| (c, sh.input_text(c))).collect()) + .unwrap_or_default(); for (c, text) in cells { if (!all && c != active) || selection.as_ref().is_some_and(|sel| !sel.contains(c)) { continue; diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index aa2638801..90213a5e1 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -891,6 +891,21 @@ mod tests { assert!((0.75..=1.15).contains(&ratio), "Arabic {} vs Latin {}", width(0), width(1)); } + #[test] + fn autofit_of_many_separate_columns_fits_only_the_selected_ones() { + let mut s = crate::Session::new(); + s.new_workbook(); + let long = "a much longer text than the others"; + // Columns A, C, E, G, I, K are selected (six runs), the ones between hold long text too. + let row: Vec<_> = (0..11).map(|c| if c % 2 == 0 { json!("x") } else { json!(long) }).collect(); + s.execute("range.setValues", json!({"range": "A1", "values": [row]})).unwrap(); + s.execute("home.autofitColumnWidth", json!({"cols": "A:A,C:C,E:E,G:G,I:I,K:K"})).unwrap(); + let doc = s.doc().unwrap(); + let size = |c: u32| doc.wb.active().and_then(|sh| sh.cols.get(&c)).and_then(|e| e.size); + assert!([0, 2, 4, 6, 8, 10].iter().all(|&c| size(c).is_some())); + assert!([1, 3, 5, 7, 9].iter().all(|&c| size(c).is_none())); + } + #[test] fn arabic_word_ligatures_are_wider_than_letters() { let em = |t: &str| approx_text_width(t, 72.0, false) / 96.0; diff --git a/crates/engine/src/cmd/format/sizes.rs b/crates/engine/src/cmd/format/sizes.rs index 8b304c889..5a4ba863a 100644 --- a/crates/engine/src/cmd/format/sizes.rs +++ b/crates/engine/src/cmd/format/sizes.rs @@ -194,16 +194,35 @@ pub(super) fn autofit_cols(s: &mut Session, p: &Json) -> Result { let measure = s.text_width.clone(); edit(s, |cx| { let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; - let mut widths = Vec::new(); let mut measured: std::collections::HashMap> = Default::default(); let mut distinct = 0usize; - let used = sh.used_range(); - for (a, b) in lines { - for c in a..=b.min(a + 16383) { - let Some(u) = used else { continue }; - let col = RangeRef::new(CellRef::new(u.start.row, c), CellRef::new(u.end.row, c)); - let mut max: f32 = 0.0; - for (cell, _) in sh.cells.iter_range(col) { + // Selected columns, then one pass over the populated cells (the store is row-oriented, so a + // scan per column would revisit every row for each selected column, empty or not). + let cols: std::collections::BTreeSet = lines.iter().flat_map(|&(a, b)| a..=b.min(a + 16383)).collect(); + let mut maxes: std::collections::BTreeMap = Default::default(); + if let Some(u) = sh.used_range() { + // One pass per run of adjacent selected columns, so columns between distant selections + // (A and XFD) are never visited. + let mut runs: Vec<(u32, u32)> = Vec::new(); + for &c in &cols { + match runs.last_mut() { + Some((_, end)) if *end + 1 == c => *end = c, + _ => runs.push((c, c)), + } + } + // Every run is a pass over all the stored rows: many runs (alternating columns) are one + // pass over their whole span instead, the unselected columns skipped. + if runs.len() > 4 + && let (Some(&(first, _)), Some(&(_, last))) = (runs.first(), runs.last()) + { + runs = vec![(first, last)]; + } + for (first, last) in runs { + let span = RangeRef::new(CellRef::new(u.start.row, first), CellRef::new(u.end.row, last)); + for (cell, _) in sh.cells.iter_range(span) { + if !cols.contains(&cell.col) { + continue; + } if sh.merge_at(cell).is_some_and(|m| !m.is_single()) { continue; } @@ -226,14 +245,14 @@ pub(super) fn autofit_cols(s: &mut Session, p: &Json) -> Result { } None => approx_text_width(line, st.font.size, st.font.bold), }; - max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell)); + let w = w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell); + let max = maxes.entry(cell.col).or_insert(0.0); + *max = max.max(w); } } - if max > 0.0 { - widths.push((c, max.clamp(8.0, 1785.0))); - } } } + let widths: Vec<(u32, f32)> = maxes.into_iter().filter(|&(_, m)| m > 0.0).map(|(c, m)| (c, m.clamp(8.0, 1785.0))).collect(); let sh = cx.sheet_mut(si)?; for (c, w) in widths { let e = sh.cols.entry(c).or_default(); diff --git a/crates/engine/src/cmd/whatif/advanced_filter.rs b/crates/engine/src/cmd/whatif/advanced_filter.rs index 52630a3ef..21d518a7b 100644 --- a/crates/engine/src/cmd/whatif/advanced_filter.rs +++ b/crates/engine/src/cmd/whatif/advanced_filter.rs @@ -83,6 +83,9 @@ pub(crate) fn criterion_matches(v: &Value, text: &str, crit: &Value) -> bool { } } +/// Most (list row, criteria row) pairs one Advanced Filter tests. +const MAX_FILTER_TESTS: u64 = 50_000_000; + pub(super) fn advanced_filter(s: &mut Session, p: &Json) -> Result { const CMD: &str = "data.advancedFilter"; let d = s.doc()?; @@ -133,6 +136,10 @@ pub(super) fn advanced_filter(s: &mut Session, p: &Json) -> Result { rows.push(conds); } } + // Each list row is tried against every criteria row, so bound their product too. + if u64::from(list.height()) * rows.len() as u64 > MAX_FILTER_TESTS { + return Err(bad(CMD, "the list and criteria ranges together are too large")); + } let matches_row = |row: u32| -> bool { if rows.is_empty() { return true; diff --git a/crates/engine/src/cmd/whatif/tests.rs b/crates/engine/src/cmd/whatif/tests.rs index ec3f5262d..a0a3eda32 100644 --- a/crates/engine/src/cmd/whatif/tests.rs +++ b/crates/engine/src/cmd/whatif/tests.rs @@ -298,6 +298,15 @@ fn consolidate_by_position_and_label() { assert!(s.execute("data.consolidate", json!({"sources": ["A1"], "function": "median"})).is_err()); } +#[test] +fn advanced_filter_refuses_a_list_times_criteria_product_over_the_work_limit() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name"], ["A"]]})).unwrap(); + s.execute("range.setValues", json!({"range": "H1", "values": [["Name"], ["A"]]})).unwrap(); + // A sparse million-row list against 9,999 blank criteria rows would be ~10 billion tests. + assert!(s.execute("data.advancedFilter", json!({"range": "A2:A1048576", "criteria": "H1:H10000"})).is_err()); +} + /// A hundred thousand positional rows take one key lookup each; a tall source and a wide one /// whose keys would meet in a table of 160 million cells are refused before it is built. #[test] diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 96c6f504a..1247b702c 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -1281,9 +1281,18 @@ fn content_differs(b: Option<&Cell>, a: Option<&Cell>) -> bool { /// Objects added, changed and removed between two versions of a sheet. fn objects(b: &Sheet, a: &Sheet) -> (usize, usize, usize) { fn count(b: &[T], a: &[T], id: impl Fn(&T) -> u32) -> (usize, usize, usize) { - let added = a.iter().filter(|x| !b.iter().any(|y| id(y) == id(x))).count(); - let removed = b.iter().filter(|x| !a.iter().any(|y| id(y) == id(x))).count(); - let changed = b.iter().filter(|x| a.iter().any(|y| id(y) == id(x) && y != *x)).count(); + // Objects grouped by id, so each lookup is a map probe instead of a scan of the list. + fn group<'a, T>(v: &'a [T], id: &impl Fn(&T) -> u32) -> std::collections::HashMap> { + let mut m: std::collections::HashMap> = std::collections::HashMap::new(); + for x in v { + m.entry(id(x)).or_default().push(x); + } + m + } + let (gb, ga) = (group(b, &id), group(a, &id)); + let added = a.iter().filter(|x| !gb.contains_key(&id(x))).count(); + let removed = b.iter().filter(|x| !ga.contains_key(&id(x))).count(); + let changed = b.iter().filter(|x| ga.get(&id(x)).is_some_and(|ys| ys.iter().any(|y| *y != *x))).count(); (added, changed, removed) } let parts = [ diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6555a1027..8d6562d5d 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6480,6 +6480,19 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() { assert!(sh.hyperlinks.is_empty() && sh.comments.is_empty()); } +#[test] +fn replace_does_not_search_hidden_formula_source() { + let mut s = s(); + s.execute("cell.set", json!({"cell": "A1", "input": "=SEQUENCE(3)*7"})).unwrap(); + s.execute("home.formatCells", json!({"range": "A1", "style": {"protection": {"locked": true, "hidden": true}}})).unwrap(); + s.execute("review.protectSheet", json!({})).unwrap(); + // Both give the same answer, and it is not an error: a hidden formula's source is not searched. + let hit = s.execute("edit.replace", json!({"what": "SEQUENCE", "with": "X"})).unwrap(); + let miss = s.execute("edit.replace", json!({"what": "NOSUCHTEXT", "with": "X"})).unwrap(); + assert_eq!(hit, miss); + assert_eq!(hit["replaced"], json!(0), "{hit}"); +} + #[test] fn formulatext_does_not_reveal_a_hidden_formula() { let mut s = s(); @@ -6556,6 +6569,19 @@ fn pivot_new_sheet_destination_respects_workbook_structure_protection() { assert!(s.execute("insert.pivotTable", json!({"source": "A1:B3", "location": "New"})).is_err()); } +#[test] +fn circle_invalid_says_when_its_scan_allowance_ended_early() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [[-1]]})).unwrap(); + s.execute("range.setValues", json!({"range": "A200000", "values": [[-1]]})).unwrap(); + s.execute("data.validation", json!({"range": "A1:A5", "type": "whole", "operator": "greater", "formula1": "0"})).unwrap(); + let out = s.execute("data.circleInvalid", json!({})).unwrap(); + assert_eq!((out["invalid"].as_array().map(Vec::len), out["truncated"].as_bool()), (Some(1), Some(false)), "{out}"); + s.execute("data.validation", json!({"range": "A1:A200000", "type": "whole", "operator": "greater", "formula1": "0"})).unwrap(); + let out = s.execute("data.circleInvalid", json!({})).unwrap(); + assert_eq!(out["truncated"].as_bool(), Some(true), "{out}"); +} + #[test] fn history_charges_a_changed_sheets_comment_text() { let mut s = s(); diff --git a/crates/ui-egui/src/app/commands.rs b/crates/ui-egui/src/app/commands.rs index 6dff45cb2..4825318bc 100644 --- a/crates/ui-egui/src/app/commands.rs +++ b/crates/ui-egui/src/app/commands.rs @@ -1,5 +1,5 @@ use super::{ - INTERFACE_SCALE_MAX, INTERFACE_SCALE_MIN, Json, NoticeKind, SheetApp, UiRequest, backstage, clamp_interface_scale, clean_error, dialogs, + INTERFACE_SCALE_MAX, INTERFACE_SCALE_MIN, Json, Notice, NoticeKind, SheetApp, UiRequest, backstage, clamp_interface_scale, clean_error, dialogs, formula_locale, i18n, json, now_ms, printing, }; @@ -96,6 +96,21 @@ impl SheetApp { { self.deliver_pdf(r); } + // The scan stopped at its cell allowance: say that some cells were not checked. + if id == "data.circleInvalid" + && let Ok(r) = &r + && r.get("truncated").and_then(Json::as_bool) == Some(true) + { + let scan = "The scan stopped after 100,000 cell visits; some validated cells were not checked for invalid data."; + // An open-time warning that is still shown keeps its text and action; the scan note joins it. + match self.notice.as_mut() { + Some(n) => { + n.text = format!("{} {scan}", n.text); + n.kind = NoticeKind::Warning; + } + None => self.notice = Some(Notice { text: scan.into(), kind: NoticeKind::Warning, action: None }), + } + } r } diff --git a/crates/ui-egui/tests/it/interaction.rs b/crates/ui-egui/tests/it/interaction.rs index 783c5e34c..16b54eedb 100644 --- a/crates/ui-egui/tests/it/interaction.rs +++ b/crates/ui-egui/tests/it/interaction.rs @@ -1396,6 +1396,28 @@ fn macro_workbook_bytes() -> Vec { gridcraft_engine::io::save_bytes(&wb, "macros.xlsm").unwrap() } +#[test] +fn circle_invalid_data_warns_when_its_scan_was_cut_short() { + let mut h = harness(blank()); + h.state_mut().run("range.setValues", json!({"range": "A1", "values": [[-1]]})).unwrap(); + h.state_mut().run("range.setValues", json!({"range": "A200000", "values": [[-1]]})).unwrap(); + h.state_mut().run("data.validation", json!({"range": "A1:A200000", "type": "whole", "operator": "greater", "formula1": "0"})).unwrap(); + h.state_mut().run("data.circleInvalid", json!({})).unwrap(); + let n = h.state().notice.clone().expect("a notice was shown"); + assert!(n.text.contains("100,000 cell visits"), "{}", n.text); + // An open-time warning keeps its action when the scan note joins it. + h.state_mut().notice = Some(gridcraft_ui_egui::Notice { + text: "Macros.".into(), + kind: gridcraft_ui_egui::NoticeKind::Info, + action: Some(("More".into(), "macros".into())), + }); + h.state_mut().run("data.circleInvalid", json!({})).unwrap(); + let n = h.state().notice.clone().unwrap(); + assert!(n.text.starts_with("Macros. ") && n.text.contains("cell visits"), "{}", n.text); + assert_eq!(n.action.as_ref().map(|a| a.1.as_str()), Some("macros")); + assert_eq!(n.kind, gridcraft_ui_egui::NoticeKind::Warning); +} + #[test] fn opening_a_macro_workbook_shows_a_notice() { // The reader's "macros kept but not executed" warning must reach a dismissible info bar,