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

declanvk · 2024-03-23T06:28:01Z

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 · 2024-03-23T06:28:06Z

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 · 2024-03-23T09:42:45Z

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 · 2024-03-23T21:46:56Z

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:

the number of milliseconds as a float with nanosecond precision
the number of seconds as a float with nanosecond precision
the number of seconds as a float with nanosecond precision
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 · 2024-03-30T18:53:16Z

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

blyxyas · 2024-04-01T09:40:16Z

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 ❤️

blyxyas

Great for a first iteration. Meow meow 😸

clippy_lints/src/duration_to_float_precision_loss.rs

tests/ui/duration_to_float_precision_loss.rs

declanvk · 2024-04-01T18:35:20Z

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 #12539 (comment)? Arguable, cases 1-3 have slightly off suggestions from the lint.

declanvk · 2024-04-03T04:51:48Z

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

blyxyas

Sorry for the delay, some nits, some comments. Still very good for a first contribution. I really like your use of useful comments.

clippy_lints/src/duration_to_float_precision_loss.rs

tests/ui/duration_to_float_precision_loss.rs

blyxyas · 2024-04-27T14:13:15Z

I forgot to ask, are you planning to implement #12539 (comment) or is this good enough? This is up to you.

declanvk · 2024-04-28T02:55:15Z

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 · 2024-05-12T16:46:12Z

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

declanvk · 2024-06-02T05:34:54Z

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.

blyxyas

Just a nit, and this should be ready! (We don't want to lint proc macros)

clippy_lints/src/duration_to_float_precision_loss.rs

**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

- I applied about half of these from the UI - Made more parts of the example code visible - Added test on duration ref - Rebased over conflict - Moved to the `style` lint group Co-authored-by: Alejandra González <blyxyas@gmail.com>

blyxyas

Just a version update, and this should be ready! I've already opened the FCP thread on our Zulip :)

If everything goes as planned, this should be merged in about a week! Thanks for your contribution, and welcome to the project! ❤️

https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.20.60duration_to_float_precision_loss.60

clippy_lints/src/duration_to_float_precision_loss.rs

Co-authored-by: Alejandra González <blyxyas@gmail.com>

declanvk · 2024-06-04T18:24:03Z

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 · 2024-06-04T18:25:16Z

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

Alexendoo · 2024-06-05T18:57:07Z

clippy_lints/src/duration_to_float_precision_loss.rs

+ }
+
+ fn emit_lint(&self, cx: &LateContext<'_>) {
+ let mut applicability = Applicability::MachineApplicable;


Would be good to use MaybeIncorrect since there's a behaviour change. A note making it clear that the suggestion would include fractional e.g. milliseconds would also be good

blyxyas · 2024-06-05T18:57:53Z

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 · 2024-06-15T23:47:42Z

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

xFrednet · 2024-07-04T12:36:13Z

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 · 2024-07-04T15:21:42Z

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

rustbot assigned blyxyas Mar 23, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 23, 2024

declanvk mentioned this pull request Mar 23, 2024

Tracking Issue for Duration::as_millis_{f64,f32} rust-lang/rust#122451

Open

6 tasks

declanvk force-pushed the duration-to-float branch from 0ef98ae to 98bbb37 Compare March 23, 2024 06:47

declanvk force-pushed the duration-to-float branch from 98bbb37 to 092b40d Compare April 1, 2024 02:32

blyxyas requested changes Apr 1, 2024

View reviewed changes

declanvk force-pushed the duration-to-float branch 2 times, most recently from 0749e19 to 056ed4a Compare April 3, 2024 04:36

declanvk requested a review from blyxyas April 22, 2024 17:39

blyxyas requested changes Apr 25, 2024

View reviewed changes

declanvk force-pushed the duration-to-float branch from 57fe0ef to effce8a Compare June 2, 2024 04:06

blyxyas requested changes Jun 2, 2024

View reviewed changes

clippy_lints/src/duration_to_float_precision_loss.rs Outdated Show resolved Hide resolved

declanvk and others added 2 commits June 3, 2024 15:34

Apply suggestions from code review

475f28e

- I applied about half of these from the UI - Made more parts of the example code visible - Added test on duration ref - Rebased over conflict - Moved to the `style` lint group Co-authored-by: Alejandra González <blyxyas@gmail.com>

declanvk force-pushed the duration-to-float branch from 91ead2d to 475f28e Compare June 3, 2024 22:40

declanvk requested a review from blyxyas June 3, 2024 23:11

blyxyas approved these changes Jun 4, 2024

View reviewed changes

clippy_lints/src/duration_to_float_precision_loss.rs Outdated Show resolved Hide resolved

Update clippy version to 1.80.0

2ec131d

Co-authored-by: Alejandra González <blyxyas@gmail.com>

Alexendoo reviewed Jun 5, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

declanvk commented Mar 23, 2024

rustbot commented Mar 23, 2024

samueltardieu commented Mar 23, 2024

declanvk commented Mar 23, 2024

bors commented Mar 30, 2024

blyxyas commented Apr 1, 2024 •

edited

Loading

blyxyas left a comment

declanvk commented Apr 1, 2024

declanvk commented Apr 3, 2024

blyxyas left a comment

blyxyas commented Apr 27, 2024

declanvk commented Apr 28, 2024

bors commented May 12, 2024

declanvk commented Jun 2, 2024

blyxyas left a comment •

edited

Loading

blyxyas left a comment

declanvk commented Jun 4, 2024

declanvk commented Jun 4, 2024

Alexendoo Jun 5, 2024

blyxyas commented Jun 5, 2024

bors commented Jun 15, 2024

xFrednet commented Jul 4, 2024

declanvk commented Jul 4, 2024

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

Are you sure you want to change the base?

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

Conversation

declanvk commented Mar 23, 2024

rustbot commented Mar 23, 2024

samueltardieu commented Mar 23, 2024

declanvk commented Mar 23, 2024

bors commented Mar 30, 2024

blyxyas commented Apr 1, 2024 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

declanvk commented Apr 1, 2024

declanvk commented Apr 3, 2024

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas commented Apr 27, 2024

declanvk commented Apr 28, 2024

bors commented May 12, 2024

declanvk commented Jun 2, 2024

blyxyas left a comment • edited Loading

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

declanvk commented Jun 4, 2024

declanvk commented Jun 4, 2024

Alexendoo Jun 5, 2024

Choose a reason for hiding this comment

blyxyas commented Jun 5, 2024

bors commented Jun 15, 2024

xFrednet commented Jul 4, 2024

declanvk commented Jul 4, 2024

blyxyas commented Apr 1, 2024 •

edited

Loading

blyxyas left a comment •

edited

Loading