`Duration::round` with ZonedDateTime relativeTo conflicts with `ZonedDateTime::until` results #2742

arshaw · 2023-12-21T20:16:55Z

Apologies for this rounding/diffing nitpicking. Repro:

const duration = new Temporal.Duration(1, 0, 0, 0, 24)
const past = Temporal.ZonedDateTime.from("2019-11-01T00:00[America/Vancouver]")
const future = past.add(duration)

const durationViaUntil = past.until(future, {
  largestUnit: 'years',
})
console.log(durationViaUntil.toString())
// "P1YT24H" (desired b/c of DST)

const durationViaRound = duration.round({
  largestUnit: 'years',
  relativeTo: past,
})
console.log(durationViaRound.toString())
// "P1Y1D" (undesired)

Precedents:

ZonedDateTime::add adds Y/M/W/D before time parts
ZonedDateTime::until diffs Y/M/W/D before time parts

It would make sense to have Duration::round effectively add Y/M/W/D to relativeTo before balancing time parts up to days (NanosecondsToDays). This is currently not possible because the Duration::round methods calls BalanceTimeDurationRelative before BalanceDateDurationRelative. (see here)

Encountered while working with this test:
intl402/Temporal/Duration/prototype/round/relativeto-string-datetime.js

Similar to:
#2563
#2715

The text was updated successfully, but these errors were encountered:

arshaw · 2023-12-23T00:54:43Z

Another example, but this one related to halfExpand rounding and which 1-month window it's supposed to round within (I expect it to be shortened by 1 hour due to DST and round down, not up).

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)

const durationViaUntil = past.until(future, {
  smallestUnit: 'month',
})
console.log(durationViaUntil.toString())
// P1M (desired)

const durationViaRound = duration.round({
  smallestUnit: 'month',
  relativeTo: past,
})
console.log(durationViaRound.toString())
// P2M (undesired)

This also has implications for total:

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

ptomato · 2024-01-18T01:08:40Z

Thanks for the report. I've been investigating this and I agree that something seems wrong here. However, I'm not sure I agree with the suggested solution.

It would make sense to have Duration::round effectively add Y/M/W/D to relativeTo before balancing time parts up to days (NanosecondsToDays). This is currently not possible because the Duration::round methods calls BalanceTimeDurationRelative before BalanceDateDurationRelative. (see here)

Switching the order of BalanceDateDurationRelative and BalanceTimeDurationRelative would effectively revert #2508 which we already did to fix a different bug. It fixes the problem in the OP (the result becomes P1Y24H), but it brings back the other bug from #2508 (see largestunit-correct-rebalancing.js). It may be that we need to look elsewhere for the solution. Or it may be that these two weirdnesses are mutually exclusive, in which case we'd need to figure out which one is worse.

I suspect the problem in the second post is actually a different bug, occurring in a different place. I'll need to investigate that some more. FWIW, in that case the result from round() (P2M) seems like it's the expected one, and the until() result (P1M) seems incorrect. Reasoning, adding Y/M/W/D first:

past + 1 month 15 days is on 2024-03-25, which is a 24-hour day, so 12 hours seems like it should be 0.5 days.
past + 1 month is on 2024-03-10, which is a 31-day month, so 15.5 days seems like it should be 0.5 months.
So past until future seems like it should be 1.5 months, which would round up to 2 months with half-expand rounding.

A MoveRelativeZonedDateTime step was missing, causing incorrect results. See tc39/proposal-temporal#2742

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

ptomato · 2024-01-23T00:30:54Z

I think I've tracked down the two discrepancies. The first one is addressed in #2758 (although as we discussed in the previous champions meeting, I'm still planning to add a refactor to that branch to make sure that round() and until() use the same code path)

The second discrepancy is actually not even a problem with round():

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 is because adding 1 month results in 2024-03-10T02:00[America/New_York] which is a nonexistent time, and so disambiguates to 03:00. This is technically the intended behaviour: the intermediate value between adding months and days must exist, though unfortunately this particular consequence makes it difficult to figure out what's happening. As it happens, though, we are currently discussing over at #2535 whether to change this.

Note that if you count in exact times, there is no discrepancy:

duration.round({ largestUnit: 'hours', relativeTo: past })  // => 1067 hours
past.until(future)  // => 1067 hours

arshaw · 2024-01-23T18:39:45Z

Hi @ptomato, I've resumed discussing about my first concern as a comment on your new PR.

For the ZonedDateTime::add() versus ::until() discrepancy you just brought up, I commented here. In short, I feel that +1 month should not be added alone and instead +15 days should be added as well when computing the intermediate ZonedDateTime.

However, I feel the ZonedDateTime::until() versus Duration::round() and ::total() discrepancy I brought up in my second concern is distinct. I feel that +1 month SHOULD be added alone in this case and thus the disambiguation logic SHOULD execute.

To recap the confusing behavior:

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

// EXPECTED: P1M
// ACTUAL: P2M --- inconsistent with ZonedDateTime::until()
duration.round({ smallestUnit: 'month', relativeTo: past }).toString()

// EXPECTED: 1.4986559139784945 (in my opinion)
// ACTUAL: 1.5
duration.total({ unit: 'month', relativeTo: past })

Here's my reasoning for why the "EXPECTED" is expected:

// falls in DST-gap, so time nudged up to 03:00:00 per disambiguation:compatible
const spanStart = past.add({ months: 1 }) // 2024-03-10T03:00:00-04:00[America/New_York]

// does NOT fall in DST-gap
const spanEnd = past.add({ months: 2 }) // 2024-04-10T02:00:00-04:00[America/New_York]

const partialMonths = // 0.4993270524899058
  Number(future.epochNanoseconds - spanStart.epochNanoseconds) / // 1335600000000000
  Number(spanEnd.epochNanoseconds - spanStart.epochNanoseconds) // 2674800000000000

// result for Duration::total()
const exactMonths = 1 + partialMonths // 1.4993270524899058

// result for Duration::round() with halfExpand
const roundedMonths = Math.round(exactMonths) // 1

I'd expect a virtual 1-month span of time to be created and then compute a fraction for how far future
lies within that span.

Looking forward to hearing what you think!

In order to prevent bugs due to discrepancies between two ways of calculating the same thing such as in #2742, refactor duration rounding with relativeTo so that duration.round({ smallestUnit, largestUnit, relativeTo, ...options }) goes through the same code path and gives the same result as const target = relativeTo.add(duration); relativeTo.until(target, { smallestUnit, largestUnit, ...options }) but taking into account that the until() methods have a different default roundingMode than Duration.prototype.round(), and optimizing away as many user-observable calls as possible.

In order to prevent bugs due to discrepancies between two ways of calculating the same thing such as in #2742, refactor duration rounding with relativeTo so that duration.round({ smallestUnit, largestUnit, relativeTo, ...options }) goes through the same code path and gives the same result as const target = relativeTo.add(duration); relativeTo.until(target, { smallestUnit, largestUnit, ...options }) but taking into account that the until() methods have a different default roundingMode than Duration.prototype.round(), and optimizing away as many user-observable calls as possible. Similarly, duration.total({ unit, relativeTo, ...options }) goes through the same code path, which also returns the total as a mathematical value if needed.

A MoveRelativeZonedDateTime step was missing, causing incorrect results. See tc39/proposal-temporal#2742

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

In order to prevent bugs due to discrepancies between two ways of calculating the same thing such as in #2742, refactor duration rounding with relativeTo so that duration.round({ smallestUnit, largestUnit, relativeTo, ...options }) goes through the same code path and gives the same result as const target = relativeTo.add(duration); relativeTo.until(target, { smallestUnit, largestUnit, ...options }) but taking into account that the until() methods have a different default roundingMode than Duration.prototype.round(), and optimizing away as many user-observable calls as possible. Similarly, duration.total({ unit, relativeTo, ...options }) goes through the same code path, which also returns the total as a mathematical value if needed.

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

In order to prevent bugs due to discrepancies between two ways of calculating the same thing such as in #2742, refactor duration rounding with relativeTo so that duration.round({ smallestUnit, largestUnit, relativeTo, ...options }) goes through the same code path and gives the same result as const target = relativeTo.add(duration); relativeTo.until(target, { smallestUnit, largestUnit, ...options }) but taking into account that the until() methods have a different default roundingMode than Duration.prototype.round(), and optimizing away as many user-observable calls as possible. Similarly, duration.total({ unit, relativeTo, ...options }) goes through the same code path, which also returns the total as a mathematical value if needed.

arshaw · 2024-02-05T03:53:23Z

To summarize the current state of affairs:

The bug I reported in my first comment is FIXED by Normative: Fix Duration rounding relative to ZonedDateTime #2758
The bug I reported in my second comment is FIXED by Normative: Fix Duration rounding relative to ZonedDateTime #2758 AS WELL
The bug @ptomato mentioned here is NOT YET SOLVED despite progress on Normative: Fix date difference for end-of-month edge cases #2759 . The bug is happening because DifferenceZonedDateTime does not correctly merge the Calendar-land result with the time-units result.

ptomato · 2024-02-06T18:29:13Z

The bug ptomato mentioned here is NOT YET SOLVED despite progress on Normative: Fix date difference for end-of-month edge cases #2759 . The bug is happening because DifferenceZonedDateTime does not correctly merge the Calendar-land result with the time-units result.

@arshaw I intended #2760 to fix this bug, could you take a look when you have a moment, and see if you agree with that one?

A MoveRelativeZonedDateTime step was missing, causing incorrect results. See tc39/proposal-temporal#2742

This should produce all the same results (except for a change to weeks balancing in round(), which is now more consistent with since()/until()) but leads to different observable user code calls. See tc39/proposal-temporal#2742