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,229 @@
+use std::ops::ControlFlow;
+
+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 clippy_utils::sym;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath, RustcVersion};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::ty::TyCtxt;
+use rustc_session::impl_lint_pass;
+use rustc_span::Symbol;
+
+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
+    /// ```
+    /// use std::time::Duration;
+    ///
+    /// let dur = Duration::from_millis(5_000);
+    /// let dur = Duration::from_secs(180);
+    /// let dur = Duration::from_mins(10 * 60);
+    /// ```
+    ///
+    /// Use instead:
+    /// ```
+    /// use std::time::Duration;
+    ///
+    /// let dur = Duration::from_secs(5);
+    /// let dur = Duration::from_mins(3);
+    /// let dur = 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,
+    units: Vec<Unit>,
+}
+
+impl DurationSuboptimalUnits {
+    pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
+        // The order of the units matters, as they are walked top to bottom
+        let mut units = UNITS.to_vec();
+        if tcx.features().enabled(sym::duration_constructors) {
+            units.extend(EXTENDED_UNITS.iter().copied());
+        }
+        Self { msrv: conf.msrv, units }
+    }
+}
+
+impl LateLintPass<'_> for DurationSuboptimalUnits {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
+        if !expr.span.in_external_macro(cx.sess().source_map())
+            // Check if a function on std::time::Duration is called
+            && 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, expr.span.ctxt())
+            && let value = u64::try_from(value).expect("All Duration::from_<time-unit> constructors take a u64")
+            // There is no need to promote e.g. 0 seconds to 0 hours
+            && value != 0
+            && let Some((promoted_constructor, promoted_value)) = self.promote(cx, expr, func_name.ident.name, value)
+        {
+            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_constructor}({promoted_value})"),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}
+
+impl DurationSuboptimalUnits {
+    /// Tries to promote the given constructor and value to a bigger time unit and returns the
+    /// promoted constructor name and value.
+    ///
+    /// Returns [`None`] in case no promotion could be done.
+    fn promote(
+        &self,
+        cx: &LateContext<'_>,
+        expr: &'_ Expr<'_>,
+        constructor_name: Symbol,
+        value: u64,
+    ) -> Option<(Symbol, u64)> {
+        let in_const = cx.tcx.hir_is_inside_const_context(expr.hir_id);
+
+        let (best_unit, best_value) = self
+            .units
+            .iter()
+            .skip_while(|unit| unit.constructor_name != constructor_name)
+            .skip(1)
+            .try_fold(
+                (constructor_name, value),
+                |(current_unit, current_value), bigger_unit| {
+                    if let Some(bigger_value) = current_value.div_exact(u64::from(bigger_unit.factor))
+                        && bigger_unit
+                            .stable_since(in_const)
+                            .is_none_or(|v| self.msrv.meets(cx, v))
+                    {
+                        ControlFlow::Continue((bigger_unit.constructor_name, bigger_value))
+                    } else {
+                        // We have to break early, as we can't skip versions, as they are needed to
+                        // correctly calculate the promoted value.
+                        ControlFlow::Break((current_unit, current_value))
+                    }
+                },
+            )
+            .into_value();
+        (best_unit != constructor_name).then_some((best_unit, best_value))
+    }
+}
+
+#[derive(Clone, Copy)]
+struct Unit {
+    /// Name of the constructor on [`Duration`](std::time::Duration) to construct it from the given
+    /// unit, e.g. [`Duration::from_secs`](std::time::Duration::from_secs)
+    constructor_name: Symbol,
+
+    /// The increase factor over the previous (smaller) unit
+    factor: u16,
+
+    /// In what rustc version stable support for this constructor was added.
+    stable_since: Option<RustcVersion>,
+
+    /// In what rustc version stable support for this constructor in const contexts was added.
+    const_stable_since: Option<RustcVersion>,
+}
+
+impl Unit {
+    /// In what rustc version stable support was added. This depends if the constructor call is in
+    /// a const context or not.
+    ///
+    /// If [`None`] is returned, there is no rustc version restriction (e.g. because it actually
+    /// depends on a feature).
+    pub fn stable_since(&self, in_const: bool) -> Option<RustcVersion> {
+        if in_const {
+            self.const_stable_since
+        } else {
+            self.stable_since
+        }
+    }
+}
+
+/// Time unit constructors available on stable. The order matters!
+const UNITS: [Unit; 6] = [
+    Unit {
+        constructor_name: sym::from_nanos,
+        // The value doesn't matter, as there is no previous unit
+        factor: 0,
+        stable_since: Some(msrvs::DURATION_FROM_NANOS_MICROS),
+        const_stable_since: Some(msrvs::DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS),
+    },
+    Unit {
+        constructor_name: sym::from_micros,
+        factor: 1_000,
+        stable_since: Some(msrvs::DURATION_FROM_NANOS_MICROS),
+        const_stable_since: Some(msrvs::DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS),
+    },
+    Unit {
+        constructor_name: sym::from_millis,
+        factor: 1_000,
+        stable_since: Some(msrvs::DURATION_FROM_MILLIS_SECS),
+        const_stable_since: Some(msrvs::DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS),
+    },
+    Unit {
+        constructor_name: sym::from_secs,
+        factor: 1_000,
+        stable_since: Some(msrvs::DURATION_FROM_MILLIS_SECS),
+        const_stable_since: Some(msrvs::DURATION_CONST_FROM_NANOS_MICROS_MILLIS_SECS),
+    },
+    Unit {
+        constructor_name: sym::from_mins,
+        factor: 60,
+        stable_since: Some(msrvs::DURATION_FROM_MINUTES_HOURS),
+        const_stable_since: Some(msrvs::DURATION_FROM_MINUTES_HOURS),
+    },
+    Unit {
+        constructor_name: sym::from_hours,
+        factor: 60,
+        stable_since: Some(msrvs::DURATION_FROM_MINUTES_HOURS),
+        const_stable_since: Some(msrvs::DURATION_FROM_MINUTES_HOURS),
+    },
+];
+
+/// Time unit constructors behind the `duration_constructors` feature. The order matters!
+const EXTENDED_UNITS: [Unit; 2] = [
+    Unit {
+        constructor_name: sym::from_days,
+        factor: 24,
+        stable_since: None,
+        const_stable_since: None,
+    },
+    Unit {
+        constructor_name: sym::from_weeks,
+        factor: 7,
+        stable_since: None,
+        const_stable_since: None,
+    },
+];

clippy_lints/src/lib.rs

@@ -1,11 +1,13 @@
 #![cfg_attr(bootstrap, feature(array_windows))]
 #![feature(box_patterns)]
-#![feature(macro_metavar_expr_concat)]
+#![feature(control_flow_into_value)]
+#![feature(exact_div)]
 #![feature(f128)]
 #![feature(f16)]
 #![feature(if_let_guard)]
 #![feature(iter_intersperse)]
 #![feature(iter_partition_in_place)]
+#![feature(macro_metavar_expr_concat)]
 #![feature(never_type)]
 #![feature(rustc_private)]
 #![feature(stmt_expr_attributes)]
@@ -113,6 +115,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 +858,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 |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, 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_FROM_MINUTES_HOURS }
     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,19 +70,20 @@ 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_CONST_FROM_NANOS_MICROS_MILLIS_SECS }
     1,31,0 { OPTION_REPLACE }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
     1,29,0 { ITER_FLATTEN }
     1,28,0 { FROM_BOOL, REPEAT_WITH, SLICE_FROM_REF }
-    1,27,0 { ITERATOR_TRY_FOLD, DOUBLE_ENDED_ITERATOR_RFIND }
+    1,27,0 { ITERATOR_TRY_FOLD, DOUBLE_ENDED_ITERATOR_RFIND, DURATION_FROM_NANOS_MICROS }
     1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN, POINTER_ADD_SUB_METHODS }
     1,24,0 { IS_ASCII_DIGIT, PTR_NULL }
     1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN }
     1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR }
     1,16,0 { STR_REPEAT, RESULT_UNWRAP_OR_DEFAULT }
     1,15,0 { MAYBE_BOUND_IN_WHERE }
     1,13,0 { QUESTION_MARK_OPERATOR }
+    1,3,0 { DURATION_FROM_MILLIS_SECS }
 }
 
 /// `#[clippy::msrv]` attributes are rarely used outside of Clippy's test suite, as a basic

clippy_utils/src/sym.rs

@@ -140,6 +140,7 @@ generate! {
     disallowed_types,
     drain,
     dump,
+    duration_constructors,
     ends_with,
     enum_glob_use,
     enumerate,
@@ -164,11 +165,19 @@ generate! {
     from_be_bytes,
     from_bytes_with_nul,
     from_bytes_with_nul_unchecked,
+    from_days,
+    from_hours,
     from_le_bytes,
+    from_micros,
+    from_millis,
+    from_mins,
+    from_nanos,
     from_ne_bytes,
     from_ptr,
     from_raw,
+    from_secs,
     from_str_radix,
+    from_weeks,
     fs,
     fuse,
     futures_util,