Engine: a macro in a newer format is kept unread and saved back unchanged

A macro whose format version is above this build's was dropped on open,
so the next save removed it from the file, and the warning said only
that it couldn't be opened. It is now kept as the file held it
(Workbook::macros_unread, shared between snapshots), never read, listed
or run, and written back unchanged by JSON and xlsx saves, numbered
after the readable macros and around visible names of the storage
shape. Kept macros count toward the 1,000-macro limit after the
readable ones; one too large for an xlsx item is left out. The warning
says it is kept and saved back; damaged and over-limit macros keep
their own warning.

crates/engine/src/io.rs

@@ -262,7 +262,7 @@ pub fn save_bytes_as(wb: &Workbook, path: &str, kind: FileKind) -> Result<Vec<u8
         // A workbook read from CSV keeps the separator it was read with (a `;` file stays `;`).
         FileKind::Csv => wb.sheet(sheet).map(|sh| csv(wb, sh, wb.csv_delimiter.unwrap_or(b','))).transpose().map(Option::unwrap_or_default),
         FileKind::Tsv => wb.sheet(sheet).map(|sh| csv(wb, sh, b'\t')).transpose().map(Option::unwrap_or_default),
-        FileKind::Json => serde_json::to_vec_pretty(wb).map_err(|e| EngineError::Other(e.to_string())),
+        FileKind::Json => crate::macros::json_bytes(wb),
         FileKind::Html => to_html(wb, sheet).map(String::into_bytes),
     }
 }

crates/engine/src/macros/mod.rs

@@ -101,7 +101,7 @@ pub fn with_names(wb: &Workbook) -> crate::Result<Option<Workbook>> {
     // 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) {
+    if wb.macros.is_empty() && wb.macros_unread.is_empty() && !wb.names.iter().any(stored) {
         return Ok(None);
     }
     let mut out = wb.clone();
@@ -115,23 +115,63 @@ pub fn with_names(wb: &Workbook) -> crate::Result<Option<Workbook>> {
     // exactly what `set_many` would refuse, and no arithmetic depends on a number from the file.
     let blocked: HashSet<String> = out.names.iter().filter(|n| !n.hidden && n.scope.is_none()).filter_map(|n| stash::item_key(&n.name)).collect();
     let free = (0..=u64::MAX).map(|i| format!("{MACRO_KEY}{i}")).filter(|key| !blocked.contains(key));
-    let items: Vec<(String, String)> = wb.macros.iter().zip(free).map(|(m, key)| (key, macro_json(m).to_string())).collect();
+    // Macros in a newer format follow the readable ones, written back as the file held them.
+    let texts = wb.macros.iter().map(|m| macro_json(m).to_string()).chain(wb.macros_unread.iter().map(Json::to_string));
+    let items: Vec<(String, String)> = texts.zip(free).map(|(text, key)| (key, text)).collect();
     stash::set_many(&mut out, &items)?;
     // An empty list of its own, not a copy of the shared one cleared.
     out.macros = gridcraft_model::Macros::default();
+    out.macros_unread = std::sync::Arc::default();
     Ok(Some(out))
 }
 
-/// After a file is opened: moves macros out of hidden names into `Workbook::macros` and drops
-/// macros that are damaged, over the limits or in a newer format. Returns a warning for what was dropped.
+/// The JSON format's bytes for `wb`: the workbook, with the macros in a newer format put back into
+/// the `macros` list as the file held them.
+pub fn json_bytes(wb: &Workbook) -> crate::Result<Vec<u8>> {
+    let bytes = if wb.macros_unread.is_empty() {
+        serde_json::to_vec_pretty(wb)
+    } else {
+        serde_json::to_value(wb).and_then(|mut v| {
+            if let Some(list) = v.as_object_mut().map(|o| o.entry("macros").or_insert_with(|| Json::Array(vec![]))).and_then(Json::as_array_mut) {
+                list.extend(wb.macros_unread.iter().cloned());
+            }
+            serde_json::to_vec_pretty(&v)
+        })
+    };
+    bytes.map_err(|e| crate::EngineError::Other(e.to_string()))
+}
+
+/// Whether `v` is a macro in a newer format than this build reads.
+fn is_newer(v: &Json) -> bool {
+    v.get("v").and_then(Json::as_u64).is_some_and(|found| found > u64::from(gridcraft_model::MACRO_VERSION))
+}
+
+/// Keeps a macro in a newer format unread, if it is small enough to save back
+/// ([`stash::MAX_ITEM_CHARS`] characters of JSON, the size every xlsx save can write).
+fn keep_unread(unread: &mut Vec<Json>, v: Json, bad: &mut usize) {
+    if v.to_string().chars().count() <= stash::MAX_ITEM_CHARS {
+        unread.push(v);
+    } else {
+        *bad = bad.saturating_add(1);
+    }
+}
+
+/// After a file is opened: moves macros out of hidden names into `Workbook::macros`, keeps
+/// macros in a newer format unread (`Workbook::macros_unread`), and drops macros that are damaged
+/// or over the limits. Returns a warning for what was kept unread and one for what was dropped.
 pub fn load(wb: &mut Workbook) -> Vec<String> {
     let mut warnings = Vec::new();
     let mut bad = 0usize;
+    let mut unread = Vec::new();
     // Entries of the JSON format's field, each read on its own so a damaged one drops alone.
     let read = std::mem::take(&mut wb.macros_read);
     bad = bad.saturating_add(read.len().saturating_sub(gridcraft_model::MAX_MACROS));
-    for v in read.iter().take(gridcraft_model::MAX_MACROS) {
-        match parse_macro(v) {
+    for v in read.into_iter().take(gridcraft_model::MAX_MACROS) {
+        if is_newer(&v) {
+            keep_unread(&mut unread, v, &mut bad);
+            continue;
+        }
+        match parse_macro(&v) {
             Ok(m) => wb.macros.push(m),
             Err(_) => bad += 1,
         }
@@ -155,7 +195,12 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
     let mut item = Workbook::new();
     for ((_, key), names) in stored.into_iter().take(gridcraft_model::MAX_MACROS) {
         item.names = names;
-        match stash::get_json(&item, &key).ok_or_else(|| "unreadable".to_string()).and_then(|v| parse_macro(&v)) {
+        let v = stash::get_json(&item, &key);
+        if let Some(v) = v.as_ref().filter(|v| is_newer(v)) {
+            keep_unread(&mut unread, v.clone(), &mut bad);
+            continue;
+        }
+        match v.ok_or_else(|| "unreadable".to_string()).and_then(|v| parse_macro(&v)) {
             Ok(m) => found.push(m),
             Err(_) => bad += 1,
         }
@@ -183,9 +228,25 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
             Err(_) => bad += 1,
         }
     }
+    // Readable and unread macros together stay within the limit, the readable ones first.
+    let room = gridcraft_model::MAX_MACROS.saturating_sub(wb.macros.len());
+    bad = bad.saturating_add(unread.len().saturating_sub(room));
+    unread.truncate(room);
+    if !unread.is_empty() {
+        let one = unread.len() == 1;
+        warnings.push(format!(
+            "{} macro{} in the workbook {} made by a newer version of GridCraft: this version can't show or run {}, and saving keeps {} unchanged.",
+            unread.len(),
+            if one { "" } else { "s" },
+            if one { "was" } else { "were" },
+            if one { "it" } else { "them" },
+            if one { "it" } else { "them" },
+        ));
+    }
+    wb.macros_unread = std::sync::Arc::new(unread);
     if bad > 0 {
         warnings.push(format!(
-            "{bad} macro{} in the workbook couldn't be opened: {} damaged, made by a newer version of GridCraft, or over the limits ({} macros, {} steps each, Repeat and If nested {} deep).",
+            "{bad} macro{} in the workbook couldn't be opened: {} damaged or over the limits ({} macros, {} steps each, Repeat and If nested {} deep).",
             if bad == 1 { "" } else { "s" },
             if bad == 1 { "it is" } else { "they are" },
             gridcraft_model::MAX_MACROS,

crates/engine/src/macros/tests.rs

@@ -427,10 +427,11 @@ fn a_damaged_json_macro_drops_alone() {
 }
 
 #[test]
-fn a_macro_in_a_newer_format_is_dropped_not_rewritten() {
+fn a_macro_in_a_newer_format_is_kept_unread_and_saved_back_unchanged() {
     // A newer build's macro (`v` 2, with a field this build doesn't know) would lose that field
-    // here and be saved back as if complete, so it is dropped with the usual warning instead.
-    // Version 1 and a missing version are read as before.
+    // if it were read here, so it is never read: it is kept as the file held it, saved back
+    // unchanged, and the newer build gets it back whole. Version 1 and a missing version are
+    // read as before.
     let future = json!({"name": "Future", "v": MACRO_VERSION + 1, "fromALaterBuild": {"x": 1}, "steps": [{"command": "cell.set"}]});
     let current = json!({"name": "Current", "v": MACRO_VERSION, "steps": [{"command": "cell.set"}]});
     let missing = json!({"name": "Missing"});
@@ -443,17 +444,24 @@ fn a_macro_in_a_newer_format_is_dropped_not_rewritten() {
         let names: Vec<&str> = back.macros.iter().map(|m| m.name.as_str()).collect();
         assert_eq!(names, ["Current", "Missing"], "{file}");
         assert!(back.macros.iter().all(|m| m.v == MACRO_VERSION), "{file}");
-        assert!(warnings.len() == 1 && warnings[0].starts_with("1 macro ") && warnings[0].contains("newer version"), "{file}: {warnings:?}");
-        // Saving and opening again leaves no reduced copy of it behind.
+        assert_eq!(*back.macros_unread, std::slice::from_ref(&future), "{file}");
+        assert!(
+            warnings.len() == 1 && warnings[0].starts_with("1 macro ") && warnings[0].contains("newer version") && warnings[0].contains("unchanged"),
+            "{file}: {warnings:?}"
+        );
+        // Saved and opened again, it is still there, whole, and the others are as they were.
         let (again, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(back, file).unwrap()).unwrap();
-        assert!(warnings.is_empty(), "{file}: {warnings:?}");
+        assert_eq!(warnings.len(), 1, "{file}: {warnings:?}");
         assert_eq!(again.macros, back.macros, "{file}");
+        assert_eq!(again.macros_unread, back.macros_unread, "{file}");
     };
     // The JSON format's field.
     let mut v: serde_json::Value = serde_json::from_slice(&crate::io::save_bytes(&Workbook::new(), "book.json").unwrap()).unwrap();
     v["macros"] = json!([future, current, missing]);
     let (back, warnings) = crate::io::open_bytes("book.json", v.to_string().as_bytes()).unwrap();
     check("book.json", &back, &warnings);
+    let saved: serde_json::Value = serde_json::from_slice(&crate::io::save_bytes(&back, "book.json").unwrap()).unwrap();
+    assert!(saved["macros"].as_array().unwrap().contains(&future), "the file holds it as it was");
     // Hidden names in xlsx.
     let mut wb = Workbook::new();
     for (i, m) in [&future, &current, &missing].into_iter().enumerate() {
@@ -465,6 +473,42 @@ fn a_macro_in_a_newer_format_is_dropped_not_rewritten() {
     check("book.xlsx", &back, &warnings);
 }
 
+#[test]
+fn kept_macros_share_the_save_limits_and_the_numbering() {
+    // A newer macro too large for the file is left out (keeping it would make every xlsx save
+    // fail), and readable and unread macros together stay within the macro limit, readable ones
+    // first. Kept ones are numbered after the readable ones, around a person's visible name.
+    let newer = |name: &str| json!({"name": name, "v": MACRO_VERSION + 1});
+    let mut big = newer("Big");
+    big["pad"] = json!(long(crate::cmd::stash::MAX_ITEM_CHARS));
+    let mut file: serde_json::Value = serde_json::from_slice(&crate::io::save_bytes(&Workbook::new(), "book.json").unwrap()).unwrap();
+    let mut list = vec![big, newer("Kept"), json!({"name": "Read"})];
+    file["macros"] = json!(list);
+    let (mut wb, warnings) = crate::io::open_bytes("book.json", file.to_string().as_bytes()).unwrap();
+    assert_eq!(*wb.macros_unread, [newer("Kept")]);
+    assert_eq!(warnings.len(), 2, "one kept, one left out: {warnings:?}");
+    wb.names.push(visible("_gc_macro_1_0"));
+    let (raw, back) = {
+        let bytes = crate::io::save_bytes(&wb, "book.xlsx").unwrap();
+        let raw = names_of(&gridcraft_xlsx::read_xlsx(&bytes).unwrap().0);
+        (raw, crate::io::open_bytes("book.xlsx", &bytes).unwrap().0)
+    };
+    assert_eq!(raw, ["_gc_macro_1_0", "_gc_macro_0_0", "_gc_macro_2_0"]);
+    assert_eq!((back.macros.len(), back.macros_unread.len()), (1, 1));
+    // Past the limit the rest are left out; and when a JSON file's list is full, an unread item
+    // it also holds in hidden names is the one left out: readable macros come first.
+    list = (0..MAX_MACROS).map(|i| json!({"name": format!("M{i}")})).collect();
+    let mut with_item = Workbook::new();
+    with_item.names.push(hidden("_gc_macro_0_0", &newer("InNames").to_string()));
+    let mut file: serde_json::Value = serde_json::from_slice(&crate::io::save_bytes(&with_item, "book.json").unwrap()).unwrap();
+    for extra in [None, Some(newer("Over"))] {
+        file["macros"] = json!(list.iter().cloned().chain(extra).collect::<Vec<_>>());
+        let (wb, warnings) = crate::io::open_bytes("book.json", file.to_string().as_bytes()).unwrap();
+        assert_eq!((wb.macros.len(), wb.macros_unread.len()), (MAX_MACROS, 0));
+        assert!(warnings.len() == 1 && warnings[0].contains("couldn't be opened"), "{warnings:?}");
+    }
+}
+
 #[test]
 fn many_macros_with_one_name_all_survive_save_and_reopen() {
     // MAX_MACROS macros with the same name, at the length limit, so the suffixes run from " (2)"

crates/model/src/workbook.rs

@@ -188,6 +188,11 @@ pub struct Workbook {
     /// Never written.
     #[serde(rename = "macros", default, skip_serializing, deserialize_with = "crate::macros::entries")]
     pub macros_read: Vec<serde_json::Value>,
+    /// Macros in a newer format than this build reads, as the file held them. They are never read,
+    /// listed or run; a save writes them back unchanged, so the newer build gets them back whole.
+    /// Shared between snapshots, as nothing changes them.
+    #[serde(skip)]
+    pub macros_unread: Arc<Vec<serde_json::Value>>,
     /// Unmodelled file content kept for saving (package root and workbook level). Not in JSON.
     #[serde(skip)]
     pub preserved: Option<Arc<crate::preserved::Preserved>>,
@@ -236,6 +241,7 @@ impl Workbook {
             csv_delimiter: None,
             macros: crate::macros::Macros::default(),
             macros_read: vec![],
+            macros_unread: Arc::default(),
             preserved: None,
             source_dir: None,
             source_path: None,

docs/macros.md

@@ -66,10 +66,12 @@ is someone's own name: opening a file leaves it alone, and saving stores the mac
 it does not use.
 
 Each macro carries its format version, `v` (1 in this version; a missing `v` reads as 1). When a
-file is opened, a macro that is damaged, over the limits or in a newer format (a `v` above 1) is
-left out with one warning, and the rest of the workbook and its other macros open. A newer
-macro is never read in part, so a save can't write it back with its newer fields missing; a
-macro that was left out is not in the saved file.
+file is opened, a macro in a newer format (a `v` above 1) is kept unread, with a warning: this
+version can't show or run it, and saving writes it back exactly as the file held it, so the newer
+version gets it back whole. It is never read in part. Kept macros count toward the 1,000-macro
+limit after the readable ones, and one too large for an xlsx file is left out. A macro that is
+damaged or over the limits is left out with one warning, and the rest of the workbook and its
+other macros open; a macro that was left out is not in the saved file.
 
 ## Automate scripts and VBA