math: make Rational arithmetic total — saturating operators plus checked add/sub/mul

[Yaraslaut]

Refs #130. Also closes the UB in #156.

Rational's arithmetic was fixed-width int64 that neither saturated nor reported failure. At ledger-realistic magnitudes that's reachable — summing dp=2 legs of 10⁹ minor units overflows at exactly INT64_MAX / 10⁹ + 1 rows — and signed overflow is undefined behaviour, not a wrong-but-detectable answer.

Revised after review

The first version added checkedAdd/checkedSub/checkedMul alongside the operators. As you pointed out, that left the type still misusable into UB: opt-in safety isn't safety, because the operators were unchanged and anyone using + still got UB.

The operators now delegate to the same checks. operator+=/-=/*= run the overflow predicate first; on overflow they log at error and saturate to the largest representable magnitude of the correct sign, instead of overflowing.

The sign comes from exact comparison, not the operands' magnitudes — a + b compares a against -b using 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:

expression	result
INT64_MAX + 1	+INT64_MAX
-INT64_MAX - 2	-INT64_MAX
INT64_MAX * 3	+INT64_MAX
INT64_MAX * -3	-INT64_MAX
1/INT64_MAX + 1/(INT64_MAX-1)	+INT64_MAX (tiny positive value, unrepresentable common denominator)

Saturating rather than throwing because these operators run inside strand-bound model code and in constexpr expressions: 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 it is defined wrong, and it is logged.

Division of labour

The operators stay usable and defined for code that can absorb a clamped value. checkedAdd/checkedSub/checkedMul stay exact-or-nothing for code that must not — a ledger totalling rows needs to stop, not carry on with a clamped balance. Both share one set of predicates (addWouldOverflow, subWouldOverflow, mulWouldOverflow), so the operators and the checked forms cannot disagree about what overflows.

Fixing the operators exposed the same bug one level down

canonicalise() negated the numerator unguarded, so an INT64_MIN component was UB — and it 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; the UB was in canonicalising the perfectly valid result.

It now clamps such a component to -INT64_MAX and logs, matching what setWire already did for the same values arriving off the wire, and takes magnitudes through the existing detail::absU64 helper so no negation can overflow. That resolves #156's UB, choosing its option 1 (clamp, mirroring setWire) — worth a look, since the issue listed three and this picks one.

Verification

Under UBSan, every case that previously reported signed integer overflow or negation of -9223372036854775808 now logs and saturates with no diagnostic at all.

Tests assert the saturated values, the sign in each direction, the intermediate-only overflow case, that ordinary arithmetic is undisturbed, and — via ScopedLoggerOverride — that the log actually fires.

Full suite 20,206 assertions / 1,092 cases. gcc -Wall -Wextra -Werror and clang -Wdocumentation both clean locally.

noexcept is preserved (from your review)

The operators keep noexcept, including on the logging path. That needs a local try/catch around the log call, because morph::log offers no noexcept guarantee — docs/spec/core/logger.md 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. An arithmetic operator must not begin failing because logging failed.

CompletionState's destructor already carries the identical workaround, with a bugprone-empty-catch suppression and the comment "logError may throw; we swallow to avoid noexcept-escape" — so this is the second site paying the same per-call-site tax. Filed as #158: the logging layer should offer the guarantee once. Both workarounds come out if that lands.

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.

No VERSIONING.md change is needed as a result — the operators' exception specification is unchanged from master.

[Yaraslaut]

I think that we need to use his functions in the arithmetic operations for the Rational and safely unwrap them to provide valid results, we want to make save this type, while this addition adds save functionality it is still possible to misuse and get UB using this type, we should deligate to this functions from the arithmetic operations and log when it is failing

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
include/morph/util/rational.hpp	90.47%	2 Missing and 12 partials ⚠️

📢 Thoughts on this report? Let us know!

[Yaraslaut]

Fixed the gcc/MSVC/Valgrind failures — the test was wrong, and it found something

Those legs aborted on checkedSub mirrors checkedAdd while clang passed. checkedSub was not at fault: my test constructed Rational{Numerator{INT64_MIN}, ...}, and that is undefined behaviour on its own — canonicalise() negates the numerator unguarded, before any arithmetic runs.

Isolated with UBSan down to construction alone, nothing else in the program:

rational.hpp:562: runtime error: negation of -9223372036854775808
  cannot be represented in type 'std::int64_t'

setWire guards exactly this case on the wire path — its doc comment names "INT64_MIN components whose negation would overflow" — but the public constructor does not. Filed separately as #156, since it is a distinct pre-existing defect: #130 is about arithmetic overflowing, #156 is about a value that cannot be constructed at all.

This PR now only stops the test depending on it: kMin + 1 with a difference of 2 (since kMin + 1 - 1 is exactly INT64_MIN, which still fits, 2 is the first difference that genuinely does not). Verified under UBSan — clean, and checkedSub reports Overflow as intended.

Worth noting the shape of this: the checked-arithmetic work is what surfaced a UB hole in the constructor, which no amount of arithmetic testing would have found.