panic with_timezone Jan 1, 1601 00:00 UTC

[davehorner]

panic with_timezone Jan 1, 1601 00:00 UTC

[ChrisDenton]

Adjusted for UTC, the time given is 1/1/1601 0:00. Which is exactly the minimum time Windows supports.

[pitdicker]

Both of the options require the consumer of the library to do something special on windows?

No. Shifting by 400 years (a leap year cycle) is the solution proposed in #997.

And #1017 is the solution to use GetTimeZoneInformationForYear to get the timezone data from Windows and do the conversion in chrono instead of using the conversion function TzSpecificLocalTimeToSystemTime.

[djc]

Does this work better with #1017?

[davehorner]

Are you suggesting to attempt to patch my system with that PR and attempt again? If you have any notes on doing such a thing, I would be appreciative.

[djc]

Yup, I suggest you use a [patch] section (see https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section) to use a local clone of that branch to see if that helps.

[davehorner]

I forked chrono, pulled in win_timezone, git merge -X theirs win_timezone on main and pushed it up to https://github.com/davehorner/chrono.git.

I tried to make it as hard on myself as possible and modified .cargo\config.toml to include:

[patch.crates-io]
chrono = { git = 'https://github.com/davehorner/chrono.git', branch="win_timezone" }

I did a ripgrep and ran through all the Cargo.toml that had a chrono dependency, placing the patch section in each. I also added it to the Cargo.toml workspace root file.

warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   c:\w\rust\nushell\crates\nu-cli\Cargo.toml
workspace: c:\w\rust\nushell\Cargo.toml
warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   C:\w\rust\nushell\crates\nu-protocol\Cargo.toml
workspace: c:\w\rust\nushell\Cargo.toml
warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   C:\w\rust\nushell\crates\nu-system\Cargo.toml
workspace: c:\w\rust\nushell\Cargo.toml
warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   C:\w\rust\nushell\crates\nu-parser\Cargo.toml
workspace: c:\w\rust\nushell\Cargo.toml
warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   c:\w\rust\nushell\crates\nu-cmd-dataframe\Cargo.toml
workspace: c:\w\rust\nushell\Cargo.toml
warning: Patch `chrono v0.4.26 (https://github.com/davehorner/chrono.git?branch=win_timezone#631f114c)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

I did a cargo update since this was suggested in the output.

My output continues to show a panic with and without the modified patch sections. I may be doing this wrong, let me know.

ls \\.\pipe\                                                                                                                                                                                             1 11/22/23 12:23:19 PM
Failed to read metadata for '\\.\pipe\uv\00000000-7644'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000000-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000000-29020'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000000-38092'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000001-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000002-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000003-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000004-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\0000000000000005-21632'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\00000001-7644'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\00000002-7644'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\00000003-7644'. It may have an illegal filename
Failed to read metadata for '\\.\pipe\uv\00000004-7644'. It may have an illegal filename
thread 'main' panicked at 'No such local time', C:\Users\dhorner\.cargo\registry\src\index.crates.io-6f17d22bba15001f\chrono-0.4.31\src\offset\local\mod.rs:176:46
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\release\nu.exe` (exit code: 101)

I can trace through it with a debugger later. cheers.

[djc]

Okay. I thought there was a PR to improve on this, maybe it was #997 instead.

(Note that you should only need the patch section in the workspace Cargo.toml (not the .config/cargo.toml) and you can also use path = "../../chrono" instead of going via git dependency.)

If it doesn't work with that branch either, I'm happy to review a PR but won't have time to work on this much myself.

[davehorner]

Ok, gh pr checkout 997, and I just modified the workspace Cargo.toml.

 [patch.crates-io]
+chrono = { path = '..\chrono' }

I'm still seeing this warning,

warning: Patch `chrono v0.4.24 (C:\w\rust\chrono)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

I did the cargo update and I still see the warning with this in the lock file.

+
+[[patch.unused]]
+name = "chrono"
+version = "0.4.24"

I didn't feel comfortable that it was overridden well enough on the last attempt so I made the patch section for any Cargo.toml that had chrono as a dep. nushell has a number of crates internally so there is some complication there to ensure that its actually using the code.

The attempt I just did using #997 did not change the nushell panic. I'm not sure that the patch is being applied given the lock and warning provided.

It would be nice to have a small code sample that panics...maybe I can try and make something.

[davehorner]

I don't want to make another ticket, don't mean to add unrelated comments...but did want to note that the windows tests are not functional for me.

cargo test
   Compiling winapi v0.3.9
   Compiling chrono v0.5.0-alpha.1 (C:\w\rust\chrono)
error[E0432]: unresolved import `winapi::um::timezoneapi`
  --> src\offset\local\windows.rs:16:17
   |
16 | use winapi::um::timezoneapi::{GetTimeZoneInformationForYear, TIME_ZONE_INFORMATION};
   |                 ^^^^^^^^^^^ could not find `timezoneapi` in `um`

error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:6:3
  |
6 | ::windows_targets::link!("kernel32.dll" "system" fn TzSpecificLocalTimeToSystemTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lplocaltime : *const SYSTEMTIME, lpuniversaltime : *mut SYSTEMTIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:5:3
  |
5 | ::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToTzSpecificLocalTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lpuniversaltime : *const SYSTEMTIME, lplocaltime : *mut SYSTEMTIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:4:3
  |
4 | ::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToFileTime(lpsystemtime : *const SYSTEMTIME, lpfiletime : *mut FILETIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

Some errors have detailed explanations: E0432, E0433.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `chrono` (lib) due to 4 previous errors
warning: build failed, waiting for other jobs to finish...
error[E0432]: unresolved import `winapi::um::timezoneapi`
   --> src\offset\local\windows.rs:149:21
    |
149 |     use winapi::um::timezoneapi::{SystemTimeToFileTime, TzSpecificLocalTimeToSystemTime};
    |                     ^^^^^^^^^^^ could not find `timezoneapi` in `um`

error[E0432]: unresolved import `crate::Duration`
   --> src\offset\local\windows.rs:143:27
    |
143 |     use crate::{DateTime, Duration, FixedOffset, Local, NaiveDate, NaiveDateTime};
    |                           ^^^^^^^^ no `Duration` in the root
    |
    = help: consider importing one of these items instead:
            std::time::Duration
            core::time::Duration

error[E0432]: unresolved import `crate::Duration`
   --> src\offset\local\mod.rs:247:27
    |
247 |     use crate::{Datelike, Duration, Utc};
    |                           ^^^^^^^^ no `Duration` in the root
    |
    = help: consider importing one of these items instead:
            std::time::Duration
            core::time::Duration

warning: use of deprecated associated function `datetime::DateTime::<Tz>::from_utc`: Use TimeZone::from_utc_datetime() or DateTime::from_naive_utc_and_offset instead
   --> src\offset\local\windows.rs:165:23
    |
165 |             DateTime::from_utc(*dt - offset, offset)
    |                       ^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

error[E0599]: no method named `ymd` found for struct `Local` in the current scope
   --> src\offset\local\mod.rs:443:36
    |
110 | pub struct Local;
    | ---------------- method `ymd` not found for this struct
...
443 |         let local_20221106 = Local.ymd(2022, 11, 6);
    |                                    ^^^ method not found in `Local`

Some errors have detailed explanations: E0432, E0433, E0599.
warning: `chrono` (lib test) generated 1 warning
error: could not compile `chrono` (lib test) due to 8 previous errors; 1 warning emitted

doing cargo add winapi --features timezoneapi,winbase

gets me closer.

   Compiling chrono v0.5.0-alpha.1 (C:\w\rust\chrono)
error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:6:3
  |
6 | ::windows_targets::link!("kernel32.dll" "system" fn TzSpecificLocalTimeToSystemTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lplocaltime : *const SYSTEMTIME, lpuniversaltime : *mut SYSTEMTIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:5:3
  |
5 | ::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToTzSpecificLocalTime(lptimezoneinformation : *const TIME_ZONE_INFORMATION, lpuniversaltime : *const SYSTEMTIME, lplocaltime : *mut SYSTEMTIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

error[E0433]: failed to resolve: could not find `windows_targets` in the list of imported crates
 --> src\offset\local\win_bindings.rs:4:3
  |
4 | ::windows_targets::link!("kernel32.dll" "system" fn SystemTimeToFileTime(lpsystemtime : *const SYSTEMTIME, lpfiletime : *mut FILETIME) -> BOOL);
  |   ^^^^^^^^^^^^^^^ could not find `windows_targets` in the list of imported crates

error[E0432]: unresolved import `crate::Duration`
   --> src\offset\local\windows.rs:143:27
    |
143 |     use crate::{DateTime, Duration, FixedOffset, Local, NaiveDate, NaiveDateTime};
    |                           ^^^^^^^^ no `Duration` in the root
    |
    = help: consider importing one of these items instead:
            std::time::Duration
            core::time::Duration

error[E0432]: unresolved import `crate::Duration`
   --> src\offset\local\mod.rs:247:27
    |
247 |     use crate::{Datelike, Duration, Utc};
    |                           ^^^^^^^^ no `Duration` in the root
    |
    = help: consider importing one of these items instead:
            std::time::Duration
            core::time::Duration

For more information about this error, try `rustc --explain E0433`.
error: could not compile `chrono` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...
warning: use of deprecated associated function `datetime::DateTime::<Tz>::from_utc`: Use TimeZone::from_utc_datetime() or DateTime::from_naive_utc_and_offset instead
   --> src\offset\local\windows.rs:165:23
    |
165 |             DateTime::from_utc(*dt - offset, offset)
    |                       ^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

error[E0599]: no method named `ymd` found for struct `Local` in the current scope
   --> src\offset\local\mod.rs:443:36
    |
110 | pub struct Local;
    | ---------------- method `ymd` not found for this struct
...
443 |         let local_20221106 = Local.ymd(2022, 11, 6);
    |                                    ^^^ method not found in `Local`

Some errors have detailed explanations: E0432, E0433, E0599.
For more information about an error, try `rustc --explain E0432`.
warning: `chrono` (lib test) generated 1 warning
error: could not compile `chrono` (lib test) due to 6 previous errors; 1 warning emitted

let me know if you have any suggestions on how to get my tests unstuck and I will work on a test to reproduce. Or if you have some other ideas for testing those PRs. thanks.

[djc]

Compiling chrono v0.5.0-alpha.1 (C:\w\rust\chrono)

You should be using the 0.4.x branch. See if that works better? These tests are executed in CI so stuff should work.

[davehorner]

Thank you, using the proper branch does provide a working test suite on my machine.

I created a tests\panics.rs

use chrono::{Utc, Datelike, Local, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Timelike};
use std::{path, process, thread};
use chrono::LocalResult;

#[cfg(test)]
#[test]
/// Jan 1, 1601 00:00 UTC should not panic
fn assert_panic_epoch_1601() {
    let epoch_1601 = Utc.ymd(1601, 1, 1).and_hms(0, 0, 0);
    let ret = match Utc.timestamp_opt(epoch_1601.timestamp(), 0) {
        LocalResult::Single(t) => Some(t.with_timezone(&Local)),
        _ => None,
    };
    assert!(true,"Jan 1, 1601 00:00 UTC timezone: {:?}", ret);
}

The above test panics on my machine using:
git checkout 0.4.x
gh pr checkout 997
gh pr checkout 1017

because I added it to panics.rs it was quite easy to cargo test each branch/PR and be confident the patches are being used.

I can provide a PR of panics.rs if you'd like. I'm not sure what to do for a change to resolve it. I'm all ears.

[pitdicker]

I'm trying to do an ls .\pipe in nushell which causes a panic.
The ls .\pipe lists the pipes on windows and according to powershell that date is 12/31/1600 7:00PM.

Sorry to say, but cool. This value is seems to be right beyond the range that Windows can represent. Currently I am on Linux and can't test, but would this be Windows way to encode something like Option<SYSTEMTIME>? With a pipe not having a time?

While chrono should just not panic, maybe in this case the better solution is for nushell is to detect this as a sentinel value?

[davehorner]

I feel checking sentinel values should be in the library and not in each project that takes on the dependency.

[pitdicker]

I feel checking sentinel values should be in the library and not in each project that takes on the dependency.

In any case chrono is not the right place. For chrono there is no reason to treat a timestamp 1601-01-01T00:00:00+00:00 any different from any other timestamp. But for an application that lists files on Windows there may be. But treating it specially is just a suggestion.

[davehorner]

Maybe a new interface with the Option<> which would test for sentinels? Why can't we wrap the call which has the pre-conditions in a windows cfg if inside chrono?

I don't think it should panic. Panic wrapping or protecting calls which are known to panic is no ideal. Knowing what the sentinel values are is half the battle. Why shouldn't chrono be responsible for this pre-condition of the windows interface it uses?

It would be good to know if Linux has this issue, my guess is no.
I really don't think the solution here is for me to wrap nushell's calls with sentinel checks. It ignores the issue for others to find.

[pitdicker]

Please think about it some more.

Of course I would love to see #1017 make progress so at least chrono is not as prone to panics on Windows as it is now.

[davehorner]

#1017 fails 4 tests for me right now, if I can help there I will too.

failures:
    offset::local::inner::tests::verify_against_tz_specific_local_time_to_system_time
    offset::local::tests::verify_correct_offsets
    offset::local::tests::verify_correct_offsets_distant_future
    offset::local::tests::verify_correct_offsets_distant_past

if I can help, I will just not sure what to do with #1017

[pitdicker]

Thank you 👍. If you want #1017 is rebased and should be working again.

[davehorner]

Paul, thank you. All tests are passing for #1017.

test result: ok. 252 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 16.33s

This issue #1364, is still open. I've thought about this quite a bit, making the caller do the checks or wraps is not great. It is a group discussion, I welcome any additional feedback.

Thanks for submitting code to reduce the panic in library code!

[djc]

So as I understand it this issue happens because we're unable to determine the correct Local offset for dates this far in the past, due to Windows API limitations? Perhaps we could have some fallible Local::localize() API?

[davehorner]

@djc Yes, from what I understand we're unable to determine the correct Local offset for dates this far in the past due to the windows API.

A new api that does not panic would be nice.

[davehorner]

There is something to be said for protecting the existing interface. If windows api doesn't support dates outside a range, shouldn't chrono ensure this pre-condition is met?

Are you looking for me to provide a Local::localize() implementation or might you have time to implement? Where would it live in the codebase? Any notes or words of encouragement towards that improvement are welcome.

[djc]

Unfortunately we can't change the existing interface to become fallible because that would require making a semver-incompatible change, which is fairly fraught for something as widely used as chrono.

Unfortunately I probably won't have time to implement this, so it would be great if you can give it a shot. It would just live in the impl Local block, around here:

https://github.com/chronotope/chrono/blob/0.4.x/src/offset/local/mod.rs#L152

[pitdicker]

@djc I've started working on your review. Sorry again for how much I have been absent.

I don't think there is any need to make the API in chrono fallible. It would only be for Windows, and we already have two possible solutions: shift the date by 400 years (not my preference) or work directly with the timezone data from Windows instead of the timezone API.

[djc]

Sorry again for how much I have been absent.

No need to be sorry, I haven't exactly been responsive to most of your PRs either...

I don't think there is any need to make the API in chrono fallible. It would only be for Windows, and we already have two possible solutions: shift the date by 400 years (not my preference) or work directly with the timezone data from Windows instead of the timezone API.

The latter does sound like it might be more attractive, if it does cover these earlier dates.

[davehorner]

@pitdicker

work directly with the timezone data from Windows instead of the timezone API.

Both of the options require the consumer of the library to do something special on windows? I don't know exactly what it means to work directly with the timezone data vs timezone API.

[davehorner]

naive\mod.rs defines MIN/MAX for NaiveDateTime and it also has the UNIX_EPOCH in there. 1600 doesn't have a definition as #997 mentions. nevevss mentions there's a docstring somewhere in SYSTEMTIME that designates a valid value.

From testing it seems that dates greater than 1601 work. 1601 panics. So >1600 is not a good test since it has to be >1601.

We use 1600 a few places.

src\offset\local\tz_info\rule.rs
750:        result += (year - 1600) / 400;

src\naive\internals.rs
529:        assert_eq!(YearFlags::from_year(1600).ndays(), 366);

src\naive\date.rs
2894:        assert_eq!(d.with_year(1600), Some(NaiveDate::from_ymd_opt(1600, 2, 29).unwrap()));

src\format\scan.rs
399:        assert_eq!(timezone_offset_2822("cSt").unwrap(), ("", Some(-21600)));

src\datetime\tests.rs
1499:        NaiveDate::from_ymd_opt(1600, 12, 29).unwrap().and_hms_opt(23, 59, 59).unwrap();

We use 1601 one place in a comment.

src\offset\local\windows.rs
82:        // Valid values: 1601-30827

that comment is from:

fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> SYSTEMTIME {
    SYSTEMTIME {
        // Valid values: 1601-30827
        wYear: dt.year() as u16,
        // Valid values:1-12
        wMonth: dt.month() as u16,
        // Valid values: 0-6, starting Sunday.
        // NOTE: enum returns 1-7, starting Monday, so we are
        // off here, but this is not currently used in local.
        wDayOfWeek: dt.weekday() as u16,
        // Valid values: 1-31
        wDay: dt.day() as u16,
        // Valid values: 0-23
        wHour: dt.hour() as u16,
        // Valid values: 0-59
        wMinute: dt.minute() as u16,
        // Valid values: 0-59
        wSecond: dt.second() as u16,
        // Valid values: 0-999
        wMilliseconds: 0,
    }
}

In my testing greater than 1601 works, the comment seems to indicate 1601 should work but does not for me.

https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-systemtime
The SYSTEMTIME does not check to see if the date represented is a real and valid date. with this API, you should ensure its validity

Should the system_time_from_naive_date_time be checking for validity and falliable. In other words, the caller of the with_timezone should ensure validity of the date before calling with_timezone.

@pitdicker I spent some time tonight trying to understand how to implement either proposed change and I'm not sure how to proceed. I'm still a little lost on what to do. lmk if you have any additional thoughts, I will continue looking at it.

[ChrisDenton]

In my testing greater than 1601 works, the comment seems to indicate 1601 should work but does not for me.

Hm, as far as the Windows API is concerned it should only be a issue for times on 1601-01-01. From 1601-01-02 onward it should work. The only issue with the 1st is that the time offset could cause the year to rollback to 1600 and therefore SystemTimeToTzSpecificLocalTime can not calculate the timezone change information which means it errors with ERROR_INVALID_PARAMETER.

I think using GetTimeZoneInformationForYear (or otherwise manually calculating the offset) is the better solution for the specific problem in the OP because the UTC date will always be in range (>= 1601) when gotten from a FILETIME even if the calculated local time is not. If you did want to limit the impact of the changes then the manual step could be done only if using SystemTimeToTzSpecificLocalTime fails.

[davehorner]

running 4 tests
test assert_panic_epoch_1602_01_01 ... ok
test assert_panic_epoch_1601_01_02 ... ok
test assert_panic_epoch_1600_01_01 ... FAILED
test assert_panic_epoch_1601_01_01 ... FAILED

@ChrisDenton thanks. The panic tests work as you indicated. Which means we can't just check the year for >1600 and we have to address the 01/01 special. BTW what does OP in 'in the OP' mean?

I'm still not sure of a solution without some additional direction, I'm talking to ChatGPT.

Are we still talking about a new interface Local::localize()?

fn localize(dt: &NaiveDateTime) -> Option<DateTime<Local>> {
    // Check if year is less than 1601
    if dt.year() < 1601 {
        return None;
    }

    // Replace this with your actual logic for determining localization based on dt
    // Assuming you have a way to convert localized data to NaiveDateTime
    let naive_datetime: NaiveDateTime = /* Convert localized data to NaiveDateTime */;

    // Create a DateTime<Local> from NaiveDateTime
    let localized_datetime = Local.from_local_datetime(&naive_datetime).ok()?;

    // Use with_timezone to set the desired time zone
    let localized_datetime_with_timezone = localized_datetime.with_timezone(&Local);

    Some(localized_datetime_with_timezone)
}

this is GPT filling in junk. Actually,

nushell created the Option<> interface and points to 4.19 as the source. I'd ask why calculating the UNIX_EPOCH makes sense at all and a general function to test for SYSTEMTIME validity be called instead.

fn try_convert_to_local_date_time(t: SystemTime) -> Option<DateTime<Local>>
    // Adapted from https://github.com/chronotope/chrono/blob/v0.4.19/src/datetime.rs#L755-L767.
    let (sec, nsec, was_success) = match t.duration_since(UNIX_EPOCH) {
        Ok(dur) => {
            (dur.as_secs() as i64, dur.subsec_nanos(),true)
        },
        Err(e) => {
            // unlikely but should be handled   <--- unlikely straight forward implementation. why pull this from chrono...
            let dur = e.duration();
            let (sec, nsec) = (dur.as_secs() as i64, dur.subsec_nanos());
            if nsec == 0 {
                (-sec, 0,false)
            } else {
                (-sec - 1, 1_000_000_000 - nsec,false)
            }
        }
    };

the -sec, and 1_000_000_000 constants make me sad.

Are we just missing a validity check for those of us on a win32 machine?

// Function to check if each member of a SYSTEMTIME is valid
fn is_valid_system_time(system_time: &SYSTEMTIME) -> bool {
    // Check if each member falls within valid ranges
    let valid_year = system_time.wYear > 1600 && system_time.wYear <= 9999;
    let valid_month = system_time.wMonth >= 1 && system_time.wMonth <= 12;
    let valid_day = system_time.wDay >= 1 && system_time.wDay <= 31;
    let valid_hour = system_time.wHour < 24;
    let valid_minute = system_time.wMinute < 60;
    let valid_second = system_time.wSecond < 60;
    let valid_millisecond = system_time.wMilliseconds < 1000;

    // Return true if all members are valid
    valid_year
        && valid_month
        && valid_day
        && valid_hour
        && valid_minute
        && valid_second
        && valid_millisecond
}

chrono has a

pub(crate) fn utc_to_local_time(utc_time: &SYSTEMTIME) -> Result<SYSTEMTIME, Error> {
    let mut local = MaybeUninit::<SYSTEMTIME>::uninit();
    unsafe {
        windows_sys_call!(
            SystemTimeToTzSpecificLocalTime(ptr::null(), utc_time, local.as_mut_ptr()),
            0
        )
    };
    // SAFETY: SystemTimeToTzSpecificLocalTime must have succeeded at this point, so we can
    // assume the value is initialized.
    Ok(unsafe { local.assume_init() })
}

Don't know anything about this MaybeUninit which might provide a means to do this check idk. SAFETY comments aside, I think this call to SystemTimeTzSpecificLocalTime should be protected with a validity check.

[ChrisDenton]

BTW what does OP in 'in the OP' mean?

Sorry, it's shorthand for "Opening Post". I just meant to say that the specific problem that started this thread is the 1/1/1601 UTC problem.

When getting a time from a file, the current way chrono is use means we start with a UTC, FILETIME which is then converted to a UTC SYSTEMTIME. This all works fine. It's the SystemTimeToTzSpecificLocalTime that may error in the 1/1/1601 case. One way of working around this is replacing that function with a manual calculation (as in #1017) or perhaps doing so only if SystemTimeToTzSpecificLocalTime errors.

[davehorner]

I submitted a patch to nushell to filter out the unsafe values.
I see @ChrisDenton added some code https://github.com/chronotope/chrono/pull/1377/files

This ticket may well be closed -- maybe live as long as the above PR?, I'm unsure how to fix the api in general to protect against these panics.

[ChrisDenton]

#1017 is the preferred solution