Add RataDie::in_well_behaved_astronomical_range(), use to avoid panics #6876

Manishearth · 2025-08-18T23:44:59Z

Attempting to address #4917, #5068, https://issues.chromium.org/issues/439624688.

Related: #5778

tc39/proposal-temporal#2869 has told us that we are allowed to break calendar-internal invariants outside of the useful range of the calendar.

We currently hit debug assertions for some of these dates. This patches it up by

This is a partial PR, I haven't fixed all of the assertions. We'll likely have to make the function in calendrical_calculations pub+hidden (or make a copy in icu_calendar) and use it.

I would like feedback on the approach.

sffc

Ok. Relaxing the calendar-specific guarantees outside of a very large range seems reasonable and sufficiently orthogonal to the other questions about proleptic approximations.

components/calendar/src/tests/extrema.rs

utils/calendrical_calculations/src/rata_die.rs

robertbastian · 2025-08-19T12:17:37Z

components/calendar/src/tests/extrema.rs

+  }
+
+  // Assertion failure:
+  // > assertion failed: Date::try_new_ummalqura_date(y, m, d, IslamicUmmAlQura::new_always_calculating()).is_ok()


UAQ is not an astronomical calendar and it doesn't have this constructor.

Yeah this is just the old test that anba wrote.

We should still test that it doesn't panic.

then you might as well test that Gregorian doesn't panic. there is no astronomical code

Yeah but it feels like overkill to test all the calendars. These are the iffy ones, some iffier than others. It's a test, it's fine if it tests extra things.

A client (anba) found it useful to test this, so I'm keeping it in.

The purpose of this test is not to test that the code in this PR is doing something. The purpose of this test is to test that large Temporal-allowed dates work without panicking in the calendars that have calendrical debug assertions. We should ideally test more dates.

In the future if we moved the chinese calendar over to the pinqi approximation I would still wish to continue testing it here.

by that logic we might change the implementation of every calendar, or add debug assertions to any calendar. then just add these tests to any_calendar.rs which has tests for all calendars.

I mean yeah but it's only a couple calendars that are insufficiently defined outside the modern range. I think there's a pretty clear destination here.

But I might just make this an AnyCalendar test. I hadn't done that mostly because I was trying to reuse anba's test, and also the current AnyCalendar test is kind of annoyingly merged. Might try and clean it up a bit.

I added more calendars.

any_calendar.rs has a massive omnibus test that I do not think we should expand. A good refactor to do here would be to design a "for all calendars" test utility.

Until then, I added some more calendars. I do not think we need to exhaustively list them all. I do think adding extra tests protects us from future issues.

We should definitely test the calendars that are known-astronomical. For others, we can accept these unit tests but not necessarily require that they all be covered consistently.

sffc · 2025-08-26T21:32:04Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a mechanism to handle extreme dates in astronomical calendars by defining a 'well-behaved' range for RataDie. The new function RataDie::in_well_behaved_astronomical_range() is used to suppress debug assertions that would otherwise panic for dates far in the past or future. This change is applied to the Chinese-based and Islamic calendars. Additionally, a new test suite is added to ensure these extreme dates no longer cause panics. The approach is solid and effectively addresses the reported issues. I have one suggestion to improve the implementation of the new function by using constants for magic numbers, which will enhance readability and maintainability.

utils/calendrical_calculations/src/rata_die.rs

Copilot

Pull Request Overview

This PR addresses panic issues in astronomical calendars when processing dates outside well-behaved ranges by adding range checks to debug assertions. The changes introduce a WELL_BEHAVED_ASTRONOMICAL_RANGE constant and modify debug assertions to skip invariant checks for dates outside this range, preventing panics in extreme date scenarios while maintaining calendar correctness within expected ranges.

Key changes:

Added WELL_BEHAVED_ASTRONOMICAL_RANGE constant to define the astronomical validity range
Modified debug assertions across multiple calendar implementations to skip checks outside the well-behaved range
Added test cases to verify extreme date handling

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
utils/calendrical_calculations/src/chinese_based.rs	Added range constant and updated debug assertions for Chinese-based calendar calculations
utils/calendrical_calculations/src/islamic.rs	Re-exported the range constant and updated Saudi Islamic calendar assertions
components/calendar/src/provider/chinese_based.rs	Modified PackedChineseBasedYearInfo to handle out-of-range dates with clamping
components/calendar/src/cal/hijri.rs	Updated Hijri calendar debug assertions to use the astronomical range check
components/calendar/src/cal/chinese_based.rs	Added range checks for Chinese-based calendar date conversions
components/calendar/src/tests/extrema.rs	Added new test file to verify extreme date handling
components/calendar/src/tests/mod.rs	Added the new extrema test module

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

utils/calendrical_calculations/src/chinese_based.rs

components/calendar/src/provider/chinese_based.rs

components/calendar/src/cal/chinese_based.rs

components/calendar/src/tests/extrema.rs

components/calendar/src/provider/chinese_based.rs

robertbastian · 2025-08-29T06:56:12Z

components/calendar/src/tests/extrema.rs

+  }
+
+  // Assertion failure:
+  // > assertion failed: Date::try_new_ummalqura_date(y, m, d, IslamicUmmAlQura::new_always_calculating()).is_ok()


by that logic we might change the implementation of every calendar, or add debug assertions to any calendar. then just add these tests to any_calendar.rs which has tests for all calendars.

components/calendar/src/cal/hijri.rs

Manishearth requested a review from sffc August 18, 2025 23:45

sffc reviewed Aug 19, 2025

View reviewed changes

components/calendar/src/tests/extrema.rs Outdated Show resolved Hide resolved

utils/calendrical_calculations/src/rata_die.rs Outdated Show resolved Hide resolved

robertbastian reviewed Aug 19, 2025

View reviewed changes

sffc requested a review from Copilot August 26, 2025 21:31

This comment was marked as outdated.

Sign in to view

gemini-code-assist bot reviewed Aug 26, 2025

View reviewed changes

utils/calendrical_calculations/src/rata_die.rs Outdated Show resolved Hide resolved

Add checks for valid astronomical range

0de8442

Manishearth force-pushed the valid-astronomical-range branch from 15d3036 to 0de8442 Compare August 28, 2025 22:15

Manishearth marked this pull request as ready for review August 28, 2025 22:16

Manishearth requested a review from a team as a code owner August 28, 2025 22:16

Manishearth requested review from sffc, Copilot and robertbastian August 28, 2025 22:18

Copilot AI reviewed Aug 28, 2025

View reviewed changes

utils/calendrical_calculations/src/chinese_based.rs Show resolved Hide resolved

components/calendar/src/provider/chinese_based.rs Outdated Show resolved Hide resolved

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

Manishearth added 3 commits August 28, 2025 15:29

Be more lenient on the maximum year offset

56563fe

clippy

a0b04e0

license

1a19387

robertbastian reviewed Aug 29, 2025

View reviewed changes

components/calendar/src/tests/extrema.rs Outdated Show resolved Hide resolved

remove assertion comments

d3db5e6

robertbastian reviewed Aug 29, 2025

View reviewed changes

components/calendar/src/tests/extrema.rs Outdated Show resolved Hide resolved

robertbastian reviewed Aug 29, 2025

View reviewed changes

Manishearth added 2 commits August 29, 2025 07:56

clamp

1afadfc

update test

3730d3e

sffc previously approved these changes Aug 29, 2025

View reviewed changes

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

test split

49a9e49

Manishearth dismissed sffc’s stale review via 49a9e49 August 29, 2025 17:56

Manishearth requested a review from robertbastian August 30, 2025 01:16

Add RataDie::in_well_behaved_astronomical_range(), use to avoid panics #6876

Are you sure you want to change the base?

Add RataDie::in_well_behaved_astronomical_range(), use to avoid panics #6876

Conversation

Manishearth commented Aug 18, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Manishearth Aug 29, 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!

sffc commented Aug 26, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Manishearth Aug 29, 2025 •

edited

Loading