Bug in Temporal.Duration.compare

[ptomato]

Found a case that breaks the invariant of comparing durations as equal relative to a ZonedDateTime:

// Note: 2020-11-01 is a 25-hour day in America/Vancouver time zone
relativeTo = Temporal.ZonedDateTime.from('2019-11-01T00:00-07:00[America/Vancouver]');
duration1 = Temporal.Duration.from({ years: 1, days: 1 })
duration2 = Temporal.Duration.from({ years: 1, hours: 25 })
relativeTo.add(duration1)  // => 2020-11-02T00:00:00-08:00[America/Vancouver]
relativeTo.add(duration2)  // => 2020-11-02T00:00:00-08:00[America/Vancouver]
// (therefore, the durations are equal relative to 'relativeTo')
Temporal.Duration.compare(duration1, duration2, { relativeTo }) // => -1; wrong, expected 0
Temporal.Duration.compare(duration1, { years: 1, hours: 24 }, { relativeTo }) // => 0; also wrong

[gibson042]

Found a case that breaks the invariant of comparing durations as equal relative to a ZonedDateTime:

// Note: 2020-11-01 is a 25-hour day in America/Vancouver time zone
relativeTo = Temporal.ZonedDateTime.from('2019-11-01T00:00-07:00[America/Vancouver]');
duration1 = Temporal.Duration.from({ years: 1, days: 1 })
duration2 = Temporal.Duration.from({ years: 1, hours: 25 })
z.add(duration1)  // => 2020-11-02T00:00:00-08:00[America/Vancouver]
z.add(duration2)  // => 2020-11-02T00:00:00-08:00[America/Vancouver]
// (therefore, the durations are equal relative to 'z')

Should these three zs be relativeTo?

Temporal.Duration.compare(duration1, duration2, { relativeTo }) // => -1; wrong, expected 0
Temporal.Duration.compare(duration1, { years: 1, hours: 24 }, { relativeTo }) // => 0; also wrong

👍

[ptomato]

Thanks, fixed in the example code.

[justingrant]

I didn't have time to PR this, but I know DST math pretty well so I'm adding notes here to explain the problem and suggest a fix to help out whoever ends up PR-ing this. First, here's the code with the bug.

proposal-temporal/polyfill/lib/duration.mjs

Lines 498 to 506 in 42c964e

	const shift1 = ES.CalculateOffsetShift(relativeTo, y1, mon1, w1, d1, h1, min1, s1, ms1, µs1, ns1);
	const shift2 = ES.CalculateOffsetShift(relativeTo, y2, mon2, w2, d2, h2, min2, s2, ms2, µs2, ns2);
	if (y1 !== 0 || y2 !== 0 || mon1 !== 0 || mon2 !== 0 || w1 !== 0 || w2 !== 0) {
	({ days: d1 } = ES.UnbalanceDurationRelative(y1, mon1, w1, d1, 'day', relativeTo));
	({ days: d2 } = ES.UnbalanceDurationRelative(y2, mon2, w2, d2, 'day', relativeTo));
	}
	ns1 = ES.TotalDurationNanoseconds(d1, h1, min1, s1, ms1, µs1, ns1, shift1);
	ns2 = ES.TotalDurationNanoseconds(d2, h2, min2, s2, ms2, µs2, ns2, shift2);
	return ES.ComparisonResult(ns1.minus(ns2).toJSNumber());

To make the bug apparent, I'll translate what the code is doing into English:

1. Calculate the change in offset from relativeTo until the end of the duration
2. Unbalance years/months/weeks/days into a total number of days (ignoring the time units)
3. Calculate the total number of nanoseconds by summing the negated offset shift from (1), the number of 24-hour days from (2), and the time units of the duration.
4. Return the comparison of nanoseconds from (3)

I assume that the intent was something like this: "Even though (3) assumes all days are 24 hours long, that's OK because we're capturing the offset shift in (1)." But this doesn't work because if you include the time units, the offset shifts is identical for the repro case because both durations end at the same exact time. The offset shift that matters here is the offset shift at the end of the date portion of the duration, not the offset shift at the end of the duration overall.

In general, all iCalendar-style duration math follows the same pattern, which is that durations are the sum of:

• days (assuming all days are 24 hours)
• offset shift at the end of the days portion (note that offset shift is reverse-sign, so this is a subtraction)
• time units

There's a bunch of ways to fix the bug, but here's one way that I think should work: (and that worked for your repro case and that passed all tests when I tried it)

    const shift1 = ES.CalculateOffsetShift(relativeTo, y1, mon1, w1, d1, 0, 0, 0, 0, 0, 0);
    const shift2 = ES.CalculateOffsetShift(relativeTo, y2, mon2, w2, d2, 0, 0, 0, 0, 0, 0);

With this change, (3) in the repro case is:

• Duration 1: 366 24-hour days + 0 hours - negative-1-hour offset shift
• Duration 2: 365 24-hour days + 25 hours - 0 offset shift

This now yields the correct result in (4) which is that these are identical-length durations.

BTW, the spec has the same bug so this is a normative change that can't be merged until after October's TC39 meeting.