Skip to content

Commit c9fdeef

Browse files
borsbors
committed
Auto merge of #6078 - ebroto:unnecessary_sort_by_take_2, r=phansch
unnecessary sort by: avoid dereferencing the suggested closure parameter This change tries to simplify the solution for problematic cases but is less restrictive than #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 #6001
2 parents 3239b46 + 9365660 commit c9fdeef

File tree

4 files changed

+81
-69
lines changed

4 files changed

+81
-69
lines changed

‎clippy_lints/src/unnecessary_sort_by.rs

Lines changed: 20 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -170,22 +170,12 @@ fn mirrored_exprs(
170170
}
171171

172172
fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
173-
// NOTE: Vectors of references are not supported. In order to avoid hitting https://github.com/rust-lang/rust/issues/34162,
174-
// (different unnamed lifetimes for closure arg and return type) we need to make sure the suggested
175-
// closure parameter is not a reference in case we suggest `Reverse`. Trying to destructure more
176-
// than one level of references would add some extra complexity as we would have to compensate
177-
// in the closure body.
178-
179173
if_chain! {
180174
if let ExprKind::MethodCall(name_ident, _, args, _) = &expr.kind;
181175
if let name = name_ident.ident.name.to_ident_string();
182176
if name == "sort_by" || name == "sort_unstable_by";
183177
if let [vec, Expr { kind: ExprKind::Closure(_, _, closure_body_id, _, _), .. }] = args;
184-
let vec_ty = cx.typeck_results().expr_ty(vec);
185-
if utils::is_type_diagnostic_item(cx, vec_ty, sym!(vec_type));
186-
let ty = vec_ty.walk().nth(1).unwrap().expect_ty(); // T in Vec<T>
187-
if !matches!(&ty.kind(), ty::Ref(..));
188-
if utils::is_copy(cx, ty);
178+
if utils::is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(vec), sym!(vec_type));
189179
if let closure_body = cx.tcx.hir().body(*closure_body_id);
190180
if let &[
191181
Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..},
@@ -210,40 +200,32 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
210200
let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
211201
let unstable = name == "sort_unstable_by";
212202

213-
if_chain! {
214-
if let ExprKind::Path(QPath::Resolved(_, Path {
215-
segments: [PathSegment { ident: left_name, .. }], ..
216-
})) = &left_expr.kind;
217-
if left_name == left_ident;
218-
then {
219-
return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
220-
} else {
221-
if !key_returns_borrow(cx, left_expr) {
222-
return Some(LintTrigger::SortByKey(SortByKeyDetection {
223-
vec_name,
224-
unstable,
225-
closure_arg,
226-
closure_body,
227-
reverse
228-
}))
229-
}
203+
if let ExprKind::Path(QPath::Resolved(_, Path {
204+
segments: [PathSegment { ident: left_name, .. }], ..
205+
})) = &left_expr.kind {
206+
if left_name == left_ident {
207+
return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }));
230208
}
231209
}
210+
211+
if !expr_borrows(cx, left_expr) {
212+
return Some(LintTrigger::SortByKey(SortByKeyDetection {
213+
vec_name,
214+
unstable,
215+
closure_arg,
216+
closure_body,
217+
reverse
218+
}));
219+
}
232220
}
233221
}
234222

235223
None
236224
}
237225

238-
fn key_returns_borrow(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
239-
if let Some(def_id) = utils::fn_def_id(cx, expr) {
240-
let output = cx.tcx.fn_sig(def_id).output();
241-
let ty = output.skip_binder();
242-
return matches!(ty.kind(), ty::Ref(..))
243-
|| ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)));
244-
}
245-
246-
false
226+
fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
227+
let ty = cx.typeck_results().expr_ty(expr);
228+
matches!(ty.kind(), ty::Ref(..)) || ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)))
247229
}
248230

249231
impl LateLintPass<'_> for UnnecessarySortBy {
@@ -256,7 +238,7 @@ impl LateLintPass<'_> for UnnecessarySortBy {
256238
"use Vec::sort_by_key here instead",
257239
"try",
258240
format!(
259-
"{}.sort{}_by_key(|&{}| {})",
241+
"{}.sort{}_by_key(|{}| {})",
260242
trigger.vec_name,
261243
if trigger.unstable { "_unstable" } else { "" },
262244
trigger.closure_arg,

‎tests/ui/unnecessary_sort_by.fixed

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,24 @@ fn unnecessary_sort_by() {
1313
// Forward examples
1414
vec.sort();
1515
vec.sort_unstable();
16-
vec.sort_by_key(|&a| (a + 5).abs());
17-
vec.sort_unstable_by_key(|&a| id(-a));
16+
vec.sort_by_key(|a| (a + 5).abs());
17+
vec.sort_unstable_by_key(|a| id(-a));
1818
// Reverse examples
19-
vec.sort_by_key(|&b| Reverse(b));
20-
vec.sort_by_key(|&b| Reverse((b + 5).abs()));
21-
vec.sort_unstable_by_key(|&b| Reverse(id(-b)));
19+
vec.sort_by(|a, b| b.cmp(a)); // not linted to avoid suggesting `Reverse(b)` which would borrow
20+
vec.sort_by_key(|b| Reverse((b + 5).abs()));
21+
vec.sort_unstable_by_key(|b| Reverse(id(-b)));
2222
// Negative examples (shouldn't be changed)
2323
let c = &7;
2424
vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
2525
vec.sort_by(|_, b| b.cmp(&5));
2626
vec.sort_by(|_, b| b.cmp(c));
2727
vec.sort_unstable_by(|a, _| a.cmp(c));
2828

29-
// Ignore vectors of references
29+
// Vectors of references are fine as long as the resulting key does not borrow
3030
let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
31-
vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
32-
vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
31+
vec.sort_by_key(|a| (***a).abs());
32+
vec.sort_unstable_by_key(|a| (***a).abs());
33+
// `Reverse(b)` would borrow in the following cases, don't lint
3334
vec.sort_by(|a, b| b.cmp(a));
3435
vec.sort_unstable_by(|a, b| b.cmp(a));
3536
}
@@ -68,10 +69,9 @@ mod issue_5754 {
6869
}
6970
}
7071

71-
// `Vec::sort_by_key` closure parameter is `F: FnMut(&T) -> K`
72-
// The suggestion is destructuring T and we know T is not a reference, so test that non-Copy T are
73-
// not linted.
72+
// The closure parameter is not dereferenced anymore, so non-Copy types can be linted
7473
mod issue_6001 {
74+
use super::*;
7575
struct Test(String);
7676

7777
impl Test {
@@ -85,11 +85,11 @@ mod issue_6001 {
8585
let mut args: Vec<Test> = vec![];
8686

8787
// Forward
88-
args.sort_by(|a, b| a.name().cmp(&b.name()));
89-
args.sort_unstable_by(|a, b| a.name().cmp(&b.name()));
88+
args.sort_by_key(|a| a.name());
89+
args.sort_unstable_by_key(|a| a.name());
9090
// Reverse
91-
args.sort_by(|a, b| b.name().cmp(&a.name()));
92-
args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
91+
args.sort_by_key(|b| Reverse(b.name()));
92+
args.sort_unstable_by_key(|b| Reverse(b.name()));
9393
}
9494
}
9595

‎tests/ui/unnecessary_sort_by.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ fn unnecessary_sort_by() {
1616
vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
1717
vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
1818
// Reverse examples
19-
vec.sort_by(|a, b| b.cmp(a));
19+
vec.sort_by(|a, b| b.cmp(a)); // not linted to avoid suggesting `Reverse(b)` which would borrow
2020
vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
2121
vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
2222
// Negative examples (shouldn't be changed)
@@ -26,10 +26,11 @@ fn unnecessary_sort_by() {
2626
vec.sort_by(|_, b| b.cmp(c));
2727
vec.sort_unstable_by(|a, _| a.cmp(c));
2828

29-
// Ignore vectors of references
29+
// Vectors of references are fine as long as the resulting key does not borrow
3030
let mut vec: Vec<&&&isize> = vec![&&&3, &&&6, &&&1, &&&2, &&&5];
3131
vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
3232
vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
33+
// `Reverse(b)` would borrow in the following cases, don't lint
3334
vec.sort_by(|a, b| b.cmp(a));
3435
vec.sort_unstable_by(|a, b| b.cmp(a));
3536
}
@@ -68,10 +69,9 @@ mod issue_5754 {
6869
}
6970
}
7071

71-
// `Vec::sort_by_key` closure parameter is `F: FnMut(&T) -> K`
72-
// The suggestion is destructuring T and we know T is not a reference, so test that non-Copy T are
73-
// not linted.
72+
// The closure parameter is not dereferenced anymore, so non-Copy types can be linted
7473
mod issue_6001 {
74+
use super::*;
7575
struct Test(String);
7676

7777
impl Test {

‎tests/ui/unnecessary_sort_by.stderr

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,31 +16,61 @@ error: use Vec::sort_by_key here instead
1616
--> $DIR/unnecessary_sort_by.rs:16:5
1717
|
1818
LL | vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
19-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| (a + 5).abs())`
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| (a + 5).abs())`
2020

2121
error: use Vec::sort_by_key here instead
2222
--> $DIR/unnecessary_sort_by.rs:17:5
2323
|
2424
LL | vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
25-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&a| id(-a))`
26-
27-
error: use Vec::sort_by_key here instead
28-
--> $DIR/unnecessary_sort_by.rs:19:5
29-
|
30-
LL | vec.sort_by(|a, b| b.cmp(a));
31-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(b))`
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|a| id(-a))`
3226

3327
error: use Vec::sort_by_key here instead
3428
--> $DIR/unnecessary_sort_by.rs:20:5
3529
|
3630
LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
37-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))`
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|b| Reverse((b + 5).abs()))`
3832

3933
error: use Vec::sort_by_key here instead
4034
--> $DIR/unnecessary_sort_by.rs:21:5
4135
|
4236
LL | vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
43-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&b| Reverse(id(-b)))`
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|b| Reverse(id(-b)))`
38+
39+
error: use Vec::sort_by_key here instead
40+
--> $DIR/unnecessary_sort_by.rs:31:5
41+
|
42+
LL | vec.sort_by(|a, b| (***a).abs().cmp(&(***b).abs()));
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| (***a).abs())`
44+
45+
error: use Vec::sort_by_key here instead
46+
--> $DIR/unnecessary_sort_by.rs:32:5
47+
|
48+
LL | vec.sort_unstable_by(|a, b| (***a).abs().cmp(&(***b).abs()));
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|a| (***a).abs())`
50+
51+
error: use Vec::sort_by_key here instead
52+
--> $DIR/unnecessary_sort_by.rs:88:9
53+
|
54+
LL | args.sort_by(|a, b| a.name().cmp(&b.name()));
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_by_key(|a| a.name())`
56+
57+
error: use Vec::sort_by_key here instead
58+
--> $DIR/unnecessary_sort_by.rs:89:9
59+
|
60+
LL | args.sort_unstable_by(|a, b| a.name().cmp(&b.name()));
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_unstable_by_key(|a| a.name())`
62+
63+
error: use Vec::sort_by_key here instead
64+
--> $DIR/unnecessary_sort_by.rs:91:9
65+
|
66+
LL | args.sort_by(|a, b| b.name().cmp(&a.name()));
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_by_key(|b| Reverse(b.name()))`
68+
69+
error: use Vec::sort_by_key here instead
70+
--> $DIR/unnecessary_sort_by.rs:92:9
71+
|
72+
LL | args.sort_unstable_by(|a, b| b.name().cmp(&a.name()));
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `args.sort_unstable_by_key(|b| Reverse(b.name()))`
4474

45-
error: aborting due to 7 previous errors
75+
error: aborting due to 12 previous errors
4676

0 commit comments

Comments
 (0)