Prevent deletion from an attached mjSpec.

This change disallows calling `mjs_delete` on an mjSpec that has been attached to another mjSpec via `mjs_attach`. Attempting to delete an element from an attached spec will now result in an error. The Python bindings for `mjSpec.delete` have been updated to raise a ValueError when this occurs.

PiperOrigin-RevId: 897266695
Change-Id: Ic0670125a3028191ec50eca890f02b5910ec8b03

python/mujoco/specs.cc

@@ -450,7 +450,9 @@ PYBIND11_MODULE(_specs, m) {
       },
       py::return_value_policy::reference_internal);
   mjSpec.def("delete", [](MjSpec& self, raw::MjsBody& body) {
-    mjs_delete(self.ptr, body.element);
+    if (mjs_delete(self.ptr, body.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjSpec.def(
       "attach",
@@ -867,7 +869,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSFRAME ====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsFrame& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsFrame.def("set_frame", [](raw::MjsFrame& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -906,7 +910,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSGEOM =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsGeom& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsGeom.def("set_frame", [](raw::MjsGeom& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -937,7 +943,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSJOINT ====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsJoint& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsJoint.def("set_frame", [](raw::MjsJoint& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -968,7 +976,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSSITE =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsSite& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsSite.def("set_frame", [](raw::MjsSite& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -1016,7 +1026,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSCAMERA ===================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsCamera& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsCamera.def("set_frame", [](raw::MjsCamera& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -1047,7 +1059,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSLIGHT ====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsLight& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsLight.def("set_frame", [](raw::MjsLight& self, raw::MjsFrame& frame) {
     if (mjs_setFrame(self.element, &frame) != 0) {
@@ -1078,7 +1092,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSMATERIAL =================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsMaterial& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsMaterial.def_property(
       "classname",
@@ -1092,7 +1108,9 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSMESH =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsMesh& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
   mjsMesh.def_property(
       "classname",
@@ -1452,47 +1470,65 @@ PYBIND11_MODULE(_specs, m) {
 
   // ============================= MJSFLEX =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsFlex& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSHFIELD ===================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsHField& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSSKIN =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsSkin& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSTEXTURE ==================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsTexture& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSKEY ======================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsKey& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSTEXT =====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsText& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSNUMERIC ==================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsNumeric& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSEXCLUDE ==================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsExclude& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSTUPLE ====================================
   mjSpec.def("delete", [](MjSpec& self, raw::MjsTuple& obj) {
-    mjs_delete(self.ptr, obj.element);
+    if (mjs_delete(self.ptr, obj.element) != 0) {
+      throw pybind11::value_error(mjs_getError(self.ptr));
+    }
   });
 
   // ============================= MJSPLUGIN ===================================

python/mujoco/specs_test.py

@@ -979,7 +979,6 @@ class SpecsTest(absltest.TestCase):
     self.assertEqual(mesh.plugin.name, 'inst')
     self.assertEqual(mesh.plugin.plugin_name, 'mujoco.sdf.torus')
 
-
   def test_duplicate_name_error(self):
     main_xml = """
     <mujoco>
@@ -1391,6 +1390,21 @@ class SpecsTest(absltest.TestCase):
     with self.assertRaisesRegex(ValueError, 'Frame not found.'):
       parent.attach(child4, frame='invalid_frame', prefix='child3-')
 
+  def test_delete_from_attached_spec_error(self):
+    parent = mujoco.MjSpec()
+    child = mujoco.MjSpec()
+    body = child.worldbody.add_body(name='child_body')
+    geom = body.add_geom(name='child_geom')
+
+    frame = parent.worldbody.add_frame()
+    parent.attach(child, frame=frame, prefix='child_')
+
+    # Now child spec is attached. Deleting from it should raise ValueError.
+    with self.assertRaisesRegex(
+        ValueError, 'Cannot delete element from an attached mjSpec.'
+    ):
+      child.delete(geom)
+
   def test_attach_valid_child_lists(self):
     xml1 = """
     <mujoco>

src/user/user_api.cc

@@ -474,8 +474,12 @@ int mj_copyBack(mjSpec* s, const mjModel* m) {
 // remove body from mjSpec, return 0 on success
 int mjs_delete(mjSpec* s, mjsElement* element) {
   mjCModel* model = static_cast<mjCModel*>(s->element);
+  if (model->IsAttached()) {
+    model->SetError(mjCError(nullptr, "Cannot delete element from an attached mjSpec."));
+    return -1;
+  }
   if (!element) {
-    model->SetError(mjCError(0, "Element is null."));
+    model->SetError(mjCError(nullptr, "Element is null."));
     return -1;
   }
   try {

src/user/user_model.h

@@ -327,6 +327,9 @@ class mjCModel : public mjCModel_, private mjSpec {
   // set attached flag
   void SetAttached(bool deepcopy) { attached_ |= !deepcopy; }
 
+  // check if model is attached
+  bool IsAttached() const { return attached_; }
+
   // check for repeated names in list
   void CheckRepeat(mjtObj type);
 

test/user/user_api_test.cc

@@ -173,6 +173,36 @@ TEST_F(MujocoTest, TreeTraversal) {
   mj_deleteSpec(spec);
 }
 
+TEST_F(MujocoTest, AttachAndChildDeletion) {
+  mjSpec* child_spec = mj_makeSpec();
+  mjsBody* child_world = mjs_findBody(child_spec, "world");
+  mjsBody* child_body = mjs_addBody(child_world, 0);
+  mjsJoint* freejoint = mjs_addJoint(child_body, 0);
+  freejoint->type = mjJNT_FREE;
+  mjs_setName(freejoint->element, "child_freejoint");
+
+  mjSpec* parent_spec = mj_makeSpec();
+  mjsBody* parent_world = mjs_findBody(parent_spec, "world");
+  mjsBody* parent_body = mjs_addBody(parent_world, 0);
+
+  // Attach child spec to parent_body
+  mjsElement* attached =
+      mjs_attach(parent_body->element, child_spec->element, "pre_", "");
+  ASSERT_THAT(attached, NotNull());
+
+  // Delete freejoint from child_spec, should fail because it is attached
+  int result = mjs_delete(child_spec, freejoint->element);
+  EXPECT_EQ(result, -1);
+
+  // The freejoint should still be in parent_spec because deletion failed
+  mjsElement* found_joint =
+      mjs_findElement(parent_spec, mjOBJ_JOINT, "pre_child_freejoint");
+  EXPECT_THAT(found_joint, NotNull());
+
+  mj_deleteSpec(child_spec);
+  mj_deleteSpec(parent_spec);
+}
+
 TEST_F(MujocoTest, ActivatePlugin) {
   mjSpec* spec = mj_makeSpec();
   mjs_activatePlugin(spec, "mujoco.elasticity.cable");