Normative: Fix Duration rounding relative to ZonedDateTime

[ptomato]

When the time portion of a duration, rounded relative to a ZonedDateTime, would land on a non-24-hour day, we'd get incorrect results. This is a regression from #2508 where although switching the order of BalanceDateDurationRelative and BalanceTimeDurationRelative was correct, we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742

[codecov]

Codecov Report

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

Project coverage is 96.49%. Comparing base (985b826) to head (c4c071a).
Report is 47 commits behind head on main.

❗ Current head c4c071a differs from pull request most recent head 55439c6. Consider uploading reports for the commit 55439c6 to get more accurate results

Files	Patch %	Lines
polyfill/lib/ecmascript.mjs	97.04%	25 Missing ⚠️
polyfill/lib/duration.mjs	96.45%	4 Missing and 1 partial ⚠️
polyfill/lib/timeduration.mjs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2758      +/-   ##
==========================================
- Coverage   96.51%   96.49%   -0.02%     
==========================================
  Files          23       23              
  Lines       12386    12501     +115     
  Branches     2294     2257      -37     
==========================================
+ Hits        11954    12063     +109     
- Misses        372      380       +8     
+ Partials       60       58       -2     

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

[arshaw]

Thanks @ptomato, this successfully addresses my concerns brought up in #2742 (comment)

I've verified it succeeds under my original test case...

const past = Temporal.ZonedDateTime.from("2019-11-01T00:00:00-07:00[America/Vancouver]")
const duration = Temporal.Duration.from({ years: 1, hours: 24 })
const future = past.add(duration) // 2020-11-01T23:00:00-08:00[America/Vancouver]
duration.round({ largestUnit: 'years', relativeTo: past }).toString() // P1YT24H -- CORRECT

...as well as succeeding in some others...

const past = Temporal.ZonedDateTime.from("2019-11-01T00:00:00-07:00[America/Vancouver]")
const duration = Temporal.Duration.from({ days: 366, hours: 24 })
const future = past.add(duration) // 2020-11-01T23:00:00-08:00[America/Vancouver]
duration.round({ largestUnit: 'years', relativeTo: past }).toString() // P1YT24H -- CORRECT

const past = Temporal.ZonedDateTime.from("2020-10-31T00:00:00-07:00[America/Vancouver]")
const duration = Temporal.Duration.from({ days: 1, hours: 24 })
const future = past.add(duration) // 2020-11-01T23:00:00-08:00[America/Vancouver]
duration.round({ largestUnit: 'years', relativeTo: past }).toString() // P1YT24H -- CORRECT

You said in #2742 (comment) you'd like to consolidate the code path for round() and until(). FWIW, temporal-polyfill achieves this by creating an intermediate value for BOTH the PlainDate and ZonedDateTime cases, and includes ALL of the input duration's units, not just years/month/weeks. It then directly calls the equivalent of DifferenceTemporalPlainDateTime / DifferenceTemporalZonedDateTime with the relativeTo and intermediate values, delegating all balancing to those two methods.

[ptomato]

This now includes a second commit with the consolidation of the code paths for round(), total(), and until(). I'll still be auditing this before presenting to TC39 to make sure results don't change unintentionally, and to minimize unnecessary calls into user code. If that will pose a problem reviewing the materials for TC39 plenary, please let me know.

[jasonwilliams]

LGTM so far

[arshaw]

Looks great to me. Love this round/total/until consolidation.

This consolidation fixes the bug I brought up here:
#2742 (comment)

Temporal.Duration.from({ months: 1, days: 15, hours: 12 }).total({
  unit: 'month',
  relativeTo: '2024-02-10T02:00[America/New_York]'
})
// expected: 1.4986559139784945

[ptomato]

I've rebased this and brought it up to date. Tests are in tc39/test262#4041. May still change to address some comments from @arshaw.

[ptomato]

This is ready now, addressing the opportunities for optimizing the new shared code path that @arshaw identified. Tests are in tc39/test262#4041. Review appreciated before merging.

[arshaw]

^^^ @ptomato, nevermind, all tests are passing. reviewing now...

[arshaw]

Things look fantastic @ptomato! The only potential bug I see is with deltaDays at the end of RoundRelativeDuration. All my other feedback should be considered editorial.

In the future, we may want to detangle the total() code path from the RoundRelativeDuration as evident from the code-smelly total: NaN in NudgeToZonedTime.

There might be an opportunity to give a formal shape to "relativeTo". Kevin Ness brought this up in the last meeting regarding difficulty in statically typed languages.

type RelativeTo = {
  plainDateTime: Temporal.PlainDateTime
  timeZone: Temporal.TimeZone // or null if UTC?
}

function normalizeRelativeTo(input): RelativeTo {
  if (input instanceof Temporal.ZonedDateTime) {
    return {
      plainDateTime: input.toPlainDateTime(),
      timeZone: input.getTimeZone(),
    }
  } else if (input instanceof Temporal.PlainDate) {
    return {
      plainDateTime: input.toPlainDateTime(), // gives 00:00:00
      timeZone: new Temporal.TimeZone('UTC')
    }
  }
}

/*
Adds a Duration to the relativeTo and returns an epoch nanoseconds
This function is only ever called with time units zeroed out!
(It's just the way our nudging/bubbling system works during relative-rounding)
Thus, we don't need to worry about the intricacies of *when* time units are
added in PlainDateTime vs ZonedDateTime.

A utility function is nice because we don't need to call GetUTCEpochNanoseconds and
GetInstantFor conditionally each time.
*/
function addToRelativeTo(relativeTo: RelativeTo, duration): bigint {
  return relativeTo.timeZone.getInstantFor(
    relativeTo.plainDateTime.add(duration)
  ).epochNanoseconds
}

Thanks

[ptomato]

OK, with the bug fixed and an extra test case added, I'm hoping this is ready to merge now. I'll plan to land it as soon as tc39/test262#4041 gets the green light.

[arshaw]

Ok great, glad everything worked out. Things look good from my POV.

[ptomato]

OK, last outstanding change approved in February 2024 TC39 meeting, tests are merged, all review comments addressed. Thanks for the help everyone.