Stop Duration arithmetic from wrapping silently on int overflow [patch]

Semantics.Music/Duration.cs

@@ -39,39 +39,45 @@ public sealed record Duration
 	/// <param name="denominator">The denominator; must be non-zero.</param>
 	/// <returns>A new, reduced duration with a positive denominator.</returns>
 	/// <exception cref="ArgumentOutOfRangeException">Thrown when <paramref name="denominator"/> is 0.</exception>
+	/// <exception cref="OverflowException">Thrown when the reduced fraction does not fit in <see cref="int"/>, as for <c>int.MinValue/-1</c>.</exception>
 	public static Duration Create(int numerator, int denominator)
 	{
 		if (denominator == 0)
 		{
 			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);
 	}
 
 	/// <summary>Returns the sum of this duration and another.</summary>
 	/// <param name="other">The duration to add.</param>
 	/// <returns>The summed duration.</returns>
 	/// <exception cref="ArgumentNullException">Thrown when <paramref name="other"/> is null.</exception>
+	/// <exception cref="OverflowException">Thrown when the reduced sum does not fit in <see cref="int"/>.</exception>
 	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);
 	}
 
 	/// <summary>Returns this duration multiplied by an integer factor.</summary>
 	/// <param name="factor">The integer factor.</param>
 	/// <returns>The scaled duration.</returns>
-	public Duration Multiply(int factor) => Create(Numerator * factor, Denominator);
+	/// <exception cref="OverflowException">Thrown when the reduced product does not fit in <see cref="int"/>.</exception>
+	public Duration Multiply(int factor) => Reduce((long)Numerator * factor, Denominator);
 
 	/// <summary>Returns this duration with a single augmentation dot (×3/2).</summary>
 	/// <returns>The dotted duration.</returns>
-	public Duration Dotted() => Create(Numerator * 3, Denominator * 2);
+	/// <exception cref="OverflowException">Thrown when the reduced result does not fit in <see cref="int"/>.</exception>
+	public Duration Dotted() => Reduce((long)Numerator * 3, (long)Denominator * 2);
 
 	/// <summary>Parses a fraction "n/d".</summary>
 	/// <param name="text">The fraction text.</param>
@@ -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)
+	/// <summary>
+	/// Reduces a fraction computed in <see cref="long"/> and narrows it to <see cref="int"/>, throwing
+	/// rather than wrapping when it does not fit.
+	/// </summary>
+	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)
 		{

Semantics.Test/Music/DurationTests.cs

@@ -42,6 +42,66 @@
 		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<OverflowException>(() =>
+		{
+			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<OverflowException>(() => Duration.Create(2, 1).Multiply(int.MaxValue));
+		_ = Assert.ThrowsExactly<OverflowException>(() => Duration.Create(int.MaxValue, 1).Dotted());
+		_ = Assert.ThrowsExactly<OverflowException>(() => Duration.Create(1, int.MaxValue).Add(Duration.Create(1, int.MaxValue - 1)));
+		_ = Assert.ThrowsExactly<OverflowException>(() => 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()
 	{