Normative: Fix intermediate value in ZonedDateTime difference

[ptomato]

To find the difference between two ZonedDateTimes, we first find the calendar difference between their date parts. Then we find the time difference between an intermediate ZonedDateTime value (calculated by adding the calendar parts to the first ZonedDateTime) and the second ZonedDateTime.

Previously we would calculate the intermediate value by adding years, months, and weeks from the date difference, because the days were calculated as part of the time difference.

This was incorrect, because the intermediate value can shift if it falls in the middle of a DST transition. However, that could be on a completely different date than would be expected, leading to surprising results like this:

const duration = Temporal.Duration.from({ months: 1, days: 15, hours: 12 });
const past = Temporal.ZonedDateTime.from('2024-02-10T02:00[America/New_York]');
const future = past.add(duration);

duration  // => 1 month, 15 days, 12 hours
past.until(future, { largestUnit: 'months' })  // => 1 month, 15 days, 11 hours

This result would occur because of a DST change on 2024-03-10T02:00, unrelated to either the start date of 2024-02-10 or the end date of 2024-03-25. With this change, the intermediate value occurs on 2024-03-25T02:00 and would only shift if that was when the DST change occurred.

[codecov]

Codecov Report

Attention: Patch coverage is 95.52239% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.55%. Comparing base (4b476fa) to head (161cdbc).

Files	Patch %	Lines
polyfill/lib/ecmascript.mjs	95.52%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2760      +/-   ##
==========================================
- Coverage   96.61%   96.55%   -0.06%     
==========================================
  Files          23       23              
  Lines       12310    12336      +26     
  Branches     2271     2275       +4     
==========================================
+ Hits        11893    11911      +18     
- Misses        355      364       +9     
+ Partials       62       61       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Aditi-1400]

Looks good to me, thanks!

[arshaw]

Thank you so much for the hard work on this and everything else @ptomato !

As mentioned in one of my comments, this could result in a mixed-sign duration.

The solution entails making a tentative dtEnd, and if it results in mixed signs, retrying the computation by backing it up a day. Something like this:

// DifferenceZonedDateTime
// ...

let years, months, weeks, days
let norm // represents hours/minutes/seconds/etc
let dayCorrection = 0

while (true) { // breaking condition below
  let dtEndAttempt = [
    GetSlot(dtEnd, ISO_YEAR),
    GetSlot(dtEnd, ISO_MONTH),
    GetSlot(dtEnd, ISO_DAY),
    GetSlot(dtEnd, ISO_HOUR),
    GetSlot(dtEnd, ISO_MINUTE),
    GetSlot(dtEnd, ISO_SECOND),
    GetSlot(dtEnd, ISO_MILLISECOND),
    GetSlot(dtEnd, ISO_MICROSECOND),
    GetSlot(dtEnd, ISO_NANOSECOND),
  ]

  dtEndAttempt = addDays(dtEndAttemp, dayCorrection * -sign)

  ({ years, months, weeks, days } = DifferenceISODateTime(
    GetSlot(dtStart, ISO_YEAR),
    GetSlot(dtStart, ISO_MONTH),
    GetSlot(dtStart, ISO_DAY),
    GetSlot(dtStart, ISO_HOUR),
    GetSlot(dtStart, ISO_MINUTE),
    GetSlot(dtStart, ISO_SECOND),
    GetSlot(dtStart, ISO_MILLISECOND),
    GetSlot(dtStart, ISO_MICROSECOND),
    GetSlot(dtStart, ISO_NANOSECOND),
    ...dtEndAttempt,
    calendarRec,
    largestUnit,
    options
  ))
  
  let intermediateNs = AddZonedDateTime(
    start,
    timeZoneRec,
    calendarRec,
    years,
    months,
    weeks,
    days,
    TimeDuration.ZERO,
    dtStart
  );
  
  norm = TimeDuration.fromEpochNsDiff(ns2, intermediateNs);

  if (!hasSameSigns([years, months, weeks, days], norm)) {
    dayCorrection++
  } else {
    break
  }
}

return { years, months, weeks, days, norm };

[ptomato]

@arshaw That had been on my list to investigate, thank you very much for the proposed fix. I'll incorporate it soon.

[arshaw]

Forgot to add: instead of computing intermediateNs by calling AddZonedDateTime, it'd be fewer operations to just call GetInstantFor with dtEndAttempt

[ptomato]

@arshaw I agree largely with your suggestion. Instead of AddDays I used BalanceISODate. I also suspect that we can assert that dayCorrection is never larger than 1. I've updated the PR assuming that, although I will keep investigating tomorrow whether it's possible to create a contrary example with a contrived calendar and time zone.

I was a bit surprised that we no longer needed to call NormalizedTimeDurationToDays there, but it makes sense. NormalizedTimeDurationToDays is in fact a 1-day backoff itself, but with a bunch of extra calculations tacked on the end that we don't need here.

However,

Forgot to add: instead of computing intermediateNs by calling AddZonedDateTime, it'd be fewer operations to just call GetInstantFor with dtEndAttempt

...that I'm not sure I agree with. dtEndAttempt wouldn't be the same as intermediateNs because intermediateNs is calculated by adding the date duration to dtStart, not dtEnd. Neither could you calculate it by calling GetInstantFor with the date part of dtEndAttempt and the time part of dtStart, because then you'd be off by 1 day sometimes.

[ptomato]

I also suspect that we can assert that dayCorrection is never larger than 1. I've updated the PR assuming that, although I will keep investigating tomorrow whether it's possible to create a contrary example with a contrived calendar and time zone.

@anba's test case from #2779 actually worked perfectly for this. It's possible to do just by overriding the calendar's dateUntil to return the negation of what it's supposed to return. I'd propose that we throw a RangeError in this case, as we do for other inconsistent values returned from calendar and time zone methods.

[arshaw]

Hi @ptomato,

Instead of AddDays I used BalanceISODate

Yup, "AddDays" was a fictional pseudocode function of mine. What you're doing with BalanceISODate is exactly what I'd expect.

I also suspect that we can assert that dayCorrection is never larger than 1... @anba's test case from #2779 actually worked perfectly for this...

Sounds good!

...that I'm not sure I agree with. dtEndAttempt wouldn't be the same as intermediateNs because intermediateNs is calculated by adding the date duration to dtStart, not dtEnd...

True. This suggestion is not applicable to your code as it stands. My polyfill's implementation of DifferenceZonedDateTime is a bit different, and I realized I was getting the two mixed up, apologies. Speaking of which, a had a chance to further compare to the two algorithms and realized a few things:

1. Potential Bug

Imagine diffing these two ZonedDateTimes:

2000-09-01T02:00:00[America/Something]
2001-03-01T01:00:00[America/Something]

If the timezone yields the same timezone offset for each, the answer will be:

{ months: 5, days: 30, hours: 23 }

HOWEVER, if the timezone had a DST transition that pushed 2001-03-01T02:00:00 1 hour back to 2001-03-01T01:00:00, then the correct answer would be:

{ months: 6 }

I believe your algorithm would incorrectly yield { months: 5, days: 30, hours: 23 } in this case (or 24 hours?). I'm nearly positive there's a test262 test for this somewhere.

2. Calls to User-Code

Within the backoff loop I see calls to:

• 2 iterations max for...
  • CalendarDateUntil (from DifferenceISODateTime)
  • GetPlainDateTimeFor, CalendarDateAdd, GetInstantFor (from AddZonedDateTime)

So 8 calls max. I believe its possible to get this lower.

Alternate Implementation

Here's an alternate implementation. It attempts to avoid the aforementioned bug (?) and has fewer calls to user-code (4 max):

• 3 iterations max for...
  • GetInstantFor
• CalendarDateUntil (from DifferenceDate) after the loop

Please let me know what you think!

export function DifferenceZonedDateTime(
  ns1,
  ns2,
  timeZoneRec,
  calendarRec,
  largestUnit,
  options,
  precalculatedDtStart = undefined
) {
  const nsDiff = ns2.subtract(ns1);
  if (nsDiff.isZero()) {
    return {
      years: 0,
      months: 0,
      weeks: 0,
      days: 0,
      norm: TimeDuration.ZERO
    };
  }

  const sign = nsDiff.lt(0) ? -1 : 1;

  const TemporalInstant = GetIntrinsic('%Temporal.Instant%');
  const start = new TemporalInstant(ns1);
  const end = new TemporalInstant(ns2);
  const dtStart = precalculatedDtStart ?? GetPlainDateTimeFor(timeZoneRec, start, calendarRec.receiver);
  const dtEnd = GetPlainDateTimeFor(timeZoneRec, end, calendarRec.receiver);

  let dtIntermediate
  let nsIntermediate
  let dayCorrection

  // Simulate moving ns1 as many years/months/weeks/days as possible without surpassing ns2.
  // This value is stored in dtIntermediate/nsIntermediate.
  // We do not literally move years/month/weeks/days, but rather assume dtIntermediate will
  // have the same time-parts as dtStart, and move backwards from YearMonthDay(dtEnd).
  //
  // This loop will run 3 times max:
  // 1. initial run
  // 2. backoff if the time-parts of dtIntermediate have conflicting sign with overall diff direction,
  //    just how DifferenceISODateTime works:
  //    https://github.com/tc39/proposal-temporal/blob/0c44b877bb8d647db1a4e240e8c9d47a3e5ec68e/polyfill/lib/ecmascript.mjs#L3854-L3858
  // 3. backoff if dtIntermediate fell into a DST gap and was pushed in a direction
  //    that would make the diff of the time-parts conflict with the sign of the overall direction.
  //
  for (dayCorrection = 0; dayCorrection <= 2; dayCorrection++) {
    // YearMonthDay(dtEnd) w/ dayCorrection backoff
    const isoDateIntermediate = BalanceISODate(
      GetSlot(dtEnd, ISO_YEAR),
      GetSlot(dtEnd, ISO_MONTH),
      GetSlot(dtEnd, ISO_DAY) + dayCorrection * -sign
    );

    // Incorporate time parts from ~dtStart~
    dtIntermediate = CreateTemporalDateTime(
      isoDateIntermediate.year,
      isoDateIntermediate.month,
      isoDateIntermediate.day,
      GetSlot(dtStart, ISO_HOUR),
      GetSlot(dtStart, ISO_MINUTE),
      GetSlot(dtStart, ISO_SECOND),
      GetSlot(dtStart, ISO_MILLISECOND),
      GetSlot(dtStart, ISO_MICROSECOND),
      GetSlot(dtStart, ISO_NANOSECOND),
    )
    nsIntermediate = GetInstantFor(timeZoneRec, dtIntermediate, 'compatible');

    // Did nsIntermediate surpass ns1?
    const signIntermediate = Math.sign(Number(ns2 - nsIntermediate))
    if (sign && signIntermediate && signIntermediate !== sign) {
      // If so, keep backing off...
    } else {
      break; // Stop
    }
  }

  if (dayCorrection > 2) {
    throw new Error('assert not reached: more than 2 day correction needed');
  }

  // Guaranteed to be compatible with the overall `sign`
  let timeDuration = new TimeDuration(ns2 - nsIntermediate)

  // Similar to what happens in DifferenceISODateTime with date parts only...
  const dateLargestUnit = LargerOfTwoTemporalUnits('day', largestUnit);
  const untilOptions = SnapshotOwnProperties(options, null);
  untilOptions.largestUnit = dateLargestUnit;
  const untilResult = DifferenceDate(calendarRec, dtStart, dtIntermediate, untilOptions);
  const years = GetSlot(untilResult, YEARS);
  const months = GetSlot(untilResult, MONTHS);
  const weeks = GetSlot(untilResult, WEEKS);
  const days = GetSlot(untilResult, DAYS);

  return { years, months, weeks, days, norm: timeDuration };
}

[ptomato]

@arshaw Thanks for this. Nice trick of combining dtEnd's date-parts with dtStart's time-parts. As far as I can tell, I agree with you that it is equivalent but with fewer user code calls, and the test262 results back that up. (Of course that doesn't guarantee it's correct, but it gives me some measure of confidence.) I'd be happy to adapt the PR to use this.

Regarding the bug, I might well be missing something, but currently I don't think it is a bug — or if it is, the new code has it too. I've tried to construct the situation you sketched but with an actual time zone transition. I've taken this year's fall-back DST in my time zone, where 2024-11-03T02:00 is turned back to 2024-11-03T01:00, and a start time of 5 months earlier (not 6 months, so that we don't get interference depending on whether the spring-forward transition is just over or just under 6 months earlier). Here's my test program:

{
  const d1 = Temporal.ZonedDateTime.from('2024-06-03T02:00[UTC]');
  const d2 = Temporal.ZonedDateTime.from('2024-11-03T01:00[UTC]');
  console.log(`no DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}

{
  const d1 = Temporal.ZonedDateTime.from('2024-06-03T02:00[America/Vancouver]');
  const d2 = Temporal.ZonedDateTime.from('2024-11-03T01:00-07:00[America/Vancouver]');
  console.log(`DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}

With this I get, both before and after the change,

no DST: 2024-06-03T02:00:00+00:00[UTC]..2024-11-03T01:00:00+00:00[UTC] = P4M30DT23H
DST: 2024-06-03T02:00:00-07:00[America/Vancouver]..2024-11-03T01:00:00-07:00[America/Vancouver] = P4M30DT23H

I think that result is correct! 2024-11-03 is a 25-hour day, so 23 hours should not balance up to 1 day.

If you meant to test it out on a 23-hour day (spring-forward transition), here's what I tried:

{
  const d1 = Temporal.ZonedDateTime.from('2023-10-10T03:00[UTC]');
  const d2 = Temporal.ZonedDateTime.from('2024-03-10T02:00[UTC]');
  console.log(`no DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}

{
  const d1 = Temporal.ZonedDateTime.from('2023-10-10T03:00[America/Vancouver]');
  const d2 = Temporal.ZonedDateTime.from('2024-03-10T02:00[America/Vancouver]');
  console.log(`DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}

Here's the output I get, again both before and after:

no DST: 2023-10-10T03:00:00+00:00[UTC]..2024-03-10T02:00:00+00:00[UTC] = P4M28DT23H
DST: 2023-10-10T03:00:00-07:00[America/Vancouver]..2024-03-10T03:00:00-07:00[America/Vancouver] = P5M

[ptomato]

I've updated this and written more tests in tc39/test262#4012. I think it's ready to merge now.

@arshaw In addition to what I wrote yesterday, I think we can tighten the loop conditions a bit more — it seems to me that dayCorrection = 2 is only possible when sign = 1. I'll use the Pacific/Apia time zone as an example, in which the whole day of 2011-12-30 was skipped, because I find that simplest to reason about:

Going forwards, let's say you have a dtEnd on 2011-12-31 with a time of day that's earlier than dtStart's time of day. So you get one backoff due to overflow (dtEnd's date + dtStart's time of day surpasses ns2). You subtract one day, putting you in the skipped day, which disambiguates forward. So you again have dtEnd's date + dtStart's time of day, which surpasses ns2, so you get one more backoff due to UTC offset shifts. You subtract another day to get the final intermediate date-time that doesn't surpass ns2.

Going backwards, you can either get one backoff due to overflow or one due to UTC shift, but not two. Since a skipped time only disambiguates forward, it always goes in the same direction as the backoff would, so you can never get a timeSign that's different from the overall sign in that second step.

Let me know if you concur with this reasoning. I may very well be missing something.

[arshaw]

Hi @ptomato,

Regarding the bug, I might well be missing something, but currently I don't think it is a bug...

You're right, the hypothetical bug I gave, in the various forms you tried, would not yield any buggy behavior. Sorry about that. But I DID remember a different similar case. Concrete example:

// UTC as a "control"
{
  const d1 = Temporal.ZonedDateTime.from('2024-04-10T02:00[UTC]');
  const d2 = Temporal.ZonedDateTime.from('2024-03-10T03:00[UTC]');
  console.log(`DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}
// -P30DT23H

// America/Vancouver as an "experiment"
{
  const d1 = Temporal.ZonedDateTime.from('2024-04-10T02:00[America/Vancouver]');
  const d2 = Temporal.ZonedDateTime.from('2024-03-10T03:00[America/Vancouver]');
  // NOTE: nonexistent hour just before d2 from 2024-03-10T02:00 to 02:59
  console.log(`DST: ${d1}..${d2} = ${d1.until(d2, { largestUnit: 'months' })}`);
}
//   Current main branch: -P31D (obviously wrong)
// Previously in this PR: ???
//  Currently in this PR: -P1M
//           My polyfill: -P1M

So, the bug has been fixed.

However, I want to think more about what is "correct". -P30DT23H could be equally as correct:

// adding-back two different durations yields the same result
{
  const d1 = Temporal.ZonedDateTime.from('2024-04-10T02:00[America/Vancouver]');
  console.log(d1.add('-P1M').toString())
  // 2024-03-10T03:00:00-07:00[America/Vancouver]
}
{
  const d1 = Temporal.ZonedDateTime.from('2024-04-10T02:00[America/Vancouver]');
  console.log(d1.add('-P30DT23H').toString())
  // 2024-03-10T03:00:00-07:00[America/Vancouver] (same!)
}

Given your teammates' preference towards using smaller units in situations of ambiguity, I think this should be discussed.

Moving on...

I think we can tighten the loop conditions a bit more — it seems to me that dayCorrection = 2 is only possible when sign = 1

Your logic sounds legit to me! Makes sense when you think about how DST transitions can only push forward during diffing (because of assumed 'compatible' disambiguation). To write it out:

• Moving foward in time (worst case)
  • +1 dayCorrection due to wall-clock overshoot
  • +1 dayCorrection due to push-forward DST
• Moving backward in time (worst case)
  • +1 dayCorrection due to wall-clock overshoot
  • (no dayCorrection due to DST because it will never push backwards causing an overshoot)

So, the loop will run max 3 times in the first situation, 2 in the second.

This makes me think of an additional optimization... if you're "moving forward" and you know for sure the wall-clock will overshoot, you don't even need query the timezone. It's a guaranteed dayCorrection of 1 or more.

That's not the case with "moving backwards" because a wall-clock time that would normally seem to have overshot the end-time might be pushed back in bounds by a DST transition, and thus a dayCorrection of zero might be an acceptable solution (see America/Vancouver example above). Of course, this will change if we decide to change that behavior.

[arshaw]

@ptomato, I will take over on this one. I'll incorporate the more conservative diffing strategy we talked about in the meeting as well as make the dayCorrection optimization I mentioned in prior comments. I'll post a new PR that supersedes this one.

[ptomato]

@arshaw Thanks for volunteering to take this on.

For those following along at home, we agreed in the champions meeting of 2024-02-29 that -P30DT23H is the correct answer to the case that Adam noted above, so there is still a bug in this PR that needs a fix and test coverage.

[arshaw]

Hi @ptomato, I've created a new PR that superseded this one
#2810

[ptomato]

For anyone following this, it moved to #2810 where Adam made the above-discussed fixes. Closing.