Bump time to 0.2.23

[timvisee]

Fixes #553

This updates the time dependency to 0.2.23. The crate has been refactored to follow time API changes.

This is mostly done, but still requires some work. I'm seeing a failing test (this case fails) for which I don't clearly see the problem nor a solution.

@ quodlibetor Do you have some insight on this failing test?

Tasks:

• Bump time to 0.2.23
• Refactor codebase for time API changes
• Fix test naive::datetime::tests::test_datetime_add (failing case) (thanks @blackghost1987!)
• Have you added yourself and the change to the changelog? (Don't worry about adding the PR number)

[blackghost1987]

I've tried to debug this, and solved the original panic, but the test case still fails with some overflow errors. Here are my results so far, maybe it will help somebody. I will investigate it further though and get back here if I've got anything.

So the failing test case is test_datetime_add and it fails in the last line, when testing the handling of the minimal duration here.
It will eventually call NaiveTime::overflowing_add_signed, which causes the panic in this line.

The problematic call is a simple subtraction of two time::Duration (OldDuration) instances. Duration subtraction is done by negating the input and using addition, but i64::min_value() cannot be negated, at least not inside i64. Fortunately that subtraction can be avoided, because the whole point of it is to get the nanoseconds part of the Duration. The nanoseconds part is stored in a field in v0.2 and can be accessed by the subsec_nanoseconds function, like so:

        let rhssecs = rhs.whole_seconds();
        let rhsfrac = rhs.subsec_nanoseconds();
        debug_assert_eq!(
            OldDuration::seconds(rhssecs) + OldDuration::nanoseconds(rhsfrac as i64),
            rhs
        );

Unfortunately now the code panics at the overflow calculations here

[blackghost1987]

I think I figured this out. So the last part of NaiveTime::overflowing_add_signed is trying to make sure the result is a valid NaiveTime, so 0 < secs < 86400. It manages this by adding or removing whole days while maintaining the morerhssecs which is the remainder value in seconds. The problem can be reproduced with the following code:

NaiveTime::from_hms(0, 0, 0).overflowing_add_signed(Duration::min_value())

The result is correctly calculated to be 08:29:51.000000001 but the remainder is so huge, it cannot be stored in an i64. We could detect the overflow on the subtraction, but the overflowing_add_signed function has no ways of reporting an overflow, it supposed to always work.

This could only be solved by changing the return type of the function, which would be a breaking change!

Maybe a better alternative would be to consider this as a "known limitation" of NaiveTime, and solve this in NaiveDateTime. The NaiveDateTime::checked_add_signed already returns None if there's an overflow, so maybe we could add some checks to it to prevent this. The date handling part already checks the remainder value received from NaiveTime::overflowing_add_signed against a MAX_SECS_BITS to see if the Duration::seconds function would overflow, and if so it returns early. Maybe we could add a check like that before the overflowing_add_signed call, and return early if it would fail.

[timvisee]

(...)

This could only be solved by changing the return type of the function, which would be a breaking change!

Maybe a better alternative would be to consider this as a "known limitation" of NaiveTime, and solve this in NaiveDateTime. The NaiveDateTime::checked_add_signed already returns None if there's an overflow, so maybe we could add some checks to it to prevent this. The date handling part already checks the remainder value received from NaiveTime::overflowing_add_signed against a MAX_SECS_BITS to see if the Duration::seconds function would overflow, and if so it returns early. Maybe we could add a check like that before the overflowing_add_signed call, and return early if it would fail.

Do you suggest to resolve this (here) before merging this PR?

[blackghost1987]

Do you suggest to resolve this (here) before merging this PR?

I already did it, and you already merged it 🙂 I just copied the existing MAX_SECS_BITS check to check it before and after the addition as well, and it did the trick. I'm not sure if it's required after the addition, but it doesn't hurt.

[timvisee]

Superseded by #578, which implements these changes but for the newer time v0.3. You'd likely want to merge it instead.

[djc]

I don't think we'll be updating the version of time used in chrono. Since #478, chrono has a minimal dependency on time, and in the next semver-incompatible version we'll remove the dependency entirely.

(Note that chrono is unaffected by https://rustsec.org/advisories/RUSTSEC-2020-0071.)