Use more specific calendar-dependent operations in Until, Add, and ToISO #69

sffc · 2025-07-26T00:18:17Z

Fixes #55. More steps toward #57.

This PR is based on #66, and it also depends on #68 and tc39/proposal-temporal#3138.

I rewrote NonISODateAdd, NonISODateUntil, and NonISOCalendarDateToISO to more precisely specify their algorithms. They are still ultimately calendar-dependent, but the calendar-dependent parts are boiled down to more bite-sized abstract operations:

CalendarDateArithmeticalToISO
CalendarDaysInMonth
CalendarMonthsInYear
YearContainsMonthCode

Manishearth

Partial review up til NonISODateSurpasses

spec.emu

Manishearth

Very minor points overall. Looks good!

Implementing this efficiently may be tricky but that's fine; at least now we have more concrete guidance for behavior

spec.emu

sffc · 2025-07-29T17:40:08Z

A bit more commentary on my choices of AOs:

RegulateNonISOYearMonth ( calendar, arithmeticalYear, monthCode, years )
- This is a key calendar-dependent operation that adds years but not months. This is where we resolve common/leap month constraining issues in Chinese, Dangi, and Hebrew.
BalanceNonISODate ( calendar, year, month, day )
- This AO works the same across all calendars. There are zero special cases. It depends only on CalendarDaysInMonth and CalendarMonthsInYear.
NonISODateSurpasses ( calendar, sign, baseDate, isoDate2, years, months, weeks, days )
- This is my generalization of Editorial: Move regulating and balancing logic into ISODateSurpasses proposal-temporal#3138 to non-ISO. I tweaked the dependent AOs slightly. The ISO version combines the first few steps into a BalanceISOYearMonth, but that is a fundamentally different operation because it can be implemented with a DivRem. For non-ISO, I wanted to emphasize that there are multiple steps: first you add years, then you convert month codes to arithmetical months, then you continue with the rest of the balancing. This order of operations is crucial for the interoperability of calendar arithmetic.
NonISODateAdd and NonISODateUntil
- My goal is to refactor these AOs back into main Temporal so that they depend on the same low-level operations. I have a start in Editorial: Rewrite CalendarDateUntil to be simpler and more general proposal-temporal#3136.

sffc · 2025-07-29T17:45:01Z

To put Add and Until's algorithm in words:

Add years
Constrain to a valid year/month
Add and balance months
Constrain to a valid year/month/day
Add and balance weeks and days

The key difference with non-ISO is step 2. ISO doesn't need to constrain to a valid year/month because all months are valid in all years.

This also feeds into #56. Is it more consistent or less consistent for non-ISO to reject when constraining year/month? I took the position that non-ISO is better off rejecting on step 4 only. I think it's a valid position that it should reject on both steps 2 and 4. However, maybe there should be a third option in the enum to allow for either behavior.

spec.emu

sffc · 2025-07-30T17:27:52Z

This PR was included in the Stage 2.7 presentation. This PR was approved as part of Stage 2.7 with the following understanding: "changes the spec text to transform some parts of the spec from prose to algorithmic spec steps"

sffc · 2025-07-31T22:35:24Z

I believe I have responded to all of the threads. PTAL. Thank you @Manishearth @gibson042 @ryzokuken!

justingrant

Minor suggestion: the adjective for "arithmetic" can be "arithmetical" but could also be "arithmetic", e.g. "arithmetic mean".

Brevity is important in spec text, so I'd suggest globally replacing "arithmetical" wtih just "arithmetic".

justingrant

Overall looks good. A few minor suggestions only.

spec.emu

sffc · 2025-08-02T23:40:52Z

Minor suggestion: the adjective for "arithmetic" can be "arithmetical" but could also be "arithmetic", e.g. "arithmetic mean".

Brevity is important in spec text, so I'd suggest globally replacing "arithmetical" wtih just "arithmetic".

I prefer using the explicitly-adjective "arithmetical", but I was actually thinking of using a term like "monotonic" to emphasize how they actually behave, in another PR. But since this spec currently uses "arithmetic", I can keep using that term to reduce the diff, if other people feel the difference warrants fixing.

sffc · 2025-08-11T20:24:13Z

I'll integrate @justingrant's suggestions, but I'm waiting for the other re-reviews to come in first so that I can update the PR at one time.

ptomato

The month code stuff is all great. In order to facilitate merging this PR so that we don't have to do a follow up, I've opened a Temporal PR tc39/proposal-temporal#3144 to include it in Temporal.

I'm less convinced about the changes to NonISODateAdd, NonISODateUntil, and NonISOCalendarDateToISO. I preferred them the way they were in the prose form, because I think that's sufficiently well-defined to allow for unambiguous implementation while also not re-specifying stuff that is already in some external library such as ICU. This form seems more bug-prone to me, since realistically what are implementations going to do other than use ICU? By rewriting ICU in spec text here, that is never going to be validated by someone directly writing code that follows the spec, we're just inviting busywork on ourselves when they inevitably don't match in some edge case.

Ultimately I'd leave it up to the 402 editors whether they want that, but my recommendation would be no.

(I'd still like to use ConstrainMonthCodeToArithmeticalMonth in NonISODateAdd though.)

What I do feel more strongly about: I do not see it as a desirable goal to refactor NonISODateAdd and NonISODateUntil back into CalendarDateAdd and CalendarDateUntil. That loses the separation between machine calendar and human calendar. I would prefer that the ISO operations remain completely separate. I get that it is aesthetically more pleasing to unify them! But I think that is the same type of mistake I made with user-defined calendars, trying to express all of Temporal's operations in terms of calls to dateAdd and dateUntil, inviting lots of churn and additional bugs as various well-meaning people figured out ever-slightly-more-optimized ways to express it. Only in this case the abstraction is one level lower. And most likely, implementations are not going to write their code in terms of these low-level operations, they are going to use ICU.

ptomato · 2025-08-12T17:43:14Z

biblio.json

+    },
+    {
+        "location": "https://tc39.es/ecma262/",
+        "entries": [
+            {
+                "type": "op",
+                "aoid": "ℝ",
+                "id": "ℝ"
+            }
+        ]


Nothing to do with the substance of the PR, but I'm surprised this doesn't get linked automatically with --load-biblio @tc39/ecma262-biblio!

spec.emu

ptomato · 2025-08-12T20:02:07Z

spec.emu

+        1. Repeat, while _number_ &le; 12,
+          1. Let _currentMonthCode_ be CreateMonthCode(_number_, _isLeap_).
+          1. If YearContainsMonthCode(_calendar_, _year_, _currentMonthCode_), then
+            1. Set _monthsBefore_ to _monthsBefore_ + 1.
+          1. If _currentMonthCode_ is _resolvedMonthCode_, then
+            1. Return _monthsBefore_.
+          1. If _isLeap_ is *false*, then
+            1. Set _isLeap_ to *true*.
+          1. Else,
+            1. Set _isLeap_ to *false*.
+            1. Set _number_ to _number_ + 1.
+        1. Assert: The above loop should have returned before terminating.


You might be able to simplify this somehow since month codes are lexicographically ordered, but it's probably not worth it unless you're really enthusiastic about it.

I think the way I do the loop is fine, unless you have a specific suggestion to improve it.

spec.emu

ptomato · 2025-08-12T23:15:31Z

spec.emu

+      </emu-alg>
+    </emu-clause>
+
+    <emu-clause id="sec-temporal-calendardaysinmonth" type="implementation-defined abstract operation">


Minor point, I would suggest deleting the one-line calendar-dependent operations CalendarDaysInMonth and CalendarMonthsInYear. We used to have those in Temporal and got rid of them because they take up space without really adding anything meaningful. Instead, I'd just put something in NonISODateAdd etc. like "Let monthsInYear be the calendar-dependent number of months in year."

@gibson042 suggested smaller AOs earlier in #69 (comment). I don't have a strong opinion except that it's kind-of nice to have a "googlable" place to plug things in.

sffc · 2025-08-12T23:55:04Z

The month code stuff is all great. In order to facilitate merging this PR so that we don't have to do a follow up, I've opened a Temporal PR tc39/proposal-temporal#3144 to include it in Temporal.

Nice!

I'm less convinced about the changes to NonISODateAdd, NonISODateUntil, and NonISOCalendarDateToISO. I preferred them the way they were in the prose form, because I think that's sufficiently well-defined to allow for unambiguous implementation while also not re-specifying stuff that is already in some external library such as ICU. This form seems more bug-prone to me, since realistically what are implementations going to do other than use ICU? By rewriting ICU in spec text here, that is never going to be validated by someone directly writing code that follows the spec, we're just inviting busywork on ourselves when they inevitably don't match in some edge case.

"I"* am an implementer and also an ICU contributor. I have a bug assigned to me to implement calendar arithmetic in ICU[4X]. I don't know what to do: there is no spec to follow. I can try to replicate existing behavior, but I don't always know exactly what it does, and I can't authoritatively test that I did it correctly. The CLDR standard doesn't specify this because it deals only with formatting. I can read the prose of Temporal / ECMA-402, but what I would rather do is read a spec algorithm and then test my code against the spec's test suite.

So, yes, this spec logic is likely to be implemented in ICU more than in the engine or in Temporal_rs, but the logic is more squarely in Temporal's domain than in CLDR's domain.

* "I" is in quotes because although I am in this situation, it is a situation that isn't necessarily specific to me: it could be for any implementer.

What I do feel more strongly about: I do not see it as a desirable goal to refactor NonISODateAdd and NonISODateUntil back into CalendarDateAdd and CalendarDateUntil. ...

OK. I won't do that then. (This PR does not touch CalendarDateAdd and CalendarDateUntil.) I realized that writing them in terms of the lower-level operations I added here like NonISODateSurpasses works perfectly fine and achieves my objectives.

I may desire to write an editorial PR to align ISODateAdd to look a little bit more like NonISODateAdd, but I won't touch CalendarDateAdd.

Would you like me to (or would you like to) pull ConstrainMonthCodeToArithmeticalMonth (and its child YearContainsMonthCode) out into its own PR also, in order to reduce the scope of this one?

sffc · 2025-08-14T01:37:13Z

I plan to rebase this PR on top of #73. I've addressed almost all of the outstanding comments on this PR, and I will highlight any other changes between the current state of the PR and the updated version.

Capitalize "Boolean" RegulateNonISOYearMonth => ConstrainMonthCodeToArithmeticalMonth Change unqualified _month_ to _arithmeticalMonth_ Label a few things as "positive integer" fromIsoDate, toIsoDate

sffc · 2025-08-14T08:05:23Z

I performed the squash-rebase in 8138fc3 and then added 3484587 with some editorial review feedback.

gibson042

I think many uses of integer in this specification text should actually be positive integer.

spec.emu

Co-authored-by: Richard Gibson <[email protected]>

sffc · 2025-08-15T03:56:03Z

TG2 discussion: https://github.com/tc39/ecma402/blob/main/meetings/notes-2025-08-14.md#use-more-specific-calendar-dependent-operations-in-until-add-and-toiso-69

sffc · 2025-08-15T05:03:09Z

I quite like CalendarDateArithmeticalToISO so I pulled it out into another PR. #74

ptomato

I have comments, but only minor ones.

spec.emu

Co-authored-by: Philip Chimento <[email protected]>

sffc · 2025-10-08T01:00:15Z

OK, I've resolved all comment threads. Thanks @ptomato for the review. @gibson042 will have another chance to review when Intl Era Month Code goes for Stage 4, and I'm happy to open further editorial PRs addressing feedback. I will proceed to merge this PR.

Thank you everyone for your time here!

sffc force-pushed the 55-more-specific-ops branch 4 times, most recently from a72de40 to 12195b9 Compare July 29, 2025 07:37

sffc changed the title ~~Use more specific calendar-dependent operations in NonISODateUntil~~ Use more specific calendar-dependent operations in Until, Add, and ToISO Jul 29, 2025

sffc requested review from ptomato and ryzokuken July 29, 2025 07:57

Manishearth reviewed Jul 29, 2025

View reviewed changes

Manishearth approved these changes Jul 29, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Show resolved Hide resolved

sffc mentioned this pull request Jul 29, 2025

Decide behavior for Hebrew common/leap resolution #70

Closed

sffc mentioned this pull request Jul 30, 2025

Stage 2.7 review feedback: tweak the cases when rejecting in CalendarDateAdd #56

Closed

gibson042 reviewed Jul 30, 2025

View reviewed changes

ryzokuken approved these changes Jul 30, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

Manishearth mentioned this pull request Jul 31, 2025

Stage 2.7 review: Be more specific about [[DaysInWeek]] #71

Closed

sffc requested review from Manishearth, gibson042 and ryzokuken July 31, 2025 22:34

justingrant reviewed Aug 2, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

This was referenced Aug 4, 2025

Expose non-ISO equivalent of RegulateISODate / NonISOCalendarDateToISO unicode-org/icu4x#6790

Closed

Editorial: Rewrite CalendarDateUntil to be simpler and more general tc39/proposal-temporal#3136

Closed

ptomato mentioned this pull request Aug 12, 2025

Editorial: Month code improvements tc39/proposal-temporal#3144

Merged

ptomato reviewed Aug 12, 2025

View reviewed changes

Rewrite Add, Until, and ToISO on top of narrower calendar-dependent AOs

8138fc3

Capitalize "Boolean" RegulateNonISOYearMonth => ConstrainMonthCodeToArithmeticalMonth Change unqualified _month_ to _arithmeticalMonth_ Label a few things as "positive integer" fromIsoDate, toIsoDate

sffc force-pushed the 55-more-specific-ops branch from f157b7a to 8138fc3 Compare August 14, 2025 07:58

Review feedback

3484587

gibson042 reviewed Aug 14, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Show resolved Hide resolved

gibson042 reviewed Aug 14, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

Apply suggestions from code review

a358230

Co-authored-by: Richard Gibson <[email protected]>

sffc mentioned this pull request Aug 15, 2025

Editorial: Refactor the last bit of prose in NonISOCalendarDateToISO to AOs #74

Merged

sffc moved this from Priority Issues to Previously Discussed in ECMA-402 Meeting Topics Sep 9, 2025

sffc mentioned this pull request Sep 25, 2025

Stage 2.7 review feedback: symmetry of CalendarDateAdd and CalendarDateUntil #55

Closed

Manishearth mentioned this pull request Sep 25, 2025

Stage 2.7 review feedback: Include more information for CalendarResolveFields and arithmetic year #57

Open

sffc added 6 commits September 30, 2025 14:51

Merge branch 'main' into 55-more-specific-ops

a407f48

CalendarIntegersToISO

f8db768

Remove duplicate CalendarDaysInMonth

2748433

Update spec.emu

663b09d

Update spec.emu

eac8653

Merge branch 'main' into 55-more-specific-ops

c79d968

sffc requested review from gibson042 and ptomato October 2, 2025 15:30

sffc mentioned this pull request Oct 3, 2025

Implement date arithmetic according to Temporal specification unicode-org/icu4x#7012

Merged

ptomato approved these changes Oct 7, 2025

View reviewed changes

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Outdated Show resolved Hide resolved

spec.emu Show resolved Hide resolved

sffc and others added 2 commits October 7, 2025 17:23

Apply suggestions from code review

2fdb18d

Co-authored-by: Philip Chimento <[email protected]>

More feedback

6c44d86

sffc merged commit 0a7577f into tc39:main Oct 8, 2025

sffc deleted the 55-more-specific-ops branch October 8, 2025 01:00

sffc restored the 55-more-specific-ops branch October 8, 2025 01:00

Use more specific calendar-dependent operations in Until, Add, and ToISO #69

Use more specific calendar-dependent operations in Until, Add, and ToISO #69

Uh oh!

Conversation

sffc commented Jul 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manishearth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sffc commented Jul 29, 2025

Uh oh!

sffc commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sffc commented Jul 30, 2025

Uh oh!

sffc commented Jul 31, 2025

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sffc commented Aug 2, 2025

Uh oh!

sffc commented Aug 11, 2025

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

ptomato Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ptomato Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

sffc Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sffc commented Jul 26, 2025 •

edited

Loading

Manishearth left a comment •

edited

Loading

sffc commented Jul 29, 2025 •

edited

Loading

sffc commented Aug 12, 2025 •

edited

Loading