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

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

Closed
declanvk opened this issue Mar 5, 2024 · 1 comment
Closed

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

declanvk opened this issue Mar 5, 2024 · 1 comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@declanvk
Copy link

declanvk commented Mar 5, 2024

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

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@declanvk declanvk added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Mar 5, 2024
@declanvk declanvk changed the title Add as_*_f64 helper functions for Duration for units other than seconds Add as_millis_{f64,f32} helper functions for Duration Mar 5, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 12, 2024

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.

@Amanieu Amanieu closed this as completed Mar 12, 2024
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Mar 12, 2024
declanvk added a commit to declanvk/rust-clippy that referenced this issue Mar 23, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Mar 23, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Apr 1, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Apr 3, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Apr 3, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Jun 2, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
declanvk added a commit to declanvk/rust-clippy that referenced this issue Jun 3, 2024
**Description**
Add a lint which checks for a binop or cast where the expression
is converting a Duration to a floating point number, and losing
precision along the way.

The lint won't fire for most cases involving `as_nanos()` since
converting that to floating point already gives the max precision.

The lint is also restricted to an MSRV of 1.38.0, since that is
when the `as_secs_{f32,f64}()` methods were stabilized.

**Motivation**
This change is motivated by [a rust stdlib ACP which proposed
`as_millis_{f64,f32}`][ACP-link]. As part of that I did some code
searches on github (see ACP for links) that showed a lot of code
which converted Duration values to floating point using methods other
than `as_secs_{f32,64}()`, and were losing precision because of that.

This lint seems like a good way to raise awareness and prompt
using the existing methods.

[ACP-link]: rust-lang/libs-team#349

**Testing Done**
Added UI tests, ran `cargo test`, followed the Clippy manual
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

2 participants