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 panic in TimeZone::from_local_datetime #1071

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented May 18, 2023

Split out from #1048. Depends on #1069 for NaiveDateTime::checked_sub_offset, only the last commit is new.

The change is that TimeZone::from_local_datetime should check for overflow instead of blindly subtracting the offset. But you can't do so easily in LocalResult::map, so this adds a helper method.

Note that this PR only fixes the default implementation. But the implementations of TimeZone for Local don't use the default implementation, and are not fixed yet.

The last commit in #1017 (comment) refactors Local to pick up this fix. @djc do you want me to split out that commit and try to apply it to 0.4.x? Edit: Split it out to #1072.

This was referenced May 18, 2023
src/naive/date.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

pitdicker commented May 19, 2023

It is a bit unfortunate that from_local_timezone is a provided method, that can be overridden by other implementers of TimeZone. On such a type from_local_timezone may still panic, like currently with Local.

I think it is better to implement the conversion in NaiveDateTime::and_local_timezone, and let the methods on TimeZone use that.

Then we can guarantee that NaiveDateTime::and_local_timezone never panics because of this issue, independent of the type implementing TimeZone. And by using and_local_timezone in Datetime::map_local, all Datetime::with_* methods inherit that guarantee.

@pitdicker
Copy link
Collaborator Author

I also found a place in format/parsed.rs that had a workaround because of this panic, which is no longer necessary.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pitdicker pitdicker force-pushed the from_local_datetime_panic branch 2 times, most recently from 1564bfd to cb98162 Compare May 23, 2023 11:55
@pitdicker pitdicker force-pushed the from_local_datetime_panic branch 2 times, most recently from 86cd777 to bc10f89 Compare August 6, 2023 19:28
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #1071 (e50dd89) into 0.4.x (c836a1f) will increase coverage by 0.01%.
Report is 2 commits behind head on 0.4.x.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##            0.4.x    #1071      +/-   ##
==========================================
+ Coverage   91.21%   91.23%   +0.01%     
==========================================
  Files          38       38              
  Lines       17121    17157      +36     
==========================================
+ Hits        15617    15653      +36     
  Misses       1504     1504              
Files Changed Coverage Δ
src/format/parsed.rs 96.91% <ø> (+0.10%) ⬆️
src/offset/mod.rs 56.00% <75.75%> (+2.31%) ⬆️
src/naive/datetime/tests.rs 98.82% <100.00%> (+0.04%) ⬆️

... 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

pitdicker commented Sep 22, 2023

It is a bit unfortunate that from_local_timezone is a provided method, that can be overridden by other implementers of TimeZone. On such a type from_local_timezone may still panic, like currently with Local.

I think it is better to implement the conversion in NaiveDateTime::and_local_timezone, and let the methods on TimeZone use that.

Then we can guarantee that NaiveDateTime::and_local_timezone never panics because of this issue, independent of the type implementing TimeZone. And by using and_local_timezone in Datetime::map_local, all Datetime::with_* methods inherit that guarantee.

This was based on a wrong assumption.
Why would you ever want to do something different from simply subtracting the offset in TimeZone::from_local_timezone?
Answer: when implementing something like a Tai timezone, that should handle leap seconds differently.

So this is now back to a simple fix of the provided method TimeZone::from_local_timezone.

@pitdicker pitdicker force-pushed the from_local_datetime_panic branch 2 times, most recently from 88add13 to 9454970 Compare September 25, 2023 11:38
@pitdicker
Copy link
Collaborator Author

This no longer depends on other PRs.

@@ -527,10 +527,55 @@ pub trait TimeZone: Sized + Clone {
}
}

/// Helper function to map `LocalResult<FixedOffset>` to `LocalResult<DateTime<Local>>`.
/// Returns `None` on out-of-range.
fn local_time_min_offset<Tz: TimeZone>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this has a single caller, let's just inline it?

Copy link
Collaborator Author

@pitdicker pitdicker Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a separate function I could use ?, and convert to LocalResult::None in the caller. It seemed the cleanest solution at the time.

Found a nicer way to write it now.

@pitdicker pitdicker merged commit 0032431 into chronotope:0.4.x Sep 26, 2023
36 of 37 checks passed
@pitdicker pitdicker deleted the from_local_datetime_panic branch September 26, 2023 15:44
@pitdicker
Copy link
Collaborator Author

Thank you for all your reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants