Round-9 review fixes: nested adds from negative/sub fallthroughs, unconverted tensor insert, zero coeff

Round 9 (adversarial probe of the round-8 delta) confirmed three
value-preserving structural defects:

- Nested adds: add + (-t) for a non-cancelling t, and add - expr in
  both the core and tensor sub get_default, pushed the WHOLE lhs add
  as a single child. (C-A)-B built tensor_add{-B, tensor_add{C,-A}},
  and buried children then defeated merge cancellation:
  (A+B)+((C-A)-B) kept an unreachable interior -A ('A+C-A'). The
  negative dispatch and both get_defaults now copy the add and
  signed-insert the other operand, finishing through the usual
  collapse. A tensor print expectation that had pinned the nested
  shape's output ordering was updated to the flat form.

- The tensor n-ary template dispatch still inserted its combined term
  via plain merge_or_insert: (5A-2A)+(-3A) held an exact {2A,-(2A)}
  pair instead of collapsing to zero. Now add_insert_signed, matching
  the scalar/t2s merge_and_finish.

- merge_add stored a cancelled coefficient as a literal zero holder:
  (2+x)+(y-2) printed '0+x+y' and compared unequal to x+y. Cancelled
  coefficients are now dropped.

Also converted every remaining dispatcher-side merge_or_insert in
simplifier_sub.h/simplifier_add.h to the shared insert_signed helper
(exact-negation combining + zero filter) so no insertion path can
leave a {t,-t} pair or a zero child.

Regression tests in CoreBugFixTest (RoundNineReview suite).

Refs #340.

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

Author: petlenz

include/numsim_cas/core/simplifier/simplifier_add.h

@@ -423,7 +423,12 @@ class n_ary_add_dispatch
       return base::m_lhs - rhs.expr();
     }
 
-    return base::get_default();
+    // non-cancelling -t: insert into a copy — get_default would nest the
+    // whole lhs add as a single child (round-9 review)
+    auto add_expr{make_expression<typename Traits::add_type>(lhs)};
+    auto &add{add_expr.template get<typename Traits::add_type>()};
+    insert_signed<Traits>(add, base::m_rhs);
+    return detail::finalize_add<Traits>(std::move(add_expr));
   }
 
 protected:
@@ -560,7 +565,7 @@ class negative_add_dispatch
     auto &add{add_expr.template get<typename Traits::add_type>()};
     // (-x) + (y - x): the map may already hold -x, so merge instead of a
     // raw push_back (which asserts on duplicates)
-    add.merge_or_insert(base::m_lhs);
+    insert_signed<Traits>(add, base::m_lhs);
     return detail::finalize_add<Traits>(std::move(add_expr));
   }
 

include/numsim_cas/core/simplifier/simplifier_common.h

@@ -4,6 +4,7 @@
 #include <numsim_cas/core/domain_traits.h>
 #include <numsim_cas/core/expression_holder.h>
 #include <numsim_cas/core/scalar_number.h>
+#include <numsim_cas/functions.h>
 
 namespace numsim::cas::detail {
 
@@ -44,6 +45,18 @@ finalize_add(expression_holder<typename Traits::expression_type> expr) {
   return expr;
 }
 
+// merge_or_insert with exact-negation combining and zero filtering:
+// dispatcher-side child insertion must never leave a {t, -t} pair or a
+// literal zero child (round-8/9 reviews).
+template <typename Traits>
+inline void insert_signed(typename Traits::add_type &add,
+                          typename Traits::expr_holder_t entry) {
+  add_insert_signed(add, std::move(entry),
+                    [](typename Traits::expr_holder_t const &e) {
+                      return is_same<typename Traits::zero_type>(e);
+                    });
+}
+
 // Fold a numeric delta into the coefficient of `expr` (an add node).
 // A valid-but-non-numeric coefficient (symbolic, t2s) is combined as an
 // expression — get_coefficient reports 0 for it, and the old free()/

include/numsim_cas/core/simplifier/simplifier_sub.h

@@ -41,6 +41,18 @@ class sub_dispatch {
       return Traits::make_constant(result);
     }
 
+    if (is_same<add_type>(m_lhs)) {
+      // copy children — pushing the whole add would nest it (round-9)
+      auto add_expr{make_expression<add_type>(m_lhs.template get<add_type>())};
+      if (rhs_val) {
+        return fold_constant_into_add_coeff<Traits>(std::move(add_expr),
+                                                    -(*rhs_val));
+      }
+      auto &add{add_expr.template get<add_type>()};
+      insert_signed<Traits>(add, -m_rhs);
+      return finalize_add<Traits>(std::move(add_expr));
+    }
+
     auto add_new{make_expression<add_type>()};
     auto &add{add_new.template get<add_type>()};
     if (lhs_val) {
@@ -54,7 +66,7 @@ class sub_dispatch {
     } else {
       // -m_rhs may collapse onto m_lhs (z - (-z)); a raw push_back
       // would hit the no-duplicates assert
-      add.merge_or_insert(-m_rhs);
+      insert_signed<Traits>(add, -m_rhs);
     }
     return finalize_add<Traits>(std::move(add_new));
   }
@@ -99,7 +111,7 @@ class negative_sub_dispatch
   expr_holder_t dispatch(typename Traits::add_type const &rhs) {
     auto add_expr{make_expression<typename Traits::add_type>(rhs)};
     auto &add{add_expr.template get<typename Traits::add_type>()};
-    add.merge_or_insert(lhs.expr());
+    insert_signed<Traits>(add, lhs.expr());
     return make_expression<typename Traits::negative_type>(std::move(add_expr));
   }
 
@@ -145,7 +157,7 @@ class constant_sub_dispatch
   // call operator- on an invalid holder and throw. Mirrors the guard
   // pattern in n_ary_sub_dispatch::dispatch(add_type) from PR #98.
   //
-  // Children are negated and pushed via merge_or_insert (not push_back)
+  // Children are negated and inserted via insert_signed (not push_back)
   // so a `-child` that collides with another entry in the result is
   // combined rather than throwing duplicate-child internal_error.
   //
@@ -163,7 +175,7 @@ class constant_sub_dispatch
       add.set_coeff(base::m_lhs);
     }
     for (auto &child : rhs.symbol_map() | std::views::values) {
-      add.merge_or_insert(-child);
+      insert_signed<Traits>(add, -child);
     }
     return finalize_add<Traits>(std::move(add_expr));
   }
@@ -258,15 +270,15 @@ class n_ary_sub_dispatch
         used_expr.insert(pos->second);
         auto combined = child - pos->second;
         if (!is_same<typename Traits::zero_type>(combined))
-          add.merge_or_insert(std::move(combined));
+          insert_signed<Traits>(add, std::move(combined));
       } else {
-        add.merge_or_insert(child);
+        insert_signed<Traits>(add, child);
       }
     }
     if (used_expr.size() != rhs.size()) {
       for (auto &child : rhs.symbol_map() | std::views::values) {
         if (!used_expr.count(child)) {
-          add.merge_or_insert(-child);
+          insert_signed<Traits>(add, -child);
         }
       }
     }
@@ -289,10 +301,10 @@ class n_ary_sub_dispatch
       auto combined{pos->second - base::m_rhs};
       add.symbol_map().erase(pos);
       if (!is_same<typename Traits::zero_type>(combined))
-        add.merge_or_insert(std::move(combined));
+        insert_signed<Traits>(add, std::move(combined));
       return finalize_add<Traits>(std::move(expr_add));
     }
-    add.merge_or_insert(-base::m_rhs);
+    insert_signed<Traits>(add, -base::m_rhs);
     return finalize_add<Traits>(std::move(expr_add));
   }
 

include/numsim_cas/functions.h

@@ -48,7 +48,12 @@ constexpr inline void merge_add(n_ary_tree<Derived> const &lhs,
   using expr_t = typename Derived::expr_t;
 
   if (lhs.coeff().is_valid() && rhs.coeff().is_valid()) {
-    result.set_coeff(lhs.coeff() + rhs.coeff());
+    // cancelled coefficients must be dropped, not stored as a literal
+    // zero ((2+x)+(y-2) printed "0+x+y", round-9 review)
+    auto coeff{lhs.coeff() + rhs.coeff()};
+    if (!is_zero(coeff)) {
+      result.set_coeff(std::move(coeff));
+    }
   } else {
     if (lhs.coeff().is_valid()) {
       result.set_coeff(lhs.coeff());

include/numsim_cas/tensor/simplifier/tensor_simplifier_sub.h

@@ -3,6 +3,7 @@
 
 #include <numsim_cas/basic_functions.h>
 #include <numsim_cas/core/operators.h>
+#include <numsim_cas/functions.h>
 #include <numsim_cas/tensor/tensor_definitions.h>
 #include <numsim_cas/tensor/tensor_expression.h>
 #include <numsim_cas/tensor/tensor_std.h>
@@ -32,12 +33,30 @@ class sub_default : public tensor_visitor_return_expr_t {
 #undef NUMSIM_ADD_OVR
 
   // rhs is negative
-  auto get_default() {
+  expr_holder_t get_default() {
+    // copy an add lhs (pushing it wholesale would nest, round-9 review);
+    // signed insert so X-(-X) combines instead of hitting the dup assert
     auto add_new{
-        make_expression<tensor_add>(m_lhs.get().dim(), m_lhs.get().rank())};
+        is_same<tensor_add>(m_lhs)
+            ? make_expression<tensor_add>(m_lhs.template get<tensor_add>())
+            : make_expression<tensor_add>(m_lhs.get().dim(),
+                                          m_lhs.get().rank())};
     auto &add{add_new.template get<tensor_add>()};
-    add.push_back(m_lhs);
-    add.push_back(-m_rhs);
+    if (!is_same<tensor_add>(m_lhs)) {
+      add.push_back(m_lhs);
+    }
+    add_insert_signed(add, -m_rhs, [](expr_holder_t const &e) {
+      return is_same<tensor_zero>(e);
+    });
+    add.invalidate_hash();
+    add.recompute_space();
+    if (add.size() == 0) {
+      return make_expression<tensor_zero>(m_lhs.get().dim(),
+                                          m_lhs.get().rank());
+    }
+    if (add.size() == 1 && !add.coeff().is_valid()) {
+      return add.symbol_map().begin()->second;
+    }
     return add_new;
   }
 

src/numsim_cas/tensor/simplifier/tensor_simplifier_add.cpp

@@ -54,7 +54,11 @@ n_ary_add::dispatch([[maybe_unused]] Expr const &rhs) {
     auto combined{pos->second + m_rhs};
     add.symbol_map().erase(pos);
     if (!is_same<tensor_zero>(combined)) {
-      add.merge_or_insert(std::move(combined));
+      // signed insert: the combined term may exactly negate another child
+      // ((5A-2A)+(-3A) left a {2A,-(2A)} pair, round-9 review)
+      add_insert_signed(add, std::move(combined), [](expr_holder_t const &e) {
+        return is_same<tensor_zero>(e);
+      });
     }
     add.invalidate_hash();
     add.recompute_space();
@@ -104,7 +108,22 @@ n_ary_add::dispatch(tensor_negative const &rhs) {
     }
     return expr;
   }
-  return get_default();
+  // non-cancelling -t: insert into a copy — get_default would nest the
+  // whole lhs add as a single child (round-9 review)
+  auto expr{make_expression<tensor_add>(m_lhs_node)};
+  auto &add{expr.template get<tensor_add>()};
+  add_insert_signed(add, m_rhs, [](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

@@ -1485,6 +1485,45 @@ TEST(RoundEightReview, TensorMergeAddZeroFiltered) {
   EXPECT_TRUE(is_same<tensor_zero>(e3)) << to_string(e3);
 }
 
+// Round-9 review: nested adds from the negative fall-through, one
+// unconverted tensor insert, and a literal zero coefficient.
+
+// R9-1: add + (-t) with a non-cancelling t fell to get_default, which
+// nested the whole lhs add as a single child; buried terms then defeated
+// merge cancellation.
+TEST(RoundNineReview, AddNegativeStaysFlat) {
+  auto [x, y] = make_scalar_variable("x", "y");
+  auto e = (x + 5.0 * y) + (-y);
+  // flat children: the exact 5*y child stays reachable for cancellation
+  auto e2 = e + (-(5.0 * y));
+  EXPECT_TRUE(*e2 == *(x - y)) << to_string(e2);
+  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 t = (A + B) + ((C - A) - B);
+  EXPECT_TRUE(*t == *C) << to_string(t);
+  EXPECT_TRUE(is_same<tensor_zero>(t - C)) << to_string(t - C);
+}
+
+// R9-2: the tensor n-ary fallback still plain-inserted the combined term;
+// (5A-2A)+(-3A) held an exact {2A, -(2A)} pair instead of collapsing.
+TEST(RoundNineReview, TensorCombinedInsertIsSigned) {
+  auto [A] = make_tensor_variable(std::tuple{"A", std::size_t{3}, 2});
+  auto e = (5.0 * A + (-(2.0 * A))) + (-3.0) * A;
+  EXPECT_TRUE(is_same<tensor_zero>(e)) << to_string(e);
+}
+
+// R9-3: merge_add stored a cancelled coefficient as a literal zero —
+// (2+x)+(y-2) printed "0+x+y" and compared unequal to x+y.
+TEST(RoundNineReview, MergeAddDropsCancelledCoeff) {
+  auto [x, y] = make_scalar_variable("x", "y");
+  auto e = (2.0 + x) + (y - 2.0);
+  EXPECT_TRUE(*e == *(x + y)) << to_string(e);
+  auto [A] = make_tensor_variable(std::tuple{"A", std::size_t{3}, 2});
+  auto t = (2.0 + trace(A)) + (det(A) - 2.0);
+  EXPECT_TRUE(*t == *(trace(A) + det(A))) << to_string(t);
+}
+
 } // namespace numsim::cas
 
 #endif // COREBUGFIXTEST_H

tests/TensorExpressionTest.h

@@ -438,8 +438,9 @@ TYPED_TEST(TensorExpressionTest, AddNaryPlusNegativeCancellation) {
   EXPECT_PRINT((X + Y) + (-X), "Y");
   EXPECT_PRINT((X + Y) + (-Y), "X");
   EXPECT_PRINT((X + Y + Z) + (-Y), "X+Z");
-  // negative not in sum → default add (hash-ordered output)
-  EXPECT_PRINT((X + Y) + (-Z), "-Z+X+Y");
+  // negative not in sum → flat insert into the copy (round-9: the old
+  // fallback nested the whole lhs add, which printed as "-Z+X+Y")
+  EXPECT_PRINT((X + Y) + (-Z), "X+Y-Z");
 }
 
 // n_ary_add: merge two adds: (X+Y) + (Y+Z) → X+2*Y+Z