Detect also a non-reversed comparison

JarredAllen · JarredAllen · commit 059e8edd1540 · 2020-05-31T11:55:45.000-07:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -304,7 +304,7 @@ mod serde_api;
 mod shadow;
 mod single_component_path_imports;
 mod slow_vector_initialization;
-mod sort_by_key_reverse;
+mod sort_by_key;
 mod strings;
 mod suspicious_trait_impl;
 mod swap;
@@ -780,7 +780,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &shadow::SHADOW_UNRELATED,
         &single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
         &slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,
-        &sort_by_key_reverse::SORT_BY_KEY_REVERSE,
+        &sort_by_key::SORT_BY_KEY,
         &strings::STRING_ADD,
         &strings::STRING_ADD_ASSIGN,
         &strings::STRING_LIT_AS_BYTES,
@@ -998,7 +998,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box ptr_offset_with_cast::PtrOffsetWithCast);
     store.register_late_pass(|| box redundant_clone::RedundantClone);
     store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit);
-    store.register_late_pass(|| box sort_by_key_reverse::SortByKeyReverse);
+    store.register_late_pass(|| box sort_by_key::SortByKey);
     store.register_late_pass(|| box types::RefToMut);
     store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants);
     store.register_late_pass(|| box missing_const_for_fn::MissingConstForFn);
@@ -1394,7 +1394,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&serde_api::SERDE_API_MISUSE),
         LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
         LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
-        LintId::of(&sort_by_key_reverse::SORT_BY_KEY_REVERSE),
+        LintId::of(&sort_by_key::SORT_BY_KEY),
         LintId::of(&strings::STRING_LIT_AS_BYTES),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
@@ -1596,7 +1596,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
         LintId::of(&reference::DEREF_ADDROF),
         LintId::of(&reference::REF_IN_DEREF),
-        LintId::of(&sort_by_key_reverse::SORT_BY_KEY_REVERSE),
+        LintId::of(&sort_by_key::SORT_BY_KEY),
         LintId::of(&swap::MANUAL_SWAP),
         LintId::of(&temporary_assignment::TEMPORARY_ASSIGNMENT),
         LintId::of(&transmute::CROSSPOINTER_TRANSMUTE),
diff --git a/clippy_lints/src/sort_by_key.rs b/clippy_lints/src/sort_by_key.rs
@@ -11,33 +11,35 @@ use rustc_span::symbol::Ident;
 declare_clippy_lint! {
     /// **What it does:**
     /// Detects when people use `Vec::sort_by` and pass in a function
-    /// which compares the second argument to the first.
+    /// which compares the two arguments, either directly or indirectly.
     ///
     /// **Why is this bad?**
-    /// It is more clear to use `Vec::sort_by_key` and `std::cmp::Reverse`
+    /// It is more clear to use `Vec::sort_by_key` (or
+    /// `Vec::sort_by_key` and `std::cmp::Reverse` if necessary) than
+    /// using 
     ///
     /// **Known problems:** None.
     ///
     /// **Example:**
     ///
     /// ```rust
-    /// vec.sort_by(|a, b| b.foo().cmp(&a.foo()));
+    /// vec.sort_by(|a, b| a.foo().cmp(b.foo()));
     /// ```
     /// Use instead:
     /// ```rust
-    /// vec.sort_by_key(|e| Reverse(e.foo()));
+    /// vec.sort_by_key(|a| a.foo());
     /// ```
-    pub SORT_BY_KEY_REVERSE,
+    pub SORT_BY_KEY,
     complexity,
     "Use of `Vec::sort_by` when `Vec::sort_by_key` would be clearer"
 }
 
-declare_lint_pass!(SortByKeyReverse => [SORT_BY_KEY_REVERSE]);
+declare_lint_pass!(SortByKey => [SORT_BY_KEY]);
 
 struct LintTrigger {
     vec_name: String,
     closure_arg: String,
-    closure_reverse_body: String,
+    closure_body: String,
     unstable: bool,
 }
 
@@ -154,43 +156,49 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
         if utils::match_type(cx, &cx.tables.expr_ty(vec), &paths::VEC);
         if let closure_body = cx.tcx.hir().body(*closure_body_id);
         if let &[
-            Param { pat: Pat { kind: PatKind::Binding(_, _, a_ident, _), .. }, ..},
-            Param { pat: Pat { kind: PatKind::Binding(_, _, b_ident, _), .. }, .. }
+            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 b_expr, ref a_expr]) = &closure_body.value.kind;
+        if let ExprKind::MethodCall(method_path, _, [ref left_expr, ref right_expr]) = &closure_body.value.kind;
         if method_path.ident.name.to_ident_string() == "cmp";
-        if mirrored_exprs(&cx, &a_expr, &a_ident, &b_expr, &b_ident);
         then {
+            let (closure_body, closure_arg) = if mirrored_exprs(
+                &cx,
+                &left_expr,
+                &left_ident,
+                &right_expr,
+                &right_ident
+            ) {
+                (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string())
+            } else if mirrored_exprs(&cx, &left_expr, &right_ident, &right_expr, &left_ident) {
+                (format!("Reverse({})", Sugg::hir(cx, &left_expr, "..").to_string()), right_ident.name.to_string())
+            } else {
+                return None;
+            };
             let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
             let unstable = name == "sort_unstable_by";
-            let closure_arg = format!("&{}", b_ident.name.to_ident_string());
-            let closure_reverse_body = Sugg::hir(cx, &b_expr, "..").to_string();
-            // Get rid of parentheses, because they aren't needed anymore
-            // while closure_reverse_body.chars().next() == Some('(') && closure_reverse_body.chars().last() == Some(')') {
-                // closure_reverse_body = String::from(&closure_reverse_body[1..closure_reverse_body.len()-1]);
-            // }
-            Some(LintTrigger { vec_name, unstable, closure_arg, closure_reverse_body })
+            Some(LintTrigger { vec_name, unstable, closure_arg, closure_body })
         } else {
             None
         }
     }
 }
 
-impl LateLintPass<'_, '_> for SortByKeyReverse {
+impl LateLintPass<'_, '_> for SortByKey {
     fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
         if let Some(trigger) = detect_lint(cx, expr) {
             utils::span_lint_and_sugg(
                 cx,
-                SORT_BY_KEY_REVERSE,
+                SORT_BY_KEY,
                 expr.span,
                 "use Vec::sort_by_key here instead",
                 "try",
                 format!(
-                    "{}.sort{}_by_key(|{}| Reverse({}))",
+                    "{}.sort{}_by_key(|&{}| {})",
                     trigger.vec_name,
                     if trigger.unstable { "_unstable" } else { "" },
                     trigger.closure_arg,
-                    trigger.closure_reverse_body,
+                    trigger.closure_body,
                 ),
                 Applicability::MachineApplicable,
             );
diff --git a/tests/ui/sort_by_key.fixed b/tests/ui/sort_by_key.fixed
@@ -1,5 +1,5 @@
 // run-rustfix
-#![warn(clippy::sort_by_key_reverse)]
+#![warn(clippy::sort_by_key)]
 
 use std::cmp::Reverse;
 
@@ -9,6 +9,11 @@ fn id(x: isize) -> isize {
 
 fn main() {
     let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
+    // Forward examples
+    vec.sort_by_key(|&a| a);
+    vec.sort_by_key(|&a| (a + 5).abs());
+    vec.sort_by_key(|&a| id(-a));
+    // Reverse examples
     vec.sort_by_key(|&b| Reverse(b));
     vec.sort_by_key(|&b| Reverse((b + 5).abs()));
     vec.sort_by_key(|&b| Reverse(id(-b)));
@@ -18,5 +23,4 @@ fn main() {
     vec.sort_by(|_, b| b.cmp(&5));
     vec.sort_by(|_, b| b.cmp(c));
     vec.sort_by(|a, _| a.cmp(c));
-    vec.sort_by(|a, b| a.cmp(b));
 }
diff --git a/tests/ui/sort_by_key.rs b/tests/ui/sort_by_key.rs
@@ -9,6 +9,11 @@ fn id(x: isize) -> isize {
 
 fn main() {
     let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
+    // Forward examples
+    vec.sort_by(|a, b| a.cmp(b));
+    vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
+    vec.sort_by(|a, b| id(-a).cmp(&id(-b)));
+    // Reverse examples
     vec.sort_by(|a, b| b.cmp(a));
     vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
     vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
@@ -18,5 +23,4 @@ fn main() {
     vec.sort_by(|_, b| b.cmp(&5));
     vec.sort_by(|_, b| b.cmp(c));
     vec.sort_by(|a, _| a.cmp(c));
-    vec.sort_by(|a, b| a.cmp(b));
 }
diff --git a/tests/ui/sort_by_key.stderr b/tests/ui/sort_by_key.stderr
@@ -0,0 +1,48 @@
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:13:5
+   |
+LL |     vec.sort_by(|a, b| a.cmp(b));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| a)`
+   |
+   = note: `-D clippy::sort-by-key` implied by `-D warnings`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:14:5
+   |
+LL |     vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| (a + 5).abs())`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:15:5
+   |
+LL |     vec.sort_by(|a, b| id(-a).cmp(&id(-b)));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| id(-a))`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:17:5
+   |
+LL |     vec.sort_by(|a, b| b.cmp(a));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(b))`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:18:5
+   |
+LL |     vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/sort_by_key.rs:19:5
+   |
+LL |     vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))`
+
+error: unknown clippy lint: clippy::sort_by_key_reverse
+  --> $DIR/sort_by_key.rs:2:9
+   |
+LL | #![warn(clippy::sort_by_key_reverse)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: did you mean: `clippy::sort_by_key`
+   |
+   = note: `-D clippy::unknown-clippy-lints` implied by `-D warnings`
+
+error: aborting due to 7 previous errors
+
diff --git a/tests/ui/sort_by_key_reverse.stderr b/tests/ui/sort_by_key_reverse.stderr
