Implement suggestions from phansch

4 files changed, 57 insertions, 24 deletions

diff --git a/clippy_lints/src/unnecessary_sort_by.rs b/clippy_lints/src/unnecessary_sort_by.rs
index c0858ec4c88..33d8331c292 100644
--- a/clippy_lints/src/unnecessary_sort_by.rs
+++ b/clippy_lints/src/unnecessary_sort_by.rs
@@ -18,15 +18,25 @@ declare_clippy_lint! {
     /// possible) than to use `Vec::sort_by` and and a more complicated
     /// closure.
     ///
-    /// **Known problems:** None.
+    /// **Known problems:**
+    /// If the suggested `Vec::sort_by_key` uses Reverse and it isn't
+    /// imported by a use statement in the current frame, then a `use`
+    /// statement that imports it will need to be added (which this lint
+    /// can't do).
     ///
     /// **Example:**
     ///
     /// ```rust
-    /// vec.sort_by(|a, b| a.foo().cmp(b.foo()));
+    /// # 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());
     /// ```
     pub UNNECESSARY_SORT_BY,
@@ -50,6 +60,7 @@ struct SortByKeyDetection {
     vec_name: String,
     closure_arg: String,
     closure_body: String,
+    reverse: bool,
     unstable: bool,
 }
 
@@ -172,16 +183,16 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
         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";
         then {
-            let (closure_body, closure_arg) = if mirrored_exprs(
+            let (closure_body, closure_arg, reverse) = if mirrored_exprs(
                 &cx,
                 &left_expr,
                 &left_ident,
                 &right_expr,
                 &right_ident
             ) {
-                (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string())
+                (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string(), false)
             } 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())
+                (Sugg::hir(cx, &left_expr, "..").to_string(), right_ident.name.to_string(), true)
             } else {
                 return None;
             };
@@ -196,7 +207,13 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
                     Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
                 }
                 else {
-                    Some(LintTrigger::SortByKey(SortByKeyDetection { vec_name, unstable, closure_arg, closure_body }))
+                    Some(LintTrigger::SortByKey(SortByKeyDetection {
+                        vec_name,
+                        unstable,
+                        closure_arg,
+                        closure_body,
+                        reverse
+                    }))
                 }
             }
         } else {
@@ -219,9 +236,17 @@ impl LateLintPass<'_, '_> for UnnecessarySortBy {
                     trigger.vec_name,
                     if trigger.unstable { "_unstable" } else { "" },
                     trigger.closure_arg,
-                    trigger.closure_body,
+                    if trigger.reverse {
+                        format!("Reverse({})", trigger.closure_body)
+                    } else {
+                        trigger.closure_body.to_string()
+                    },
                 ),
-                Applicability::MachineApplicable,
+                if trigger.reverse {
+                    Applicability::MaybeIncorrect
+                } else {
+                    Applicability::MachineApplicable
+                },
             ),
             Some(LintTrigger::Sort(trigger)) => utils::span_lint_and_sugg(
                 cx,
diff --git a/tests/ui/unnecessary_sort_by.fixed b/tests/ui/unnecessary_sort_by.fixed
index 4521ae38d49..779fd57707a 100644
--- a/tests/ui/unnecessary_sort_by.fixed
+++ b/tests/ui/unnecessary_sort_by.fixed
@@ -10,16 +10,17 @@ fn main() {
     let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
     // Forward examples
     vec.sort();
+    vec.sort_unstable();
     vec.sort_by_key(|&a| (a + 5).abs());
-    vec.sort_by_key(|&a| id(-a));
+    vec.sort_unstable_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)));
+    vec.sort_unstable_by_key(|&b| Reverse(id(-b)));
     // Negative examples (shouldn't be changed)
     let c = &7;
     vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
     vec.sort_by(|_, b| b.cmp(&5));
     vec.sort_by(|_, b| b.cmp(c));
-    vec.sort_by(|a, _| a.cmp(c));
+    vec.sort_unstable_by(|a, _| a.cmp(c));
 }
diff --git a/tests/ui/unnecessary_sort_by.rs b/tests/ui/unnecessary_sort_by.rs
index fdb5a823369..0485a5630af 100644
--- a/tests/ui/unnecessary_sort_by.rs
+++ b/tests/ui/unnecessary_sort_by.rs
@@ -10,16 +10,17 @@ 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_unstable_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)));
+    vec.sort_unstable_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)));
+    vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
     // Negative examples (shouldn't be changed)
     let c = &7;
     vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
     vec.sort_by(|_, b| b.cmp(&5));
     vec.sort_by(|_, b| b.cmp(c));
-    vec.sort_by(|a, _| a.cmp(c));
+    vec.sort_unstable_by(|a, _| a.cmp(c));
 }
diff --git a/tests/ui/unnecessary_sort_by.stderr b/tests/ui/unnecessary_sort_by.stderr
index b6365c1709d..903b6e5099c 100644
--- a/tests/ui/unnecessary_sort_by.stderr
+++ b/tests/ui/unnecessary_sort_by.stderr
@@ -6,35 +6,41 @@ LL |     vec.sort_by(|a, b| a.cmp(b));
    |
    = note: `-D clippy::unnecessary-sort-by` implied by `-D warnings`
 
-error: use Vec::sort_by_key here instead
+error: use Vec::sort here instead
   --> $DIR/unnecessary_sort_by.rs:13:5
    |
+LL |     vec.sort_unstable_by(|a, b| a.cmp(b));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.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/unnecessary_sort_by.rs:14:5
+  --> $DIR/unnecessary_sort_by.rs:15:5
    |
-LL |     vec.sort_by(|a, b| id(-a).cmp(&id(-b)));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| id(-a))`
+LL |     vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&a| id(-a))`
 
 error: use Vec::sort_by_key here instead
-  --> $DIR/unnecessary_sort_by.rs:16:5
+  --> $DIR/unnecessary_sort_by.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/unnecessary_sort_by.rs:17:5
+  --> $DIR/unnecessary_sort_by.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/unnecessary_sort_by.rs:18:5
+  --> $DIR/unnecessary_sort_by.rs:19:5
    |
-LL |     vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))`
+LL |     vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&b| Reverse(id(-b)))`
 
-error: aborting due to 6 previous errors
+error: aborting due to 7 previous errors