Fix 32-bit Int overflow in Rational add/sub/mul/div - #10
Conversation
add, sub, mul, and div formed their naive cross-products (na*db, da*db, na*nb, ...) before any reduction, which only happened later inside new. Since Int is 32-bit, denominators or numerators whose product exceeds 2^31 overflowed and produced wrong fractions even when the fully-reduced result was perfectly representable, e.g. (add (new 1 46341) (new 1 46341)) corrupted the tiny 2/46341. Cancel common factors first using the existing gcd: add/sub reduce the denominators to their lcm before combining; mul and div cross-cancel each numerator against the opposing denominator. Results are unchanged for all non-overflowing inputs (still normalized through new).
There was a problem hiding this comment.
Build & Tests
Checked out claude/fix-arithmetic-overflow at 4d9c1d5 (== PR head SHA; merge-base = current origin/master HEAD 19b8456, so no stale-branch skew). Full CI gate locally:
carp -x tests/rational.carp— 132 / 0.angler,carp-fmt --check,carp -x gendocs.carp— all clean. Matches green CI (ubuntu + macos).
Findings
I verified the fix is value-preserving algebraically, not just green:
- add / sub:
g = gcd(da,db), denominatorda*qb = da*db/g = lcm(da,db). Sincelcm/da = qbandlcm/db = qa, the numeratorna*qb ± nb*qais exactly the naive numerator over the true LCD. Intermediates shrink to the LCM whenever the denominators share a factor (the reported1/46341 + 1/46341case: denom stays46341, not46341²), and collapse to the oldda*dbonly when they're coprime — which is unavoidable and still correct. - mul / div: the two
gcds cross-cancel, and theg1*g2factor divides out of both numerator and denominator, so the value equalsna*nb/(da*db)(mul) /na*db/(da*nb)(div) exactly. Traced each/— every divisor divides its dividend evenly (eachgis a gcd of one of the operands), so no truncation, and each result still flows throughnewfor final reduction + sign normalization. Outputs are therefore byte-identical to master for non-overflowing inputs.
Beyond the suite I ran a 19-case probe (zero operands to all four ops, div-by-zero sign, negatives/sign-normalization, and exact reduced results on the overflow-avoiding paths) — all pass. Notably mul/div with a zero operand don't divide by a zero gcd: gcd(0,x)=x, so the cancelling divisions stay well-defined and div(±n, 0) still yields ±1/0 with the correct sign.
Two non-blocking notes (both pre-existing and out of this PR's scope, flagging only for a future pass):
modulo(rational.carp:264) still forms the naive(/ (* na db) (* nb da))quotient, so it retains the same 32-bit overflow this PR fixes for the four core ops.<(rational.carp:274) compares naive cross-products(* na db)vs(* nb da), which can likewise overflow for large denominators and silently misorder.- The one input that still traps is
div(0/1, 0/1)→0/0integer division; I confirmed master crashes identically (itsnew(0,0)hits the samegcd(0,0)=0then/0), so this is not a regression — just a latent, mathematically-undefined case.
Verdict: merge
The overflow bug is a real silent-wrong-answer defect on core arithmetic; the fix is correct, exactly value-preserving, tightly scoped, and green on every gate. It's currently a draft — leaving the un-draft/merge call to you.
Fix 32-bit
Intoverflow in coreRationalarithmeticadd,sub,mul, anddivformed their naive cross-products before anyreduction — reduction only happened later inside
new. BecauseIntis 32-bit,an intermediate product exceeding 2³¹ overflowed and produced a wrong
fraction, even when the fully-reduced answer was perfectly representable.
Concretely, on
master:Fix
Cancel common factors first, using the existing private
gcd:g = gcd(da, db), common denominatorda*(db/g), numeratorna*(db/g) ± nb*(da/g). Intermediates stay as small as the reduceddenominators allow.
g1 = gcd(na, db),g2 = gcd(nb, da).g1 = gcd(na, nb),g2 = gcd(da, db).Every result still passes through
new, so sign normalization and finalreduction are unchanged. Outputs are byte-identical to
masterfor allnon-overflowing inputs; div-by-zero behaviour (
±1/0) is preserved. No publicsignature changes.
Tests
Added five cases to
tests/rational.carpthat fail onmasterand pass here —one per operation, each with intermediate products that overflow 2³¹ but a small
representable reduced result (e.g.
mul/divof denominators near√(2³¹)thatcross-cancel to
4/9). All 132 tests pass;carp-fmt -candanglerare clean.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.