Objects: a copied sheet's objects get free ids even when a file used the largest id

`Workbook::next_object_id` returned one more than the largest id in use,
saturating at `u32::MAX`. A workbook read from a file can carry an object
with that id; every later insert (a chart, picture, shape, table or
slicer) then got `u32::MAX` again, and so did each object of a copied
sheet, whose ids also came from a saturating counter. Lookups and
selection take the first object with an id, and `object.delete` removes
every one.

`Workbook::free_object_ids(count)` now returns distinct unused ids: those
above the largest in use while they last, then the lowest free ones from
1. `next_object_id` takes the first of them, and the sheet copy takes one
per copied object (and refuses if the id space were ever exhausted). The
model test that recorded the saturation now checks the free ids instead,
and a new one covers a file that used the largest ids: the free ids come
from below, 100,000 of them distinct and in order.

Test: object_ids_stay_unique_when_a_file_used_the_largest_id gives a
shape id u32::MAX, inserts two shapes and copies the sheet; every shape id
is distinct across both sheets. It fails without the fix (both new shapes
get 4294967295).

crates/engine/src/cmd/sheet/tabs.rs

@@ -164,13 +164,15 @@ pub(super) fn move_sheet(s: &mut Session, p: &Json) -> Result<Json> {
             sh.tables.clear(); // table names must be unique
             sh.preserved = None; // kept file content (form controls, OLE objects…) stays with the original
             // Object ids are unique across the workbook (selection and the `{kind, id}` commands
-            // rely on it), so the copied objects get fresh ones.
-            let mut next = cx.wb.next_object_id();
-            let mut fresh = || {
-                let id = next;
-                next = next.saturating_add(1);
-                id
-            };
+            // rely on it), so the copied objects get fresh ones (free ones, even when a file used
+            // the largest id).
+            let count = sh.charts.len() + sh.images.len() + sh.shapes.len() + sh.slicers.len();
+            let ids = cx.wb.free_object_ids(count);
+            if ids.len() < count {
+                return Err(bad("sheet.move", "no free object ids are left for the copied objects"));
+            }
+            let mut ids = ids.into_iter();
+            let mut fresh = || ids.next().unwrap_or(u32::MAX);
             sh.charts.iter_mut().for_each(|o| o.id = fresh());
             sh.images.iter_mut().for_each(|o| o.id = fresh());
             sh.shapes.iter_mut().for_each(|o| o.id = fresh());

crates/engine/src/tests/objects.rs

@@ -281,3 +281,23 @@ fn bubble_sizes_follow_edits_like_the_other_series_references() {
     s.execute("sheet.rename", json!({"sheet": 0, "name": "Data"})).unwrap();
     assert_eq!(sizes(&s), "Data!$F$2:$F$4", "a renamed sheet is renamed in the bubble sizes");
 }
+
+/// A file can carry an object with the largest id. New objects and a copied sheet's objects then
+/// take free ids instead of the largest one again, so `{kind, id}` still names one object.
+#[test]
+fn object_ids_stay_unique_when_a_file_used_the_largest_id() {
+    let mut s = s();
+    let first = id_of(&s.execute("insert.shape", json!({"kind": "rectangle", "at": "B2"})).unwrap(), "shape");
+    // The file's shape carries the largest id (set on the model, as a loaded file would).
+    std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).sheet_mut(0).unwrap().shapes.iter_mut().find(|o| o.id == first).unwrap().id = u32::MAX;
+    let second = id_of(&s.execute("insert.shape", json!({"kind": "ellipse", "at": "D2"})).unwrap(), "shape");
+    let third = id_of(&s.execute("insert.shape", json!({"kind": "triangle", "at": "F2"})).unwrap(), "shape");
+    assert!(second != u32::MAX && third != u32::MAX && second != third, "new shapes take free ids, not the largest again: {second}, {third}");
+    s.execute("sheet.move", json!({"sheet": 0, "to": 1, "copy": true})).unwrap();
+    let wb = &s.doc().unwrap().wb;
+    let mut ids: Vec<u32> = wb.sheets.iter().flat_map(|sh| sh.shapes.iter().map(|o| o.id)).collect();
+    assert_eq!(ids.len(), 6);
+    ids.sort_unstable();
+    ids.dedup();
+    assert_eq!(ids.len(), 6, "every shape id is distinct across both sheets");
+}

crates/model/src/workbook.rs

@@ -463,26 +463,54 @@ impl Workbook {
         }
         None
     }
+    /// A free id for a new chart, picture, shape, table or slicer: one more than the largest in
+    /// use, or, when that is `u32::MAX` (a file can carry any id), the lowest free one.
     pub fn next_object_id(&self) -> u32 {
-        let mut m = 0;
-        for s in &self.sheets {
-            for c in &s.charts {
-                m = m.max(c.id);
-            }
-            for c in &s.images {
-                m = m.max(c.id);
-            }
-            for c in &s.shapes {
-                m = m.max(c.id);
-            }
-            for c in &s.tables {
-                m = m.max(c.id);
-            }
-            for c in &s.slicers {
-                m = m.max(c.id);
-            }
+        self.free_object_ids(1).first().copied().unwrap_or(u32::MAX)
+    }
+
+    /// `count` distinct ids no object in the workbook uses, sorted: the ones above the largest id
+    /// in use while they last, then the lowest free ones. Fewer come back only when the id space
+    /// is exhausted.
+    pub fn free_object_ids(&self, count: usize) -> Vec<u32> {
+        let mut used: Vec<u32> = self
+            .sheets
+            .iter()
+            .flat_map(|s| {
+                s.charts
+                    .iter()
+                    .map(|o| o.id)
+                    .chain(s.images.iter().map(|o| o.id))
+                    .chain(s.shapes.iter().map(|o| o.id))
+                    .chain(s.tables.iter().map(|o| o.id))
+                    .chain(s.slicers.iter().map(|o| o.id))
+            })
+            .collect();
+        used.sort_unstable();
+        used.dedup();
+        let max = used.last().copied().unwrap_or(0);
+        let mut out = Vec::with_capacity(count.min(1 << 16));
+        let mut next = max.checked_add(1);
+        while out.len() < count
+            && let Some(id) = next
+        {
+            out.push(id);
+            next = id.checked_add(1);
         }
-        m.saturating_add(1)
+        // Past the top: the gaps below the largest id, from 1 (0 is never an object's id).
+        let mut taken = used.iter().copied().peekable();
+        let mut id = 1u32;
+        while out.len() < count && id < max {
+            while taken.next_if(|&t| t < id).is_some() {}
+            if taken.peek() == Some(&id) {
+                taken.next();
+            } else {
+                out.push(id);
+            }
+            id += 1;
+        }
+        out.sort_unstable();
+        out
     }
 }
 
@@ -534,7 +562,7 @@ mod tests {
     }
 
     #[test]
-    fn object_ids_saturate_instead_of_overflowing() {
+    fn object_ids_never_overflow_or_repeat() {
         let mut wb = Workbook::new();
         let image = |id| crate::Image { id, anchor: Default::default(), data: Default::default(), mime: String::new(), alt: String::new() };
         assert_eq!(wb.next_object_id(), 1);
@@ -545,6 +573,40 @@ mod tests {
         if let Some(sh) = wb.sheet_mut(0) {
             sh.images.push(image(u32::MAX));
         }
-        assert_eq!(wb.next_object_id(), u32::MAX, "an id read from a file at the maximum saturates");
+        // An id read from a file at the maximum: the lowest free id, not the maximum again.
+        assert_eq!(wb.next_object_id(), 1);
+        if let Some(sh) = wb.sheet_mut(0) {
+            sh.images.push(image(1));
+            sh.images.push(image(3));
+        }
+        assert_eq!(wb.next_object_id(), 2);
+        assert_eq!(wb.free_object_ids(4), vec![2, 4, 5, 6], "the gaps, skipping 1, 3 and 7");
+        let mut wb = Workbook::new();
+        if let Some(sh) = wb.sheet_mut(0) {
+            sh.images.push(image(u32::MAX - 1));
+        }
+        assert_eq!(wb.free_object_ids(3), vec![1, 2, u32::MAX], "the top first, then from 1");
+    }
+
+    /// A file can use the largest id and the one below it: nothing is free above them, so the free
+    /// ids are the lowest unused ones from 1 (0 is never an object's id), and a large request is
+    /// served in time linear in the count and the ids in use.
+    #[test]
+    fn free_object_ids_come_from_below_when_nothing_is_free_above() {
+        let mut wb = Workbook::new();
+        let image = |id| crate::Image { id, anchor: Default::default(), data: Default::default(), mime: String::new(), alt: String::new() };
+        if let Some(sh) = wb.sheet_mut(0) {
+            sh.images.push(image(u32::MAX));
+            sh.images.push(image(u32::MAX - 1));
+            sh.images.push(image(0));
+        }
+        assert_eq!(wb.free_object_ids(3), vec![1, 2, 3]);
+        assert_eq!(wb.next_object_id(), 1);
+        // The 100,000 lowest free ids: distinct, ascending, none of them in use.
+        let many = wb.free_object_ids(100_000);
+        assert_eq!(many.len(), 100_000);
+        assert!(many.windows(2).all(|w| w[0] < w[1]), "ascending without repeats");
+        assert!(!many.contains(&0) && !many.contains(&(u32::MAX - 1)) && !many.contains(&u32::MAX));
+        assert_eq!(wb.free_object_ids(0), Vec::<u32>::new());
     }
 }