`Duration::add` with relativeTo:ZonedDateTime intermediate DST bug?

[arshaw]

In Duration::add, using a ZonedDateTime relativeTo value, it's possible for an intermediate ZDT to fall into a DST gap and affect the final result:

d0 = Temporal.Duration.from({ months: 6 })
d1 = Temporal.Duration.from({ months: 6 })
d2 = d0.add(d1, { relativeTo: '2023-09-10T02:00:00[America/New_York]' })
// { months: 12, hours: 1 }

The intermediate ZDT falls into a DST gap at 2024-03-10T02:00:00 and gets pushed forward an hour.

We've been avoiding intermediate-value DST in other places of the spec. Is this in a similar vein? Is this considered a bug?

Other behavior we might consider:

1. converting the ZDT to a PlainDateTime
2. adding y0, m0, w0, d0
3. adding y1, m1, w1, d1
4. converting to an epochNanoseconds
5. combining time parts of both durations into a nanosecond value and adding to epochNanoseconds
6. doing a ZDT diff between original ZDT and the new one to get the final balanced Duration

Steps 1-5 is inspired by ZonedDateTime::add, but with two durations instead of one.

[justingrant]

Good find. Avoiding intermediate values triggering DST disambiguation seems like a good thing.

And I agree that your proposed algorithm seems both correct and better.

[ptomato]

On the other hand, I think in #856 we intentionally defined dur1.add(dur2, { relativeTo }) as relativeTo.until(relativeTo.add(dur1).add(dur2)). If that needs to be relitigated now, I'd rather remove Duration.p.add/subtract from the proposal and punt them to Temporal v2.

[arshaw]

What's interesting is that if relativeTo is a PlainDate, the durations' time parts are handled separately from their y/m/w/d parts:

https://github.com/tc39/proposal-temporal/blob/a493578be5631ecfd924df4434505e235a33c8e0/polyfill/lib/ecmascript.mjs#L4585-L4603

This is very similar to the alternate approach to ZonedDateTime's that I described in my original post.

(though of course there's a big with that reported in #2812, but it can be easily resolved)

@ptomato, in summary, the "relativeTo.until(relativeTo.add(dur1).add(dur2))" approach isn't leveraged for both relativeTo scenarios in the current spec anyway.

[ptomato]

But for the result, that doesn't matter, because PlainDate arithmetic has no disambiguation, right?

[arshaw]

Regardless of TZ disambiguation, the current algorithm for PlainDates (stripping duration time parts and then reapplying after) is still different that the "relativeTo.until(relativeTo.add(dur1).add(dur2))" approach because applying the durations' time parts in the middle of the process vs end of the process could yield different result due to constraining. Example:

const relativeTo = Temporal.PlainDate.from('2023-01-01')
const dur1 = Temporal.Duration.from({ days: 29, hours: 24 })
const dur2 = Temporal.Duration.from({ months: 1 })

// Approach: naive adding and diffing
//
// Jan1 -> Jan30 -> Jan31 -> Feb28
{
  const sum = relativeTo.until(relativeTo.add(dur1).add(dur2))
  console.log(sum.toString()) // P58D
}

// Approach: saving time parts until end (currently used for PlainDates)
// https://github.com/tc39/proposal-temporal/blob/a493578be5631ecfd924df4434505e235a33c8e0/polyfill/lib/ecmascript.mjs#L4585-L4603
//
// Run this in playground!
// https://tc39.es/proposal-temporal/docs/
//
// Jan1 -> Jan30 -> Feb29 -> Mar1
{

  const sum = dur1.add(dur2, { relativeTo })
  console.log(sum.toString()) // P1M28D
}

[arshaw]

To summarize, there are two approaches to adding durations:

A) naive adding and diffing B) saving time parts until end

Currently A is used for ZonedDateTimes and B is used for PlainDates. This strikes me as odd and I feel the same approach should be used for both scenarios.

My opinions about each

A - easy to explain to someone. easier to implement B - feels more consistent w/ how intermediate ZDT disambiguation is avoided in other parts of the code (more "correct").

Either way works for me!

[ptomato]

I'm not convinced this inconsistency is so bad, but I noticed today that if we fixed the bug in #2812 by adopting A for both PlainDate and ZonedDateTime, it'd have the nice bonus of making these consistent.

(Note also PlainDate relativeTo has to be converted to PlainDateTime for the addition, otherwise the time units are dropped.)

I also thought about proposing a "less naive" adding and diffing, still simple to explain and implement but without the intermediate point. Basically making d1.add(d2, {relativeTo}) roughly equivalent to

relativeTo.until(relativeTo.add(new Temporal.Duration(
  d1.years + d2.years,
  d1.months + d2.months,
  ...
  d1.nanoseconds + d2.nanoseconds
))

But unfortunately that only works when the two durations have the same sign.

[justingrant]

After our previous long discussion about this issue, I think it may be helpful to take a step back to discuss the use cases for Duration.p.add. I dug into as many use cases as I could find, and it was clear that they fit into two patterns:

• "Standalone" durations - intended to store or display to users without ever being combined with a date and/or time
• "Incrementing" durations - intended to be later added to a date and/or time type like ZDT or PlainDate.

Examples of standalone durations:

• Calculating total flight time of an airplane before it needs scheduled maintenance
• A profiling tool calculating total time inside a particular function
• A payroll system calculating pay for an hourly employee based on timeclock data
• A travel site showing the average delay (over the past year) of each flight in search results.
• Sorting a list of multi-stop plane trips by total layover time or total flight time.

Examples of incrementing durations:

• A delivery app trying to figure out how many more deliveries can be made before the driver needs to quit at 5PM. Or any similar case where you add up a bunch of durations to a starting point and then compare against an ending point.
• Extending an endpoint based on "time outs" during an activity. Like injury time during a football/soccer match.
• A video game where health is measured as aging. When you pick up a pile of treasure items, some are worth 1 day or rejuvenation, others 1 month, and special ones 1 year, and then the sum is subtracted from the character's age.
• Beyond that I couldn't think of more cases. I did think of several cases that on the surface seemed like good fits, but actually required custom addition logic that Duration.p.add couldn't handle. These "close but not close enough" cases included:
  • A "parent task" in a Gantt chart that's added to the start date to figure out a completion date, but most Gantt charts don't go 24x7 so this would be custom calculations anyways, e.g. only 8 hours per day and excluding weekends and holidays.
  • Projecting when a plane traveller will arrive given a delayed departure time of a multi-leg plane trip, but airlines do all kinds things to "make up time in flight" to avoid arriving late, so knowing the departure delay doesn't necessarily give you the arrival time.
  • A billing system for a service that's charged either daily or monthly (e.g. equipment rental) to aggregate the billing from multiple pieces of equipment. But I don't think Temporal.Duration math will work for this case, because it seems unlikely that renting 30 pieces for one day will be the same price as one piece for a month. Also, it seems likely that the company would want to use a standard month length (e.g. 30 days) so customers won't complain that they're being cheated on 31-day months.

Here's a more structured way to understand the two types:

	Standalone Durations	Incrementing Durations
Primary Use	Storing as-is or displaying to users	Adding to a date/time value
Prevalence	Common	Rare
Balancing	Required (i.e. displaying "2h 80m" is an obvious bug)	Custom (e.g. treat every month as 30 days) or unnecessary
DST Impact	None (never added to a ZDT)	Problematic (e.g. this issue!)
Needs relativeTo?	Usually not	Maybe
Impact of removing relativeTo?	Probably small, because W/M/Y cases are unusual	Probably small, because W/M/Y cases often need custom balancing

With the above framework in mind, here's some options to resolve this issue:

Option 1: Leave as-is, accept DST oddity

• Pros:
  • No changes to proposal
  • Easy to explain behavior
• Con:
  • DST oddity

Option 2: Change duration addition algorithm to remove DST oddity

• Pros:
  • No change to public API or current capabilities of that API
  • Easy to position to plenary as a narrow-scoped bugfix for an edge case
  • Easy to explain behavior
• Cons:
  • Week-addition issue in #2818
  • Change is bad in Stage 3

Option 3: Remove relativeTo, leave other behavior unchanged Throw if non-zero units present in weeks, months, or years (current behavior treats days as 24 hours if no relativeTo)

• Pros:
  • Avoids DST oddity
  • Easy to explain behavior
  • No impact on existing non-relativeTo-using code and tests
  • Easy to explain to plenary: "we removed relativeTo, and any code that didn't use it works exactly the same".
  • Works if durations have opposite signs
  • Fixes week-addition issue in #2818
  • May reduce proposal code size
  • Could add W/M/Y units in a follow-up proposal.
• Cons:
  • Won't work for date units larger than days (do addition use cases exist for larger units?)
  • Change is bad in Stage 3

Option 4: Remove relativeTo, week/month/year units are arithmetically added Throw if non-zero units present in weeks, months, or years AND if durations have opposite signs

• Pros:
  • Supports adding all date units
  • Avoids DST oddity
• Cons:
  • Inconsistent balancing behavior across units => hard to explain
  • Data-driven exception conditions => hard to explain
  • Complex design requirements, e.g. unclear if time units should be balanced if WMY units are present.
  • Would be only API that returns an unbalanced duration in some cases, except from and with where the unbalanced parts are provided explicitly by the user so that unbalancing is less surprising.
  • Could NOT change W/M/Y units behavior in a follow-up proposal.
  • Change is bad in Stage 3

[justingrant]

Option 1: Leave as-is, accept DST oddity Option 2: Change duration addition algorithm to remove DST oddity Option 3: Remove relativeTo, leave other behavior unchanged Option 4: Remove relativeTo, week/month/year units are arithmetically added

Meeting 2023-04-18: Consensus is for Option 3

[ptomato]

I've written up the conclusion in #2825. Let's close this, since the solution now encompasses more than just the original bug in this thread.