Consider reverting stabilization of Duration::from_nanos #51107
Comments
retep998
added
the
T-libs-api
|
It's not clear if we may actually hit that limit. Filesystem like ext4, btrfs has nano second level timestamps but they are both capped at 64 bits. One possible use case though, may be scientific emulation where this can potentially be a problem; but I don't actually do such emulations so I have no further ideas. |
SimonSapin
added
the
beta-nominated
|
@retep998, what do you think of keeping it stable but changing the argument to CC @rust-lang/libs |
|
I would prefer to keep it as u64. We provide other constructors for dealing with large durations, such as |
Meanwhile on Windows NTFS has 64bit timestamps that are multiples of 100ns, which can overflow a |
|
@SimonSapin Changing the argument to u128 means the conversion can fail, since Duration stores seconds in an u64 (and this is effectively exposed in stable, infallible APIs like |
|
I agree with @dtolnay in that we've got other constructors for dealing with overflow or larger durations, so having this as a convenience constructor seems like it should be good to do still. |
|
I also prefer a u64 argument. |
|
I find it convincing that converting an NTFS timestamp to a |
|
Another point. If we ever add methods like |
|
The consensus from the libs team seems to be to keep using |
|
@pietroalbini I believe so, yes. |
pietroalbini
removed
the
beta-nominated
|
Well, this can be closed then. |
|
I guess it's too late, but IMO a compelling argument for that change would have been that That said, now that it's stable, I guess changing it even in a new edition would basically not be doable? |
…shtriplett 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)
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)
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/rust#103332 [5] rust-lang/rust#51107 (comment)
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/rust#103332 [5] rust-lang/rust#51107 (comment)
rust/src/libcore/time.rs
Lines 169 to 176 in 3fd82a5
As it turns out, using
u64forDuration::from_nanosisn't the greatest idea because2^64 / 1,000,000,000 / 60 / 60 / 24 / 365comes out to 585 years which is rather short. I don't think the libs team realized how short this was when stabilizing it, but fortunately it hasn't actually hit stable yet and is still in beta so we have the time to reconsider that decision.cc @SimonSapin who kicked off stabilization in #46507
The text was updated successfully, but these errors were encountered: