Engine: a Python cell that keeps only its value keeps its whole cached result

When a file is opened, a Python cell falls back to keeping only its value in
four cases: its code is missing from the Python part or the part is damaged
(the xlsx reader), its code is too long for a cell, its kept result is
malformed, or its result is past the adoption budget. The first kept the
values the cell spilled as plain values; the others dropped them, so values
the file showed were lost. All of them now follow one rule, through
Sheet::settle_spill: the anchor and every spilled cell keep their cached
values as plain values, with no live spill range; a cell's own content and
format are never overwritten; and the work is bounded by the spilled values
stored, not by the range. The values move from the spill into the cells, so
nothing is copied. The cut-by-budget test is renamed for what it now checks,
the dropped-result test now expects the values kept, and the adoption warning
and the file-format docs say so.

crates/engine/src/io.rs

@@ -136,8 +136,8 @@ pub const MAX_ADOPTED_RESULT_CELLS: u64 = 4_000_000;
 /// Python cells from a file: a cell whose formula is `PY("…",0|1)` (a format that kept it as a
 /// formula) becomes a Python cell, and a Python cell whose spilled values the file cached gets
 /// them back as its result, so it shows them until it runs again. Results are rebuilt for at most
-/// [`MAX_ADOPTED_RESULT_CELLS`] cells in all; the rest keep only their own cached value. Returns
-/// the warnings.
+/// [`MAX_ADOPTED_RESULT_CELLS`] cells in all; the rest keep only their own cached value, and the
+/// values they spilled stay as plain values. Returns the warnings.
 pub fn adopt_python(wb: &mut Workbook) -> Vec<String> {
     let mut budget = MAX_ADOPTED_RESULT_CELLS;
     let mut skipped = 0usize;
@@ -168,11 +168,9 @@ pub fn adopt_python(wb: &mut Workbook) -> Vec<String> {
         for (anchor, range) in spills {
             let Some(left) = budget.checked_sub(range.count()) else {
                 skipped += 1;
-                // The cell keeps only its own value, so its cached spill goes too: the sheet would
-                // otherwise show spilled values that no result holds.
-                if let Some(r) = sh.spill_ranges.remove(&anchor) {
-                    sh.clear_spill(r);
-                }
+                // The cell keeps only its own value; the values it spilled stay as plain values, so
+                // nothing shows as a spill that no result holds, and nothing the file showed is lost.
+                sh.settle_spill(anchor);
                 continue;
             };
             budget = left;
@@ -189,14 +187,15 @@ pub fn adopt_python(wb: &mut Workbook) -> Vec<String> {
         return Vec::new();
     }
     vec![format!(
-        "The file's Python results are larger than {MAX_ADOPTED_RESULT_CELLS} cells in all, so {skipped} Python cell(s) show only their first value until they run again."
+        "The file's Python results are larger than {MAX_ADOPTED_RESULT_CELLS} cells in all, so {skipped} Python cell(s) have no result until they run again; the values they spilled were kept as plain values."
     )]
 }
 
 /// A file is somebody else's work: a Python cell whose code doesn't fit in Excel's
 /// 8,192-character formula keeps only its cached value, and a kept result that is not a
 /// well-formed array of at most [`gridcraft_model::python::MAX_RESULT_CELLS`] cells is dropped
-/// (the cell keeps its value). Returns the warnings.
+/// (the cell keeps its value). Either way the values the cell spilled stay as plain values
+/// ([`gridcraft_model::Sheet::settle_spill`]). Returns the warnings.
 pub fn enforce_python_limits(wb: &mut Workbook) -> Vec<String> {
     let mut long = Vec::new();
     for si in 0..wb.sheets.len() {
@@ -227,12 +226,10 @@ pub fn enforce_python_limits(wb: &mut Workbook) -> Vec<String> {
                 std::sync::Arc::make_mut(py).output = None;
             }
         }
-        // The spill of a dropped result (or of code dropped to its value) goes with it, so the
-        // sheet and a save no longer show the old spilled values.
+        // A dropped result (or code dropped to its value) leaves the values it spilled as plain
+        // values, as the file showed them, with no live spill.
         for c in dropped {
-            if let Some(r) = sh.spill_ranges.remove(&c) {
-                sh.clear_spill(r);
-            }
+            sh.settle_spill(c);
         }
     }
     let Some(first) = long.first() else { return vec![] };

crates/engine/src/tests/python_files.rs

@@ -353,40 +353,95 @@ fn many_cells_naming_one_long_script_take_from_one_budget() {
     assert_eq!(sh.value(CellRef::parse("A20000").unwrap()), Value::Number(1.0), "the others keep their values");
 }
 
+/// Plants a cached spill over `range` (anchored at its start) with `value` at `at`, as a file
+/// read leaves it.
+fn cached_spill(sh: &mut gridcraft_model::Sheet, range: RangeRef, spilled: &[(CellRef, Value)]) {
+    sh.spill_ranges.insert(range.start, range);
+    for (c, v) in spilled {
+        sh.spill.insert(*c, v.clone());
+    }
+}
+
+/// The value stored in the cell itself (not a spilled one), with whether it holds code or a
+/// formula.
+fn stored(sh: &gridcraft_model::Sheet, c: CellRef) -> (Value, bool) {
+    sh.cell(c).map(|x| (x.value.clone(), x.py.is_some() || x.formula.is_some())).unwrap_or((Value::Empty, false))
+}
+
 #[test]
-fn a_result_cut_by_the_budget_takes_its_cached_spill_with_it() {
+fn a_result_cut_by_the_budget_keeps_its_cached_spill_as_values() {
     // Five Python cells each with a cached 1,000 x 1,000 spill: the budget rebuilds four results;
-    // the fifth cell keeps only its own value, and its spilled values no longer show.
+    // the fifth cell keeps only its own value, and the values it spilled stay as plain values.
     let mut wb = Workbook::new();
+    let bold = wb.styles.derive(gridcraft_model::StyleId::DEFAULT, |st| st.font.bold = true);
     let sh = wb.sheet_mut(0).unwrap();
     for i in 0..5u32 {
-        let anchor = CellRef::new(0, i * 1_000);
+        let col = i * 1_000;
+        let anchor = CellRef::new(0, col);
         sh.set_cell(anchor, Cell { py: Some(Arc::new(PyCode::new("x"))), ..Cell::value(Value::Number(1.0)) });
-        sh.spill_ranges.insert(anchor, RangeRef::new(anchor, CellRef::new(999, i * 1_000 + 999)));
-        sh.spill.insert(CellRef::new(1, i * 1_000), Value::Number(7.0));
-        sh.spill.insert(CellRef::new(999, i * 1_000 + 999), Value::Number(8.0));
+        // A cell with its own content, and a formatted empty one, inside the spill.
+        sh.set_cell(CellRef::new(2, col), Cell::value(Value::text("own")));
+        sh.set_cell(CellRef::new(3, col), Cell::styled(Value::Empty, bold));
+        let spilled = [
+            (CellRef::new(1, col), Value::Number(7.0)),
+            (CellRef::new(2, col), Value::Number(9.0)),
+            (CellRef::new(3, col), Value::Number(6.0)),
+            (CellRef::new(999, col + 999), Value::Number(8.0)),
+        ];
+        cached_spill(sh, RangeRef::new(anchor, CellRef::new(999, col + 999)), &spilled);
     }
     let warnings = crate::io::adopt_python(&mut wb);
     assert!(warnings.iter().any(|w| w.contains("Python")), "{warnings:?}");
     let sh = wb.sheet(0).unwrap();
     let mut cut = 0;
     for i in 0..5u32 {
-        let anchor = CellRef::new(0, i * 1_000);
-        let rebuilt = sh.cell(anchor).and_then(|c| c.py.as_ref()).is_some_and(|p| p.output.is_some());
-        let (below, corner) = (sh.value(CellRef::new(1, i * 1_000)), sh.value(CellRef::new(999, i * 1_000 + 999)));
-        if rebuilt {
-            assert_eq!((below, corner), (Value::Number(7.0), Value::Number(8.0)), "a result within the budget keeps its spill");
-            assert!(sh.spill_ranges.contains_key(&anchor));
-        } else {
-            cut += 1;
-            assert_eq!((below, corner), (Value::Empty, Value::Empty), "no stale spilled values beyond the anchor");
-            assert!(!sh.spill_ranges.contains_key(&anchor));
-            assert_eq!(sh.value(anchor), Value::Number(1.0), "the cell keeps its own cached value");
+        let col = i * 1_000;
+        let anchor = CellRef::new(0, col);
+        let py = sh.cell(anchor).and_then(|c| c.py.clone()).expect("the code is kept");
+        let shown =
+            [sh.value(CellRef::new(1, col)), sh.value(CellRef::new(2, col)), sh.value(CellRef::new(3, col)), sh.value(CellRef::new(999, col + 999))];
+        assert_eq!(shown, [Value::Number(7.0), Value::text("own"), Value::Number(6.0), Value::Number(8.0)], "every result shows the same values");
+        if py.output.is_some() {
+            assert!(sh.spill_ranges.contains_key(&anchor), "a result within the budget keeps its spill");
+            continue;
         }
+        cut += 1;
+        assert!(!sh.spill_ranges.contains_key(&anchor), "no live spill");
+        assert!(sh.spill.range(anchor..=CellRef::new(999, col + 999)).all(|(c, _)| c.col < col || c.col > col + 999), "no spilled values left");
+        assert_eq!(stored(sh, anchor), (Value::Number(1.0), true), "the anchor keeps its value and its code");
+        assert_eq!(stored(sh, CellRef::new(1, col)), (Value::Number(7.0), false), "a spilled value is a plain value now");
+        assert_eq!(stored(sh, CellRef::new(999, col + 999)), (Value::Number(8.0), false));
+        assert_eq!(stored(sh, CellRef::new(2, col)), (Value::text("own"), false), "content is never overwritten");
+        assert_eq!(sh.style_id(CellRef::new(3, col)), bold, "nor a format");
     }
     assert_eq!(cut, 1);
 }
 
+#[test]
+fn code_too_long_for_a_cell_keeps_its_whole_cached_result_as_values() {
+    let long = PyCode::new(&"x".repeat(gridcraft_model::python::MAX_FORMULA_CHARS));
+    assert!(!long.fits());
+    let mut wb = Workbook::new();
+    let sh = wb.sheet_mut(0).unwrap();
+    // A1 spills over A1:B2; D1's own value is empty.
+    sh.set_cell(CellRef::new(0, 0), Cell { py: Some(Arc::new(long.clone())), ..Cell::value(Value::Number(1.0)) });
+    sh.set_cell(CellRef::new(1, 1), Cell::value(Value::text("own")));
+    let spilled = [(CellRef::new(0, 1), Value::Number(2.0)), (CellRef::new(1, 0), Value::Number(3.0)), (CellRef::new(1, 1), Value::Number(4.0))];
+    cached_spill(sh, RangeRef::new(CellRef::new(0, 0), CellRef::new(1, 1)), &spilled);
+    sh.set_cell(CellRef::new(0, 3), Cell { py: Some(Arc::new(long)), ..Cell::default() });
+    cached_spill(sh, RangeRef::new(CellRef::new(0, 3), CellRef::new(1, 3)), &[(CellRef::new(1, 3), Value::Number(5.0))]);
+    let warnings = crate::io::enforce_python_limits(&mut wb);
+    assert!(warnings.iter().any(|w| w.contains("A1")), "{warnings:?}");
+    let sh = wb.sheet(0).unwrap();
+    assert!(sh.spill_ranges.is_empty() && sh.spill.is_empty(), "no live spill: {:?}", sh.spill_ranges);
+    assert_eq!(stored(sh, CellRef::new(0, 0)), (Value::Number(1.0), false), "the anchor keeps only its value");
+    assert_eq!(stored(sh, CellRef::new(0, 1)), (Value::Number(2.0), false));
+    assert_eq!(stored(sh, CellRef::new(1, 0)), (Value::Number(3.0), false));
+    assert_eq!(stored(sh, CellRef::new(1, 1)), (Value::text("own"), false), "content is never overwritten");
+    assert_eq!(stored(sh, CellRef::new(0, 3)), (Value::Empty, false), "an empty anchor stays empty, without code");
+    assert_eq!(stored(sh, CellRef::new(1, 3)), (Value::Number(5.0), false));
+}
+
 #[test]
 fn native_json_keeps_python_cells_and_their_results() {
     let (mut s, long) = build();
@@ -513,7 +568,7 @@ fn py_formulas_over_the_size_limit_keep_only_their_value() {
 }
 
 #[test]
-fn a_dropped_python_result_takes_its_spill_with_it() {
+fn a_dropped_python_result_keeps_its_spill_as_values() {
     // A1's code is over the limit and B1's result is malformed; each had spilled down to row 3,
     // and C1's well-formed result keeps its spill.
     let mut wb = Workbook::new();
@@ -536,13 +591,14 @@ fn a_dropped_python_result_takes_its_spill_with_it() {
     let sh = wb.sheet(0).unwrap();
     for (anchor, below) in [("A1", "A2"), ("B1", "B3")] {
         assert!(!sh.spill_ranges.contains_key(&CellRef::parse(anchor).unwrap()), "{anchor}'s spill range stays");
-        assert_eq!(sh.value(CellRef::parse(below).unwrap()), Value::Empty, "{below} still shows {anchor}'s old spill");
+        let cell = sh.cell(CellRef::parse(below).unwrap()).unwrap_or_else(|| panic!("{below} is a plain value"));
+        assert_eq!((&cell.value, cell.py.is_none() && cell.formula.is_none()), (&Value::Number(8.0), true), "{below} keeps {anchor}'s value");
         assert_eq!(sh.value(CellRef::parse(anchor).unwrap()), Value::Number(7.0), "{anchor} keeps its value");
     }
     assert_eq!(sh.value(CellRef::parse("C2").unwrap()), Value::Number(8.0), "a sound result keeps its spill");
-    // A save writes no value for the dropped spills.
+    // A save writes the kept values as plain values, and the sound spill as a spill.
     let xml = sheet_xml(&gridcraft_xlsx::write_xlsx(&wb).unwrap());
-    assert!(!xml.contains("r=\"A2\"") && !xml.contains("r=\"B3\""), "{xml}");
+    assert!(xml.contains("<c r=\"A2\"><v>8</v></c>") && xml.contains("<c r=\"B3\"><v>8</v></c>"), "{xml}");
     assert!(xml.contains("r=\"C2\""), "{xml}");
 }