forked from 1b-agent/opencadstudio-upstream
fix(table): clamp formula range walks to the table
Bug: evaluate_table_formula walked for row in r1.min(r2)..=r1.max(r2) across columns with no bound, and table_cell_reference parses any usize - =SUM(A1:A18446744073709551615) names about 1.8e19 rows. table.cell() returns None outside the table, so results were correct; the loop just never finished. The walk runs during tessellation, i.e. on the render path - the app freezes permanently and cannot even be closed. Reproduce: 1. Insert a table, double-click a cell. 2. Type =SUM(A1:A18446744073709551615) (or =SUM(A1:A1000000000) - a minutes-long freeze) and press Enter. 3. Before: the next repaint hangs forever. 4. After: the value appears immediately. Fix: The walk is clamped to the table's own rows/columns: row_end = r1.max(r2).min(rows - 1) (same for columns). Every skipped cell was a None, so the collected values - and every function's result (SUM/AVERAGE/COUNT/MIN/MAX) - are identical; the loop now runs at most rows x columns. start > end after clamping is an empty range, so out-of-table references simply collect nothing (as before). The regression test also asserts total evaluation time under 5 seconds, turning any future un-clamped loop into a fast failure.
This commit is contained in:
1 parent
2041d5f1ab
commit
9b3842c0f2
1 file changed
+45
-2
+45
-2
@@ -520,8 +520,15 @@ fn evaluate_table_formula(table: &Table, expression: &str) -> Option<String> {
|
|||||||
let (r1, c1) = table_cell_reference(first)?;
|
let (r1, c1) = table_cell_reference(first)?;
|
||||||
let (r2, c2) = table_cell_reference(last)?;
|
let (r2, c2) = table_cell_reference(last)?;
|
||||||
let mut values = Vec::new();
|
let mut values = Vec::new();
|
||||||
for row in r1.min(r2)..=r1.max(r2) {
|
// A range can name any `usize` (`A1:A18446744073709551615`): `cell()`
|
||||||
for column in c1.min(c2)..=c1.max(c2) {
|
// returns None outside the table, so clamping the walk to the table's
|
||||||
|
// own rows/columns collects exactly the same values — every skipped
|
||||||
|
// cell would have been None — while keeping an unbounded walk off the
|
||||||
|
// render path (1.8e19 iterations of `..=`).
|
||||||
|
let row_end = r1.max(r2).min(table.rows.len().saturating_sub(1));
|
||||||
|
let col_end = c1.max(c2).min(table.columns.len().saturating_sub(1));
|
||||||
|
for row in r1.min(r2)..=row_end {
|
||||||
|
for column in c1.min(c2)..=col_end {
|
||||||
if let Some(value) = table
|
if let Some(value) = table
|
||||||
.cell(row, column)
|
.cell(row, column)
|
||||||
.and_then(|cell| cell.text_value().trim().parse::<f64>().ok())
|
.and_then(|cell| cell.text_value().trim().parse::<f64>().ok())
|
||||||
@@ -3432,6 +3439,42 @@ mod tests {
|
|||||||
assert_eq!(evaluate_table_formula(&table, "not a formula"), None);
|
assert_eq!(evaluate_table_formula(&table, "not a formula"), None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A cell range can name any `usize` — `A1:A18446744073709551615`
|
||||||
|
/// parses to row ≈ 1.8e19. `cell()` returns None for out-of-bounds
|
||||||
|
/// indices, but the range loop still *walked* every index on the
|
||||||
|
/// render path. Clamping the walk to the table's own rows/columns
|
||||||
|
/// must collect exactly the same values (each skipped cell would
|
||||||
|
/// have been None) and stay instant; the elapsed guard keeps a
|
||||||
|
/// future un-clamped loop from creeping back in.
|
||||||
|
#[test]
|
||||||
|
fn formula_range_evaluation_is_bounded_by_the_table() {
|
||||||
|
let table = sample_table();
|
||||||
|
let start = std::time::Instant::now();
|
||||||
|
assert_eq!(
|
||||||
|
evaluate_table_formula(&table, "=SUM(A1:A18446744073709551615)"),
|
||||||
|
Some("40".to_string()),
|
||||||
|
"A1:A… is column A only: 10 + 30"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
evaluate_table_formula(&table, "=COUNT(A1:A1000000000)"),
|
||||||
|
Some("2".to_string()),
|
||||||
|
"column A holds two numeric cells"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
evaluate_table_formula(&table, "=SUM(A1:Z999999999)"),
|
||||||
|
Some("100".to_string())
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
evaluate_table_formula(&table, "=AVERAGE(A1:ZZ18446744073709551615)"),
|
||||||
|
Some("25".to_string())
|
||||||
|
);
|
||||||
|
let elapsed = start.elapsed();
|
||||||
|
assert!(
|
||||||
|
elapsed < std::time::Duration::from_secs(5),
|
||||||
|
"formula evaluation took {elapsed:?} — the range walk is unbounded again"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// A column count driven to `MAX_TYPED_MAGNITUDE` (1e15) grows one
|
/// A column count driven to `MAX_TYPED_MAGNITUDE` (1e15) grows one
|
||||||
/// column per loop iteration until the allocator fails: the magnitude
|
/// column per loop iteration until the allocator fails: the magnitude
|
||||||
/// check bounds geometry, not iteration counts. Bound the cell grid
|
/// check bounds geometry, not iteration counts. Bound the cell grid
|
||||||
|
|||||||
Reference in new issue
Block a user