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.