Add lint which checks that duration conversion aren't losing precision

[declanvk]

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

Testing Done Added UI tests, ran cargo test, followed the Clippy manual

---

changelog: [duration_to_float_precision_loss]: Add new lint which checks for precision loss in duration to float conversions

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[samueltardieu]

I've ran lintcheck with your patch applied on the top 500 crates:

bindgen-0.69.4/time.rs:38:24 clippy::duration_to_float_precision_loss "calling `as_secs_f64() * 1e3` is more precise than this calculation"
indicatif-0.17.8/src/state.rs:655:5 clippy::duration_to_float_precision_loss "calling `as_secs_f64()` is more precise than this calculation"
serde_with-3.7.0/src/utils.rs:117:5 clippy::duration_to_float_precision_loss "calling `as_secs_f64()` is more precise than this calculation"
serde_with-3.7.0/src/utils/duration.rs:181:42 clippy::duration_to_float_precision_loss "calling `as_secs_f64()` is more precise than this calculation"

So it looks like this lint could be useful.

[declanvk]

bindgen-0.69.4/time.rs:38:24:

let time = (elapsed.as_secs() as f64) * 1e3 +
    (elapsed.subsec_nanos() as f64) / 1e6;

indicatif-0.17.8/src/state.rs:655:

d.as_secs() as f64 + f64::from(d.subsec_nanos()) / 1_000_000_000f64

serde_with-3.7.0/src/utils.rs:117:5

(dur.as_secs() as f64) + (dur.subsec_nanos() as f64) / (NANOS_PER_SEC as f64)

serde_with-3.7.0/src/utils/duration.rs:181:42

let mut secs = source.sign.apply(source.duration.as_secs() as f64);

---

The source code of as_secs_f64 is:

pub const fn as_secs_f64(&self) -> f64 {
    (self.secs as f64) + (self.nanos.0 as f64) / (NANOS_PER_SEC as f64)
}

In order, the code from the linted crates appears to be calculating:

1. the number of milliseconds as a float with nanosecond precision
2. the number of seconds as a float with nanosecond precision
3. the number of seconds as a float with nanosecond precision
4. the number of seconds with no added precision

I think the lint application is probably okay advice for 2-4, but the advice it will give in the 1st case may be a bit confusing.

Maybe it would be worthwhile to try to recognize the case where someone is doing the as_secs_f64 calculation manually? Also might be worthwhile to support {f32,f64}::from(number) instead of just the _ as {f32,f64} cases.

[bors]

:umbrella: The latest upstream changes (presumably #12312) made this pull request unmergeable. Please resolve the merge conflicts.

[blyxyas]

Honestly @declanvk, this is some of the best-written code, comments and PR description I've ever read. Congrats on making the code so understandable!

Normally, 500+ lines diffs are quite painful to read, but this one is quite nice :heart:

[declanvk]

Thank you @blyxyas for reviewing! I'll address your comments in my next attempt.

I wanted to get your opinion on the lint category versus the number of false positives. I placed the lint in the nursery to start, but I wasn't sure if it should move elsewhere? Or if that should be a later PR after the lint has baked a while.

Also, based on the data @samueltardieu gathered (thank you!), are you worried about any of the cases from https://github.com/rust-lang/rust-clippy/pull/12539#issuecomment-2016611234? Arguable, cases 1-3 have slightly off suggestions from the lint.

[declanvk]

I pushed a new revision addressing 3/4 comments, let me know your thoughts on the last comment I left unresolved. Thanks again!

[blyxyas]

I forgot to ask, are you planning to implement https://github.com/rust-lang/rust-clippy/pull/12539#discussion_r1579882036 or is this good enough? This is up to you.

[declanvk]

Oh sorry! I added the commit just because it was the easiest way to apply your suggestions, I'm still planning to work on the other comments/open that issue possibly

[bors]

:umbrella: The latest upstream changes (presumably #12107) made this pull request unmergeable. Please resolve the merge conflicts.

[declanvk]

I added the rest of the requested changes to the commit, and I'm planning to cut that issue about recognizing (duration.as_secs() * 1000) as f64 as a separate issue once this merged.

[declanvk]

Thanks for all your work in this code review! And sorry about the mini-hiatus I took in the middle 😁 . I'm looking forward to merging this in!

I'm still planning to create that followup issue about the alternate forms of duration conversion too.

[declanvk]

Also, should I respond to questions/comments on the Zulip thread? Or is that mostly for core contributors?

[blyxyas]

Also, should I respond to questions/comments on the Zulip thread? Or is that mostly for core contributors?

Rust's Zulip is open to anyone and everyone, in fact, it's better that you yourself (as the PR author) respond these questions. Meow! =^w^=

[bors]

:umbrella: The latest upstream changes (presumably #12937) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey @declanvk , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[declanvk]

Hey @xFrednet, I am planning to continue, but it might take me a second to get back to this. Thanks for checking in!