Skip to content
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

Utc nanoseconds do not always roundtrip #1289

Closed
crepererum opened this issue Sep 14, 2023 · 2 comments
Closed

Utc nanoseconds do not always roundtrip #1289

crepererum opened this issue Sep 14, 2023 · 2 comments

Comments

@crepererum
Copy link
Contributor

crepererum commented Sep 14, 2023

Reproducer

Code

#[test]
fn nano_roundrip() {
    let nanos = i64::MIN + 2;
    let dt = Utc.timestamp_nanos(nanos);
    let nanos2 = dt.timestamp_nanos();
    assert_eq!(nanos, nanos2);
}

Expected Result

Test passes.

Actual Result

Test panics when calling dt.timestamp_nanos() (so we don't even get to the actual comparison:

value can not be represented in a timestamp with nanosecond precision.

Technical Background

dt.timestamp() is -9223372037, however nanos / 1_000_000_000 is -9223372036, hence I conclude that this IF branch was taken:

chrono/src/offset/mod.rs

Lines 427 to 430 in b64cedc

if nanos < 0 {
secs -= 1;
nanos += 1_000_000_000;
}

Hence this checked_mul overflows:

.checked_mul(1_000_000_000)

My guess is that the conversion routine (first code block) isn't entirely accurate. The conversion is alright, I rather think when converting back we have to be more careful.

@pitdicker
Copy link
Collaborator

@crepererum Excellent issue report.

As far as I can tell in this code the first checked_mul pushes the value beyond i64::MIN, and the following addition should bring it back in range. Which of course doesn't work...

    pub fn timestamp_nanos(&self) -> i64 {
        self.timestamp()
            .checked_mul(1_000_000_000)
            .and_then(|ns| ns.checked_add(i64::from(self.timestamp_subsec_nanos())))
            .expect("value can not be represented in a timestamp with nanosecond precision.")
    }

For negative timestamps we should instead add 1 to the timestamp, do the multiply and addition, and subtract 1_000_000_000.

@crepererum Interested in making a PR?

crepererum added a commit to crepererum/chrono that referenced this issue Sep 15, 2023
crepererum added a commit to crepererum/chrono that referenced this issue Sep 15, 2023
crepererum added a commit to crepererum/chrono that referenced this issue Sep 15, 2023
pitdicker pushed a commit to pitdicker/chrono that referenced this issue Sep 16, 2023
@crepererum
Copy link
Contributor Author

Fixed by #1294 and released in 0.4.31.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants