Add exhaustive tests for calendar round tripping and `from_fields` and fix overflow bugs #7065

robertbastian · 2025-10-10T13:37:29Z

This tests the RataDie -> Date<C> -> Date<Iso> -> Date<C> -> RataDie round trip for all dates between -270000-01-01 and 270000-12-31. It also asserts that the produced Date<C> follows the same ordering as the associated RataDie (some calendars already test this for some ranges). For this purpose I added the DateInner: PartialOrd bound to Calendar. Four calendars, (including AnyCalendar, which is why it can't be Ord) were lacking this implementation.

This discovered a bug in Hijri, where the start of the year was stored relative to the approximate f64 mean synodic year length, not the exact tabular year length (this was missed in #7057). I changed the debug_assertion!s in the Hijri code to be actual errors, which are unwrapped in const¹ for hardcoded data, and can be shown to not happen at runtime because the only year infos we create are tabular, which obeys all the invariants (the simulated rules now fall back to tabular for years where they would break invariants, I couldn't care less about this calendar). These invariants also let us remove the while loops added in #7057.

This PR also adds an exhaustive test for from_fields over the extended years -270000..=270000, all month codes, days in 1..=100 and ordinal months in 1..=100.

Both tests require release-with-assertions to run in an appropriate time, but the round trip tests then finishes in seconds. We should figure out some way to run at least that one in CI, as it would supersede a lot of manually written tests for specific calendars and ranges, and provides valuable coverage.

This requires MSRV 1.83, which is more than 6 versions old, so I can use ↩

gemini-code-assist · 2025-10-10T13:37:32Z

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

Manishearth

I still have a preference for avoiding release panics. Fix looks good, though.

components/calendar/src/cal/hijri.rs

Manishearth · 2025-10-10T16:50:26Z

components/calendar/src/cal/hijri.rs

    }
 }

-const LONG_YEAR_LEN: u16 = 355;


question: why remove these?

uh I tried to refactor some of the simulated code and prefer them inline, because then you see that the three cases it checks are continuous values

Fair enough. I find that the constant name also serves as documentation, so perhaps include a short comment in the new hardcoded constants saying what they are? Non blocking.

I know we're not consistent about documenting "obvious" per-calendar internals but it's nice to try.

components/calendar/tests/exhaustive.rs

components/calendar/src/calendar.rs

components/calendar/src/provider/hijri.rs

Manishearth · 2025-10-10T17:21:53Z

components/calendar/src/cal/hijri.rs

-            while rd >= year.start_day() + year.last_day_of_month(12) as i64 {
-                year = self.0.year_data(year.extended_year + 1)
-            }
+            year = self.0.year_data(year.extended_year - 1);


thought: still a little wary about removing the while loop.

components/calendar/src/calendar.rs

components/calendar/src/cal/hijri.rs

In policy and needed for #7065

components/calendar/fuzz/fuzz_targets/construction.rs

Manishearth

(github is buggy and is not letting me post these comments without a review message)

components/calendar/src/date.rs

Manishearth · 2025-10-13T17:32:20Z

components/calendar/src/cal/chinese/simple.rs

-            .div_euclid(duration.0);
-            base_moment + duration * num_periods
+            let diff_millis = (rata_die - base_moment.rata_die) as i128
+                * MILLISECONDS_IN_EPHEMERIS_DAY as i128


Yes, this PR is attempting to extend those bounds to ±2³⁰.

With this PR, all calendars work with that wide range, as opposed to most calendars working there except for chinese, dangi, and Hijri (and Hijri is a very simple fix).

sffc

(github didn't record my review)

Manishearth · 2025-10-13T17:39:51Z

Github review functionality has been really buggy these days, often on the "files changed" view if you hit refresh (with nothing in the URL query or fragment) it still shows only a part of the changes. Beware.

Manishearth

Fuzzer changes have been fixed, and Chinese changes have been reverted for now (aside from a non-behavior-changing refactor).

I'm fine with this, and I think none of my open comments are blocking. Shane still has a PartialOrd concern it seems, unsure if that's blocking.

Manishearth · 2025-10-13T20:27:49Z

components/calendar/src/cal/chinese/simple.rs

+            let diff_millis = (rata_die - base_moment.rata_die) * MILLISECONDS_IN_EPHEMERIS_DAY
+                - base_moment.local_milliseconds as i64;
+
+            let num_periods =


nit: ideally, explain each step. There's a -1 here that is now easier to document

This is a preexisting issue that need not be fixed here

components/calendar/src/cal/chinese/simple.rs

sffc

The PR looks fine overall. I defer to @Manishearth on the review of the test as well as the changes to Hijri. I trust you or @Manishearth to decide that my comments are sufficiently addressed and dismiss my review and merge when ready.

components/calendar/src/cal/chinese/simple.rs

sffc · 2025-10-14T02:57:13Z

components/calendar/src/cal/chinese/simple.rs

+            let millis = base_moment.rata_die.to_i64_date() * MILLISECONDS_IN_EPHEMERIS_DAY
+                + base_moment.local_milliseconds as i64
+                + num_periods * duration.0;


Nit: I don't see how inlining the impl core::ops::Add<Milliseconds> for LocalMoment and impl core::ops::Mul<i64> for Milliseconds fixes any overflow bugs. Please un-do the inlining or explain why you need to inline it.

I think it doesn't fix overflow bugs: previously this code did, but then we asked to not upgrade Milliseconds to i128, so now this leftover refactor doesn't really do anything. I think it's overall cleaner, at least in this function, but I think it's fine to split that out into a separate PR.

I removed the Mul impl because it's more debuggable to have an overflow panic here than there

components/calendar/src/calendar.rs

robertbastian · 2025-10-14T07:54:34Z

@sffc none of you comments seem blocking, why are you blocking this PR?

components/calendar/src/cal/chinese/simple.rs

My outstanding issues are about code comments and clarity; please fix them, but can be done in a follow-up if this blocks other words

Co-authored-by: Shane F. Carr <[email protected]>

robertbastian requested review from a team, Manishearth and sffc as code owners October 10, 2025 13:37

robertbastian mentioned this pull request Oct 10, 2025

Update MSRV to 1.83 #7066

Merged

Test calendars exhaustively, fix hijri

852c96d

robertbastian force-pushed the exhaustive branch from d3314c7 to 852c96d Compare October 10, 2025 14:48

robertbastian requested a review from zbraniecki as a code owner October 10, 2025 14:48

robertbastian mentioned this pull request Oct 10, 2025

Throw error in Date::try_from_fields for very large year values. #7062

Merged

Manishearth reviewed Oct 10, 2025

View reviewed changes

smaller ranges

f0daf63

sffc reviewed Oct 13, 2025

View reviewed changes

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

components/calendar/src/cal/hijri.rs Outdated Show resolved Hide resolved

components/calendar/src/cal/hijri.rs Outdated Show resolved Hide resolved

robertbastian added 5 commits October 13, 2025 10:21

try_new

9fb139f

new unchecked

53f3fd0

old comment

0a225ed

avoid overflow

2c525c5

Merge remote-tracking branch 'upstream/main' into exhaustive

a3716b8

robertbastian mentioned this pull request Oct 13, 2025

Implement date arithmetic according to Temporal specification #7012

Merged

robertbastian added a commit that referenced this pull request Oct 13, 2025

Update MSRV to 1.83 (#7066)

f00ad5b

In policy and needed for #7065

robertbastian added 3 commits October 13, 2025 10:36

Merge remote-tracking branch 'upstream/main' into exhaustive

9942728

clippy

2f90997

cleanup

9e44aa3

robertbastian requested review from Manishearth and sffc October 13, 2025 09:31

robertbastian added 3 commits October 13, 2025 14:35

fix chinese

bffb8be

update fuzzer

d249ae9

avoid overflows in hardcoded data lookup

13341d4

robertbastian commented Oct 13, 2025

View reviewed changes

components/calendar/fuzz/fuzz_targets/construction.rs Outdated Show resolved Hide resolved

robertbastian changed the title ~~Add exhaustive tests for calendar round tripping and from_fields~~ Add exhaustive tests for calendar round tripping and from_fields and fix overflow bugs Oct 13, 2025

allow more extended years

a627a48

Manishearth reviewed Oct 13, 2025

View reviewed changes

sffc reviewed Oct 13, 2025

View reviewed changes

robertbastian force-pushed the exhaustive branch from fff5205 to 7b882d3 Compare October 13, 2025 20:06

back to i64

b5d2087

robertbastian force-pushed the exhaustive branch from 7b882d3 to b5d2087 Compare October 13, 2025 20:08

robertbastian added 2 commits October 13, 2025 22:08

phrasing

2dbeda4

cleanup

2c2b7d5

Manishearth previously approved these changes Oct 13, 2025

View reviewed changes

sffc previously requested changes Oct 14, 2025

View reviewed changes

components/calendar/src/cal/chinese/simple.rs Show resolved Hide resolved

sffc reviewed Oct 14, 2025

View reviewed changes

revert hebrew test

2df04ca

robertbastian dismissed Manishearth’s stale review via 2df04ca October 14, 2025 13:38

robertbastian added 3 commits October 14, 2025 15:43

recover framing

a01e61a

Merge remote-tracking branch 'upstream/main' into exhaustive

ec03e9e

Merge remote-tracking branch 'upstream/main' into exhaustive

15c1c89

robertbastian requested review from Manishearth and sffc and removed request for zbraniecki October 14, 2025 17:00

robertbastian added 2 commits October 14, 2025 19:04

merge issue

0ea0db0

remove saturating operations

42f693b

sffc reviewed Oct 14, 2025

View reviewed changes

components/calendar/src/cal/chinese/simple.rs Show resolved Hide resolved

components/calendar/src/cal/chinese/simple.rs Outdated Show resolved Hide resolved

robertbastian and others added 3 commits October 14, 2025 21:41

Apply suggestions from code review

aef0884

Co-authored-by: Shane F. Carr <[email protected]>

bigger range

acfd164

fmt

bedef22

sffc approved these changes Oct 15, 2025

View reviewed changes

robertbastian merged commit 901f12d into unicode-org:main Oct 15, 2025
30 checks passed

robertbastian deleted the exhaustive branch October 15, 2025 09:28

Add exhaustive tests for calendar round tripping and from_fields and fix overflow bugs #7065

Add exhaustive tests for calendar round tripping and from_fields and fix overflow bugs #7065

Uh oh!

Conversation

robertbastian commented Oct 10, 2025

Footnotes

Uh oh!

gemini-code-assist bot commented Oct 10, 2025

Uh oh!

Manishearth left a comment

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 13, 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!

Uh oh!

sffc 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertbastian commented Oct 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add exhaustive tests for calendar round tripping and `from_fields` and fix overflow bugs #7065

Add exhaustive tests for calendar round tripping and `from_fields` and fix overflow bugs #7065