OffsetDateTime subtracted by OffsetDateTime can panic

[DoumanAsh]

I hit panic due to this assert
https://github.com/time-rs/time/blob/main/src/duration.rs#L205=

What I was subtracting

2022-06-22 0:00:00.0 +00:00:00 - 2022-06-21 1:57:08.3107779 +00:00:00

Callstack

thread 'should_handle_generate_crontab_events' panicked at 'assertion failed: nanoseconds <= 0', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.10/src/duration.rs:205:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:143:14
   2: core::panicking::panic
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:48:5
   3: time::duration::Duration::new_unchecked
             at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.10/src/duration.rs:205:13
   4: <time::time::Time as core::ops::arith::Sub>::sub
             at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.10/src/time.rs:751:9
   5: <time::primitive_date_time::PrimitiveDateTime as core::ops::arith::Sub>::sub
             at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.10/src/primitive_date_time.rs:933:34
   6: <time::offset_date_time::OffsetDateTime as core::ops::arith::Sub>::sub
             at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/time-0.3.10/src/offset_date_time.rs:1218:9
   7: task::cron::timer_duration
             at ./src/cron.rs:16:24
   8: task::cron::CronFuture::next::{{closure}}
             at ./src/cron.rs:33:40
   9: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/future/mod.rs:91:19
  10: task::cron::cron::{{closure}}
             at ./src/cron.rs:63:47

[DoumanAsh]

I'm not sure if it is correct to assert in this case as you only create part of Duration
I think new_unchecked should not have these asserts, since it is you know _unchecked

[DoumanAsh]

In 0.2 this code was fine because it didn't go through these useless asserts

[jhpratt]

The assertions are very much intentional, and were added recently. They are only present when in debug mode, so there's zero overhead in an optimized build.

In 0.2 this code was fine because it didn't go through these useless asserts

Just because an assertion isn't present doesn't mean the code was correct.

I will look into the panic, but the assertions are there for a reason: to catch bugs.

[DoumanAsh]

Well, unnecessary complicated implementation is reason to catch bugs.
In reality Time - Time should just sub seconds and nanoseconds and then do Duration::new(...) which would adjust resulting seconds and nanoseconds

[jhpratt]

Please be respectful.

You have not read the underlying code. I know this because your suggested implementation is irreconcilable with the internal structure. Handling time is extremely difficult and I have written nearly all of the code in this crate. All that I have written was from scratch, a large portion of it even without known algorithms. Bugs exist; that's the nature of programming. I have already provided justification for the debug assertions — something you have apparently never used in Rust. You're free to disagree on whether they should be there, but the evidence is clear: they should be. Were they not, this bug wouldn't have been caught. Describing code as "useless" and "unnecessarily complicated" is not only unproductive, it is just plain rude.

[jhpratt]

I have verified that using Duration::new returns the expected result, and have added a regression test to ensure that this does not change in the future. This patch was just released in 0.3.11.