Fix DateTime out-of-range panics

[pitdicker]

If the timestamp of a DateTime is near the end of the range of NaiveDateTime and the offset pushes the timestamp beyond that range, all kinds of methods currently panic. About 40 methods that are part of the public API. With the Debug implementation panicking being the most fun. See #1047.

Most of these methods fall in two categories:

• They panic on an intermediate value, but should be able to return a meaningful result. Examples are formatting, year(), hour().
• They can return None. Examples are checked_add_days, with_month.

A somewhat easy fix is to slightly restrict the range of NaiveDate, so that we have some buffer space to represent the out-of-range datetimes. Everything that needs just the intermediate value will be fixed by this. But care should be taken to never let an invalid intermediate value escape to the library user.

---

This PR grew larger than hoped. I'll describe the various commits to hopefully help review.

Adjust MIN_YEAR and MAX_YEAR

I made MIN_YEAR and MAX_YEAR smaller, so that we have 1 year of buffer space. 1 day would have been enough, but having the minimum date be January 2 and the maximum date December 30 is just strange.

NaiveDate::MIN and NaiveDate::MAX are derived from these. There is a very helpful test_date_bounds to confirm the flags and oridinal are correct.

There is something subtly wrong in the calculation of MIN_DAYS_FROM_YEAR_0, which is only used in tests. It was only correct if MIN_YEAR was a leap year. I changed it to a correct formula that tries to be less smart, but couldn't figure out the problem in the derivation yet. Added a comment.

checked_add_offset and unchecked_add_offset

Instead of panicking in the Add or Sub implementation of NaiveDateTime with FixedOffset, we need a way to be informed of out-of-range result. Or in other cases we need to be able to construct a value in the buffer space for intermediate use.

I added the following methods (not public for now):

• NaiveTime::overflowing_add_offset and NaiveTime::overflowing_sub_offset
• NaiveDateTime::checked_add_offset and NaiveDateTime::checked_sub_offset
• NaiveDateTime::unchecked_add_offset (in a later commit)

The Add and Sub implementations of FixedOffset with NaiveTime, NaiveDateTime and DateTime are reimplemented using of these methods. This fixes a code comment:

this should be implemented more efficiently, but for the time being, this is generic right now.

The best place to put the Add and Sub implementations for DateTime where in the module of DateTime, because there they have access to its private fields. I have moved all implementations to the module of their output type for consistency.

Simplify implementation

Adding an offset works differently from adding a duration. Adding a duration tries to do something smart—but still rudimentary—with leap seconds. Adding an offset should not touch the leap seconds at all. So the methods that operate on NaiveTime should be different.

I extracted the part that operates on the NaiveDate that could be shared into an add_days methods. Previously NaiveDate::checked_add_days would convert the days to a Duration in seconds, and later devide them to get the value in days again. This now works in a less roundabout way. Might also help with the const implementation later.

Fix creating a DateTime with NaiveDateTime::{MIN, MAX}

The creation of a DateTime in TimeZone::from_local_datetime should use checked_sub_offset, and return LocalResult::None on overflow.

The implementation of Local had grown into a mess (sorry, that is the best word for it). With the last commit in #1017 they should just use the provided implementation and pick up this fix.

The actual fixes

The new method DateTime::overflowing_naive_local is not public, and can be used to create an out-of-bounds NaiveDateTime for use as an intermediate value.

Most of the problematic methods on DateTime simply work when converted to use overflowing_naive_local. If they don't return a DateTime, NaiveDateTime or NaiveDate there is also no worry the intermediate value may be exposed outside chrono.

The DateTime::with_* methods that are part of the Datelike trait have an interesting property: if the local NaiveDateTime would be out-of-range, there is only exactly one year/month/day/ordinal they can be set to that would result in a valid DateTime: the one that is already there. This is thanks to the restriction that offset is always less then 24h. To prevent creating an out-of-range NaiveDateTime all these methods short-circuit when possible.

The only two methods on NaiveDate (note: not DateTime) that had to change are checked_add_months and checked_sub_months. Both had a short-circuiting behaviour that with the changes in this PR could return invalid dates. When the input is out of range and months == 0, checked_*_months didn't check whether the result would be valid.

Not fixed: parsing

All the relevant methods on Parsed are public.

Methods in there like Parsed::to_naive_date, Parsed::to_naive_datetime_with_offset and NaiveDateTime::from_timestamp_opt can't be converted to return an intermediate out-of-range value.

I have left it as is. Parsing doesn't panic, it just can't round-trip some DateTime's.

Tests

About 275 lines in this PR are tests, so its size may not be as bad as it looks. @jtmoon79 I tried to behave 😇

test_min_max_datetimes is in my opinion the interesting one, testing all methods that would previously panic.

[pitdicker]

Found another somewhat related panic.

NaiveWeek::last_day creates an intermediate date with NaiveWeek::first_day, and adds 7. For weeks near NaiveDate::MIN the intermediate date could be out of range, while the final value is representable.

This was easily fixable in the implementation of last_day.

let date_min = NaiveDate::MIN;
assert!(date_min.week(Weekday::Mon).last_day() >= date_min);

[djc]

Yeah, this PR is too large for me to digest. If you have to preface with a bunch of prose in the PR description to make it all make sense, you've already lost me. Please split this in smaller chunks that I can easily review (here's some early feedback from the first couple of commits).

[djc]

I'd like this commit to be split in multiple parts that do one thing each. This seems to do refactoring (divide by 1000) which is distracting from the more material change of the value, making it harder to review.

[pitdicker]

If you have to preface with a bunch of prose in the PR description to make it all make sense, you've already left.

Haha. That's fair. I'll see what I can do to split it up.
At least this PR shows how it all comes together to avoid the panics 😄.

[pitdicker]

I split out three PRs from this one. It now depends on #1071, which in turn depends on #1069. I'll try to clean up the commits here a bit more, so setting the status to draft for now.

[pitdicker]

Okay, I have done my best to split this up into smaller commits.
The ones that are not part of #1071 start at "Fix *_DAYS_FROM_YEAR_0 calculation:.

[codecov]

Codecov Report

Merging #1048 (ffee507) into 0.4.x (604b92a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            0.4.x    #1048      +/-   ##
==========================================
+ Coverage   91.50%   91.55%   +0.04%     
==========================================
  Files          38       38              
  Lines       17314    17385      +71     
==========================================
+ Hits        15844    15916      +72     
+ Misses       1470     1469       -1     

Files	Coverage Δ
src/datetime/mod.rs	89.35% <100.00%> (+0.06%)	⬆️
src/datetime/tests.rs	96.92% <100.00%> (+0.13%)	⬆️
src/naive/date.rs	96.29% <100.00%> (+0.06%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[djc]

FWIW, would help me to start the PR description out with other PRs that it depends on.