Document constructor ranges #7227

robertbastian · 2025-11-07T12:04:50Z

Changed the range from applying to era years to applying to extended years. We cannot apply it to era years, as Japanese has data-driven eras higher than any era from any calendar, so a new Japanese era would need a bigger RD range.

#7076

components/calendar/src/cal/buddhist.rs

components/calendar/src/calendar_arithmetic.rs

Manishearth · 2025-11-07T17:24:14Z

components/calendar/src/date.rs

-    /// Construct a date from a [`RataDie`] and some calendar representation
+    /// The lowest [`RataDie`] that will round-trip through [`Date::from_rata_die`]
+    /// and [`Date::to_rata_die`].
+    pub const LOWEST_RATA_DIE: RataDie = *VALID_RD_RANGE.start();


note: we should make sure this new public API is where we want it

I think it is.

Alternative:

Date::new_max().to_rata_die()

new_max returns Date<Iso>

I think to_rata_die() can't be called in const

I want to stop treating ISO as a special calendar, RD is the calendar-neutral date representation. Also I don't want to expose this as the max date, only as the max input for the from_rata_die function.

I think that's reasonable.

hmm it's not ideal, because Date is generic and callers have to pick a calendar to access the const, even though it's calendar-agnostic

If from_rata_die saturates, people can get the max via

Date::from_rata_die(RataDie::MAX_VALUE).to_rata_die()

RataDie::MAX_VALUE does not exist though

And we probably shouldn't add one since this is an icu_calendar constraint, not a calendrical_calculations constraint.

I'm not overly bothered by this value requiring you to specify a calendar. I don't expect this to be used a lot.

I had intended to observe that if we were to add RataDie::MAX_VALUE returning i64::MAX in calendrical_calculations, icu_calendar still results in it being constrained.

sffc · 2025-11-07T17:40:53Z

components/calendar/src/cal/buddhist.rs

+    /// Construct a new Buddhist [`Date`].
    ///
-    /// Years are specified as BE years.
+    /// Years are arithmetic, meaning there is a year 0 preceded by negative years, with a


Nit: This sentence doesn't read correctly for me. The word "arithmetic" is a noun by default, so "years are arithmetic" doesn't make sense. If using "arithmetic" as an adjective, please don't use it in a position in the sentence where it could be interpreted as a noun.

I didn't write this

icu4x/components/calendar/src/cal/julian.rs

Line 275 in b6b00b9

/// Years are arithmetic, meaning there is a year 0. Zero and negative years are in BC, with year 0 = 1 BC

components/calendar/src/calendar_arithmetic.rs

sffc · 2025-11-07T17:44:30Z

components/calendar/src/calendar_arithmetic.rs

            (Some(era), Some(era_year)) => {
-                let era_year_as_year_info = calendar
-                    .year_info_from_era(era, range_check(era_year, "year", VALID_YEAR_RANGE)?)?;
+                let year = calendar.year_info_from_era(era, era_year)?;


Issue: The era_year shouldn't get plumbed into the calendar impl until we've checked that it is in some reasonable range, maybe +/- 1e7, to allow calendars like simplified chinese to make assumptions that years aren't too big.

I don't think this is fixed. It looks like you fixed it for calendar-specific constructors, but not for from-codes or from-fields.

sffc · 2025-11-07T17:45:53Z

components/calendar/src/date.rs

-    /// Construct a date from a [`RataDie`] and some calendar representation
+    /// The lowest [`RataDie`] that will round-trip through [`Date::from_rata_die`]
+    /// and [`Date::to_rata_die`].
+    pub const LOWEST_RATA_DIE: RataDie = *VALID_RD_RANGE.start();


Alternative:

Date::new_max().to_rata_die()

new_max returns Date<Iso>

sffc · 2025-11-07T17:49:10Z

components/calendar/src/types.rs

+        /// [`Date::try_from_fields`](crate::Date::try_from_fields) accepts years that correspond to
+        /// `extended_year`s in the range `-1,000,000..=1,000,000`.


Suggestion/Issue: I think this should range-check the era year to +/- 1e6.

The range check should either be on the input or on the output. If it is on the input, it should check the era year. If it is on the output, it should check the rata die. It doesn't make sense to range check the extended year, since it is the output of a calculation, which is valid so long as the Date is in range of its invariant.

as explained in the PR description, that doesn't work because anyone can add a Japanese era that starts in the year 2e6.

Manishearth · 2025-11-07T21:14:07Z

Can we special case Japanese so that it checks in eraYear range and then also checks RD ranges?

sffc · 2025-11-07T22:55:17Z

My concern is that someone could give era: "reiwa", era_year: i32::MAX which would cause an arithmetic overflow unless we constrain era_year first.

sffc · 2025-11-07T22:58:48Z

I'm okay constraining both era year and extended year. The behavior would be, like,

Input	Range Error Field	Min	Max	Note
era: "reiwa", era_year: i32::MAX	era_year	-1e6	1e6
extended_year: i32::MAX	extended_year	^	^
era: "reiwa", era_year: 1e6	extended_year	^	^	Valid era year but not extended year

Manishearth · 2025-11-07T23:13:11Z

Yeah, that's my concern too, and that's the blocking part of my comment there.

Manishearth · 2025-11-07T23:43:11Z

Hmm, doing a double check everywhere is fine, I guess. I was hoping we could just check on the inputted year and specifically in the Japanese case constrain extra.

But this is fine.

sffc

Would really love to see tests for the limits across the constructors.

sffc · 2025-11-08T01:21:22Z

components/calendar/src/calendar_arithmetic.rs

            (Some(era), Some(era_year)) => {
-                let era_year_as_year_info = calendar
-                    .year_info_from_era(era, range_check(era_year, "year", VALID_YEAR_RANGE)?)?;
+                let year = calendar.year_info_from_era(era, era_year)?;


I don't think this is fixed. It looks like you fixed it for calendar-specific constructors, but not for from-codes or from-fields.

Manishearth

I think this seems about right!

If you would like you can remove the guards in the fuzz tests but don't worry about it, I can do that myself later.

sffc · 2025-11-10T21:50:48Z

(I do consider tests to be a requirement for shipping this new "feature", but they can be in a follow-up if necessary)

robertbastian requested review from Manishearth and sffc as code owners November 7, 2025 12:04

robertbastian force-pushed the ctor-docs branch from 95b6ee7 to d521063 Compare November 7, 2025 13:01

docs

6123a1d

robertbastian force-pushed the ctor-docs branch from d521063 to 6123a1d Compare November 7, 2025 13:37

Manishearth reviewed Nov 7, 2025

View reviewed changes

sffc reviewed Nov 7, 2025

View reviewed changes

wip

4f595fb

robertbastian requested review from Manishearth and sffc November 7, 2025 20:04

check both ranges

17ded15

sffc reviewed Nov 8, 2025

View reviewed changes

robertbastian added 2 commits November 10, 2025 12:08

remove ranges

9b721e4

protect all year_info_from_era calls

d2ea0a9

Manishearth approved these changes Nov 10, 2025

View reviewed changes

		/// [`Date::try_from_fields`](crate::Date::try_from_fields) accepts years that correspond to
		/// `extended_year`s in the range `-1,000,000..=1,000,000`.

Document constructor ranges #7227

Are you sure you want to change the base?

Document constructor ranges #7227

Conversation

robertbastian commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Nov 7, 2025

Uh oh!

sffc commented Nov 7, 2025

Uh oh!

sffc commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth commented Nov 7, 2025

Uh oh!

Manishearth commented Nov 7, 2025

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robertbastian commented Nov 7, 2025 •

edited

Loading

sffc commented Nov 7, 2025 •

edited

Loading