Move UnnecessarySortBy into Methods lint pass

Author: Jarcho

clippy_lints/src/lib.register_all.rs

@@ -213,6 +213,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::UNNECESSARY_FIND_MAP),
     LintId::of(methods::UNNECESSARY_FOLD),
     LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS),
+    LintId::of(methods::UNNECESSARY_SORT_BY),
     LintId::of(methods::UNNECESSARY_TO_OWNED),
     LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT),
     LintId::of(methods::USELESS_ASREF),
@@ -334,7 +335,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS),
     LintId::of(unnamed_address::VTABLE_ADDRESS_COMPARISONS),
     LintId::of(unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS),
-    LintId::of(unnecessary_sort_by::UNNECESSARY_SORT_BY),
     LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
     LintId::of(unused_io_amount::UNUSED_IO_AMOUNT),
     LintId::of(unused_unit::UNUSED_UNIT),

clippy_lints/src/lib.register_complexity.rs

@@ -57,6 +57,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(methods::SKIP_WHILE_NEXT),
     LintId::of(methods::UNNECESSARY_FILTER_MAP),
     LintId::of(methods::UNNECESSARY_FIND_MAP),
+    LintId::of(methods::UNNECESSARY_SORT_BY),
     LintId::of(methods::USELESS_ASREF),
     LintId::of(misc::SHORT_CIRCUIT_STATEMENT),
     LintId::of(misc_early::UNNEEDED_WILDCARD_PATTERN),
@@ -99,7 +100,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(types::TYPE_COMPLEXITY),
     LintId::of(types::VEC_BOX),
     LintId::of(unit_types::UNIT_ARG),
-    LintId::of(unnecessary_sort_by::UNNECESSARY_SORT_BY),
     LintId::of(unwrap::UNNECESSARY_UNWRAP),
     LintId::of(useless_conversion::USELESS_CONVERSION),
     LintId::of(zero_div_zero::ZERO_DIVIDED_BY_ZERO),

clippy_lints/src/lib.register_lints.rs

@@ -364,6 +364,7 @@ store.register_lints(&[
     methods::UNNECESSARY_FOLD,
     methods::UNNECESSARY_JOIN,
     methods::UNNECESSARY_LAZY_EVALUATIONS,
+    methods::UNNECESSARY_SORT_BY,
     methods::UNNECESSARY_TO_OWNED,
     methods::UNWRAP_OR_ELSE_DEFAULT,
     methods::UNWRAP_USED,
@@ -567,7 +568,6 @@ store.register_lints(&[
     unnamed_address::VTABLE_ADDRESS_COMPARISONS,
     unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS,
     unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS,
-    unnecessary_sort_by::UNNECESSARY_SORT_BY,
     unnecessary_wraps::UNNECESSARY_WRAPS,
     unnested_or_patterns::UNNESTED_OR_PATTERNS,
     unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,

clippy_lints/src/lib.rs

@@ -376,7 +376,6 @@ mod unit_types;
 mod unnamed_address;
 mod unnecessary_owned_empty_strings;
 mod unnecessary_self_imports;
-mod unnecessary_sort_by;
 mod unnecessary_wraps;
 mod unnested_or_patterns;
 mod unsafe_removed_from_name;
@@ -716,7 +715,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(ptr_offset_with_cast::PtrOffsetWithCast));
     store.register_late_pass(|| Box::new(redundant_clone::RedundantClone));
     store.register_late_pass(|| Box::new(slow_vector_initialization::SlowVectorInit));
-    store.register_late_pass(|| Box::new(unnecessary_sort_by::UnnecessarySortBy));
     store.register_late_pass(move || Box::new(unnecessary_wraps::UnnecessaryWraps::new(avoid_breaking_exported_api)));
     store.register_late_pass(|| Box::new(assertions_on_constants::AssertionsOnConstants));
     store.register_late_pass(|| Box::new(assertions_on_result_states::AssertionsOnResultStates));

clippy_lints/src/methods/mod.rs

@@ -84,6 +84,7 @@ mod unnecessary_fold;
 mod unnecessary_iter_cloned;
 mod unnecessary_join;
 mod unnecessary_lazy_eval;
+mod unnecessary_sort_by;
 mod unnecessary_to_owned;
 mod unwrap_or_else_default;
 mod unwrap_used;
@@ -2872,6 +2873,40 @@ declare_clippy_lint! {
     "hashing a unit value, which does nothing"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects uses of `Vec::sort_by` passing in a closure
+    /// which compares the two arguments, either directly or indirectly.
+    ///
+    /// ### Why is this bad?
+    /// It is more clear to use `Vec::sort_by_key` (or `Vec::sort` if
+    /// possible) than to use `Vec::sort_by` and a more complicated
+    /// closure.
+    ///
+    /// ### Known problems
+    /// If the suggested `Vec::sort_by_key` uses Reverse and it isn't already
+    /// imported by a use statement, then it will need to be added manually.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # struct A;
+    /// # impl A { fn foo(&self) {} }
+    /// # let mut vec: Vec<A> = Vec::new();
+    /// vec.sort_by(|a, b| a.foo().cmp(&b.foo()));
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # struct A;
+    /// # impl A { fn foo(&self) {} }
+    /// # let mut vec: Vec<A> = Vec::new();
+    /// vec.sort_by_key(|a| a.foo());
+    /// ```
+    #[clippy::version = "1.46.0"]
+    pub UNNECESSARY_SORT_BY,
+    complexity,
+    "Use of `Vec::sort_by` when `Vec::sort_by_key` or `Vec::sort` would be clearer"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Option<RustcVersion>,
@@ -2990,6 +3025,7 @@ impl_lint_pass!(Methods => [
     REPEAT_ONCE,
     STABLE_SORT_PRIMITIVE,
     UNIT_HASH,
+    UNNECESSARY_SORT_BY,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3387,6 +3423,12 @@ impl Methods {
                 ("sort", []) => {
                     stable_sort_primitive::check(cx, expr, recv);
                 },
+                ("sort_by", [arg]) => {
+                    unnecessary_sort_by::check(cx, expr, recv, arg, false);
+                },
+                ("sort_unstable_by", [arg]) => {
+                    unnecessary_sort_by::check(cx, expr, recv, arg, true);
+                },
                 ("splitn" | "rsplitn", [count_arg, pat_arg]) => {
                     if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
                         suspicious_splitn::check(cx, name, expr, recv, count);

clippy_lints/src/unnecessary_sort_by.rs renamed to clippy_lints/src/methods/unnecessary_sort_by.rs

@@ -1,51 +1,17 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_trait_method;
 use clippy_utils::sugg::Sugg;
-use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
+use clippy_utils::ty::implements_trait;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{Closure, Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_lint::LateContext;
 use rustc_middle::ty::{self, subst::GenericArgKind};
-use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::sym;
 use rustc_span::symbol::Ident;
 use std::iter;
 
-declare_clippy_lint! {
-    /// ### What it does
-    /// Detects uses of `Vec::sort_by` passing in a closure
-    /// which compares the two arguments, either directly or indirectly.
-    ///
-    /// ### Why is this bad?
-    /// It is more clear to use `Vec::sort_by_key` (or `Vec::sort` if
-    /// possible) than to use `Vec::sort_by` and a more complicated
-    /// closure.
-    ///
-    /// ### Known problems
-    /// If the suggested `Vec::sort_by_key` uses Reverse and it isn't already
-    /// imported by a use statement, then it will need to be added manually.
-    ///
-    /// ### Example
-    /// ```rust
-    /// # struct A;
-    /// # impl A { fn foo(&self) {} }
-    /// # let mut vec: Vec<A> = Vec::new();
-    /// vec.sort_by(|a, b| a.foo().cmp(&b.foo()));
-    /// ```
-    /// Use instead:
-    /// ```rust
-    /// # struct A;
-    /// # impl A { fn foo(&self) {} }
-    /// # let mut vec: Vec<A> = Vec::new();
-    /// vec.sort_by_key(|a| a.foo());
-    /// ```
-    #[clippy::version = "1.46.0"]
-    pub UNNECESSARY_SORT_BY,
-    complexity,
-    "Use of `Vec::sort_by` when `Vec::sort_by_key` or `Vec::sort` would be clearer"
-}
-
-declare_lint_pass!(UnnecessarySortBy => [UNNECESSARY_SORT_BY]);
+use super::UNNECESSARY_SORT_BY;
 
 enum LintTrigger {
     Sort(SortDetection),
@@ -54,15 +20,13 @@ enum LintTrigger {
 
 struct SortDetection {
     vec_name: String,
-    unstable: bool,
 }
 
 struct SortByKeyDetection {
     vec_name: String,
     closure_arg: String,
     closure_body: String,
     reverse: bool,
-    unstable: bool,
 }
 
 /// Detect if the two expressions are mirrored (identical, except one
@@ -150,20 +114,20 @@ fn mirrored_exprs(a_expr: &Expr<'_>, a_ident: &Ident, b_expr: &Expr<'_>, b_ident
     }
 }
 
-fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
+fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, arg: &Expr<'_>) -> Option<LintTrigger> {
     if_chain! {
-        if let ExprKind::MethodCall(name_ident, args, _) = &expr.kind;
-        if let name = name_ident.ident.name.to_ident_string();
-        if name == "sort_by" || name == "sort_unstable_by";
-        if let [vec, Expr { kind: ExprKind::Closure(Closure { body: closure_body_id, .. }), .. }] = args;
-        if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(vec), sym::Vec);
-        if let closure_body = cx.tcx.hir().body(*closure_body_id);
+        if let Some(method_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
+        if let Some(impl_id) = cx.tcx.impl_of_method(method_id);
+        if cx.tcx.type_of(impl_id).is_slice();
+        if let ExprKind::Closure(&Closure { body, .. }) = arg.kind;
+        if let closure_body = cx.tcx.hir().body(body);
         if let &[
             Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..},
             Param { pat: Pat { kind: PatKind::Binding(_, _, right_ident, _), .. }, .. }
         ] = &closure_body.params;
-        if let ExprKind::MethodCall(method_path, [ref left_expr, ref right_expr], _) = &closure_body.value.kind;
+        if let ExprKind::MethodCall(method_path, [left_expr, right_expr], _) = closure_body.value.kind;
         if method_path.ident.name == sym::cmp;
+        if is_trait_method(cx, &closure_body.value, sym::Ord);
         then {
             let (closure_body, closure_arg, reverse) = if mirrored_exprs(
                 left_expr,
@@ -177,19 +141,18 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
             } else {
                 return None;
             };
-            let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
-            let unstable = name == "sort_unstable_by";
+            let vec_name = Sugg::hir(cx, recv, "..").to_string();
 
             if_chain! {
-            if let ExprKind::Path(QPath::Resolved(_, Path {
-                segments: [PathSegment { ident: left_name, .. }], ..
-            })) = &left_expr.kind;
-            if left_name == left_ident;
-            if cx.tcx.get_diagnostic_item(sym::Ord).map_or(false, |id| {
-                implements_trait(cx, cx.typeck_results().expr_ty(left_expr), id, &[])
-            });
+                if let ExprKind::Path(QPath::Resolved(_, Path {
+                    segments: [PathSegment { ident: left_name, .. }], ..
+                })) = &left_expr.kind;
+                if left_name == left_ident;
+                if cx.tcx.get_diagnostic_item(sym::Ord).map_or(false, |id| {
+                    implements_trait(cx, cx.typeck_results().expr_ty(left_expr), id, &[])
+                });
                 then {
-                    return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }));
+                    return Some(LintTrigger::Sort(SortDetection { vec_name }));
                 }
             }
 
@@ -199,7 +162,6 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
                     closure_arg,
                     closure_body,
                     reverse,
-                    unstable,
                 }));
             }
         }
@@ -213,46 +175,50 @@ fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     matches!(ty.kind(), ty::Ref(..)) || ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)))
 }
 
-impl LateLintPass<'_> for UnnecessarySortBy {
-    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
-        match detect_lint(cx, expr) {
-            Some(LintTrigger::SortByKey(trigger)) => span_lint_and_sugg(
-                cx,
-                UNNECESSARY_SORT_BY,
-                expr.span,
-                "use Vec::sort_by_key here instead",
-                "try",
-                format!(
-                    "{}.sort{}_by_key(|{}| {})",
-                    trigger.vec_name,
-                    if trigger.unstable { "_unstable" } else { "" },
-                    trigger.closure_arg,
-                    if trigger.reverse {
-                        format!("std::cmp::Reverse({})", trigger.closure_body)
-                    } else {
-                        trigger.closure_body.to_string()
-                    },
-                ),
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'_>,
+    recv: &'tcx Expr<'_>,
+    arg: &'tcx Expr<'_>,
+    is_unstable: bool,
+) {
+    match detect_lint(cx, expr, recv, arg) {
+        Some(LintTrigger::SortByKey(trigger)) => span_lint_and_sugg(
+            cx,
+            UNNECESSARY_SORT_BY,
+            expr.span,
+            "use Vec::sort_by_key here instead",
+            "try",
+            format!(
+                "{}.sort{}_by_key(|{}| {})",
+                trigger.vec_name,
+                if is_unstable { "_unstable" } else { "" },
+                trigger.closure_arg,
                 if trigger.reverse {
-                    Applicability::MaybeIncorrect
+                    format!("std::cmp::Reverse({})", trigger.closure_body)
                 } else {
-                    Applicability::MachineApplicable
+                    trigger.closure_body.to_string()
                 },
             ),
-            Some(LintTrigger::Sort(trigger)) => span_lint_and_sugg(
-                cx,
-                UNNECESSARY_SORT_BY,
-                expr.span,
-                "use Vec::sort here instead",
-                "try",
-                format!(
-                    "{}.sort{}()",
-                    trigger.vec_name,
-                    if trigger.unstable { "_unstable" } else { "" },
-                ),
-                Applicability::MachineApplicable,
+            if trigger.reverse {
+                Applicability::MaybeIncorrect
+            } else {
+                Applicability::MachineApplicable
+            },
+        ),
+        Some(LintTrigger::Sort(trigger)) => span_lint_and_sugg(
+            cx,
+            UNNECESSARY_SORT_BY,
+            expr.span,
+            "use Vec::sort here instead",
+            "try",
+            format!(
+                "{}.sort{}()",
+                trigger.vec_name,
+                if is_unstable { "_unstable" } else { "" },
             ),
-            None => {},
-        }
+            Applicability::MachineApplicable,
+        ),
+        None => {},
     }
 }