Merge #465 (673af7ec, with #462 at 712bb766 and main at 530a8cc8) into #2151

# Conflicts:
#	docs/macros.md

crates/engine/src/macros/mod.rs

@@ -37,11 +37,18 @@ pub fn parse_macro(v: &Json) -> Result<Macro, String> {
     }
     let mut m: Macro = serde_json::from_value(v).map_err(|e| format!("This isn't a valid macro: {e}"))?;
     m.name = m.name.trim().to_string();
-    m.check_limits()?;
-    check_size(&m)?;
+    check(&m)?;
     Ok(m)
 }
 
+/// Every check a macro must pass before it is kept in a workbook: the model's limits
+/// ([`Macro::check_limits`]) and the size an xlsx file can hold. Every path that adds a macro,
+/// from JSON, from a file or built in code, goes through this one check.
+pub fn check(m: &Macro) -> Result<(), String> {
+    m.check_limits()?;
+    check_size(m)
+}
+
 /// Refuses a macro too large to keep in an xlsx file ([`stash::MAX_ITEM_CHARS`] characters of
 /// JSON), so saving a workbook never fails because of one of its macros.
 fn check_size(m: &Macro) -> Result<(), String> {
@@ -55,9 +62,6 @@ fn check_size(m: &Macro) -> Result<(), String> {
 /// before the steps are deserialized.
 fn normalize_steps(steps: &mut Json, level: usize) -> Result<(), String> {
     let Some(list) = steps.as_array_mut() else { return Err("`steps` must be a list.".into()) };
-    if level > gridcraft_model::MAX_MACRO_DEPTH {
-        return Err(format!("Repeat and If steps can nest at most {} deep.", gridcraft_model::MAX_MACRO_DEPTH));
-    }
     for st in list {
         let Some(o) = st.as_object_mut() else { return Err("Every step must be an object.".into()) };
         if !o.contains_key("type")
@@ -66,6 +70,11 @@ fn normalize_steps(steps: &mut Json, level: usize) -> Result<(), String> {
             o.insert("type".into(), json!("command"));
             o.insert("id".into(), id);
         }
+        // A step with a body is a Repeat or an If: one at the top is nested 1 deep.
+        let nested = ["body", "then", "else"].iter().any(|key| o.contains_key(*key));
+        if nested && level > gridcraft_model::MAX_MACRO_DEPTH {
+            return Err(format!("Repeat and If steps can nest at most {} deep.", gridcraft_model::MAX_MACRO_DEPTH));
+        }
         for key in ["body", "then", "else"] {
             if let Some(body) = o.get_mut(key) {
                 normalize_steps(body, level + 1)?;
@@ -89,17 +98,18 @@ pub fn find(wb: &Workbook, name: &str) -> Option<usize> {
 /// A copy of `wb` for xlsx with each macro in hidden names (`_gc_macro_<n>_<k>`) instead of the
 /// field, or `None` when there is nothing to change.
 pub fn with_names(wb: &Workbook) -> crate::Result<Option<Workbook>> {
-    let stale = !stash::keys(wb, MACRO_KEY).is_empty();
-    if wb.macros.is_empty() && !stale {
+    // Only the numbered items are this build's: a hidden `_gc_macro_<x>_<k>` name with some
+    // other key stays, as `load` leaves it.
+    let stored = |n: &DefinedName| macro_item(n).is_some_and(|(_, i)| i.is_some());
+    if wb.macros.is_empty() && !wb.names.iter().any(stored) {
         return Ok(None);
     }
     let mut out = wb.clone();
-    for key in stash::keys(&out, MACRO_KEY) {
-        stash::remove(&mut out, &key);
-    }
-    for (i, m) in wb.macros.iter().enumerate() {
-        stash::set(&mut out, &format!("{MACRO_KEY}{i}"), &macro_json(m).to_string())?;
-    }
+    // One pass drops the items of the last save, and one writes the macros (a file can hold a
+    // thousand macros and many thousands of names).
+    out.names.retain(|n| !stored(n));
+    let items: Vec<(String, String)> = wb.macros.iter().enumerate().map(|(i, m)| (format!("{MACRO_KEY}{i}"), macro_json(m).to_string())).collect();
+    stash::set_many(&mut out, &items)?;
     out.macros.clear();
     Ok(Some(out))
 }
@@ -125,8 +135,9 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
         match macro_item(&n) {
             None => kept.push(n),
             Some((key, Some(i))) => stored.entry((i, key)).or_default().push(n),
-            // A `macro_` item that isn't numbered isn't one this build wrote; it goes unread.
-            Some((_, None)) => {}
+            // A `macro_` item that isn't numbered isn't one this build wrote: it stays as it is,
+            // unread, and a save keeps it.
+            Some((_, None)) => kept.push(n),
         }
     }
     wb.names = kept;
@@ -154,7 +165,7 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
         let checked = m.check_limits().and_then(|()| {
             m.name = taken.unique(m.name.trim());
             // The macro as it is kept, renamed, must still pass, or the next save would lose it.
-            m.check_limits().and_then(|()| check_size(&m))
+            check(&m)
         });
         match checked {
             Ok(()) => {
@@ -186,8 +197,9 @@ fn macro_item(n: &DefinedName) -> Option<(String, Option<u64>)> {
     Some((key, i))
 }
 
-/// `name`, or `name (2)`, `name (3)`… if a macro already has it. A long name is shortened to make
-/// room for the suffix, so the result stays within [`gridcraft_model::MAX_MACRO_NAME`] characters.
+/// `name`, or `name (2)`, `name (3)`… if a macro already has it. A free name comes back as it is;
+/// a renamed one is shortened to make room for the suffix, so it stays within
+/// [`gridcraft_model::MAX_MACRO_NAME`] characters. The result still goes through [`check`].
 pub fn unique_name(wb: &Workbook, name: &str) -> String {
     let mut taken = Taken::default();
     for m in &wb.macros {

crates/engine/src/macros/tests.rs

@@ -55,16 +55,25 @@ fn macros_over_the_limits_are_refused() {
     let flat = |n: usize| json!({"name": "Big", "steps": vec![json!({"command": "cell.set"}); n]});
     assert!(parse_macro(&flat(MAX_MACRO_STEPS)).is_ok());
     assert!(parse_macro(&flat(MAX_MACRO_STEPS + 1)).unwrap_err().contains("10000 steps"));
-    // Nested Repeat steps: 4 levels are fine, 5 are not.
-    let nest = |levels: usize| {
+    // Nested Repeat steps: 4 Repeats one inside the other are fine (the innermost holds a
+    // command, or nothing), a 5th is not.
+    let nest = |repeats: usize| {
         let mut step = json!({"command": "cell.set"});
-        for _ in 1..levels {
+        for _ in 0..repeats {
             step = json!({"type": "repeat", "over": "Input", "body": [step]});
         }
         json!({"name": "Deep", "steps": [step]})
     };
-    assert!(parse_macro(&nest(4)).is_ok());
-    assert!(parse_macro(&nest(5)).unwrap_err().contains("4 deep"));
+    assert!(parse_macro(&nest(MAX_MACRO_DEPTH)).is_ok());
+    assert_eq!(parse_macro(&nest(MAX_MACRO_DEPTH)).unwrap().step_count(), MAX_MACRO_DEPTH + 1);
+    assert!(parse_macro(&nest(MAX_MACRO_DEPTH + 1)).unwrap_err().contains("4 deep"));
+    let mut empty = json!({"type": "repeat", "over": "Input", "body": []});
+    for _ in 1..MAX_MACRO_DEPTH {
+        empty = json!({"type": "if", "cell": "A1", "op": "eq", "value": 1, "then": [empty], "else": []});
+    }
+    let deep = parse_macro(&json!({"name": "Deep", "steps": [empty]})).unwrap();
+    assert_eq!(deep.step_count(), MAX_MACRO_DEPTH);
+    assert!(deep.check_limits().is_ok(), "the model's check agrees with the parser");
     // Far deeper than serde would allow is refused before it is read.
     assert!(parse_macro(&nest(200)).is_err());
     for bad in [
@@ -140,6 +149,90 @@ fn a_visible_name_that_looks_like_macro_storage_is_left_alone() {
     assert_eq!(wb.names, vec![name]);
 }
 
+#[test]
+fn input_names_that_read_as_cell_references_are_refused() {
+    // A recorded step holds plain A1 text; an input called `Q1` would take its place.
+    let with_input = |name: &str| json!({"name": "M", "inputs": [{"name": name}]});
+    for name in ["Q1", "AB12", "Sum1", "a1"] {
+        let err = parse_macro(&with_input(name)).unwrap_err();
+        assert!(err.contains("reads as a cell reference"), "{name}: {err}");
+    }
+    for name in ["Amount", "Q", "Sales_Q1", "Total1a"] {
+        assert!(parse_macro(&with_input(name)).is_ok(), "{name}");
+    }
+    assert!(parse_macro(&with_input("Row")).unwrap_err().contains("token of its own"));
+}
+
+#[test]
+fn a_hidden_macro_name_this_build_did_not_write_is_kept() {
+    // `_gc_macro_index_0` is not a numbered item: it is neither read nor dropped on open, and a
+    // save with macros keeps it beside the items written.
+    let mut wb = Workbook::new();
+    let theirs = hidden("_gc_macro_index_0", "{}");
+    wb.names.push(theirs.clone());
+    wb.macros.push(sample_macro("Totals"));
+    let (raw, _) = gridcraft_xlsx::read_xlsx(&crate::io::save_bytes(&wb, "book.xlsx").unwrap()).unwrap();
+    let names = names_of(&raw);
+    assert!(names.contains(&"_gc_macro_index_0".to_string()) && names.contains(&"_gc_macro_0_0".to_string()), "{names:?}");
+    let (back, warnings) = crate::io::open_bytes("book.xlsx", &crate::io::save_bytes(&wb, "book.xlsx").unwrap()).unwrap();
+    assert!(warnings.is_empty(), "{warnings:?}");
+    assert_eq!(back.macros, wb.macros);
+    assert_eq!(names_of(&back), ["_gc_macro_index_0"]);
+}
+
+#[test]
+fn workbook_snapshots_share_the_macro_list_until_it_changes() {
+    // An undo snapshot or a copy made for a cell edit holds the same list, not a copy of every
+    // step; the undo budget charges a list only once it is a snapshot's own.
+    let mut wb = Workbook::new();
+    wb.macros.push(sample_macro("Totals"));
+    let snapshot = wb.clone();
+    assert!(wb.macros.shares(&snapshot.macros));
+    assert_eq!(crate::history_retained_bytes(&snapshot, &wb), std::mem::size_of::<Workbook>());
+    wb.macros.push(sample_macro("More"));
+    assert!(!wb.macros.shares(&snapshot.macros), "a write copies the list first");
+    assert_eq!(snapshot.macros.len(), 1);
+    assert!(crate::history_retained_bytes(&snapshot, &wb) > std::mem::size_of::<Workbook>());
+    // The copy is per list, not per workbook copy: cloning again shares the new list.
+    let again = wb.clone();
+    assert!(wb.macros.shares(&again.macros));
+    let rebuilt: Macros = wb.macros.iter().cloned().collect();
+    assert_eq!(rebuilt, wb.macros);
+    assert_eq!(rebuilt.into_iter().count(), 2);
+}
+
+#[test]
+fn a_macro_built_in_code_goes_through_the_size_check_too() {
+    // `check` is the one check for every path, so a macro that passes it never makes a save fail.
+    let big = Macro {
+        name: "Big".into(),
+        steps: vec![Step::command("cell.set", json!({"input": "x".repeat(crate::cmd::stash::MAX_ITEM_CHARS)}))],
+        ..Macro::default()
+    };
+    assert!(big.check_limits().is_ok(), "the model's limits say nothing about the size");
+    assert!(check(&big).unwrap_err().contains("too large"));
+    let mut wb = Workbook::new();
+    wb.macros.push(big);
+    assert!(crate::io::save_bytes(&wb, "book.xlsx").is_err(), "a macro that fails `check` fails the save");
+}
+
+#[test]
+fn a_save_refuses_a_clashing_visible_name_before_changing_anything() {
+    let mut wb = Workbook::new();
+    wb.macros.push(sample_macro("A"));
+    wb.macros.push(sample_macro("B"));
+    let own = DefinedName { hidden: false, ..hidden("_GC_MACRO_1_0", "mine") };
+    wb.names.push(own.clone());
+    wb.names.push(hidden("_gc_macro_7_0", "stale"));
+    let err = crate::io::save_bytes(&wb, "book.xlsx").unwrap_err().to_string();
+    assert!(err.contains("_GC_MACRO_1_0"), "{err}");
+    assert_eq!(wb.names.len(), 2, "nothing changed");
+    wb.names.remove(0);
+    let (raw, _) = gridcraft_xlsx::read_xlsx(&crate::io::save_bytes(&wb, "book.xlsx").unwrap()).unwrap();
+    let names = names_of(&raw);
+    assert_eq!(names, ["_gc_macro_0_0", "_gc_macro_1_0"], "the stale item went in the same pass");
+}
+
 #[test]
 fn scripts_and_vba_stay_as_they_are() {
     // Automate scripts keep their own storage, and a VBA project is carried through unchanged.