Should PlainYearMonth throw when adding days?

[devongovett]

I find this confusing:

Temporal.PlainYearMonth.from('2019-06').add({days: 500}).toString()
// => '2020-10'

It seems to leak implementation details: PlainYearMonth is the same as PlainDate but with the day field set to 1 by default. Conceptually, PlainYearMonth is meant to represent the entire month, i.e any day within the month, but adding days assumes that it actually represents the first of the month.

This is true of other fields and data types as well, e.g. you can add hours to a PlainDate, which assumes it is midnight by default.

Temporal.PlainDate.from('2019-06-02').add({hours: 24}).toString()
// => '2019-06-03'

I think it would make more sense to be more strict about what fields can be added to each type such that only the smallest field represented by the type is allowed to be added.

[ljharb]

You added 500 days and got a result that’s in the same year?

[devongovett]

Sorry, copy paste error. Fixed.

[devongovett]

A better example that shows clearly that PlainYearMonth is really just PlainDate with the day field set to 1:

Temporal.PlainYearMonth.from('2019-06').add({days: 29}).toString()
// => '2019-06'

Temporal.PlainYearMonth.from('2019-06').add({days: 30}).toString()
// => '2019-07'

[ptomato]

This was previously discussed in #324 and in https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-03-05.md. I agree this is a tradeoff. My opinion at the time was that it would be more surprising to users for such additions to throw, than it would be to assume midnight on the first of the month for the purposes of addition.

[devongovett]

Ignoring unknown fields would also be acceptable. For example, foo in this example is unknown and ignored:

Temporal.PlainDate.from('2019-06-02').add({days: 1, foo: 1})

Throwing would be more a more obvious way to point out the more likely programmer error, but I think hours could also be ignored the same way here.

Perhaps the problem is in the definition: PlainDate#add accepts a Temporal.Duration when perhaps it should accept a date like object similar to the one with accepts. If a duration is passed it could be converted to a date like by either ignoring unknown fields or throwing when they are set.

The current behavior goes against the conceptual definition of the types at hand. A PlainDate is meant to represent a whole day, not a particular instant. And it's even more obvious with PlainYearMonth. I think defining the API to be specific to the type would be better than being overly flexible, but tell me if it's too late to change this.

[justingrant]

Thanks @devongovett for raising this issue. I'll add a few notes.

There are really two cases:

1. Durations with nonzero units that are larger than the capacity of the current type. PlainTime is the only type affected, e.g. plainTime.add({days}).
2. Durations with nonzero units that are smaller than the capacity of the current type. PlainDate and PlainYearMonth are affected, e.g. plainYearMonth.add({days}) , plainDate.add({hours}).

(1) seems like a lower-priority issue because the larger units can never affect the result. There's no ambiguity involved. So I think that ignoring the larger units is fine for this case.

(2) is potentially problematic. Thankfully leap days & months are not a concern because PlainMonthDay doesn't have add nor subtract. But:

• The smaller-units remainder is truncated, so if I loop 1000 times adding 23 hours to a PlainDate, the result will be identical to what I started with. But if I first add the durations together 1000 times and then add to the PlainDate, then I'll get a date that's 958 days later. This error seems unlikely in born-in-Temporal code. But when migrating code using legacy Date, I can see this error happening because Date isn't sensitive to order of operations.
• A corollary of truncation is that rounding isn't supported so there's no control over the remainder. What developers should be doing is calling Duration.prototype.round on the duration to both remove the too-small units and to explicitly decide how rounding should work. (If we do throw here, the error message could tell developers to use round first.)
• There are off-by-one-day issues possible when adding hours or smaller units where the original duration was measured across a DST-start date where that day is 23 hours long, while PlainDate/PlainMonthDay always assume 24-hour days. This problem isn't unique to this case. It's also true when adding two durations with a relativeTo: PlainDateTime when the durations were originally created by ZonedDateTime.since. But it's potentially worse here because the error can be a whole day (or even a whole month in rare cases), as opposed to just one hour.

In other parts of Temporal when there's possible ambiguity, we throw or provide options (e.g. relativeTo, offset, etc.) that force the developer to declare their intent. Throwing here seems like it'd be most consistent with the rest of Temporal.

I do feel strongly that we should not add additional options to add on these types. Either we should throw or we should leave the as-is weird behavior.

My weakly-held opinion is that throwing makes sense here, and I'd support a normative change to throw. Specifically:

• add/subtract on PlainYearMonth would throw if the input has nonzero days or smaller units.
• add/subtract on PlainDate would throw if the input has nonzero hours or smaller units.

[ptomato]

My preference in order of best to worst is:

• Keep the current behaviour.
• Change the operation to throw.
• Change the operation to ignore the lower units. (This one is a non-starter for me, since it silently ignores the user's intention.)

My opinion on this hasn't changed since #324. Although, I will note that what has changed in the meantime is that we throw on plainDate1.until(plainDate2, {largestUnit: 'hours'}), so that is one point in favour of changing this operation to throw: difference arithmetic cannot produce a duration that you can't add to or subtract from either of the original operands.

[ptomato]

One argument in favour of keeping the current behaviour is that it "just works" in a way that I believe would be the most common user expectation with negative durations, subtracting from the end of the month and adding to the beginning of the month:

ym = Temporal.Now.plainDateISO().toPlainYearMonth()  // 2021-08
ym.add({ days: 15 })  // 2021-08
ym.add({ days: -15 })  // 2021-08
ym.add({ days: 31 })  // 2021-09
ym.add({ days: -31 })  // 2021-07

This isn't obvious if you have to reimplement it for yourself in userland, and is a potential pitfall.

[ljharb]

The more I look at it, the more it really feels wrong to operate on a "year month" - a type in which days effectively do not exist - with "days". If you want to add days, you should convert it to a full date.

[ptomato]

Does anyone feel that we have learned new information that would change the consensus reached in #324? If not, I think we should close this.

[ljharb]

Yes, my previous comment indicates that i think throwing is more preferable. Users shouldn’t have a mental model that a YearMonth is just “Y-M-01”; it’s a distinct type.

[ptomato]

We had a previous discussion on this where we weighed all the alternatives in #324 and came to a tradeoff, and we haven't learned any information that invalidates any of the input that we used to determine that tradeoff, so we'll close this.

[ljharb]

@ptomato my comment contradicts that; can you address it?

[ptomato]

YearMonth indeed was overlooked in the original discussion, but it was eventually discussed in #418. The principle deemed most important for the resolution of #324 was Shane and Philipp's point about data-driven exceptions. I don't think anyone was overjoyed about introducing an inconsistency with YearMonth, but it doesn't change anything about the weight of YearMonth in the tradeoff. In fact, later we explicitly formulated a design principle about inconsistencies in YearMonth and MonthDay (#874) weighing less.

[ljharb]

Rereading those, I'm still not clear why we wouldn't want YearMonth to always throw when adding days. That wouldn't be a data-driven exception, it'd be a type-driven exception - an appropriate one.