From ec2f3cd5963b435db96e0bde7fd6dcbd2d9c752d Mon Sep 17 00:00:00 2001 From: petlenz Date: Wed, 16 Sep 2026 22:25:00 +0200 Subject: [PATCH 1/2] Fix #348: discriminate symbols across domains in expression identity Node ids index each domain's own type list, so a scalar and a tensor symbol of the same name tie on id and on the name-only hash. Comparison then downcast to the wrong type: abort in Debug, SIGSEGV in Release. - expression::operator==/< compare the dynamic type once id and hash tie, before the same-type downcast. - symbol_base::operator== compares the name, operator< tiebreaks on it, so per-char hash collisions no longer merge distinct symbols. - relation::operator< orders by its operands rather than their hashes, so colliding relations stay distinct members of relation::set. Signed-off-by: petlenz --- include/numsim_cas/core/assumptions.h | 14 ++++++------- include/numsim_cas/core/symbol_base.h | 7 +++++-- src/numsim_cas/core/expression.cpp | 11 ++++++++++ tests/CoreBugFixTest.h | 29 +++++++++++++++++++++++++++ tests/ScalarAssumptionTest.h | 16 +++++++++++++++ 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/include/numsim_cas/core/assumptions.h b/include/numsim_cas/core/assumptions.h index 6cc0973a..ebe38ba3 100644 --- a/include/numsim_cas/core/assumptions.h +++ b/include/numsim_cas/core/assumptions.h @@ -204,13 +204,13 @@ template struct relation { auto ai = a.k.index(), bi = b.k.index(); if (ai != bi) return ai < bi; - auto al = a.lhs.get().hash_value(); - auto bl = b.lhs.get().hash_value(); - if (al != bl) - return al < bl; - auto ar = a.rhs.get().hash_value(); - auto br = b.rhs.get().hash_value(); - return ar < br; + // Compare the operands themselves: hash-only ordering treats colliding + // relations as duplicates, which drops them from relation::set. + if (a.lhs.get() != b.lhs.get()) + return a.lhs.get() < b.lhs.get(); + if (a.rhs.get() != b.rhs.get()) + return a.rhs.get() < b.rhs.get(); + return false; } }; diff --git a/include/numsim_cas/core/symbol_base.h b/include/numsim_cas/core/symbol_base.h index 52143adb..e0d3aa2b 100644 --- a/include/numsim_cas/core/symbol_base.h +++ b/include/numsim_cas/core/symbol_base.h @@ -55,7 +55,10 @@ template class symbol_base : public BaseExpr { template bool operator<(symbol_base const &lhs, symbol_base const &rhs) { - return lhs.hash_value() < rhs.hash_value(); + if (lhs.hash_value() != rhs.hash_value()) + return lhs.hash_value() < rhs.hash_value(); + // The hash covers only the name, so colliding names need a real tiebreak. + return lhs.name() < rhs.name(); } template @@ -67,7 +70,7 @@ bool operator>(symbol_base const &lhs, template bool operator==(symbol_base const &lhs, symbol_base const &rhs) { - return lhs.hash_value() == rhs.hash_value(); + return lhs.hash_value() == rhs.hash_value() && lhs.name() == rhs.name(); } template diff --git a/src/numsim_cas/core/expression.cpp b/src/numsim_cas/core/expression.cpp index 417f40d0..176440fb 100644 --- a/src/numsim_cas/core/expression.cpp +++ b/src/numsim_cas/core/expression.cpp @@ -1,5 +1,7 @@ #include +#include + namespace numsim::cas { expression::hash_type const &expression::hash_value() const { @@ -21,6 +23,11 @@ bool expression::operator==(expression const &rhs) const noexcept { if (hash_value() != rhs.hash_value()) return false; + // id() indexes the node's own domain type list, so nodes from different + // domains share ids; the downcast in equals_same_type needs the exact type. + if (typeid(*this) != typeid(rhs)) + return false; + // same type => do the real compare return equals_same_type(rhs); } @@ -34,6 +41,10 @@ bool expression::operator<(expression const &rhs) const noexcept { return hash_value() < rhs.hash_value(); if (id() != rhs.id()) return id() < rhs.id(); + // Cross-domain ties need an order before the same-type downcast. before() + // is arbitrary but consistent within a run, which is all a key needs. + if (typeid(*this) != typeid(rhs)) + return typeid(*this).before(typeid(rhs)); return less_than_same_type(rhs); } diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 48b30406..1d3465e4 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -949,6 +949,35 @@ TEST(TensorConstHashInvariant, EXPECT_EQ(m.get().hash_value(), A.get().hash_value()); } +// Node ids are per-domain indices and a symbol's hash covers only its name, +// so a scalar and a tensor named alike tie on both and used to reach the +// same-type downcast. +TEST(SymbolIdentity, SameNameAcrossDomainsIsDistinct) { + auto [xs] = make_scalar_variable("x"); + auto [xt] = + make_tensor_variable(std::tuple{"x", std::size_t{3}, std::size_t{2}}); + expression const &a = xs.get(); + expression const &b = xt.get(); + + EXPECT_FALSE(a == b); + EXPECT_TRUE(a != b); + EXPECT_TRUE((a < b) != (b < a)) << "cross-domain order must be total"; +} + +// evaluator_base keys one map by expression_holder, so symbols +// from different domains share it: binding both must keep both values. +TEST(SymbolIdentity, EvaluatorKeepsBothDomainBindings) { + auto [xs] = make_scalar_variable("x"); + auto [xt] = + make_tensor_variable(std::tuple{"x", std::size_t{3}, std::size_t{2}}); + + scalar_evaluator ev; + ev.set(xs, 2.0); + ev.set(xt, 3.0); + + EXPECT_DOUBLE_EQ(ev.apply(xs), 2.0); +} + // #93 — a tensor_mul's space() must survive copy reconstruction // (tensor_add did this; mul dropped it). TEST(CoreBugFix, TensorMulCopyPreservesSpaceAnnotation) { diff --git a/tests/ScalarAssumptionTest.h b/tests/ScalarAssumptionTest.h index df0872c3..b5d0db27 100644 --- a/tests/ScalarAssumptionTest.h +++ b/tests/ScalarAssumptionTest.h @@ -911,4 +911,20 @@ TEST_F(AssumptionFixture, Step6_ScalarAssumeUniformGuardSampling) { numsim::cas::invalid_assumption_error); } +// relation::set orders by its operands; relations differing only in an +// operand must stay distinct members. +TEST_F(AssumptionFixture, RelationSetKeepsDistinctOperands) { + using rel_t = numsim::cas::relation::relation; + numsim::cas::relation::set + set; + set.insert(rel_t{x, y, numsim::cas::relation::equal{}}); + set.insert(rel_t{x, z, numsim::cas::relation::equal{}}); + set.insert(rel_t{x, y, numsim::cas::relation::less{}}); + set.insert(rel_t{x, y, numsim::cas::relation::equal{}}); + + EXPECT_EQ(set.size(), 3u); +} + #endif // SCALARASSUMPTIONTEST_H From 1a8616f1eef086f0f6fa2829b08a004282db966b Mon Sep 17 00:00:00 2001 From: petlenz Date: Wed, 16 Sep 2026 23:06:14 +0200 Subject: [PATCH 2/2] Review fixes on #348: stale comment, both-bindings assertion, cast assert - Drop the relation ordering comment that still described hash ordering. - EvaluatorKeepsBothDomainBindings now asserts both entries coexist under evaluator_base's keying, not just that the scalar value survived. - tensor_scalar_mul::like_term_of asserts the downcast like n_ary_tree does. Signed-off-by: petlenz --- include/numsim_cas/core/assumptions.h | 1 - .../tensor/operators/scalar/tensor_scalar_mul.h | 2 ++ tests/CoreBugFixTest.h | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/numsim_cas/core/assumptions.h b/include/numsim_cas/core/assumptions.h index ebe38ba3..a48f7204 100644 --- a/include/numsim_cas/core/assumptions.h +++ b/include/numsim_cas/core/assumptions.h @@ -199,7 +199,6 @@ template struct relation { RHS rhs; kind k; - // order by (kind_index, lhs_hash, rhs_hash) friend bool operator<(relation const &a, relation const &b) { auto ai = a.k.index(), bi = b.k.index(); if (ai != bi) diff --git a/include/numsim_cas/tensor/operators/scalar/tensor_scalar_mul.h b/include/numsim_cas/tensor/operators/scalar/tensor_scalar_mul.h index 7ff6724b..3af8c995 100644 --- a/include/numsim_cas/tensor/operators/scalar/tensor_scalar_mul.h +++ b/include/numsim_cas/tensor/operators/scalar/tensor_scalar_mul.h @@ -1,6 +1,7 @@ #ifndef TENSOR_SCALAR_MUL_H #define TENSOR_SCALAR_MUL_H +#include #include #include #include @@ -45,6 +46,7 @@ class tensor_scalar_mul final if (this->hash_value() != rhs.hash_value()) return false; if (rhs.id() == this->id()) { + assert(dynamic_cast(&rhs) != nullptr); auto const &r = static_cast(rhs); return this->expr_rhs() == r.expr_rhs(); } diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 1d3465e4..da308f7d 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -976,6 +976,20 @@ TEST(SymbolIdentity, EvaluatorKeepsBothDomainBindings) { ev.set(xt, 3.0); EXPECT_DOUBLE_EQ(ev.apply(xs), 2.0); + + // Same keying as evaluator_base, so both entries must coexist. + std::map, double> keys; + keys[expression_holder( + std::static_pointer_cast(xs.data()))] = 2.0; + keys[expression_holder( + std::static_pointer_cast(xt.data()))] = 3.0; + EXPECT_EQ(keys.size(), 2u); + EXPECT_DOUBLE_EQ(keys.at(expression_holder( + std::static_pointer_cast(xs.data()))), + 2.0); + EXPECT_DOUBLE_EQ(keys.at(expression_holder( + std::static_pointer_cast(xt.data()))), + 3.0); } // #93 — a tensor_mul's space() must survive copy reconstruction