Add std::time::Duration::{from_days, from_hours, from_mins}

[ChristopherRabotin]

Minutes, hours and days* have a fixed number of seconds. Allowing to construct a Duration from a number of these common time spans alleviates a minor implementation by each dev needing this.

Note of warning: This is my first contribution attempt to the rust code base. I think I've followed all the rules, but let me know if I haven't.

I am already planning to using this in hifitime, and I thought it may be useful to the broader Rust community. BTW, hifitime is a crate for date time handling in rust built on top of the current std::time::Duration and it supports leap seconds (which I need for other projects).

(*) Days may have an extra second called the leap second. It's announced and determined by the IETF one year around the end of December. In recent years, there's been one leap second every two years. So we can safely assume that when a dev would called Duration::from_days they expect the usual number of seconds in a day.

Tests passed locally:

test time/duration.rs - time::duration::Duration (line 37) ... ok
test time/duration.rs - time::duration::Duration::as_secs (line 238) ... ok
test time/duration.rs - time::duration::Duration::as_secs (line 227) ... ok
test time/duration.rs - time::duration::Duration::checked_add (line 320) ... ok
test time/duration.rs - time::duration::Duration::checked_div (line 429) ... ok
test time/duration.rs - time::duration::Duration::checked_mul (line 394) ... ok
test time/duration.rs - time::duration::Duration::checked_sub (line 358) ... ok
test thread/mod.rs - thread::spawn (line 488) ... ok
test time/duration.rs - time::duration::Duration::from_hours (line 106) ... ok
test time/duration.rs - time::duration::Duration::from_days (line 87) ... ok
test time/duration.rs - time::duration::Duration::from_millis (line 162) ... ok
test time/duration.rs - time::duration::Duration::from_micros (line 182) ... ok
test time/mod.rs - time::Instant (line 56) ... ok
test time/mod.rs - time::Instant::duration_since (line 170) ... ok
test time/duration.rs - time::duration::Duration::from_secs (line 144) ... ok
test time/duration.rs - time::duration::Duration::new (line 69) ... ok
test time/mod.rs - time::Instant::elapsed (line 194) ... ok
test time/duration.rs - time::duration::Duration::from_mins (line 125) ... ok
test time/duration.rs - time::duration::Duration::from_nanos (line 203) ... ok

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[BurntSushi]

I think in principle this seems fine, but we should check what other time libraries do. Go, for example, has minutes and hours, but not days. Are there other issues aside from leap seconds with using day as a duration unit? (I'm pretty sure there is, but I'm not knowledgeable enough to articulate what's inside my head. Days aren't always exactly 24 hours, for example?)

If we do want to keep the from_days constructor, then we probably want a stronger specification that says that it doesn't do anything special and interprets a day as 24 hours.

cc @rust-lang/libs

[BurntSushi]

I think in principle this seems fine, but we should check what other time libraries do. Go, for example, has minutes and hours, but not days. Are there other issues aside from leap seconds with using day as a duration unit? (I'm pretty sure there is, but I'm not knowledgeable enough to articulate what's inside my head. Days aren't always exactly 24 hours, for example?)

If we do want to keep the from_days constructor, then we probably want a stronger specification that says that it doesn't do anything special and interprets a day as 24 hours.

cc @rust-lang/libs

[BurntSushi]

I think we generally create a separate (non-PR) tracking issue for new features. I think you can leave it like this for now, but if we do merge this, then I think we'll want to create a tracking issue and then update the PR.

Is that right @alexcrichton?

[BurntSushi]

Also, I note that this feature name is duration_from_hours, which is the same as the from_hours method but different from the name duration_from_mins used for the from_mins method.

I suspect you probably just want to use one feature name for all of them (duration_from perhaps?).

[BurntSushi]

I think I'd rather this be called from_minutes.

[retep998]

Well there is also the issue of daylight savings time. 5:00 on some day plus Duration::from_days(3) could end up being 4:00 on another day or perhaps 6:00.

Basically, splitting the timestamp into parts, adding one to the day part, and joining it back together can give a different result than adding Duration::from_days(1) to a SystemTime, and some people would consider that bad.

[ChristopherRabotin]

Thanks for the prompt review!

Concerning the length of days, they are always strictly 86_400 seconds long, which is strictly 24 hours. (Note that there's also a "sideral day" which lasts 23 hours 56 minutes 4 seconds and a bit, but that notion is only used in some astronomical observations and relies on a different definition of the day.) The confusing part with leap seconds is that there are several "TAI seconds" associated to a unique UTC time. In other words, UTC moves slightly faster than the real atomic time.

The issue pinpointed by retep998 #47097 (comment) is valid (and it's the reason Go didn't include Days, cf. golang/go#11473 (comment)), but I would argue that it's an implementor's problem.

All that said, if there's a strong preference to not include from_days, that's totally fine of course.

[BurntSushi]

I guess I would personally want to be conservative here and not have a from_days constructor. If someone wants it, then writing out the math themselves is very simple, so it's not a big loss or anything. I guess I just feel like the potential for confusion here just isn't worth it.

But, I do not have a lot of domain knowledge here so my opinion is held pretty weakly. Let's see what the rest of the library team thinks. :)

[nagisa]

The initial RFC didn’t add these constructors specifically because these units only make sense with a starting date in mind.

EDIT:

This proposal does not, at this time, include mechanisms for instantiating a Duration from weeks, days, hours or minutes, because there are caveats to each of those units. In particular, the existence of leap seconds means that it is only possible to properly understand them relative to a particular starting point.

The Joda-Time library in Java explains the problem well in their documentation:

A duration in Joda-Time represents a duration of time measured in milliseconds. The duration is often obtained from an interval. Durations are a very simple concept, and the implementation is also simple. They have no chronology or time zone, and consist solely of the millisecond duration.

A period in Joda-Time represents a period of time defined in terms of fields, for example, 3 years 5 months 2 days and 7 hours. This differs from a duration in that it is inexact in terms of milliseconds. A period can only be resolved to an exact number of milliseconds by specifying the instant (including chronology and time zone) it is relative to.

[ChristopherRabotin]

I don't find the argumentation in the initial RFC to be that convincing, if I'm honest. The Joda Time library specifically provides these standardHours and standardDays constructors, as quoted by the RFC.

I'll go ahead and remove the from_days constructor now, and can add it back if the rest of the library team turns out wanting it.

[kennytm]

Triage ping @rust-lang/libs!

(Note that this PR now only provides from_hours and from_minutes.)

[BurntSushi]

cc @wycats

[aturon]

Sorry for the delay on this -- @wycats (who has thought deeply about these issues) plans to reply this weekend.

[wycats]

As noted, the original RFC left out a lot of expected APIs for a few reasons:

1. At the time (around 1.0), the primary motivation for working on this feature was to provide a good type for other std APIs that required durations (notably timeouts). The design was therefore quite conservative.
2. A lot of common date and duration APIs have cargo culted the same few patterns over time, with the same set of known failure modes. I was interested in more carefully working through the design space to reduce the error-prone-ness.

An example of (2) is the way that people often use time and duration APIs to benchmark. In many languages, it's common to create the equivalent of SystemTime twice, and compare the difference in time between the two times.

There are some problems with this:

• The default time is often something analogous to SystemTime (not monotonic, not high precision), with extra effort needed to switch to monotonic, high precision time.
• In addition to the imprecision caused by NTP, this can actually result in negative elapsed time, which is even more nonsensical than incorrect durations.

Because of the fact that certain times (like the time that comes from mtime) really must be SystemTime, and people sometimes do want to see how much time has elapsed since a file was created, most time libraries attempt to "simplify" the situation by unifying things into a single type most people will need to understand. The consequence, of course, is that many normal, everyday uses of these APIs have all kinds of regular gotchas (that emerge unpredictably).

In contrast, the RFC I wrote creates a very nice API for tracking elapsed time:

let i = Instant::now();
// later
let amount = i.elapsed();

The elapsed() API is a genuinely more ergonomic API than the more traditional method of subtracting two times, and it covers one of the most dominant uses of this API. But an attempt to use SystemTime instead of Instant will produce a Result, hopefully nudging the user back towards the more convenient and more reliable API.

My philosophy for extensions to the Duration design is to do a similar analysis of the use-cases that are driving the extension and look for "win-win" alternative designs that are both nice to use and avoid some of the gotchas.

After that very long aside, in this case, I'd like to start by understanding why people want these very large constants to begin with. I can see why somebody would genuinely want a Duration in the seconds (or perhaps even minutes) to conveniently construct parameters to pass to timeouts.

But I'm not quite sure why somebody would want hours or days.

When looking at my own backend code, I see cases where people are using these very large constants as an approximation for day-based math, which I have strong objections to (day-based math has even more gotchas).

So I'd like us to try to enumerate some good examples of code that constructs durations lasting days (or even hours) so we can see what gotchas might apply to them.

[ChristopherRabotin]

The only good examples I have for measuring hours or days are simulation software. Specifically, a large part of my day-to-day coding involves writing integration software of dynamic systems, and these have time spans on the order of months or years.

Even more specifically, I wrote a simulation toolkit last year which, for many reasons, I'll be converting to Rust while adding numerous functionalities. This requires long term precise time measurements (and justifies the coding of hifitime). You may find specific time scale examples in this test file in Go. You'll notice that the integration time steps for the tests run between 24 hours to 190 days.

[wycats]

@ChristopherRabotin Thanks so much for that information.

I've done a review of a lot of my own codebases in various languages, and I don't have any reservations about standardizing seconds.

In my own codebases, one antipattern I found with very long durations is that code sometimes relied upon the process itself remaining up, and would have been more resilient by using crons or other externalized timers. Because Instants aren't shareable across processes easily (because monotonic time backends might rely on time-since-process-start), it's not even particularly easy using std to take do some Instant/Duration math and make it durable across process restarts.

Writing this comment, I realize we probably should do something about that use case (the ability to store Instant::now() + durably).

In my own code, some of the uses of minutes had this problem (better off as crons), but I don't mind adding minutes (as standard_minutes, like Joda-Time).

Once you get beyond seconds and minutes, in addition to the problem of reliance on long-lived processes, there is also the problem of edge cases interacting poorly with calendars (adding 1 standard day to midnight might not increment the day when another calendar is in play), and I'd want to see a broader set of use-cases to help drive a possible design for this feature.

[ChristopherRabotin]

I agree that calendar and specifically time zones are not easy to handle. That's why in hifitime I've focused on using only std (unless compiling the simulation module which handles clock drifts of hardware clocks). As such, all times spans can be added and subtracted by the intermediary of a hifitime::Instant which computes the exact number of seconds between the given input date time and 1900 Jan 01 midnight (leap seconds included). To be honest, I would be thrilled if my library somehow ended up part of std:: but that is definitely a huge stretch from its current status.

Anyhow, would you encourage me to change my PR such that from_hours becomes from_std_hours and from_minutes become from_std_minutes?

[bors]

☔️ The latest upstream changes (presumably #46666) made this pull request unmergeable. Please resolve the merge conflicts.

[aturon]

@ChristopherRabotin Sorry for the delayed response! My take from @wycats's comment is that he feels comfortable adding minutes, but still feels like hours are ripe for misuse (and are relatively niche).

So I think at this point we'd be happy to take from_std_mins but probably want to stop there. Alternatively, we could take both as unstable APIs, and pick back up the discussion around hours prior to stabilization (where we'd have more data about usage).

[ChristopherRabotin]

@aturon , @wycats , if this PR will only add minutes, I really don't see much of a point in all honesty, especially if I need to rewrite this PR to accommodate for the upstream changes. =/

Converting from seconds to minutes doesn't change a whole lot (factor of 60), but I definitely have use in the seconds to hours conversion. Now, if I'm the only one who will ever use this, then I can just continue using the hack I currently use.

[shepmaster]

Soo... where do we stand on this PR? Should it just be closed due to disinterest?

[ChristopherRabotin]

Is the consensus that there should only be a standard minutes initializer? If so then yes, we should close this PR.

…

On Fri, Mar 2, 2018, 17:15 Jake Goulding ***@***.***> wrote: Soo... where do we stand on this PR? Should it just be closed due to disinterest? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47097 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEma6O037K8RGcxbbZNT7A5812u_lMl9ks5taeCGgaJpZM4RP80O> .

[pietroalbini]

Yep, the consensus seems to have only a minutes initializer. Closing as the author's wish.

[newpavlov]

I would love to hear your opinions on minute, hour and day constants discussed in #52556. Proposed solution is to name them as STD_MINUTE, STD_HOUR and STD_DAY to highlight the difference from UTC units which can have leap seconds.