Fix id overflow (#1589)

Thanks @Ian-cmd-ops.

crates/engine/src/cmd/browser.rs

@@ -87,7 +87,7 @@ fn group(s: &mut Session, p: &Value) -> Result<Value> {
     let component = component_of(s, p, cmd)?;
     let items = keys(p, "items", cmd)?;
     let d = s.doc_mut();
-    let id = d.browser_groups.iter().map(|g| g.id).max().unwrap_or(0) + 1;
+    let id = super::next_free_id(cmd, d.browser_groups.iter().map(|g| g.id))?;
     let name = match str_(p, "name").map(str::trim).filter(|n| !n.is_empty()) {
         Some(n) if n.len() <= 128 => n.to_string(),
         Some(_) => return Err(bad(cmd, "`name` is too long")),
@@ -1004,6 +1004,31 @@ mod tests {
         assert_eq!(s.visibility, before);
     }
 
+    /// A design file can carry any id, the largest a `u64` holds too: a new group then has no id
+    /// to take. That is an error, not an overflow (a panic where overflow is checked, a duplicate
+    /// id where it is not).
+    #[test]
+    fn a_new_group_after_the_largest_id_is_an_error_not_an_overflow() {
+        let mut s = Session::default();
+        run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10}));
+        let a = names(&s)[0].clone();
+        run(&mut s, "browser.group", json!({"folder": "bodies", "items": [a.clone()]}));
+        s.doc_mut().browser_groups[0].id = u64::MAX;
+        let result = s.execute("browser.group", &json!({"folder": "bodies", "items": [a]}));
+        assert!(!matches!(result, Err(crate::EngineError::Internal(..))), "panicked inside the command: {result:?}");
+        assert!(result.is_err(), "no id is left to give: {result:?}");
+        assert_eq!(s.doc.browser_groups.len(), 1, "nothing was added");
+    }
+
+    #[test]
+    fn the_next_free_id_is_one_past_the_largest_and_never_wraps() {
+        use super::super::next_free_id;
+        assert_eq!(next_free_id("t", std::iter::empty()).unwrap(), 1);
+        assert_eq!(next_free_id("t", [3, 9, 4].into_iter()).unwrap(), 10);
+        assert_eq!(next_free_id("t", [u64::MAX - 1].into_iter()).unwrap(), u64::MAX);
+        assert!(next_free_id("t", [1, u64::MAX].into_iter()).is_err());
+    }
+
     #[test]
     fn groups_hold_items_and_follow_renames() {
         let mut s = Session::default();

crates/engine/src/cmd/joints.rs

@@ -393,8 +393,8 @@ fn limits_param(s: &Session, p: &Value, kind: JointKind, cmd: &str) -> Result<Ve
     Ok(out)
 }
 
-fn next_joint_id(s: &Session) -> u64 {
-    s.doc.assembly.joints.iter().map(|j| j.id).max().unwrap_or(0) + 1
+fn next_joint_id(s: &Session, cmd: &str) -> Result<u64> {
+    super::next_free_id(cmd, s.doc.assembly.joints.iter().map(|j| j.id))
 }
 
 fn add_joint(s: &mut Session, mut j: Joint, p: &Value) -> Result<Value> {
@@ -474,7 +474,7 @@ fn joint(s: &mut Session, p: &Value) -> Result<Value> {
     let kind = kind_param(p, cmd)?;
     let a = origin_param(s, p.get("a"), cmd)?;
     let b = origin_param(s, p.get("b"), cmd)?;
-    let id = next_joint_id(s);
+    let id = next_joint_id(s, cmd)?;
     let name = str_(p, "name").map(str::to_string).unwrap_or_else(|| format!("Joint{id}"));
     let offset = match p.get("offset") {
         Some(v) => value(s, v, false, cmd)?,
@@ -514,7 +514,7 @@ fn as_built(s: &mut Session, p: &Value) -> Result<Value> {
     };
     let fa = s.doc.frame_in_occurrence(oa, &w).ok_or_else(|| bad(cmd, "occurrence transform"))?;
     let fb = s.doc.frame_in_occurrence(ob, &w).ok_or_else(|| bad(cmd, "occurrence transform"))?;
-    let id = next_joint_id(s);
+    let id = next_joint_id(s, cmd)?;
     let name = str_(p, "name").map(str::to_string).unwrap_or_else(|| format!("Joint{id}"));
     let n = kind.dofs().len();
     let j = Joint {

crates/engine/src/cmd/mod.rs

@@ -287,6 +287,13 @@ pub fn find_command(id: &str) -> Option<&'static CommandSpec> {
     all.iter().find(|c| c.id == id).or_else(|| all.iter().find(|c| c.id.eq_ignore_ascii_case(id))).copied()
 }
 
+/// The id for a new item of a list: one past the largest id in it. Ids come from the design file,
+/// so the largest may be the largest a `u64` holds, and one past that is an error, not an overflow
+/// (a panic where overflow is checked, an id that is already taken where it is not).
+pub(crate) fn next_free_id(cmd: &str, ids: impl Iterator<Item = u64>) -> Result<u64> {
+    ids.max().unwrap_or(0).checked_add(1).ok_or_else(|| crate::params::bad(cmd, "this design has no ids left to give"))
+}
+
 /// The end of an unknown command's error: up to three ids like `id`, nearest first. Ids a few
 /// letters away (`solid.extrud` → `solid.extrude`) come before ids that end in the same word
 /// (`file.open` → `doc.open`). Empty when nothing is close.

crates/engine/src/cmd/motion.rs

@@ -95,10 +95,10 @@ pub static COMMANDS: &[CommandSpec] = &[
 
 const MAX_ITEMS: usize = 10_000;
 
-fn next_id(s: &Session) -> u64 {
+fn next_id(s: &Session, cmd: &str) -> Result<u64> {
     let a = &s.doc.assembly;
     let ids = a.contact_sets.iter().map(|c| c.id).chain(a.studies.iter().map(|m| m.id)).chain(a.exploded.iter().map(|e| e.id));
-    ids.max().unwrap_or(0) + 1
+    super::next_free_id(cmd, ids)
 }
 
 fn occurrences(s: &Session, p: &Value, cmd: &str) -> Result<Vec<u64>> {
@@ -424,7 +424,7 @@ fn new_set(s: &mut Session, p: &Value) -> Result<Value> {
     if s.doc.assembly.contact_sets.len() >= MAX_ITEMS {
         return Err(bad(cmd, "too many contact sets"));
     }
-    let id = next_id(s);
+    let id = next_id(s, cmd)?;
     let name = str_(p, "name").map(str::to_string).unwrap_or_else(|| format!("Contact Set{}", s.doc.assembly.contact_sets.len() + 1));
     let a = &mut s.doc_mut().assembly;
     a.contact_sets.push(ContactSet { id, name: name.clone(), occurrences: occ, suppressed: false });
@@ -532,7 +532,7 @@ fn study(s: &mut Session, p: &Value) -> Result<Value> {
             keys.push(StudyKey { joint, index, points });
         }
     }
-    let id = next_id(s);
+    let id = next_id(s, cmd)?;
     let count = s.doc.assembly.studies.len();
     let a = &mut s.doc_mut().assembly;
     let out = match existing.and_then(|i| a.studies.get_mut(i)) {
@@ -952,7 +952,7 @@ fn explode_create(s: &mut Session, p: &Value) -> Result<Value> {
     if s.doc.assembly.exploded.len() >= MAX_ITEMS {
         return Err(bad(cmd, "too many exploded views"));
     }
-    let id = next_id(s);
+    let id = next_id(s, cmd)?;
     let name = str_(p, "name").map(str::to_string).unwrap_or_else(|| format!("Exploded View{}", s.doc.assembly.exploded.len() + 1));
     s.doc_mut().assembly.exploded.push(ExplodedView { id, name: name.clone(), moves: moves.clone() });
     Ok(json!({"view": id, "name": name, "moves": moves.iter().map(|(o, t)| json!({"occurrence": o, "translate": t})).collect::<Vec<_>>()}))

crates/engine/src/cmd/sketch_canvas.rs

@@ -56,7 +56,7 @@ fn make(s: &mut Session, p: &Value, cmd: &str, decal: bool) -> Result<Value> {
     // It belongs to the active component (its plane is in that frame) and moves with it.
     let component = s.active_component;
     let doc = s.doc_mut();
-    let id = doc.canvases.iter().map(|c| c.id).max().unwrap_or(0) + 1;
+    let id = super::next_free_id(cmd, doc.canvases.iter().map(|c| c.id))?;
     let base = if decal { "Decal" } else { "Canvas" };
     let name = str_(p, "name").map(str::to_string).unwrap_or_else(|| format!("{base}{id}"));
     doc.canvases.push(Canvas {