From 9595fa5a7da2def55c440b1e192091a1cf1c4620 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 13:34:59 +0000 Subject: [PATCH] Stop Duration arithmetic from wrapping silently on int overflow [patch] Duration.Add multiplied the two denominators before reducing, so a running sum such as Progression.TotalDuration grew as the product of every distinct denominator and wrapped once it passed int.MaxValue, reporting a wrong and often negative length. Add now works over the least common denominator, and Add, Multiply, Dotted and Create compute in long and narrow with checked casts. A result that fits once reduced is now returned exactly; one that cannot be held as an int fraction throws OverflowException instead of wrapping. Create no longer trips over Math.Abs(int.MinValue). Fixes #306 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016cMuggtJH1AeNFWqHHnG2h --- Semantics.Music/Duration.cs | 40 +++++++++++++----- Semantics.Test/Music/DurationTests.cs | 60 +++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/Semantics.Music/Duration.cs b/Semantics.Music/Duration.cs index 05f65c8b..722929d3 100644 --- a/Semantics.Music/Duration.cs +++ b/Semantics.Music/Duration.cs @@ -39,6 +39,7 @@ public sealed record Duration /// The denominator; must be non-zero. /// A new, reduced duration with a positive denominator. /// Thrown when is 0. + /// Thrown when the reduced fraction does not fit in , as for int.MinValue/-1. public static Duration Create(int numerator, int denominator) { if (denominator == 0) @@ -46,32 +47,37 @@ public static Duration Create(int numerator, int denominator) throw new ArgumentOutOfRangeException(nameof(denominator), "Denominator must be non-zero."); } - int sign = denominator < 0 ? -1 : 1; - int n = numerator * sign; - int d = denominator * sign; - int g = Gcd(Math.Abs(n), d); - g = g == 0 ? 1 : g; - return new() { Numerator = n / g, Denominator = d / g }; + return Reduce(numerator, denominator); } /// Returns the sum of this duration and another. /// The duration to add. /// The summed duration. /// Thrown when is null. + /// Thrown when the reduced sum does not fit in . public Duration Add(Duration other) { Ensure.NotNull(other); - return Create((Numerator * other.Denominator) + (other.Numerator * Denominator), Denominator * other.Denominator); + + // Over the least common denominator, so a run of sums grows only as fast as the lcm of the + // denominators seen, not their product. Every intermediate fits in long because each factor + // is an int. + long g = Gcd(Denominator, other.Denominator); + long numerator = (Numerator * (other.Denominator / g)) + (other.Numerator * (Denominator / g)); + long denominator = Denominator / g * other.Denominator; + return Reduce(numerator, denominator); } /// Returns this duration multiplied by an integer factor. /// The integer factor. /// The scaled duration. - public Duration Multiply(int factor) => Create(Numerator * factor, Denominator); + /// Thrown when the reduced product does not fit in . + public Duration Multiply(int factor) => Reduce((long)Numerator * factor, Denominator); /// Returns this duration with a single augmentation dot (×3/2). /// The dotted duration. - public Duration Dotted() => Create(Numerator * 3, Denominator * 2); + /// Thrown when the reduced result does not fit in . + public Duration Dotted() => Reduce((long)Numerator * 3, (long)Denominator * 2); /// Parses a fraction "n/d". /// The fraction text. @@ -120,7 +126,21 @@ public static bool TryParse(string? text, [System.Diagnostics.CodeAnalysis.NotNu public override string ToString() => $"{Numerator.ToString(CultureInfo.InvariantCulture)}/{Denominator.ToString(CultureInfo.InvariantCulture)}"; - private static int Gcd(int a, int b) + /// + /// Reduces a fraction computed in and narrows it to , throwing + /// rather than wrapping when it does not fit. + /// + private static Duration Reduce(long numerator, long denominator) + { + long sign = denominator < 0 ? -1 : 1; + long n = numerator * sign; + long d = denominator * sign; + long g = Gcd(Math.Abs(n), d); + g = g == 0 ? 1 : g; + return new() { Numerator = checked((int)(n / g)), Denominator = checked((int)(d / g)) }; + } + + private static long Gcd(long a, long b) { while (b != 0) { diff --git a/Semantics.Test/Music/DurationTests.cs b/Semantics.Test/Music/DurationTests.cs index cefa849f..ae375fb0 100644 --- a/Semantics.Test/Music/DurationTests.cs +++ b/Semantics.Test/Music/DurationTests.cs @@ -42,6 +42,66 @@ public void Add_SumsFractions() Assert.AreEqual(Duration.Create(3, 4), Duration.Half.Add(Duration.Quarter)); } + [TestMethod] + public void Add_TenTupletDurations_ThrowsOverflowInsteadOfWrapping() + { + // 1/3 + 1/5 + ... + 1/31 is exactly n/100280245065, which no int fraction can hold. It used + // to wrap to -505842482/1495997257 and report a negative total. + int[] denominators = [3, 5, 7, 11, 13, 17, 19, 23, 29, 31]; + Duration total = Duration.Create(0, 1); + _ = Assert.ThrowsExactly(() => + { + foreach (int denominator in denominators) + { + total = total.Add(Duration.Create(1, denominator)); + } + }); + Assert.IsTrue(total.Numerator > 0, $"A partial sum wrapped to {total}"); + } + + [TestMethod] + public void Add_WhoseCommonDenominatorFits_SumsExactly() + { + // The product of the denominators passes int.MaxValue but their lcm does not, so the sum is + // representable. Multiplying the denominators first wrapped these. + Assert.AreEqual(Duration.Create(1, 32768), Duration.Create(1, 65536).Add(Duration.Create(1, 65536))); + + Duration eightTuplets = Duration.Create(0, 1); + foreach (int denominator in new[] { 3, 5, 7, 11, 13, 17, 19, 23 }) + { + eightTuplets = eightTuplets.Add(Duration.Create(1, denominator)); + } + + Duration total = eightTuplets.Add(Duration.Create(1, 3 * 5 * 7 * 11 * 13 * 17 * 19)); + Assert.AreEqual(0, 111546435 % total.Denominator, $"{total} should be over a divisor of the lcm"); + Assert.AreEqual(eightTuplets.AsWholeNotes + (1.0 / 4849845), total.AsWholeNotes, 1e-12); + } + + [TestMethod] + public void Add_SharedDenominators_StayReduced() + { + // Mixing triplets and plain values reuses denominators, so the sum stays over their lcm. + Duration total = Duration.Create(1, 12).Add(Duration.Create(1, 6)).Add(Duration.Create(1, 4)).Add(Duration.Create(1, 12)); + Assert.AreEqual(Duration.Create(7, 12), total); + } + + [TestMethod] + public void Arithmetic_ThatDoesNotFitInInt_ThrowsOverflow() + { + _ = Assert.ThrowsExactly(() => Duration.Create(2, 1).Multiply(int.MaxValue)); + _ = Assert.ThrowsExactly(() => Duration.Create(int.MaxValue, 1).Dotted()); + _ = Assert.ThrowsExactly(() => Duration.Create(1, int.MaxValue).Add(Duration.Create(1, int.MaxValue - 1))); + _ = Assert.ThrowsExactly(() => Duration.Create(int.MinValue, -1)); + } + + [TestMethod] + public void Arithmetic_WhoseReducedResultFits_DoesNotThrow() + { + // The unreduced product passes int.MaxValue, but the reduced result does not. + Assert.AreEqual(Duration.Create(1, 1), Duration.Create(1, int.MaxValue).Multiply(int.MaxValue)); + Assert.AreEqual(Duration.Create(int.MinValue, 1), Duration.Create(int.MinValue, 1)); + } + [TestMethod] public void Create_RejectsZeroDenominator() {