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

Fix implementation of Duration::checked_div #114238

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Jul 30, 2023

I ran across this while running some sanity checks on time. Quickcheck immediately found a bug, and as I'd modified the code from std I knew there was a bug here as well.

tl;dr this code fails (playground)

use std::time::Duration;

fn main() {
    assert_eq!(
        Duration::new(1, 1).checked_div(7),
        Some(Duration::new(0, 142_857_143)),
    );
}

The existing code determines that 1/7 = 0 (seconds), 1/7 = 0 (nanoseconds), 1 billion / 7 = 142,857,142 (extra nanoseconds). The billion comes from multiplying the remainder of the seconds (1) by the number of nanoseconds in a second. However, this wrongly ignores any remaining nanoseconds. This PR takes that into consideration, adds a test, and also changes the roundabout way of calculating the remainder into directly computing it.

Note: This is not a rounding error. This result divides evenly.

@rustbot label +A-time +C-bug +S-waiting-on-reviewer +T-libs

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2023

Failed to set assignee to T-libs: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 30, 2023
@thomcc
Copy link
Member

thomcc commented Aug 28, 2023

Nice catch. Sorry for delay.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 28, 2023

📌 Commit f1d4e48 has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@jhpratt
Copy link
Member Author

jhpratt commented Aug 28, 2023

No worries! It's not as if it's urgent given that the bug has been around literally since the method's introduction.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#109660 (Document that SystemTime does not count leap seconds)
 - rust-lang#114238 (Fix implementation of `Duration::checked_div`)
 - rust-lang#114512 (std/tests: disable ancillary tests on freebsd since the feature itsel…)
 - rust-lang#114919 (style-guide: Add guidance for defining formatting for specific macros)
 - rust-lang#115278 (tell people what to do when removing an error code)
 - rust-lang#115280 (avoid triple-backtrace due to panic-during-cleanup)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d2644d9 into rust-lang:master Aug 28, 2023
@rustbot rustbot added this to the 1.74.0 milestone Aug 28, 2023
@jhpratt jhpratt deleted the fix-duration-div branch August 28, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants