rust-lang / libs-team

The home of the library team
Apache License 2.0
123 stars 19 forks source link

Add `as_millis_{f64,f32}` helper functions for `Duration` #349

Closed declanvk closed 6 months ago

declanvk commented 7 months ago

Proposal

Problem statement

We have a very common pattern in our code base when we're recording metrics that involve a span of time:

duration.as_millis() as f64 // CASE 1 - problematic because it loses the sub-milliseconds precision
duration.as_secs_f64() * 1000.0 // CASE 2 - preferred

Reminding people to use the second case so that preserve sub-millisecond precision is not very difficult, but I'd like to make it an even easier pattern for people to use and simply express that they want "the number of milliseconds as a floating point number, including the sub-millisecond values for precision".

Motivating examples or use cases

As far as I can tell, the .as_secs_f64() * 1000.0 is a very common pattern and it searching for it on Github returns hundreds of results (alt search for `*f32).

Doing a little research for this I realized people also write this as .as_nanos() as f64 / 1_000_000.0, Github search yield some additional results.

Solution sketch

Ideally the new methods would include:

impl Duration {
    pub fn as_millis_f64() -> f64;
    pub fn as_millis_f32() -> f64;
}

I would also be interested in as_{micros,nanos}_{f64,f32} from a completeness perspective, but I haven't seen this pattern as commonly for conversion to those units.

Alternatives

The main alternative I see is to continue using the duration.as_secs_f64() * 1000.0. This alternative is quick to type and easy to understand.

Links and related work

A Github search provides some existing examples of Duration extension traits or other implementations, here are a couple:

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

Amanieu commented 6 months ago

We discussed this in the libs-api meeting today. The first point that came up is that the original motivation (precision loss due to as_millis() as f64) is something that really should be addressed by a clippy lint. We would strongly encourage adding this lint as a first step.

Regarding the as_millis_f64/f32 methods, there was some hesitation but in the end we were convinced by how often the pattern appeared in the code search results. This pattern can be expressed in a much more readable way with the new methods.

Feel free to open a tracking issue and open a PR to rust-lang/rust to add it as an unstable feature.