Duration::round with ZonedDateTime relativeTo conflicts with ZonedDateTime::until results

[arshaw]

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

[arshaw]

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]

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.

[ptomato]

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]

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!

[arshaw]

To summarize the current state of affairs:

1. The bug I reported in my first comment is FIXED by Normative: Fix Duration rounding relative to ZonedDateTime #2758
2. The bug I reported in my second comment is FIXED by Normative: Fix Duration rounding relative to ZonedDateTime #2758 AS WELL
3. 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]

3. 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?