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

Suggest less bug-prone construction of Duration in docs #119242

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

BenWiederhake
Copy link
Contributor

std::time::Duration has a well-known quirk: Duration::as_nanos() returns u128 [1], but Duration::from_nanos() takes u64 [2]. So these methods cannot easily roundtrip [3]. It is not possible to simply accept u128 in from_nanos [4], because it requires breaking other API [5].

It seems to me that callers have basically only two options:

  1. Duration::from_nanos(d.as_nanos() as u64), which is the "obvious" and buggy approach.
  2. Duration::new(d.as_secs(), d.subsecs_nanos()), which only becomes apparent after reading and digesting the entire Duration struct documentation.

I suggest that the documentation of from_nanos is changed to make option 2 more easily discoverable.

There are two major usecases for this:

  • "Weird math" operations that should not be supported directly by Duration, like squaring.
  • "Disconnected roundtrips", where the u128 value is passed through various other stack frames, and perhaps reconstructed into a Duration on a different machine.

In both cases, it seems like a good idea to not tempt people into thinking "Eh, u64 is good enough, what could possibly go wrong!". That's why I want to add a note that points out the similarly-easy and safe way to reconstruct a Duration.

[1] https://doc.rust-lang.org/stable/std/time/struct.Duration.html#method.as_nanos
[2] https://doc.rust-lang.org/stable/std/time/struct.Duration.html#method.from_nanos
[3] https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fa6bab2b6b72f20c14b5243610ea1dde
[4] #103332
[5] #51107 (comment)

@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2023

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 23, 2023
@the8472
Copy link
Member

the8472 commented Dec 23, 2023

It seems to me that callers have basically only two options:

You can also split the u128 with a division and modulo and feed that to new. Not very efficient, but simple to write.

@BenWiederhake
Copy link
Contributor Author

That's exactly what as_secs() and subsecs_nanos() do, just with the possibility of getting it wrong, and also with suspicious casts. That's why I suggest as_secs() and subsecs_nanos(). :)

@joshtriplett
Copy link
Member

This is certainly the right approach for if you need to round-trip Duration through numeric types. It'd help to clarify with something like "and you cannot copy the Duration itself".

@BenWiederhake
Copy link
Contributor Author

I included your suggested rewording and rebased to current master.

@ChrisDenton
Copy link
Member

I wonder if this would be better phrased as an example? So you can show that d.clone() is the same as Duration::new(d.as_secs(), d.subsec_nanos()).

@BenWiederhake
Copy link
Contributor Author

My main point is to make sure that readers informed that and why the obvious approach is wrong. Playing it down as an example does not do it justice. However, I could certainly add an example. What does @joshtriplett think?

@BenWiederhake
Copy link
Contributor Author

Changes since last push:

  • Fix a typo in the "suggestion" code – Thanks @ChrisDenton!
  • Rebased on current master

@joshtriplett
Copy link
Member

@bors r+

This seems reasonable to me.

Separate from this, it might make sense to add a from_nanos_u128.

@bors
Copy link
Contributor

bors commented Feb 11, 2024

📌 Commit 27ba1c1 has been approved by joshtriplett

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 Feb 11, 2024
@joshtriplett
Copy link
Member

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117740 (Add some links and minor explanatory comments to `std::fmt`)
 - rust-lang#118307 (Remove an unneeded helper from the tuple library code)
 - rust-lang#119242 (Suggest less bug-prone construction of Duration in docs)
 - rust-lang#119449 (Fix `clippy::correctness` in the library)
 - rust-lang#120307 (core: add Duration constructors)
 - rust-lang#120459 (Document various I/O descriptor/handle conversions)
 - rust-lang#120729 (Update mdbook to 0.4.37)
 - rust-lang#120763 (Suggest pattern tests when modifying exhaustiveness)
 - rust-lang#120906 (Remove myself from some review rotations)
 - rust-lang#120916 (Subtree update of ` rust-analyzer`)

r? `@ghost`
`@rustbot` modify labels: rollup
@BenWiederhake
Copy link
Contributor Author

from_nanos_u128 would need to be fallible, because Duration only has a bit less than 96 bits of internal storage. Due to that hassle, I question whether try_from_nanos_u128 is really needed.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117740 (Add some links and minor explanatory comments to `std::fmt`)
 - rust-lang#118307 (Remove an unneeded helper from the tuple library code)
 - rust-lang#119242 (Suggest less bug-prone construction of Duration in docs)
 - rust-lang#119449 (Fix `clippy::correctness` in the library)
 - rust-lang#120307 (core: add Duration constructors)
 - rust-lang#120459 (Document various I/O descriptor/handle conversions)
 - rust-lang#120729 (Update mdbook to 0.4.37)
 - rust-lang#120763 (Suggest pattern tests when modifying exhaustiveness)
 - rust-lang#120906 (Remove myself from some review rotations)
 - rust-lang#120916 (Subtree update of ` rust-analyzer`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a576e81 into rust-lang:master Feb 11, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
Rollup merge of rust-lang#119242 - BenWiederhake:dev-from-nanos, r=joshtriplett

Suggest less bug-prone construction of Duration in docs

std::time::Duration has a well-known quirk: Duration::as_nanos() returns u128 [1], but Duration::from_nanos() takes u64 [2]. So these methods cannot easily roundtrip [3]. It is not possible to simply accept u128 in from_nanos [4], because it requires breaking other API [5].

It seems to me that callers have basically only two options:
1. `Duration::from_nanos(d.as_nanos() as u64)`, which is the "obvious" and buggy approach.
2. `Duration::new(d.as_secs(), d.subsecs_nanos())`, which only becomes apparent after reading and digesting the entire Duration struct documentation.

I suggest that the documentation of `from_nanos` is changed to make option 2 more easily discoverable.

There are two major usecases for this:
- "Weird math" operations that should not be supported directly by `Duration`, like squaring.
- "Disconnected roundtrips", where the u128 value is passed through various other stack frames, and perhaps reconstructed into a Duration on a different machine.

In both cases, it seems like a good idea to not tempt people into thinking "Eh, u64 is good enough, what could possibly go wrong!". That's why I want to add a note that points out the similarly-easy and *safe* way to reconstruct a Duration.

[1] https://doc.rust-lang.org/stable/std/time/struct.Duration.html#method.as_nanos
[2] https://doc.rust-lang.org/stable/std/time/struct.Duration.html#method.from_nanos
[3] https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fa6bab2b6b72f20c14b5243610ea1dde
[4] rust-lang#103332
[5] rust-lang#51107 (comment)
@BenWiederhake BenWiederhake deleted the dev-from-nanos branch February 11, 2024 19:33
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.

6 participants