Skip to content

Fix duration_suboptimal_units false positive on small literal values #16596

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.

Sign up for GitHub

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

Closed
veeceey wants to merge 1 commit into from
Closed

Fix duration_suboptimal_units false positive on small literal values #16596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions clippy_lints/src/duration_suboptimal_units.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ impl LateLintPass<'_> for DurationSuboptimalUnits {
// There is no need to promote e.g. 0 seconds to 0 hours
&& value != 0
&& let Some((promoted_constructor, promoted_value)) = self.promote(cx, func_name.ident.name, value)
// For plain integer literals, only lint if the promoted value is large enough.
// Small promoted values (e.g. `from_millis(1_000)` -> `from_secs(1)`) are not
// necessarily more readable, and keeping the smaller unit makes quick adjustments
// easier (e.g. changing 1_000 to 1_200 without also changing the function name).
// For expressions (e.g. `10 * 60`), always lint since the expression already
// signals intent to compute a converted value.
&& !(matches!(arg.kind, ExprKind::Lit(_)) && promoted_value <= 10)
{
span_lint_and_then(
cx,
Expand Down
38 changes: 26 additions & 12 deletions tests/ui/duration_suboptimal_units.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ macro_rules! mac {
3600
};
(duration) => {
Duration::from_mins(5)
Duration::from_mins(12)
//~^ duration_suboptimal_units
};
(arg => $e:expr) => {
Expand All @@ -23,18 +23,28 @@ fn main() {
let dur = Duration::from_secs(42);
let dur = Duration::from_hours(3);

let dur = Duration::from_mins(1);
// Literals with small promoted values should not lint (issue #16532)
let dur = Duration::from_secs(60);
let dur = Duration::from_secs(180);
let dur = Duration::from_millis(5_000);
let dur = Duration::from_millis(1_000);
let dur = Duration::from_secs(3_600);

// Literals with large promoted values should still lint
let dur = Duration::from_mins(11);
//~^ duration_suboptimal_units
let dur = Duration::from_mins(3);
let dur = Duration::from_mins(11);
//~^ duration_suboptimal_units

// Expressions should always lint regardless of the promoted value
let dur = Duration::from_mins(10);
//~^ duration_suboptimal_units
let dur = Duration::from_hours(24);
//~^ duration_suboptimal_units
let dur = Duration::from_secs(5);
//~^ duration_suboptimal_units
let dur = Duration::from_hours(13);
//~^ duration_suboptimal_units
let dur = Duration::from_mins(1);
//~^ duration_suboptimal_units

// Constants are intentionally not resolved, as we don't want to recommend a literal value over
// using constants.
Expand All @@ -44,22 +54,26 @@ fn main() {

const {
let dur = Duration::from_secs(0);
let dur = Duration::from_secs(5);
//~^ duration_suboptimal_units
let dur = Duration::from_mins(3);
//~^ duration_suboptimal_units
// Literals with small promoted values should not lint in const blocks either
let dur = Duration::from_millis(5_000);
let dur = Duration::from_secs(180);

// Expressions should still lint in const blocks
let dur = Duration::from_hours(24);
//~^ duration_suboptimal_units

let dur = Duration::from_secs(SIXTY);
}

// Qualified Durations must be kept
std::time::Duration::from_mins(1);
// Qualified Durations with small promoted values should not lint
std::time::Duration::from_secs(60);

// Qualified Durations with expressions should still lint
std::time::Duration::from_mins(10);
//~^ duration_suboptimal_units

// We lint in normal macros
assert_eq!(Duration::from_hours(1), Duration::from_mins(6));
assert_eq!(Duration::from_mins(10), Duration::from_mins(6));
//~^ duration_suboptimal_units

// We lint in normal macros (marker is in macro itself)
Expand Down
30 changes: 22 additions & 8 deletions tests/ui/duration_suboptimal_units.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ macro_rules! mac {
3600
};
(duration) => {
Duration::from_secs(300)
Duration::from_secs(12 * 60)
//~^ duration_suboptimal_units
};
(arg => $e:expr) => {
Expand All @@ -23,18 +23,28 @@ fn main() {
let dur = Duration::from_secs(42);
let dur = Duration::from_hours(3);

// Literals with small promoted values should not lint (issue #16532)
let dur = Duration::from_secs(60);
//~^ duration_suboptimal_units
let dur = Duration::from_secs(180);
let dur = Duration::from_millis(5_000);
let dur = Duration::from_millis(1_000);
let dur = Duration::from_secs(3_600);

// Literals with large promoted values should still lint
let dur = Duration::from_secs(660);
//~^ duration_suboptimal_units
let dur = Duration::from_millis(660_000);
//~^ duration_suboptimal_units

// Expressions should always lint regardless of the promoted value
let dur = Duration::from_secs(10 * 60);
//~^ duration_suboptimal_units
let dur = Duration::from_mins(24 * 60);
//~^ duration_suboptimal_units
let dur = Duration::from_millis(5_000);
//~^ duration_suboptimal_units
let dur = Duration::from_nanos(13 * 60 * 60 * 1_000 * 1_000 * 1_000);
//~^ duration_suboptimal_units
let dur = Duration::from_secs(2 * 30);
//~^ duration_suboptimal_units

// Constants are intentionally not resolved, as we don't want to recommend a literal value over
// using constants.
Expand All @@ -44,22 +54,26 @@ fn main() {

const {
let dur = Duration::from_secs(0);
// Literals with small promoted values should not lint in const blocks either
let dur = Duration::from_millis(5_000);
//~^ duration_suboptimal_units
let dur = Duration::from_secs(180);
//~^ duration_suboptimal_units

// Expressions should still lint in const blocks
let dur = Duration::from_mins(24 * 60);
//~^ duration_suboptimal_units

let dur = Duration::from_secs(SIXTY);
}

// Qualified Durations must be kept
// Qualified Durations with small promoted values should not lint
std::time::Duration::from_secs(60);

// Qualified Durations with expressions should still lint
std::time::Duration::from_secs(10 * 60);
//~^ duration_suboptimal_units

// We lint in normal macros
assert_eq!(Duration::from_secs(3_600), Duration::from_mins(6));
assert_eq!(Duration::from_secs(10 * 60), Duration::from_mins(6));
//~^ duration_suboptimal_units

// We lint in normal macros (marker is in macro itself)
Expand Down
96 changes: 36 additions & 60 deletions tests/ui/duration_suboptimal_units.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:26:15
--> tests/ui/duration_suboptimal_units.rs:34:15
|
LL | let dur = Duration::from_secs(60);
| ^^^^^^^^^^^^^^^^^^^^^^^
LL | let dur = Duration::from_secs(660);
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::duration-suboptimal-units` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::duration_suboptimal_units)]`
help: try using from_mins
|
LL - let dur = Duration::from_secs(60);
LL + let dur = Duration::from_mins(1);
LL - let dur = Duration::from_secs(660);
LL + let dur = Duration::from_mins(11);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:28:15
--> tests/ui/duration_suboptimal_units.rs:36:15
|
LL | let dur = Duration::from_secs(180);
| ^^^^^^^^^^^^^^^^^^^^^^^^
LL | let dur = Duration::from_millis(660_000);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_mins
|
LL - let dur = Duration::from_secs(180);
LL + let dur = Duration::from_mins(3);
LL - let dur = Duration::from_millis(660_000);
LL + let dur = Duration::from_mins(11);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:30:15
--> tests/ui/duration_suboptimal_units.rs:40:15
|
LL | let dur = Duration::from_secs(10 * 60);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -37,7 +37,7 @@ LL + let dur = Duration::from_mins(10);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:32:15
--> tests/ui/duration_suboptimal_units.rs:42:15
|
LL | let dur = Duration::from_mins(24 * 60);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -49,19 +49,7 @@ LL + let dur = Duration::from_hours(24);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:34:15
|
LL | let dur = Duration::from_millis(5_000);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_secs
|
LL - let dur = Duration::from_millis(5_000);
LL + let dur = Duration::from_secs(5);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:36:15
--> tests/ui/duration_suboptimal_units.rs:44:15
|
LL | let dur = Duration::from_nanos(13 * 60 * 60 * 1_000 * 1_000 * 1_000);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -73,31 +61,19 @@ LL + let dur = Duration::from_hours(13);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:47:19
|
LL | let dur = Duration::from_millis(5_000);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_secs
|
LL - let dur = Duration::from_millis(5_000);
LL + let dur = Duration::from_secs(5);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:49:19
--> tests/ui/duration_suboptimal_units.rs:46:15
|
LL | let dur = Duration::from_secs(180);
| ^^^^^^^^^^^^^^^^^^^^^^^^
LL | let dur = Duration::from_secs(2 * 30);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_mins
|
LL - let dur = Duration::from_secs(180);
LL + let dur = Duration::from_mins(3);
LL - let dur = Duration::from_secs(2 * 30);
LL + let dur = Duration::from_mins(1);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:51:19
--> tests/ui/duration_suboptimal_units.rs:62:19
|
LL | let dur = Duration::from_mins(24 * 60);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -109,47 +85,47 @@ LL + let dur = Duration::from_hours(24);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:58:5
--> tests/ui/duration_suboptimal_units.rs:72:5
|
LL | std::time::Duration::from_secs(60);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | std::time::Duration::from_secs(10 * 60);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_mins
|
LL - std::time::Duration::from_secs(60);
LL + std::time::Duration::from_mins(1);
LL - std::time::Duration::from_secs(10 * 60);
LL + std::time::Duration::from_mins(10);
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:62:16
--> tests/ui/duration_suboptimal_units.rs:76:16
|
LL | assert_eq!(Duration::from_secs(3_600), Duration::from_mins(6));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | assert_eq!(Duration::from_secs(10 * 60), Duration::from_mins(6));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try using from_hours
help: try using from_mins
|
LL - assert_eq!(Duration::from_secs(3_600), Duration::from_mins(6));
LL + assert_eq!(Duration::from_hours(1), Duration::from_mins(6));
LL - assert_eq!(Duration::from_secs(10 * 60), Duration::from_mins(6));
LL + assert_eq!(Duration::from_mins(10), Duration::from_mins(6));
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:13:9
|
LL | Duration::from_secs(300)
| ^^^^^^^^^^^^^^^^^^^^^^^^
LL | Duration::from_secs(12 * 60)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
LL | let dur = mac!(duration);
| -------------- in this macro invocation
|
= note: this error originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info)
help: try using from_mins
|
LL - Duration::from_secs(300)
LL + Duration::from_mins(5)
LL - Duration::from_secs(12 * 60)
LL + Duration::from_mins(12)
|

error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
--> tests/ui/duration_suboptimal_units.rs:105:9
--> tests/ui/duration_suboptimal_units.rs:119:9
|
LL | _ = Duration::from_secs(67_768_040_922_076_800);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -160,5 +136,5 @@ LL - _ = Duration::from_secs(67_768_040_922_076_800);
LL + _ = Duration::from_hours(18824455811688);
|

error: aborting due to 13 previous errors
error: aborting due to 11 previous errors

11 changes: 8 additions & 3 deletions tests/ui/duration_suboptimal_units_days_weeks.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@
use std::time::Duration;

fn main() {
let dur = Duration::from_mins(1);
//~^ duration_suboptimal_units
// Literals with small promoted values should not lint (issue #16532)
let dur = Duration::from_secs(60);
let dur = Duration::from_hours(24);

let dur = Duration::from_days(1);
// Literals with large promoted values should still lint
let dur = Duration::from_days(11);
//~^ duration_suboptimal_units

// Expressions should always lint
let dur = Duration::from_weeks(13);
//~^ duration_suboptimal_units
let dur = Duration::from_days(1);
//~^ duration_suboptimal_units
}
9 changes: 7 additions & 2 deletions tests/ui/duration_suboptimal_units_days_weeks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@
use std::time::Duration;

fn main() {
// Literals with small promoted values should not lint (issue #16532)
let dur = Duration::from_secs(60);
//~^ duration_suboptimal_units

let dur = Duration::from_hours(24);

// Literals with large promoted values should still lint
let dur = Duration::from_hours(264);
//~^ duration_suboptimal_units

// Expressions should always lint
let dur = Duration::from_nanos(13 * 7 * 24 * 60 * 60 * 1_000 * 1_000 * 1_000);
//~^ duration_suboptimal_units
let dur = Duration::from_hours(2 * 12);
//~^ duration_suboptimal_units
}
Loading