Skip to content

Conversation

robertbastian
Copy link
Member

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 const1 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.

Footnotes

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

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

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.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

const LONG_YEAR_LEN: u16 = 355;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why remove these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

robertbastian added a commit that referenced this pull request Oct 13, 2025
In policy and needed for #7065
@robertbastian 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
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(github didn't record my review)

@Manishearth
Copy link
Member

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
Manishearth previously approved these changes Oct 13, 2025
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

let num_periods =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sffc
sffc previously requested changes Oct 14, 2025
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +92 to +94
let millis = base_moment.rata_die.to_i64_date() * MILLISECONDS_IN_EPHEMERIS_DAY
+ base_moment.local_milliseconds as i64
+ num_periods * duration.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@robertbastian
Copy link
Member Author

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

@robertbastian robertbastian requested review from Manishearth and sffc and removed request for zbraniecki October 14, 2025 17:00
@sffc sffc dismissed their stale review October 14, 2025 18:53

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

@robertbastian robertbastian merged commit 901f12d into unicode-org:main Oct 15, 2025
30 checks passed
@robertbastian robertbastian deleted the exhaustive branch October 15, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants