Normative: Disallow adding weeks, days, and time units to PlainYearMonth

[ptomato]

This feature has been buggy for years and nobody noticed, so it must not be used very often. This is an alternative solution to fix the bug in issue #3197 and would supersede #3208 if accepted.

This changes PlainYearMonth.prototype.add/subtract to throw a RangeError if the supplied duration has nonzero weeks, days, or time units. (Only years and months are allowed.)

This does not change the behaviour of PlainDate.prototype.add/subtract, because there are more use cases for adding e.g. { hours: 48 }, and days are always 24 hours in PlainDate.

Closes: #3197

[acutmore]

This PR achieved consensus during TC39 plenary yesterday (2025-01-20) 🎉

[ptomato]

Test262 tests in tc39/test262#4883

[Manishearth]

Anything stopping us landing this?

[Manishearth]

Slash would it be bad if I implemented this in V8 now instead of waiting?

[Manishearth]

FWIW, the current placement of the check is quite awkward for V8 order-of-operations, because it's doing further validation of an input before other inputs are parsed.

boa-dev/temporal#671 is our temporal_rs implementation, https://chromium-review.googlesource.com/c/v8/v8/+/7528669 uplifts to V8, and https://chromium-review.googlesource.com/c/v8/v8/+/7527580 duplicates that check in V8 for order-of-operations reasons.

[ptomato]

Anything stopping us landing this?

Just waiting for #3256 and #3263, since it's convenient to pull in changes at the same time as pulling in the test262 commit that tests them. Those are landed as of today.
(That, and it needs a green checkmark — technically it hasn't been reviewed yet.)

FWIW, the current placement of the check is quite awkward for V8 order-of-operations, because it's doing further validation of an input before other inputs are parsed.

For future reference, it would have made things a lot easier to have this feedback before the plenary! But you're right, it doesn't fit with the precedent we set in #3130. I'm assuming it's OK to incorporate this implementation feedback before landing this PR.

[ptomato]

Updated.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.94%. Comparing base (3ffa677) to head (faae30c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3253   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files          22       22           
  Lines       10408    10408           
  Branches     1816     1816           
=======================================
  Hits        10194    10194           
  Misses        196      196           
  Partials       18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ptomato]

Additional test coverage for the order of operations in tc39/test262#4894

[Manishearth]

For future reference, it would have made things a lot easier to have this feedback before the plenary!

I'm usually not implementing things until they land or there's relative certainty that they will land (for example, from the presence of test262 tests), so I wouldn't have really had an opportunity until now.

I could try and review future changes for this type of thing but I also just didn't expect changes like this at this stage.

[Manishearth]

Spec text looks good. Current implementation expectedly fails a bunch of test262 tests, but I haven't yet tried to update V8's test262 pin.

[Manishearth]

test262 commit 1115529eaccb7411b11834efbd0551dbd04f42cd passes with https://chromium-review.googlesource.com/c/v8/v8/+/7528669 with the added SKIPs removed.