diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 36fd1ac41..4699a5e7c 100644 --- a/crates/engine/src/macros/run.rs +++ b/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, pub sheets_added: Vec, pub sheets_removed: Vec, @@ -839,6 +842,9 @@ impl Footprint { struct Saved { active: usize, docs: Vec, + /// 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>, sheet_selections: Vec, 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 { +pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64, executed: u64) -> Result { 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 { } /// 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 { 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 { - 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 { 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 { diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index a42a60dd7..d1babe4eb 100644 --- a/crates/engine/src/tests/macros.rs +++ b/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();