diff --git a/clippy_lints/src/methods/unnecessary_sort_by.rs b/clippy_lints/src/methods/unnecessary_sort_by.rs index 9dddbe814317..c7339a3bdd05 100644 --- a/clippy_lints/src/methods/unnecessary_sort_by.rs +++ b/clippy_lints/src/methods/unnecessary_sort_by.rs @@ -2,12 +2,11 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::res::{MaybeDef, MaybeTypeckRes}; use clippy_utils::std_or_core; use clippy_utils::sugg::Sugg; -use clippy_utils::ty::implements_trait; +use clippy_utils::ty::{implements_trait, is_copy}; use rustc_errors::Applicability; use rustc_hir::{Closure, Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath}; use rustc_lint::LateContext; -use rustc_middle::ty; -use rustc_middle::ty::GenericArgKind; +use rustc_middle::ty::{self, GenericArgKind, Ty}; use rustc_span::sym; use rustc_span::symbol::{Ident, Symbol}; use std::iter; @@ -70,7 +69,7 @@ fn mirrored_exprs(a_expr: &Expr<'_>, a_ident: Ident, b_expr: &Expr<'_>, b_ident: mirrored_exprs(left_block, a_ident, right_block, b_ident) }, (ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident)) => { - left_ident.name == right_ident.name && mirrored_exprs(left_expr, a_ident, right_expr, right_ident) + left_ident.name == right_ident.name && mirrored_exprs(left_expr, a_ident, right_expr, b_ident) }, // Two paths: either one is a and the other is b, or they're identical to each other ( @@ -159,7 +158,11 @@ fn detect_lint<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, arg: &Expr<'_>) -> return Some(LintTrigger::Sort); } - if !expr_borrows(cx, left_expr) { + let left_expr_ty = cx.typeck_results().expr_ty(left_expr); + if !expr_borrows(left_expr_ty) + // Don't lint if the closure is accessing non-Copy fields + && (!expr_is_field_access(left_expr) || is_copy(cx, left_expr_ty)) + { return Some(LintTrigger::SortByKey(SortByKeyDetection { closure_arg, closure_body, @@ -171,11 +174,18 @@ fn detect_lint<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, arg: &Expr<'_>) -> None } -fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - let ty = cx.typeck_results().expr_ty(expr); +fn expr_borrows(ty: Ty<'_>) -> bool { matches!(ty.kind(), ty::Ref(..)) || ty.walk().any(|arg| matches!(arg.kind(), GenericArgKind::Lifetime(_))) } +fn expr_is_field_access(expr: &Expr<'_>) -> bool { + match expr.kind { + ExprKind::Field(_, _) => true, + ExprKind::AddrOf(_, Mutability::Not, inner) => expr_is_field_access(inner), + _ => false, + } +} + pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, diff --git a/tests/ui/unnecessary_sort_by.fixed b/tests/ui/unnecessary_sort_by.fixed index 5255ab173efb..372cf683f223 100644 --- a/tests/ui/unnecessary_sort_by.fixed +++ b/tests/ui/unnecessary_sort_by.fixed @@ -111,3 +111,33 @@ fn main() { issue_5754::test(); issue_6001::test(); } + +fn issue16405() { + let mut v: Vec<(i32, &str)> = vec![(1, "foo"), (2, "bar")]; + + v.sort_by_key(|a| a.0); + //~^ unnecessary_sort_by + + struct Item { + key: i32, + value: String, + } + + let mut items = vec![ + Item { + key: 2, + value: "b".to_string(), + }, + Item { + key: 1, + value: "a".to_string(), + }, + ]; + items.sort_by_key(|item1| item1.key); + //~^ unnecessary_sort_by + + items.sort_by(|item1, item2| item1.value.cmp(&item2.value)); + + items.sort_by_key(|item1| item1.value.clone()); + //~^ unnecessary_sort_by +} diff --git a/tests/ui/unnecessary_sort_by.rs b/tests/ui/unnecessary_sort_by.rs index 65db7ca3f137..7dd274623b35 100644 --- a/tests/ui/unnecessary_sort_by.rs +++ b/tests/ui/unnecessary_sort_by.rs @@ -111,3 +111,33 @@ fn main() { issue_5754::test(); issue_6001::test(); } + +fn issue16405() { + let mut v: Vec<(i32, &str)> = vec![(1, "foo"), (2, "bar")]; + + v.sort_by(|a, b| a.0.cmp(&b.0)); + //~^ unnecessary_sort_by + + struct Item { + key: i32, + value: String, + } + + let mut items = vec![ + Item { + key: 2, + value: "b".to_string(), + }, + Item { + key: 1, + value: "a".to_string(), + }, + ]; + items.sort_by(|item1, item2| item1.key.cmp(&item2.key)); + //~^ unnecessary_sort_by + + items.sort_by(|item1, item2| item1.value.cmp(&item2.value)); + + items.sort_by(|item1, item2| item1.value.clone().cmp(&item2.value.clone())); + //~^ unnecessary_sort_by +} diff --git a/tests/ui/unnecessary_sort_by.stderr b/tests/ui/unnecessary_sort_by.stderr index a066554037fe..b555245b0d01 100644 --- a/tests/ui/unnecessary_sort_by.stderr +++ b/tests/ui/unnecessary_sort_by.stderr @@ -73,5 +73,23 @@ error: consider using `sort_unstable_by_key` LL | args.sort_unstable_by(|a, b| b.name().cmp(&a.name())); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_unstable_by_key(|b| std::cmp::Reverse(b.name()))` -error: aborting due to 12 previous errors +error: consider using `sort_by_key` + --> tests/ui/unnecessary_sort_by.rs:118:5 + | +LL | v.sort_by(|a, b| a.0.cmp(&b.0)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `v.sort_by_key(|a| a.0)` + +error: consider using `sort_by_key` + --> tests/ui/unnecessary_sort_by.rs:136:5 + | +LL | items.sort_by(|item1, item2| item1.key.cmp(&item2.key)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `items.sort_by_key(|item1| item1.key)` + +error: consider using `sort_by_key` + --> tests/ui/unnecessary_sort_by.rs:141:5 + | +LL | items.sort_by(|item1, item2| item1.value.clone().cmp(&item2.value.clone())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `items.sort_by_key(|item1| item1.value.clone())` + +error: aborting due to 15 previous errors