Rust: Tracking issue for duration_constants

Created on 7 Jan 2019  路  29Comments  路  Source: rust-lang/rust

Implemented in https://github.com/rust-lang/rust/pull/57375/

This will make working with durations more ergonomic. Compare:

// Convenient, but deprecated function.
thread::sleep_ms(2000);

// The current canonical way to sleep for two seconds.
thread::sleep(Duration::from_secs(2));

// Sleeping using one of the new constants.
thread::sleep(2 * SECOND);

```rust
impl Duration {
pub const SECOND: Duration = Duration::from_secs(1);
pub const MILLISECOND: Duration = Duration::from_millis(1);
pub const MICROSECOND: Duration = Duration::from_micros(1);
pub const NANOSECOND: Duration = Duration::from_nanos(1);
}

B-unstable C-tracking-issue Libs-Tracked T-libs

Most helpful comment

I wouldn't expect there to need to be a prelude because of this new trait (I'm in the preludes-are-bad camp). It can just be a trait import. I also think that since the trait would have the methods called on integers (and such), the fuller name is required to be clearer. Otherwise, what is 10.millis()? It could mean milliseconds, but in other contexts, it could mean millimeters, or milliliters, etc. I wouldn't worry about adding as_ prefixes just to follow some other conventions if it makes the API worse.

// name TBD
trait TimeUnits {
    fn seconds(&self) -> Duration;
    fn milliseconds(&self) -> Duration;
    // etc
}

Then it's usage could look like this:

use std::time::{Instant, TimeUnits as _};

sleep(100.milliseconds());

let deadline = Instant::now() + 5.seconds();

All 29 comments

Currently these constants are free constants in the std::time module. I think we should consider moving them onto the Duration type as associated constants.

Associated constants would reduce the (typical) number of imports in code using this. @stjepang @frewsxcv what do you think?

I鈥檒l FCP to merge once this is resolved one way or another.

@SimonSapin We should definitely do that!

Another related constant is UNIX_EPOCH, which was introduced as std::time::UNIX_EPOCH in 1.8.0:
https://doc.rust-lang.org/nightly/std/time/constant.UNIX_EPOCH.html

Then, in 1.28.0, it was added again as associated constant SystemTime::UNIX_EPOCH:
https://doc.rust-lang.org/nightly/std/time/struct.SystemTime.html#associatedconstant.UNIX_EPOCH

Following that precedent, I'd infer associated constants would probably be more idiomatic here.

@SimonSapin I think we're ready for FCP to merge now.

Updated the issue description.

@rfcbot fcp merge

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged team members:

  • [x] @Amanieu
  • [x] @Kimundi
  • [x] @SimonSapin
  • [x] @alexcrichton
  • [ ] @dtolnay
  • [ ] @sfackler
  • [ ] @withoutboats

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot concern missed the point

As I understand it, the point of duration constants when they were suggested in https://github.com/rust-lang/rust/pull/51610#issuecomment-398459098 was to enable pithy code like:

thread::sleep(2 * SECOND);

As currently implemented, this would not be supported because associated constants cannot currently be imported.

error[E0432]: unresolved import `std::time::Duration`
 --> src/main.rs:3:16
  |
3 | use std::time::Duration::SECOND;
  |                ^^^^^^^^ not a module `Duration`

So what we have is:

thread::sleep(2 * Duration::SECOND);

which may not be sufficiently better than:

thread::sleep(Duration::from_secs(2));

Would it make sense to revisit whether module level constants would be better? Or is there work in progress on making associated constants importable?

@rfcbot concern consider unit structs

This looks weird:

thread::sleep(SECOND);

In practice hopefully people would realize to write:

thread::sleep(1 * SECOND);

I haven't thought through this fully, but maybe it would be better to define units as unit structs rather than constants of type Duration?

thread::sleep(Second); // does not compile

thread::sleep(1 * Second);

Worth it, or too confusing?

@rfcbot concern clippy

It would be nice if Clippy didn't lint 1 * Duration::CONSTANT at stabilization.

warning: the operation is ineffective. Consider reducing it to `Duration::SECOND`
 --> src/main.rs:7:19
  |
7 |     thread::sleep(1 * Duration::SECOND);
  |                   ^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(clippy::identity_op)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op

Another thought, but not a blocker: Duration::from_... are const fns.

const TIMEOUT: Duration = Duration::from_secs(5);

I hope that eventually N * Duration can be const. Does anyone know if there is any hope of this being supported, given that there is a trait method call involved?

const TIMEOUT: Duration = 5 * Duration::SECOND;
error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
 --> src/main.rs:6:27
  |
6 | const TIMEOUT: Duration = 5 * Duration::SECOND;
  |                           ^^^^^^^^^^^^^^^^^^^^

As currently implemented, this would not be supported because associated constants cannot currently be imported.

Oh, I didn't realize that! :(

Would it make sense to revisit whether module level constants would be better?

Another possibility is to have both, just like there is SystemTime::UNIX_EPOCH and std::time::UNIX_EPOCH. But maybe not since std::time::UNIX_EPOCH is sort of obsoleted in favor of the associated constant anyway (if we had them in Rust 1.0, std::time::UNIX_EPOCH probably wouldn't exist).

Also yes, I don't see a reason why associated constants shouldn't be importable.

I haven't thought through this fully, but maybe it would be better to define units as unit structs rather than constants of type Duration?

Interesting idea... this way we would distinguish durations from units. I'm not against, but also find it a little magical.

@rfcbot resolve consider unit structs

Too confusing.


@rfcbot resolve clippy

I filed https://github.com/rust-lang/rust-clippy/issues/3866 to follow up in Clippy. This does not need to block stabilization.


@rfcbot resolve missed the point

I don't want to block on this if others feel that these constants are a better idiom than Duration::from_N. I would ask that we please not stabilize under the hope that they become directly importable. If we stabilize, it should be because we believe the constants are good to have even if importing them never ends up happening. What do you think @stjepang?

The most recent I could find on importing associated items is this i.r-l.o comment by @petrochenkov and it does not sound imminent.

I would slightly favor moving these out to std::time. I would prefer not to stabilize them in both places up front.


@rfcbot concern documentation

One thing I would still like to block on is better documentation before I would be comfortable with these appearing in release notes. The current example code for these constants looks like:

use std::time::Duration;

assert_eq!(Duration::SECOND, Duration::from_secs(1));

Example code is supposed to communicate why someone would want to use a thing, not only how to refer to it. These examples only show how to import and refer to the constant but not why anyone would want to do that.

I would ask that we please not stabilize under the hope that they become directly importable. If we stabilize, it should be because we believe the constants are good to have even if importing them never ends up happening.

Not being able to import associated constants is a real bummer. Still, I prefer n * Duration::SECOND over Duration::from_secs(n) as constants make arithmetic on durations feel more natural.

In the ideal world, we'd probably only have importable associated constants. But it's interesting how there is some precedent for redundancy in other similar cases:

I think I'd be fine with both std::time::X and Duration::X, but would also suggest we agree on a consistent philosophy on how to organize constants for such data types:

  • Should they be associated constants? If so, why don't we have u32::MAX?
  • Should they be free-standing constants? If so, why are we considering deprecating std::time::UNIX_EPOCH?
  • Should we have both?

And what about const methods like u32::max_value() - should they be replaced with associated constants? What would we do if we could design the standard library from scratch?

One thing I would still like to block on is better documentation before I would be comfortable with these appearing in release notes

Would you be okay with copying examples from constructor methods? For example, we could steal the example from from_millis and adapt it to MILLISECOND:

use std::time::Duration;

let duration = 2569 * Duration::MILLISECOND;

assert_eq!(2, duration.as_secs());
assert_eq!(569_000_000, duration.subsec_nanos());

I think we should use associated constants and deprecate redundancies. Assuming that associated constant imports will work of course.

I always found absence of u32::MAX quite confusing. And use std::u32::MAX; looks like an import from primitive type, not from u32 module.

I think if we'll get a working import of associated constants we can remove integer modules altogether in a backwards-compatible way. Float modules unfortunately contain const sub-module, so without some kind of "associated module" it will not work for them. :)

If I may bring up a bikeshed, considering that the from_* methods use the abbreviations secs, millis, micros and nanos and that brevity can help with ergonomics, we could consider naming these Duration::SEC, Duration::MILLI, Duration::MICRO and Duration::NANO for consistency and conciseness.

I was trying this out today, and was confused for a bit that std::time::UNIX_EPOCH was available, but the other constants are under a different namespace. The constant shorthands all living under the time namespace would be nice so we are able to do:

use std::time;

futures_timer::wait_for(30 * time::SECONDS).await;
for instant in futures_timer::repeat(5 * time::MINUTES).await {
    println!("the time is {}", instant);
}

Having these live on Duration feels more clunky when writing these inline:

use std::time;

futures_timer::wait_for(30 * time::Duration::SECONDS).await;
for instant in futures_timer::repeat(5 * time::Duration::MINUTES).await {
    println!("the time is {}", instant);
}

I guess importing Duration is an option, but then it still looks unlike any other construct in the current stdlib. And when also using UNIX_EPOCH the difference in import locations can be felt:

use std::time::{UNIX_EPOCH, Duration};

futures_timer::wait_for(30 * Duration::SECONDS).await;
for now in futures_timer::repeat(5 * Duration::MINUTES).await {
    println!("{} hours since the unix epoch", now - UNIX_EPOCH);
}

As others have said before, a function would be about as long to write as the current exports, and feel far more familiar. But I do think there's a lot of value in having these constants exist, if they are placed in a slightly more accessible location.

I hope sharing my experience here is useful. Thanks!

_edit: I've written a blog post about this https://blog.yoshuawuyts.com/std-time/_

Here's a short question that perhaps turns this discussion around a bit. If we move the Duration constants to the std::time module, then under what circumstances would we want to use associated constants in other cases?

(A possible answer to this question is that "std::time is a special case," but I still think that noodling on what the general guidelines might be could be useful framing for this discussion.)

This was suggested in one of the failed PRs adding the constants, and it had some support, but it wasn't considered again in the successful PR: an alternative to these constants and multiplication is adding a trait that can be implemented by integer types.

It feels to me that other languages have unit multiplication for durations because they frequently cannot extend types with new functionality. With Rust traits, we can easily support things like 30.seconds().

I realize not all agree, but it'd be good to explicitly acknowledge if we don't want to use a trait.

@seanmonstar ohey, that's the first time I'm hearing about that. That's quite interesting! I have some thoughts on this:

Imports

If this were an extension trait, it would not necessarily be included in the Rust prelude. Which I believe is because the prelude tries to be quite conservative in expanding. This means that any use of a trait would likely require a manual import. Where the established convention is to create a prelude if there are extension traits. This means usage would be:

use std::time::{prelude::*, Instant};
use std::thread;

let now = Instant::now();
thread::sleep(2.secs());
dbg!(now.elapsed());

Having to do a manual import to get this trait in scope feels quite painful here. A pain that constants directly under std::time would not have.

Naming

I think it's also worth talking about naming. This is a unit conversion, but the trait is not using the conventional as_<type>/into_<type> scheme. Which I guess is kind of the point of the trait. But it's worth acknowledging that it does something that it might feel a bit off the beaten path to some.

Also as a side note, in general I think we should follow the conventions established by Duration. E.g. 30.secs, not 30.seconds. In particular when dealing with milliseconds that might save people from typos.

Module Hierarchy

It could be argued that temporal extensions to integers do not belong. But I think the UX is nice enough that as long as they're part of a trait that it doesn't feel too bad. Rust has dead code elimination, and none of this concerns anything that would cause trouble for no-std.

Conclusion

I agree 2 * time::SECS can be a bit awkward (the multiplication imo _does_ look odd). But I'm not sure if introducing a new trait for these conversions is even more awkward. I feel there's value in both ideas, though as long as the trait requires manual importing, I think the constants are likely to be better.

That said; perhaps we don't have to choose. I don't see much harm in exploring both options. I can see value in both options, and don't think it's bad if they would they co-exist.

I don't think having to import something should be considered a pain point. I think we too often focus discussions like these too much on tiny example snippets. When in reality we should likely be designing the language so huge projects are as readable and understandable as possible. In a five line example an import can feel painful. But in a >100 LOC module an extra import is nothing.

I think Duration::SECOND is to be preferred over time::SECOND. As @BurntSushi touches on: Why do we have and when do we want to use associated constants if not here?

The plural time::SECS makes no sense to me. Even if a constant is introduced mostly for one specific use case, it has to be able to stand on its own, and it's a single second, not multiple seconds.

Additionally, do we want to encourage inline time arithmetic like that? Unless you disagree with me that magic constants are bad, one should likely never sleep(2 * Duration::SECOND), but rather sleep(POLL_INTERVAL) or whatever name is suitable for the sleep duration in question.

EDIT: I don't feel it's fair to compare with time::UNIX_EPOCH. It's a legacy constant. Compare with the newer SystemTime::UNIX_EPOCH and suddenly Duration::SECOND will feel more symmetrical.

I wouldn't expect there to need to be a prelude because of this new trait (I'm in the preludes-are-bad camp). It can just be a trait import. I also think that since the trait would have the methods called on integers (and such), the fuller name is required to be clearer. Otherwise, what is 10.millis()? It could mean milliseconds, but in other contexts, it could mean millimeters, or milliliters, etc. I wouldn't worry about adding as_ prefixes just to follow some other conventions if it makes the API worse.

// name TBD
trait TimeUnits {
    fn seconds(&self) -> Duration;
    fn milliseconds(&self) -> Duration;
    // etc
}

Then it's usage could look like this:

use std::time::{Instant, TimeUnits as _};

sleep(100.milliseconds());

let deadline = Instant::now() + 5.seconds();

@BurntSushi @faern @stepancheg See https://github.com/rust-lang/rfcs/pull/2700 for similar discussions about associated constants.

I agree with @leoyvens.
NANO (or ONE_NANO) is more suitable for Duration constant. NANOSECOND is more suitable for enumeration TimeUnit (or ChronoUnit).

Cancelling because this stalled out, but a significant amount of design discussion happened after the first FCP proposal. It would be good if someone could put together a recap of the alternatives and tradeoffs and based on that propose how it's best to move forward

@rfcbot fcp cancel

@dtolnay proposal cancelled.

Would you be okay with copying examples from constructor methods? For example, we could steal the example from from_millis and adapt it to MILLISECOND:

use std::time::Duration;

let duration = 2569 * Duration::MILLISECOND;

assert_eq!(2, duration.as_secs());
assert_eq!(569_000_000, duration.subsec_nanos());

Yes, this would be much more appropriate than the current documentation.

It would be awesome if one could "bundle" together definitions so that using one brings the other into scope for the expression where it's used. For example, if one used std::thread::sleep and that automatically brought Duration into scope for the arguments only, then one could just write std::thread::sleep(Duration::from_secs(10)), which would alleviate my pain point...

The Duration::MIN and Duration::MAX constants are now part of this feature flag. So I comment here rather then where they were introduced (#76114). Will the smallest and largest amount of time that a Duration can hold always be exactly the same? The underlying implementation of Duration could change to a 128 bit integer for the secs field, keep its API identical but simply allow adding together durations to a greater extent than now. Or has Duration already promised to not do that anywhere?

Currently the docs specify that it will be equal to zero and "roughly 584,942,417,355 years". I would find it very surprising if the value of such a constant would change between Rust versions. The integer equivalents, u64::MAX etc are not the same, because the extreme values are part of the definition of the type. But that's not true for a duration of time in general.

So we must either be fine with having constants that change value between Rust versions. Or we must accept the current definition of Duration as the final one. Or not have these constants.

In #78216 I introduced Duration::ZERO, which resolves that for Duration::MIN by being a logical constant and not a repr-sensitive constant, and accordingly removed Duration::MIN. That is one way to simplify an API and its concerns, I suppose.

I do not believe Duration::MAX _should_ be under this feature flag because while it is a constant it has a very different purpose and that is a reasonable concern that is directly related to its intention (saturating ops) and is completely different from most of the concerns here. I also do not have a good answer for what to do with it aside from that reshuffling, however.

Was this page helpful?
0 / 5 - 0 ratings