Arithmetic with out-of-scope duration fields

[sffc]

See #388 for the original discussion regarding Temporal.Duration and disambiguation = "balance"

[ptomato]

I agree it should be valid to have a Duration object with too-high values. However I just realized that means we need to re-think which Duration fields we allow in the various arithmetic methods.

For example we throw a RangeError if you do this:

const wallClockNow = Temporal.now.time()
wallClockNow.plus({days: 2})  // throws if one of days, months, or years ≠ 0

but not this:

wallClockNow.plus({hours: 48})  // doesn't throw

This also goes for MonthDay since you can add or subtract months ≥ 12 or days ≥ 365.

[ljharb]

I'm not sure what the rationale would be that would lead the first example to throw. Anything that lets me "plus" anything should let me plus everything, no?

[sffc]

I'm not sure what the rationale would be that would lead the first example to throw. Anything that lets me "plus" anything should let me plus everything, no?

I misread the post too. What @ptomato is saying is that since Temporal.now.time() is a Temporal.Time, which does not have a year/month/day field, it does not make sense to add years, months, or days to it.

In situations like this, we could:

1. Throw RangeError if there are fields in the duration that aren't in the receiver object
2. Silently ignore the out-of-range fields
3. EDIT 2020-03-05: Before performing arithmetic, balance small out-of-range fields up into the smallest valid field, and ignore large out-of-range fields

Note: @pipobscure has pointed out before that since the durations can be data-driven, like reading a string from a database or from user input, it would be undesirable to have .plus/.minus throw RangeError dependent on the fields in the duration.

@younies Thoughts on this?

[gibson042]

Other cases of particular interest include Temporal.Date.from("2020-02-16").plus({hours: 36}) and especially Temporal.Date.from("2020-02-16").plus(Temporal.Duration.from({hours: 36})), where ignoring the non-overlapping fields will definitely surprise authors.

[ptomato]

I'd propose option 2 for units that are too large for the type (e.g. adding years to Time), because they have no effect on the result. However, I'd propose option 3 for units that are too small for the type (e.g. adding hours to Date) because of what @gibson042 mentioned above.

[sffc]

If we do option 3 only in one direction, what happens when you do

let ym = // some Temporal.YearMonth
ym.plus({ days: 30 })

The number of days depends on the month of course.

I guess the semantic could be: expand the type to DateTime (both Date and YearMonth) at midnight on the first of the month; add the fields; disambiguate according to the input option; and then convert back to the input type (which truncates). Does that work?

[ptomato]

That would work IMO.

On the other hand maybe the answer is to treat YearMonth differently than Date since date.plus({hours: 24}) is unambiguously the same date while yearMonth.plus({days: 30}) is not.

[sffc]

If you want YearMonth arithmetic to be predictable, then don't give it days. This is an edge case where we're trying to figure out the best behavior.

[sffc]

There are two discussions going on here. Since most of this thread is about the issue @ptomato raised about out-of-scope fields, I renamed this issue to address that particular issue. I moved the original question about disambiguation="balance" behavior to #388.

[gibson042]

For the new question at hand here, I think it's fine to ignore fields that are too large, but methods should throw when the input includes nonzero fields that are too small for their type (because there's no general way to support them).

let date = Temporal.Date.from("2020-02-16");
assert.strictEquals(date.plus({hours: 0}).toString(), "2020-02-16");
Array(60).fill().map((_, i) => i + 1).forEach(hours =>
  assert.throws(() => date.plus({hours}));
);

let yearMonth = Temporal.YearMonth.from("2020-02");
assert.strictEquals(yearMonth.plus({days: 0}).toString(), "2020-02");
Array(60).fill().map((_, i) => i + 1).forEach(days =>
  assert.throws(() => yearMonth.plus({days}));
);

If you want to do arithmetic with smaller units, then first map the receiver to a type that understands them (e.g., date.constructor.from(date.withTime("00:00").plus({hours: 24})) or yearMonth.constructor.from(yearMonth.withDay(1).plus({days: 30}))).

[sffc]

I'm slightly confused by your comment, because 3 days ago, you said:

Other cases of particular interest include Temporal.Date.from("2020-02-16").plus({hours: 36}) and especially Temporal.Date.from("2020-02-16").plus(Temporal.Duration.from({hours: 36})), where ignoring the non-overlapping fields will definitely surprise authors.

Now you are suggesting that we throw when the fields are too small.

[gibson042]

Ignoring the non-overlapping fields will definitely surprise authors, so they should not be ignored. And because too-small fields cannot be processed in the general case (e.g. adding 36 hours to a Temporal.Date), the proper alternative to ignoring them is rejection.

Too-large fields that can't affect the calculation (e.g., time.plus({days: 1})) are safe to ignore because that is identical to processing them, but if they can affect the calculation then they too should be rejected.

[sffc]

Consensus is to go with Option 3 from #324 (comment)

[gibson042]

So what happens for Temporal.Date.from("2020-02-16").plus({hours: 36}), and why is that different from either Temporal.Date.from("2020-02-16").plus({hours: 24}) or Temporal.Date.from("2020-02-16").plus({hours: 48})?

[sffc]

So what happens for Temporal.Date.from("2020-02-16").plus({hours: 36})

The duration would be balanced into {days: 1, hours: 12}, and you would add 1 day to the Date, ignoring the small units.

and why is that different from either Temporal.Date.from("2020-02-16").plus({hours: 24})

The answer would be the same for all hours between 24 and 47 inclusive.

or Temporal.Date.from("2020-02-16").plus({hours: 48})?

Now you're adding 2 days.

[gibson042]

I object to that treatment, but am not prepared to fight consensus at this time.