Implement date arithmetic according to Temporal specification #7012

sffc · 2025-10-01T03:53:23Z

This implementation aligns line-for-line with the spec. I intend to go back and add fast paths (like adding multiple years or months at once), but let's check in something that we know to be spec-compliant first.

sffc · 2025-10-03T05:24:22Z

I found some cases in the Hebrew calendar that I need to fix by fixing tc39/proposal-intl-era-monthcode#69. (This is why we have spec: so we can decide on the right thing upstream)

Manishearth

Generally a fan of the approach here. Will do full review on Monday when I can actually compare with the spec.

components/calendar/src/calendar_arithmetic.rs

Manishearth · 2025-10-03T06:10:44Z

components/calendar/src/options.rs

+/// Options for taking the difference between two dates.
+#[derive(Copy, Clone, PartialEq, Debug, Default)]
+#[non_exhaustive]
+pub struct DateUntilOptions {


bikeshed: Until or Difference, for this and related names?

I don't mind Until but it'll be confusing if we ever add since()

Not a blocker, worth discussing separately

We should add since on Date, doesn't need to be on the trait

Absolutely, but if we do, does it make sense for the error and Options and other public API bits to be called Until?

I'm fine calling the struct DateDifferenceOptions

Slight preference for that (and on the trait associated type, and elsewhere). Followup ok, and if people prefer Until that's fine too.

I renamed it to DateDifferenceOptions, and also UntilError to DifferenceError

robertbastian · 2025-10-01T07:57:55Z

components/calendar/src/duration.rs

+#[derive(Copy, Clone, PartialEq, Debug, Default)]
+#[non_exhaustive]
+pub struct DateUntilOptions {
+    pub largest_unit: Option<DateDurationUnit>,


Does this need to be optional?

Good point, the default value is contextually set by Temporal but I don't think we have all that context here and I think this is an ok thing to require from users.

But for convenience perhaps Unit or UntilOptions should have a default value? It's not fun to work with non exhaustives without one.

the question is does None have a behaviour that is different from all values?

We've previously used None as a way of distinguishing user intent, so while None may have the same behavior as an option, we might change it later or add other magical defaults.

In some APIs we have None and Auto and that's intentional.

Options bags in ICU4X take Options by convention in ICU4X. In this particular case, I don't think there's a difference between None and Some(default), but I'd rather stick with what we do elsewhere

robertbastian · 2025-10-03T07:55:46Z

components/calendar/src/options.rs

+/// Options for taking the difference between two dates.
+#[derive(Copy, Clone, PartialEq, Debug, Default)]
+#[non_exhaustive]
+pub struct DateUntilOptions {


We should add since on Date, doesn't need to be on the trait

Manishearth · 2025-10-04T04:11:56Z

With Robert's review and my partial review I'm comfortable landing this so you can start optimizing, I'll let you know if there are post merge review issues.

And if there are test failures in V8.

Manishearth

Looks good. Slight preference for landing this and iterating so that upstream temporal_rs work can start sooner.

Main issue is that I would like to see more comments that explain the code without relying on the spec (we should still keep the spec comments)

Manishearth · 2025-10-06T04:32:12Z

components/calendar/src/calendar_arithmetic.rs

 }

+impl<C: CalendarArithmetic> ArithmeticDate<C> {
+    /// Implements BalanceNonISODate


nit: spec link, and an in-words explanation. Balance is Temporal-internal jargon, we should explain what this does in non-Temporal words too

I can't link to the spec because tc39/proposal-intl-era-monthcode#69 still isn't merged, but I can explain what it does

Manishearth · 2025-10-06T04:35:46Z

components/calendar/src/calendar_arithmetic.rs

+            //   1. Set _resolvedMonth_ to _resolvedMonth_ - 1.
+            //   1. If _resolvedMonth_ is 0, then
+            resolved_month -= 1;
+            if resolved_month == 0 {


thought: it may be possible to write this code more efficiently as DateBalancer {y, m, d, m_in_y, d_in_m} with an add_years() add_months() add_days() set of methods.

But then we don't retain the spec correspondence

Yeah I want to follow up with a PR after the line-for-line code is landed to implement optimizations like this

Manishearth · 2025-10-06T04:36:13Z

components/calendar/src/calendar_arithmetic.rs

+        Self::new_unchecked(resolved_year, resolved_month, resolved_day)
+    }
+
+    pub(crate) fn surpasses(


nit: link to temporal spec if possible, and doc comment explaining this in non-temporal terms

Manishearth · 2025-10-06T04:39:35Z

components/calendar/src/calendar_arithmetic.rs

+        // 1. Let _parts_ be CalendarISOToDate(_calendar_, _fromIsoDate_).
+        // 1. Let _y0_ be _parts_.[[Year]] + _years_.
+        let y0 = cal.year_info_from_extended(duration.add_years_to(self.year.to_extended_year()));
+        // 1. Let _m0_ be MonthCodeToOrdinal(_calendar_, _y0_, ! ConstrainMonthCode(_calendar_, _y0_, _parts_.[[MonthCode]], ~constrain~)).


nit: include an explanatory comment somewhere saying what this does ("transfer the month code to the new year" or something)

I think for both this function and the one above, prose explanations of the various sections would be good. Matching the spec is nice for ensuring correctness, but if some behavior needs to be debugged it's good to have explanatory comments.

Prefer followup.

Will do in a follow-up

Manishearth · 2025-10-06T15:45:21Z

components/calendar/src/duration.rs

 #[derive(Debug, Copy, Clone, Eq, PartialEq, Default)]
 #[allow(clippy::exhaustive_structs)] // spec-defined in Temporal
 #[doc(hidden)] // unstable
 pub struct DateDuration {


n.b. this is still hidden, as are add() and until().

This PR is to implement arithmetic according to Temporal. I want to decouple it from the graduation PR.

Manishearth · 2025-10-06T15:58:26Z

components/calendar/src/any_calendar.rs

+/// Error returned when comparing two [`Date`]s with [`AnyCalendar`].
+#[derive(Debug, Copy, Clone)]
+#[non_exhaustive]
+pub enum AnyCalendarUntilError {


thought: maybe this should go in mod error

I'll consider this as part of #7010

components/calendar/src/any_calendar.rs

robertbastian · 2025-10-06T16:08:54Z

components/calendar/src/any_calendar.rs

 impl Calendar for AnyCalendar {
    type DateInner = AnyDateInner;
    type Year = YearInfo;
+    type UntilError = AnyCalendarUntilError;


issue: I think this should go on a new trait, not calendar. AnyCalendar is the only calendar where the date difference is fallible, but this makes all calendars fallible. we can impl Date::since infallibly for a new trait CalendarDiff, and implement it fallibly as Date::try_since for Date<AnyCalendar>

It doesn't make all calendars infallible, since the other calendars return an UntilError = Infallible, right?

I think being able to abstract over this is good.

but you still get a Result that you have to unwrap, unwrap_infallible doesn't exist, and even when it does, is not an ergonomic API.

actually the docs currently say "In case the specific calendar objects differ on data, the data for the first calendar is used, and date2 may be converted if necessary.". Why don't we do that and make the AnyCalendar method infallible?

alternatively something like this to at least not make the Date API use infallible:

impl<A: AsCalendar> Date<A> where A::Calendar: Calendar<UntilError = core::convert::Infallible>, { pub fn until<B: AsCalendar<Calendar = A::Calendar>>( &self, other: &Date<B>, options: DateUntilOptions, ) -> DateDuration { self.calendar .as_calendar() .until(self.inner(), other.inner(), options) .unwrap_or_else(|e| match e {}) } } impl<A: AsCalendar<Calendar = AnyCalendar>> Date<A> { pub fn try_until<B: AsCalendar<Calendar = A::Calendar>>( &self, other: &Date<B>, options: DateUntilOptions, ) -> Result<DateDuration, AnyCalendarUntilError> { self.calendar .as_calendar() .until(self.inner(), other.inner(), options) } }

I'm open to tweaking the APIs on Date as @robertbastian suggested.

If we do that, then should we try for consistency with the add functions?

Also, I want to note that I'm not intending to go for ergonomics in this PR. There is a lot of room we have to improve ergonomics, and I consider that very explicitly out of scope. I want to focus on landing the minimal usable API here. I'm open to prefixing functions by try_* if it gives us more room to improve in the future.

I prefer having one single Date API, even if the Result is a little unergonomic.

I'm also okay with having this never error because AnyCalendar converts.

I'm not auto-converting in AnyCalendar any more. I think it's a big footgun.

In that case I think I prefer having Result everywhere. let Ok(..) is fine. Rust has Infallible errors in a lot of places.

I renamed the Date functions to be try_added_with_options, try_add_with_options, and try_until_with_options. I think these names are very specific and give us room to add nicer ones in a follow-up.

sffc · 2025-10-07T02:36:24Z

I fixed the Hebrew bug; fortunately it was just a typo where I was using the wrong year variable.

I think this is ready to land. The APIs are still doc(hidden). I want follow-ups to:

Graduate the APIs and bikeshed them a bit more. Perhaps make the fields of DateDuration be private.
Add fast paths for adding multiple days, months, or years at once
Add more tests on algorithm behavior, such as Hebrew tests in the backwards direction
Add more tests on constrain/reject behavior and edge cases (constrain should never fail)

sffc · 2025-10-07T15:05:56Z


thread 'main' panicked at components/calendar/benches/date.rs:38:6:
called `Result::unwrap()` on an `Err` value: UnknownMonthCode(MonthCode("M04L"))
stack backtrace:
Testing date/calendar/julian
Success
Testing date/calendar/chinese_cached
   0:     0x55fdc93170b2 - std::backtrace_rs::backtrace::libunwind::trace::h2d45396358f41939
...
  17:     0x55fdc8d194f3 - core::result::Result<T,E>::unwrap::hcbc1ca801f2b226b
                               at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/result.rs:1167:23
  18:     0x55fdc8d194f3 - date::bench_date::hd4df229a13bf42b9
                               at /home/runner/work/icu4x/icu4x/components/calendar/benches/date.rs:38:6
  19:     0x55fdc8d1ba27 - date::bench_calendar::{{closure}}::{{closure}}::h4e96f5fed167e80f
                               at /home/runner/work/icu4x/icu4x/components/calendar/benches/date.rs:68:17
  20:     0x55fdc8d4590c - criterion::bencher::Bencher<M>::iter::h0dd6b3415aadf0c9
                               at /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/criterion-0.5.1/src/bencher.rs:88:23
  21:     0x55fdc8d1a42f - date::bench_calendar::{{closure}}::h15d5e9f347cbe884
                               at /home/runner/work/icu4x/icu4x/components/calendar/benches/date.rs:57:11

sffc and others added 3 commits September 30, 2025 20:52

DateAdd, DateUntil implemented. Stuck in trait land

1d3648e

only failing file is abstract_gregorian

2a38bfe

fix abstract_gregorian

40f70a0

sffc force-pushed the dateadduntil branch from 8b28264 to 337bc07 Compare October 3, 2025 02:28

Tests compiling; lots of docs tests

d65db3f

sffc force-pushed the dateadduntil branch from 337bc07 to d65db3f Compare October 3, 2025 02:28

sffc added 2 commits October 2, 2025 19:37

More tests passing

7447707

All tests passing in icu_calendar?

547ccac

sffc changed the title ~~Implement CalendarDateAdd and CalendarDateUntil~~ Implement date arithmetic according to Temporal specification Oct 3, 2025

sffc added 6 commits October 2, 2025 20:31

Merge branch 'main' into dateadduntil

457b3f1

Delete commented-out test

f683574

diplomat coverage

faf38ac

clippy

d9ab920

Add another test

860c630

Actually add the file

b31cf42

sffc marked this pull request as ready for review October 3, 2025 04:47

sffc requested a review from Manishearth as a code owner October 3, 2025 04:47

sffc requested a review from nekevss October 3, 2025 04:47

Hebrew tests

b332e05

sffc added 3 commits October 2, 2025 22:24

fmt

f8ec962

Add more Hebrew tests

c4059a4

clippy

3c43228

Manishearth reviewed Oct 3, 2025

View reviewed changes

Manishearth mentioned this pull request Oct 3, 2025

Calendar arithmetic support #3147

Open

robertbastian previously approved these changes Oct 3, 2025

View reviewed changes

Manishearth previously approved these changes Oct 6, 2025

View reviewed changes

Manishearth reviewed Oct 6, 2025

View reviewed changes

components/calendar/src/any_calendar.rs Outdated Show resolved Hide resolved

Add PartialEq

f1e4dbd

Manishearth dismissed stale reviews from robertbastian and themself via f1e4dbd October 6, 2025 16:02

robertbastian requested changes Oct 6, 2025

View reviewed changes

Manishearth mentioned this pull request Oct 6, 2025

WIP: Move to new ICU4X arithmetic code boa-dev/temporal#595

Draft

sffc mentioned this pull request Oct 7, 2025

Consider narrower error type between try_new_from_codes and try_from_fields #7010

Open

sffc added 3 commits October 6, 2025 19:33

Add tricky Hebrew standalone test case

300361d

Fix leap month arithmetic and re-enable tests

a5890fd

Renames, docs, comments, fmt

daa6f27

sffc requested review from robertbastian and Manishearth October 7, 2025 02:33

ci

4b03c6d

Manishearth previously approved these changes Oct 7, 2025

View reviewed changes

Fix benchmark and set AnyCalendarDifferenceError to hidden

bca8cae

sffc dismissed Manishearth’s stale review via bca8cae October 7, 2025 16:00

sffc requested a review from Manishearth October 7, 2025 17:51

Manishearth approved these changes Oct 7, 2025

View reviewed changes

Implement date arithmetic according to Temporal specification #7012

Are you sure you want to change the base?

Implement date arithmetic according to Temporal specification #7012

Uh oh!

Conversation

sffc commented Oct 1, 2025 • edited by Manishearth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sffc commented Oct 3, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth Oct 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth Oct 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 4, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sffc commented Oct 1, 2025 •

edited by Manishearth

Loading

Manishearth Oct 3, 2025 •

edited

Loading

Manishearth Oct 3, 2025 •

edited

Loading

sffc commented Oct 7, 2025 •

edited

Loading