Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix out of range panics in DateTime getters and setters #1317

Merged

Conversation

pitdicker
Copy link
Collaborator

Split out from #1048.

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 only fixes the first group of methods: those that panic on an intermediate value, but should be able to return a meaningful result. This includes formatting, and getters and setters of the Datelike and Timelike traits.

I made MIN_YEAR and MAX_YEAR 1 year smaller to have some buffer space. DateTime::overflowing_naive_local and NaiveDateTime::overflowing_add_offset are added to create a value in that buffer space.

The rest of the PR consists of just using these methods and adding tests.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #1317 (e002ce9) into 0.4.x (0032431) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            0.4.x    #1317      +/-   ##
==========================================
+ Coverage   91.25%   91.50%   +0.25%     
==========================================
  Files          38       38              
  Lines       17164    17310     +146     
==========================================
+ Hits        15663    15840     +177     
+ Misses       1501     1470      -31     
Files Coverage Δ
src/datetime/mod.rs 89.10% <100.00%> (+4.13%) ⬆️
src/datetime/tests.rs 96.77% <100.00%> (+0.27%) ⬆️
src/naive/date.rs 96.22% <100.00%> (+0.01%) ⬆️
src/naive/datetime/mod.rs 97.63% <100.00%> (+0.28%) ⬆️
src/naive/datetime/tests.rs 98.87% <100.00%> (+0.05%) ⬆️
src/naive/internals.rs 88.93% <ø> (+0.19%) ⬆️

... and 2 files with indirect coverage changes

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

@pitdicker
Copy link
Collaborator Author

This does not yet fix the with_year, checked_add_months, checked_sub_months, checked_add_days, and checked_sub_days methods on DateTime, or parsing. If we want to support round-tripping these should all be able to create dates in the buffer space, and I am not yet sure how to do so in a clean way. Parts of that work remain in #1048.

@pitdicker pitdicker force-pushed the datetime_overflow_getters_setters branch 2 times, most recently from bdc574a to 90e2607 Compare September 26, 2023 11:33
@pitdicker pitdicker force-pushed the datetime_overflow_getters_setters branch from 90e2607 to e002ce9 Compare September 26, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants