Add new `duration_suboptimal_units` lint #16250

sbernauer · 2025-12-17T15:04:23Z

Duration::from_mins and Duration::from_hours where recently stabilized in Rust 1.91.0.

In our codebase we often times have things like

Duration::from_secs(5 * 60);
// Since Rust 1.91.0 one can use
Duration::from_mins(5)

During the Rust 1.91.0 bump I noticed we can finally switch to Duration::from_mins(5), but many users might not be aware of this. I started manually updating the places, but halfway through I figured "Can't a lint do this for me?", so I added exactly that in this PR. It does it for all stabilized from_XXX time units.

changelog: Add new [duration_suboptimal_units] lint

rustbot · 2025-12-17T15:04:28Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

sbernauer · 2025-12-17T15:07:51Z

clippy_dev/src/serve.rs


        // Delay to avoid updating the metadata too aggressively.
-        thread::sleep(Duration::from_millis(1000));
+        thread::sleep(Duration::from_secs(1));


This was brought up by the dogfood test

ada4a

Very nicely implemented:)

Left some minor suggestions

View changes since this review

ada4a · 2025-12-17T19:56:43Z

clippy_lints/src/duration_suboptimal_units.rs

+            let function_name = func_name.ident.name.as_str();
+            let Some((promoted_function_name, promoted_value)) = self.promote(cx, function_name, value) else {
+                return;
+            };


It would be marginally better to make (promoted_)function_name Symbols:

Currently, we do the somewhat costly symbol-to-str conversion already when checking if the lint is applicable (when creating function_name). This is unfortunate, as a lint runs much more often than it fires.

If, however, we make function_name a Symbol, we'll:

be able to have Unit.function_name be a Symbol as well, making the equality checks in DurationSuboptimalUnits::promote a bit faster

only need to perform the final conversion of promoted_function_name inside span_lint_and_sugg

This PR is now using Symbol everywhere

ada4a · 2025-12-17T20:26:57Z

clippy_lints/src/duration_suboptimal_units.rs

+        function_name: "from_nanos",
+        // The value doesn't matter, as there is no previous unit
+        factor: 0,
+        const_support_added: msrvs::DURATION_CONSTS,


Using the rust version where the function was constified is a bit.. surprising? Combined with the fact that the lint doesn't make a distinction between const and non-const contexts, two interesting properties emerge:

The lint doesn't suggest Duration::from_mins in non-const context for rust versions 1.3.0 to 1.31.0, even though that would be totally valid. This is admittedly not that big of a problem.

The lint's suggestions in const context are valid "for free". I'd argue that this one's an unexpected property, and so it would be nice to have explicit tests for the const context as well (basically all the existing tests, but in a big const block)

Yes, I took a shortcut of only suggesting functions that are stable in non-const and const, so that we don't need to distinguish them.
The code now clearly separates between const and non-const contexts.
Also added a const block to the tests

ada4a · 2025-12-17T20:27:39Z

clippy_lints/src/duration_suboptimal_units.rs

+struct Unit {
+    /// Name of the function on [`Duration`](std::time::Duration) to construct it from the given
+    /// unit, e.g. [`Duration::from_secs`](std::time::Duration::from_secs)
+    function_name: &'static str,
+
+    /// The increase factor over the previous (smaller) unit
+    factor: u128,
+
+    /// In what rustc version support for this function in const contexts was added
+    const_support_added: RustcVersion,
+}


Nice documentation here and elsewhere:)

ada4a · 2025-12-17T20:30:04Z

clippy_lints/src/duration_suboptimal_units.rs

+    const_support_added: RustcVersion,
+}
+
+const UNITS: &[Unit] = &[


nit: could make this an array to avoid some copying

ada4a · 2025-12-17T20:31:17Z

clippy_lints/src/duration_suboptimal_units.rs

+            // We intentionally don't want to evaluate referenced constants, as we don't want to
+            // recommend a literal value over using constants:
+            //
+            // let dur = Duration::from_secs(SIXTY);
+            //           ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Duration::from_mins(1)`
+            let Some(Constant::Int(value)) = ConstEvalCtxt::new(cx).eval_local(arg, arg.span.ctxt()) else {
+                return;
+            };
+            // There is no need to promote e.g. 0 seconds to 0 hours
+            if value == 0 {
+                return;
+            }
+
+            let function_name = func_name.ident.name.as_str();
+            let Some((promoted_function_name, promoted_value)) = self.promote(cx, function_name, value) else {
+                return;
+            };


nit: all of this could be pulled into the let-chain

ada4a · 2025-12-17T20:34:18Z

clippy_lints/src/duration_suboptimal_units.rs

+impl LateLintPass<'_> for DurationSuboptimalUnits {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
+        // Check if a function on std::time::Duration is called
+        if let ExprKind::Call(func, [arg]) = expr.kind


it could make sense to prepend a !expr.span.from_expansion() check here, just to stay on the safer side

I would prefer to check !expr.span.from_external_macro(cx.sess().source_map()), as linting inside local macros is appreciated.

Note: you can emulate code coming from an external macro by adding //@aux-build:proc_macros.rs at the beginning of the test file, and using code like proc_macros::external! { Duration::from_secs(3600) } in your tests.

Note that I could only find Span::in_external_macro, I will try to use that

Yes, I mistyped

Added !expr.span.in_external_macro(cx.sess().source_map()) as well as a test.
I'm not sure of the distinction between an normal/internal(?) macro and an external macro, so please feel free to correct my comments in the tests.

samueltardieu

This will be quite a long review. First of all, this looks great and I like the structure of the proposed lint.

The changes I'd like to see are mostly:

Don't use &str when Symbol are fine. They are lightweight (integers) and can be compared by equality.
Use narrower types instead of u128 when possible, especially in constants.
You may easily add .from_days() and .from_weeks() to the mix by checking if the feature is enabled. That would make adding them definitely a breeze when they are stabilized.
Add more macros checks: code in macros, code coming from macros, etc.

You are lucky in that you don't have to deal with const contexts: if one constructor works in a const context, then later constructors will also work if they exist as they have been const-stabilized at the same time as they have been stabilized, or at the same time their smaller variants have been stabilized.

View changes since this review

samueltardieu · 2025-12-17T20:03:05Z

clippy_lints/src/duration_suboptimal_units.rs

+pub struct DurationSuboptimalUnits {
+    msrv: Msrv,
+}


Wrt to including Duration::from_days() and Duration::from_weeks(). You might want to add a units: Vec<Unit> to this struct, and initialize it in the constructor: this would allow you to initialize units from UNITS, and if the duration_constructors feature is enabled to add entries from EXTENDED_UNITS that would reference those two functions. Note that this requires that you derive Clone on Unit.

You can check if features are enabled from a TyCtxt: tcx.features().enabled(sym::duration_constructors). And you'll get one when initializing the lint: Box::new(move |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, conf))).

Then you can add #![feature(duration_constructors)] in the test file, or in another one if you want to test both situations.

Implemented this alongside with some tests.
I was not able to contain all the tests in a single file with regard to #![feature(duration_constructors)], so there are now two separate test files. I hope that's ok

samueltardieu · 2025-12-17T20:05:02Z

clippy_lints/src/duration_suboptimal_units.rs

+            //
+            // let dur = Duration::from_secs(SIXTY);
+            //           ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Duration::from_mins(1)`
+            let Some(Constant::Int(value)) = ConstEvalCtxt::new(cx).eval_local(arg, arg.span.ctxt()) else {


To avoid linting on duration values coming from macros, you need to use .eval_local(arg, expr.span.ctxt()) so that the following value is considered non-local:

macro_rules! mac { (slow_rythm) => { 3600 }; } let dur = Duration::from_secs(mac!(slow_rythm));

(please add this as a test)

Added the mentioned test

samueltardieu · 2025-12-17T20:07:11Z

clippy_lints/src/duration_suboptimal_units.rs

+                return;
+            }
+
+            let function_name = func_name.ident.name.as_str();


There is no need to leave the Symbol interning world to do string comparisons. You should stay with symbols as long as possible, that is until you print a diagnostic (and even there, Symbol implements Display).

The code now sticks to Symbols everywhere

samueltardieu · 2025-12-17T20:09:17Z

clippy_lints/src/duration_suboptimal_units.rs

+            }
+
+            let function_name = func_name.ident.name.as_str();
+            let Some((promoted_function_name, promoted_value)) = self.promote(cx, function_name, value) else {


All values for those functions fit in a u64:

Suggested change

let Some((promoted_function_name, promoted_value)) = self.promote(cx, function_name, value) else {

let Some((promoted_function_name, promoted_value)) =

self.promote(cx, func_name.ident.name, u64::try_from(value).unwrap())

else {

I mistakenly though stuff like from_nanos takes an u128. But all constructors we are using here take an u64 and only lower the value, so this is fine (tm).
Updated the code (i used expect instead of unwrap, hope that's ok)

samueltardieu · 2025-12-17T20:13:10Z

clippy_lints/src/duration_suboptimal_units.rs

+    /// function name and value.
+    ///
+    /// Returns [`None`] in case no promotion could be done.
+    fn promote<'a>(&self, cx: &LateContext<'_>, function_name: &'a str, value: u128) -> Option<(&'a str, u128)> {


Suggested change

fn promote<'a>(&self, cx: &LateContext<'_>, function_name: &'a str, value: u128) -> Option<(&'a str, u128)> {

fn promote(&self, cx: &LateContext<'_>, function_name: Symbol, value: u64) -> Option<(Symbol, u64)> {

Updated the types as shown in the suggestion

samueltardieu · 2025-12-17T20:20:19Z

clippy_lints/src/duration_suboptimal_units.rs

+
+const UNITS: &[Unit] = &[
+    Unit {
+        function_name: "from_nanos",


Suggested change

function_name: "from_nanos",

function_name: sym::from_nanos,

(and similarly for the others)

samueltardieu · 2025-12-17T20:21:13Z

clippy_lints/src/duration_suboptimal_units.rs

+    pub fn new(conf: &'static Conf) -> Self {
+        Self { msrv: conf.msrv }
+    }


Others suggestion would lead to something like:

Suggested change

pub fn new(conf: &'static Conf) -> Self {

Self { msrv: conf.msrv }

}

pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {

let mut units = UNITS.to_vec();

if tcx.features().enabled(sym::duration_constructors) {

units.extend(EXTENDED_UNITS.iter().cloned());

}

Self { msrv: conf.msrv, units }

}

Thanks, picked. Except I use copied instead of cloned

I think .iter().copied() could even be left out completely, so that EXTENDED_UNITS is taken by value?

You are absolutely right, now simply went with units.extend(EXTENDED_UNITS);

samueltardieu · 2025-12-17T20:24:16Z

clippy_utils/src/msrvs.rs

    1,34,0 { TRY_FROM }
    1,33,0 { UNDERSCORE_IMPORTS }
-    1,32,0 { CONST_IS_POWER_OF_TWO }
+    1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONSTS }


Suggested change

1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONSTS }

1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_FROM_MICROS_MILLIS }

The entire const and non-const min version detection has changed, can you please have another look at the new naming please?

samueltardieu · 2025-12-17T20:26:12Z

clippy_utils/src/msrvs.rs


 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
+    1,91,0 { DURATION_CONSTRUCTORS_LITE }


Suggested change

1,91,0 { DURATION_CONSTRUCTORS_LITE }

1,91,0 { DURATION_FROM_MINUTES_HOURS }

The entire const and non-const min version detection has changed, can you please have another look at the new naming please?

samueltardieu · 2025-12-17T20:30:49Z

clippy_lints/src/duration_suboptimal_units.rs

+    function_name: &'static str,
+
+    /// The increase factor over the previous (smaller) unit
+    factor: u128,


No factor is larger than 1000.

Suggested change

factor: u128,

factor: u16,

rustbot · 2025-12-17T20:39:34Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

ada4a · 2025-12-17T20:44:04Z

Oops, sorry for stealing your work @samueltardieu ^^ It looks like our suggestions largely overlap; the only thing that only I have suggested is extending the let-chain

samueltardieu · 2025-12-17T20:48:05Z

Oops, sorry for stealing your work @samueltardieu ^^ It looks like our suggestions largely overlap; the only thing that only I have suggested is extending the let-chain

No worries!

sbernauer · 2025-12-17T22:03:49Z

Thanks for your quick and very helpful review! I will try my best to address it - this is my first clippy PR after all 😅

sbernauer · 2025-12-18T09:51:11Z

@rustbot ready

ada4a · 2025-12-18T10:13:11Z

clippy_lints/src/duration_suboptimal_units.rs

+                expr.span,
+                "constructing a `Duration` using a smaller unit when a larger unit would be more readable",
+                "try",
+                format!("Duration::{promoted_constructor}({promoted_value})"),


This could become a problem if the original Duration was used qualified (e.g. as std:: time::Duration) instead of being imported. For this reason, the more correct approach would be to:

replace span_lint_and_sugg with span_lint_and_then

in it, emit a multipart_suggestion_verbose¹, which performs 2 separate replacements:

the span of the original function name (e.g. from_secs) with promoted_constructor

the span of the original arg with promoted_value

You can grep for multipart_suggestion in the repo should you need some inspiration.

Footnotes

theoretically, multipart_suggestion would work just as well, but the verbose version shows the suggestion in a nice diff-like view, which will great thanks to the suggestion actually changing only the relevant things. ↩

Good catch! Updated the PR to use multipart_suggestion_verbose and added a test for qualified Durations.
Just FYI that in the tests multipart_suggestion and multipart_suggestion_verbose formatted exactly the same way

Good catch! Updated the PR to use multipart_suggestion_verbose and added a test for qualified Durations.

Nice, thank you:)

Just FYI that in the tests multipart_suggestion and multipart_suggestion_verbose formatted exactly the same way

Ah, I misremembered, sorry for the confusion -- multipart suggestions are indeed verbose by default; it's span_suggestion (which is used in span_lint_and_sugg) and span_suggestion_verbose for which this distinction matters

Makes sense 👍 Should we leave it as-is or shall I change something?

Makes sense 👍 Should we leave it as-is or shall I change something?

No no, leaving it as _verbose is fine.

One more thing: usually, the "try" message is used with non-verbose diagnostics, as the space is limited -- since we're being verbose now anyway, we might as well give a more descriptive message, something like "try using <promoted_constructor>"

Sure thing, switched to "try using {promoted_constructor}"

ada4a · 2025-12-18T10:21:08Z

clippy_utils/src/msrvs.rs

    1,34,0 { TRY_FROM }
    1,33,0 { UNDERSCORE_IMPORTS }
-    1,32,0 { CONST_IS_POWER_OF_TWO }
+    1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS }


nit: the other constants have CONST_ at the start

Suggested change

1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS }

1,32,0 { CONST_IS_POWER_OF_TWO, CONST_DURATION_FROM_NANOS_MICROS_MILLIS_SECS }

upsi, fixed

rustbot · 2025-12-18T14:16:58Z

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.

rustbot assigned samueltardieu Dec 17, 2025

rustbot added needs-fcp S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 17, 2025

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch from 2113a96 to 6f9d615 Compare December 17, 2025 15:05

This comment has been minimized.

Sign in to view

sbernauer commented Dec 17, 2025

View reviewed changes

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch 2 times, most recently from 8eddec5 to 993e4f1 Compare December 17, 2025 19:52

ada4a suggested changes Dec 17, 2025

View reviewed changes

samueltardieu requested changes Dec 17, 2025

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 Dec 17, 2025

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch 2 times, most recently from b0f1852 to 467969d Compare December 18, 2025 09:37

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

ada4a reviewed Dec 18, 2025

View reviewed changes

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch from 467969d to 31740b4 Compare December 18, 2025 10:15

ada4a suggested changes Dec 18, 2025

View reviewed changes

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch 2 times, most recently from a064750 to c66e4a4 Compare December 18, 2025 11:28

This comment has been minimized.

Sign in to view

Add new duration_suboptimal_units lint

a3a052d

sbernauer force-pushed the feat/duration-suboptimal-unit-lint branch from c66e4a4 to a3a052d Compare December 18, 2025 14:16

	fn promote<'a>(&self, cx: &LateContext<'_>, function_name: &'a str, value: u128) -> Option<(&'a str, u128)> {
	fn promote(&self, cx: &LateContext<'_>, function_name: Symbol, value: u64) -> Option<(Symbol, u64)> {

-    pub fn new(conf: &'static Conf) -> Self {
-        Self { msrv: conf.msrv }
-    }
+    pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
+        let mut units = UNITS.to_vec();
+        if tcx.features().enabled(sym::duration_constructors) {
+            units.extend(EXTENDED_UNITS.iter().cloned());
+        }
+        Self { msrv: conf.msrv, units }
+    }

	1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONSTS }
	1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_FROM_MICROS_MILLIS }

	1,91,0 { DURATION_CONSTRUCTORS_LITE }
	1,91,0 { DURATION_FROM_MINUTES_HOURS }

	1,32,0 { CONST_IS_POWER_OF_TWO, DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS }
	1,32,0 { CONST_IS_POWER_OF_TWO, CONST_DURATION_FROM_NANOS_MICROS_MILLIS_SECS }

Add new duration_suboptimal_units lint #16250

Are you sure you want to change the base?

Add new duration_suboptimal_units lint #16250

Conversation

sbernauer commented Dec 17, 2025

Uh oh!

rustbot commented Dec 17, 2025

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

ada4a left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Add new `duration_suboptimal_units` lint #16250

Add new `duration_suboptimal_units` lint #16250

ada4a left a comment •

edited by rustbot

Loading

sbernauer Dec 18, 2025 •

edited

Loading

samueltardieu Dec 17, 2025 •

edited

Loading

sbernauer Dec 18, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

sbernauer Dec 18, 2025 •

edited

Loading

sbernauer Dec 18, 2025 •

edited

Loading