diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 6011cd9a..b2e2222e 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -183,6 +183,69 @@ 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, +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. + +**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 +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 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. + +**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`). + +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) Whenever an arithmetic expression contains an @@ -298,6 +361,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..5ab61c57 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 @@ -160,7 +162,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 +189,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{}; @@ -401,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(); + 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(); + if (subWouldOverflow(rhs)) { + reportOverflow("operator-="); + saturateToward(compareForSaturation(rhs, false), rhs.decimalPlaces); + return *this; + } + subAssignUnchecked(rhs); return *this; } @@ -432,19 +498,18 @@ struct Rational { /// Cross-cancels common factors before multiplying. /// @param rhs Value to multiply by. /// @return `*this`. + /// + /// Saturates rather than overflowing; see `operator+=`. 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(); + 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; } @@ -488,6 +553,197 @@ 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. + /// + /// 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) noexcept { + if (!std::is_constant_evaluated()) { + // 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, and non-throwing, like + /// `reportOverflow` -- see that function for why the `catch` is there. + static constexpr void reportClamp() noexcept { + if (!std::is_constant_evaluated()) { + // 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) + } + } + + /// @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); + // 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 std::is_gt(ordering) ? 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` @@ -497,12 +753,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; @@ -607,6 +886,65 @@ static_assert(std::is_standard_layout_v); return lhs.dividedBy(rhs); } +/// @brief Adds two Rationals, reporting overflow instead of saturating. +/// +/// `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. +/// +/// 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. +/// @param rhs Right addend. +/// @return The exact sum, or `unexpected(RationalError::Overflow)`. +[[nodiscard]] constexpr std::expected checkedAdd(const Rational& lhs, + const Rational& rhs) noexcept { + if (lhs.addWouldOverflow(rhs)) { + return std::unexpected(RationalError::Overflow); + } + auto result = lhs; + result.addAssignChecked(rhs); + return result; +} + +/// @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 { + if (lhs.subWouldOverflow(rhs)) { + return std::unexpected(RationalError::Overflow); + } + auto result = lhs; + result.subAssignChecked(rhs); + return result; +} + +/// @brief Multiplies two Rationals, reporting overflow instead of saturating. +/// +/// Checks the cross-cancelled factors `operator*` actually multiplies, not the +/// 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 { + if (lhs.mulWouldOverflow(rhs)) { + return std::unexpected(RationalError::Overflow); + } + auto result = lhs; + result.mulAssignChecked(rhs); + return result; +} + // --------------------------------------------------------------------------- // 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..fb725f4e --- /dev/null +++ b/tests/test_rational_checked.cpp @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Rational's checked arithmetic: report overflow instead of committing it. + +#include + +#include + +#include +#include +#include +#include +#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)); + + // 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); +} + +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); +} + +// --------------------------------------------------------------------------- +// 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); +} + +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); +}