n_ary_sub_dispatch::dispatch(add, add) has unguarded coeff add and uses + instead of -

[petlenz]

Summary

n_ary_sub_dispatch::dispatch(typename Traits::add_type const &rhs) — the "merge two add expressions" branch in include/numsim_cas/core/simplifier/simplifier_sub.h — has two pre-existing defects that surface when both operands are add expressions with the same coefficient state.

Discovered during the PR #90 review while routing the other sub-side dispatchers through n_ary_tree::merge_or_insert. Out of scope for that PR; filing here for follow-up.

Repro

auto [x, y, z] = make_scalar_variable("x", "y", "z");
auto lhs = x + y + z;       // scalar_add, no constant coefficient
auto rhs = x + y;           // scalar_add, no constant coefficient
auto diff = lhs - rhs;      // throws invalid_expression_error

(x+y+z) - (x+y) should simplify to z. Instead it throws "expression_holder: access to invalid (null) expression".

Root cause

The dispatch body, abbreviated:

expr_holder_t dispatch(typename Traits::add_type const &rhs) {
  auto expr{make_expression<typename Traits::add_type>()};
  auto &add{expr.template get<typename Traits::add_type>()};
  add.set_coeff(lhs.coeff() + rhs.coeff());          // <-- defect 1 and 2
  ...
  for (auto &child : lhs.symbol_map() | std::views::values) {
    auto pos{rhs.symbol_map().find(child)};
    if (pos != rhs.symbol_map().end()) {
      add.merge_or_insert(child + pos->second);      // <-- defect 2 (also here)
    } else {
      add.merge_or_insert(child);
    }
  }
  ...
}

Defect 1 — unguarded coefficient addition

lhs.coeff() + rhs.coeff() is only safe when both holders are valid. When both adds have no constant term (their default state after x + y + z style construction), both coeffs are default-constructed (invalid). tag_invoke(add_fn, invalid, invalid) calls lhs.get<scalar_visitable_t>() which throws invalid_expression_error.

The symmetric routine in functions.h already handles this correctly:

// merge_add — already robust
if (lhs.coeff().is_valid() && rhs.coeff().is_valid()) {
  result.set_coeff(lhs.coeff() + rhs.coeff());
} else {
  if (lhs.coeff().is_valid()) result.set_coeff(lhs.coeff());
  if (rhs.coeff().is_valid()) result.set_coeff(rhs.coeff());
}

The sub dispatch should use the same shape (with - semantics — see defect 2).

Defect 2 — wrong sign for subtraction

The function lives in simplifier_sub.h and constructs the result of lhs_add - rhs_add. Mathematically:

(c_l + a + b) - (c_r + a + d) = (c_l - c_r) + 0 + b - d
                              = (c_l - c_r) + b - d

• Coefficient should be lhs.coeff() - rhs.coeff(), not +.
• Child-child combination (when both add children share a key) should be child - pos->second, not child + pos->second.
• Children present only in rhs should be added as -child, not child.

The current implementation looks like a copy-paste from the corresponding add routine that was never converted to subtraction semantics.

Why existing tests don't catch this

Two factors:

1. The combination of (a) both LHS and RHS being add types and (b) both having invalid coefficients isn't exercised by any current named test. The fuzz suite may have hit it intermittently but cas_error is caught and the seed silently skipped.
2. The +-instead-of-- defect produces a wrong but non-throwing result on most inputs (e.g. (2+x) - (1+x) would compute 3 + 2x instead of 1), but no test asserts the value of a 2-add subtraction whose result depends on the merge path.

Fix sketch

expr_holder_t dispatch(typename Traits::add_type const &rhs) {
  auto expr{make_expression<typename Traits::add_type>()};
  auto &add{expr.template get<typename Traits::add_type>()};

  // Coefficient: c_l - c_r, guarded against invalid sides.
  if (lhs.coeff().is_valid() && rhs.coeff().is_valid()) {
    add.set_coeff(lhs.coeff() - rhs.coeff());
  } else if (lhs.coeff().is_valid()) {
    add.set_coeff(lhs.coeff());
  } else if (rhs.coeff().is_valid()) {
    add.set_coeff(-rhs.coeff());
  }

  std::set<expr_holder_t> used_expr;
  for (auto &child : lhs.symbol_map() | std::views::values) {
    auto pos{rhs.symbol_map().find(child)};
    if (pos != rhs.symbol_map().end()) {
      used_expr.insert(pos->second);
      add.merge_or_insert(child - pos->second);   // was `+`
    } else {
      add.merge_or_insert(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);              // was `child`
      }
    }
  }
  return expr;
}

Acceptance criteria

• (x+y+z) - (x+y) simplifies to z without throwing.
• (2+x) - (1+x) simplifies to 1.
• (a + b) - (a + b) simplifies to 0.
• Deterministic regression test added to CoreBugFixTest covering at least the three cases above (replaces the marker comment near SubSymbolDispatchSmoke in tests/CoreBugFixTest.h).
• Same fix applied symmetrically to the t2s and tensor specializations if they have parallel implementations.

References

• Surfaced in PR Remove flaky tensor diff seed skip list by fixing underlying bugs #90, fifth-pass review and follow-up commit bf75393.
• Marker comment in simplifier_sub.h at the affected dispatch.
• Marker comment in tests/CoreBugFixTest.h near SubSymbolDispatchSmoke.
• Working reference for the coefficient-guard pattern: merge_add in include/numsim_cas/functions.h.

[petlenz]

Triage (2026-05-16): Open and tracked. Status summary across recent issues:

• Closed via PR chain when it merges (tagged Closes #N in the PR body):

  • Fuzzy tensor diff seeds skipped due to underlying CAS bugs #89 (fuzz seeds) → PR Remove flaky tensor diff seed skip list by fixing underlying bugs #90.
  • n_ary_sub_dispatch::dispatch(add, add) has unguarded coeff add and uses + instead of - #91 (n_ary_sub_dispatch bug) → PR Audit simplifier dispatchers and fix n_ary_sub_dispatch(add, add) bug #98.
  • Add deterministic regression for merge_or_insert transitive collision loop #92 (merge_or_insert regression) → PR Add merge_or_insert iteration-counter instrumentation and lock-in tests #106.
  • Add tensor pow simplifier (missing entire family) #96 (tensor pow simplifier) → PR Add tensor pow simplifications for pow(I, n) and pow(inv(A), n) #105.
  • Add n_ary_mul_mul_dispatch (LHS=mul) sibling for the mul dispatcher family #97 (n_ary_mul_mul_dispatch) → PR Correct #97 audit claim; lock in existing mul × mul merge behavior #103.
  • Centralize trivial-result collapse pattern from simplifier dispatchers #99 (collapse pattern centralisation) → PR Centralize trivial-result collapse as finalize_add<Traits> helper #100.

• Partial work in flight (issue stays open):

  • Investigate platform-dependent Skew space annotation loss inside tensor_mul #93 (Skew space loss) — partial work in PR Investigate #93: lock in Skew preservation (cannot reproduce original loss) #108; cannot reproduce on current platform; structural detection in skew_classification.h is the defensive fallback.
  • tensor_evaluator rebuilds t2s_eval per tensor_to_scalar_with_tensor_mul visit #94 (t2s_eval rebuild) — partial work in PR Lock in correctness of t2s_eval rebuild (partial work on #94) #107; full optimization deferred per Roadmap: execute t2s_eval optimization (post-bench) #112 roadmap.
  • Unify simplifier dispatchers across add/sub/mul/pow and across domains #95 (simplifier-unification umbrella) — audit landed in PR Audit simplifier dispatchers and fix n_ary_sub_dispatch(add, add) bug #98; consolidation follow-ups are Consolidate scalar and t2s mul × mul merge into a generic n_ary_mul_mul_dispatch #104, Unify tensor pow simplifier with the dispatcher pattern used by scalar/t2s #109, others.

• Filed during review but not yet implemented:

  • Audit *DispatchSmoke / *ConstructionSmoke tests for missing value assertions #101 (smoke-test audit) — sweep tracked by Execute smoke-test audit sweep per #101 #113.
  • constant_sub_dispatch::dispatch(add) has same unguarded-coeff + push_back bug pattern #102 (constant_sub_dispatch bug) — fix shape documented in the issue body.
  • Consolidate scalar and t2s mul × mul merge into a generic n_ary_mul_mul_dispatch #104 (mul × mul consolidation) — design space documented.
  • Unify tensor pow simplifier with the dispatcher pattern used by scalar/t2s #109 (tensor pow dispatcher) — design space documented.
  • Add microbenchmark for tensor_evaluator (prerequisite for #94 optimization) #110 (microbenchmark) — prerequisite for Roadmap: execute t2s_eval optimization (post-bench) #112.
  • Add comprehensive tensor space-annotation preservation matrix #111 (annotation preservation matrix) — design space documented.

No code change planned from this triage; each item has its own tracking ticket and an implementation path.