Engine: macro runs refuse a shift that moves an input, stay clean when nothing changed, take the answer however ask was given, and share one step budget

Four fixes from a review of the runner, each with a regression test that
fails without it:

- A shift inserts cells over the block of every written cell, which moves
  that block's columns (or rows) from there on. When a bound input lay in
  the way, its data moved and sheared while the run kept the old coordinates
  and reported success. The shift is now refused, naming the input and its
  place. The output is not counted: it is where the macro writes, and the
  second footprint catches data a shift moved into it.
- A dry run or a refused run left a clean workbook dirty: view.zoom,
  sheet.activate and the view modes move the saved mark to the current
  workbook on a clean document, and the rollback put the workbook back but
  not the mark. Saved keeps and restores it.
- overwrite: true answers the question however ask was given; with an
  explicit onConflict: ask the answer was ignored and the question came back
  forever.
- The dry runs and the run share one step budget as they share one clock;
  each phase counted its own 100,000 steps before. ran reports the run's
  own steps.

crates/engine/src/macros/run.rs

@@ -769,6 +769,9 @@ fn compare(v: &Value, op: CompareOp, want: &Json) -> bool {
 #[derive(Clone, Debug, Default, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct Footprint {
+    /// Steps executed by the end of the dry run that made this footprint (the run's one budget).
+    #[serde(skip)]
+    pub executed: u64,
     pub sheets: Vec<SheetFootprint>,
     pub sheets_added: Vec<String>,
     pub sheets_removed: Vec<String>,
@@ -839,6 +842,9 @@ impl Footprint {
 struct Saved {
     active: usize,
     docs: Vec<u64>,
+    /// The workbook as last saved: `sheet.activate` and the view commands move it to the
+    /// current workbook on a clean document, and the rollback puts the workbook back but not this.
+    saved: Option<Arc<Workbook>>,
     sheet_selections: Vec<Selection>,
     unlocked_ranges: Vec<(String, String)>,
     trust: Trust,
@@ -863,6 +869,7 @@ impl Saved {
         Saved {
             active: s.active,
             docs: s.docs.iter().map(|d| d.uid).collect(),
+            saved: d.map(|d| d.saved.clone()),
             sheet_selections: d.map(|d| d.sheet_selections.clone()).unwrap_or_default(),
             unlocked_ranges: d.map(|d| d.unlocked_ranges.clone()).unwrap_or_default(),
             trust: d.map_or(Trust::Local, |d| d.trust),
@@ -885,6 +892,9 @@ impl Saved {
     fn restore(self, s: &mut Session, doc: u64) {
         s.docs.retain(|d| self.docs.contains(&d.uid));
         if let Some(d) = s.docs.iter_mut().find(|d| d.uid == doc) {
+            if let Some(saved) = self.saved {
+                d.saved = saved;
+            }
             d.sheet_selections = self.sheet_selections;
             d.unlocked_ranges = self.unlocked_ranges;
             d.trust = self.trust;
@@ -908,7 +918,7 @@ impl Saved {
 
 /// Runs the macro in dry-run mode on its workbook, puts everything back, and says what it would
 /// change. The dry run stops once `time_limit_ms` has passed on `clock`.
-pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64) -> Result<Footprint> {
+pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64, executed: u64) -> Result<Footprint> {
     let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?;
     s.set_active(i);
     let saved = Saved::take(s, p.doc);
@@ -918,7 +928,7 @@ pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms:
         d.touched = Some(Vec::new());
     }
     let mut m = Machine::new(p, true);
-    (m.clock, m.time_limit_ms) = (clock, time_limit_ms);
+    (m.clock, m.time_limit_ms, m.executed) = (clock, time_limit_ms, executed);
     let outcome = m.run_to_end(s);
     let (after, touched) = match s.docs.iter_mut().find(|d| d.uid == p.doc) {
         Some(d) => (d.wb.clone(), d.touched.take().unwrap_or_default()),
@@ -929,6 +939,7 @@ pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms:
     rolled?;
     let (after, touched) = if matches!(m.stopped, Some((_, true))) { (before.clone(), Vec::new()) } else { (after, touched) };
     let mut fp = diff(&before, &after, &p.inputs, &touched);
+    fp.executed = m.executed;
     fp.error = outcome.err();
     fp.stopped = m.stopped.map(|(msg, _)| msg);
     fp.partial = m.partial.map(|(step, reason)| json!({"step": step, "reason": reason}));
@@ -1069,16 +1080,38 @@ pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec<String> {
 }
 
 /// Inserts blank cells where the footprint lands (moving what is there down or right), so the
-/// macro writes into empty cells.
-fn make_room(s: &mut Session, fp: &Footprint, down: bool) -> Result<()> {
+/// macro writes into empty cells. The block is every written cell's, so a row the macro writes
+/// stays together. A shift that would move one of the macro's inputs is refused: the inputs were
+/// checked and bound where they are, and only the block's columns (or rows) would move.
+fn make_room(s: &mut Session, fp: &Footprint, p: &Prepared, down: bool) -> Result<()> {
     if !fp.sheets_removed.is_empty() || fp.objects_removed > 0 {
         return Err(EngineError::Other("Shifting can't make room: the macro deletes sheets or objects.".into()));
     }
     let wb = s.doc()?.wb.clone();
+    let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None };
+    let mut moves = Vec::new();
     for sh in fp.sheets.iter().filter(|s| s.conflict_count > 0) {
-        let Some(bounds) = sh.bounds_range else { continue };
-        let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None };
-        let range = resolve::a1(Place { sheet: sh.index, range: bounds }, &env);
+        let Some(block) = sh.bounds_range else { continue };
+        // What the insertion moves: the block's columns from its top row down, or its rows from
+        // its left column rightwards.
+        let moved = |r: &RangeRef| {
+            if down {
+                r.start.col <= block.end.col && r.end.col >= block.start.col && r.end.row >= block.start.row
+            } else {
+                r.start.row <= block.end.row && r.end.row >= block.start.row && r.end.col >= block.start.col
+            }
+        };
+        // The output is where the macro writes, so moving what is there is the point; the second
+        // footprint catches data the shift moved into it.
+        if let Some((name, pl)) = p.inputs.iter().map(|(n, pl, _)| (n.as_str(), *pl)).find(|(_, pl)| pl.sheet == sh.index && moved(&pl.range)) {
+            return Err(EngineError::Other(format!(
+                "Shifting the data would move {name} ({}). Run with onConflict overwrite or fail, or make room first.",
+                resolve::a1(pl, &env)
+            )));
+        }
+        moves.push(resolve::a1(Place { sheet: sh.index, range: block }, &env));
+    }
+    for range in moves {
         s.execute("home.insertCells", json!({"range": range, "shift": if down { "down" } else { "right" }}))?;
     }
     Ok(())
@@ -1127,7 +1160,9 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> {
     if rule == OnConflict::Overwrite {
         return Ok((machine, false));
     }
-    let mut fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms)?;
+    // The dry runs and the run share one step budget as they share one clock.
+    let mut fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?;
+    machine.executed = fp.executed;
     if let Some(e) = fp.error.take() {
         return Err(e.into_engine(&m.name));
     }
@@ -1145,8 +1180,9 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> {
                 return Err(EngineError::Other(format!("Macro '{}' would write over cells that hold data ({what}). Nothing was changed.", m.name)));
             }
             OnConflict::ShiftDown | OnConflict::ShiftRight => {
-                make_room(s, &fp, rule == OnConflict::ShiftDown)?;
-                fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms)?;
+                make_room(s, &fp, &b.prepared, rule == OnConflict::ShiftDown)?;
+                fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?;
+                machine.executed = fp.executed;
                 if let Some(e) = fp.error.take() {
                     return Err(e.into_engine(&m.name));
                 }
@@ -1176,11 +1212,13 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result<Json> {
     s.grouped(&label, |s| {
         let (mut machine, shifted) = make_way(s, &b)?;
         let name = &b.mac.name;
+        // `ran` counts the run's own steps; the budget also holds the dry runs'.
+        let before_run = machine.executed;
         machine.run_to_end(s).map_err(|e| e.into_engine(name))?;
         match machine.stopped {
             Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} Nothing was changed."))),
             stopped => {
-                let mut out = json!({"macro": name, "ran": machine.executed, "inputs": b.inputs});
+                let mut out = json!({"macro": name, "ran": machine.executed.saturating_sub(before_run), "inputs": b.inputs});
                 if let Some((msg, _)) = stopped {
                     out["stopped"] = json!(msg);
                 }
@@ -1198,17 +1236,18 @@ fn time_limit(p: &Json) -> f64 {
     p.get("timeLimit").and_then(Json::as_f64).map(|t| (t * 1000.0).clamp(1.0, 600_000.0)).unwrap_or(DEFAULT_TIME_LIMIT_MS)
 }
 
-/// The conflict rule for this run: `onConflict`, or `overwrite: true` (the answer to the
-/// question), else the macro's own.
+/// The conflict rule for this run: `onConflict`, else the macro's own. `overwrite: true` is the
+/// answer to the question, so it turns `ask` into `overwrite` whichever way `ask` was given.
 fn conflict_rule(p: &Json, m: &Macro) -> Result<OnConflict> {
-    if let Some(t) = p.get("onConflict").and_then(Json::as_str) {
-        return OnConflict::parse(t)
-            .ok_or_else(|| EngineError::Other(format!("onConflict must be ask, fail, overwrite, shiftDown or shiftRight, not '{t}'.")));
-    }
-    if p.get("overwrite").and_then(Json::as_bool) == Some(true) {
+    let rule = match p.get("onConflict").and_then(Json::as_str) {
+        Some(t) => OnConflict::parse(t)
+            .ok_or_else(|| EngineError::Other(format!("onConflict must be ask, fail, overwrite, shiftDown or shiftRight, not '{t}'.")))?,
+        None => m.output.on_conflict,
+    };
+    if rule == OnConflict::Ask && p.get("overwrite").and_then(Json::as_bool) == Some(true) {
         return Ok(OnConflict::Overwrite);
     }
-    Ok(m.output.on_conflict)
+    Ok(rule)
 }
 
 /// `macro.check`: what running the macro would do, without changing anything.
@@ -1247,7 +1286,7 @@ pub fn check(s: &mut Session, name: &str, p: &Json) -> Result<Json> {
     if !asks.is_empty() && !approved {
         return Ok(out);
     }
-    let fp = footprint(s, &prepared, Stopwatch::start(), time_limit(p))?;
+    let fp = footprint(s, &prepared, Stopwatch::start(), time_limit(p), 0)?;
     out["ok"] = json!(fp.error.is_none());
     out["conflicts"] = json!(fp.conflict_count());
     if let Some(e) = &fp.error {

crates/engine/src/tests/macros.rs

@@ -232,6 +232,110 @@ fn shifting_makes_room_and_is_checked_again() {
     assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start));
 }
 
+#[test]
+fn a_shift_that_would_move_an_input_is_refused() {
+    // A shift inserts cells over the conflicting block, which moves the block's columns (or rows)
+    // from there on. When one of the macro's own inputs lies in the way, its data would move and
+    // shear while the run keeps the old coordinates, so the shift is refused and nothing changes.
+    let mut s = s();
+    data(&mut s);
+    set(&mut s, "B1", "Old title");
+    put(
+        &mut s,
+        json!({"name": "Title", "inputs": [{"name": "Input", "header": true}], "steps": [
+            {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}}
+        ]}),
+    );
+    let start = s.doc().unwrap().wb.clone();
+    let err = s.execute("macro.run", json!({"name": "Title", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string();
+    assert!(err.contains("would move Input (") && err.contains("B2:C5"), "{err}");
+    assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "nothing changed");
+    assert_eq!((v(&s, "B1").display(), v(&s, "B3").display()), ("Old title".into(), "Ann".into()));
+    // Writing left of the input and shifting right would move it too.
+    put(
+        &mut s,
+        json!({"name": "Left", "inputs": [{"name": "Input", "header": true}], "steps": [
+            {"command": "cell.set", "params": {"cell": "Input.offset(1,-1).cell(1,1)", "input": "x"}}
+        ]}),
+    );
+    set(&mut s, "A3", "in the way");
+    let err = s.execute("macro.run", json!({"name": "Left", "inputs": "B2:C5", "onConflict": "shiftRight"})).unwrap_err().to_string();
+    assert!(err.contains("would move Input"), "{err}");
+    // The output counts as well.
+    put(
+        &mut s,
+        json!({"name": "Out", "inputs": [{"name": "Input", "header": true}], "output": {"anchor": "Input.below(1)"}, "steps": [
+            {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}}
+        ]}),
+    );
+    let err = s.execute("macro.run", json!({"name": "Out", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string();
+    assert!(err.contains("would move Input"), "{err}");
+    // A shift that moves nothing of the macro's own still works, over the block of every written
+    // cell, so a row the macro writes stays together.
+    set(&mut s, "H1", "elsewhere");
+    put(
+        &mut s,
+        json!({"name": "Wide", "inputs": [{"name": "Input", "header": true}], "steps": [
+            {"command": "cell.set", "params": {"cell": "H1", "input": "over"}},
+            {"command": "cell.set", "params": {"cell": "H9", "input": "far"}}
+        ]}),
+    );
+    s.execute("macro.run", json!({"name": "Wide", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap();
+    assert_eq!((v(&s, "H1").display(), v(&s, "H9").display(), v(&s, "H10").display()), ("over".into(), "far".into(), "elsewhere".into()));
+    assert_eq!(v(&s, "B3").display(), "Ann", "the input did not move");
+}
+
+#[test]
+fn a_dry_run_leaves_a_clean_workbook_clean() {
+    // `view.zoom` and `sheet.activate` move the saved mark to the current workbook on a clean
+    // document; the dry run's rollback puts the workbook back, so the mark must come back too.
+    let mut s = s();
+    data(&mut s);
+    let d = s.doc_mut().unwrap();
+    d.saved = d.wb.clone();
+    assert!(!s.doc().unwrap().is_dirty());
+    put(&mut s, json!({"name": "Look", "steps": [{"command": "view.zoom", "params": {"percent": 120}}]}));
+    s.execute("macro.check", json!({"name": "Look"})).unwrap();
+    assert!(!s.doc().unwrap().is_dirty(), "a dry run changed nothing");
+    // A run refused by the conflict rule changed nothing either.
+    put(&mut s, totals());
+    set(&mut s, "E3", "taken");
+    let d = s.doc_mut().unwrap();
+    d.saved = d.wb.clone();
+    assert!(s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "fail"})).is_err());
+    assert!(!s.doc().unwrap().is_dirty());
+}
+
+#[test]
+fn the_answer_to_the_question_counts_however_ask_was_given() {
+    let mut s = s();
+    data(&mut s);
+    put(&mut s, totals());
+    set(&mut s, "E3", "taken");
+    let p = json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "ask"});
+    assert!(matches!(s.execute("macro.run", p.clone()), Err(EngineError::Confirm(_))));
+    let mut answered = p;
+    answered["overwrite"] = json!(true);
+    s.execute("macro.run", answered).unwrap();
+    assert_eq!(v(&s, "E3").display(), "400");
+}
+
+#[test]
+fn the_step_limit_is_one_budget_across_the_dry_run_and_the_run() {
+    // 300 x 300 passes is under the limit for one phase; the dry run and the run together are not.
+    let mut s = s();
+    put(
+        &mut s,
+        json!({"name": "Spin", "steps": [{"type": "repeat", "over": "A1:A300", "body": [
+            {"type": "repeat", "over": "A1:A300", "body": [{"type": "if", "cell": "A1", "op": "empty"}]}
+        ]}]}),
+    );
+    let err = s.execute("macro.run", json!({"name": "Spin"})).unwrap_err().to_string();
+    assert!(err.contains("more than 100000 steps"), "{err}");
+    // With no dry run (overwrite), the same macro fits.
+    s.execute("macro.run", json!({"name": "Spin", "onConflict": "overwrite"})).unwrap();
+}
+
 #[test]
 fn the_footprint_covers_other_sheets_objects_and_edges() {
     let mut s = s();