Decide on potentially per-calendar behavior near extreme date values

[Manishearth]

Our calendars currently use i32 year values, and there are a couple ways that that can cause arithmetic overflow. The simplest way is when converting between different eras: i32::MAX reiwa is going to be an out-of-range extended_year. More subtle cases happen due to use of various calendars for internal calculation, or things like "get the R.D. as milliseconds". Some of these cases can be solved by using wider integer types, but by and large i32::MAX is not really a useful year to try and shove into this API.

We should ideally never be debug asserting in a way reachable to users.

#7062 attempts to improve the situation by adding coarse guards to pan-Calendar construction APIs: all year/extended_year inputs to from_fields/from_codes are now checked to be within ±2²⁷. It does not attempt to do anything about Date::try_new_from_specificcalendar, calendar conversions, or calendar arithmetic.

#7065 goes further to remove most sources of overflow errors except the small ones really close to i32::MAX. There's an open question there about Chinese: whether we should do millisecond math in i128, or error on those large dates.

We can and should try and expand the coarse year range. @robertbastian thinks we can get it to be ±2³⁰ (i32::MAX / 2). That would in and of itself be quite good, I think it would require us to run the simple chinese approximation in i128.

Regardless of the expansion of the range, we have some decisions to make. Note that all of these basically impact really large dates, which don't necessarily actually matter too much. It mostly matters that applications can use this code without experiencing strange behavior when users input overlarge dates.

The main one is that whether these ranges should be per calendar or pan-calendar. Per calendar means that we can be more or less liberal for individual calendars, pan calendar means that all calendars. This would mean that calendar conversions cannot be used to produce dates that cannot be directly produced, and thus all dates continue to roundtrip without error.

#7062 avoids the question by making it an input constraint on specific APIs: it's not really trying to figure out whether it should be per-calendar or pan-calendar, it is just saying that any year input on a pan-calendar API has a rough constraint.

If we decide it should be per-calendar, what happens on calendar conversion?

In either case, what happens on out-of-range duration arithmetic?

Note that these answers are entangled with implementation detail: it's not sufficient to say that we have per-calendar R.D. ranges, because from an implementation perspective you may need to guard against something before you have an R.D. to work with.

[Manishearth]

ugh, typed up a whole issue body and then clicked cancel by accident

[Manishearth]

Note that these answers are entangled with implementation detail: it's not sufficient to say that we have per-calendar R.D. ranges, because from an implementation perspective you may need to guard against something before you have an R.D. to work with.

Generally this reason is why I think the only strategy we should have is the current one of restricting inputs: if someone creates an "out of range" date by calendar conversion, that's fine: it's not going to be too much larger than the range min/max. It's also better for documentation, when a calendar (or the crate) can declare the valid range for year or extended_year.

We can decide on whether the input restriction needs to be global or per calendar, or per-API even.

[Manishearth]

cc @sffc

[sffc]

I remember at some point I had a RataDie limit that was i64 / 256 or something, in order to give "plenty" of room for transient calculations to use more bits in the RataDie.

We complain about Temporal's limits a lot, and it "only" requires 200k years. We don't need to go too much higher than that.

I had suggested 1e6 extended years, because I prefer round numbers in the Decimal system for human-imposed limits.

[Manishearth]

We complain about Temporal's limits a lot, and it "only" requires 200k years. We don't need to go too much higher than that.

FWIW I complain about Temporal's limits there because it is a spec and it mandates particular behavior there. I think specifications should be very careful about what they mandate, and it feels silly to mandate 200k years when most of the calendars do not have meaning in that range.

As a library I am more willing to try and exercise some freedom here. It's nice for our ISO calendar at least to have as much validity as possible. I'm not strongly on the side of "let's have a huge validity range", but it seems like if it's easy to attain, why not?

[sffc]

I'd be fine making the limit RD-based instead of year-based.

+/- 1e8 RD? (the smallest power of 10 that is greater than the Temporal limit)

Each calendar is required to return an error when attempting to construct a value exceeding that limit. This can be implemented cleanly for most ICU4X calendars by adding a trait method to CalendarArithmetic and then checking the ranges in the ArithmeticDate constructor.

Date::try_from_fields continues to enforce its safe extended year range to a number that exceeds the limits of the RD range in all calendars. I think 1e6 still works if the RD limit is 1e8.

To summarize:

1. User calls Date::try_from_fields.
2. Date::try_from_fields checks in a calendar-agnostic way whether the extended or era year exceeds +/- 1e6. If it does, it immediately returns a RangeError.
3. Date::try_from_fields calls calendar-specific code to convert the given fields to a calendar YMD.
4. Date::try_from_fields calls calendar-specific code to check whether the calendar YMD, when converted to a RD, is within RD +/- 1e8. Calendars can optimize this check to avoid actually converting to an RD. If the check fails, return a RangeError.
5. Done, return an Ok result.

[Manishearth]

I think that's a decent plan, and I appreciate the implementation details (since they're a part of the challenge here) The two-part range check on years and on RDs would make it work. We'd need to be careful to insert the RD checks in all the right places, but it's doable.

[sffc]

My post above focuses on try_from_fields (and try_new_from_codes which is a wrapper over try_from_fields). To address the other constructors (CC @robertbastian):

• from_rata_die is infallible. This suggests that RataDie's constructors should enforce the same range. Unfortunately RataDie::new is also infallible. (This is an example of why I really didn't want to graduate RataDie as part of icu_calendar's API.) So, we should mark either Date::from_rata_die or RataDie::new as deprecated and introduce a fallible variant.
• new_from_iso is infallible. This is fine, because Date<Iso> complies by these same bounds.
• from_raw should probably be named from_raw_unchecked to emphasize that it is skipping invariant checks and can produce panicky code. I suggest a deprecate-and-rename.
• try_new_buddhist, try_new_coptic, ... are already fallible, and they share some code with try_from_fields, so the bounds check should be implementable in those constructors.

[Manishearth]

Slight preference for deprecating from_rata_die, but I recognize that's much more churn than deprecating RataDie::new(). I'd like calendrical_calculations to not be beholden to any RD limits if possible.

from_raw should probably be named from_raw_unchecked to emphasize that it is skipping invariant checks and can produce panicky code. I suggest a deprecate-and-rename.

Is it even possible to make a raw SomeCalendar::Inner from outside of icu_calendar? Most of these types are internally private and unconstructible, IIRC.

[robertbastian]

from_rata_die already saturates at the range of ArithmeticDate, we can just have it saturate in a tighter range. I do not want to deprecate it.

Is it even possible to make a raw SomeCalendar::Inner from outside of icu_calendar?

No

[robertbastian]

If we limited years to 2^23 (or 2^20 if we want to be more round), we could pack an ArithmeticDate into 4 bytes. Months need 4 bits, days need 5. This would require a little work when YearInfo is not i32, but I think it's feasible.

[robertbastian]

add will also have to either saturate or return errors when it leaves the safe range.