From 52b00fe8f87737c2b3283700afe57aa026fb383c Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 1 Nov 2026 15:35:54 -0500 Subject: [PATCH] Engine: an edit that would make Python code too long is refused Structural edits, sheet renames and deletes, and a cut to another sheet rewrite the xl() references in Python code, and could make a cell's code longer than the 8,192-character limit that python.set enforces. Opening the saved file then dropped that code to its value. Such an edit is now refused with a message naming the cell, and nothing changes; keeping the old text would have left a reference the edit had moved. --- crates/engine/src/cmd/edit/paste_special.rs | 53 +++++++++++++++----- crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/insert/table.rs | 2 +- crates/engine/src/cmd/sheet.rs | 19 ++++---- crates/engine/src/cmd/sheet/tabs.rs | 4 +- crates/engine/src/tests/python_cells.rs | 54 +++++++++++++++++++++ docs/architecture.md | 7 ++- 7 files changed, 113 insertions(+), 28 deletions(-) diff --git a/crates/engine/src/cmd/edit/paste_special.rs b/crates/engine/src/cmd/edit/paste_special.rs index f5cc2bf5c..ff1adfc29 100644 --- a/crates/engine/src/cmd/edit/paste_special.rs +++ b/crates/engine/src/cmd/edit/paste_special.rs @@ -145,6 +145,7 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { } moved_formulas = taken; } + let dest_name = cx.wb.sheet(dest_sheet).map(|sh| sh.name.clone()).unwrap_or_default(); for (tr, tc, r, c) in tiled_cells(src.height(), src.width(), tiles_r, tiles_c) { let sc = CellRef::new(src.start.row + r, src.start.col + c); let (dr, dc) = if transpose { (c, r) } else { (r, c) }; @@ -189,10 +190,10 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { // A Python cell's code travels with it. Cut to another sheet, it keeps reading the // cells its `xl()` references named on its original sheet, as a moved formula does: // they are qualified with that sheet before the workbook-wide move adjustment below. - let moved_py = |cell: &Cell| -> Option> { - let py = cell.py.clone()?; + let moved_py = |cell: &Cell| -> Result>> { + let Some(py) = cell.py.clone() else { return Ok(None) }; if !cut_move || dest_sheet == clip.sheet { - return Some(py); + return Ok(Some(py)); } let qualified = gridcraft_model::python::rewrite_xl(&py.code, |reference| { let e = gridcraft_formula::parse(reference).ok()?.map(&mut |x| match x { @@ -204,10 +205,14 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { }); Some(gridcraft_formula::print(&e)) }); - Some(match qualified { - Some(code) => std::sync::Arc::new(gridcraft_model::PyCode { code, ..(*py).clone() }), + Ok(Some(match qualified { + Some(code) => { + let moved = gridcraft_model::PyCode { code, ..(*py).clone() }; + check_python_fits(&moved, &dest_name, dest)?; + std::sync::Arc::new(moved) + } None => py, - }) + })) }; if link { let sheet_prefix = if dest_sheet == clip.sheet && same_doc { @@ -226,7 +231,7 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { new_picture = src_picture.clone(); new.formula = shift(&sc_cell); new.value = if new.formula.is_some() { Value::Empty } else { sc_cell.value.clone() }; - new.py = moved_py(&sc_cell); + new.py = moved_py(&sc_cell)?; let st = map_style(sc_cell.style, &mut cx.wb); new.style = if what == What::AllExceptBorders { let borders = cx.wb.styles.get(old.style).borders; @@ -239,7 +244,7 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { new_picture = src_picture.clone(); new.formula = shift(&sc_cell); new.value = if new.formula.is_some() { Value::Empty } else { sc_cell.value.clone() }; - new.py = moved_py(&sc_cell); + new.py = moved_py(&sc_cell)?; if what == What::FormulasAndNumberFormats { let nf = src_wb.styles.get(sc_cell.style).num_fmt.clone(); new.style = cx.wb.styles.derive(old.style, |s| s.num_fmt = nf); @@ -323,7 +328,7 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { } else { gridcraft_formula::adjust::Edit::MoveToSheet { from: src, to_sheet: dest_name, to_row: at.row, to_col: at.col } }; - rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e); + rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e)?; cx.structural = true; } let end = at.offset_clamped((tiles_r * h) as i64 - 1, (tiles_c * w) as i64 - 1); @@ -347,7 +352,7 @@ pub(super) fn paste_special(s: &mut Session, p: &Json) -> Result { } /// Applies a reference adjustment to every cell formula and defined name in the workbook. -pub(crate) fn rewrite_all_formulas(wb: &mut gridcraft_model::Workbook, target: &str, e: &gridcraft_formula::adjust::Edit) { +pub(crate) fn rewrite_all_formulas(wb: &mut gridcraft_model::Workbook, target: &str, e: &gridcraft_formula::adjust::Edit) -> Result<()> { use gridcraft_formula::adjust::Edit; // Rows and columns inserted or deleted on the target sheet move its legacy array ranges too // (a cut moves its arrays itself). @@ -386,14 +391,19 @@ pub(crate) fn rewrite_all_formulas(wb: &mut gridcraft_model::Workbook, target: & n.formula = gridcraft_formula::print(&new); } } - rewrite_python_refs(wb, &|host, expr| gridcraft_formula::adjust::adjust(expr, host, target, e)); + rewrite_python_refs(wb, &|host, expr| gridcraft_formula::adjust::adjust(expr, host, target, e)) } /// Rewrites the `xl("…")` literals in every Python cell with `f`, which maps a reference held on /// sheet `host` to its new form. Every reference-moving edit (insert and delete of lines or cells, /// cut-move, sheet rename and delete) runs it, as it adjusts formulas. A literal is only replaced -/// when the reference it names changed, and text that doesn't parse is kept. -pub(crate) fn rewrite_python_refs(wb: &mut gridcraft_model::Workbook, f: &impl Fn(&str, gridcraft_formula::Expr) -> gridcraft_formula::Expr) { +/// when the reference it names changed, and text that doesn't parse is kept. A rewrite that would +/// make a cell's code longer than one cell holds (a longer sheet name, `Z1` becoming `AA1`) refuses +/// the whole edit, which then changes nothing: see [`check_python_fits`]. +pub(crate) fn rewrite_python_refs( + wb: &mut gridcraft_model::Workbook, + f: &impl Fn(&str, gridcraft_formula::Expr) -> gridcraft_formula::Expr, +) -> Result<()> { for si in 0..wb.sheets.len() { let Some(sh) = wb.sheets.get(si) else { continue }; let host = sh.name.clone(); @@ -407,6 +417,7 @@ pub(crate) fn rewrite_python_refs(wb: &mut gridcraft_model::Workbook, f: &impl F (after != before).then_some(after) }); if let Some(code) = changed { + check_python_fits(&gridcraft_model::PyCode { code: code.clone(), object: py.object, ..Default::default() }, &host, c)?; new_code.push((c, code)); } } @@ -422,4 +433,20 @@ pub(crate) fn rewrite_python_refs(wb: &mut gridcraft_model::Workbook, f: &impl F } } } + Ok(()) +} + +/// Refuses Python code an edit has made longer than one cell holds: the limit `python.set` +/// enforces and opening a file applies (code over it is dropped to its value on open). The edit +/// is refused rather than made, so no code is lost and none keeps a reference the edit moved. +pub(crate) fn check_python_fits(py: &gridcraft_model::PyCode, sheet: &str, at: CellRef) -> Result<()> { + if py.fits() { + return Ok(()); + } + Err(EngineError::Other(format!( + "The edit would make the Python code in {}!{} longer than Excel's {}-character formula limit, so nothing was changed. Shorten the code first.", + gridcraft_formula::quote_sheet(sheet), + at.a1(), + gridcraft_model::python::MAX_FORMULA_CHARS + ))) } diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 1c0619f76..6e2a3895b 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -301,7 +301,7 @@ fn insert_table(s: &mut Session, p: &Json) -> Result { let e = Edit::InsertCells { axis: Axis::Rows, range: RangeRef::new(r.start, CellRef::new(r.start.row, r.end.col)) }; shift_cell_features(sh, &e); let name = sh.name.clone(); - rewrite(cx, &name, &e); + rewrite(cx, &name, &e)?; let sh = cx.sheet_mut(sheet)?; for (i, c) in (r.start.col..=r.end.col).enumerate() { sh.set_value(CellRef::new(r.start.row, c), gridcraft_core::Value::text(format!("Column{}", i + 1))); diff --git a/crates/engine/src/cmd/insert/table.rs b/crates/engine/src/cmd/insert/table.rs index 381a9e64b..4ca624826 100644 --- a/crates/engine/src/cmd/insert/table.rs +++ b/crates/engine/src/cmd/insert/table.rs @@ -60,7 +60,7 @@ pub(super) fn insert_table(s: &mut Session, p: &Json) -> Result { let e = Edit::InsertCells { axis: Axis::Rows, range: RangeRef::new(r.start, CellRef::new(r.start.row, r.end.col)) }; shift_cell_features(sh, &e); let name = sh.name.clone(); - rewrite(cx, &name, &e); + rewrite(cx, &name, &e)?; let sh = cx.sheet_mut(sheet)?; for (i, c) in (r.start.col..=r.end.col).enumerate() { sh.set_value(CellRef::new(r.start.row, c), gridcraft_core::Value::text(format!("Column{}", i + 1))); diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index 32122abd6..e2d4b77d5 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -232,10 +232,10 @@ fn shift_sheet_features(sh: &mut Sheet, axis: Axis, at: u32, count: u32, insert: } /// Rewrites every formula reference (and chart series) for a structural edit on `target`. -pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) { +pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) -> Result<()> { cx.edits.push((target.to_string(), e.clone())); let wb = &mut cx.wb; - super::edit::rewrite_all_formulas(wb, target, e); + super::edit::rewrite_all_formulas(wb, target, e)?; for si in 0..wb.sheets.len() { let host = wb.sheets.get(si).map(|s| s.name.clone()).unwrap_or_default(); let Some(sh) = wb.sheet_mut(si) else { continue }; @@ -270,6 +270,7 @@ pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) { } } } + Ok(()) } /// Inserting or deleting cells moves the copied block away from the stored source range, so, like @@ -327,7 +328,7 @@ fn insert_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { // New lines take the format of the line before (Excel's "Format Same As Above"). inherit_line_format(sh, axis, at, count); } - rewrite(cx, &name, &Edit::Insert { axis, at, count }); + rewrite(cx, &name, &Edit::Insert { axis, at, count })?; cx.structural = true; Ok(Json::Null) })?; @@ -392,7 +393,7 @@ fn delete_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } shift_sheet_features(sh, axis, at, count, false); } - rewrite(cx, &name, &Edit::Delete { axis, at, count }); + rewrite(cx, &name, &Edit::Delete { axis, at, count })?; } cx.structural = true; Ok(Json::Null) @@ -444,7 +445,7 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { sh.cells.shift_cols(before, count as i64); shift_sheet_features(sh, Axis::Cols, before, count, true); } - rewrite(cx, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); + rewrite(cx, &name, &Edit::Insert { axis: Axis::Cols, at: before, count })?; let mut touched = Vec::new(); { @@ -482,14 +483,14 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { for cell in touched { cx.touch(sheet, cell); } - rewrite(cx, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); + rewrite(cx, &name, &Edit::Move { from: source, to_row: 0, to_col: destination })?; { let sh = cx.sheet_mut(sheet)?; sh.cells.shift_cols(delete_at + count, -(count as i64)); shift_sheet_features(sh, Axis::Cols, delete_at, count, false); } - rewrite(cx, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); + rewrite(cx, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count })?; *cx.sel = crate::Selection { active: CellRef::new(active_row, final_start), anchor: CellRef::new(active_row, final_start), @@ -534,7 +535,7 @@ fn insert_cells(s: &mut Session, p: &Json) -> Result { } shift_cell_features(sh, &e); let name = cx.wb.sheet(sheet).map(|s| s.name.clone()).unwrap_or_default(); - rewrite(cx, &name, &e); + rewrite(cx, &name, &e)?; cx.structural = true; Ok(Json::Null) })?; @@ -569,7 +570,7 @@ fn delete_cells(s: &mut Session, p: &Json) -> Result { shift_cell_features(sh, &e); } // References into the deleted cells become #REF!; the rest move. - rewrite(cx, &name, &e); + rewrite(cx, &name, &e)?; cx.structural = true; Ok(Json::Null) })?; diff --git a/crates/engine/src/cmd/sheet/tabs.rs b/crates/engine/src/cmd/sheet/tabs.rs index cb331b1b9..a20db5291 100644 --- a/crates/engine/src/cmd/sheet/tabs.rs +++ b/crates/engine/src/cmd/sheet/tabs.rs @@ -63,7 +63,7 @@ pub(super) fn delete_sheet(s: &mut Session, p: &Json) -> Result { } } } - crate::cmd::edit::rewrite_python_refs(&mut cx.wb, &|_, e| gridcraft_formula::adjust::delete_sheet(e, &name, &order)); + crate::cmd::edit::rewrite_python_refs(&mut cx.wb, &|_, e| gridcraft_formula::adjust::delete_sheet(e, &name, &order))?; cx.wb.names.retain(|n| n.scope != Some(i)); for n in cx.wb.names.iter_mut() { if let Some(sc) = n.scope.as_mut() @@ -141,7 +141,7 @@ pub(super) fn rename_sheet(s: &mut Session, p: &Json) -> Result { n.formula = gridcraft_formula::print(&gridcraft_formula::adjust::rename_sheet(e, &old, &name)); } } - crate::cmd::edit::rewrite_python_refs(&mut cx.wb, &|_, e| gridcraft_formula::adjust::rename_sheet(e, &old, &name)); + crate::cmd::edit::rewrite_python_refs(&mut cx.wb, &|_, e| gridcraft_formula::adjust::rename_sheet(e, &old, &name))?; cx.structural = true; Ok(Json::Null) }) diff --git a/crates/engine/src/tests/python_cells.rs b/crates/engine/src/tests/python_cells.rs index 92efdad29..913956af8 100644 --- a/crates/engine/src/tests/python_cells.rs +++ b/crates/engine/src/tests/python_cells.rs @@ -430,3 +430,57 @@ fn clearing_pasting_over_or_blocking_a_python_spill_recalculates_its_readers() { s.execute("edit.redo", json!({})).unwrap(); assert_eq!(super::v(&s, "B1"), Value::Number(0.0), "redo"); } + +/// `prefix` and a comment padded to the longest code that still fits in one cell. +fn at_the_limit(prefix: &str) -> String { + let with = |n: usize| format!("{prefix}\n#{}", "x".repeat(n)); + let (mut lo, mut hi) = (0, gridcraft_model::python::MAX_FORMULA_CHARS); + while lo < hi { + let mid = (lo + hi).div_ceil(2); + if PyCode::new(&with(mid)).fits() { lo = mid } else { hi = mid - 1 } + } + with(lo) +} + +#[test] +fn an_edit_that_would_make_python_code_too_long_is_refused() { + let mut s = s(); + s.execute("home.insertSheet", json!({})).unwrap(); + s.execute("sheet.activate", json!({"sheet": 0})).unwrap(); + let other = s.doc().unwrap().wb.sheet(1).unwrap().name.clone(); + let refused = |e: crate::EngineError, cell: &str| { + let e = e.to_string(); + assert!(e.contains(&format!("!{cell} longer than Excel's 8192-character")) && e.contains("nothing was changed"), "{e}"); + }; + // A sheet rename that lengthens a reference. + let a1 = at_the_limit(&format!("a = xl(\"{other}!B2\")")); + plant(&mut s, 0, "A1", &a1); + refused(s.execute("sheet.rename", json!({"sheet": 1, "name": "A much longer sheet name"})).unwrap_err(), "A1"); + assert_eq!(code(&s, 0, "A1").unwrap(), a1); + assert_eq!(s.doc().unwrap().wb.sheet(1).unwrap().name, other); + // One of the same length is made. + let same = "Z".repeat(other.chars().count()); + s.execute("sheet.rename", json!({"sheet": 1, "name": same})).unwrap(); + assert_eq!(code(&s, 0, "A1").unwrap(), a1.replace(&other, &same)); + // Code short of the limit still follows each edit. + plant(&mut s, 0, "A7", "d = xl(\"Z1\")"); + s.execute("selection.set", json!({"range": "B10"})).unwrap(); + s.execute("home.insertColumns", json!({"cols": "Y:Y"})).unwrap(); + assert_eq!(code(&s, 0, "A7").unwrap(), "d = xl(\"AA1\")"); + // An inserted column that turns Z1 into AA1. + let a3 = at_the_limit("b = xl(\"Z1\")"); + plant(&mut s, 0, "A3", &a3); + s.execute("cell.set", json!({"cell": "D1", "input": "kept"})).unwrap(); + s.execute("selection.set", json!({"range": "C1"})).unwrap(); + refused(s.execute("home.insertColumns", json!({"cols": "C:C"})).unwrap_err(), "A3"); + assert_eq!(code(&s, 0, "A3").unwrap(), a3); + assert_eq!(s.doc().unwrap().wb.sheet(0).unwrap().value(CellRef::parse("D1").unwrap()), Value::text("kept")); + // A cut to another sheet, which qualifies B9 with the source sheet. + let a5 = at_the_limit("c = xl(\"B9\")"); + plant(&mut s, 0, "A5", &a5); + s.execute("edit.cut", json!({"range": "A5"})).unwrap(); + s.execute("sheet.activate", json!({"sheet": 1})).unwrap(); + refused(s.execute("edit.paste", json!({"at": "C1"})).unwrap_err(), "C1"); + assert_eq!(code(&s, 0, "A5").unwrap(), a5); + assert_eq!(code(&s, 1, "C1"), None); +} diff --git a/docs/architecture.md b/docs/architecture.md index 08dcebff0..c3373edf5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -160,7 +160,10 @@ characters (the function's syntax, which some files hold), the `_xlfn._xlws.` pr code, which structural edits rewrite like formula references. Only those edits rewrite them: inserting or deleting cells, rows and columns, a cut and paste, and -renaming or deleting a sheet. Copying, filling and sorting carry the code unchanged, so an `xl()` +renaming or deleting a sheet. An edit that would make a cell's code longer than the limit (a +longer sheet name, `Z1` becoming `AA1`, a cut to another sheet that qualifies the references) is +refused and changes nothing, since code over the limit would be dropped to its value on the next +open. Copying, filling and sorting carry the code unchanged, so an `xl()` reference acts as an absolute reference. A sort that moves a Python cell to another row leaves its code reading the cells it named before (a formula moved by the same sort has its relative references shifted), and references to the sorted cells from other cells, formulas and Python code alike, keep @@ -200,7 +203,7 @@ rpm, tarball), FreeBSD, and web (WASM; WebGPU or WebGL2). Release workflows in | Date | Change | Summary | |---|---|---| -| 2026-11-01 | minor | Python cells as data: `model::PyCode`, the file forms, calc laying out kept results, the limits on open, and what copying, filling and sorting do to `xl()` references | +| 2026-11-01 | minor | Python cells as data: `model::PyCode`, the file forms, calc laying out kept results, the limits on open, and what copying, filling and sorting do to `xl()` references; edits that would push code past the limit are refused | | 2026-10-31 | minor | Recently used functions (`recent.rs`, `Prefs::recent_functions`) behind `formulas.recentlyUsed` | | 2026-10-31 | minor | Function Library module (`function_library.rs`); accent icons (`icons::paint_accent`, `widgets::small_button_accent`) | | 2026-10-25 | trivial | The hidden names that hold macros are `_gc_macro__` |