Correctly check the RD range for Date::try_from_fields and Date::try_add* by Manishearth · Pull Request #7676 · unicode-org/icu4x

Manishearth · 2026-02-20T05:56:11Z

Progress on #7076

This does a bunch of things:

Adds GENEROUS_YEAR_RANGE, which is a year range that is slightly wider than VALID_RD_RANGE for all calendars, "good enough" for early checks.
Uses GENEROUS_YEAR_RANGE as a precondition in try_from_fields, and also checks VALID_RD_RANGE as a postcondition.
Tests for try_from_fields.
Adds (internal) YearOverflowError used for internal functions which can only error when out of year range
Changes (internal) DateFieldsResolver::year_info_from_era to extended_year_from_era_year. This splits its responsibilities more cleanly: it doesn't have to bother with generating a YearInfo (which is risky), it just adjusts the era year.
Adds a (internal) DateFieldsResolver::year_info_from_extended_checked which checks the generous range and uses it in from_fields and arithmetic code.
Adds DateAddError based on consensus in Consider narrower error type between try_new_from_codes and try_from_fields #7010.
Updates the fuzzer to remove guards around addition

If there are issues in DateAddError or the test, I would prefer to work on them separately, since this PR is getting pretty large.

Manishearth · 2026-02-20T05:57:29Z

components/calendar/src/calendar_arithmetic.rs

    ) -> Result<Self, DateAddError> {
+        // We preemptively protect ourselves from overly-large values
+        //
+        // This is mostly needed for avoiding overflows on Duration arithmetic,


Open to different approaches here. It feels a bit strange to have all of this nice infrastructure range-checking years and we still need a coarse preemptive check because the duration math overflows.

One thing we could do is have surpasses_inner returning a YearOverflowError, and surpasses() returning a bool where the error is turned into true.

I don't know this code enough to comment on that. However, if you're establishing an invariant here, propagate it in documentation to the relevant functions (I guess add_year_to, month_from_ordinal?)

I think the "coarse preemptive check" should be the only check, until the final RD check at the end.

… cases

components/calendar/src/calendar_arithmetic.rs

components/calendar/src/error.rs

components/calendar/src/calendar_arithmetic.rs

robertbastian · 2026-02-23T16:42:37Z

components/calendar/src/calendar_arithmetic.rs

    ) -> Result<Self, DateAddError> {
+        // We preemptively protect ourselves from overly-large values
+        //
+        // This is mostly needed for avoiding overflows on Duration arithmetic,


I don't know this code enough to comment on that. However, if you're establishing an invariant here, propagate it in documentation to the relevant functions (I guess add_year_to, month_from_ordinal?)

robertbastian · 2026-02-23T16:43:58Z

components/calendar/src/calendar_arithmetic.rs

-                base_month,
-                DateFromFieldsOptions::from_add_options(options),
-            )
-            .map_err(|e| {


prefer this as well

Why? In this case the errors map cleanly. If the errors stop mapping cleanly we can introduce a new match the way we did before.

this is less readable if you're trying to figure out how an error was produced. rust-analyzer cannot tell you which ?s implicitly use the From implementation

That's just a general problem with Rust errors: that's an argument against using ? period. These errors are going to go through ? a half dozen more times before hitting the user. Changing that here won't do anything.

There are Rust error libraries that will attach context to ? for you but they're heavier so we don't use them.

That's just a general problem with Rust errors: that's an argument against using ? period.

yep. doesn't mean it's not an argument here.

These errors are going to go through ? a half dozen more times before hitting the user.

that's not our problem. they don't go through another ? before being returned from our public API, and appearing in our unit tests where it's often needed to figure out where they're from

Wait, hold on, I already fixed this instance of this pattern because the errors do not map cleanly.. I thought you were talking about the other instance of this pattern.

But still, I disagree that we should avoid ?. It's annoying in unit tests, but it's also annoying to wade through extra match statements.

well it's not an extra match statement, the MonthError to LunisolarDateError conversion only happens in once, let's just keep it there and not move it to another file

Okay, so you are talking about the other instance of this pattern.

It's an extra match statement within complicated code. Moving the match statement elsewhere is better. All of the calendar arithmetic/construction code is already messy.

I could introduce a .map_err(MonthError::into)? if you really want but I still think that is kind of silly. The Rust community has pretty universally settled on the style of using ? wherever it works and I don't plan on changing how I use ? unless you argue for and land a style rule for it.

robertbastian · 2026-02-23T16:45:52Z

components/calendar/src/calendar_arithmetic.rs

            cal,
-        ))
+        )?;
+        // We early checked for a generous range of years, now we must check


ah, here we go. move this check into new_balanced as that calls ArithmeticDate::new_unchecked and violates its invariant

We cannot, new_balanced must be able to produce slightly-out-of-range years.

I can't see a way to make the code work without it. I documented this new invariant on new_balanced, and I'm about to push a commit that documents it more. I also updated the documentation on new_unchecked with the new invariant.

When I wrote the tests my core desired property was that for arithmetic should work as long as the input and output were in RD range. The arithmetic algorithm needs to be able to temporarily "peek" out of range to do so, see the end_of_month calculation here (and basically all of the surpasses stuff used in until). So new_balanced needs to work for near-edge dates on either side of the range boundary, which means we cannot have it follow the strict RD range.

If we want to give up that property we need to more carefully figure out what the desired behavior is here. I find that property to be rather sensible and nice to have.

We could add new_unchecked_with_assertion that debug asserts in all of the other call sites that it is within RD range. But I don't really think it's necessary.

components/calendar/src/calendar_arithmetic.rs

robertbastian · 2026-02-23T22:32:50Z

components/calendar/src/error.rs

+        /// use icu::calendar::options::{DateAddOptions, Overflow};
+        ///
+        /// // Hebrew year 5784 is a leap year, 5785 is not.
+        /// // Adar I (the leap month) is month 5 in a leap year.


isn't it month 6?

sffc · 2026-02-23T21:40:53Z

components/calendar/src/cal/east_asian_traditional.rs

    #[inline]
    fn year_info_from_extended(&self, extended_year: i32) -> Self::YearInfo {
-        debug_assert!(crate::calendar_arithmetic::VALID_YEAR_RANGE.contains(&extended_year));
+        debug_assert!(crate::calendar_arithmetic::GENEROUS_YEAR_RANGE.contains(&extended_year));


Thought: This check doesn't really achieve anything since GENEROUS_YEAR_RANGE is bigger than the RD range. I guess it's harmless

Yeah, this was added deliberately because previously it was quite haphazard, it's nice to have a check right before we call the tricky calendrical APIs.

components/calendar/src/calendar_arithmetic.rs

components/calendar/src/tests/extrema.rs

sffc · 2026-02-23T22:20:44Z

components/calendar/src/calendar_arithmetic.rs

+        let extended_year = if let Some(era) = era {
+            calendar.extended_year_from_era_year(
+                era.as_bytes(),
+                range_check(year, "era_year", CONSTRUCTOR_YEAR_RANGE)?,


Issue: I think this one should check GENEROUS_YEAR_RANGE because it seems really weird for era year and extended year ranges to be double-checked. It results in a smaller effective range. This is especially noticeable in a calendar where the era and extended years are not the same, like Ethiopian with input era "aa"; only ~15000 dates can be constructed instead of ~20000 dates when the era and extended year are the same.

Alternatively, don't check the extended year range if the input was an era year.

This is existing documented behavior (on from_codes). Separate discussion to change if desired.

sffc · 2026-02-23T22:26:50Z

components/calendar/src/calendar_arithmetic.rs

+        let rd = C::to_rata_die_inner(year, month, day);
+
+        // We early checked for a generous range of years, now we must check
+        // to ensure we are actually in range for our core invariant.
+        if !VALID_RD_RANGE.contains(&rd) {
+            return Err(DateFromFieldsError::Overflow);
+        }


Issue: I don't love converting to RD just to check the RD range. I would rather have a calendar-specific check. This function is already generic in the calendar. (ok for follow-up)

I don't really wish to maintain calendar-specific range checking code, it's too easy to get wrong.

Fine for follow-up. to_rata_die is slow and we otherwise don't need it. Maintaining a const calendar-specific min and max date where we use Ord to check if in-range is fine and better.

sffc · 2026-02-23T22:29:09Z

components/calendar/src/calendar_arithmetic.rs

            year, // == year_info.to_extended_year() + offset
            "year",
-            (VALID_YEAR_RANGE.start() + offset)..=(VALID_YEAR_RANGE.end() + offset),
+            (CONSTRUCTOR_YEAR_RANGE.start() + offset)..=(CONSTRUCTOR_YEAR_RANGE.end() + offset),


Issue (here and elsewhere, similar to above): I really don't like checking the year against CONSTRUCTOR_YEAR_RANGE multiple times

Preexisting behavior, often (but not always?) documented. @robertbastian introduced this I believe.

sffc · 2026-02-23T22:33:10Z

components/calendar/src/calendar_arithmetic.rs

+            resolved_year =
+                cal.year_info_from_extended_checked(resolved_year.to_extended_year() - 1)?;


Issue: I thought we were going to make sure the input to this function (new_balanced) was such that we never cause calendar-specific integer overflow. A precondition should be that year is in the generous range, ordinal_month does not exceed the generous range converted to months, and day does not exceed the generous range converted to days.

sffc · 2026-02-23T22:35:38Z

components/calendar/src/calendar_arithmetic.rs

    ) -> Result<Self, DateAddError> {
+        // We preemptively protect ourselves from overly-large values
+        //
+        // This is mostly needed for avoiding overflows on Duration arithmetic,


I think the "coarse preemptive check" should be the only check, until the final RD check at the end.

components/calendar/src/calendar_arithmetic.rs

Manishearth · 2026-02-23T23:22:45Z

Regarding the coarse presumtpive checks and the other checks:

My main worry is that YearInfo calculations should never get a year that is too large for them to handle, and should be able to preemptively assert that they get a good year. I don't want calendar-specific code to need to get into the business of worrying too much about range.

My first commit here introduced SAFE_YEAR_RANGE, which was "the year range that is safe for all internal arithmetic. I got rid of it, though, since it would need to be much larger to work in the arithmetic space. I don't like expanding the safe year range too much (we have previously seen fuzzer issues from years in the millions. This might have since been fixed.)

I also want to limit overly-slow APIs. #7077 is the issue for fixing that, and I'm hoping to work on it, but it's not a release blocker.

~~I'm happy to make a new PR that reintroduces that change. It ought to simplify the code significantly. But I'd like to land this first.~~

I made the change. It's a separate commit. If people later disagree about this architecture, please open a PR with a revert of that commit.

Manishearth requested a review from sffc as a code owner February 20, 2026 05:56

Manishearth requested a review from robertbastian February 20, 2026 05:56

Manishearth commented Feb 20, 2026

View reviewed changes

Manishearth added 8 commits February 19, 2026 22:29

Add generous year range check to from_fields

28d163b

Replace year_info_from_era with extended_year_from_era_year

9684e2e

Introduce year_info_from_extended_checked for use in overflow-checked…

d509ec4

… cases

Add and use DateAddError

a772a33

use year_info_from_extended_checked in arithmetic code

51f7590

Add preemptive check for date addition

ad51e40

Update fuzzer

19f2e02

Add test for arithmetic

2390e3f

Manishearth force-pushed the range-checks branch from 76b730c to 2390e3f Compare February 20, 2026 07:25

robertbastian reviewed Feb 23, 2026

View reviewed changes

Review comments

27cd5b6

Manishearth requested a review from robertbastian February 23, 2026 17:30

robertbastian reviewed Feb 23, 2026

View reviewed changes

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

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

Manishearth added 2 commits February 23, 2026 10:30

VALID_YEAR_RANGE to CONSTRUCTOR_YEAR_RANGE

843625a

Have new_balanced return an unchecked arithmetic date

c9afc3c

robertbastian reviewed Feb 23, 2026

View reviewed changes

Manishearth added 2 commits February 23, 2026 11:12

address comments

542635e

document tests

7e619e6

Manishearth requested a review from robertbastian February 23, 2026 21:23

Manishearth added 2 commits February 23, 2026 13:25

fix

b50965c

clippy

867db09

Manishearth force-pushed the range-checks branch from 3f44cd3 to 867db09 Compare February 23, 2026 22:08

robertbastian reviewed Feb 23, 2026

View reviewed changes

sffc requested changes Feb 23, 2026

View reviewed changes

Manishearth requested a review from sffc February 23, 2026 23:17

unchecked

5f32e15

Reintroduce SAFE_YEAR_RANGE

6994df0

Manishearth force-pushed the range-checks branch from eabc756 to 6994df0 Compare February 23, 2026 23:33

sffc approved these changes Feb 23, 2026

View reviewed changes

fix

c4eb9d1

sffc approved these changes Feb 24, 2026

View reviewed changes

Manishearth merged commit 7054d89 into unicode-org:main Feb 24, 2026
32 checks passed

Manishearth deleted the range-checks branch February 24, 2026 01:02

		resolved_year =
		cal.year_info_from_extended_checked(resolved_year.to_extended_year() - 1)?;

Comments

Conversation

Manishearth commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth Feb 20, 2026 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

Manishearth commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manishearth commented Feb 20, 2026 •

edited

Loading

Manishearth Feb 20, 2026 •

edited

Loading

Manishearth commented Feb 23, 2026 •

edited

Loading