-
Notifications
You must be signed in to change notification settings - Fork 531
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
Windows: base implementation on GetTimeZoneInformationForYear
#1017
Conversation
Did you see #997 already? |
It looks like these two PRs should be somewhat orthogonal, but this is also related to #750 which is great as by extending this we should be able to extract the name of the timezone as well |
Can you add that here as a separate commit? It does sound like something we'd want. |
Somewhat, I forgot about it. That workaround would no longer be necessary. But the PR before by @nekevss was very useful for feeling my way in the Windows implementation.
I can dig up the commit, but it will probably be tomorrow because it needs some cleaning up. |
We could also review and merge this as is, before enhancing to get the timezone name via another PR if that is more suitable @djc? |
I added the commits to share part of the implementation with Unix. It touches more than I like, because Windows and Unix need to pass around the same type for the std/dst offsets: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
2b1bb99
to
d3be65b
Compare
Just noticed issue #651. That issue would be fixed by this PR, added a test. |
I noticed On Windows the standard library uses |
The benchmark results have improved a bit.
After:
|
src/offset/local/mod.rs
Outdated
|
||
#[cfg(any(unix, windows))] | ||
impl TzInfo { | ||
fn lookup_with_dst_transitions(&self, dt: &NaiveDateTime) -> LocalResult<FixedOffset> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me hours to convince myself the logic in this function was correct in the Unix version, so I simplified it a bit here.
If the first transition is to DST, the year starts in STD, transitions to DST, and ends with STD.
If the first transition is to STD, the opposite.
For every transition we take in local time the earliest clock value, and latest clock value (transition_min
and transition_max
).
- If the transition crates a gap, all times exclusive do not exist
(*_transition_min..*_transition_max)
. - If the transition crates an overlap, all times inclusive do not exist
[*_transition_min..*_transition_max]
. - The time in between the transitions should have the remaining comparison operator (i.e.
>
if the transition uses<=
).
The result to return in LocalResult::Single
is straightforward, dst_offset
or std_offset
as appropriate.
The correct order in LocalResult::Ambiguous
is the offset right before the transition, then the offset right after.
Hope that helps for the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a delicate function. I'd feel better if there was a fn test_lookup_with_dst_transitions
that exercises the logic of every branch within, and of any corner cases.
I'm guessing the test case below verify_against_tz_specific_local_time_to_system_time()
might also do that. I think it'd be an improvement to have a 1:1 test case just for this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you lay your explanation here down in a comment?
If this is created to replace existing logic for the Unix time zone logic, I think we should switch the Unix implementation in the same commit so that it's easier for me to compare the new code with the old code.
src/offset/local/mod.rs
Outdated
|
||
#[cfg(any(unix, windows))] | ||
impl TzInfo { | ||
fn lookup_with_dst_transitions(&self, dt: &NaiveDateTime) -> LocalResult<FixedOffset> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a delicate function. I'd feel better if there was a fn test_lookup_with_dst_transitions
that exercises the logic of every branch within, and of any corner cases.
I'm guessing the test case below verify_against_tz_specific_local_time_to_system_time()
might also do that. I think it'd be an improvement to have a 1:1 test case just for this function.
@jtmoon79 Thank you for the review again! I am a slow learning when it comes to adding tests 🤣. This will take some work. |
f078179
to
87b45ba
Compare
Now that Windows supports for greater year ranges, it might overflow when near the limits of
This can be fixed inside the provided implementation of But |
Thought better of this. I have removed the commit that fixes handling overflow when near the limits of I have added a commit to this PR removing all the roundabout stuff in the Now that all inner modules return offsets, any fix for overflow handling at the |
d6e8c7a
to
47f240c
Compare
Thank you so much for working on this. The lasted update does simplify things and I find it much easier to follow. |
]; | ||
compare_lookup(&transitions, 2023, 3, 26, 2, 0, 0, LocalResult::Single(std)); | ||
compare_lookup(&transitions, 2023, 10, 29, 3, 0, 0, LocalResult::Single(std)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worthwhile adding edge cases to this test function test_lookup_with_dst_transitions
? i.e. should this also test transitions at or near DateTime::MIN
and DateTime::MAX
(or whatever the minimum and maximum should on Windows)? At first glance, that looks important to me.
::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToFileTime(lpsystemtime : *const SYSTEMTIME, lpfiletime : *mut FILETIME) -> BOOL); | ||
::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToTzSpecificLocalTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lpuniversaltime : *const SYSTEMTIME, lplocaltime : *mut SYSTEMTIME) -> BOOL); | ||
::windows_targets::link!("kernel32.dll" "system" fn TzSpecificLocalTimeToSystemTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lplocaltime : *const SYSTEMTIME, lpuniversaltime : *mut SYSTEMTIME) -> BOOL); | ||
pub type BOOL = i32; | ||
pub type BOOLEAN = u8; | ||
#[repr(C)] | ||
pub struct DYNAMIC_TIME_ZONE_INFORMATION { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct seems to have dropped out of the sky.
I prefer to give the reader some help finding the source material for magic values and structures, e.g.
/// struct from Windows Win32 API
/// <https://learn.microsoft.com/en-us/windows/win32/api/timezoneapi/ns-timezoneapi-dynamic_time_zone_information>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct seems to have dropped out of the sky.
Quite close, the entire file gets generated by windows-bindgen.
|
||
// We don't use `SystemTimeToTzSpecificLocalTime` because it doesn't support the same range of dates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great comment.
let local_secs = system_time_as_unix_seconds(&local_time)?; | ||
let offset = (local_secs - utc_secs) as i32; | ||
Ok(FixedOffset::east_opt(offset).unwrap()) | ||
// The basis for Windows timezone and DST support has been in place since Windows 2000. It does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great info @ChrisDenton . I suggest placing your reply into the code as a code comment including the links.
1d930f1
to
cd740d0
Compare
I think the logic is pretty much what I would want it to be. The logic in If anyone wants to help out with adding some tests I would deeply appreciate it. But otherwise I'll get to it. |
5905846
to
9cc4572
Compare
Added tests for |
src/offset/local/windows.rs
Outdated
// The basis for Windows timezone and DST support has been in place since Windows 2000. It does not | ||
// allow for complex rules like the IANA timezone database: | ||
// - A timezone has the same base offset the whole year. | ||
// - There some to be either zero or two DST transitions (but we support having just one). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"There some to be"?
src/offset/local/windows.rs
Outdated
let local_secs = system_time_as_unix_seconds(&local_time)?; | ||
let offset = (local_secs - utc_secs) as i32; | ||
Ok(FixedOffset::east_opt(offset).unwrap()) | ||
fn tz_info_for_year(year: i32) -> Option<TzInfo> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a method? TzInfo::for_year()
?
src/offset/local/windows.rs
Outdated
wSecond: dt.second() as u16, | ||
// Valid values: 0-999 | ||
wMilliseconds: 0, | ||
fn systemtime_to_naive_dt(st: SYSTEMTIME, year: i32) -> Option<NaiveDateTime> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I preferred the old name? (And don't see a strong reason to change it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have anything like this function before. I don't mind changing the name to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry -- I was confused because the diff shows this right next to system_time_from_naive_date_time()
and I missed the from
/to
difference. I think system_time
is more idiomatic than systemtime
, and would probably prefer to spell out date_time
over dt
for this function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
#[test] | ||
#[cfg(feature = "clock")] | ||
fn test_datetime_before_windows_api_limits() { | ||
// dt corresponds to `FILETIME = 147221225472` from issue 651. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you refer to the issue by making it a link?
846a6ff
to
f6fd7fa
Compare
src/offset/local/windows.rs
Outdated
// Valid values: 0-999 | ||
wMilliseconds: 0, | ||
impl TzInfo { | ||
fn tz_info_for_year(year: i32) -> Option<TzInfo> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested previously, let's call this for_year()
-- no need to duplicate the type name in the method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, only now I get your intention. Yes, that is better.
And add test against previous implementation
f6fd7fa
to
3d7daa5
Compare
🎉Happy to see this completed. Thank you @djc, @ChrisDenton and others! |
Oh wow! Thanks so much for working on this @pitdicker, you're a star! |
Issues caused by the current use of Windows API's:
TzSpecificLocalTimeToSystemTime
doesn't let us choose how to handle ambiguous cases during a DST transition. It always returns the earliest option.GetLocalTime
withTzSpecificLocalTimeToSystemTime
can give wrong results during a DST transition because the issue mentioned above.TzSpecificLocalTimeToSystemTime
always returns a result, also for non-existing datetimes during a DST transition.1601..30828
.The alternative in this PR is to calculate the offset ourselves using the information provided by
GetTimeZoneInformationForYear
.This is similar to what we do on Unix.The tricky bits to determine the correct offset between DST transitions is copied from the Unix implementation. Initially I wanted to share the code between the two systems, but that didn't make the Unix implementation any clearer.
Part of the old Windows implementation have been moved to a test, so our implementation can be compared to
TzSpecificLocalTimeToSystemTime
. I have set my computer timezone to ca. 20 'interesting' timezones (determined by looking at data of the entries enumerated byEnumDynamicTimeZoneInformation
) and found no differences.I ended up writing this PR because of a yak shaving. In order to remove some
unwrap
s from my code, I need some progress on #716. To show we need a fourth enum variant forLocalResult
, I started implementing a proof of concept forDateTime<Local>
. And it turned out the Windows implementation had no place to hook into...Fixes #1150, #651.