CAD: undo() never rolled back variables, so a bad one bricked the recipe

checkpoint() snapshotted `features` and undo() restored `features`, but
`variables` is a separate member of CadDocument. Every caller of the documented
checkpoint -> mutate -> recompute -> undo-on-failure pattern therefore failed to
roll a variable back: the bad value stayed in the document and every later
recompute failed, which is exactly the corruption the pattern exists to prevent.
Feature `expr` bindings were unaffected only because expr lives inside
CadFeature and rode along in the features snapshot — which is why the feature
side appeared to work.

This was a kernel gap, not a GUI one: McpControl::action_set_variable has the
same sequence and was equally broken.

The undo/redo stacks now hold a {features, variables} Snapshot. Nothing here is
serialized, so no recipe version change and no golden-fixture regeneration.

Two tests, both verified to FAIL against a faithful reproduction of the bug
(undo() leaving `variables` untouched) at test_caddocument.cpp:4420 and :4441:
one covers restoring a variable's previous value, the other covers removing a
variable that did not exist before the checkpoint. Worth recording that the
first mutation attempt was NOT faithful — it dropped the restore but kept
std::move(variables) into the redo stack, which empties the map as a side effect
and made the second test pass for the wrong reason. A mutation has to reproduce
the original defect, not merely break the code.

Second defect, same area: undo() calls recompute(), which succeeds and clears
doc.error, so the reason an edit was rejected was destroyed before anything
could display it. Six sites — four in DesignPanel, two in McpControl — now carry
the message across the rollback. on_remove_variable additionally asserted
"referenced by a feature expression" as fact; it now offers that as the likely
cause and appends the real error, since that diagnosis is wrong for any other
failure.

Suite 141 cases / 1972 assertions (was 139/1960). GUI compiles clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

src/libslic3r/CadDocument.cpp

@@ -1728,8 +1728,8 @@ void CadDocument::clear()
 
 void CadDocument::checkpoint()
 {
-    m_undo.push_back(features);   // snapshot the pre-mutation recipe
-    m_redo.clear();               // any new action invalidates the redo branch
+    m_undo.push_back(Snapshot{features, variables});   // snapshot the pre-mutation recipe
+    m_redo.clear();                                    // any new action invalidates the redo branch
     if (m_undo.size() > k_undo_cap)
         m_undo.erase(m_undo.begin());
 }
@@ -1738,8 +1738,9 @@ bool CadDocument::undo()
 {
     if (m_undo.empty())
         return false;
-    m_redo.push_back(std::move(features));   // current state becomes redoable
-    features = std::move(m_undo.back());
+    m_redo.push_back(Snapshot{std::move(features), std::move(variables)});   // current state becomes redoable
+    features  = std::move(m_undo.back().features);
+    variables = std::move(m_undo.back().variables);
     m_undo.pop_back();
     recompute();   // benign-empty (only a sketch / empty doc) is a valid undo target
     return true;
@@ -1749,8 +1750,9 @@ bool CadDocument::redo()
 {
     if (m_redo.empty())
         return false;
-    m_undo.push_back(std::move(features));
-    features = std::move(m_redo.back());
+    m_undo.push_back(Snapshot{std::move(features), std::move(variables)});
+    features  = std::move(m_redo.back().features);
+    variables = std::move(m_redo.back().variables);
     m_redo.pop_back();
     recompute();
     return true;

src/libslic3r/CadDocument.hpp

@@ -662,11 +662,21 @@ private:
     static DatumCoordSys datum_frame(const std::vector<CadBody>& bodies, const CadFeature& f);
     void apply_mate(std::vector<CadBody>& bodies, const CadFeature& f) const;
 
-    // Undo/redo stacks of feature-list snapshots. checkpoint() pushes onto m_undo and
-    // clears m_redo; undo()/redo() shuffle the current state between them. Capped so a
-    // long session can't grow unbounded.
-    std::vector<std::vector<CadFeature>> m_undo;
-    std::vector<std::vector<CadFeature>> m_redo;
+    // Undo/redo stacks of recipe snapshots. checkpoint() pushes onto m_undo and clears
+    // m_redo; undo()/redo() shuffle the current state between them. Capped so a long
+    // session can't grow unbounded.
+    //
+    // The snapshot MUST carry `variables` as well as `features`: a caller that sets a bad
+    // variable, sees recompute() fail and calls undo() to roll it back would otherwise be
+    // left with the bad variable still in the document, so every later recompute fails —
+    // the exact corruption the checkpoint/undo pattern exists to prevent. Not serialized,
+    // so this changes no on-disk format.
+    struct Snapshot {
+        std::vector<CadFeature>            features;
+        std::map<std::string, std::string> variables;
+    };
+    std::vector<Snapshot> m_undo;
+    std::vector<Snapshot> m_redo;
     static constexpr size_t k_undo_cap = 200;
 };
 

src/slic3r/GUI/DesignPanel.cpp

@@ -8792,7 +8792,9 @@ void DesignPanel::on_add_variable()
     m_doc.checkpoint();
     m_doc.variables[name_str] = expr_str;
     bool ok = m_doc.recompute();
-    if (!ok) { m_doc.undo(); }
+    // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was
+    // rejected is gone before anything can display it. Carry it across the rollback.
+    if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; }
     after_tree_edit(ok);
     refresh_variables();
 }
@@ -8817,7 +8819,9 @@ void DesignPanel::on_edit_variable()
     m_doc.checkpoint();
     m_doc.variables[name_str] = expr_str;
     bool ok = m_doc.recompute();
-    if (!ok) { m_doc.undo(); }
+    // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was
+    // rejected is gone before anything can display it. Carry it across the rollback.
+    if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; }
     after_tree_edit(ok);
     refresh_variables();
 }
@@ -8837,11 +8841,16 @@ void DesignPanel::on_remove_variable()
     m_doc.variables.erase(name_str);
     bool ok = m_doc.recompute();
     if (!ok) {
+        // Capture before undo(): its recompute succeeds and clears doc.error.
+        const std::string why = m_doc.error;
         m_doc.undo();
-        // A failed recompute on remove means a feature expr still references it
+        // Almost always a feature expr still referencing the variable — but say so as the
+        // likely cause and keep the real message, rather than asserting a diagnosis that
+        // would be wrong for any other failure.
         m_doc.error = wxString::Format(
-            _L("Variable '%s' is referenced by a feature expression and cannot be removed"),
-            m_var_list->GetItemText(sel, 0)).ToUTF8().data();
+            _L("Variable '%s' could not be removed (likely still referenced by a feature "
+               "expression): %s"),
+            m_var_list->GetItemText(sel, 0), wxString::FromUTF8(why)).ToUTF8().data();
     }
     after_tree_edit(ok);
     refresh_variables();
@@ -8916,7 +8925,9 @@ void DesignPanel::on_set_expr()
     m_doc.checkpoint();
     m_doc.features[m_edit_index].expr[field] = expr;
     bool ok = m_doc.recompute();
-    if (!ok) { m_doc.undo(); }
+    // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was
+    // rejected is gone before anything can display it. Carry it across the rollback.
+    if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; }
     after_tree_edit(ok);
     if (ok) m_expr_text->Clear();
 
@@ -8958,7 +8969,9 @@ void DesignPanel::on_clear_expr()
     m_doc.checkpoint();
     feat_expr.erase(field);
     bool ok = m_doc.recompute();
-    if (!ok) { m_doc.undo(); }
+    // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was
+    // rejected is gone before anything can display it. Carry it across the rollback.
+    if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; }
     after_tree_edit(ok);
 
     if (m_edit_index >= 0 && m_edit_index < int(m_doc.features.size())) {

src/slic3r/GUI/McpControl.cpp

@@ -1372,7 +1372,9 @@ json action_set_variable(DesignPanel* panel, const json& params)
     doc.checkpoint();
     doc.variables[name] = expr;
     bool ok = doc.recompute();
-    if (!ok) { doc.undo(); }
+    // undo() recomputes, which succeeds and clears doc.error; carry the reason across so
+    // the JSON reply below reports why the edit was rejected instead of an empty string.
+    if (!ok) { const std::string why = doc.error; doc.undo(); doc.error = why; }
     panel->mcp_after_change();
     return json{{"ok", ok}, {"name", name}, {"error", doc.error}};
 }
@@ -1392,7 +1394,9 @@ json action_set_feature_expr(DesignPanel* panel, const json& params)
     doc.checkpoint();
     doc.features[fi].expr[field] = expr;
     bool ok = doc.recompute();
-    if (!ok) { doc.undo(); }
+    // undo() recomputes, which succeeds and clears doc.error; carry the reason across so
+    // the JSON reply below reports why the edit was rejected instead of an empty string.
+    if (!ok) { const std::string why = doc.error; doc.undo(); doc.error = why; }
     panel->mcp_after_change();
     return json{{"ok", ok}, {"feature", fi}, {"field", field}, {"error", doc.error}};
 }

tests/libslic3r/test_caddocument.cpp

@@ -4503,6 +4503,51 @@ TEST_CASE("unknown identifier fails recompute", "[CadDocument][variables]")
     REQUIRE_CONTAINS(doc.error, "unknown identifier");
 }
 
+// The checkpoint -> mutate -> recompute -> undo-on-failure pattern is what both the GUI and
+// McpControl use to keep a bad edit out of the document. It only works if the snapshot covers
+// `variables` as well as `features`: undo() used to restore features alone, so a bad variable
+// survived the rollback and every later recompute failed — the document was left unusable.
+TEST_CASE("undo rolls back a bad variable, not just features", "[CadDocument][variables]")
+{
+    CadDocument doc;
+    int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Sketch");
+    doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude");
+    doc.features[sk].expr = {{"width", "w"}};
+    doc.variables = {{"w", "20"}};
+    REQUIRE(doc.recompute());
+
+    // Caller sets a variable to something unevaluable, exactly as action_set_variable does.
+    doc.checkpoint();
+    doc.variables["w"] = "nosuchvar + 1";
+    REQUIRE_FALSE(doc.recompute());
+    REQUIRE(doc.undo());
+
+    // The good value must be back...
+    REQUIRE(doc.variables.at("w") == "20");
+    // ...and, the part that actually bit, the document must still be usable.
+    REQUIRE(doc.recompute());
+    REQUIRE(doc.error.empty());
+}
+
+// A variable ADDED under a checkpoint must disappear entirely on undo, not linger with a
+// stale value: the pre-mutation snapshot simply did not contain the key.
+TEST_CASE("undo removes a variable that did not exist before the checkpoint", "[CadDocument][variables]")
+{
+    CadDocument doc;
+    int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Sketch");
+    doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude");
+    REQUIRE(doc.recompute());
+    REQUIRE(doc.variables.empty());
+
+    doc.checkpoint();
+    doc.variables["bogus"] = "1/0 +";     // syntactically broken
+    REQUIRE_FALSE(doc.recompute());
+    REQUIRE(doc.undo());
+
+    REQUIRE(doc.variables.count("bogus") == 0);
+    REQUIRE(doc.recompute());
+}
+
 TEST_CASE("parametric recipe round-trips through serialize/deserialize", "[CadDocument][variables]")
 {
     using Catch::Matchers::WithinAbs;