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

Tracking issue for duration_constants #57391

Open
frewsxcv opened this Issue Jan 6, 2019 · 14 comments

Comments

Projects
None yet
7 participants
@frewsxcv
Copy link
Member

frewsxcv commented Jan 6, 2019

Implemented in #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);
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);
}
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 7, 2019

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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 1, 2019

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

I’ll FCP to merge once this is resolved one way or another.

@stjepang

This comment has been minimized.

Copy link
Contributor

stjepang commented Feb 2, 2019

@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.

Centril added a commit to Centril/rust that referenced this issue Feb 23, 2019

Rollup merge of rust-lang#58595 - stjepang:make-duration-consts-assoc…
…iated, r=oli-obk

Turn duration consts into associated consts

As suggested in rust-lang#57391 (comment), I'm moving `Duration` constants (`SECOND`, `MILLISECOND` and so on; currently behind unstable `duration_constants` feature) into the `impl Duration` block.

cc @frewsxcv @SimonSapin

Centril added a commit to Centril/rust that referenced this issue Feb 23, 2019

Rollup merge of rust-lang#58595 - stjepang:make-duration-consts-assoc…
…iated, r=oli-obk

Turn duration consts into associated consts

As suggested in rust-lang#57391 (comment), I'm moving `Duration` constants (`SECOND`, `MILLISECOND` and so on; currently behind unstable `duration_constants` feature) into the `impl Duration` block.

cc @frewsxcv @SimonSapin

Centril added a commit to Centril/rust that referenced this issue Feb 23, 2019

Rollup merge of rust-lang#58595 - stjepang:make-duration-consts-assoc…
…iated, r=oli-obk

Turn duration consts into associated consts

As suggested in rust-lang#57391 (comment), I'm moving `Duration` constants (`SECOND`, `MILLISECOND` and so on; currently behind unstable `duration_constants` feature) into the `impl Duration` block.

cc @frewsxcv @SimonSapin

Centril added a commit to Centril/rust that referenced this issue Feb 23, 2019

Rollup merge of rust-lang#58595 - stjepang:make-duration-consts-assoc…
…iated, r=oli-obk

Turn duration consts into associated consts

As suggested in rust-lang#57391 (comment), I'm moving `Duration` constants (`SECOND`, `MILLISECOND` and so on; currently behind unstable `duration_constants` feature) into the `impl Duration` block.

cc @frewsxcv @SimonSapin

Centril added a commit to Centril/rust that referenced this issue Feb 23, 2019

Rollup merge of rust-lang#58595 - stjepang:make-duration-consts-assoc…
…iated, r=oli-obk

Turn duration consts into associated consts

As suggested in rust-lang#57391 (comment), I'm moving `Duration` constants (`SECOND`, `MILLISECOND` and so on; currently behind unstable `duration_constants` feature) into the `impl Duration` block.

cc @frewsxcv @SimonSapin
@stjepang

This comment has been minimized.

Copy link
Contributor

stjepang commented Feb 23, 2019

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

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 27, 2019

Updated the issue description.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 27, 2019

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

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.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 28, 2019

@rfcbot concern missed the point

As I understand it, the point of duration constants when they were suggested in #51610 (comment) 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?

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 28, 2019

@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?

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 28, 2019

@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
@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 28, 2019

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;
  |                           ^^^^^^^^^^^^^^^^^^^^
@stjepang

This comment has been minimized.

Copy link
Contributor

stjepang commented Feb 28, 2019

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.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Mar 11, 2019

@rfcbot resolve consider unit structs

Too confusing.


@rfcbot resolve clippy

I filed rust-lang/rust-clippy#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.

@stjepang

This comment has been minimized.

Copy link
Contributor

stjepang commented Mar 11, 2019

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());
@newpavlov

This comment has been minimized.

Copy link
Contributor

newpavlov commented Mar 11, 2019

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.