Fix duration_suboptimal_units false positive on small literal values#16596
Fix duration_suboptimal_units false positive on small literal values#16596veeceey wants to merge 1 commit into
duration_suboptimal_units false positive on small literal values#16596Conversation
|
r? @Jarcho rustbot has assigned @Jarcho. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
dee229e to
fc970c2
Compare
This comment has been minimized.
This comment has been minimized.
fc970c2 to
1f9e74b
Compare
|
It looks like there is already another PR for this: #16565. Do you want to collaborate with the it's author to decide between, or maybe combine, your approaches? |
This comment has been minimized.
This comment has been minimized.
|
Thanks for flagging #16565! I didn't see that one when I started working on this. Happy to close this in favor of that PR if the approach there covers the same case, or we can coordinate if there are differences. Let me know what makes sense. |
For plain integer literals, suppress the lint when the promoted value would be <= 10, as the conversion is not necessarily more readable and keeping the smaller unit makes quick adjustments easier (e.g. changing `from_millis(1_000)` to `from_millis(1_200)` without also changing the function name). Expressions like `10 * 60` continue to always trigger the lint since they already signal intent to compute a converted value.
1f9e74b to
19c7555
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. |
|
I think I prefer your comments in the code and in the tests, since they describe things in a bit more detail, but the test cases themselves look fine in both PRs. I'd say let's concentrate our efforts in the older PR |
|
Thanks for the pointer — happy to consolidate efforts on #16565. That PR covers the same fix and I agree concentrating on one is better. Closing this in favor of that one. |
Summary
Fixes #16532
For plain integer literals, suppress the
duration_suboptimal_unitslint when the promoted (converted) value would be <= 10. Small promoted values likefrom_millis(1_000)->from_secs(1)are not necessarily more readable, and keeping the smaller unit makes quick adjustments easier (e.g. changing1_000to1_200).For expressions (e.g.
from_secs(10 * 60)), always trigger the lint regardless of the promoted value, since the expression already signals intent to compute a converted value.This follows the approach discussed in the issue.
Test plan
tests/ui/duration_suboptimal_units.rswith test cases for literals with small/large promoted values and expressionstests/ui/duration_suboptimal_units_days_weeks.rswith analogous test casescargo test --test compile-test -- duration_suboptimal_unitschangelog: [
duration_suboptimal_units]: no longer triggers on small literal values where the promoted unit conversion result is <= 10