Auto merge of rust-lang#6078 - ebroto:unnecessary_sort_by_take_2, r=p…

…hansch

unnecessary sort by: avoid dereferencing the suggested closure parameter

This change tries to simplify the solution for problematic cases but is less restrictive than  rust-lang#6006.

* We can't dereference shared references to non-Copy types, so the new suggestion does not do that. Note that this implies that the suggested closure parameter will be a reference.
* We can't take a reference to the closure parameter in the returned key, so we don't lint in those cases. This can happen either because the key borrows from the parameter (e.g. `|a| a.borrows()`), or because we suggest `|a| Reverse(a)`. If we did we would hit this error:
```
error: lifetime may not live long enough
  --> /home/ebroto/src/ebroto-clippy/tests/ui/unnecessary_sort_by.fixed:19:25
   |
19 |     vec.sort_by_key(|b| Reverse(b));
   |                      -- ^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
   |                      ||
   |                      |return type of closure is Reverse<&'2 isize>
   |                      has type `&'1 isize`

error: aborting due to previous error
```

Note that Clippy does not currently have the (MIR-based) machinery necessary to check that what is borrowed is actually the closure parameter.

changelog: [`unnecessary_sort_by`]: avoid dereferencing the suggested closure parameter

Fixes rust-lang#6001

clippy_lints/src/unnecessary_sort_by.rs

@@ -170,22 +170,12 @@ fn mirrored_exprs(
 }
 
 fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
-    // NOTE: Vectors of references are not supported. In order to avoid hitting https://github.com/rust-lang/rust/issues/34162,
-    // (different unnamed lifetimes for closure arg and return type) we need to make sure the suggested
-    // closure parameter is not a reference in case we suggest `Reverse`. Trying to destructure more
-    // than one level of references would add some extra complexity as we would have to compensate
-    // in the closure body.
-
     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_id, _, _), .. }] = args;
-        let vec_ty = cx.typeck_results().expr_ty(vec);
-        if utils::is_type_diagnostic_item(cx, vec_ty, sym!(vec_type));
-        let ty = vec_ty.walk().nth(1).unwrap().expect_ty(); // T in Vec<T>
-        if !matches!(&ty.kind(), ty::Ref(..));
-        if utils::is_copy(cx, ty);
+        if utils::is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(vec), sym!(vec_type));
         if let closure_body = cx.tcx.hir().body(*closure_body_id);
         if let &[
             Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..},
@@ -210,40 +200,32 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
             let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
             let unstable = name == "sort_unstable_by";
 
-            if_chain! {
-                if let ExprKind::Path(QPath::Resolved(_, Path {
-                    segments: [PathSegment { ident: left_name, .. }], ..
-                })) = &left_expr.kind;
-                if left_name == left_ident;
-                then {
-                    return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
-                } else {
-                    if !key_returns_borrow(cx, left_expr) {
-                        return Some(LintTrigger::SortByKey(SortByKeyDetection {
-                            vec_name,
-                            unstable,
-                            closure_arg,
-                            closure_body,
-                            reverse
-                        }))
-                    }
+            if let ExprKind::Path(QPath::Resolved(_, Path {
+                segments: [PathSegment { ident: left_name, .. }], ..
+            })) = &left_expr.kind {
+                if left_name == left_ident {
+                    return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }));
                 }
             }
+
+            if !expr_borrows(cx, left_expr) {
+                return Some(LintTrigger::SortByKey(SortByKeyDetection {
+                    vec_name,
+                    unstable,
+                    closure_arg,
+                    closure_body,
+                    reverse
+                }));
+            }
         }
     }
 
     None
 }
 
-fn key_returns_borrow(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if let Some(def_id) = utils::fn_def_id(cx, expr) {
-        let output = cx.tcx.fn_sig(def_id).output();
-        let ty = output.skip_binder();
-        return matches!(ty.kind(), ty::Ref(..))
-            || ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)));
-    }
-
-    false
+fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let ty = cx.typeck_results().expr_ty(expr);
+    matches!(ty.kind(), ty::Ref(..)) || ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)))
 }
 
 impl LateLintPass<'_> for UnnecessarySortBy {
@@ -256,7 +238,7 @@ impl LateLintPass<'_> for UnnecessarySortBy {
                 "use Vec::sort_by_key here instead",
                 "try",
                 format!(
-                    "{}.sort{}_by_key(|&{}| {})",
+                    "{}.sort{}_by_key(|{}| {})",
                     trigger.vec_name,
                     if trigger.unstable { "_unstable" } else { "" },
                     trigger.closure_arg,

tests/ui/unnecessary_sort_by.fixed

@@ -13,23 +13,24 @@ fn unnecessary_sort_by() {
     // Forward examples
     vec.sort();
     vec.sort_unstable();
-    vec.sort_by_key(|&a| (a + 5).abs());
-    vec.sort_unstable_by_key(|&a| id(-a));
+    vec.sort_by_key(|a| (a + 5).abs());
+    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_unstable_by_key(|&b| Reverse(id(-b)));
+    vec.sort_by(|a, b| b.cmp(a)); // not linted to avoid suggesting `Reverse(b)` which would borrow
+    vec.sort_by_key(|b| Reverse((b + 5).abs()));
+    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_unstable_by(|a, _| a.cmp(c));
 
-    // Ignore vectors of references
+    // Vectors of references are fine as long as the resulting key does not borrow
     let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
-    vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
-    vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
+    vec.sort_by_key(|a| (***a).abs());
+    vec.sort_unstable_by_key(|a| (***a).abs());
+    // `Reverse(b)` would borrow in the following cases, don't lint
     vec.sort_by(|a, b| b.cmp(a));
     vec.sort_unstable_by(|a, b| b.cmp(a));
 }
@@ -68,10 +69,9 @@ mod issue_5754 {
     }
 }
 
-// `Vec::sort_by_key` closure parameter is `F: FnMut(&T) -> K`
-// The suggestion is destructuring T and we know T is not a reference, so test that non-Copy T are
-// not linted.
+// The closure parameter is not dereferenced anymore, so non-Copy types can be linted
 mod issue_6001 {
+    use super::*;
     struct Test(String);
 
     impl Test {
@@ -85,11 +85,11 @@ mod issue_6001 {
         let mut args: Vec<Test> = vec![];
 
         // Forward
-        args.sort_by(|a, b| a.name().cmp(&b.name()));
-        args.sort_unstable_by(|a, b| a.name().cmp(&b.name()));
+        args.sort_by_key(|a| a.name());
+        args.sort_unstable_by_key(|a| a.name());
         // Reverse
-        args.sort_by(|a, b| b.name().cmp(&a.name()));
-        args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
+        args.sort_by_key(|b| Reverse(b.name()));
+        args.sort_unstable_by_key(|b| Reverse(b.name()));
     }
 }
 

tests/ui/unnecessary_sort_by.rs

@@ -16,7 +16,7 @@ fn unnecessary_sort_by() {
     vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
     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.cmp(a)); // not linted to avoid suggesting `Reverse(b)` which would borrow
     vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
     vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
     // Negative examples (shouldn't be changed)
@@ -26,10 +26,11 @@ fn unnecessary_sort_by() {
     vec.sort_by(|_, b| b.cmp(c));
     vec.sort_unstable_by(|a, _| a.cmp(c));
 
-    // Ignore vectors of references
+    // Vectors of references are fine as long as the resulting key does not borrow
     let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
     vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
     vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
+    // `Reverse(b)` would borrow in the following cases, don't lint
     vec.sort_by(|a, b| b.cmp(a));
     vec.sort_unstable_by(|a, b| b.cmp(a));
 }
@@ -68,10 +69,9 @@ mod issue_5754 {
     }
 }
 
-// `Vec::sort_by_key` closure parameter is `F: FnMut(&T) -> K`
-// The suggestion is destructuring T and we know T is not a reference, so test that non-Copy T are
-// not linted.
+// The closure parameter is not dereferenced anymore, so non-Copy types can be linted
 mod issue_6001 {
+    use super::*;
     struct Test(String);
 
     impl Test {

tests/ui/unnecessary_sort_by.stderr

@@ -16,31 +16,61 @@ error: use Vec::sort_by_key here instead
   --> $DIR/unnecessary_sort_by.rs:16:5
    |
 LL |     vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| (a + 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:17:5
    |
 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:19:5
-   |
-LL |     vec.sort_by(|a, b| b.cmp(a));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(b))`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|a| id(-a))`
 
 error: use Vec::sort_by_key here instead
   --> $DIR/unnecessary_sort_by.rs:20: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()))`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|b| Reverse((b + 5).abs()))`
 
 error: use Vec::sort_by_key here instead
   --> $DIR/unnecessary_sort_by.rs:21:5
    |
 LL |     vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&b| Reverse(id(-b)))`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|b| Reverse(id(-b)))`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:31:5
+   |
+LL |     vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| (***a).abs())`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:32:5
+   |
+LL |     vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|a| (***a).abs())`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:88:9
+   |
+LL |         args.sort_by(|a, b| a.name().cmp(&b.name()));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_by_key(|a| a.name())`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:89:9
+   |
+LL |         args.sort_unstable_by(|a, b| a.name().cmp(&b.name()));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_unstable_by_key(|a| a.name())`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:91:9
+   |
+LL |         args.sort_by(|a, b| b.name().cmp(&a.name()));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_by_key(|b| Reverse(b.name()))`
+
+error: use Vec::sort_by_key here instead
+  --> $DIR/unnecessary_sort_by.rs:92:9
+   |
+LL |         args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_unstable_by_key(|b| Reverse(b.name()))`
 
-error: aborting due to 7 previous errors
+error: aborting due to 12 previous errors