BLOCK: a keep value other than convert, retain or delete is an error (fixes #935) (#1160)

make_block erased the selected objects for every `keep` value except
"retain" and only converted them for "convert", so a misspelt value such
as "retian" removed the objects from the drawing without a reference to
the new block.

`keep` is now parsed into an enum before anything is touched; a value
that is not documented is an error naming the three accepted values. The
documented values are accepted in any case. BLOCK and -BLOCK share the
check.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

crates/engine/src/cmd/blocks.rs

@@ -108,8 +108,41 @@ fn references_block(d: &cadcraft_doc::Drawing, ents: &[Entity], name: &str) -> b
     false
 }
 
+/// What BLOCK does with the selected objects once the block is defined (the dialog's Convert to
+/// block, Retain and Delete).
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub(crate) enum Keep {
+    /// Replace the objects by a reference to the new block.
+    Convert,
+    /// Leave the objects in the drawing.
+    Retain,
+    /// Erase the objects.
+    Delete,
+}
+
+impl Keep {
+    /// The `keep` parameter's documented values, in any case; `None` for anything else.
+    pub(crate) fn parse(value: &str) -> Option<Keep> {
+        match value.trim().to_ascii_lowercase().as_str() {
+            "convert" => Some(Keep::Convert),
+            "retain" => Some(Keep::Retain),
+            "delete" => Some(Keep::Delete),
+            _ => None,
+        }
+    }
+}
+
+/// The `keep` parameter of `cmd`, `default` when absent. A value that is not documented is an
+/// error (issue #935: a misspelt value used to erase the objects without converting them).
+fn keep_param(cmd: &str, p: &Value, default: Keep) -> Result<Keep> {
+    match str_param(p, "keep") {
+        None => Ok(default),
+        Some(v) => Keep::parse(v).ok_or_else(|| bad(cmd, format!("`keep` must be \"convert\", \"retain\" or \"delete\", not {v:?}"))),
+    }
+}
+
 /// Create a block from entities. Returns the insert handle when converting.
-pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle], keep: &str, description: &str) -> Result<Option<Handle>> {
+pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle], keep: Keep, description: &str) -> Result<Option<Handle>> {
     if !valid_block_name(name) {
         return Err(bad("block", "invalid block name"));
     }
@@ -136,13 +169,13 @@ pub(crate) fn make_block(s: &mut Session, name: &str, base: Vec2, hs: &[Handle],
         b.entities.push(e);
     }
     doc.blocks.insert(name.to_string(), Arc::new(b));
-    if keep != "retain" {
+    if keep != Keep::Retain {
         for h in hs {
             doc.remove_entity(*h);
         }
     }
     s.set_selection(Vec::new());
-    if keep == "convert" {
+    if keep == Keep::Convert {
         let h = insert(s, name, base, 1.0, 0.0, &serde_json::Map::new())?;
         return Ok(Some(h));
     }
@@ -219,8 +252,8 @@ fn run_block(s: &mut Session, p: &Value) -> Result<Value> {
     let name = str_param(p, "name").ok_or_else(|| bad("block", "`name` is required"))?.to_string();
     let base = point_param(p, "base").unwrap_or(Vec2::ZERO);
     let hs = targets(s, p)?;
-    let keep = str_param(p, "keep").unwrap_or("convert").to_string();
-    let r = make_block(s, &name, base, &hs, &keep, str_param(p, "description").unwrap_or(""))?;
+    let keep = keep_param("block", p, Keep::Convert)?;
+    let r = make_block(s, &name, base, &hs, keep, str_param(p, "description").unwrap_or(""))?;
     // An annotative block: its references (the converted selection too) are annotative.
     if bool_or(p, "annotative", false) {
         let d = s.doc_mut()?;
@@ -495,7 +528,7 @@ impl BlockM {
     fn finish(&mut self, s: &mut Session, hs: &[Handle]) -> Result<Step> {
         let (Some(name), Some(base)) = (self.name.clone(), self.base) else { return Ok(Step::Cancel) };
         // A definition that can't be made (e.g. one that would reference itself) ends the command.
-        let keep = if self.dash { "delete" } else { "convert" };
+        let keep = if self.dash { Keep::Delete } else { Keep::Convert };
         if let Err(e) = make_block(s, &name, base, hs, keep, "") {
             s.echo(e.to_string());
             return Ok(Step::Done);

crates/engine/tests/block_keep.rs

@@ -0,0 +1,63 @@
+//! BLOCK's `keep` parameter takes convert, retain or delete, in any case. Anything else is an
+//! error that changes nothing (issue #935: a misspelt value erased the objects without making a
+//! reference to the new block).
+
+use cadcraft_engine::Session;
+use cadcraft_engine::doc::{EntityKind, Handle};
+use serde_json::{Value, json};
+
+fn line(s: &mut Session) -> Value {
+    s.execute("line", &json!({ "points": [[0, 0], [10, 0]] })).unwrap()["handles"][0].clone()
+}
+
+fn references(s: &Session) -> usize {
+    s.doc().unwrap().model.iter().filter(|e| matches!(e.kind, EntityKind::Insert(_))).count()
+}
+
+/// Everything a refused BLOCK must leave alone: the objects, the blocks, the selection and the
+/// undo history.
+fn state(s: &Session) -> (Vec<String>, Vec<String>, Vec<String>, usize) {
+    let d = s.doc().unwrap();
+    let objects = d.model.iter().map(|e| format!("{:?}", e.kind)).collect();
+    let blocks = d.blocks.keys().cloned().collect();
+    let selection = s.selection().iter().map(|h| h.hex()).collect();
+    (objects, blocks, selection, s.state().unwrap().undo.len())
+}
+
+#[test]
+fn a_value_that_is_not_documented_is_an_error_and_changes_nothing() {
+    let mut s = Session::new();
+    let h = line(&mut s);
+    s.set_selection(vec![Handle::parse_hex(h.as_str().unwrap()).unwrap()]);
+    for (cmd, keep) in [("block", "retian"), ("block", ""), ("block", "erase"), ("-block", "convert-all"), ("-block", "keep")] {
+        let before = state(&s);
+        let err = s.execute(cmd, &json!({ "name": "B", "base": [0, 0], "handles": [h], "keep": keep })).unwrap_err();
+        assert!(err.to_string().contains("`keep`"), "{cmd} {keep:?}: {err}");
+        assert_eq!(state(&s), before, "{cmd} {keep:?} changed the drawing");
+        assert!(s.doc().unwrap().block("B").is_none(), "{cmd} {keep:?} defined the block");
+    }
+    assert_eq!(s.doc().unwrap().model.len(), 1, "the line is still there");
+    assert_eq!(references(&s), 0);
+    assert_eq!(s.selection().len(), 1, "the line is still selected");
+}
+
+#[test]
+fn the_documented_values_work_in_any_case() {
+    let mut s = Session::new();
+    let h = line(&mut s);
+    let r = s.execute("block", &json!({ "name": "R", "base": [0, 0], "handles": [h], "keep": "Retain" })).unwrap();
+    assert!(r["insert"].is_null());
+    assert_eq!(s.doc().unwrap().model.len(), 1, "the line stays");
+    assert!(s.doc().unwrap().block("R").is_some());
+
+    let h = line(&mut s);
+    let r = s.execute("block", &json!({ "name": "D", "base": [0, 0], "handles": [h], "keep": "DELETE" })).unwrap();
+    assert!(r["insert"].is_null());
+    assert_eq!(s.doc().unwrap().model.len(), 1, "the second line is erased");
+
+    let h = line(&mut s);
+    let r = s.execute("-block", &json!({ "name": "C", "base": [0, 0], "handles": [h], "keep": " convert " })).unwrap();
+    assert!(r["insert"].is_string());
+    assert_eq!(references(&s), 1);
+    assert_eq!(s.doc().unwrap().model.len(), 2, "the retained line and C's reference");
+}