`Timelike` does not correct account for nanoseconds exceeding 1e9

[samuelcolvin]

I'm been using chrono as a reference implementation while fuzzing my own code, and I've found an error in chrono.

.to_string() correctly rounds seconds up to account for nanoseconds exceeding 1e9, but .second() and .nanosecond() from TimeLike do not.

Example:

use chrono::{NaiveDate, Timelike};

#[test]
fn chrono_error() {
    let chrono_dt = NaiveDate::from_ymd(2000, 1, 1).and_hms_nano(1, 0, 0, 1_010_000_000);
    assert_eq!(chrono_dt.to_string(), "2000-01-01 01:00:01.010");
    assert_eq!((chrono_dt.second(), chrono_dt.nanosecond()), (1, 10_000_000));
}

gives:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `(0, 1010000000)`,
 right: `(1, 10000000)`', src/main.rs:6:5

rust playground showing the error.

[djc]

Thanks for the report. Would you be able to submit a PR?

[esheppa]

Some further thoughts here:

chrono uses nanoseconds between 1_000_000_000 and 1_999_999_999 to account for leap seconds, hence why this shows up at all. This behaviour is inline with the API contract which states

Chrono does not try to accurately implement leap seconds; it is impossible. Rather, it allows for leap seconds but behaves as if there are no other leap seconds. Various operations will ignore any possible leap second(s) except when any of the operands were actually leap seconds

(see Leap Second Handling and Representing Leap Seconds)

and

It should be noted that, for leap seconds not on the minute boundary, it may print a representation not distinguishable from non-leap seconds. This doesn't matter in practice, since such leap seconds never happened. (By the time of the first leap second on 1972-06-30, every time zone offset around the world has standardized to the 5-minute alignment.)

(see Display impl)

From what I can see, if we agree that it would be desirable that the formatted representation and Timelike methods return the same, then a solution could be changing the Timelike methods dealing with seconds and nanoseconds to return the calculated second / nanosecond (eg seconds is increased by one where internal nanos are greater than 999_999_999) - this changes the existing documented contracts which state that .second() is in 0..=59 and .nanosecond() values in 1_000_000_000..=1_999_999_999. This would also involve the with_second/nanosecond methods so that with_second accepts a seconds value of up to 60, and with_nanosecond accepts only 0..=999_999_999. This wouldn't change the function signatures but as it changes the API contract it would still be a significant breaking change. We could also consider some further validations, like only allowing a seconds value of 60 when the hour is also 23 and the minute is 59, this still allows invalid leap seconds however as they can only occur at the end of June and December.

@samuelcolvin - do you think this makes sense - do you have any experience with other date/time libraries where this is handled differently?

I'm currently experimenting with using the core::time::Duration where possible and these changes could be nice to be combined with that.

[pitdicker]

This currently works as documented.

/// Returns the second number from 0 to 59.
fn second(&self) -> u32;

/// Returns the number of nanoseconds since the whole non-leap second.
/// The range from 1,000,000,000 to 1,999,999,999 represents
/// the [leap second](./naive/struct.NaiveTime.html#leap-second-handling).
fn nanosecond(&self) -> u32;

I wonder if an extra method would make sense: second_incl_leap