Add new duration_suboptimal_units lint

Author: sbernauer

CHANGELOG.md

@@ -6406,6 +6406,7 @@ Released 2018-09-13
 [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod
 [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument
 [`duplicated_attributes`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes
+[`duration_suboptimal_units`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_suboptimal_units
 [`duration_subsec`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_subsec
 [`eager_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_transmute
 [`elidable_lifetime_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#elidable_lifetime_names

clippy_dev/src/serve.rs

@@ -54,7 +54,7 @@ pub fn run(port: u16, lint: Option<String>) -> ! {
         }
 
         // Delay to avoid updating the metadata too aggressively.
-        thread::sleep(Duration::from_millis(1000));
+        thread::sleep(Duration::from_secs(1));
     }
 }
 

clippy_lints/src/declared_lints.rs

@@ -135,6 +135,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::drop_forget_ref::FORGET_NON_DROP_INFO,
     crate::drop_forget_ref::MEM_FORGET_INFO,
     crate::duplicate_mod::DUPLICATE_MOD_INFO,
+    crate::duration_suboptimal_units::DURATION_SUBOPTIMAL_UNITS_INFO,
     crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO,
     crate::empty_drop::EMPTY_DROP_INFO,
     crate::empty_enums::EMPTY_ENUMS_INFO,

clippy_lints/src/duration_suboptimal_units.rs

@@ -0,0 +1,175 @@
+use clippy_config::Conf;
+use clippy_utils::consts::{ConstEvalCtxt, Constant};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::msrvs::{self, Msrv};
+use clippy_utils::res::MaybeDef;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath, RustcVersion};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+use rustc_span::symbol::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for instances where a `std::time::Duration` is constructed using a smaller time unit
+    /// when the value could be expressed more clearly using a larger unit.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Using a smaller unit for a duration that is evenly divisible by a larger unit reduces
+    /// readability. Readers have to mentally convert values, which can be error-prone and makes
+    /// the code less clear.
+    ///
+    /// ### Example
+    /// ```
+    /// let dur = Duration::from_secs(180);
+    /// let dur2 = Duration::from_mins(10 * 60);
+    /// ```
+    ///
+    /// Use instead:
+    /// ```
+    /// let dur = Duration::from_mins(3);
+    /// let dur2 = Duration::from_hours(10);
+    /// ```
+    #[clippy::version = "1.94.0"]
+    pub DURATION_SUBOPTIMAL_UNITS,
+    pedantic,
+    "constructing a `Duration` using a smaller unit when a larger unit would be more readable"
+}
+
+impl_lint_pass!(DurationSuboptimalUnits => [DURATION_SUBOPTIMAL_UNITS]);
+
+pub struct DurationSuboptimalUnits {
+    msrv: Msrv,
+}
+
+impl DurationSuboptimalUnits {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self { msrv: conf.msrv }
+    }
+}
+
+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
+            && let ExprKind::Path(QPath::TypeRelative(func_ty, func_name)) = func.kind
+            && cx
+                .typeck_results()
+                .node_type(func_ty.hir_id)
+                .is_diag_item(cx, sym::Duration)
+        {
+            // 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;
+            };
+
+            span_lint_and_sugg(
+                cx,
+                DURATION_SUBOPTIMAL_UNITS,
+                expr.span,
+                "constructing a `Duration` using a smaller unit when a larger unit would be more readable",
+                "try",
+                format!("Duration::{promoted_function_name}({promoted_value})"),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}
+
+impl DurationSuboptimalUnits {
+    /// Tries to promote the given function and value to a bigger time unit and returns the promoted
+    /// 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)> {
+        let mut promoted = false;
+        let mut promoted_function_name = function_name;
+        let mut promoted_value = value;
+
+        let bigger_units = UNITS
+            .iter()
+            .skip_while(|unit| unit.function_name != function_name)
+            .skip(1);
+
+        for bigger_unit in bigger_units {
+            if !self.msrv.meets(cx, bigger_unit.const_support_added) {
+                // We have to break early, as we can not skip versions.
+                // We can't skip any, as they are needed to correctly calculate the promoted value.
+                break;
+            }
+
+            if promoted_value.is_multiple_of(bigger_unit.factor) {
+                promoted = true;
+                promoted_value /= bigger_unit.factor;
+                promoted_function_name = bigger_unit.function_name;
+            } else {
+                break;
+            }
+        }
+
+        promoted.then_some((promoted_function_name, promoted_value))
+    }
+}
+
+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,
+}
+
+const UNITS: &[Unit] = &[
+    Unit {
+        function_name: "from_nanos",
+        // The value doesn't matter, as there is no previous unit
+        factor: 0,
+        const_support_added: msrvs::DURATION_CONSTS,
+    },
+    Unit {
+        function_name: "from_micros",
+        factor: 1_000,
+        const_support_added: msrvs::DURATION_CONSTS,
+    },
+    Unit {
+        function_name: "from_millis",
+        factor: 1_000,
+        const_support_added: msrvs::DURATION_CONSTS,
+    },
+    Unit {
+        function_name: "from_secs",
+        factor: 1_000,
+        const_support_added: msrvs::DURATION_CONSTS,
+    },
+    Unit {
+        function_name: "from_mins",
+        factor: 60,
+        const_support_added: msrvs::DURATION_CONSTRUCTORS_LITE,
+    },
+    Unit {
+        function_name: "from_hours",
+        factor: 60,
+        const_support_added: msrvs::DURATION_CONSTRUCTORS_LITE,
+    },
+    // `from_days` and `from_weeks` are behind the nightly `duration_constructors` feature and can
+    // be added once stabilized
+];

clippy_lints/src/lib.rs

@@ -113,6 +113,7 @@ mod doc;
 mod double_parens;
 mod drop_forget_ref;
 mod duplicate_mod;
+mod duration_suboptimal_units;
 mod else_if_without_else;
 mod empty_drop;
 mod empty_enums;
@@ -855,6 +856,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
         Box::new(|_| Box::new(volatile_composites::VolatileComposites)),
         Box::new(|_| Box::<replace_box::ReplaceBox>::default()),
         Box::new(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))),
+        Box::new(move |_| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(conf))),
         // add late passes here, used by `cargo dev new_lint`
     ];
     store.late_passes.extend(late_lints);

clippy_utils/src/msrvs.rs

@@ -23,6 +23,7 @@ macro_rules! msrv_aliases {
 
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
+    1,91,0 { DURATION_CONSTRUCTORS_LITE }
     1,88,0 { LET_CHAINS }
     1,87,0 { OS_STR_DISPLAY, INT_MIDPOINT, CONST_CHAR_IS_DIGIT, UNSIGNED_IS_MULTIPLE_OF, INTEGER_SIGN_CAST }
     1,85,0 { UINT_FLOAT_MIDPOINT, CONST_SIZE_OF_VAL }
@@ -69,7 +70,7 @@ msrv_aliases! {
     1,35,0 { OPTION_COPIED, RANGE_CONTAINS }
     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 }
     1,31,0 { OPTION_REPLACE }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
     1,29,0 { ITER_FLATTEN }

tests/ui/duration_suboptimal_units.fixed

@@ -0,0 +1,42 @@
+#![warn(clippy::duration_suboptimal_units)]
+
+use std::time::Duration;
+
+fn main() {
+    let dur = Duration::from_secs(0);
+    let dur = Duration::from_secs(42);
+    let dur = Duration::from_hours(3);
+
+    let dur = Duration::from_mins(1);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_mins(3);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_mins(10);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_hours(24);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_hours(13);
+    //~^ duration_suboptimal_units
+
+    // Constants are intentionally not resolved, as we don't want to recommend a literal value over
+    // using constants.
+    const SIXTY: u64 = 60;
+    let dur = Duration::from_secs(SIXTY);
+    // Technically it would be nice to use Duration::from_mins(SIXTY) here, but that is a follow-up
+    let dur = Duration::from_secs(SIXTY * 60);
+}
+
+mod my_duration {
+    struct Duration {}
+
+    impl Duration {
+        pub const fn from_secs(_secs: u64) -> Self {
+            Self {}
+        }
+    }
+
+    fn test() {
+        // Only suggest the change for std::time::Duration, not for other Duration structs
+        let dur = Duration::from_secs(60);
+    }
+}

tests/ui/duration_suboptimal_units.rs

@@ -0,0 +1,42 @@
+#![warn(clippy::duration_suboptimal_units)]
+
+use std::time::Duration;
+
+fn main() {
+    let dur = Duration::from_secs(0);
+    let dur = Duration::from_secs(42);
+    let dur = Duration::from_hours(3);
+
+    let dur = Duration::from_secs(60);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_secs(180);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_secs(10 * 60);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_mins(24 * 60);
+    //~^ duration_suboptimal_units
+    let dur = Duration::from_nanos(13 * 60 * 60 * 1_000 * 1_000 * 1_000);
+    //~^ duration_suboptimal_units
+
+    // Constants are intentionally not resolved, as we don't want to recommend a literal value over
+    // using constants.
+    const SIXTY: u64 = 60;
+    let dur = Duration::from_secs(SIXTY);
+    // Technically it would be nice to use Duration::from_mins(SIXTY) here, but that is a follow-up
+    let dur = Duration::from_secs(SIXTY * 60);
+}
+
+mod my_duration {
+    struct Duration {}
+
+    impl Duration {
+        pub const fn from_secs(_secs: u64) -> Self {
+            Self {}
+        }
+    }
+
+    fn test() {
+        // Only suggest the change for std::time::Duration, not for other Duration structs
+        let dur = Duration::from_secs(60);
+    }
+}

tests/ui/duration_suboptimal_units.stderr

@@ -0,0 +1,35 @@
+error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
+  --> tests/ui/duration_suboptimal_units.rs:10:15
+   |
+LL |     let dur = Duration::from_secs(60);
+   |               ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Duration::from_mins(1)`
+   |
+   = note: `-D clippy::duration-suboptimal-units` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::duration_suboptimal_units)]`
+
+error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
+  --> tests/ui/duration_suboptimal_units.rs:12:15
+   |
+LL |     let dur = Duration::from_secs(180);
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Duration::from_mins(3)`
+
+error: constructing a `Duration` using a smaller unit when a larger unit would be more readable
+  --> tests/ui/duration_suboptimal_units.rs:14:15
+   |
+LL |     let dur = Duration::from_secs(10 * 60);
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `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:16:15
+   |
+LL |     let dur = Duration::from_mins(24 * 60);
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `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:18:15
+   |
+LL |     let dur = Duration::from_nanos(13 * 60 * 60 * 1_000 * 1_000 * 1_000);
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Duration::from_hours(13)`
+
+error: aborting due to 5 previous errors
+