Fix duration_suboptimal_units for small literals#16922
Fix duration_suboptimal_units for small literals#16922tanndlin wants to merge 1 commit intorust-lang:masterfrom
duration_suboptimal_units for small literals#16922Conversation
|
r? @llogiq rustbot has assigned @llogiq. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
Pull request overview
Updates duration_suboptimal_units to avoid linting on small integer literal values where the promoted (larger-unit) value would be <= 10, addressing the false positive described in #16532.
Changes:
- Adjusts lint logic to suppress suggestions for small integer literals while still linting expressions.
- Updates lint documentation/examples to avoid the new suppression edge cases.
- Expands UI tests (including
duration_constructorscoverage) to validate the new behavior and expected suggestions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
clippy_lints/src/duration_suboptimal_units.rs |
Implements the <= 10 suppression rule for integer literals and updates lint docs/examples. |
tests/ui/duration_suboptimal_units.rs |
Adds UI coverage for small literals vs expressions and updates expected lint sites. |
tests/ui/duration_suboptimal_units.fixed |
Updates fixed-output expectations to match the new lint behavior. |
tests/ui/duration_suboptimal_units.stderr |
Updates stderr expectations for the new/shifted diagnostics. |
tests/ui/duration_suboptimal_units_days_weeks.rs |
Adds non-linting literal + linting expression cases under duration_constructors. |
tests/ui/duration_suboptimal_units_days_weeks.fixed |
Updates fixed-output expectations for the days/weeks test. |
tests/ui/duration_suboptimal_units_days_weeks.stderr |
Updates stderr expectations for the days/weeks test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
|
Could you please squash down the commits? I think it would also be nice to add @Hectonight and @veeceey as co-authors to the commit |
3c536bd to
a1ffdf3
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Absolutely! It's squashed with all 3 of us on the commit now |
| let dur = Duration::from_secs(60); | ||
| let dur = Duration::from_secs(61 - 1); | ||
| //~^ duration_suboptimal_units | ||
|
|
||
| let dur = Duration::from_hours(24); | ||
| let dur = Duration::from_hours(12 * 2); | ||
| //~^ duration_suboptimal_units |
There was a problem hiding this comment.
Instead of using these imo awkward expressions, could you please use literals that are large enough to trigger the lint? I think it'd make sense to use 6000 and 2400, respectively, here, as those are clear multiples of the minimal values that the lint fires on (60 and 24, respectively). You could then move the value-too-small cases into fn issue16532() { } again, but it's fine to not bother to...
There was a problem hiding this comment.
lol i never even realized the original commit touches this file. Are we cool with deleting this file?
Instead of using these imo awkward expressions, could you please use literals that are large enough to trigger the lint?
I do know part of the intent of these test cases is that it only triggers for literals, not expressions, because an expression would likely show intent that you are trying to use a bigger unit. I'll add some that are quite as awkward
There was a problem hiding this comment.
Are we cool with deleting this file?
The reason it's there is to test the currently unstable constructors Duration::from_{days,weeks}. Granted, I'm not sure why it also has the test for minutes (the first one highlighted) – maybe it's just a sanity check to ensure that the logic for the unstable constructors doesn't mess up the logic for the stable ones.
So I'd say let's leave it be^^
Co-authored-by: tanndlin <tanndlin@gmail.com> Co-authored-by: veeceey <varun_6april@hotmail.com>
a1ffdf3 to
da72354
Compare
fixes #16532
changelog: Fix [duration_suboptimal_units] false positive for small integer literals whose promoted value would be at most 10.
Combined #16565 and #16596