From 6166d81ad98e950978424a7528f12341bf3207e0 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 22:55:21 +0200 Subject: [PATCH 01/24] Fix #349: overflow-safe scalar_number arithmetic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit int64 arithmetic in scalar_number wrapped silently (UB): pow(10,30) folded to 5076944270305263616 (10^30 mod 2^64), rat_add/rat_sub cross products were signed-overflow UB at ~2^40 magnitudes (UBSan-confirmed), rat_div could construct an invariant-violating 1/0 rational, and normalize_rational/negation hit UB at INT64_MIN. All int64 paths are now overflow-checked (builtin overflow intrinsics on GCC/Clang, manual range checks on MSVC) and demote to double when the exact value does not fit — value stays correct, exactness is lost only at the extremes. rat_div routes zero denominators to +-inf double like the int/int path, and INT64_MIN operands demote instead of negating. pow needs no change: it composes operator*, so the checked multiply fixes the wraparound. Exact behavior in range is unchanged and lock-in tested (1/3 + 1/6 stays the exact rational 1/2). Signed-off-by: petlenz --- src/numsim_cas/core/scalar_number.cpp | 142 +++++++++++++++++++++----- tests/CoreBugFixTest.h | 47 +++++++++ 2 files changed, 165 insertions(+), 24 deletions(-) diff --git a/src/numsim_cas/core/scalar_number.cpp b/src/numsim_cas/core/scalar_number.cpp index 7720c937..481415c6 100644 --- a/src/numsim_cas/core/scalar_number.cpp +++ b/src/numsim_cas/core/scalar_number.cpp @@ -15,6 +15,11 @@ static scalar_number::variant_t normalize_rational(std::int64_t num, // Division by zero — fall back to double for inf/nan return static_cast(num) / 0.0; } + // INT64_MIN cannot be negated/abs'd — demote to double (#349) + if (num == std::numeric_limits::min() || + den == std::numeric_limits::min()) { + return static_cast(num) / static_cast(den); + } if (num == 0) { return std::int64_t{0}; } @@ -116,36 +121,101 @@ scalar_number::variant_t promote_binary(scalar_number::variant_t const &a, a, b); } -// Rational arithmetic helpers -rational_t rat_add(rational_t a, rational_t b) { +// Overflow-checked int64 arithmetic (#349). MSVC has no +// __builtin_*_overflow, hence the manual fallbacks. +inline bool add_overflows(std::int64_t a, std::int64_t b, std::int64_t &r) { +#if defined(__GNUC__) || defined(__clang__) + return __builtin_add_overflow(a, b, &r); +#else + constexpr auto mx = std::numeric_limits::max(); + constexpr auto mn = std::numeric_limits::min(); + if ((b > 0 && a > mx - b) || (b < 0 && a < mn - b)) + return true; + r = a + b; + return false; +#endif +} + +inline bool sub_overflows(std::int64_t a, std::int64_t b, std::int64_t &r) { +#if defined(__GNUC__) || defined(__clang__) + return __builtin_sub_overflow(a, b, &r); +#else + constexpr auto mx = std::numeric_limits::max(); + constexpr auto mn = std::numeric_limits::min(); + if ((b < 0 && a > mx + b) || (b > 0 && a < mn + b)) + return true; + r = a - b; + return false; +#endif +} + +inline bool mul_overflows(std::int64_t a, std::int64_t b, std::int64_t &r) { +#if defined(__GNUC__) || defined(__clang__) + return __builtin_mul_overflow(a, b, &r); +#else + constexpr auto mx = std::numeric_limits::max(); + constexpr auto mn = std::numeric_limits::min(); + if (a == 0 || b == 0) { + r = 0; + return false; + } + if (a == -1 && b == mn) + return true; + if (b == -1 && a == mn) + return true; + if (a > 0 ? (b > 0 ? a > mx / b : b < mn / a) + : (b > 0 ? a < mn / b : a < mx / b)) + return true; + r = a * b; + return false; +#endif +} + +inline double rat_to_double(rational_t const &r) { + return static_cast(r.num) / static_cast(r.den); +} + +// Rational arithmetic helpers. Overflowing intermediates demote to double +// instead of wrapping (UB) — value stays correct, exactness is lost (#349). +scalar_number::variant_t rat_add(rational_t a, rational_t b) { // a.num/a.den + b.num/b.den = (a.num*b.den + b.num*a.den) / (a.den*b.den) - auto num = a.num * b.den + b.num * a.den; - auto den = a.den * b.den; - auto g = std::gcd(std::abs(num), std::abs(den)); - return {num / g, den / g}; + std::int64_t t1, t2, num, den; + if (mul_overflows(a.num, b.den, t1) || mul_overflows(b.num, a.den, t2) || + add_overflows(t1, t2, num) || mul_overflows(a.den, b.den, den)) { + return rat_to_double(a) + rat_to_double(b); + } + return normalize_rational(num, den); } -rational_t rat_sub(rational_t a, rational_t b) { - auto num = a.num * b.den - b.num * a.den; - auto den = a.den * b.den; - auto g = std::gcd(std::abs(num), std::abs(den)); - return {num / g, den / g}; +scalar_number::variant_t rat_sub(rational_t a, rational_t b) { + std::int64_t t1, t2, num, den; + if (mul_overflows(a.num, b.den, t1) || mul_overflows(b.num, a.den, t2) || + sub_overflows(t1, t2, num) || mul_overflows(a.den, b.den, den)) { + return rat_to_double(a) - rat_to_double(b); + } + return normalize_rational(num, den); } -rational_t rat_mul(rational_t a, rational_t b) { - // Cross-cancel before multiplying to avoid overflow +scalar_number::variant_t rat_mul(rational_t a, rational_t b) { + // Cross-cancel before multiplying to keep intermediates small auto g1 = std::gcd(std::abs(a.num), std::abs(b.den)); auto g2 = std::gcd(std::abs(b.num), std::abs(a.den)); - auto num = (a.num / g1) * (b.num / g2); - auto den = (a.den / g2) * (b.den / g1); - if (den < 0) { - num = -num; - den = -den; + std::int64_t num, den; + if (mul_overflows(a.num / g1, b.num / g2, num) || + mul_overflows(a.den / g2, b.den / g1, den)) { + return rat_to_double(a) * rat_to_double(b); } - return {num, den}; + return normalize_rational(num, den); } -rational_t rat_div(rational_t a, rational_t b) { +scalar_number::variant_t rat_div(rational_t a, rational_t b) { + if (b.num == 0) { + // a / 0 → ±inf double, matching the int/int path (#349) + return rat_to_double(a) / 0.0; + } + if (b.num == std::numeric_limits::min()) { + return rat_to_double(a) / rat_to_double(b); + } return rat_mul(a, {b.den, b.num}); } @@ -157,7 +227,13 @@ scalar_number operator+(scalar_number const &a, scalar_number const &b) { return scalar_number(promote_binary(a.v_, b.v_, [](auto x, auto y) { using T = std::decay_t; if constexpr (is_rat_v) { - return scalar_number::variant_t{rat_add(x, y)}; + return rat_add(x, y); + } else if constexpr (std::is_same_v) { + std::int64_t r; + if (add_overflows(x, y, r)) + return scalar_number::variant_t{static_cast(x) + + static_cast(y)}; + return scalar_number::variant_t{r}; } else { return scalar_number::variant_t{x + y}; } @@ -168,7 +244,13 @@ scalar_number operator-(scalar_number const &a, scalar_number const &b) { return scalar_number(promote_binary(a.v_, b.v_, [](auto x, auto y) { using T = std::decay_t; if constexpr (is_rat_v) { - return scalar_number::variant_t{rat_sub(x, y)}; + return rat_sub(x, y); + } else if constexpr (std::is_same_v) { + std::int64_t r; + if (sub_overflows(x, y, r)) + return scalar_number::variant_t{static_cast(x) - + static_cast(y)}; + return scalar_number::variant_t{r}; } else { return scalar_number::variant_t{x - y}; } @@ -179,7 +261,13 @@ scalar_number operator*(scalar_number const &a, scalar_number const &b) { return scalar_number(promote_binary(a.v_, b.v_, [](auto x, auto y) { using T = std::decay_t; if constexpr (is_rat_v) { - return scalar_number::variant_t{rat_mul(x, y)}; + return rat_mul(x, y); + } else if constexpr (std::is_same_v) { + std::int64_t r; + if (mul_overflows(x, y, r)) + return scalar_number::variant_t{static_cast(x) * + static_cast(y)}; + return scalar_number::variant_t{r}; } else { return scalar_number::variant_t{x * y}; } @@ -190,7 +278,7 @@ scalar_number operator/(scalar_number const &a, scalar_number const &b) { return scalar_number(promote_binary(a.v_, b.v_, [](auto x, auto y) { using T = std::decay_t; if constexpr (is_rat_v) { - return scalar_number::variant_t{rat_div(x, y)}; + return rat_div(x, y); } else if constexpr (std::is_same_v) { // int / int → rational (exact) return normalize_rational(x, y); @@ -205,7 +293,13 @@ scalar_number operator-(scalar_number const &a) { [](auto const &x) -> scalar_number::variant_t { using T = std::decay_t; if constexpr (is_rat_v) { + if (x.num == std::numeric_limits::min()) + return -rat_to_double(x); return rational_t{-x.num, x.den}; + } else if constexpr (std::is_same_v) { + if (x == std::numeric_limits::min()) + return -static_cast(x); + return -x; } else { return -x; } diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index d3bfcc6d..87a18556 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1693,6 +1693,53 @@ TEST(RoundTwoReview, MulPowEraseProducesCanonicalRemainder) { EXPECT_EQ(to_string(sin(r) - sin(y)), "0"); // no stale hash either } +// #349 — scalar_number int64 arithmetic must demote to double instead of +// wrapping (UB / silent corruption). +TEST(ScalarNumberOverflow, PowDoesNotWrap) { + auto p = pow(make_scalar_constant(10), make_scalar_constant(30)); + scalar_evaluator ev; + EXPECT_NEAR(ev.apply(p), 1e30, 1e16); // was 5076944270305263616 (mod 2^64) +} + +TEST(ScalarNumberOverflow, RationalAddLargeMagnitudes) { + const auto big = std::int64_t{1} << 40; + auto a = scalar_number(rational_t{big + 1, big}); + auto b = scalar_number(rational_t{big + 3, big + 2}); + auto s = a + b; // cross-products overflow int64; must not be UB + double val = std::visit( + [](auto const &v) -> double { + using T = std::decay_t; + if constexpr (std::is_same_v) + return v; + else if constexpr (std::is_same_v) + return static_cast(v); + else if constexpr (std::is_same_v) + return static_cast(v.num) / static_cast(v.den); + else + return 0.0; + }, + s.raw()); + EXPECT_NEAR(val, 2.0, 1e-9); +} + +TEST(ScalarNumberOverflow, RationalDivByZeroIsInf) { + auto q = scalar_number(1, 2) / scalar_number(std::int64_t{0}); + auto const *d = std::get_if(&q.raw()); + ASSERT_NE(d, nullptr); // not a stored 1/0 rational + EXPECT_TRUE(std::isinf(*d)); + EXPECT_GT(*d, 0.0); +} + +TEST(ScalarNumberOverflow, ExactArithmeticUnchanged) { + auto a = scalar_number(1, 3) + scalar_number(1, 6); // = 1/2 exact + auto const *r = std::get_if(&a.raw()); + ASSERT_NE(r, nullptr); + EXPECT_EQ(r->num, 1); + EXPECT_EQ(r->den, 2); + auto b = scalar_number(std::int64_t{2}) * scalar_number(std::int64_t{3}); + EXPECT_EQ(b, scalar_number(std::int64_t{6})); +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From fee2bfcc4c8466c3a8f5cfe9f7bda77b597b1d90 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:19:02 +0200 Subject: [PATCH 02/24] Review fix on #349: guard INT64_MIN in the rational cross-cancel to_rational promotes int64 operands without normalization, so scalar_number(INT64_MIN) * scalar_number(1,2) reached std::abs(INT64_MIN) in rat_mul's gcd cross-cancel (UBSan-confirmed abort) - the exact class #349 eliminates elsewhere. rat_mul (and rat_div through it) now demotes to double when any component is INT64_MIN, matching the existing normalize_rational guard. Regression test covers mul both ways and div. Signed-off-by: petlenz --- src/numsim_cas/core/scalar_number.cpp | 6 ++++++ tests/CoreBugFixTest.h | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/numsim_cas/core/scalar_number.cpp b/src/numsim_cas/core/scalar_number.cpp index 481415c6..043b0754 100644 --- a/src/numsim_cas/core/scalar_number.cpp +++ b/src/numsim_cas/core/scalar_number.cpp @@ -197,6 +197,12 @@ scalar_number::variant_t rat_sub(rational_t a, rational_t b) { } scalar_number::variant_t rat_mul(rational_t a, rational_t b) { + // INT64_MIN cannot be abs'd for the gcd cross-cancel; it can arrive via + // int->rational promotion, which skips normalization (review on #349) + constexpr auto mn = std::numeric_limits::min(); + if (a.num == mn || b.num == mn || a.den == mn || b.den == mn) { + return rat_to_double(a) * rat_to_double(b); + } // Cross-cancel before multiplying to keep intermediates small auto g1 = std::gcd(std::abs(a.num), std::abs(b.den)); auto g2 = std::gcd(std::abs(b.num), std::abs(a.den)); diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 87a18556..79262e5c 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1676,6 +1676,7 @@ TEST(PowDistributeGuard, FractionalSameExponentDoesNotMerge) { // integer exponent still merges EXPECT_EQ(to_string(pow(x, 2.0) * pow(y, 2.0)), "pow(x*y,2)"); } +<<<<<<< HEAD // Round-2 review on #345: pow-split canonical form and the mul producer. TEST(RoundTwoReview, PowSplitCollapsesSingleChildMul) { @@ -1692,6 +1693,8 @@ TEST(RoundTwoReview, MulPowEraseProducesCanonicalRemainder) { EXPECT_TRUE(is_same(r)); EXPECT_EQ(to_string(sin(r) - sin(y)), "0"); // no stale hash either } +======= +>>>>>>> 88df476 (Review fix on #349: guard INT64_MIN in the rational cross-cancel) // #349 — scalar_number int64 arithmetic must demote to double instead of // wrapping (UB / silent corruption). @@ -1740,6 +1743,20 @@ TEST(ScalarNumberOverflow, ExactArithmeticUnchanged) { EXPECT_EQ(b, scalar_number(std::int64_t{6})); } +// Review on #349: INT64_MIN reaches the rational cross-cancel via the +// int->rational promotion, which skips normalization. +TEST(ScalarNumberOverflow, Int64MinTimesRational) { + constexpr auto mn = std::numeric_limits::min(); + auto p = scalar_number(mn) * scalar_number(1, 2); // was std::abs(mn) UB + auto q = scalar_number(1, 2) * scalar_number(mn); + auto d = scalar_number(mn) / scalar_number(1, 2); + auto const *pd = std::get_if(&p.raw()); + ASSERT_NE(pd, nullptr); + EXPECT_NEAR(*pd, static_cast(mn) / 2.0, 1e3); + EXPECT_TRUE(std::get_if(&q.raw()) != nullptr); + EXPECT_TRUE(std::get_if(&d.raw()) != nullptr); +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From 33a9689662fbc1100b32be6f99bc8d72502ba928 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 15:35:58 +0200 Subject: [PATCH 03/24] Round-2 review fix on #349: UB-free |n| in pow; repair a rebase seam pow's repeated-squaring negated the exponent with -n before the uint64 cast - signed-negation UB for INT64_MIN (round-2 review; UBSan abort on pow(1/2, INT64_MIN)). |n| is now computed as 0u - unsigned(n). Also repairs conflict markers committed during the stack rebase. Signed-off-by: petlenz --- src/numsim_cas/core/scalar_number.cpp | 7 +++++-- tests/CoreBugFixTest.h | 3 --- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/numsim_cas/core/scalar_number.cpp b/src/numsim_cas/core/scalar_number.cpp index 043b0754..5352515c 100644 --- a/src/numsim_cas/core/scalar_number.cpp +++ b/src/numsim_cas/core/scalar_number.cpp @@ -507,8 +507,11 @@ std::optional pow(scalar_number const &base, // Compute |n| via repeated squaring bool negative = n < 0; - std::uint64_t abs_n = - negative ? static_cast(-n) : static_cast(n); + // 0u - unsigned(n) computes |n| without the signed negation that is UB + // for INT64_MIN (round-2 review) + std::uint64_t abs_n = negative + ? std::uint64_t{0} - static_cast(n) + : static_cast(n); scalar_number result{1}; scalar_number b = base; diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 79262e5c..d98e4b61 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1676,7 +1676,6 @@ TEST(PowDistributeGuard, FractionalSameExponentDoesNotMerge) { // integer exponent still merges EXPECT_EQ(to_string(pow(x, 2.0) * pow(y, 2.0)), "pow(x*y,2)"); } -<<<<<<< HEAD // Round-2 review on #345: pow-split canonical form and the mul producer. TEST(RoundTwoReview, PowSplitCollapsesSingleChildMul) { @@ -1693,8 +1692,6 @@ TEST(RoundTwoReview, MulPowEraseProducesCanonicalRemainder) { EXPECT_TRUE(is_same(r)); EXPECT_EQ(to_string(sin(r) - sin(y)), "0"); // no stale hash either } -======= ->>>>>>> 88df476 (Review fix on #349: guard INT64_MIN in the rational cross-cancel) // #349 — scalar_number int64 arithmetic must demote to double instead of // wrapping (UB / silent corruption). From 98f05270a0761971a50073c2304d87605e4368d7 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:04:46 +0200 Subject: [PATCH 04/24] Fix #361: hash doubles by bit pattern, normalize numerically equal constants The generic hash_combine did static_cast(value): formal UB for any negative double (every negative constant hashes on first comparison) and gross truncation - all fractions in (0,1) collided with 0.0. Doubles now hash via std::bit_cast (with -0.0 normalized), and scalar_number gets a value-normalizing hash_combine overload so numerically equal constants keep hashing equal across variant alternatives (int64 2 == double 2.0 == rational 2/1 - whole doubles hash as their integer). scalar_constant::update_hash_value routes through it; without the normalization, expressions built from int and double spellings of the same constant stop canceling (caught by ScalarDifferentiationAudit). Signed-off-by: petlenz --- include/numsim_cas/core/hash_functions.h | 14 +++++++++++++ include/numsim_cas/core/scalar_number.h | 19 +++++++++++++++++ include/numsim_cas/scalar/scalar_constant.h | 3 +-- tests/CoreBugFixTest.h | 23 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/include/numsim_cas/core/hash_functions.h b/include/numsim_cas/core/hash_functions.h index d3d96043..5393d45a 100644 --- a/include/numsim_cas/core/hash_functions.h +++ b/include/numsim_cas/core/hash_functions.h @@ -1,7 +1,9 @@ #ifndef HASH_FUNCTIONS_H #define HASH_FUNCTIONS_H +#include #include +#include #include #include @@ -15,6 +17,18 @@ inline void hash_combine(std::size_t &seed, const T &value) { static_cast(0x9e3779b9) + (seed << 6) + (seed >> 2); } +// Doubles hash by bit pattern: the generic static_cast is UB for negative +// values and collapses all fractions in (0,1) onto 0 (#361). +inline void hash_combine(std::size_t &seed, double value) { + if (value == 0.0) + value = 0.0; // normalize -0.0 + hash_combine(seed, std::bit_cast(value)); +} + +inline void hash_combine(std::size_t &seed, float value) { + hash_combine(seed, static_cast(value)); +} + inline void hash_combine(std::size_t &seed, const std::string &value) { for (const auto &c : value) { hash_combine(seed, c); diff --git a/include/numsim_cas/core/scalar_number.h b/include/numsim_cas/core/scalar_number.h index 44df03d4..b3c838a7 100644 --- a/include/numsim_cas/core/scalar_number.h +++ b/include/numsim_cas/core/scalar_number.h @@ -106,6 +106,25 @@ class scalar_number { variant_t v_; }; +// Value-normalizing hash: numerically equal constants must hash equal +// across variant alternatives (int64 2 vs double 2.0 vs rational 2/1), +// while doubles otherwise hash by bit pattern (#361). +inline void hash_combine(std::size_t &seed, scalar_number const &value) { + std::visit( + [&](auto const &x) { + using T = std::decay_t; + if constexpr (std::is_same_v) { + if (x == static_cast(static_cast(x)) && + x >= -9.2e18 && x <= 9.2e18) { + hash_combine(seed, static_cast(x)); + return; + } + } + hash_combine(seed, x); + }, + value.raw()); +} + } // namespace numsim::cas #endif // SCALAR_NUMBER_H diff --git a/include/numsim_cas/scalar/scalar_constant.h b/include/numsim_cas/scalar/scalar_constant.h index e8d08ab6..67d297d9 100644 --- a/include/numsim_cas/scalar/scalar_constant.h +++ b/include/numsim_cas/scalar/scalar_constant.h @@ -61,8 +61,7 @@ class scalar_constant final : public scalar_node_base_t { void update_hash_value() const noexcept override { this->m_hash_value = 0; hash_combine(this->m_hash_value, this->id()); - std::visit([&](auto const &x) { hash_combine(this->m_hash_value, x); }, - m_value.raw()); + hash_combine(this->m_hash_value, m_value); } private: diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index d98e4b61..f29338e3 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1754,6 +1754,29 @@ TEST(ScalarNumberOverflow, Int64MinTimesRational) { EXPECT_TRUE(std::get_if(&d.raw()) != nullptr); } +// #361 — hash_combine(double) hashed via static_cast: UB for +// negatives, and every fraction in (0,1) collided with 0. +TEST(HashCombineDouble, BitPatternNoTruncation) { + std::size_t a = 0, b = 0, c = 0, d = 0, e = 0; + hash_combine(a, 0.5); + hash_combine(b, 0.9); + EXPECT_NE(a, b); + hash_combine(c, -2.5); // UB-free under -fsanitize=float-cast-overflow + hash_combine(d, 0.0); + hash_combine(e, -0.0); + EXPECT_EQ(d, e); // ±0 normalize together +} + +TEST(HashCombineDouble, NumericallyEqualConstantsHashEqual) { + auto ci = make_scalar_constant(2); + auto cd = make_expression(2.0); + EXPECT_EQ(ci.get().hash_value(), cd.get().hash_value()); + EXPECT_EQ(to_string(ci * cd), "4"); // folding across alternatives intact + // fractional constants distinct + auto h1 = make_expression(0.5); + auto h2 = make_expression(0.9); + EXPECT_NE(h1.get().hash_value(), h2.get().hash_value());} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From dddc4d25b321b1fdf0725096a74ed578246e9bac Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:21:37 +0200 Subject: [PATCH 05/24] Review fix on #361: range-guard before the int64 cast in the hash normalizer The value-normalizing hash cast the double to int64 in the FIRST conjunct, before the range checks - UB for NaN, inf, and |x| >= 2^63 (UBSan float-cast-overflow abort at scalar_number.h:117, the very class this PR removes elsewhere). NaN/inf now short-circuit via the range comparisons (false for NaN) before any cast. Regression test hashes 1e300/NaN/inf constants. Signed-off-by: petlenz --- include/numsim_cas/core/scalar_number.h | 6 ++++-- tests/CoreBugFixTest.h | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/include/numsim_cas/core/scalar_number.h b/include/numsim_cas/core/scalar_number.h index b3c838a7..a8b4e130 100644 --- a/include/numsim_cas/core/scalar_number.h +++ b/include/numsim_cas/core/scalar_number.h @@ -114,8 +114,10 @@ inline void hash_combine(std::size_t &seed, scalar_number const &value) { [&](auto const &x) { using T = std::decay_t; if constexpr (std::is_same_v) { - if (x == static_cast(static_cast(x)) && - x >= -9.2e18 && x <= 9.2e18) { + // guard BEFORE casting: the int64 cast is UB for NaN/inf and + // |x| >= 2^63 (review on #361) + if (x >= -9.2e18 && x <= 9.2e18 && + x == static_cast(static_cast(x))) { hash_combine(seed, static_cast(x)); return; } diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index f29338e3..b54df4f3 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1775,7 +1775,23 @@ TEST(HashCombineDouble, NumericallyEqualConstantsHashEqual) { // fractional constants distinct auto h1 = make_expression(0.5); auto h2 = make_expression(0.9); - EXPECT_NE(h1.get().hash_value(), h2.get().hash_value());} + EXPECT_NE(h1.get().hash_value(), h2.get().hash_value()); +} + +// Review on #361: the int64 cast in the value-normalizing hash ran before +// its range guard - UB for NaN, inf, and huge doubles. +TEST(HashCombineDouble, HugeAndNonFiniteConstantsHashSafely) { + auto big = make_expression(1e300); + auto nan = make_expression( + std::numeric_limits::quiet_NaN()); + auto inf = + make_expression(std::numeric_limits::infinity()); + // must be UB-free under -fsanitize=float-cast-overflow (CI leg, #356) + (void)big.get().hash_value(); + (void)nan.get().hash_value(); + (void)inf.get().hash_value(); + EXPECT_NE(big.get().hash_value(), inf.get().hash_value()); +} } // namespace numsim::cas From 920a482b8bf22a22e775db726a8861c2cfe5a6b9 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:25:02 +0200 Subject: [PATCH 06/24] Fix #351: rank-4 identity_tensor is major-symmetric, not MinorMajor I4_ijkl = delta_ik*delta_jl has major symmetry only: swapping the pairs (ij)<->(kl) gives delta_ki*delta_lj (equal), but swapping i<->j gives delta_jk*delta_il (a different tensor - the minor-symmetric identity is P_sym). The MinorMajor tag propagated through negation and scalar-mul and routed inv() evaluation through the symmetric 6x6 Voigt path, so inv(-I4) evaluated to -0.25 at component (0,1,0,1) instead of -1; is_symmetric/is_minor_major also misreported, mis-selecting the D4 symmetrizer in the rank-4 inv-diff kernels (cf. #283, #299). space_for_rank now tags rank 4 as Major. The three tests pinning the MinorMajor annotation are updated (Rank4IdentityIsMajorOnly plus the move/copy preservation pair); a numeric lock-in evaluates inv(-I4) through the Major path. Signed-off-by: petlenz --- include/numsim_cas/tensor/identity_tensor.h | 12 ++++++------ tests/CoreBugFixTest.h | 11 +++++++++++ tests/TensorAlgebraAssumeTest.h | 16 +++++++++------- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/include/numsim_cas/tensor/identity_tensor.h b/include/numsim_cas/tensor/identity_tensor.h index 5ba7d7b5..7e748823 100644 --- a/include/numsim_cas/tensor/identity_tensor.h +++ b/include/numsim_cas/tensor/identity_tensor.h @@ -154,16 +154,16 @@ class identity_tensor final : public tensor_node_base_t { private: // Closed-form structural classification by rank. Rank-2 is Kronecker δ_ij - // (Symmetric). Rank-4 minor identity is δ_ik·δ_jl (MinorMajor — fully - // symmetric at rank-4). Higher ranks return nullopt; the variant has no - // general "all-pairs-minor" alternative and higher-rank identity is - // rarely queried. Open decision tracked in - // docs/sympy-assumption-redesign.md. + // (Symmetric). Rank-4 identity δ_ik·δ_jl has MAJOR symmetry only: + // swapping (ij)<->(kl) gives δ_ki·δ_lj = δ_ik·δ_jl, but swapping i<->j + // gives δ_jk·δ_il ≠ δ_ik·δ_jl (the minor-symmetric identity is P_sym, + // a different tensor). The former MinorMajor tag routed inv() through + // the symmetric Voigt path, evaluating inv(-I4) wrongly (#351). static std::optional space_for_rank(std::size_t rank) noexcept { if (rank == 2) return tensor_space{Symmetric{}, AnyTraceTag{}}; if (rank == 4) - return tensor_space{MinorMajor{}, AnyTraceTag{}}; + return tensor_space{Major{}, AnyTraceTag{}}; return std::nullopt; } }; diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index b54df4f3..91f7c036 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1793,6 +1793,17 @@ TEST(HashCombineDouble, HugeAndNonFiniteConstantsHashSafely) { EXPECT_NE(big.get().hash_value(), inf.get().hash_value()); } +// #351 — rank-4 identity is major-symmetric only; the MinorMajor tag routed +// inv() through the symmetric Voigt path, evaluating inv(-I4) to -0.25 at +// component (0,1,0,1) instead of -1 (the inverse of -I4 is -I4). +TEST(Rank4IdentityTag, InvOfNegatedIdentity) { + auto I4 = make_expression(std::size_t{3}, std::size_t{4}); + tensor_evaluator ev; + auto r = ev.apply(inv(-I4)); + // (0,1,0,1) flattens to ((0*3+1)*3+0)*3+1 = 10 + EXPECT_NEAR(r->raw_data()[10], -1.0, 1e-12); + EXPECT_NEAR(r->raw_data()[0], -1.0, 1e-12); // (0,0,0,0)} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H diff --git a/tests/TensorAlgebraAssumeTest.h b/tests/TensorAlgebraAssumeTest.h index 5851d48a..8a0ef7d7 100644 --- a/tests/TensorAlgebraAssumeTest.h +++ b/tests/TensorAlgebraAssumeTest.h @@ -1214,11 +1214,13 @@ TEST(TensorAlgebraIdentityAssumptions, Rank2IsNotSkew) { EXPECT_FALSE(is_skew(I)); } -TEST(TensorAlgebraIdentityAssumptions, Rank4MinorIdentityIsMinorMajor) { - // I_{ijkl} = δ_ik · δ_jl. Has minor symmetry (swap (i,j) or (k,l)) AND - // major symmetry (swap (ij)↔(kl)). +TEST(TensorAlgebraIdentityAssumptions, Rank4IdentityIsMajorOnly) { + // I_{ijkl} = δ_ik · δ_jl has MAJOR symmetry only (#351): swapping i↔j + // gives δ_jk·δ_il ≠ δ_ik·δ_jl. The MinorMajor tag routed inv() through + // the symmetric Voigt path and evaluated inv(-I4) wrongly. auto I = make_expression(std::size_t{3}, std::size_t{4}); - EXPECT_TRUE(is_minor_major(I)); + EXPECT_TRUE(is_major(I)); + EXPECT_FALSE(is_minor_major(I)); } TEST(TensorAlgebraIdentityAssumptions, Rank4MinorIdentityIsSymmetric) { @@ -1334,13 +1336,13 @@ TEST(TensorAlgebraIdentityAssumptions, CopyPreservesAnnotationRank2) { TEST(TensorAlgebraIdentityAssumptions, MovePreservesAnnotationRank4) { // Rank-4 move ctor coverage (cpp-pro F7 gap): the rank-4 branch - // produces MinorMajor, distinct from rank-2 Symmetric. Both must + // produces Major (#351), distinct from rank-2 Symmetric. Both must // survive move construction. Pass-1 review on #258: also assert PD // survives (orthogonal is rank-2-only by design). identity_tensor src{std::size_t{3}, std::size_t{4}}; identity_tensor moved{std::move(src)}; ASSERT_TRUE(moved.space().has_value()); - EXPECT_TRUE(std::holds_alternative(moved.space()->perm)); + EXPECT_TRUE(std::holds_alternative(moved.space()->perm)); EXPECT_TRUE(moved.tensor_algebra_assumptions().contains(positive_definite{})); EXPECT_TRUE( moved.tensor_algebra_assumptions().contains(positive_semidefinite{})); @@ -1378,7 +1380,7 @@ TEST(TensorAlgebraIdentityAssumptions, CopyPreservesAnnotationRank4) { identity_tensor src{std::size_t{3}, std::size_t{4}}; identity_tensor copy{src}; ASSERT_TRUE(copy.space().has_value()); - EXPECT_TRUE(std::holds_alternative(copy.space()->perm)); + EXPECT_TRUE(std::holds_alternative(copy.space()->perm)); EXPECT_TRUE(copy.tensor_algebra_assumptions().contains(positive_definite{})); EXPECT_TRUE( copy.tensor_algebra_assumptions().contains(positive_semidefinite{})); From 1b8fa1a906a485bf0c8959a3af1df49d01d2ba62 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:27:38 +0200 Subject: [PATCH 07/24] Review fix on #351: comment rationale + repair a rebase-seam brace Updates the set_symmetric no-op comment (the MinorMajor early-return it cited is gone; the rank gate is the actual reason at rank 4) and repairs a conflict-resolution artifact that glued a test's closing brace into a trailing comment, leaving the test namespace open. Signed-off-by: petlenz --- include/numsim_cas/tensor/identity_tensor.h | 7 +++---- tests/CoreBugFixTest.h | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/numsim_cas/tensor/identity_tensor.h b/include/numsim_cas/tensor/identity_tensor.h index 7e748823..fa5cd7c6 100644 --- a/include/numsim_cas/tensor/identity_tensor.h +++ b/include/numsim_cas/tensor/identity_tensor.h @@ -106,10 +106,9 @@ class identity_tensor final : public tensor_node_base_t { a.insert(positive_semidefinite{}); // PD ⇒ PSD, mirrors // assume_positive_definite() // PD ⇒ symmetric, via the same helper assume_positive_definite() - // uses. Mechanically a no-op today because space_for_rank above - // already wrote a qualifying space tag at both supported ranks - // (rank-2 Symmetric ⇒ ProjKind::Sym early-return; rank-4 - // MinorMajor ⇒ holds_alternative early-return). The call exists + // uses. Mechanically a no-op today: rank-2 already carries + // Symmetric, and the helper's rank()!=2 gate skips rank 4 (whose + // tag is Major since #351). The call exists // so that any future drift in detail::set_symmetric_unless_more_specific // (e.g. it grows a sibling-field write that we'd otherwise miss) // tracks automatically. Pairs with the helper in tensor_assume.h. diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 91f7c036..1f0281be 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1802,7 +1802,8 @@ TEST(Rank4IdentityTag, InvOfNegatedIdentity) { auto r = ev.apply(inv(-I4)); // (0,1,0,1) flattens to ((0*3+1)*3+0)*3+1 = 10 EXPECT_NEAR(r->raw_data()[10], -1.0, 1e-12); - EXPECT_NEAR(r->raw_data()[0], -1.0, 1e-12); // (0,0,0,0)} + EXPECT_NEAR(r->raw_data()[0], -1.0, 1e-12); // (0,0,0,0) +} } // namespace numsim::cas From aa9f972374ba3c8821d7c3956fec827604e4f3bc Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:31:31 +0200 Subject: [PATCH 08/24] Fix #352: substitution no longer inherits the source's space annotation tensor_rebuild_visitor::apply restored the source expression's space() onto any rebuilt result lacking one (the #93 fix for variadic-ctor reconstruction dropping post-construction annotations). Sound for pure rebuilds, wrong when a subclass swapped children: substitute( trans(A)-A, trans(A), C) returned C-A tagged Skew, after which sym() folded it to zero and skew() returned it unchanged - every consumer of the tag (projector guards, trans, inv, diff kernels) trusted it. The restore is now gated on structural equality (m_result == expr, honest since #339), with one deliberate exception: projector contractions (skew(X), sym(X), ...) restore across argument substitution because their space is derived from the projector, not the argument - substituting inside skew(A) keeps Skew (the original deterministic #93 reproducer stays green). Structure-derived tags (trans(X)-X) are re-derived by construction when the substituted form still qualifies. Signed-off-by: petlenz --- .../tensor/visitors/tensor_rebuild_visitor.h | 20 +++++++++++++--- tests/CoreBugFixTest.h | 24 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h index 2c83183f..00287f17 100644 --- a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h +++ b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -25,10 +26,14 @@ class tensor_rebuild_visitor : public tensor_visitor_const_t { expr.get().accept(*this); // #93 — reconstructions below build fresh nodes (variadic ctors) // that drop post-construction space(). Restore from source, but not - // over a self-computed space (e.g. tensor_add's child-join). + // over a self-computed space (e.g. tensor_add's child-join), and only + // for structurally unchanged rebuilds: a subclass that swapped + // children (substitution) must not inherit the source's space (#352). if (m_result.is_valid() && !m_result.get().space()) { - if (auto const &sp = expr.get().space()) - m_result.data()->set_space(*sp); + if (auto const &sp = expr.get().space()) { + if (m_result == expr || same_projector_contraction(expr, m_result)) + m_result.data()->set_space(*sp); + } } return std::move(m_result); } @@ -41,6 +46,15 @@ class tensor_rebuild_visitor : public tensor_visitor_const_t { virtual t2s_holder_t apply_t2s(t2s_holder_t const &expr) { return expr; } + // skew(X)/sym(X)/...: the space comes from the projector, not the + // argument, so it survives child substitution (#93/#352). + static bool same_projector_contraction(tensor_holder_t const &a, + tensor_holder_t const &b) { + auto ia = as_projector_contraction(a); + auto ib = as_projector_contraction(b); + return ia && ib && *ia->proj == *ib->proj; + } + // Leaf nodes: return as-is void operator()(tensor const &) override { m_result = m_current; } void operator()(tensor_zero const &) override { m_result = m_current; } diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 1f0281be..0d5f54f4 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1805,6 +1805,30 @@ TEST(Rank4IdentityTag, InvOfNegatedIdentity) { EXPECT_NEAR(r->raw_data()[0], -1.0, 1e-12); // (0,0,0,0) } +// #352 — substitution must not inherit the source's space annotation when +// the structure changed: substitute(trans(A)-A, trans(A), C) is C-A, +// which is NOT skew for general C. +TEST(SubstitutionSpace, ChangedStructureDropsStaleTag) { + auto [A, C] = + make_tensor_variable(std::tuple{"A", std::size_t{3}, std::size_t{2}}, + std::tuple{"C", std::size_t{3}, std::size_t{2}}); + auto f = substitute(trans(A) - A, trans(A), C); // C - A, general + EXPECT_NE(to_string(sym(f)), "0{2}"); + EXPECT_NE(to_string(skew(f)), to_string(f)); +} + +TEST(SubstitutionSpace, OperatorDerivedTagSurvives) { + auto [A, B] = + make_tensor_variable(std::tuple{"A", std::size_t{3}, std::size_t{2}}, + std::tuple{"B", std::size_t{3}, std::size_t{2}}); + // skew(X) is skew for any X: substituting the argument keeps the tag + auto s = substitute(skew(A), A, B); + EXPECT_TRUE(is_skew_annotated(s)); + // structurally skew trans(C)-C is re-derived by construction + auto h = substitute(trans(A) - A, A, B); + EXPECT_EQ(to_string(sym(h)), "0{2}"); +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From 669905429d1a9b879b0c9b369b1e856694fe0147 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:29:55 +0200 Subject: [PATCH 09/24] Review fix on #352: shape guard in same_projector_contraction The projector-restore exception compared only the projectors; a substitution that changed the argument's rank or dim could restore a rank-2-style tag onto a shape-inconsistent result. Rank and dim must match before any restore. Signed-off-by: petlenz --- include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h index 00287f17..93384144 100644 --- a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h +++ b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h @@ -50,6 +50,9 @@ class tensor_rebuild_visitor : public tensor_visitor_const_t { // argument, so it survives child substitution (#93/#352). static bool same_projector_contraction(tensor_holder_t const &a, tensor_holder_t const &b) { + if (a.get().rank() != b.get().rank() || a.get().dim() != b.get().dim()) { + return false; // review on #352: never restore across a shape change + } auto ia = as_projector_contraction(a); auto ib = as_projector_contraction(b); return ia && ib && *ia->proj == *ib->proj; From 50e99f4051f4b3a4eb0754bbe1d417d8565b35fa Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 15:38:33 +0200 Subject: [PATCH 10/24] Round-2 review fix on #352: compare projector-argument shapes The node-level dim guard was inert: inner_product_wrapper reports the projector's dim, so substitute(sym(A_dim3), A, E_dim2) passed the guard, restored the stale Symmetric tag onto a shape-broken node, and evaluation ran the projector at dim 3 over a dim-2 buffer (ASan-confirmed heap-buffer-overflow read). The guard now compares the actual contraction arguments' rank and dim. Signed-off-by: petlenz --- .../tensor/visitors/tensor_rebuild_visitor.h | 12 ++++++++---- tests/CoreBugFixTest.h | 9 +++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h index 93384144..aa9651bb 100644 --- a/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h +++ b/include/numsim_cas/tensor/visitors/tensor_rebuild_visitor.h @@ -50,12 +50,16 @@ class tensor_rebuild_visitor : public tensor_visitor_const_t { // argument, so it survives child substitution (#93/#352). static bool same_projector_contraction(tensor_holder_t const &a, tensor_holder_t const &b) { - if (a.get().rank() != b.get().rank() || a.get().dim() != b.get().dim()) { - return false; // review on #352: never restore across a shape change - } auto ia = as_projector_contraction(a); auto ib = as_projector_contraction(b); - return ia && ib && *ia->proj == *ib->proj; + if (!(ia && ib && *ia->proj == *ib->proj)) { + return false; + } + // round-2 review: the wrapper's dim() comes from the projector LHS, so + // node-level shape checks are inert - compare the actual arguments + // (a dim-changing substitution reached a heap overflow at evaluation) + return ia->argument.get().rank() == ib->argument.get().rank() && + ia->argument.get().dim() == ib->argument.get().dim(); } // Leaf nodes: return as-is diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 0d5f54f4..9bf66d4c 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1829,6 +1829,15 @@ TEST(SubstitutionSpace, OperatorDerivedTagSurvives) { EXPECT_EQ(to_string(sym(h)), "0{2}"); } +// Round-2 review on #352: the shape guard must compare the projector +// ARGUMENTS (the wrapper's dim() reports the projector's). +TEST(RoundTwoReview, DimChangingSubstitutionDropsTag) { + auto [A] = make_tensor_variable(std::tuple{"A", std::size_t{3}, 2}); + auto [E] = make_tensor_variable(std::tuple{"E", std::size_t{2}, 2}); + auto s = substitute(sym(A), A, E); // dim 3 projector : dim 2 argument + EXPECT_FALSE(is_symmetric(s)); // stale tag restored -> heap overflow at eval +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From a703520ea967e5314ae5e0e4a1a9d78d45270c51 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 16:15:51 +0200 Subject: [PATCH 11/24] Round-3 review fix on #352: guard the projector short-circuit's operand shape Round 3 proved the round-2 commit closed only the tag restoration: the heap overflow it cited lived in eval_projector_unary, which drives the unary wrapper with the PROJECTOR's dim and never compares the operand's - substitute(sym(A_dim3), A, E_dim2) still over-read the dim-2 buffer at evaluation (UBSan vptr abort). The short-circuit now throws evaluation_error on operand dim/rank mismatch, and the regression test evaluates the reproducer instead of only checking the tag. Signed-off-by: petlenz --- include/numsim_cas/tensor/visitors/tensor_evaluator.h | 8 ++++++++ tests/CoreBugFixTest.h | 8 +++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/include/numsim_cas/tensor/visitors/tensor_evaluator.h b/include/numsim_cas/tensor/visitors/tensor_evaluator.h index f06a1d93..c92004fe 100644 --- a/include/numsim_cas/tensor/visitors/tensor_evaluator.h +++ b/include/numsim_cas/tensor/visitors/tensor_evaluator.h @@ -385,6 +385,14 @@ class tensor_evaluator final : public tensor_visitor_const_t { template void eval_projector_unary(inner_product_wrapper const &visitable) { auto rhs_data = apply(visitable.expr_rhs()); + // round-3 review: the wrapper's dim() is the projector's; a + // dim-changing substitution produced an operand whose buffer this + // shortcut then over-read (ASan heap-buffer-overflow) + if (rhs_data->dim() != visitable.dim() || + rhs_data->rank() != visitable.rank()) { + throw evaluation_error( + "projector contraction: operand dim/rank mismatch"); + } m_result = make_tensor_data(visitable.dim(), visitable.rank()); tensor_data_unary_wrapper op(*m_result, *rhs_data); op.evaluate(visitable.dim(), visitable.rank()); diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 9bf66d4c..b3f76a95 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1835,7 +1835,13 @@ TEST(RoundTwoReview, DimChangingSubstitutionDropsTag) { auto [A] = make_tensor_variable(std::tuple{"A", std::size_t{3}, 2}); auto [E] = make_tensor_variable(std::tuple{"E", std::size_t{2}, 2}); auto s = substitute(sym(A), A, E); // dim 3 projector : dim 2 argument - EXPECT_FALSE(is_symmetric(s)); // stale tag restored -> heap overflow at eval + EXPECT_FALSE(is_symmetric(s)); + // round-3 review: the overflow lived in the projector short-circuit, + // not the tag - evaluation must throw, not over-read the buffer + tensor_evaluator ev; + auto data = std::make_shared>(); + ev.set(E, data); + EXPECT_THROW((void)ev.apply(s), evaluation_error); } } // namespace numsim::cas From b88f5054dca23572ecefa9ff0ff9d229e750d661 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:34:27 +0200 Subject: [PATCH 12/24] Fix #354: t2s constant_mul keeps a symbolic scalar_wrapper factor constant_mul::dispatch(tensor_to_scalar_mul) handled a numeric LHS by merging it into the coefficient, but a non-numeric scalar_wrapper LHS fell through the same path: the factor was never inserted and the coefficient was reset to the wrapped default, so wrapper(x) * (trace(A)*det(A)) evaluated as if x were 1. The promoted route (plain x * (f*g)) goes through mul_base and was correct, which is why tests missed it - the bug fires whenever the wrapper is the visitor's LHS. Non-numeric wrappers are now inserted as factors via push_or_combine (merging with an existing wrapper child through the unwrap-multiply- rewrap path). Signed-off-by: petlenz --- .../tensor_to_scalar_simplifier_mul.cpp | 13 ++++++++++--- tests/TensorToScalarExpressionTest.h | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp b/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp index 41f4543c..2006d109 100644 --- a/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp +++ b/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp @@ -12,6 +12,9 @@ namespace simplifier { // --- constant_mul --- using t2s_traits = domain_traits; +static void push_or_combine(tensor_to_scalar_mul &mul, + mul_base::expr_holder_t const &child); + constant_mul::constant_mul(expr_holder_t lhs, expr_holder_t rhs) : base(std::move(lhs), std::move(rhs)), lhs_val{t2s_traits::try_numeric(base::m_lhs)} {} @@ -36,10 +39,14 @@ constant_mul::dispatch(tensor_to_scalar_mul const &rhs) { } auto mul_expr{make_expression(rhs)}; auto &mul{mul_expr.template get()}; - auto coeff{get_coefficient(mul, 1)}; - if (lhs_val) { - coeff = coeff * *lhs_val; + if (!lhs_val) { + // symbolic scalar_wrapper: keep it as a factor instead of silently + // resetting the coefficient (#354) + push_or_combine(mul, base::m_lhs); + return mul_expr; } + auto coeff{get_coefficient(mul, 1)}; + coeff = coeff * *lhs_val; mul.set_coeff(t2s_traits::make_constant(coeff)); return mul_expr; } diff --git a/tests/TensorToScalarExpressionTest.h b/tests/TensorToScalarExpressionTest.h index 967ad73e..40fe68b3 100644 --- a/tests/TensorToScalarExpressionTest.h +++ b/tests/TensorToScalarExpressionTest.h @@ -921,4 +921,20 @@ TYPED_TEST(TensorToScalarExpressionTest, EXPECT_EQ(d.get().dim(), X.get().dim()); } +// #354 — a symbolic scalar_wrapper multiplied into an existing t2s mul must +// survive as a factor (it was silently dropped and the coefficient reset). +TYPED_TEST(TensorToScalarExpressionTest, + SymbolicWrapperFactorSurvivesMulMerge) { + auto &X = this->X; + auto &x = this->x; + using numsim::cas::det; + using numsim::cas::trace; + auto f = trace(X) * det(X); // tensor_to_scalar_mul + auto w = numsim::cas::make_expression< + numsim::cas::tensor_to_scalar_scalar_wrapper>(x); + auto e = w * f; // wrapper-first: hits constant_mul::dispatch(mul) + auto const s = numsim::cas::to_string(e); + EXPECT_NE(s.find("x"), std::string::npos) << s; +} + #endif // TENSORTOSCALAREXPRESSIONTEST_H From 7a945d5c02c8c183ff831ea7ddf0b8e7d6305abb Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:33:12 +0200 Subject: [PATCH 13/24] Review fix on #354: push_or_combine loops on chained collisions A single-shot find/combine threw 'duplicate child insertion' when the combined factor collided with another existing child (w(x)*w(x) -> w(x^2) meeting a stored w(x^2)) - the same pattern merge_or_insert_mul already loops on. Pre-existing hole; #354's new route made it easier to reach. Regression test builds the chained collision explicitly. Signed-off-by: petlenz --- .../tensor_to_scalar_simplifier_mul.cpp | 17 +++++++++++------ tests/TensorToScalarExpressionTest.h | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp b/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp index 2006d109..a8daa1fa 100644 --- a/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp +++ b/src/numsim_cas/tensor_to_scalar/simplifier/tensor_to_scalar_simplifier_mul.cpp @@ -101,14 +101,19 @@ static void push_or_combine(tensor_to_scalar_mul &mul, mul_base::expr_holder_t const &child) { if (try_fold_numeric_pow(mul, child)) return; - auto pos = mul.symbol_map().find(child); - if (pos != mul.symbol_map().end()) { - auto combined = pos->second * child; + // Loop: the combined factor can collide with yet another existing child + // (w(x)*w(x) -> w(x^2) meeting a stored w(x^2)); a single-shot combine + // then threw on duplicate insertion (review on #354/#346). + auto entry = child; + while (true) { + auto pos = mul.symbol_map().find(entry); + if (pos == mul.symbol_map().end()) + break; + auto combined = pos->second * entry; mul.symbol_map().erase(pos); - mul.push_back(std::move(combined)); - return; + entry = std::move(combined); } - mul.push_back(child); + mul.push_back(std::move(entry)); } // --- mul_base --- diff --git a/tests/TensorToScalarExpressionTest.h b/tests/TensorToScalarExpressionTest.h index 40fe68b3..5898260c 100644 --- a/tests/TensorToScalarExpressionTest.h +++ b/tests/TensorToScalarExpressionTest.h @@ -937,4 +937,22 @@ TYPED_TEST(TensorToScalarExpressionTest, EXPECT_NE(s.find("x"), std::string::npos) << s; } +// Review on #354: chained wrapper collisions must fold, not throw. +TYPED_TEST(TensorToScalarExpressionTest, ChainedWrapperCollisionFolds) { + auto &X = this->X; + auto &x = this->x; + using numsim::cas::trace; + auto w = [](auto e) { + return numsim::cas::make_expression< + numsim::cas::tensor_to_scalar_scalar_wrapper>(e); + }; + // build a mul already holding w(x) and w(x*x); multiplying by w(x) makes + // w(x)*w(x) -> w(x^2) collide with the stored w(x*x) + auto m = (trace(X) * w(x)) * w(x * x); + numsim::cas::expression_holder e; + EXPECT_NO_THROW(e = w(x) * m); + auto const s = numsim::cas::to_string(e); + EXPECT_NE(s.find("pow("), std::string::npos) << s; +} + #endif // TENSORTOSCALAREXPRESSIONTEST_H From 400e9fce1fb758d5c0ac8e6c3e5b4acfcae4da04 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:38:31 +0200 Subject: [PATCH 14/24] Fix #353: t2s contraction evaluation honors the index sequences The tensor_inner_product_to_scalar evaluator always computed the plain tmech::dcontract(l, r): dot_product(A,{1,2},B,{2,1}) (= A_ij B_ji) silently evaluated as A : B, and rank-1 full contractions threw 'requires rank 2' even though the node is a legal dot product (which also broke evaluating derivatives of dot() on rank-1 arguments). tensor_data_dcontract_wrapper now receives both sequences: matching rank-2 sequences contract plain ({2,1}/{2,1} sums the same pairs), mismatched ones contract against the transpose, and rank-1 uses tmech::dot. Rank>2 general contraction stays a clear not-implemented error (tracked by the #383 evaluation-ceilings epic) instead of a silently wrong value. The identity half of this node (hash/== ignoring the sequences) landed in #399. Signed-off-by: petlenz --- .../data/tensor_data_to_scalar_wrapper.h | 29 ++++++++++++++--- .../visitors/tensor_to_scalar_evaluator.h | 3 +- tests/TensorToScalarEvaluatorTest.h | 31 +++++++++++++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h b/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h index 5577c0b0..cde18583 100644 --- a/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h +++ b/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h @@ -4,6 +4,7 @@ #include "spectral_decomposition_cache.h" #include "tensor_data.h" #include +#include #include #include @@ -58,18 +59,36 @@ class tensor_data_dcontract_wrapper final : public tensor_data_eval_up_unary, ValueType> { public: + // #353 — the contraction sequences are part of the node semantics: + // {1,2}/{2,1} is A_ij*B_ji (A : B^T), not A : B. tensor_data_dcontract_wrapper(tensor_data_base const &lhs, - tensor_data_base const &rhs) - : m_lhs(lhs), m_rhs(rhs) {} + tensor_data_base const &rhs, + sequence const &lhs_indices, + sequence const &rhs_indices) + : m_lhs(lhs), m_rhs(rhs), m_lhs_indices(lhs_indices), + m_rhs_indices(rhs_indices) {} template ValueType evaluate_imp() { if constexpr (Rank == 2) { using Tensor = tensor_data; auto const &l = static_cast(m_lhs).data(); auto const &r = static_cast(m_rhs).data(); - return static_cast(tmech::dcontract(l, r)); + const bool straight{m_lhs_indices == m_rhs_indices}; + if (straight) { + // {1,2}/{1,2} = A_ij B_ij; {2,1}/{2,1} = A_ji B_ji = same sum + return static_cast(tmech::dcontract(l, r)); + } + // {1,2}/{2,1} (either orientation) = A_ij B_ji + return static_cast(tmech::dcontract(l, tmech::trans(r))); + } else if constexpr (Rank == 1) { + using Tensor = tensor_data; + auto const &l = static_cast(m_lhs).data(); + auto const &r = static_cast(m_rhs).data(); + return static_cast(tmech::dot(l, r)); } else { - throw evaluation_error("tensor_data_dcontract_wrapper: requires rank 2"); + throw evaluation_error( + "tensor_data_dcontract_wrapper: rank > 2 contraction not " + "implemented (#353 follow-up in #383)"); } } @@ -86,6 +105,8 @@ class tensor_data_dcontract_wrapper final private: tensor_data_base const &m_lhs; tensor_data_base const &m_rhs; + sequence m_lhs_indices; + sequence m_rhs_indices; }; // ─── Eigenvalue wrapper: dispatches runtime (dim,rank), computes the diff --git a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_evaluator.h b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_evaluator.h index b16d4430..8b03a5b2 100644 --- a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_evaluator.h +++ b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_evaluator.h @@ -160,7 +160,8 @@ class tensor_to_scalar_evaluator final auto rhs_data = m_tensor_eval.apply(v.expr_rhs()); const auto dim = lhs_data->dim(); const auto rank = lhs_data->rank(); - tensor_data_dcontract_wrapper op(*lhs_data, *rhs_data); + tensor_data_dcontract_wrapper op( + *lhs_data, *rhs_data, v.indices_lhs(), v.indices_rhs()); m_result = op.evaluate(dim, rank); } diff --git a/tests/TensorToScalarEvaluatorTest.h b/tests/TensorToScalarEvaluatorTest.h index 532089f3..11f3530f 100644 --- a/tests/TensorToScalarEvaluatorTest.h +++ b/tests/TensorToScalarEvaluatorTest.h @@ -482,4 +482,35 @@ TEST(T2sEval, SpectralCacheInterleavedTensors) { } // namespace numsim::cas +// #353 — the evaluator must honor the stored contraction sequences. +TEST(T2sContractionSequences, TransposedRank2AndRank1Dot) { + using namespace numsim::cas; + auto [A, B] = + make_tensor_variable(std::tuple{"A", std::size_t{3}, std::size_t{2}}, + std::tuple{"B", std::size_t{3}, std::size_t{2}}); + tensor_to_scalar_evaluator ev; + const std::array av{1, 2, 3, 4, 5, 6, 7, 8, 9}; + const std::array bv{2, 3, 1, 0.5, -1, 4, 2, 2, -3}; + ev.set(A, make_test_data<3, 2>({1, 2, 3, 4, 5, 6, 7, 8, 9})); + ev.set(B, make_test_data<3, 2>({2, 3, 1, 0.5, -1, 4, 2, 2, -3})); + double sum_plain = 0.0, sum_trans = 0.0; + for (std::size_t i = 0; i < 3; ++i) + for (std::size_t j = 0; j < 3; ++j) { + sum_plain += av[i * 3 + j] * bv[i * 3 + j]; + sum_trans += av[i * 3 + j] * bv[j * 3 + i]; + } + auto plain = dot_product(A, sequence{1, 2}, B, sequence{1, 2}); + auto transp = dot_product(A, sequence{1, 2}, B, sequence{2, 1}); + EXPECT_NEAR(ev.apply(plain), sum_plain, 1e-12); + EXPECT_NEAR(ev.apply(transp), sum_trans, 1e-12); // was sum_plain (#353) + + auto [u, v] = + make_tensor_variable(std::tuple{"u", std::size_t{3}, std::size_t{1}}, + std::tuple{"v", std::size_t{3}, std::size_t{1}}); + ev.set(u, make_test_data<3, 1>({1, 2, 3})); + ev.set(v, make_test_data<3, 1>({4, 3, 2})); + auto d = dot_product(u, sequence{1}, v, sequence{1}); + EXPECT_NEAR(ev.apply(d), 4.0 + 6.0 + 6.0, 1e-12); // threw pre-#353 +} + #endif // TENSORTOSCALAREVALUATORTEST_H From 3eb5bf33fef8d58af8497f96cd67c9694132c6bc Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:35:29 +0200 Subject: [PATCH 15/24] Review fix on #353: reject mixed-shape operands before the data cast The dispatch derives Dim/Rank from the LHS; a mixed-rank node (constructible through the weak || precondition in dot_product, #360) reached a wrong-type static_cast and, with the new rank-1 branch, returned silent garbage where it previously threw. The wrapper now throws evaluation_error on operand rank/dim mismatch and on sequence sizes not covering the rank. Signed-off-by: petlenz --- .../tensor/data/tensor_data_to_scalar_wrapper.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h b/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h index cde18583..e093150e 100644 --- a/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h +++ b/include/numsim_cas/tensor/data/tensor_data_to_scalar_wrapper.h @@ -69,6 +69,17 @@ class tensor_data_dcontract_wrapper final m_rhs_indices(rhs_indices) {} template ValueType evaluate_imp() { + // The dispatch picks Dim/Rank from the LHS; a mixed-rank/dim node + // (constructible through the weak dot_product precondition, #360) + // would type-pun the RHS cast into silent garbage (review on #353). + if (m_lhs.rank() != m_rhs.rank() || m_lhs.dim() != m_rhs.dim()) { + throw evaluation_error( + "tensor_data_dcontract_wrapper: operand rank/dim mismatch"); + } + if (m_lhs_indices.size() != Rank || m_rhs_indices.size() != Rank) { + throw evaluation_error( + "tensor_data_dcontract_wrapper: sequence size != operand rank"); + } if constexpr (Rank == 2) { using Tensor = tensor_data; auto const &l = static_cast(m_lhs).data(); From e68cd2ee0568fd2aee3b64f4b7da90d7b259e37e Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:46:20 +0200 Subject: [PATCH 16/24] Fix #350: tensor_pow contract - rank-2 only, integer exponents, working negative powers tensor_pow was broken beyond positive-integer/rank-2 use, each layer failing differently and silently: the evaluator cast the exponent with static_cast (pow(X, 0.5) truncated to the identity) and looped k < abs(n) with no inversion (pow(X, -1) evaluated to X itself); both diff visitors' product-rule loops never ran for n < 0, silently coercing the tangent to zero; and the factory accepted any rank while hard-coding a rank-2 identity for pow(C4, 0). - Factory: rank-2 gate and integer-constant exponent gate (invalid_expression_error; fractional powers belong to the isotropic function API, #227). - Evaluator: non-integer exponent value throws evaluation_error; n < 0 inverts the accumulated power via tmech::inv. - Both diff visitors: n < 0 throws not_implemented_error pointing at the inv(pow(A, n)) spelling, which differentiates correctly today. Signed-off-by: petlenz --- include/numsim_cas/tensor/tensor_std.h | 15 ++++++++ .../tensor/visitors/tensor_evaluator.h | 15 ++++++++ .../visitors/tensor_differentiation.cpp | 8 ++++ .../tensor_differentiation_wrt_scalar.cpp | 6 +++ tests/CoreBugFixTest.h | 37 +++++++++++++++++++ 5 files changed, 81 insertions(+) diff --git a/include/numsim_cas/tensor/tensor_std.h b/include/numsim_cas/tensor/tensor_std.h index 06ff2dd3..81ec0368 100644 --- a/include/numsim_cas/tensor/tensor_std.h +++ b/include/numsim_cas/tensor/tensor_std.h @@ -39,6 +39,21 @@ namespace numsim::cas { template [[nodiscard]] auto pow(ExprLHS &&expr_lhs, ExprRHS &&expr_rhs) { + // Matrix powers are rank-2 only; the evaluator's repeated contraction + // and the diff kernels are meaningless for other ranks (#350). + if (expr_lhs.get().rank() != 2) { + throw invalid_expression_error( + "pow: tensor operand must be rank 2 (got rank " + + std::to_string(expr_lhs.get().rank()) + ")"); + } + // Non-integer constant exponents have no matrix-power meaning here; + // isotropic tensor functions (#227) cover fractional powers (#350). + if (is_same(expr_rhs)) { + if (!try_int_constant(expr_rhs)) { + throw invalid_expression_error( + "pow: tensor exponent must be an integer constant"); + } + } // pow(0, n) → 0 if (is_same(expr_lhs)) return make_expression(expr_lhs.get().dim(), diff --git a/include/numsim_cas/tensor/visitors/tensor_evaluator.h b/include/numsim_cas/tensor/visitors/tensor_evaluator.h index c92004fe..77ff5144 100644 --- a/include/numsim_cas/tensor/visitors/tensor_evaluator.h +++ b/include/numsim_cas/tensor/visitors/tensor_evaluator.h @@ -1,6 +1,8 @@ #ifndef TENSOR_EVALUATOR_H #define TENSOR_EVALUATOR_H +#include + #include #include #include @@ -257,6 +259,11 @@ class tensor_evaluator final : public tensor_visitor_const_t { void operator()(tensor_pow const &visitable) override { auto base_data = apply(visitable.expr_lhs()); const auto exp_val = m_scalar_eval.apply(visitable.expr_rhs()); + if (exp_val != std::round(exp_val)) { + // silently truncating pow(A, 0.5) to the identity was #350 + throw evaluation_error( + "tensor_pow: exponent must evaluate to an integer"); + } const auto n = static_cast(exp_val); const auto d = visitable.dim(); const auto r = visitable.rank(); @@ -284,6 +291,14 @@ class tensor_evaluator final : public tensor_visitor_const_t { ip.evaluate(d, r, r); accumulated = std::move(temp); } + if (n < 0) { + // A^-n = inv(A^n); without this, pow(X,-1) returned X itself (#350) + auto inverted = make_tensor_data(d, r); + tensor_data_unary_wrapper iv(*inverted, + *accumulated); + iv.evaluate(d, r); + accumulated = std::move(inverted); + } m_result = std::move(accumulated); } diff --git a/src/numsim_cas/tensor/visitors/tensor_differentiation.cpp b/src/numsim_cas/tensor/visitors/tensor_differentiation.cpp index 71cb118b..e0a18661 100644 --- a/src/numsim_cas/tensor/visitors/tensor_differentiation.cpp +++ b/src/numsim_cas/tensor/visitors/tensor_differentiation.cpp @@ -43,6 +43,14 @@ void tensor_differentiation::operator()(tensor_pow const &visitable) { } auto n = static_cast(*int_n); + if (n < 0) { + // the product-rule loop below never runs for n < 0 and the invalid + // sum silently coerced to zero (#350); rewrite via inv() instead + throw not_implemented_error( + "tensor_differentiation: negative tensor power - rewrite as " + "inv(pow(A, n))"); + } + // n == 0: pow(A, 0) = I (constant), derivative is zero. The loop // below would correctly leave sum invalid and apply()'s fallback // would coerce to tensor_zero, but an explicit guard documents the diff --git a/src/numsim_cas/tensor/visitors/tensor_differentiation_wrt_scalar.cpp b/src/numsim_cas/tensor/visitors/tensor_differentiation_wrt_scalar.cpp index a6b3ea5a..8b4557aa 100644 --- a/src/numsim_cas/tensor/visitors/tensor_differentiation_wrt_scalar.cpp +++ b/src/numsim_cas/tensor/visitors/tensor_differentiation_wrt_scalar.cpp @@ -55,6 +55,12 @@ void tensor_differentiation_wrt_scalar::operator()( } auto n = static_cast(*int_n); + if (n < 0) { + throw not_implemented_error( + "tensor_differentiation_wrt_scalar: negative tensor power - " + "rewrite as inv(pow(A, n))"); + } + // Build sum: sum_{r=0}^{n-1} (A^r) * (dA/ds) * (A^{n-1-r}) // For rank-2 A, matrix multiplication is inner_product on the // contraction index. A^r and A^{n-1-r} are also rank-2. diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index b3f76a95..e4aeb2dc 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1844,6 +1844,43 @@ TEST(RoundTwoReview, DimChangingSubstitutionDropsTag) { EXPECT_THROW((void)ev.apply(s), evaluation_error); } +// #350 — tensor_pow contract: rank-2 only, integer exponents, negative +// exponents invert, diff of negative powers no longer silently zero. +TEST(TensorPowContract, RankAndExponentGates) { + auto [C] = + make_tensor_variable(std::tuple{"C", std::size_t{3}, std::size_t{4}}); + auto [X] = + make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); + EXPECT_THROW((void)pow(C, 2), invalid_expression_error); + EXPECT_THROW((void)pow(C, 0), invalid_expression_error); + EXPECT_THROW((void)pow(X, make_expression(0.5)), + invalid_expression_error); + EXPECT_NO_THROW((void)pow(X, 3)); + EXPECT_NO_THROW((void)pow(X, -2)); +} + +TEST(TensorPowContract, NegativeExponentEvaluatesInverse) { + auto [X] = + make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); + tensor_evaluator ev; + auto data = std::make_shared>(); + data->data()(0, 0) = 2.0; + data->data()(1, 1) = 4.0; + data->data()(2, 2) = 5.0; + ev.set(X, data); + auto r1 = ev.apply(pow(X, -1)); + EXPECT_NEAR(r1->raw_data()[0], 0.5, 1e-12); // was 2.0 (#350) + auto r2 = ev.apply(pow(X, -2)); + EXPECT_NEAR(r2->raw_data()[0], 0.25, 1e-12); +} + +TEST(TensorPowContract, DiffOfNegativePowerThrows) { + auto [X] = + make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); + // was a silent 0{4} (#350); inv(pow(X, 2)) is the supported spelling + EXPECT_THROW((void)diff(pow(X, -2), X), not_implemented_error); + EXPECT_NO_THROW((void)diff(inv(pow(X, 2)), X));} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From f5446aeae708b6befc52334fdf864f4f93423bcd Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:38:54 +0200 Subject: [PATCH 17/24] Review fixes on #350: evaluator rank gate, exponent range, wrapped constants - The rank gate was factory-only: tensor_rebuild_visitor recreates tensor_pow ungated, so substitute(pow(X,2), X, C_rank4) evaluated into heap corruption (rank-6 contraction written into a rank-4 buffer). The evaluator now throws on rank != 2. - The exponent round-check gains a range bound so the int cast stays defined for huge/inf exponent values (would abort under the fatal UBSan leg). - The factory's integer gate now also sees negation-wrapped constants (pow(X, neg(0.5)) previously slipped to evaluation). Regression test covers the substitution route and the wrapped exponent. Signed-off-by: petlenz --- include/numsim_cas/tensor/tensor_std.h | 5 ++++- .../tensor/visitors/tensor_evaluator.h | 13 +++++++++--- tests/CoreBugFixTest.h | 21 +++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/include/numsim_cas/tensor/tensor_std.h b/include/numsim_cas/tensor/tensor_std.h index 81ec0368..47cd9d40 100644 --- a/include/numsim_cas/tensor/tensor_std.h +++ b/include/numsim_cas/tensor/tensor_std.h @@ -48,7 +48,10 @@ template } // Non-integer constant exponents have no matrix-power meaning here; // isotropic tensor functions (#227) cover fractional powers (#350). - if (is_same(expr_rhs)) { + if (is_same(expr_rhs) || + (is_same(expr_rhs) && + is_same( + expr_rhs.template get().expr()))) { if (!try_int_constant(expr_rhs)) { throw invalid_expression_error( "pow: tensor exponent must be an integer constant"); diff --git a/include/numsim_cas/tensor/visitors/tensor_evaluator.h b/include/numsim_cas/tensor/visitors/tensor_evaluator.h index 77ff5144..420964df 100644 --- a/include/numsim_cas/tensor/visitors/tensor_evaluator.h +++ b/include/numsim_cas/tensor/visitors/tensor_evaluator.h @@ -257,12 +257,19 @@ class tensor_evaluator final : public tensor_visitor_const_t { // ─── Tensor functions (tmech wrappers) ───────────────────── void operator()(tensor_pow const &visitable) override { + if (visitable.rank() != 2) { + // the repeated single-index contraction is rank-2-only; a rebuilt + // tree can bypass the factory gate (review on #350: substitution + // recreated a rank-4 pow and corrupted the heap) + throw evaluation_error("tensor_pow: rank-2 operand required"); + } auto base_data = apply(visitable.expr_lhs()); const auto exp_val = m_scalar_eval.apply(visitable.expr_rhs()); - if (exp_val != std::round(exp_val)) { - // silently truncating pow(A, 0.5) to the identity was #350 + if (exp_val != std::round(exp_val) || exp_val < -1e9 || exp_val > 1e9) { + // silently truncating pow(A, 0.5) to the identity was #350; the + // range check keeps the int cast below defined for huge values throw evaluation_error( - "tensor_pow: exponent must evaluate to an integer"); + "tensor_pow: exponent must evaluate to a moderate integer"); } const auto n = static_cast(exp_val); const auto d = visitable.dim(); diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index e4aeb2dc..34094a9e 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1881,6 +1881,27 @@ TEST(TensorPowContract, DiffOfNegativePowerThrows) { EXPECT_THROW((void)diff(pow(X, -2), X), not_implemented_error); EXPECT_NO_THROW((void)diff(inv(pow(X, 2)), X));} +// Review on #350: the rank gate must also hold at evaluation (rebuilt +// trees bypass the factory), and the factory gate must see negation- +// wrapped constants. +TEST(TensorPowContract, EvaluatorRankGateAndWrappedExponent) { + auto [X] = + make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); + auto [C] = + make_tensor_variable(std::tuple{"C", std::size_t{3}, std::size_t{4}}); + // substitution recreates the node without the factory gate; evaluation + // must throw instead of corrupting the heap + auto p4 = substitute(pow(X, 2), X, C); + tensor_evaluator ev; + auto data = std::make_shared>(); + ev.set(C, data); + EXPECT_THROW((void)ev.apply(p4), evaluation_error); + // negation-wrapped fractional constants are rejected at the factory + auto half = make_expression(0.5); + EXPECT_THROW((void)pow(X, make_expression(std::move(half))), + invalid_expression_error); +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From 9fa306721e516ca6e08ef62ef1dc0b617dbc32dd Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 15:43:20 +0200 Subject: [PATCH 18/24] Round-2 review fixes on #350: recursive negation unwrap, tighter work bound The factory's integer gate unwrapped only one scalar_negative layer, so pow(X, neg(neg(0.5))) constructed and failed only at evaluation; the gate now strips any negation depth before the literal check, matching try_int_constant. The evaluator's exponent bound tightens from 1e9 to 1e6: the cast was defined at 1e9 but admitted a ~4-minute single-expression evaluation. Signed-off-by: petlenz --- include/numsim_cas/tensor/tensor_std.h | 13 ++++++++----- .../tensor/visitors/tensor_evaluator.h | 2 +- tests/CoreBugFixTest.h | 17 ++++++++++++++++- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/include/numsim_cas/tensor/tensor_std.h b/include/numsim_cas/tensor/tensor_std.h index 47cd9d40..0003baf8 100644 --- a/include/numsim_cas/tensor/tensor_std.h +++ b/include/numsim_cas/tensor/tensor_std.h @@ -48,11 +48,14 @@ template } // Non-integer constant exponents have no matrix-power meaning here; // isotropic tensor functions (#227) cover fractional powers (#350). - if (is_same(expr_rhs) || - (is_same(expr_rhs) && - is_same( - expr_rhs.template get().expr()))) { - if (!try_int_constant(expr_rhs)) { + { + // strip any depth of negation before the literal check, matching + // try_int_constant's own recursion (round-2 review on #350) + expression_holder probe{expr_rhs}; + while (is_same(probe)) { + probe = probe.template get().expr(); + } + if (is_same(probe) && !try_int_constant(expr_rhs)) { throw invalid_expression_error( "pow: tensor exponent must be an integer constant"); } diff --git a/include/numsim_cas/tensor/visitors/tensor_evaluator.h b/include/numsim_cas/tensor/visitors/tensor_evaluator.h index 420964df..860a2c79 100644 --- a/include/numsim_cas/tensor/visitors/tensor_evaluator.h +++ b/include/numsim_cas/tensor/visitors/tensor_evaluator.h @@ -265,7 +265,7 @@ class tensor_evaluator final : public tensor_visitor_const_t { } auto base_data = apply(visitable.expr_lhs()); const auto exp_val = m_scalar_eval.apply(visitable.expr_rhs()); - if (exp_val != std::round(exp_val) || exp_val < -1e9 || exp_val > 1e9) { + if (exp_val != std::round(exp_val) || exp_val < -1e6 || exp_val > 1e6) { // silently truncating pow(A, 0.5) to the identity was #350; the // range check keeps the int cast below defined for huge values throw evaluation_error( diff --git a/tests/CoreBugFixTest.h b/tests/CoreBugFixTest.h index 34094a9e..fbb3c456 100644 --- a/tests/CoreBugFixTest.h +++ b/tests/CoreBugFixTest.h @@ -1879,7 +1879,8 @@ TEST(TensorPowContract, DiffOfNegativePowerThrows) { make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); // was a silent 0{4} (#350); inv(pow(X, 2)) is the supported spelling EXPECT_THROW((void)diff(pow(X, -2), X), not_implemented_error); - EXPECT_NO_THROW((void)diff(inv(pow(X, 2)), X));} + EXPECT_NO_THROW((void)diff(inv(pow(X, 2)), X)); +} // Review on #350: the rank gate must also hold at evaluation (rebuilt // trees bypass the factory), and the factory gate must see negation- @@ -1902,6 +1903,20 @@ TEST(TensorPowContract, EvaluatorRankGateAndWrappedExponent) { invalid_expression_error); } +// Round-2 review on #350: the factory gate strips any negation depth. +TEST(RoundTwoReview, DoubleNegatedFractionalExponentRejected) { + auto [X] = + make_tensor_variable(std::tuple{"X", std::size_t{3}, std::size_t{2}}); + auto inner = + make_expression(make_expression(0.5)); + EXPECT_THROW((void)pow(X, make_expression(std::move(inner))), + invalid_expression_error); + // negated integers still accepted + auto neg2 = + make_expression(make_expression(2)); + EXPECT_NO_THROW((void)pow(X, std::move(neg2))); +} + } // namespace numsim::cas #endif // COREBUGFIXTEST_H From f9fd4dc35722798007f6a80767c666f3500dab82 Mon Sep 17 00:00:00 2001 From: petlenz Date: Fri, 24 Jul 2026 23:48:30 +0200 Subject: [PATCH 19/24] Fix #355: bound parser recursion depth - nested input raises parse_error PEGTL parses by C++ recursion with no depth control: ~20k nested parens, 20k nested calls, or a 20k unary-minus chain overflowed the stack (SIGSEGV) instead of raising parse_error - including on the error path (20k unclosed parens crashed while trying to report the missing parens). The parser is exactly the component fed untrusted text and every other malformed input produces a catchable error. parse() now pre-scans the input and rejects bracket nesting or unary-minus runs deeper than 512 with syntax_error (position at the offending character). Whitespace does not reset a minus run ('- - -x' recurses per minus). The wrappers (parse_scalar/tensor/t2s) route through parse() and inherit the guard. Signed-off-by: petlenz --- src/numsim_cas/parser/parser.cpp | 30 ++++++++++++++++++++++++++++++ tests/ParserTest.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/numsim_cas/parser/parser.cpp b/src/numsim_cas/parser/parser.cpp index bb12005b..3e23bdcc 100644 --- a/src/numsim_cas/parser/parser.cpp +++ b/src/numsim_cas/parser/parser.cpp @@ -40,9 +40,39 @@ syntax_error translate_pegtl_error(pegtl::parse_error const &e, return syntax_error(std::move(msg), byte, source); } +// #355 — PEGTL parses by C++ recursion; deeply nested input overflows the +// stack (SIGSEGV at ~10-20k frames) instead of raising parse_error. A cheap +// pre-scan bounds every recursion driver: bracket nesting and unary-minus +// runs (whitespace does not reset a run: "- - -x" recurses per minus). +void check_nesting_depth(std::string_view source) { + constexpr std::size_t max_depth = 512; + std::size_t depth = 0; + std::size_t minus_run = 0; + for (std::size_t i = 0; i < source.size(); ++i) { + const char c = source[i]; + if (c == '(' || c == '[' || c == '{') { + if (++depth > max_depth) { + throw syntax_error("expression nesting too deep", i, source); + } + } else if (c == ')' || c == ']' || c == '}') { + if (depth > 0) { + --depth; + } + } + if (c == '-') { + if (++minus_run > max_depth) { + throw syntax_error("expression nesting too deep", i, source); + } + } else if (c != ' ' && c != '\t' && c != '\n' && c != '\r') { + minus_run = 0; + } + } +} + } // namespace parsed_expression parse(std::string_view source, symbol_table &syms) { + check_nesting_depth(source); // Make a copy into a std::string-backed input — PEGTL's // memory_input takes ownership of the source view (it doesn't // copy) so callers must keep the string alive across the parse. diff --git a/tests/ParserTest.h b/tests/ParserTest.h index 7872864c..31db6384 100644 --- a/tests/ParserTest.h +++ b/tests/ParserTest.h @@ -2151,4 +2151,32 @@ TEST(ParserFunctions, SpectralConstructionErrorsPropagate) { #endif // NUMSIM_CAS_PARSER_ENABLED +// #355 — deeply nested input must raise parse_error, not overflow the +// C++ stack (SIGSEGV pre-fix at ~10-20k frames, including the unclosed- +// paren error path). +TEST(ParserDepthGuard, DeepNestingRaisesParseError) { + numsim::cas::parser::symbol_table syms; + std::string deep(20000, '('); + deep += "1"; + deep += std::string(20000, ')'); + EXPECT_THROW((void)numsim::cas::parser::parse(deep, syms), + numsim::cas::parser::parse_error); + + std::string unclosed(20000, '('); + unclosed += "1"; + EXPECT_THROW((void)numsim::cas::parser::parse(unclosed, syms), + numsim::cas::parser::parse_error); + + std::string minuses(20000, '-'); + minuses += "1"; + EXPECT_THROW((void)numsim::cas::parser::parse(minuses, syms), + numsim::cas::parser::parse_error); + + // moderate nesting still parses + std::string ok(200, '('); + ok += "1"; + ok += std::string(200, ')'); + EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok, syms)); +} + #endif // PARSERTEST_H From 78e1b29b0fe1adb6264a6669da3ef8c04dd878df Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 08:47:06 +0200 Subject: [PATCH 20/24] Fixup #355: ParserDepthGuard test belongs inside the parser-enabled guard The lock-in test was appended after the NUMSIM_CAS_PARSER_ENABLED block, breaking every build with the parser disabled. Signed-off-by: petlenz --- tests/ParserTest.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ParserTest.h b/tests/ParserTest.h index 31db6384..d8e4af63 100644 --- a/tests/ParserTest.h +++ b/tests/ParserTest.h @@ -2149,8 +2149,6 @@ TEST(ParserFunctions, SpectralConstructionErrorsPropagate) { } // namespace numsim::cas::parser_test -#endif // NUMSIM_CAS_PARSER_ENABLED - // #355 — deeply nested input must raise parse_error, not overflow the // C++ stack (SIGSEGV pre-fix at ~10-20k frames, including the unclosed- // paren error path). @@ -2179,4 +2177,6 @@ TEST(ParserDepthGuard, DeepNestingRaisesParseError) { EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok, syms)); } +#endif // NUMSIM_CAS_PARSER_ENABLED + #endif // PARSERTEST_H From de6fe48a90970718427b71c9570e12cd9f312100 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:41:16 +0200 Subject: [PATCH 21/24] Review fixes on #355: cap caret chains, honor the full space set Two probe-confirmed bypasses of the new depth guard: the ^ chain is right-recursive in the grammar (50k carets still overflowed the stack), and the minus-run reset treated \v/\f as run breakers while PEGTL's space rule accepts them ('-\v' x 50000 crashed). Every ^ now counts toward the cap regardless of position, and the run persists across the full PEGTL space set. Lock-ins cover both bypasses plus a 100-caret happy path. Signed-off-by: petlenz --- src/numsim_cas/parser/parser.cpp | 15 ++++++++++++++- tests/ParserTest.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/numsim_cas/parser/parser.cpp b/src/numsim_cas/parser/parser.cpp index 3e23bdcc..a642bb3f 100644 --- a/src/numsim_cas/parser/parser.cpp +++ b/src/numsim_cas/parser/parser.cpp @@ -46,8 +46,15 @@ syntax_error translate_pegtl_error(pegtl::parse_error const &e, // runs (whitespace does not reset a run: "- - -x" recurses per minus). void check_nesting_depth(std::string_view source) { constexpr std::size_t max_depth = 512; + const auto is_space = [](char c) { + // must cover PEGTL's full space set or a whitespace variant resets + // the run and bypasses the guard (review on #355) + return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\v' || + c == '\f'; + }; std::size_t depth = 0; std::size_t minus_run = 0; + std::size_t caret_count = 0; for (std::size_t i = 0; i < source.size(); ++i) { const char c = source[i]; if (c == '(' || c == '[' || c == '{') { @@ -58,12 +65,18 @@ void check_nesting_depth(std::string_view source) { if (depth > 0) { --depth; } + } else if (c == '^') { + // every ^ contributes one right-recursion level regardless of + // position (review on #355: 50k carets overflowed the stack) + if (++caret_count > max_depth) { + throw syntax_error("expression nesting too deep", i, source); + } } if (c == '-') { if (++minus_run > max_depth) { throw syntax_error("expression nesting too deep", i, source); } - } else if (c != ' ' && c != '\t' && c != '\n' && c != '\r') { + } else if (!is_space(c)) { minus_run = 0; } } diff --git a/tests/ParserTest.h b/tests/ParserTest.h index d8e4af63..c96caa4e 100644 --- a/tests/ParserTest.h +++ b/tests/ParserTest.h @@ -2170,11 +2170,29 @@ TEST(ParserDepthGuard, DeepNestingRaisesParseError) { EXPECT_THROW((void)numsim::cas::parser::parse(minuses, syms), numsim::cas::parser::parse_error); + // review on #355: caret chains and PEGTL-space-separated minus runs + // are recursion drivers too + std::string carets = "1"; + for (int i = 0; i < 20000; ++i) + carets += "^1"; + EXPECT_THROW((void)numsim::cas::parser::parse(carets, syms), + numsim::cas::parser::parse_error); + std::string vminus; + for (int i = 0; i < 20000; ++i) + vminus += "-\v"; + vminus += "1"; + EXPECT_THROW((void)numsim::cas::parser::parse(vminus, syms), + numsim::cas::parser::parse_error); + // moderate nesting still parses std::string ok(200, '('); ok += "1"; ok += std::string(200, ')'); EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok, syms)); + std::string ok2 = "1"; + for (int i = 0; i < 100; ++i) + ok2 += "^1"; + EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok2, syms)); } #endif // NUMSIM_CAS_PARSER_ENABLED From b22b3b5937d221290ea7e9a5745414d58cd734ce Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 14:53:51 +0200 Subject: [PATCH 22/24] Round-2 review fix on #355: caret cap counts chains, not totals The total-caret cap rejected 600 independent shallow powers (x1^2*x2^2*...) even though each ^ opens a fresh one-level chain - probe-confirmed false positive. The run now resets on any chain-breaking operator/bracket, keeping the 20k-chained-carets rejection while accepting arbitrarily many shallow powers. Lock-in covers both. Signed-off-by: petlenz --- src/numsim_cas/parser/parser.cpp | 18 ++++++++++++++---- tests/ParserTest.h | 5 +++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/numsim_cas/parser/parser.cpp b/src/numsim_cas/parser/parser.cpp index a642bb3f..c455cd42 100644 --- a/src/numsim_cas/parser/parser.cpp +++ b/src/numsim_cas/parser/parser.cpp @@ -52,9 +52,16 @@ void check_nesting_depth(std::string_view source) { return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\v' || c == '\f'; }; + const auto breaks_power_chain = [](char c) { + // power_tail recursion accumulates only within one ^-chain; any + // other operator/bracket starts a fresh chain, so independent + // shallow carets (x1^2*x2^2*...) must not count together + return c == '+' || c == '-' || c == '*' || c == '/' || c == '(' || + c == ')' || c == '[' || c == ']' || c == '{' || c == '}' || c == ','; + }; std::size_t depth = 0; std::size_t minus_run = 0; - std::size_t caret_count = 0; + std::size_t caret_run = 0; for (std::size_t i = 0; i < source.size(); ++i) { const char c = source[i]; if (c == '(' || c == '[' || c == '{') { @@ -66,12 +73,15 @@ void check_nesting_depth(std::string_view source) { --depth; } } else if (c == '^') { - // every ^ contributes one right-recursion level regardless of - // position (review on #355: 50k carets overflowed the stack) - if (++caret_count > max_depth) { + // each ^ in a chain adds one right-recursion level (review on + // #355: 50k chained carets overflowed the stack) + if (++caret_run > max_depth) { throw syntax_error("expression nesting too deep", i, source); } } + if (breaks_power_chain(c)) { + caret_run = 0; + } if (c == '-') { if (++minus_run > max_depth) { throw syntax_error("expression nesting too deep", i, source); diff --git a/tests/ParserTest.h b/tests/ParserTest.h index c96caa4e..63fe1eed 100644 --- a/tests/ParserTest.h +++ b/tests/ParserTest.h @@ -2193,6 +2193,11 @@ TEST(ParserDepthGuard, DeepNestingRaisesParseError) { for (int i = 0; i < 100; ++i) ok2 += "^1"; EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok2, syms)); + // independent shallow carets are not one recursion chain (round-2 review) + std::string ok3 = "x0^2"; + for (int i = 1; i < 600; ++i) + ok3 += "*x" + std::to_string(i) + "^2"; + EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok3, syms)); } #endif // NUMSIM_CAS_PARSER_ENABLED From c566b6cebdd3f085a6469816d0c77ec94a3402e2 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 09:02:04 +0200 Subject: [PATCH 23/24] Fix #356: CI can now fail on UBSan findings and clang-tidy warnings Two CI jobs were structurally unable to fail on the defect class they exist to catch: GCC UBSan recovers by default (prints and exits 0, so the sanitizer leg only enforced the ASan half), and clang-tidy exits 0 on warnings with WarningsAsErrors unset. - Sanitizer blocks (library + parser) add -fno-sanitize-recover= undefined. Verified locally: the full suite passes under fatal ASan+UBSan (the real UB this would have caught was fixed in #349 and #361 first). - The clang-tidy workflow adds --warnings-as-errors='*', and the 15-warning baseline is fixed in the same change (no-op std::move on const-ref args and a trivially-copyable variant, missing override on two rebuild-visitor dtors, std::move on a forwarding reference, a cloned constexpr branch merged, a value param made const-ref, and a NOLINT that sat on the wrong line), so the job starts green. Signed-off-by: petlenz --- .github/workflows/clang-tidy-check.yml | 2 +- CMakeLists.txt | 10 ++++---- include/numsim_cas/core/scalar_number.h | 2 +- include/numsim_cas/parser/parse_error.h | 2 +- .../scalar/visitors/scalar_rebuild_visitor.h | 2 +- .../numsim_cas/tensor/wrappers/tensor_inv.h | 4 ++-- .../tensor_to_scalar_rebuild_visitor.h | 2 +- .../visitors/tensor_to_scalar_substitution.h | 5 ++-- src/numsim_cas/parser/parse_error.cpp | 2 +- src/numsim_cas/parser/parser.cpp | 4 ++-- ...r_to_scalar_differentiation_wrt_scalar.cpp | 23 ++++++++----------- 11 files changed, 28 insertions(+), 30 deletions(-) diff --git a/.github/workflows/clang-tidy-check.yml b/.github/workflows/clang-tidy-check.yml index 8dfa5313..911cba7c 100644 --- a/.github/workflows/clang-tidy-check.yml +++ b/.github/workflows/clang-tidy-check.yml @@ -30,4 +30,4 @@ jobs: - name: Run clang-tidy run: | find src -name '*.cpp' -print0 \ - | xargs -0 clang-tidy-18 -p build + | xargs -0 clang-tidy-18 -p build --warnings-as-errors='*' diff --git a/CMakeLists.txt b/CMakeLists.txt index ac0c741e..4020c222 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -194,9 +194,10 @@ endif() # Sanitizers if(NUMSIM_CAS_SANITIZERS) target_compile_options(${PROJECT_NAME} PUBLIC - -fsanitize=address,undefined -fno-omit-frame-pointer) + -fsanitize=address,undefined -fno-sanitize-recover=undefined + -fno-omit-frame-pointer) target_link_options(${PROJECT_NAME} PUBLIC - -fsanitize=address,undefined) + -fsanitize=address,undefined -fno-sanitize-recover=undefined) endif() # Optional convenience for Windows @@ -274,9 +275,10 @@ if(NUMSIM_CAS_BUILD_PARSER) if(NUMSIM_CAS_SANITIZERS) target_compile_options(NumSim_CAS_Parser PUBLIC - -fsanitize=address,undefined -fno-omit-frame-pointer) + -fsanitize=address,undefined -fno-sanitize-recover=undefined + -fno-omit-frame-pointer) target_link_options(NumSim_CAS_Parser PUBLIC - -fsanitize=address,undefined) + -fsanitize=address,undefined -fno-sanitize-recover=undefined) endif() if(WIN32) diff --git a/include/numsim_cas/core/scalar_number.h b/include/numsim_cas/core/scalar_number.h index a8b4e130..a592a1ec 100644 --- a/include/numsim_cas/core/scalar_number.h +++ b/include/numsim_cas/core/scalar_number.h @@ -96,7 +96,7 @@ class scalar_number { scalar_number const &exp); private: - explicit scalar_number(variant_t vv) : v_(std::move(vv)) { + explicit scalar_number(variant_t vv) : v_(vv) { if (auto *r = std::get_if(&v_)) { if (r->den == 1) v_ = r->num; diff --git a/include/numsim_cas/parser/parse_error.h b/include/numsim_cas/parser/parse_error.h index 982a56e4..48b35e06 100644 --- a/include/numsim_cas/parser/parse_error.h +++ b/include/numsim_cas/parser/parse_error.h @@ -37,7 +37,7 @@ class parse_error : public cas_error { public: /// Construct with optional source context. Pass an empty `source` and /// `byte_offset = 0` for errors raised outside the parser. - parse_error(std::string message, std::size_t byte_offset, + parse_error(std::string const &message, std::size_t byte_offset, std::string_view source); /// Byte offset into the source where the error was detected, clamped diff --git a/include/numsim_cas/scalar/visitors/scalar_rebuild_visitor.h b/include/numsim_cas/scalar/visitors/scalar_rebuild_visitor.h index d4700a74..8e01cfc7 100644 --- a/include/numsim_cas/scalar/visitors/scalar_rebuild_visitor.h +++ b/include/numsim_cas/scalar/visitors/scalar_rebuild_visitor.h @@ -14,7 +14,7 @@ class scalar_rebuild_visitor : public scalar_visitor_const_t { public: using expr_holder_t = expression_holder; - virtual ~scalar_rebuild_visitor() = default; + ~scalar_rebuild_visitor() override = default; virtual expr_holder_t apply(expr_holder_t const &expr) { if (expr.is_valid()) { diff --git a/include/numsim_cas/tensor/wrappers/tensor_inv.h b/include/numsim_cas/tensor/wrappers/tensor_inv.h index efaa8634..ba2bc300 100644 --- a/include/numsim_cas/tensor/wrappers/tensor_inv.h +++ b/include/numsim_cas/tensor/wrappers/tensor_inv.h @@ -12,8 +12,8 @@ class tensor_inv final : public unary_op> { using base = unary_op>; template - explicit tensor_inv( - Expr &&_expr) // NOLINT(bugprone-forwarding-reference-overload) + // NOLINTNEXTLINE(bugprone-forwarding-reference-overload) + explicit tensor_inv(Expr &&_expr) : base(std::forward(_expr), _expr.get().dim(), _expr.get().rank()) { // ── Rank gate (#292) ────────────────────────────────────────── // Mirror the inv() factory's rank gate (tensor_functions.h:467) diff --git a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_rebuild_visitor.h b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_rebuild_visitor.h index f977f381..b7d82a7b 100644 --- a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_rebuild_visitor.h +++ b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_rebuild_visitor.h @@ -19,7 +19,7 @@ class tensor_to_scalar_rebuild_visitor using scalar_holder_t = expression_holder; using tensor_holder_t = expression_holder; - virtual ~tensor_to_scalar_rebuild_visitor() = default; + ~tensor_to_scalar_rebuild_visitor() override = default; virtual t2s_holder_t apply(t2s_holder_t const &expr) { if (expr.is_valid()) { diff --git a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_substitution.h b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_substitution.h index 4f617bcf..81d19662 100644 --- a/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_substitution.h +++ b/include/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_substitution.h @@ -45,9 +45,8 @@ class tensor_to_scalar_substitution final } tensor_holder_t apply_tensor(tensor_holder_t const &expr) override { - if constexpr (std::is_same_v) { - return substitute(expr, m_old, m_new); - } else if constexpr (std::is_same_v) { + if constexpr (std::is_same_v || + std::is_same_v) { return substitute(expr, m_old, m_new); } else { return expr; diff --git a/src/numsim_cas/parser/parse_error.cpp b/src/numsim_cas/parser/parse_error.cpp index 4cd06af7..8e352912 100644 --- a/src/numsim_cas/parser/parse_error.cpp +++ b/src/numsim_cas/parser/parse_error.cpp @@ -63,7 +63,7 @@ std::string format_message(std::string_view body, std::string_view source, } // namespace -parse_error::parse_error(std::string message, std::size_t byte_offset, +parse_error::parse_error(std::string const &message, std::size_t byte_offset, std::string_view source) : cas_error(format_message(message, source, byte_offset)) { if (!source.empty()) { diff --git a/src/numsim_cas/parser/parser.cpp b/src/numsim_cas/parser/parser.cpp index 1b9a6193..c3e29329 100644 --- a/src/numsim_cas/parser/parser.cpp +++ b/src/numsim_cas/parser/parser.cpp @@ -38,7 +38,7 @@ syntax_error translate_pegtl_error(pegtl::parse_error const &e, // returns a string_view from message() which can't directly // construct a std::string by '='. std::string msg(e.message()); - return syntax_error(std::move(msg), byte, source); + return syntax_error(msg, byte, source); } // #355 — PEGTL parses by C++ recursion; deeply nested input overflows the @@ -150,7 +150,7 @@ parsed_expression parse(std::string_view source, symbol_table &syms) { "bracket-list literal '[...]' cannot be a top-level expression", source.size(), source); } else { - return std::move(v); + return std::forward(v); } }, std::move(state.values.front())); diff --git a/src/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_differentiation_wrt_scalar.cpp b/src/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_differentiation_wrt_scalar.cpp index 704e7d97..3aef1232 100644 --- a/src/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_differentiation_wrt_scalar.cpp +++ b/src/numsim_cas/tensor_to_scalar/visitors/tensor_to_scalar_differentiation_wrt_scalar.cpp @@ -218,7 +218,7 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( if (!dA.is_valid() || is_same(dA)) { return; } - m_result = trace(std::move(dA)); + m_result = trace(dA); } // dot(A) = A:A. d/ds = 2 * (A : dA/ds). @@ -237,8 +237,8 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( sequence idx_a(rank), idx_b(rank); std::iota(idx_a.begin(), idx_a.end(), std::size_t{0}); std::iota(idx_b.begin(), idx_b.end(), std::size_t{0}); - auto contraction = dot_product(visitable.expr(), std::move(idx_a), - std::move(dA), std::move(idx_b)); + auto contraction = + dot_product(visitable.expr(), std::move(idx_a), dA, std::move(idx_b)); m_result = wrap_scalar(make_expression(2)) * std::move(contraction); } @@ -254,8 +254,8 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( sequence idx_a(rank), idx_b(rank); std::iota(idx_a.begin(), idx_a.end(), std::size_t{0}); std::iota(idx_b.begin(), idx_b.end(), std::size_t{0}); - auto contraction = dot_product(visitable.expr(), std::move(idx_a), - std::move(dA), std::move(idx_b)); + auto contraction = + dot_product(visitable.expr(), std::move(idx_a), dA, std::move(idx_b)); auto inv_norm = pow(m_expr, -wrap_scalar(get_scalar_one())); m_result = std::move(contraction) * inv_norm; } @@ -268,8 +268,7 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( return; } auto invAT = inv(trans(visitable.expr())); - auto contracted = - dot_product(invAT, sequence{1, 2}, std::move(dA), sequence{1, 2}); + auto contracted = dot_product(invAT, sequence{1, 2}, dA, sequence{1, 2}); m_result = m_expr * std::move(contracted); } @@ -282,8 +281,7 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( return; } auto Ei = eigen_decomposition(visitable.expr()).basis(visitable.index()); - m_result = - dot_product(std::move(Ei), sequence{1, 2}, std::move(dB), sequence{1, 2}); + m_result = dot_product(Ei, sequence{1, 2}, dB, sequence{1, 2}); } // [f; λ_M]: d/ds = Σ_{distinct k in M} mult_k [f; λ_{M∪{k}}] dλ_k/ds @@ -330,14 +328,13 @@ void tensor_to_scalar_differentiation_wrt_scalar::operator()( if (dA.is_valid() && !is_same(dA)) { auto s_l = seq_lhs; auto s_r = seq_rhs; - sum = dot_product(std::move(dA), std::move(s_l), visitable.expr_rhs(), - std::move(s_r)); + sum = dot_product(dA, std::move(s_l), visitable.expr_rhs(), std::move(s_r)); } if (dB.is_valid() && !is_same(dB)) { auto s_l = seq_lhs; auto s_r = seq_rhs; - auto term = dot_product(visitable.expr_lhs(), std::move(s_l), std::move(dB), - std::move(s_r)); + auto term = + dot_product(visitable.expr_lhs(), std::move(s_l), dB, std::move(s_r)); if (sum.is_valid()) { sum += term; } else { From a4f840ab262bc00820c1bbc5ddf9406fc63d3748 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 25 Jul 2026 15:46:37 +0200 Subject: [PATCH 24/24] Round-2 review fix on #355: cumulative path budget for the depth guard Two probe-confirmed bypasses survived the per-feature caps: bracketed exponents continue the ^-chain (1^(1)^(1)... - the chain-reset fix re-opened this shape, SIGSEGV from ~13k), and compound payloads multiply independent counters along the nesting path (300 minuses per level x 200 levels, every counter under 512, SIGSEGV). The guard now keeps one cumulative budget along the parse path: brackets push the current runs (they stay on the stack while inside), ^ and unary-minus runs add at the current level, a close restores the ^-chain (x^(y)^z is one chain) and ends the minus chain, and binary operators reset both. Shallow-caret products, bracketed-exponent chains under the cap, and 600-term minus chains all stay legal (locked in); both payload shapes now throw parse_error. Signed-off-by: petlenz --- src/numsim_cas/parser/parser.cpp | 55 +++++++++++++++++++------------- tests/ParserTest.h | 20 ++++++++++++ 2 files changed, 53 insertions(+), 22 deletions(-) diff --git a/src/numsim_cas/parser/parser.cpp b/src/numsim_cas/parser/parser.cpp index c455cd42..1b9a6193 100644 --- a/src/numsim_cas/parser/parser.cpp +++ b/src/numsim_cas/parser/parser.cpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace numsim::cas::parser { @@ -52,42 +53,52 @@ void check_nesting_depth(std::string_view source) { return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\v' || c == '\f'; }; - const auto breaks_power_chain = [](char c) { - // power_tail recursion accumulates only within one ^-chain; any - // other operator/bracket starts a fresh chain, so independent - // shallow carets (x1^2*x2^2*...) must not count together - return c == '+' || c == '-' || c == '*' || c == '/' || c == '(' || - c == ')' || c == '[' || c == ']' || c == '{' || c == '}' || c == ','; - }; - std::size_t depth = 0; - std::size_t minus_run = 0; - std::size_t caret_run = 0; + // Cumulative recursion budget along the current parse path (round-2 + // review): brackets, unary-minus runs, and ^-chains all contribute, and + // runs at ENCLOSING bracket levels stay on the stack while inside - + // independent counters let compound payloads (300 minuses per level x + // 200 levels) or bracketed exponents (1^(1)^(1)...) multiply past any + // per-feature cap. A ^-chain survives its bracketed exponent + // (x^(y)^z is one chain), so the caret run is restored on close. + std::size_t base = 0; // cost contributed by enclosing levels + std::size_t minus_run = 0; // current level + std::size_t caret_run = 0; // current level + std::vector> saved; + const auto cost = [&]() { return base + minus_run + caret_run; }; for (std::size_t i = 0; i < source.size(); ++i) { const char c = source[i]; if (c == '(' || c == '[' || c == '{') { - if (++depth > max_depth) { + saved.emplace_back(minus_run, caret_run); + base += minus_run + caret_run + 1; + minus_run = 0; + caret_run = 0; + if (base > max_depth) { throw syntax_error("expression nesting too deep", i, source); } } else if (c == ')' || c == ']' || c == '}') { - if (depth > 0) { - --depth; + if (!saved.empty()) { + auto [m, k] = saved.back(); + saved.pop_back(); + base -= m + k + 1; + minus_run = 0; // the unary chain's operand just completed + caret_run = k; // the ^-chain continues past its exponent } } else if (c == '^') { - // each ^ in a chain adds one right-recursion level (review on - // #355: 50k chained carets overflowed the stack) - if (++caret_run > max_depth) { + ++caret_run; + if (cost() > max_depth) { throw syntax_error("expression nesting too deep", i, source); } - } - if (breaks_power_chain(c)) { + } else if (c == '+' || c == '*' || c == '/' || c == ',') { + minus_run = 0; caret_run = 0; - } - if (c == '-') { - if (++minus_run > max_depth) { + } else if (c == '-') { + caret_run = 0; // a binary/unary minus ends any ^-chain + ++minus_run; + if (cost() > max_depth) { throw syntax_error("expression nesting too deep", i, source); } } else if (!is_space(c)) { - minus_run = 0; + minus_run = 0; // operand characters end a unary-minus chain } } } diff --git a/tests/ParserTest.h b/tests/ParserTest.h index 63fe1eed..8a6fde1d 100644 --- a/tests/ParserTest.h +++ b/tests/ParserTest.h @@ -2198,6 +2198,26 @@ TEST(ParserDepthGuard, DeepNestingRaisesParseError) { for (int i = 1; i < 600; ++i) ok3 += "*x" + std::to_string(i) + "^2"; EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok3, syms)); + + // round-2 bypasses: bracketed exponents continue the ^-chain, and + // compound payloads must sum along the path + std::string bracket_carets = "1"; + for (int i = 0; i < 14000; ++i) + bracket_carets += "^(1)"; + EXPECT_THROW((void)numsim::cas::parser::parse(bracket_carets, syms), + numsim::cas::parser::parse_error); + std::string compound; + for (int i = 0; i < 200; ++i) + compound += std::string(300, '-') + "("; + compound += "1"; + compound += std::string(200, ')'); + EXPECT_THROW((void)numsim::cas::parser::parse(compound, syms), + numsim::cas::parser::parse_error); + // shallow bracketed exponents stay legal + std::string ok4 = "1"; + for (int i = 0; i < 100; ++i) + ok4 += "^(1)"; + EXPECT_NO_THROW((void)numsim::cas::parser::parse(ok4, syms)); } #endif // NUMSIM_CAS_PARSER_ENABLED