Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-27 07:00:55 +00:00
Code Issues Projects Releases Packages Wiki Activity

unnecessary sort by: avoid dereferencing closure param

Browse source
This commit is contained in:
Eduardo Broto 2020-09-23 23:33:50 +02:00
parent e636b88aa1
commit 9365660a2f
4 changed files with 81 additions and 69 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

58
clippy_lints/src/unnecessary_sort_by.rs
View file

@ -170,22 +170,12 @@ fn mirrored_exprs(
} }
fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> { 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_chain! {
if let ExprKind::MethodCall(name_ident, _, args, _) = &expr.kind; if let ExprKind::MethodCall(name_ident, _, args, _) = &expr.kind;
if let name = name_ident.ident.name.to_ident_string(); if let name = name_ident.ident.name.to_ident_string();
if name == "sort_by" || name == "sort_unstable_by"; if name == "sort_by" || name == "sort_unstable_by";
if let [vec, Expr { kind: ExprKind::Closure(_, _, closure_body_id, _, _), .. }] = args; 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, cx.typeck_results().expr_ty(vec), sym!(vec_type));
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 closure_body = cx.tcx.hir().body(*closure_body_id);
if let &[ if let &[
Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..}, 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 vec_name = Sugg::hir(cx, &args[0], "..").to_string();
let unstable = name == "sort_unstable_by"; let unstable = name == "sort_unstable_by";
if_chain! { if let ExprKind::Path(QPath::Resolved(_, Path {
if let ExprKind::Path(QPath::Resolved(_, Path { segments: [PathSegment { ident: left_name, .. }], ..
segments: [PathSegment { ident: left_name, .. }], .. })) = &left_expr.kind {
})) = &left_expr.kind; if left_name == left_ident {
if left_name == left_ident; return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }));
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 !expr_borrows(cx, left_expr) {
return Some(LintTrigger::SortByKey(SortByKeyDetection {
vec_name,
unstable,
closure_arg,
closure_body,
reverse
}));
}
} }
} }
None None
} }
fn key_returns_borrow(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
if let Some(def_id) = utils::fn_def_id(cx, expr) { let ty = cx.typeck_results().expr_ty(expr);
let output = cx.tcx.fn_sig(def_id).output(); matches!(ty.kind(), ty::Ref(..)) || ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)))
let ty = output.skip_binder();
return matches!(ty.kind(), ty::Ref(..))
|| ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)));
}
false
} }
impl LateLintPass<'_> for UnnecessarySortBy { impl LateLintPass<'_> for UnnecessarySortBy {
@ -256,7 +238,7 @@ impl LateLintPass<'_> for UnnecessarySortBy {
"use Vec::sort_by_key here instead", "use Vec::sort_by_key here instead",
"try", "try",
format!( format!(
"{}.sort{}_by_key(|&{}| {})", "{}.sort{}_by_key(|{}| {})",
trigger.vec_name, trigger.vec_name,
if trigger.unstable { "_unstable" } else { "" }, if trigger.unstable { "_unstable" } else { "" },
trigger.closure_arg, trigger.closure_arg,

30
tests/ui/unnecessary_sort_by.fixed
View file

@ -13,12 +13,12 @@ fn unnecessary_sort_by() {
// Forward examples // Forward examples
vec.sort(); vec.sort();
vec.sort_unstable(); vec.sort_unstable();
vec.sort_by_key(|&a| (a + 5).abs()); vec.sort_by_key(|a| (a + 5).abs());
vec.sort_unstable_by_key(|&a| id(-a)); vec.sort_unstable_by_key(|a| id(-a));
// Reverse examples // Reverse examples
vec.sort_by_key(|&b| Reverse(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_by_key(|b| Reverse((b + 5).abs()));
vec.sort_unstable_by_key(|&b| Reverse(id(-b))); vec.sort_unstable_by_key(|b| Reverse(id(-b)));
// Negative examples (shouldn't be changed) // Negative examples (shouldn't be changed)
let c = &7; let c = &7;
vec.sort_by(|a, b| (b - a).cmp(&(a - b))); vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
@ -26,10 +26,11 @@ fn unnecessary_sort_by() {
vec.sort_by(|_, b| b.cmp(c)); vec.sort_by(|_, b| b.cmp(c));
vec.sort_unstable_by(|a, _| a.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]; let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs())); vec.sort_by_key(|a| (***a).abs());
vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).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_by(|a, b| b.cmp(a));
vec.sort_unstable_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 closure parameter is not dereferenced anymore, so non-Copy types can be linted
// 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 { mod issue_6001 {
use super::*;
struct Test(String); struct Test(String);
impl Test { impl Test {
@ -85,11 +85,11 @@ mod issue_6001 {
let mut args: Vec<Test> = vec![]; let mut args: Vec<Test> = vec![];
// Forward // Forward
args.sort_by(|a, b| a.name().cmp(&b.name())); args.sort_by_key(|a| a.name());
args.sort_unstable_by(|a, b| a.name().cmp(&b.name())); args.sort_unstable_by_key(|a| a.name());
// Reverse // Reverse
args.sort_by(|a, b| b.name().cmp(&a.name())); args.sort_by_key(|b| Reverse(b.name()));
args.sort_unstable_by(|a, b| b.name().cmp(&a.name())); args.sort_unstable_by_key(|b| Reverse(b.name()));
} }
} }

10
tests/ui/unnecessary_sort_by.rs
View file

@ -16,7 +16,7 @@ fn unnecessary_sort_by() {
vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs())); vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b))); vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
// Reverse examples // 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_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
vec.sort_unstable_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) // Negative examples (shouldn't be changed)
@ -26,10 +26,11 @@ fn unnecessary_sort_by() {
vec.sort_by(|_, b| b.cmp(c)); vec.sort_by(|_, b| b.cmp(c));
vec.sort_unstable_by(|a, _| a.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]; let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs())); vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
vec.sort_unstable_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_by(|a, b| b.cmp(a));
vec.sort_unstable_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 closure parameter is not dereferenced anymore, so non-Copy types can be linted
// 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 { mod issue_6001 {
use super::*;
struct Test(String); struct Test(String);
impl Test { impl Test {

52
tests/ui/unnecessary_sort_by.stderr
View file

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