From 888bd2ff590fd1cedac285c499dc1684a34e4397 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 22 Aug 2026 17:51:52 +0300 Subject: [PATCH 1/5] math: add checked add/sub/mul for Rational Rational's arithmetic operators are fixed-width int64 and neither saturate nor report failure. At ledger-realistic magnitudes that is reachable: summing dp=2 legs of 10^9 minor units overflows at exactly INT64_MAX/10^9 + 1 rows, and signed overflow is undefined behaviour rather than a wrong-but-detectable answer, so an application has no way to notice before committing corrupted state. Adds checkedAdd/checkedSub/checkedMul returning expected, plus the detail::addOverflows/ subOverflows/mulOverflows predicates they rest on. The predicates answer from the operands rather than by performing the operation and inspecting the result, which for signed types is itself undefined; mulOverflows is division-based rather than a wide-product comparison so it stays valid in a constant expression on MSVC, which has no __int128. The intermediate cross-terms turn out to be the binding constraint, not the result: checkedAdd rejects pairs whose final value would have been perfectly representable but which cannot be reached without an intermediate that overflows -- 1/INT64_MAX + 1/(INT64_MAX-1), a tiny value with an unrepresentable common denominator. That is exactly the case a caller cannot detect by inspecting the answer, because unchecked there is no valid answer to inspect. Confirmed with UBSan: the unchecked operator+ on those operands reports "signed integer overflow: 9223372036854775806 + 9223372036854775807", while checkedAdd reports Overflow instead. checkedMul checks the cross-cancelled factors operator* actually multiplies rather than the raw operands, since cross-cancelling is what keeps most products in range -- checking beforehand would reject INT64_MAX/2 * 2/1, which reduces to INT64_MAX/1 and multiplies fine. The unchecked operators are unchanged and stay the default; nothing existing changes behaviour. Full suite 20,185 assertions / 1,086 cases. Refs #130 Co-Authored-By: Claude Opus 5 (1M context) --- docs/spec/util/rational.md | 37 ++++++++ include/morph/util/rational.hpp | 152 +++++++++++++++++++++++++++++++- tests/CMakeLists.txt | 1 + tests/test_rational_checked.cpp | 135 ++++++++++++++++++++++++++++ 4 files changed, 324 insertions(+), 1 deletion(-) create mode 100644 tests/test_rational_checked.cpp diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 6011cd9a..547b4b45 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -183,6 +183,40 @@ Only the wire codec (`setWire`) defends against this: it maps an `INT64_MIN` negates the trap value. In-code call sites get no such guard — keep operands well inside the envelope above. +### Checked arithmetic + +`operator+`/`operator-`/`operator*` are fixed-width `std::int64_t` arithmetic: +they neither saturate nor report failure, and signed overflow is undefined +behaviour rather than a wrong-but-detectable answer. At ledger-realistic +magnitudes this is reachable — summing dp=2 legs of 10^9 minor units overflows +at exactly `INT64_MAX / 10^9 + 1` rows. + +`checkedAdd`, `checkedSub` and `checkedMul` return +`std::expected`, yielding `RationalError::Overflow` +rather than committing the operation. They check every intermediate the +operation would form **before** forming any of it — detecting signed overflow +by performing it and inspecting the result is itself undefined, so the question +has to be answered from the operands alone. + +**The intermediate cross-terms are the binding constraint, not the result.** +`checkedAdd` rejects operand pairs whose *final* value would have been +perfectly representable but which cannot be reached without an intermediate +that overflows — for instance `1/INT64_MAX + 1/(INT64_MAX-1)`, a tiny value +with an unrepresentable common denominator. This is precisely the case a caller +cannot detect by inspecting the answer, because under the unchecked operators +there is no valid answer to inspect. + +`checkedMul` checks the *cross-cancelled* factors `operator*` actually +multiplies, not the raw operands: cross-cancelling is what keeps most products +in range, so checking beforehand would reject pairs that multiply perfectly +well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). + +The unchecked operators are unchanged and remain the default. They are correct +for any application whose magnitudes stay well inside the envelope described +under [Overflow & value-range envelope](#overflow--value-range-envelope); the +checked forms exist for applications that cannot make that guarantee and need +to detect the boundary rather than assume it. + ## Mixed-type expressions (expected propagation) Whenever an arithmetic expression contains an @@ -298,6 +332,9 @@ through `setWire`. | Member | Signature | |---|---| +| `checkedAdd(a, b)` | `constexpr expected noexcept` — exact sum, or `Overflow`. | +| `checkedSub(a, b)` | `constexpr expected noexcept` — exact difference, or `Overflow`. | +| `checkedMul(a, b)` | `constexpr expected noexcept` — exact product, or `Overflow`. | | `setWire(Wire)` | `void noexcept` — rebuilds through canonicalising constructor, clamping hostile input. | | `getWire()` | `Wire noexcept` — canonical members ready for JSON encoding. | | `struct Wire` | `{ int64_t num; int64_t den; uint32_t dp; }` — flat JSON representation. | diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 22d26d08..7e358572 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -160,7 +160,8 @@ inline constexpr std::uint32_t kMaxDecimalPlaces = 18; enum class RationalError : std::uint8_t { DivisionByZero, ///< Divisor numerator is zero (operator/, reciprocal, From). NotFinite, ///< Floating-point input was NaN or +/-Inf (fromFloat only). - Overflow, ///< Scaled magnitude exceeds int64_t range (fromFloat only). + Overflow, ///< Result or an intermediate term exceeds int64_t range + ///< (`fromFloat`, and the `checked*` arithmetic helpers). }; namespace detail { @@ -186,6 +187,63 @@ namespace detail { return clampWireDecimalPlaces(rawDecimalPlaces); } +/// @brief Whether `lhs + rhs` would overflow `std::int64_t`. +/// +/// Asks *before* performing the addition. Detecting overflow by doing it and +/// inspecting the result is undefined behaviour for signed types, so the +/// question has to be answered from the operands alone. +/// @param lhs Left addend. +/// @param rhs Right addend. +/// @return `true` if the sum is not representable. +[[nodiscard]] constexpr bool addOverflows(std::int64_t lhs, std::int64_t rhs) noexcept { + constexpr auto maxValue = std::numeric_limits::max(); + constexpr auto minValue = std::numeric_limits::min(); + if (rhs > 0 && lhs > maxValue - rhs) { + return true; + } + return rhs < 0 && lhs < minValue - rhs; +} + +/// @brief Whether `lhs - rhs` would overflow `std::int64_t`. +/// @param lhs Minuend. +/// @param rhs Subtrahend. +/// @return `true` if the difference is not representable. +[[nodiscard]] constexpr bool subOverflows(std::int64_t lhs, std::int64_t rhs) noexcept { + constexpr auto maxValue = std::numeric_limits::max(); + constexpr auto minValue = std::numeric_limits::min(); + if (rhs < 0 && lhs > maxValue + rhs) { + return true; + } + return rhs > 0 && lhs < minValue + rhs; +} + +/// @brief Whether `lhs * rhs` would overflow `std::int64_t`. +/// +/// Division-based rather than a wide-product comparison so it stays valid in a +/// constant expression on every supported compiler (MSVC has no `__int128`). +/// @param lhs Left factor. +/// @param rhs Right factor. +/// @return `true` if the product is not representable. +[[nodiscard]] constexpr bool mulOverflows(std::int64_t lhs, std::int64_t rhs) noexcept { + constexpr auto maxValue = std::numeric_limits::max(); + constexpr auto minValue = std::numeric_limits::min(); + if (lhs == 0 || rhs == 0) { + return false; + } + // -1 is the one factor whose product can overflow without either operand + // being large: -1 * INT64_MIN has no positive counterpart. + if (lhs == -1) { + return rhs == minValue; + } + if (rhs == -1) { + return lhs == minValue; + } + if (lhs > 0) { + return rhs > 0 ? lhs > maxValue / rhs : rhs < minValue / lhs; + } + return rhs > 0 ? lhs < minValue / rhs : lhs < maxValue / rhs; +} + /// @brief A 64x64 -> 128-bit unsigned product, comparable high-word-first. struct U128 { std::uint64_t hi{}; @@ -607,6 +665,98 @@ static_assert(std::is_standard_layout_v); return lhs.dividedBy(rhs); } +/// @brief Adds two Rationals, reporting overflow instead of committing it. +/// +/// `operator+` is fixed-width `std::int64_t` arithmetic and neither saturates +/// nor reports failure: at ledger-realistic magnitudes, summing enough rows +/// genuinely overflows, and signed overflow is undefined behaviour rather than +/// a wrong-but-detectable answer. A fuzz test measured the boundary for dp=2 +/// legs of 10^9 minor units at exactly 9,223,372,037 rows. +/// +/// This checks every intermediate the addition would form — both cross-terms +/// and their sum — *before* forming any of them, then delegates to `operator+` +/// once they are known to fit. Note the cross-terms are the tighter bound: they +/// can overflow while the final result would have been perfectly +/// representable, which is exactly the case a caller cannot detect by +/// inspecting the answer. +/// +/// @param lhs Left addend. +/// @param rhs Right addend. +/// @return The exact sum, or `unexpected(RationalError::Overflow)`. +[[nodiscard]] constexpr std::expected checkedAdd(const Rational& lhs, + const Rational& rhs) noexcept { + auto const denominatorGcd = std::gcd(lhs.denominator, rhs.denominator); + auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; + auto const leftDenominatorScaled = lhs.denominator / denominatorGcd; + + if (detail::mulOverflows(lhs.numerator, rightDenominatorScaled) + || detail::mulOverflows(rhs.numerator, leftDenominatorScaled) + || detail::mulOverflows(lhs.denominator, rightDenominatorScaled)) { + return std::unexpected(RationalError::Overflow); + } + if (detail::addOverflows(lhs.numerator * rightDenominatorScaled, rhs.numerator * leftDenominatorScaled)) { + return std::unexpected(RationalError::Overflow); + } + return lhs + rhs; +} + +/// @brief Subtracts two Rationals, reporting overflow instead of committing it. +/// +/// The `checkedAdd` counterpart; see that function for why the intermediate +/// cross-terms rather than the final result are the binding constraint. +/// +/// @param lhs Minuend. +/// @param rhs Subtrahend. +/// @return The exact difference, or `unexpected(RationalError::Overflow)`. +[[nodiscard]] constexpr std::expected checkedSub(const Rational& lhs, + const Rational& rhs) noexcept { + auto const denominatorGcd = std::gcd(lhs.denominator, rhs.denominator); + auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; + auto const leftDenominatorScaled = lhs.denominator / denominatorGcd; + + if (detail::mulOverflows(lhs.numerator, rightDenominatorScaled) + || detail::mulOverflows(rhs.numerator, leftDenominatorScaled) + || detail::mulOverflows(lhs.denominator, rightDenominatorScaled)) { + return std::unexpected(RationalError::Overflow); + } + if (detail::subOverflows(lhs.numerator * rightDenominatorScaled, rhs.numerator * leftDenominatorScaled)) { + return std::unexpected(RationalError::Overflow); + } + return lhs - rhs; +} + +/// @brief Multiplies two Rationals, reporting overflow instead of committing it. +/// +/// Checks the cross-cancelled factors `operator*` actually multiplies, not the +/// raw operands: cross-cancelling is what keeps most products in range, so +/// checking before it would reject pairs that multiply perfectly well. +/// +/// @param lhs Left factor. +/// @param rhs Right factor. +/// @return The exact product, or `unexpected(RationalError::Overflow)`. +[[nodiscard]] constexpr std::expected checkedMul(const Rational& lhs, + const Rational& rhs) noexcept { + auto const absoluteLeftNumerator = lhs.numerator < 0 ? -lhs.numerator : lhs.numerator; + auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, lhs.denominator); + if (crossDivisorOne == 0 || crossDivisorTwo == 0) { + // A zero cross-divisor means a zero numerator on that side, so the + // product is zero and cannot overflow. + return lhs * rhs; + } + auto const reducedLeftNumerator = lhs.numerator / crossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; + auto const reducedLeftDenominator = lhs.denominator / crossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; + + if (detail::mulOverflows(reducedLeftNumerator, reducedRightNumerator) + || detail::mulOverflows(reducedLeftDenominator, reducedRightDenominator)) { + return std::unexpected(RationalError::Overflow); + } + return lhs * rhs; +} + // --------------------------------------------------------------------------- // Mixed-type arithmetic with automatic expected-propagation. // --------------------------------------------------------------------------- diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ab95c85d..8fe139be 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -67,6 +67,7 @@ add_executable(morph_tests test_action_log_phase2.cpp test_journal_format_versioning.cpp test_outbox.cpp + test_rational_checked.cpp test_rational.cpp test_ledger_rational_fuzz.cpp test_quantity.cpp diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp new file mode 100644 index 00000000..c38f3534 --- /dev/null +++ b/tests/test_rational_checked.cpp @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Rational's checked arithmetic: report overflow instead of committing it. + +#include + +#include + +#include + +using morph::math::checkedAdd; +using morph::math::checkedMul; +using morph::math::checkedSub; +using morph::math::DecimalPlaces; +using morph::math::Denominator; +using morph::math::Numerator; +using morph::math::Rational; +using morph::math::RationalError; + +namespace { + +constexpr auto kMax = std::numeric_limits::max(); +constexpr auto kMin = std::numeric_limits::min(); + +[[nodiscard]] Rational whole(std::int64_t value) { + return Rational{Numerator{value}, Denominator{1}, DecimalPlaces{2}}; +} + +} // namespace + +TEST_CASE("Overflow predicates answer from the operands, never by overflowing", + "[rational][checked]") { + using morph::math::detail::addOverflows; + using morph::math::detail::mulOverflows; + using morph::math::detail::subOverflows; + + CHECK_FALSE(addOverflows(1, 2)); + CHECK(addOverflows(kMax, 1)); + CHECK(addOverflows(kMin, -1)); + CHECK_FALSE(addOverflows(kMax, -1)); + + CHECK_FALSE(subOverflows(2, 1)); + CHECK(subOverflows(kMin, 1)); + CHECK(subOverflows(kMax, -1)); + CHECK_FALSE(subOverflows(kMin, -1)); + + CHECK_FALSE(mulOverflows(0, kMin)); + CHECK_FALSE(mulOverflows(kMin, 0)); + CHECK_FALSE(mulOverflows(3, 3)); + CHECK(mulOverflows(kMax, 2)); + CHECK(mulOverflows(2, kMax)); + CHECK(mulOverflows(-4000000000LL, -4000000000LL)); // both negative + CHECK(mulOverflows(2, kMin / 2 - 1)); // positive x negative + CHECK(mulOverflows(kMin / 2 - 1, 2)); // negative x positive + // -1 is the factor that overflows without either operand being large. + CHECK(mulOverflows(-1, kMin)); + CHECK(mulOverflows(kMin, -1)); + CHECK_FALSE(mulOverflows(-1, kMax)); +} + +TEST_CASE("checkedAdd returns the same value operator+ would, when it fits", + "[rational][checked]") { + const auto sum = checkedAdd(whole(2), whole(3)); + REQUIRE(sum.has_value()); + CHECK(*sum == whole(5)); + CHECK(*sum == whole(2) + whole(3)); +} + +TEST_CASE("checkedAdd reports overflow instead of committing it", "[rational][checked]") { + const auto sum = checkedAdd(whole(kMax), whole(1)); + REQUIRE_FALSE(sum.has_value()); + CHECK(sum.error() == RationalError::Overflow); +} + +TEST_CASE("checkedAdd catches an overflowing cross-term whose result would have fit", + "[rational][checked]") { + // The case a caller cannot detect by inspecting the answer: the final sum + // is small, but reaching it requires an intermediate that does not fit. + // 1/kMax + 1/(kMax-1) has a tiny value and an unrepresentable denominator. + const Rational lhs{Numerator{1}, Denominator{kMax}, DecimalPlaces{2}}; + const Rational rhs{Numerator{1}, Denominator{kMax - 1}, DecimalPlaces{2}}; + + const auto sum = checkedAdd(lhs, rhs); + REQUIRE_FALSE(sum.has_value()); + CHECK(sum.error() == RationalError::Overflow); +} + +TEST_CASE("checkedSub mirrors checkedAdd", "[rational][checked]") { + const auto ok = checkedSub(whole(5), whole(3)); + REQUIRE(ok.has_value()); + CHECK(*ok == whole(2)); + + const auto bad = checkedSub(whole(kMin), whole(1)); + REQUIRE_FALSE(bad.has_value()); + CHECK(bad.error() == RationalError::Overflow); +} + +TEST_CASE("checkedMul checks the cross-cancelled factors, not the raw operands", + "[rational][checked]") { + // kMax/2 * 2/1 cross-cancels to kMax/1 and multiplies fine. Checking the + // raw operands would have rejected it. + const Rational lhs{Numerator{kMax}, Denominator{2}, DecimalPlaces{2}}; + const Rational rhs{Numerator{2}, Denominator{1}, DecimalPlaces{2}}; + const auto product = checkedMul(lhs, rhs); + REQUIRE(product.has_value()); + CHECK(product->numerator == kMax); + + const auto bad = checkedMul(whole(kMax), whole(3)); + REQUIRE_FALSE(bad.has_value()); + CHECK(bad.error() == RationalError::Overflow); +} + +TEST_CASE("checkedMul handles a zero operand without dividing by a zero gcd", + "[rational][checked]") { + const auto product = checkedMul(whole(0), whole(kMax)); + REQUIRE(product.has_value()); + CHECK(product->numerator == 0); +} + +TEST_CASE("Summing at ledger magnitudes reports the boundary rather than crossing it", + "[rational][checked]") { + // The issue's own scenario: dp=2 legs of 10^9 minor units. The boundary is + // kMax / 10^9 + 1 rows; this walks up to it without ever performing the + // overflowing addition. + constexpr std::int64_t leg = 1'000'000'000; + auto running = Rational{Numerator{kMax - leg}, Denominator{1}, DecimalPlaces{2}}; + + const auto stillFits = checkedAdd(running, whole(leg)); + REQUIRE(stillFits.has_value()); + + running = *stillFits; + const auto overflows = checkedAdd(running, whole(1)); + REQUIRE_FALSE(overflows.has_value()); + CHECK(overflows.error() == RationalError::Overflow); +} From d2bb5a74cf41e8b5379f6f168b08ffa66ac0942d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 22 Aug 2026 18:55:29 +0300 Subject: [PATCH 2/5] tests: keep checkedSub's overflow case inside the representable range The gcc, MSVC and Valgrind legs aborted on 'checkedSub mirrors checkedAdd' while clang passed. The test was at fault, not checkedSub: it constructed Rational{Numerator{INT64_MIN}, ...}, and canonicalise() negates the numerator unguarded, so -INT64_MIN is signed-overflow UB before any arithmetic runs. Isolated with UBSan down to construction alone: rational.hpp:562: negation of -9223372036854775808 cannot be represented setWire guards exactly this case on the wire path; the public constructor does not. That is a separate pre-existing defect and is filed on its own -- this commit only stops the test from depending on it, using kMin + 1 and a difference of 2 (kMin + 1 - 1 is exactly INT64_MIN, which still fits, so 2 is the first that genuinely does not). Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_rational_checked.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index c38f3534..92ab8b62 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -90,7 +90,16 @@ TEST_CASE("checkedSub mirrors checkedAdd", "[rational][checked]") { REQUIRE(ok.has_value()); CHECK(*ok == whole(2)); - const auto bad = checkedSub(whole(kMin), whole(1)); + // kMin + 1 (== -INT64_MAX), not kMin: constructing a Rational whose + // numerator is INT64_MIN is itself undefined behaviour -- canonicalise() + // negates the numerator unguarded, and -INT64_MIN is not representable. + // setWire guards that case on the wire path; the public constructor does + // not. Tracked separately; this test stays inside the representable range + // so it measures checkedSub rather than that. + // + // -INT64_MAX - 2 is the first difference that genuinely does not fit + // (-INT64_MAX - 1 is exactly INT64_MIN, which still does). + const auto bad = checkedSub(whole(kMin + 1), whole(2)); REQUIRE_FALSE(bad.has_value()); CHECK(bad.error() == RationalError::Overflow); } From a43dde808d2442e5bc89db1867177a2d28b0ec98 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 22 Aug 2026 22:38:38 +0300 Subject: [PATCH 3/5] math: make Rational's arithmetic operators saturate and log, never overflow Review feedback on #153: adding checked* alongside the operators left the type still misusable into UB, because the operators themselves were unchanged. The operators now delegate to the same checks. operator+=/-=/*= run the overflow predicate first. On overflow they log at error and clamp to the largest representable magnitude of the correct sign instead of committing signed-overflow undefined behaviour. The sign comes from exact comparison (a + b compares a against -b), not from the operands' magnitudes, so a mixed-sign case whose cross-terms overflow still saturates in the mathematically correct direction -- 1/INT64_MAX + 1/(INT64_MAX-1) saturates positive, and INT64_MAX * -3 saturates negative. Saturation rather than throwing: these operators run inside strand-bound model code and in constexpr expressions, so throwing would change their contract for every existing caller, while leaving the overflow undefined is the thing being fixed. A clamped value is wrong, but defined wrong, and logged. Callers that must not absorb it keep checkedAdd/checkedSub/checkedMul, which now share the operators' predicates so the two cannot disagree about what overflows. Fixing the operators exposed that canonicalise() had the same class of bug one level down: it negated the numerator unguarded, so an INT64_MIN component was UB. That was reachable not only by constructing such a value (#156) but by ordinary arithmetic landing on it exactly -- -INT64_MAX - 1 is a legal subtraction whose result is INT64_MIN, and the subtraction itself does not overflow. canonicalise now clamps such a component to -INT64_MAX and logs, matching what setWire already did for the same values off the wire, and takes magnitudes through the existing detail::absU64 helper so no negation can overflow. Verified with UBSan: every case that previously reported "signed integer overflow" or "negation of -9223372036854775808" now logs and saturates with no diagnostic. Tests assert the saturated values, the sign in each direction, and that the log actually fires (via ScopedLoggerOverride). Full suite 20,206 assertions / 1,092 cases; gcc -Wall -Wextra -Werror and clang -Wdocumentation both clean. Refs #130, #156 Co-Authored-By: Claude Opus 5 (1M context) --- docs/spec/util/rational.md | 43 +++- include/morph/util/rational.hpp | 342 +++++++++++++++++++++++--------- tests/test_rational_checked.cpp | 99 +++++++++ 3 files changed, 384 insertions(+), 100 deletions(-) diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 547b4b45..ca2502a3 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -185,15 +185,38 @@ well inside the envelope above. ### Checked arithmetic -`operator+`/`operator-`/`operator*` are fixed-width `std::int64_t` arithmetic: -they neither saturate nor report failure, and signed overflow is undefined -behaviour rather than a wrong-but-detectable answer. At ledger-realistic -magnitudes this is reachable — summing dp=2 legs of 10^9 minor units overflows -at exactly `INT64_MAX / 10^9 + 1` rows. +`operator+`/`operator-`/`operator*` are fixed-width `std::int64_t` arithmetic, +and at ledger-realistic magnitudes overflow is reachable — summing dp=2 legs of +10^9 minor units overflows at exactly `INT64_MAX / 10^9 + 1` rows. + +**The operators saturate; they never overflow.** When the exact result — or any +intermediate cross-term needed to reach it — does not fit, the operator logs at +`error` and clamps to the largest representable magnitude *of the correct +sign*, rather than committing signed-overflow undefined behaviour. The sign +comes from exact comparison (`a + b` compares `a` against `-b`), not from the +operands' magnitudes, so a mixed-sign case whose cross-terms overflow still +saturates in the mathematically correct direction. + +Saturation rather than an exception because these operators are used inside +strand-bound model code and in `constexpr` expressions: throwing would change +their contract for every existing caller, while leaving the overflow undefined +is what this exists to stop. A clamped value is wrong, but it is *defined* +wrong, and it is logged. + +`canonicalise` is total for the same reason. It previously negated the +numerator unguarded, so a component of `INT64_MIN` was undefined behaviour — +reachable both by constructing such a value directly and by *ordinary +arithmetic landing on it exactly* (`-INT64_MAX - 1` is a legal subtraction +whose result is `INT64_MIN`). Such a component is now clamped to `-INT64_MAX` +and logged, matching what `setWire` already did for the same values arriving +off the wire. `checkedAdd`, `checkedSub` and `checkedMul` return `std::expected`, yielding `RationalError::Overflow` -rather than committing the operation. They check every intermediate the +rather than saturating. That is the division of labour: the operators stay +usable and defined for code that can absorb a clamped value, while the checked +forms stay exact-or-nothing for code that must not — a ledger totalling rows +needs to *stop*, not carry on with a clamped balance. They check every intermediate the operation would form **before** forming any of it — detecting signed overflow by performing it and inspecting the result is itself undefined, so the question has to be answered from the operands alone. @@ -211,11 +234,9 @@ multiplies, not the raw operands: cross-cancelling is what keeps most products in range, so checking beforehand would reject pairs that multiply perfectly well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). -The unchecked operators are unchanged and remain the default. They are correct -for any application whose magnitudes stay well inside the envelope described -under [Overflow & value-range envelope](#overflow--value-range-envelope); the -checked forms exist for applications that cannot make that guarantee and need -to detect the boundary rather than assume it. +Both share one set of predicates (`addWouldOverflow`, `subWouldOverflow`, +`mulWouldOverflow`), so the operators and the checked forms cannot disagree +about what overflows. ## Mixed-type expressions (expected propagation) diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 7e358572..7f31b792 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -88,6 +88,8 @@ /// (e.g. sums over large coprime denominators). Keep operands within /// the decimal-scaled ranges the precision tags imply. +#include + #include #include @@ -459,30 +461,36 @@ struct Rational { /// @brief In-place addition. Result precision becomes `max` of the two. /// Uses reduce-before-multiply to extend the safe int64 range (Knuth 4.5.1). + /// + /// **Saturates rather than overflowing.** If the exact sum -- or any + /// intermediate cross-term needed to reach it -- is not representable, this + /// logs at `error` and clamps to the largest magnitude of the correct sign + /// instead of committing signed-overflow undefined behaviour. See + /// `saturateToward` for why a defined wrong answer beats UB here. /// @param rhs Value to add. /// @return `*this`. - constexpr Rational& operator+=(const Rational& rhs) noexcept { - auto const denominatorGcd = std::gcd(denominator, rhs.denominator); - auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; - auto const leftDenominatorScaled = denominator / denominatorGcd; - numerator = (numerator * rightDenominatorScaled) + (rhs.numerator * leftDenominatorScaled); - denominator = denominator * rightDenominatorScaled; - widenPrecisionTo(rhs.decimalPlaces); - canonicalise(); + constexpr Rational& operator+=(const Rational& rhs) { + if (addWouldOverflow(rhs)) { + reportOverflow("operator+="); + saturateToward(compareForSaturation(rhs, true), rhs.decimalPlaces); + return *this; + } + addAssignUnchecked(rhs); return *this; } /// @brief In-place subtraction. Result precision becomes `max` of the two. + /// + /// Saturates rather than overflowing; see `operator+=`. /// @param rhs Value to subtract. /// @return `*this`. - constexpr Rational& operator-=(const Rational& rhs) noexcept { - auto const denominatorGcd = std::gcd(denominator, rhs.denominator); - auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; - auto const leftDenominatorScaled = denominator / denominatorGcd; - numerator = (numerator * rightDenominatorScaled) - (rhs.numerator * leftDenominatorScaled); - denominator = denominator * rightDenominatorScaled; - widenPrecisionTo(rhs.decimalPlaces); - canonicalise(); + constexpr Rational& operator-=(const Rational& rhs) { + if (subWouldOverflow(rhs)) { + reportOverflow("operator-="); + saturateToward(compareForSaturation(rhs, false), rhs.decimalPlaces); + return *this; + } + subAssignUnchecked(rhs); return *this; } @@ -490,19 +498,18 @@ struct Rational { /// Cross-cancels common factors before multiplying. /// @param rhs Value to multiply by. /// @return `*this`. - constexpr Rational& operator*=(const Rational& rhs) noexcept { - auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); - auto const reducedLeftNumerator = numerator / crossDivisorOne; - auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; - auto const reducedLeftDenominator = denominator / crossDivisorTwo; - auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; - numerator = reducedLeftNumerator * reducedRightNumerator; - denominator = reducedLeftDenominator * reducedRightDenominator; - widenPrecisionTo(rhs.decimalPlaces); - canonicalise(); + /// + /// Saturates rather than overflowing; see `operator+=`. + constexpr Rational& operator*=(const Rational& rhs) { + if (mulWouldOverflow(rhs)) { + reportOverflow("operator*="); + // Sign of a product is the product of the signs; zero operands + // cannot overflow, so neither sign is zero here. + const bool negative = (numerator < 0) != (rhs.numerator < 0); + saturateToward(negative ? -1 : 1, rhs.decimalPlaces); + return *this; + } + mulAssignUnchecked(rhs); return *this; } @@ -546,6 +553,173 @@ void setWire(Wire wire) noexcept { /// @return The canonical members, ready for JSON encoding. [[nodiscard]] Wire getWire() const noexcept { return Wire{.num = numerator, .den = denominator, .dp = decimalPlaces.value}; } +private: + /// @brief Logs an overflow that `operator+=`/`-=`/`*=` saturated instead + /// of committing. + /// + /// Skipped during constant evaluation: `log` is not `constexpr`, and a + /// `constexpr` arithmetic expression that saturates should still compile. + /// @param where Which operator saturated. + static constexpr void reportOverflow(std::string_view where) { + if (!std::is_constant_evaluated()) { + ::morph::log::logError("[Rational] {} overflowed int64 and saturated; the result is clamped, " + "not exact. Use checkedAdd/checkedSub/checkedMul to detect this instead.", + where); + } + } + + /// @brief Logs an `INT64_MIN` component clamped by `canonicalise`. + /// + /// Skipped during constant evaluation, like `reportOverflow`. + static constexpr void reportClamp() { + if (!std::is_constant_evaluated()) { + ::morph::log::logError("[Rational] an INT64_MIN component was clamped to -INT64_MAX; canonicalising " + "it would require negating a value with no positive counterpart."); + } + } + + /// @brief The sign the saturated result should carry, for `+=` / `-=`. + /// + /// Determined by exact comparison rather than by computing the true sum, + /// which by definition does not fit. `a + b` has the sign of `a` versus + /// `-b`; `a - b` has the sign of `a` versus `b`. Both use the type's own + /// exact 128-bit-wide comparison, so a mixed-sign case whose cross-terms + /// overflow still saturates in the mathematically correct direction. + /// @param rhs The other operand. + /// @param addition `true` for `+=`, `false` for `-=`. + /// @return `-1`, `0` or `1`. + [[nodiscard]] constexpr int compareForSaturation(const Rational& rhs, bool addition) const noexcept { + const auto ordering = addition ? (*this <=> -rhs) : (*this <=> rhs); + if (ordering < 0) { + return -1; + } + return ordering > 0 ? 1 : 0; + } + + /// @brief Clamps this value to the largest representable magnitude with + /// sign @p sign, at `max(decimalPlaces, other)`. + /// + /// Saturation, not an exception and not UB. `operator+` and friends are + /// used inside strand-bound model code and in `constexpr` expressions; + /// throwing would change their contract for every existing caller, while + /// leaving the overflow undefined is what this whole change exists to + /// stop. A clamped value is wrong, but it is *defined* wrong, it is + /// logged, and `checkedAdd`/`checkedSub`/`checkedMul` remain available for + /// callers that need to detect the condition rather than absorb it. + /// @param sign Direction to clamp toward; `0` yields zero. + /// @param other The other operand's precision, folded in as usual. + constexpr void saturateToward(int sign, DecimalPlaces other) noexcept { + constexpr auto maxValue = std::numeric_limits::max(); + numerator = sign == 0 ? 0 : (sign < 0 ? -maxValue : maxValue); + denominator = 1; + widenPrecisionTo(other); + } + + /// @brief `operator+=`'s arithmetic, without the overflow check. + /// @param rhs Value to add. + constexpr void addAssignUnchecked(const Rational& rhs) noexcept { + auto const denominatorGcd = std::gcd(denominator, rhs.denominator); + auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; + auto const leftDenominatorScaled = denominator / denominatorGcd; + numerator = (numerator * rightDenominatorScaled) + (rhs.numerator * leftDenominatorScaled); + denominator = denominator * rightDenominatorScaled; + widenPrecisionTo(rhs.decimalPlaces); + canonicalise(); + } + + /// @brief `operator-=`'s arithmetic, without the overflow check. + /// @param rhs Value to subtract. + constexpr void subAssignUnchecked(const Rational& rhs) noexcept { + auto const denominatorGcd = std::gcd(denominator, rhs.denominator); + auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; + auto const leftDenominatorScaled = denominator / denominatorGcd; + numerator = (numerator * rightDenominatorScaled) - (rhs.numerator * leftDenominatorScaled); + denominator = denominator * rightDenominatorScaled; + widenPrecisionTo(rhs.decimalPlaces); + canonicalise(); + } + + /// @brief `operator*=`'s arithmetic, without the overflow check. + /// @param rhs Value to multiply by. + constexpr void mulAssignUnchecked(const Rational& rhs) noexcept { + auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; + auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); + auto const reducedLeftNumerator = numerator / crossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; + auto const reducedLeftDenominator = denominator / crossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; + numerator = reducedLeftNumerator * reducedRightNumerator; + denominator = reducedLeftDenominator * reducedRightDenominator; + widenPrecisionTo(rhs.decimalPlaces); + canonicalise(); + } + + public: + /// @brief Applies `+=`'s arithmetic, having already proven it cannot overflow. + /// + /// For `checkedAdd`, which has just run `addWouldOverflow` and must not + /// re-enter `operator+=`'s saturating path. Calling this without that + /// check is undefined on overflow -- the check is the precondition. + /// @param rhs Value to add. + constexpr void addAssignChecked(const Rational& rhs) noexcept { addAssignUnchecked(rhs); } + + /// @brief Applies `-=`'s arithmetic, having already proven it cannot overflow. + /// @param rhs Value to subtract. + constexpr void subAssignChecked(const Rational& rhs) noexcept { subAssignUnchecked(rhs); } + + /// @brief Applies `*=`'s arithmetic, having already proven it cannot overflow. + /// @param rhs Value to multiply by. + constexpr void mulAssignChecked(const Rational& rhs) noexcept { mulAssignUnchecked(rhs); } + + /// @brief Whether `*this + rhs` would overflow any intermediate or the result. + /// @param rhs The addend. + /// @return `true` if the addition cannot be performed exactly. + [[nodiscard]] constexpr bool addWouldOverflow(const Rational& rhs) const noexcept { + auto const denominatorGcd = std::gcd(denominator, rhs.denominator); + auto const rightScaled = rhs.denominator / denominatorGcd; + auto const leftScaled = denominator / denominatorGcd; + if (detail::mulOverflows(numerator, rightScaled) || detail::mulOverflows(rhs.numerator, leftScaled) + || detail::mulOverflows(denominator, rightScaled)) { + return true; + } + return detail::addOverflows(numerator * rightScaled, rhs.numerator * leftScaled); + } + + /// @brief Whether `*this - rhs` would overflow any intermediate or the result. + /// @param rhs The subtrahend. + /// @return `true` if the subtraction cannot be performed exactly. + [[nodiscard]] constexpr bool subWouldOverflow(const Rational& rhs) const noexcept { + auto const denominatorGcd = std::gcd(denominator, rhs.denominator); + auto const rightScaled = rhs.denominator / denominatorGcd; + auto const leftScaled = denominator / denominatorGcd; + if (detail::mulOverflows(numerator, rightScaled) || detail::mulOverflows(rhs.numerator, leftScaled) + || detail::mulOverflows(denominator, rightScaled)) { + return true; + } + return detail::subOverflows(numerator * rightScaled, rhs.numerator * leftScaled); + } + + /// @brief Whether `*this * rhs` would overflow, after cross-cancelling. + /// + /// Checks the cross-cancelled factors `operator*=` actually multiplies: + /// cross-cancelling is what keeps most products in range, so checking the + /// raw operands would reject pairs that multiply perfectly well. + /// @param rhs The factor. + /// @return `true` if the product cannot be represented. + [[nodiscard]] constexpr bool mulWouldOverflow(const Rational& rhs) const noexcept { + auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; + auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); + if (crossDivisorOne == 0 || crossDivisorTwo == 0) { + return false; // a zero numerator: the product is zero + } + return detail::mulOverflows(numerator / crossDivisorOne, rhs.numerator / crossDivisorTwo) + || detail::mulOverflows(denominator / crossDivisorTwo, rhs.denominator / crossDivisorOne); + } + private: /// @brief Restores the canonical-form invariants in place (denominator > 0, /// gcd reduced, zero denominator clamped to 1). Leaves `decimalPlaces` @@ -555,12 +729,35 @@ void setWire(Wire wire) noexcept { denominator = 1; return; } + constexpr auto minValue = std::numeric_limits::min(); + constexpr auto maxValue = std::numeric_limits::max(); + + // Neither component may be INT64_MIN past this point. Canonicalising + // needs `|value|` and a sign flip, and `-INT64_MIN` is not + // representable -- negating it is undefined behaviour, which this + // function used to commit. It was reachable two ways: constructing a + // Rational with such a numerator directly, and *ordinary arithmetic* + // landing on it exactly (`-INT64_MAX - 1` is a perfectly legal + // subtraction whose result is INT64_MIN). + // + // Clamped to the adjacent representable magnitude, matching what + // `setWire` already does for the same values arriving off the wire. + // The value is off by one ulp; it is not undefined. + if (numerator == minValue || denominator == minValue) { + reportClamp(); + numerator = numerator == minValue ? -maxValue : numerator; + denominator = denominator == minValue ? -maxValue : denominator; + } + if (denominator < 0) { numerator = -numerator; denominator = -denominator; } - auto const absoluteNumerator = numerator < 0 ? -numerator : numerator; - auto const divisor = std::gcd(absoluteNumerator, denominator); + // Magnitudes via `absU64`, so the gcd never negates either component. + // The result cannot exceed INT64_MAX: gcd(a, b) <= min(a, b) and the + // denominator is at most INT64_MAX here. + auto const divisor = + static_cast(std::gcd(detail::absU64(numerator), detail::absU64(denominator))); if (divisor > 1) { numerator /= divisor; denominator /= divisor; @@ -665,19 +862,17 @@ static_assert(std::is_standard_layout_v); return lhs.dividedBy(rhs); } -/// @brief Adds two Rationals, reporting overflow instead of committing it. +/// @brief Adds two Rationals, reporting overflow instead of saturating. /// -/// `operator+` is fixed-width `std::int64_t` arithmetic and neither saturates -/// nor reports failure: at ledger-realistic magnitudes, summing enough rows -/// genuinely overflows, and signed overflow is undefined behaviour rather than -/// a wrong-but-detectable answer. A fuzz test measured the boundary for dp=2 -/// legs of 10^9 minor units at exactly 9,223,372,037 rows. +/// `operator+` saturates and logs when the exact sum does not fit, so it is +/// never undefined -- but it is also silently inexact. This returns the +/// overflow instead, for callers that must not absorb it: a ledger totalling +/// rows needs to *stop*, not to carry on with a clamped balance. /// -/// This checks every intermediate the addition would form — both cross-terms -/// and their sum — *before* forming any of them, then delegates to `operator+` -/// once they are known to fit. Note the cross-terms are the tighter bound: they -/// can overflow while the final result would have been perfectly -/// representable, which is exactly the case a caller cannot detect by +/// Every intermediate is checked before any is formed, since detecting signed +/// overflow by performing it is itself undefined. Note the cross-terms are the +/// tighter bound: they can overflow while the final result would have been +/// perfectly representable, which is exactly the case a caller cannot spot by /// inspecting the answer. /// /// @param lhs Left addend. @@ -685,76 +880,45 @@ static_assert(std::is_standard_layout_v); /// @return The exact sum, or `unexpected(RationalError::Overflow)`. [[nodiscard]] constexpr std::expected checkedAdd(const Rational& lhs, const Rational& rhs) noexcept { - auto const denominatorGcd = std::gcd(lhs.denominator, rhs.denominator); - auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; - auto const leftDenominatorScaled = lhs.denominator / denominatorGcd; - - if (detail::mulOverflows(lhs.numerator, rightDenominatorScaled) - || detail::mulOverflows(rhs.numerator, leftDenominatorScaled) - || detail::mulOverflows(lhs.denominator, rightDenominatorScaled)) { + if (lhs.addWouldOverflow(rhs)) { return std::unexpected(RationalError::Overflow); } - if (detail::addOverflows(lhs.numerator * rightDenominatorScaled, rhs.numerator * leftDenominatorScaled)) { - return std::unexpected(RationalError::Overflow); - } - return lhs + rhs; + auto result = lhs; + result.addAssignChecked(rhs); + return result; } -/// @brief Subtracts two Rationals, reporting overflow instead of committing it. -/// -/// The `checkedAdd` counterpart; see that function for why the intermediate -/// cross-terms rather than the final result are the binding constraint. +/// @brief Subtracts two Rationals, reporting overflow instead of saturating. /// +/// The `checkedAdd` counterpart; see that function. /// @param lhs Minuend. /// @param rhs Subtrahend. /// @return The exact difference, or `unexpected(RationalError::Overflow)`. [[nodiscard]] constexpr std::expected checkedSub(const Rational& lhs, const Rational& rhs) noexcept { - auto const denominatorGcd = std::gcd(lhs.denominator, rhs.denominator); - auto const rightDenominatorScaled = rhs.denominator / denominatorGcd; - auto const leftDenominatorScaled = lhs.denominator / denominatorGcd; - - if (detail::mulOverflows(lhs.numerator, rightDenominatorScaled) - || detail::mulOverflows(rhs.numerator, leftDenominatorScaled) - || detail::mulOverflows(lhs.denominator, rightDenominatorScaled)) { - return std::unexpected(RationalError::Overflow); - } - if (detail::subOverflows(lhs.numerator * rightDenominatorScaled, rhs.numerator * leftDenominatorScaled)) { + if (lhs.subWouldOverflow(rhs)) { return std::unexpected(RationalError::Overflow); } - return lhs - rhs; + auto result = lhs; + result.subAssignChecked(rhs); + return result; } -/// @brief Multiplies two Rationals, reporting overflow instead of committing it. +/// @brief Multiplies two Rationals, reporting overflow instead of saturating. /// /// Checks the cross-cancelled factors `operator*` actually multiplies, not the -/// raw operands: cross-cancelling is what keeps most products in range, so -/// checking before it would reject pairs that multiply perfectly well. -/// +/// raw operands -- see `Rational::mulWouldOverflow`. /// @param lhs Left factor. /// @param rhs Right factor. /// @return The exact product, or `unexpected(RationalError::Overflow)`. [[nodiscard]] constexpr std::expected checkedMul(const Rational& lhs, const Rational& rhs) noexcept { - auto const absoluteLeftNumerator = lhs.numerator < 0 ? -lhs.numerator : lhs.numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, lhs.denominator); - if (crossDivisorOne == 0 || crossDivisorTwo == 0) { - // A zero cross-divisor means a zero numerator on that side, so the - // product is zero and cannot overflow. - return lhs * rhs; - } - auto const reducedLeftNumerator = lhs.numerator / crossDivisorOne; - auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; - auto const reducedLeftDenominator = lhs.denominator / crossDivisorTwo; - auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; - - if (detail::mulOverflows(reducedLeftNumerator, reducedRightNumerator) - || detail::mulOverflows(reducedLeftDenominator, reducedRightDenominator)) { + if (lhs.mulWouldOverflow(rhs)) { return std::unexpected(RationalError::Overflow); } - return lhs * rhs; + auto result = lhs; + result.mulAssignChecked(rhs); + return result; } // --------------------------------------------------------------------------- diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index 92ab8b62..b411bee6 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -7,6 +7,11 @@ #include #include +#include +#include +#include + +#include using morph::math::checkedAdd; using morph::math::checkedMul; @@ -142,3 +147,97 @@ TEST_CASE("Summing at ledger magnitudes reports the boundary rather than crossin REQUIRE_FALSE(overflows.has_value()); CHECK(overflows.error() == RationalError::Overflow); } + +// --------------------------------------------------------------------------- +// The saturating operators. Before these, every case below was undefined +// behaviour rather than a wrong-but-defined answer. +// --------------------------------------------------------------------------- + +TEST_CASE("operator+ saturates and logs instead of overflowing", "[rational][checked][saturate]") { + std::vector logged; + const morph::log::ScopedLoggerOverride capture{ + [&logged](morph::log::LogLevel, std::string_view msg) { logged.emplace_back(msg); }, + morph::log::LogLevel::error}; + + const auto sum = whole(kMax) + whole(1); + + CHECK(sum.numerator == kMax); + CHECK(sum.denominator == 1); + REQUIRE_FALSE(logged.empty()); + CHECK(logged.front().find("operator+=") != std::string::npos); +} + +TEST_CASE("Saturation carries the sign of the true result", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + CHECK((whole(kMax) + whole(1)).numerator == kMax); + CHECK((whole(kMin + 1) - whole(2)).numerator == -kMax); + CHECK((whole(kMax) * whole(3)).numerator == kMax); + // A negative product must not saturate positive. + CHECK((whole(kMax) * whole(-3)).numerator == -kMax); + CHECK((whole(-kMax) * whole(3)).numerator == -kMax); +} + +TEST_CASE("An intermediate-only overflow saturates toward the true sign", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + // 1/kMax + 1/(kMax-1): a tiny *positive* value whose common denominator is + // unrepresentable. The cross-terms overflow while the result would not, + // so the saturation direction cannot come from the operands' magnitudes -- + // it comes from exact comparison. + const Rational lhs{Numerator{1}, Denominator{kMax}, DecimalPlaces{2}}; + const Rational rhs{Numerator{1}, Denominator{kMax - 1}, DecimalPlaces{2}}; + + const auto sum = lhs + rhs; + CHECK(sum.numerator == kMax); // positive, as the true value is + + const Rational negLhs{Numerator{-1}, Denominator{kMax}, DecimalPlaces{2}}; + const Rational negRhs{Numerator{-1}, Denominator{kMax - 1}, DecimalPlaces{2}}; + CHECK((negLhs + negRhs).numerator == -kMax); +} + +TEST_CASE("A numerator of INT64_MIN is clamped, not undefined", "[rational][checked][saturate]") { + std::vector logged; + const morph::log::ScopedLoggerOverride capture{ + [&logged](morph::log::LogLevel, std::string_view msg) { logged.emplace_back(msg); }, + morph::log::LogLevel::error}; + + // Constructing this was undefined behaviour before: canonicalise() negated + // the numerator, and -INT64_MIN is not representable. + const Rational direct{Numerator{kMin}, Denominator{1}, DecimalPlaces{2}}; + CHECK(direct.numerator == -kMax); + + // And arithmetic can land on it exactly, without anyone naming it: + // -INT64_MAX - 1 is a legal subtraction whose result is INT64_MIN. + const auto landedOn = whole(-kMax) - whole(1); + CHECK(landedOn.numerator == -kMax); + + REQUIRE_FALSE(logged.empty()); +} + +TEST_CASE("Saturating operators do not disturb ordinary arithmetic", "[rational][checked][saturate]") { + CHECK((whole(2) + whole(3)) == whole(5)); + CHECK((whole(5) - whole(3)) == whole(2)); + CHECK((whole(6) * whole(7)) == whole(42)); + + const Rational third{Numerator{1}, Denominator{3}, DecimalPlaces{2}}; + const Rational sixth{Numerator{1}, Denominator{6}, DecimalPlaces{2}}; + const auto half = third + sixth; + CHECK(half.numerator == 1); + CHECK(half.denominator == 2); +} + +TEST_CASE("checked* still report rather than saturate, for callers that must not absorb it", + "[rational][checked][saturate]") { + // The division of labour: operators stay usable and defined, checked* + // stays exact-or-nothing. + const auto reported = checkedAdd(whole(kMax), whole(1)); + REQUIRE_FALSE(reported.has_value()); + CHECK(reported.error() == RationalError::Overflow); + + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + CHECK((whole(kMax) + whole(1)).numerator == kMax); +} From 169641e1de7dd8dcd7940ab01f14cd9fe8bd67d7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 22 Aug 2026 22:46:49 +0300 Subject: [PATCH 4/5] math: keep the saturating operators noexcept despite logging Review point: an arithmetic operator should not lose noexcept just because it logs. It no longer does. morph::log offers no noexcept guarantee -- the spec makes sink-propagation explicit ('a sink that must not disrupt its caller has to swallow its own exceptions internally'), and detail::log's own scoped_lock can throw std::system_error regardless of the sink. So reportOverflow/reportClamp swallow locally, exactly as CompletionState's destructor already does for the same reason, and operator+=/-=/*= carry noexcept again. Tested rather than asserted: static_assert on each operator's noexcept, plus a ScopedLoggerOverride installing a sink that throws on every call, driving all three overflow paths and the canonicalise clamp. Nothing escapes, and the values still saturate correctly. The underlying contract is filed as morph#158 -- the logging layer offering the guarantee once, rather than every noexcept caller hand-rolling a catch. Both workarounds come out if that lands. Co-Authored-By: Claude Opus 5 (1M context) --- docs/spec/util/rational.md | 8 ++++++ include/morph/util/rational.hpp | 43 ++++++++++++++++++++++++--------- tests/test_rational_checked.cpp | 28 +++++++++++++++++++++ 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index ca2502a3..b2e2222e 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -203,6 +203,14 @@ their contract for every existing caller, while leaving the overflow undefined is what this exists to stop. A clamped value is wrong, but it is *defined* wrong, and it is logged. +**The operators keep `noexcept`,** including when the overflow path logs. That +needs a local `try`/`catch` around the log call, because `morph::log` offers no +`noexcept` guarantee — a user-installed sink may throw, and `detail::log`'s own +`scoped_lock` may throw `std::system_error` — and an arithmetic operator must +not begin failing because logging failed. `CompletionState`'s destructor +carries the identical workaround for the identical reason; both can go once +morph#158 makes the logging layer non-throwing. + `canonicalise` is total for the same reason. It previously negated the numerator unguarded, so a component of `INT64_MIN` was undefined behaviour — reachable both by constructing such a value directly and by *ordinary diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 7f31b792..f7ef55f6 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -469,7 +469,7 @@ struct Rational { /// `saturateToward` for why a defined wrong answer beats UB here. /// @param rhs Value to add. /// @return `*this`. - constexpr Rational& operator+=(const Rational& rhs) { + constexpr Rational& operator+=(const Rational& rhs) noexcept { if (addWouldOverflow(rhs)) { reportOverflow("operator+="); saturateToward(compareForSaturation(rhs, true), rhs.decimalPlaces); @@ -484,7 +484,7 @@ struct Rational { /// Saturates rather than overflowing; see `operator+=`. /// @param rhs Value to subtract. /// @return `*this`. - constexpr Rational& operator-=(const Rational& rhs) { + constexpr Rational& operator-=(const Rational& rhs) noexcept { if (subWouldOverflow(rhs)) { reportOverflow("operator-="); saturateToward(compareForSaturation(rhs, false), rhs.decimalPlaces); @@ -500,7 +500,7 @@ struct Rational { /// @return `*this`. /// /// Saturates rather than overflowing; see `operator+=`. - constexpr Rational& operator*=(const Rational& rhs) { + constexpr Rational& operator*=(const Rational& rhs) noexcept { if (mulWouldOverflow(rhs)) { reportOverflow("operator*="); // Sign of a product is the product of the signs; zero operands @@ -559,22 +559,43 @@ void setWire(Wire wire) noexcept { /// /// Skipped during constant evaluation: `log` is not `constexpr`, and a /// `constexpr` arithmetic expression that saturates should still compile. + /// + /// The `try`/`catch` is not defensive padding. `morph::log` offers no + /// `noexcept` guarantee -- a user-installed sink may throw, and + /// `detail::log`'s own `scoped_lock` may throw `std::system_error` -- and + /// an arithmetic operator must not start failing because logging failed. + /// `CompletionState`'s destructor carries the identical workaround for the + /// identical reason. Both can go once morph#158 makes the logging layer + /// non-throwing. /// @param where Which operator saturated. - static constexpr void reportOverflow(std::string_view where) { + static constexpr void reportOverflow(std::string_view where) noexcept { if (!std::is_constant_evaluated()) { - ::morph::log::logError("[Rational] {} overflowed int64 and saturated; the result is clamped, " - "not exact. Use checkedAdd/checkedSub/checkedMul to detect this instead.", - where); + // NOLINTBEGIN(bugprone-empty-catch) -- see above: logging must not + // turn a defined-but-clamped result into a thrown exception. + try { + ::morph::log::logError("[Rational] {} overflowed int64 and saturated; the result is clamped, " + "not exact. Use checkedAdd/checkedSub/checkedMul to detect this instead.", + where); + } catch (...) { + } + // NOLINTEND(bugprone-empty-catch) } } /// @brief Logs an `INT64_MIN` component clamped by `canonicalise`. /// - /// Skipped during constant evaluation, like `reportOverflow`. - static constexpr void reportClamp() { + /// Skipped during constant evaluation, and non-throwing, like + /// `reportOverflow` -- see that function for why the `catch` is there. + static constexpr void reportClamp() noexcept { if (!std::is_constant_evaluated()) { - ::morph::log::logError("[Rational] an INT64_MIN component was clamped to -INT64_MAX; canonicalising " - "it would require negating a value with no positive counterpart."); + // NOLINTBEGIN(bugprone-empty-catch) + try { + ::morph::log::logError("[Rational] an INT64_MIN component was clamped to -INT64_MAX; " + "canonicalising it would require negating a value with no positive " + "counterpart."); + } catch (...) { + } + // NOLINTEND(bugprone-empty-catch) } } diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index b411bee6..fb725f4e 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -7,6 +7,8 @@ #include #include +#include +#include #include #include #include @@ -241,3 +243,29 @@ TEST_CASE("checked* still report rather than saturate, for callers that must not morph::log::LogLevel::error}; CHECK((whole(kMax) + whole(1)).numerator == kMax); } + +TEST_CASE("Saturating arithmetic is noexcept even when the log sink throws", + "[rational][checked][saturate]") { + // morph::log offers no noexcept guarantee (morph#158): a user-installed + // sink may throw, and detail::log's own scoped_lock may throw + // std::system_error. An arithmetic operator must not start failing because + // logging failed, so the reporters swallow. Without that, these operators + // could not carry noexcept -- and a throw from one would terminate. + static_assert(noexcept(std::declval() += std::declval())); + static_assert(noexcept(std::declval() -= std::declval())); + static_assert(noexcept(std::declval() *= std::declval())); + + const morph::log::ScopedLoggerOverride hostile{ + [](morph::log::LogLevel, std::string_view) { throw std::runtime_error{"sink failed"}; }, + morph::log::LogLevel::error}; + + // Each of these overflows, so each reaches the (throwing) sink. + CHECK_NOTHROW([] { return whole(kMax) + whole(1); }()); + CHECK_NOTHROW([] { return whole(kMin + 1) - whole(2); }()); + CHECK_NOTHROW([] { return whole(kMax) * whole(3); }()); + // And the INT64_MIN clamp path in canonicalise(). + CHECK_NOTHROW([] { return Rational{Numerator{kMin}, Denominator{1}, DecimalPlaces{2}}; }()); + + // Still saturated correctly despite the sink failing. + CHECK((whole(kMax) + whole(1)).numerator == kMax); +} From 944575d8066234fd7ae756071f090c4238ccd775 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 22 Aug 2026 23:19:15 +0300 Subject: [PATCH 5/5] math: compare orderings with std::is_lt/is_gt, not against literal 0 The clang legs failed with -Wzero-as-null-pointer-constant: comparing a std::strong_ordering against 0 makes the literal a null-pointer constant under -Weverything, which this repository builds with. My local test build does not apply that flag set, so it passed here and failed on all 17 CI legs. std::is_lt/std::is_gt is also what operator<=> in this same file already uses, so this matches the house idiom rather than inventing one. Co-Authored-By: Claude Opus 5 (1M context) --- include/morph/util/rational.hpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index f7ef55f6..5ab61c57 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -611,10 +611,13 @@ void setWire(Wire wire) noexcept { /// @return `-1`, `0` or `1`. [[nodiscard]] constexpr int compareForSaturation(const Rational& rhs, bool addition) const noexcept { const auto ordering = addition ? (*this <=> -rhs) : (*this <=> rhs); - if (ordering < 0) { + // std::is_lt/is_gt, not `ordering < 0`: comparing an ordering against + // the literal 0 trips -Wzero-as-null-pointer-constant under + // -Weverything, which this repository builds with. + if (std::is_lt(ordering)) { return -1; } - return ordering > 0 ? 1 : 0; + return std::is_gt(ordering) ? 1 : 0; } /// @brief Clamps this value to the largest representable magnitude with