fix(util): handle INT64_MIN rational operations by TayfurYldz · Pull Request #561 · LASTRADA-Software/morph

TayfurYldz · 2026-09-17T20:48:15Z

Summary

Fixes #537.

Rational could retain or observe INT64_MIN through the whole-integer constructor or its public numerator, then invoke undefined signed negation in unary negation, reciprocal, absolute value, and multiplication cross-cancellation.

This change:

routes the whole-integer constructor through canonicalisation;
handles INT64_MIN explicitly in unary negation, reciprocal, and abs;
uses unsigned magnitude for multiplication cross-cancellation and overflow checks;
keeps the existing saturation policy (INT64_MIN magnitude clamps to INT64_MAX);
updates the existing formatter regression to construct a deliberately poisoned public numerator directly.

Validation

Current master (df2bdb52bb521689b5a7bb2ca7a7274f0bb22869) UBSan repro: fails at rational.hpp signed negation with INT64_MIN.
Same UBSan repro after the patch: passes with no UBSan report.
morph_tests '[rational][checked]': 26 test cases / 121 assertions passed.
morph_tests '[rational]': 80 test cases / 533 assertions passed.
Non-strict GCC build of morph_tests: passed.
git diff --check: passed.

The repository's strict gcc-debug preset could not complete on this environment before reaching these tests because GCC 15 promotes a vendored Catch2 -Wctor-dtor-privacy diagnostic to an error. That failure reproduces outside this patch; the non-strict build was used for the affected Catch2 suites above.

Copilot

🟡 Changes recommended

Checked arithmetic can still return inexact success values, and reciprocal retains a signed-overflow path.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens Rational operations against INT64_MIN and aligns tests and documentation with saturation behavior.

Changes:

Canonicalizes whole-integer construction.
Uses safe magnitude handling in arithmetic.
Adds regression coverage and updates documentation.

File summaries

File	Description
tests/test_rational_checked.cpp	Adds `INT64_MIN` arithmetic regressions.
tests/test_quantity.cpp	Updates the formatter regression setup.
include/morph/util/rational.hpp	Hardens construction and arithmetic.
docs/spec/util/rational.md	Documents `INT64_MIN` handling.
CHANGELOG.md	Records the fix.

Review details

Files reviewed: 5/5 changed files
Comments generated: 5
Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+            constexpr auto minValue = std::numeric_limits<std::int64_t>::min();
+            constexpr auto maxValue = std::numeric_limits<std::int64_t>::max();
+            auto const reciprocalDenominator = numerator == minValue ? maxValue : -numerator;
+            return Rational{Numerator{-denominator}, Denominator{reciprocalDenominator}, decimalPlaces};


+            auto const reciprocalDenominator = numerator == minValue ? maxValue : -numerator;
+            return Rational{Numerator{-denominator}, Denominator{reciprocalDenominator}, decimalPlaces};


+        return detail::mulOverflows(numerator / signedCrossDivisorOne, rhs.numerator / signedCrossDivisorTwo) ||
+               detail::mulOverflows(denominator / signedCrossDivisorTwo, rhs.denominator / signedCrossDivisorOne);


+**`INT64_MIN` handling.** `INT64_MIN` (`-2^63`) has no positive counterpart
+in `int64`, so direct signed negation is undefined. The whole-integer
+constructor therefore delegates to the canonicalising constructor and clamps
+`INT64_MIN` to `-INT64_MAX`, matching the full constructor and wire path.


+    CHECK((raw * whole(2)).numerator == -kMax);
+    const auto checked = checkedMul(raw, whole(2));
+    REQUIRE_FALSE(checked.has_value());
+    CHECK(checked.error() == RationalError::Overflow);


Yaraslaut · 2026-09-19T19:35:25Z

Status note, so this is recorded rather than silently stalled: this PR has not run CI yet.

All six workflows are sitting in action_required rather than having failed —GitHub holds workflow runs on pull requests from forks until a maintainer approves them:

$ gh api "repos/LASTRADA-Software/morph/actions/runs?head_sha=edf1a5e8..."
6
CI              completed/action_required
Drift guard     completed/action_required
Docs            completed/action_required
Spec sync       completed/action_required
WASM ladder gate completed/action_required
WASM Demo       completed/action_required

That is why the PR shows no checks at all — gh pr checks reports "no checks reported on the branch", which reads like a CI outage but is not one. Nothing is wrong with the branch; it is MERGEABLE against current master.

Approval is pending with a maintainer. No action is needed from you, and there is no point pushing again to try to trigger the runs — a new push lands in the same held state.

For the record, a read of the diff found nothing that would block approval: it touches no .github/ workflow files and adds no scripts. The one non-source change, scripts/branch_partial_allowlist.json, is line-number bookkeeping (271→276, 697→717, 731→751, 757→777) tracking rational.hpp growing by +101/-27, plus one cross-reference in a reason string updated to match. No new allowlist entries and no newly suppressed branches — which is the thing worth checking in a coverage allowlist, and it is clean.

`abs()`'s new saturating ternary was wrapped by hand and does not match `.clang-format`, so the whole-tree `clang-format` gate fails on it. CI pins clang-format to CLANG_VERSION 22 and checks every tracked `.hpp`/`.cpp`, not just the changed ones, so this blocks the PR regardless of its own content. Formatting only -- reflows one expression onto two lines. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY

Addresses the review on morph#561. The first pass hardened a poisoned `INT64_MIN` *numerator* and left three sibling paths reachable through a poisoned `denominator`, which the same threat model admits since the members are public. - `reciprocal()` no longer sign-corrects by hand. It hands the inverted pair to the canonicalising constructor, which clamps an `INT64_MIN` component *before* moving a negative denominator's sign onto the numerator. The previous `-denominator` was undefined for a poisoned denominator -- the exact bug this PR exists to remove, in the function next door. - `mulAssignUnchecked` and `mulWouldOverflow` cross-cancelled through `static_cast<std::uint64_t>(denominator)`. On master these were `std::gcd(int64, int64)`, which takes `|m|, |n|` internally, so the cast turned a negative denominator into a huge magnitude and a different gcd -- wrong arithmetic where master was right. Now `detail::absU64`. - `mulWouldOverflow` reported a reduced product of exactly `INT64_MIN` as representable, so `checkedMul` could return a success that `canonicalise` had already changed to `-INT64_MAX`. It is now an overflow. This needs no poisoned operand: `(-2^62) * 2` reaches it. - `checkedDiv` declines a divisor carrying a poisoned component, before forming the reciprocal. `reciprocal()` must clamp such a component, so what it returns is not that divisor's inverse and the product would be an *inexact* value handed back as a success -- which the exact-or-`Overflow` contract forbids. The zero divisor is still reported ahead of it. The review suggested instead carrying the unsigned `2^63` magnitude through division's cross-cancellation, so that `2 / (INT64_MIN/1)` returns its exact `-1/2^62`. Declined deliberately: that would make `checkedDiv` accept quotients `dividedBy` still saturates, breaking the documented guarantee that the operators and the checked forms never disagree about which quotients fit. The conservative answer costs nothing for any value the type can produce, since every constructor canonicalises. Six tests. Five are regression tests, each verified to fail against the unfixed implementation. The sixth -- the overflowing addition with a poisoned addend, which the review asked for -- **passes without the fix**: addition was not changed here, so it pins pre-existing saturation behaviour rather than this change. Kept for the coverage the review wanted, and labelled honestly rather than counted as a regression test. Documentation updated across `rational.hpp`, `rational.md`, `quantity.hpp` and `quantity_type.md`, which still described the pre-fix behaviour. Refs LASTRADA-Software#537 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY

…ne shifts The eight `rational.hpp` entries all named lines that had moved, and the two `if (!std::is_constant_evaluated())` entries had become mutually ambiguous -- the checker disambiguates identical source text by the recorded line, so stale hints make it unable to tell them apart. Re-pinned to 276/717/751/777/795/929/ 1481/1572, and the three in-prose cross references in the reason strings now name the new lines too. No entry was added, removed, or had its reason changed: this is the mechanical consequence of the edits above it in the file. Refs LASTRADA-Software#537 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY

TayfurYldz · 2026-09-24T21:30:56Z

Rebased this onto current master (the branch had fallen 79 commits behind) and resolved the documentation, quantity formatter, and coverage-allowlist conflicts against the newer upstream wording/line layout. I preserved the current-master documentation where it had superseded the older text and retained the PR-specific INT64_MIN/checkedDiv behavior. git diff --check is clean. I have not claimed a local full build because this fresh worktree has no build dependencies/artifacts installed; CI will be the validation gate.

TayfurYldz · 2026-09-24T22:32:10Z

Rebase is complete and the current branch also includes the follow-up hardening for the earlier Copilot findings: reciprocal no longer performs signed negation on a poisoned INT64_MIN denominator, checkedMul rejects reduced products that land exactly on INT64_MIN, checkedDiv rejects poisoned components before consuming a clamped reciprocal, and the regression/spec comments were expanded for those paths. Could you run a fresh review against the rebased head when convenient?

morph PR LASTRADA-Software/morph#561 fixed undefined signed negation on INT64_MIN in five places in its own Rational. formula-cpp's independent Rational is already correct at all five -- checked_negate guards the one unnegatable numerator, checked_reciprocal never negates at all, and checked_mul cross-reduces via detail::magnitude() in the unsigned domain -- but three of those four operations had no IntMin test, so nothing would have noticed if checked_mul's cross-reduction were "simplified" to `n < 0 ? -n : n`, which is exactly morph's bug. Add the missing static_assert/TEST_CASE pairs for checked_negate, checked_reciprocal and checked_mul (both operand sides, since this project has lost coverage before by testing only one side of a two-sided operation). Verified each is load-bearing by temporarily reintroducing the corresponding unsafe code and confirming the new tests fail to compile (constexpr evaluation of the resulting signed overflow is not a core constant expression), then restoring and confirming a clean pass. Signed-off-by: Christian Parpart <c.parpart@lastrada.net>

There was no -fsanitize anywhere in this repository -- not in CI, not in any preset, not in CMakeLists.txt -- so the class of undefined behaviour that produced morph PR LASTRADA-Software/morph#561 (unguarded signed negation of INT64_MIN) had no detector here at all, independent of how carefully anyone reads the code. Add a clang-ubsan configure/build/test preset (Debug, -fsanitize=undefined -fno-sanitize-recover=undefined) and a matching Linux-clang-ubsan leg in build.yml, reusing the existing Clang 22 install step. Verified locally with both the ambient clang and clang++-22 (matching CI exactly): the full suite builds and passes clean under the sanitizer, and a standalone repro confirms these flags do catch IntMin negation at runtime, aborting with "UndefinedBehaviorSanitizer: undefined-behavior" as expected. Signed-off-by: Christian Parpart <c.parpart@lastrada.net>

Yaraslaut requested a balanced review from Copilot September 18, 2026 04:48

Yaraslaut force-pushed the fix/537-rational-int64-min branch from 248f25a to 088e1f5 Compare September 18, 2026 04:48

Copilot started reviewing on behalf of Yaraslaut September 18, 2026 04:53 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

TayfurYldz and others added 4 commits September 25, 2026 00:29

fix(util): handle INT64_MIN rational operations

bdfb5a8

TayfurYldz force-pushed the fix/537-rational-int64-min branch from edf1a5e to 31675fd Compare September 24, 2026 21:30

Navigation Menu

fix(util): handle INT64_MIN rational operations - #561

fix(util): handle INT64_MIN rational operations#561
TayfurYldz wants to merge 4 commits into
LASTRADA-Software:masterfrom
TayfurYldz:fix/537-rational-int64-min

TayfurYldz commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Yaraslaut commented Sep 19, 2026

Uh oh!

TayfurYldz commented Sep 24, 2026

Uh oh!

TayfurYldz commented Sep 24, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		auto const reciprocalDenominator = numerator == minValue ? maxValue : -numerator;
		return Rational{Numerator{-denominator}, Denominator{reciprocalDenominator}, decimalPlaces};

		return detail::mulOverflows(numerator / signedCrossDivisorOne, rhs.numerator / signedCrossDivisorTwo) \|\|
		detail::mulOverflows(denominator / signedCrossDivisorTwo, rhs.denominator / signedCrossDivisorOne);

Conversation

TayfurYldz commented Sep 17, 2026

Summary

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Yaraslaut commented Sep 19, 2026

Uh oh!

TayfurYldz commented Sep 24, 2026

Uh oh!

TayfurYldz commented Sep 24, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants