Round-8 review fixes: merge_add double-consume, tensor zero-child regression

Round 8 (adversarial probe of the round-7 delta) confirmed two
regressions introduced by the round-7 negation probe in merge_add:

- Double-consume (wrong value): when the lhs held an exact {t, -t}
  pair, t consumed the rhs child -t via the negation probe and then -t
  direct-matched the same rhs child again — (5x-5x) + (y-5x) evaluated
  to y-10x. Matches are now gated on the used-set in both probe paths.
  The enabler is closed too: child insertion in merge_add and
  merge_and_finish now goes through add_insert_signed, which combines
  an entry with an existing exact match or exact negation until no
  collision remains and zero-filters the result, so an add can no
  longer hold an exact {t, -t} pair at all ((2x-5x)+3x now collapses
  to the zero singleton at construction).

- Tensor zero child: the shared merge_add gained cancellation power in
  round 7, but only the scalar/t2s dispatch site got the zero filter —
  the tensor caller inserted fully-cancelled combines as literal 0{2}
  children ((A+B)+(C-A) -> "0{2}+B+C", breaking identity and
  round-trip cancellation). The tensor site now passes the
  is_same<tensor_zero> filter and collapses degenerate results the
  same way the scalar path does; the filter-less merge_add overload is
  gone so no caller can opt out silently.

Regression tests in CoreBugFixTest (RoundEightReview suite).

Refs #340.

Signed-off-by: petlenz <peterlenz89.pl@gmail.com>

Author: petlenz

include/numsim_cas/core/simplifier/simplifier_add.h

@@ -325,7 +325,11 @@ class n_ary_add_dispatch
                                  typename Traits::add_type &add,
                                  expr_holder_t combined) {
     if (!is_same<typename Traits::zero_type>(combined)) {
-      add.merge_or_insert(std::move(combined));
+      // signed insert: the combined term may exactly negate an existing
+      // child; a plain insert would leave a {t,-t} pair (round-8 review)
+      add_insert_signed(add, std::move(combined), [](expr_holder_t const &e) {
+        return is_same<typename Traits::zero_type>(e);
+      });
     }
     add.invalidate_hash();
     if (add.size() == 0) {

include/numsim_cas/functions.h

@@ -6,10 +6,41 @@
 
 namespace numsim::cas {
 
+/// Insert `entry` into an add-semantics tree, combining with an existing
+/// exact match or exact negation until no collision remains; `is_zero`
+/// filters fully-cancelled results so {t, -t} pairs never coexist
+/// (round-8 review: such a pair downstream caused a double-consume merge).
+template <typename Derived, typename IsZero>
+constexpr inline void
+add_insert_signed(n_ary_tree<Derived> &tree,
+                  expression_holder<typename Derived::expr_t> entry,
+                  IsZero &&is_zero) {
+  while (entry.is_valid() && !is_zero(entry)) {
+    auto pos = tree.find_like(entry);
+    if (pos == tree.symbol_map().end()) {
+      auto neg{-entry};
+      pos = tree.find_like(neg);
+      if (pos != tree.symbol_map().end() && !(pos->second == neg)) {
+        pos = tree.symbol_map().end();
+      }
+    }
+    if (pos == tree.symbol_map().end()) {
+      tree.merge_or_insert(std::move(entry));
+      return;
+    }
+    auto next = pos->second + entry;
+    tree.symbol_map().erase(pos);
+    entry = std::move(next);
+  }
+}
+
 /// merge two n_ary_trees
 ///   --> add (x+y+z) + (x+a) --> 2*x+y+z+a --> mul
 /// `is_zero` filters fully-cancelled combines (x + (-x)); negation pairs
 /// share no hash, so a failed find_like retries with -child (round-7).
+/// A matched rhs child is consumed exactly once: a second lhs child must
+/// not re-match it (round-8 review: 5x and -(5x) both consuming -(5x)
+/// turned y-5x into y-10x).
 template <typename Derived, typename IsZero>
 constexpr inline void merge_add(n_ary_tree<Derived> const &lhs,
                                 n_ary_tree<Derived> const &rhs,
@@ -30,40 +61,37 @@ constexpr inline void merge_add(n_ary_tree<Derived> const &lhs,
   expr_set<expression_holder<expr_t>> used_expr;
   for (auto &child : lhs.symbol_map() | std::views::values) {
     auto pos{rhs.find_like(child)};
+    if (pos != rhs.symbol_map().end() && used_expr.count(pos->second)) {
+      pos = rhs.symbol_map().end();
+    }
     if (pos == rhs.symbol_map().end()) {
       // only an exact -child cancels; like-terms of -child would nest adds
       auto neg_child{-child};
       pos = rhs.find_like(neg_child);
-      if (pos != rhs.symbol_map().end() && !(pos->second == neg_child)) {
+      if (pos != rhs.symbol_map().end() &&
+          (!(pos->second == neg_child) || used_expr.count(pos->second))) {
         pos = rhs.symbol_map().end();
       }
     }
     if (pos != rhs.symbol_map().end()) {
       used_expr.insert(pos->second);
       auto combined{child + pos->second};
       if (!is_zero(combined)) {
-        result.merge_or_insert(std::move(combined));
+        add_insert_signed(result, std::move(combined), is_zero);
       }
     } else {
-      result.merge_or_insert(child);
+      add_insert_signed(result, child, is_zero);
     }
   }
   if (used_expr.size() != rhs.size()) {
     for (auto &child : rhs.symbol_map() | std::views::values) {
       if (!used_expr.count(child)) {
-        result.merge_or_insert(child);
+        add_insert_signed(result, child, is_zero);
       }
     }
   }
 }
 
-template <typename Derived>
-constexpr inline void merge_add(n_ary_tree<Derived> const &lhs,
-                                n_ary_tree<Derived> const &rhs,
-                                n_ary_tree<Derived> &result) {
-  merge_add(lhs, rhs, result, [](auto const &) { return false; });
-}
-
 } // namespace numsim::cas
 
 #endif // FUNCTIONS_H

src/numsim_cas/tensor/simplifier/tensor_simplifier_add.cpp

@@ -74,8 +74,18 @@ n_ary_add::dispatch([[maybe_unused]] Expr const &rhs) {
 n_ary_add::dispatch(tensor_add const &rhs) {
   auto expr{make_expression<tensor_add>(rhs.dim(), rhs.rank())};
   auto &add{expr.template get<tensor_add>()};
-  merge_add(m_lhs_node, rhs, add);
+  // zero filter + collapse: childwise combines may fully cancel
+  // ((A+B)+(C-A) left a literal 0{2} child, round-8 review)
+  merge_add(m_lhs_node, rhs, add,
+            [](expr_holder_t const &e) { return is_same<tensor_zero>(e); });
+  add.invalidate_hash();
   add.recompute_space();
+  if (add.size() == 0) {
+    return tensor_traits::zero(m_lhs);
+  }
+  if (add.size() == 1 && !add.coeff().is_valid()) {
+    return add.symbol_map().begin()->second;
+  }
   return expr;
 }
 

tests/CoreBugFixTest.h

@@ -1446,6 +1446,45 @@ TEST(RoundSevenReview, AddCancelsAgainstNegativeChild) {
   EXPECT_TRUE(*f == *(trace(A) + w(c4))) << to_string(f);
 }
 
+// Round-8 review: regressions from the round-7 negation probe.
+
+// R8-1: merge_add consumed the same rhs child twice when the lhs held an
+// exact {t,-t} pair — 5x neg-matched -(5x), then -(5x) direct-matched it
+// again, turning y-5x into y-10x. Also kills the enabler: signed inserts
+// keep {t,-t} from coexisting at all.
+TEST(RoundEightReview, MergeAddNoDoubleConsume) {
+  auto [x, y] = make_scalar_variable("x", "y");
+  auto f = (2.0 * x + (-(5.0 * x))) + 3.0 * x;
+  EXPECT_TRUE(is_same<scalar_zero>(f)) << to_string(f);
+  scalar_evaluator<double> ev;
+  ev.set(x, 1.0);
+  ev.set(y, 0.0);
+  EXPECT_DOUBLE_EQ(ev.apply(f + (y - 5.0 * x)), -5.0);
+  // an add manually holding the exact pair must still merge correctly
+  auto pair_add = make_expression<scalar_add>();
+  auto &pa = pair_add.get<scalar_add>();
+  pa.push_back(5.0 * x);
+  pa.push_back(-(5.0 * x));
+  auto g = pair_add + (y - 5.0 * x);
+  EXPECT_DOUBLE_EQ(ev.apply(g), -5.0) << to_string(g); // was -10
+}
+
+// R8-2: the tensor add-merge got round-7's cancellation power without the
+// zero filter — (A+B)+(C-A) held a literal 0{2} child.
+TEST(RoundEightReview, TensorMergeAddZeroFiltered) {
+  auto [A, B, C] = make_tensor_variable(std::tuple{"A", std::size_t{3}, 2},
+                                        std::tuple{"B", std::size_t{3}, 2},
+                                        std::tuple{"C", std::size_t{3}, 2});
+  auto e1 = (A + B) + (C - A);
+  EXPECT_TRUE(*e1 == *(B + C)) << to_string(e1);
+  EXPECT_EQ(to_string(e1).find("0{2}"), std::string::npos) << to_string(e1);
+  auto e2 = e1 - (B + C);
+  EXPECT_TRUE(is_same<tensor_zero>(e2)) << to_string(e2);
+  // full cancellation collapses to the zero singleton, not an empty add
+  auto e3 = (A + B) + ((-A) + (-B));
+  EXPECT_TRUE(is_same<tensor_zero>(e3)) << to_string(e3);
+}
+
 } // namespace numsim::cas
 
 #endif // COREBUGFIXTEST_H