From 9f4ca54e9ab427d13f3f505455b660a3a9b58422 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Mon, 2 Nov 2026 03:33:45 -0500 Subject: [PATCH] Engine: Python code counts as content where a cell's content is checked A Python cell whose cached value is empty (a file's PY formula with no cached value) was taken for an empty cell by six checks, which use the value and the formula only: - the Analysis ToolPak report and the PivotTable report replaced it without asking for overwrite or replace (CodeRabbit, on the report); - Insert Cells and Insert Table without headers pushed it off the end of the sheet; - the data region (Ctrl+A, AutoSum, sort) ended at it; - pasting it into part of a legacy array was not refused. They now use Cell::has_content, which counts Python code. Test: python_code_with_an_empty_value_counts_as_content; reverting any one of the six changes makes it fail. --- crates/engine/src/cmd.rs | 2 +- crates/engine/src/cmd/analysis.rs | 2 +- crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/sheet.rs | 2 +- crates/engine/src/pivot.rs | 2 +- crates/engine/src/selection.rs | 2 +- crates/engine/src/tests/python_cells.rs | 46 +++++++++++++++++++++++++ 7 files changed, 52 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 00cc008ca..8092968d6 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -492,7 +492,7 @@ fn check_arrays(old: &Workbook, new: &Workbook) -> Result<()> { continue; } for c in sh.cells.diff(&after.cells) { - let filled = after.cell(c).is_some_and(|x| x.formula.is_some() || !x.value.is_empty()); + let filled = after.cell(c).is_some_and(gridcraft_model::Cell::has_content); if !filled || !sh.spill.contains_key(&c) || same_content(sh.cell(c), after.cell(c)) { continue; } diff --git a/crates/engine/src/cmd/analysis.rs b/crates/engine/src/cmd/analysis.rs index 134294542..9cd10e50f 100644 --- a/crates/engine/src/cmd/analysis.rs +++ b/crates/engine/src/cmd/analysis.rs @@ -1788,7 +1788,7 @@ fn write_report(s: &mut Session, p: &Json, tool: &str, g: Grid) -> Result { for row in rect.start.row..=rect.end.row { for (c, cell) in sh.cells.row(row, rect.start.col, rect.end.col) { - if !cell.value.is_empty() || cell.formula.is_some() { + if cell.has_content() { occupied.push(CellRef::new(row, c)); } } diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 04dc1844e..84b262152 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -308,7 +308,7 @@ fn insert_table(s: &mut Session, p: &Json) -> Result { let sh = cx.sheet_mut(sheet)?; // The header row pushes the data down one row: never off the end of the sheet. let last = RangeRef::new(CellRef::new(MAX_ROWS - 1, r.start.col), CellRef::new(MAX_ROWS - 1, r.end.col)); - if sh.cells.iter_range(last).any(|(_, x)| x.formula.is_some() || !x.value.is_empty()) { + if sh.cells.iter_range(last).any(|(_, x)| x.has_content()) { return Err(EngineError::Other("To prevent possible loss of data, cells can't be pushed off the end of the worksheet.".into())); } sh.cells.shift_rows_in_cols(r.start.col, r.end.col, r.start.row, 1); diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index e2d4b77d5..2d4b9259f 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -521,7 +521,7 @@ fn insert_cells(s: &mut Session, p: &Json) -> Result { } else { RangeRef::new(CellRef::new(r.start.row.max(MAX_ROWS.saturating_sub(r.height())), r.start.col), CellRef::new(MAX_ROWS - 1, r.end.col)) }; - if sh.cells.iter_range(doomed).any(|(_, x)| x.formula.is_some() || !x.value.is_empty()) { + if sh.cells.iter_range(doomed).any(|(_, x)| x.has_content()) { return Err(EngineError::Other("To prevent possible loss of data, GridCraft cannot shift nonblank cells off of the worksheet.".into())); } } diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 1e76c1215..bfaccace4 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -228,7 +228,7 @@ pub fn refresh(wb: &mut Workbook, sheet: usize, idx: usize, replace: bool) -> Re } if !replace { for (c, cell) in sh.cells.iter_range(area) { - if (!cell.value.is_empty() || cell.formula.is_some()) && !pt.last_range.is_some_and(|r| r.contains(c)) { + if cell.has_content() && !pt.last_range.is_some_and(|r| r.contains(c)) { return Err(format!( "There's already data in {}!{}. Do you want to replace it? (Run again with replace: true to overwrite.)", quote_sheet(&sh.name), diff --git a/crates/engine/src/selection.rs b/crates/engine/src/selection.rs index 4df6fbe4a..0e00e4869 100644 --- a/crates/engine/src/selection.rs +++ b/crates/engine/src/selection.rs @@ -190,7 +190,7 @@ pub fn jump(sheet: &Sheet, from: CellRef, dr: i64, dc: i64) -> CellRef { /// The contiguous data region around `c` (Ctrl+A / Ctrl+Shift+8, AutoSum, sort, tables). pub fn current_region(sheet: &Sheet, c: CellRef) -> RangeRef { - let filled = |c: CellRef| sheet.cell(c).is_some_and(|x| !x.value.is_empty() || x.formula.is_some()) || sheet.spill.contains_key(&c); + let filled = |c: CellRef| sheet.cell(c).is_some_and(gridcraft_model::Cell::has_content) || sheet.spill.contains_key(&c); // Whether a row (or column) has a filled cell between two columns (rows), inclusive. let row_has = |row: u32, a: u32, b: u32| (a..=b).any(|col| filled(CellRef::new(row, col))); let col_has = |col: u32, a: u32, b: u32| (a..=b).any(|row| filled(CellRef::new(row, col))); diff --git a/crates/engine/src/tests/python_cells.rs b/crates/engine/src/tests/python_cells.rs index 913956af8..63cf1d92b 100644 --- a/crates/engine/src/tests/python_cells.rs +++ b/crates/engine/src/tests/python_cells.rs @@ -484,3 +484,49 @@ fn an_edit_that_would_make_python_code_too_long_is_refused() { assert_eq!(code(&s, 0, "A5").unwrap(), a5); assert_eq!(code(&s, 1, "C1"), None); } + +/// Makes `cell` a Python cell whose cached value is empty, as a file's `PY` formula with no cached +/// value reads. +fn plant_empty(s: &mut Session, cell: &str) { + plant(s, 0, cell, "x = 1"); + let d = s.doc_mut().unwrap(); + let mut wb = (*d.wb).clone(); + if let Some(c) = wb.sheet_mut(0).unwrap().cells.get_mut(CellRef::parse(cell).unwrap()) { + c.value = Value::Empty; + } + d.wb = Arc::new(wb); +} + +#[test] +fn python_code_with_an_empty_value_counts_as_content() { + // A report or a PivotTable asks before replacing it. + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Region", "Sales"], ["East", 1], ["West", 2], ["East", 3]]})).unwrap(); + plant_empty(&mut s, "D2"); + let report = json!({"tool": "Descriptive Statistics", "range": "B1:B4", "labels": true, "output": "D1"}); + assert!(s.execute("data.analysisToolPak", report).is_err(), "the report asks for overwrite"); + plant_empty(&mut s, "H4"); + let pivot = s.execute("insert.pivotTable", json!({"destination": "Sheet1!H3", "rows": ["Region"], "values": ["Sales"]})); + assert!(pivot.is_err_and(|e| e.to_string().contains("already data"))); + assert_eq!((code(&s, 0, "D2"), code(&s, 0, "H4")), (Some("x = 1".into()), Some("x = 1".into())), "the code is kept"); + // Cells aren't pushed off the sheet over it. + plant_empty(&mut s, "K1048576"); + assert!(s.execute("home.insertCells", json!({"range": "K1", "shift": "down"})).is_err()); + assert_eq!(code(&s, 0, "K1048576"), Some("x = 1".into())); + // A table without a header row doesn't push it off the sheet either. + s.execute("range.setValues", json!({"range": "X1", "values": [[1], [2]]})).unwrap(); + plant_empty(&mut s, "X1048576"); + assert!(s.execute("insert.table", json!({"range": "X1:X2", "header": false})).is_err()); + assert_eq!(code(&s, 0, "X1048576"), Some("x = 1".into())); + // It is part of the data region. + s.execute("range.setValues", json!({"range": "P1", "values": [[1], [""], [3]]})).unwrap(); + plant_empty(&mut s, "P2"); + let sh = s.doc().unwrap().wb.sheet(0).unwrap().clone(); + assert_eq!(crate::selection::current_region(&sh, CellRef::parse("P1").unwrap()).a1(), "P1:P3"); + // Pasted into part of a legacy array, it is refused like any content. + s.execute("selection.set", json!({"range": "R1:R3"})).unwrap(); + s.execute("cell.set", json!({"input": "=5", "array": true})).unwrap(); + plant_empty(&mut s, "T1"); + s.execute("edit.copy", json!({"range": "T1"})).unwrap(); + assert!(s.execute("edit.paste", json!({"at": "R2"})).is_err_and(|e| e.to_string().contains("part of an array"))); +}