diff --git a/crates/engine/src/cmd/browser.rs b/crates/engine/src/cmd/browser.rs index b8f93bf2..45a666d8 100644 --- a/crates/engine/src/cmd/browser.rs +++ b/crates/engine/src/cmd/browser.rs @@ -87,7 +87,7 @@ fn group(s: &mut Session, p: &Value) -> Result { 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(); diff --git a/crates/engine/src/cmd/joints.rs b/crates/engine/src/cmd/joints.rs index 962af8f8..d0a8f784 100644 --- a/crates/engine/src/cmd/joints.rs +++ b/crates/engine/src/cmd/joints.rs @@ -393,8 +393,8 @@ fn limits_param(s: &Session, p: &Value, kind: JointKind, cmd: &str) -> Result u64 { - s.doc.assembly.joints.iter().map(|j| j.id).max().unwrap_or(0) + 1 +fn next_joint_id(s: &Session, cmd: &str) -> Result { + 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 { @@ -474,7 +474,7 @@ fn joint(s: &mut Session, p: &Value) -> Result { 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 { }; 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 { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 3b40c6f5..e0a4e34e 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/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) -> Result { + 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. diff --git a/crates/engine/src/cmd/motion.rs b/crates/engine/src/cmd/motion.rs index 03f01798..9cb31e41 100644 --- a/crates/engine/src/cmd/motion.rs +++ b/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 { 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> { @@ -424,7 +424,7 @@ fn new_set(s: &mut Session, p: &Value) -> Result { 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 { 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 { 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::>()})) diff --git a/crates/engine/src/cmd/sketch_canvas.rs b/crates/engine/src/cmd/sketch_canvas.rs index 08f16be3..27596c6e 100644 --- a/crates/engine/src/cmd/sketch_canvas.rs +++ b/crates/engine/src/cmd/sketch_canvas.rs @@ -56,7 +56,7 @@ fn make(s: &mut Session, p: &Value, cmd: &str, decal: bool) -> Result { // 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 {