From d053b1f54820781ebb5a2bd6f6195c600dd3fed7 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:35:02 -0400 Subject: [PATCH] Engine: a run is checked step by step even when its footprint was complete A footprint is not proof of what the run will do. RAND gives the run other values than the dry run before it (the recalculation's random sequence goes on), and NOW moves on, so an If can take another branch in the run and write over data the dry run never reached; under `fail` the run went ahead and wrote over it. - Unless the rule is `overwrite`, every run checks each step as it goes, as a run after a partial footprint already did; a write the footprint didn't see stops the run, which asks under `ask` and fails otherwise, and rolls back. - Test: `a_run_that_takes_another_path_than_its_check_is_still_checked` (an If on `=RAND()` over 40 runs under `fail`; before, one of them wrote over B1). --- crates/engine/src/macros/run.rs | 40 ++++++++++++++++++------------- crates/engine/src/tests/macros.rs | 25 +++++++++++++++++++ docs/macros.md | 6 +++-- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index f45bcfc25..9719e1e09 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -9,9 +9,9 @@ //! 3. **Footprint**: unless the macro may overwrite, it first runs on the real workbook in dry-run //! mode, the changes are compared with `CellStore::diff`, and everything is put back. Cells it //! would write outside its inputs that already hold data are conflicts, handled by the output -//! rule: ask, fail, overwrite, or shift the data down or right (and check again). A dry run -//! that stops early (a file step, Solver) proves nothing about later steps, so the run then -//! checks each step as it goes. +//! rule: ask, fail, overwrite, or shift the data down or right (and check again). The +//! footprint can miss a write (a dry run stops early at a file step or Solver, and RAND or NOW +//! can send the run another way than the dry run), so the run also checks each step as it goes. //! 4. **Run**: one undo step (`Session::grouped`); an error at any step rolls everything back and //! names the step. //! @@ -430,27 +430,30 @@ pub struct Machine { pub partial: Option<(String, String)>, /// The last step's resolved parameters (for the debugger). pub last_params: Option, - /// Set when the footprint was partial: each step is checked for writes over data as it runs. + /// Set unless the macro may overwrite: each step is checked for writes over data as it runs, + /// since the footprint can miss a write (it stopped early, or the run took another path). guard: Option, } -/// Checks each step of a real run for writes over data the footprint couldn't see. +/// Checks each step of a real run for writes over data the footprint didn't see. #[derive(Clone, Debug)] struct Guard { /// The workbook when the run started, and for each sheet now, its index there (`None` for a /// sheet the run added). start: Arc, origin: Vec>, - /// Where the footprint stopped: "step 2 (file.exportCsv writes a file)". - partial: String, + /// Where the footprint stopped, if it did: "step 2 (file.exportCsv writes a file)". + partial: Option, /// The conflict rule is ask. ask: bool, } impl Guard { - fn new(start: Arc, partial: &Json, ask: bool) -> Guard { - let text = |k: &str| partial.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); - let partial = format!("step {} ({})", text("step"), text("reason")); + fn new(start: Arc, partial: Option<&Json>, ask: bool) -> Guard { + let partial = partial.map(|p| { + let text = |k: &str| p.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); + format!("step {} ({})", text("step"), text("reason")) + }); Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, partial, ask } } @@ -714,7 +717,11 @@ impl Machine { let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { - let msg = format!("It would write over data ({what}); the check before the run stopped at {} and couldn't see this.", g.partial); + let msg = match &g.partial { + Some(at) => format!("It would write over data ({what}); the check before the run stopped at {at} and couldn't see this."), + // A value such as RAND or NOW differed from the check's, and the run went another way. + None => format!("It would write over data ({what}); the check before the run didn't see this write."), + }; return Err(StepError { confirm: g.ask, ..fail(msg) }); } } @@ -1167,8 +1174,9 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result { /// the machine for the run and whether it shifted. The dry runs count towards the run's time /// limit. /// -/// A partial footprint proves nothing about the steps it didn't reach, so then the machine -/// checks each step as it runs and stops the run at one that writes over data. +/// The footprint proves nothing about steps it didn't reach (it stopped early) or a path the run +/// takes and the dry run didn't (RAND, NOW), so the machine also checks each step as it runs and +/// stops the run at one that writes over data. pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { let (m, rule) = (&b.mac, b.rule); let mut machine = Machine::new(&b.prepared, false); @@ -1213,10 +1221,8 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { } } } - if let Some(p) = &fp.partial { - let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); - machine.guard = Some(Guard::new(start, p, rule == OnConflict::Ask)); - } + let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); + machine.guard = Some(Guard::new(start, fp.partial.as_ref(), rule == OnConflict::Ask)); Ok((machine, shifted)) } diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 2f5b71a61..94902ee7b 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -1,5 +1,6 @@ //! Macros: running, input checks, the footprint dry run, the policy and the limits. +use std::collections::BTreeSet; use std::sync::Arc; use serde_json::{Value as Json, json}; @@ -400,6 +401,30 @@ fn each_step_is_checked_for_writes_over_data() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn a_run_that_takes_another_path_than_its_check_is_still_checked() { + // RAND gives the run other values than the dry run before it, so an If can go the other way: + // the write the footprint didn't see must still not go over data. + let mut s = s(); + let steps = json!([ + {"command": "cell.set", "params": {"cell": "A1", "input": "=RAND()"}}, + {"type": "if", "cell": "A1", "op": "gt", "value": 0.5, "then": [{"command": "cell.set", "params": {"cell": "B1", "input": "over"}}]} + ]); + put(&mut s, json!({"name": "Coin", "output": {"onConflict": "fail"}, "steps": steps})); + let mut outcomes = BTreeSet::new(); + for _ in 0..40 { + set(&mut s, "B1", "keep"); + let r = s.execute("macro.run", json!({"name": "Coin"})); + assert_eq!(v(&s, "B1").display(), "keep", "{r:?}"); + outcomes.insert(match r { + Ok(_) => "ran", + Err(e) if e.to_string().contains("would write over") => "refused", + Err(e) => panic!("{e}"), + }); + } + assert_eq!(outcomes.len(), 2, "both paths were taken: {outcomes:?}"); +} + #[test] fn the_answer_to_the_question_counts_however_ask_was_given() { let mut s = s(); diff --git a/docs/macros.md b/docs/macros.md index a56bd77a9..fbda2f69a 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -114,8 +114,10 @@ refused rather than read as the header. delete, are conflicts, handled by `onConflict` (or the macro's `output.onConflict`): `ask` (the call fails with `confirm: …`; `overwrite: true` answers it), `fail`, `overwrite`, `shiftDown` or `shiftRight` (Insert Cells makes room, then the footprint is checked again). Steps that touch - files, and Solver, end the dry run early; the footprint then says it is partial, and unless - the rule is `overwrite`, the run checks every step as it goes. A step that writes over cells, + files, and Solver, end the dry run early; the footprint then says it is partial. Unless the + rule is `overwrite`, the run also checks every step as it goes, since the footprint can miss a + write: it is partial, or a value such as RAND or NOW sends the run another way than the dry + run. A step that writes over cells, sheets or objects that held data when the run started stops the run and rolls the workbook back (a file already written stays): it asks (`confirm: …`) under `ask` and fails otherwise, since shifting can't make room that late.