Auto merge of #6006 - ebroto:6001_unnecessary_sort_by, r=Manishearth

Restrict unnecessary_sort_by to non-reference, Copy types

`Vec::sort_by_key` closure parameter is `F: FnMut(&T) -> K`. The lint's suggestion destructures the `T` parameter; this was probably done to avoid different unnamed lifetimes when `K = Reverse<&T>`.

This change fixes two issues:
* Destructuring T when T is non-reference requires the type to be Copy, otherwise we would try to move from a shared reference. We make sure `T: Copy` holds.
* Make sure `T` is actually non-reference. I didn't go for destructuring multiple levels of references, as we would have to compensate in the closure body by removing derefs and maybe adding parens, which would add more complexity.

changelog: Restrict [`unnecessary_sort_by`] to non-reference, Copy types

Fixes #6001

clippy_lints/src/unnecessary_sort_by.rs

@@ -1,5 +1,4 @@
 use crate::utils;
-use crate::utils::paths;
 use crate::utils::sugg::Sugg;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -171,12 +170,22 @@ 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;
-        if utils::match_type(cx, &cx.typeck_results().expr_ty(vec), &paths::VEC);
+        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 let closure_body = cx.tcx.hir().body(*closure_body_id);
         if let &[
             Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..},

tests/ui/unnecessary_sort_by.fixed

@@ -25,17 +25,25 @@ fn unnecessary_sort_by() {
     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
+    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(|a, b| b.cmp(a));
+    vec.sort_unstable_by(|a, b| b.cmp(a));
 }
 
-// Should not be linted to avoid hitting https://github.com/rust-lang/rust/issues/34162
+// Do not suggest returning a reference to the closure parameter of `Vec::sort_by_key`
 mod issue_5754 {
-    struct Test(String);
+    #[derive(Clone, Copy)]
+    struct Test(usize);
 
     #[derive(PartialOrd, Ord, PartialEq, Eq)]
-    struct Wrapper<'a>(&'a str);
+    struct Wrapper<'a>(&'a usize);
 
     impl Test {
-        fn name(&self) -> &str {
+        fn name(&self) -> &usize {
             &self.0
         }
 
@@ -60,7 +68,33 @@ 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.
+mod issue_6001 {
+    struct Test(String);
+
+    impl Test {
+        // Return an owned type so that we don't hit the fix for 5754
+        fn name(&self) -> String {
+            self.0.clone()
+        }
+    }
+
+    pub fn test() {
+        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()));
+        // Reverse
+        args.sort_by(|a, b| b.name().cmp(&a.name()));
+        args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
+    }
+}
+
 fn main() {
     unnecessary_sort_by();
     issue_5754::test();
+    issue_6001::test();
 }

tests/ui/unnecessary_sort_by.rs

@@ -25,17 +25,25 @@ fn unnecessary_sort_by() {
     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
+    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(|a, b| b.cmp(a));
+    vec.sort_unstable_by(|a, b| b.cmp(a));
 }
 
-// Should not be linted to avoid hitting https://github.com/rust-lang/rust/issues/34162
+// Do not suggest returning a reference to the closure parameter of `Vec::sort_by_key`
 mod issue_5754 {
-    struct Test(String);
+    #[derive(Clone, Copy)]
+    struct Test(usize);
 
     #[derive(PartialOrd, Ord, PartialEq, Eq)]
-    struct Wrapper<'a>(&'a str);
+    struct Wrapper<'a>(&'a usize);
 
     impl Test {
-        fn name(&self) -> &str {
+        fn name(&self) -> &usize {
             &self.0
         }
 
@@ -60,7 +68,33 @@ 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.
+mod issue_6001 {
+    struct Test(String);
+
+    impl Test {
+        // Return an owned type so that we don't hit the fix for 5754
+        fn name(&self) -> String {
+            self.0.clone()
+        }
+    }
+
+    pub fn test() {
+        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()));
+        // Reverse
+        args.sort_by(|a, b| b.name().cmp(&a.name()));
+        args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
+    }
+}
+
 fn main() {
     unnecessary_sort_by();
     issue_5754::test();
+    issue_6001::test();
 }