Skip to content

Commit 9a5baed

Browse files
JarredAllenJarredAllen
committed
Implement suggestions from phansch
1 parent 20cb512 commit 9a5baed

File tree

4 files changed

+57
-24
lines changed

4 files changed

+57
-24
lines changed

‎clippy_lints/src/unnecessary_sort_by.rs‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,25 @@ declare_clippy_lint! {
1818
/// possible) than to use `Vec::sort_by` and and a more complicated
1919
/// closure.
2020
///
21-
/// **Known problems:** None.
21+
/// **Known problems:**
22+
/// If the suggested `Vec::sort_by_key` uses Reverse and it isn't
23+
/// imported by a use statement in the current frame, then a `use`
24+
/// statement that imports it will need to be added (which this lint
25+
/// can't do).
2226
///
2327
/// **Example:**
2428
///
2529
/// ```rust
26-
/// vec.sort_by(|a, b| a.foo().cmp(b.foo()));
30+
/// # struct A;
31+
/// # impl A { fn foo(&self) {} }
32+
/// # let mut vec: Vec<A> = Vec::new();
33+
/// vec.sort_by(|a, b| a.foo().cmp(&b.foo()));
2734
/// ```
2835
/// Use instead:
2936
/// ```rust
37+
/// # struct A;
38+
/// # impl A { fn foo(&self) {} }
39+
/// # let mut vec: Vec<A> = Vec::new();
3040
/// vec.sort_by_key(|a| a.foo());
3141
/// ```
3242
pub UNNECESSARY_SORT_BY,
@@ -50,6 +60,7 @@ struct SortByKeyDetection {
5060
vec_name: String,
5161
closure_arg: String,
5262
closure_body: String,
63+
reverse: bool,
5364
unstable: bool,
5465
}
5566

@@ -172,16 +183,16 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
172183
if let ExprKind::MethodCall(method_path, _, [ref left_expr, ref right_expr]) = &closure_body.value.kind;
173184
if method_path.ident.name.to_ident_string() == "cmp";
174185
then {
175-
let (closure_body, closure_arg) = if mirrored_exprs(
186+
let (closure_body, closure_arg, reverse) = if mirrored_exprs(
176187
&cx,
177188
&left_expr,
178189
&left_ident,
179190
&right_expr,
180191
&right_ident
181192
) {
182-
(Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string())
193+
(Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string(), false)
183194
} else if mirrored_exprs(&cx, &left_expr, &right_ident, &right_expr, &left_ident) {
184-
(format!("Reverse({})", Sugg::hir(cx, &left_expr, "..").to_string()), right_ident.name.to_string())
195+
(Sugg::hir(cx, &left_expr, "..").to_string(), right_ident.name.to_string(), true)
185196
} else {
186197
return None;
187198
};
@@ -196,7 +207,13 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
196207
Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
197208
}
198209
else {
199-
Some(LintTrigger::SortByKey(SortByKeyDetection { vec_name, unstable, closure_arg, closure_body }))
210+
Some(LintTrigger::SortByKey(SortByKeyDetection {
211+
vec_name,
212+
unstable,
213+
closure_arg,
214+
closure_body,
215+
reverse
216+
}))
200217
}
201218
}
202219
} else {
@@ -219,9 +236,17 @@ impl LateLintPass<'_, '_> for UnnecessarySortBy {
219236
trigger.vec_name,
220237
if trigger.unstable { "_unstable" } else { "" },
221238
trigger.closure_arg,
222-
trigger.closure_body,
239+
if trigger.reverse {
240+
format!("Reverse({})", trigger.closure_body)
241+
} else {
242+
trigger.closure_body.to_string()
243+
},
223244
),
224-
Applicability::MachineApplicable,
245+
if trigger.reverse {
246+
Applicability::MaybeIncorrect
247+
} else {
248+
Applicability::MachineApplicable
249+
},
225250
),
226251
Some(LintTrigger::Sort(trigger)) => utils::span_lint_and_sugg(
227252
cx,

‎tests/ui/unnecessary_sort_by.fixed‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,17 @@ fn main() {
1010
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
1111
// Forward examples
1212
vec.sort();
13+
vec.sort_unstable();
1314
vec.sort_by_key(|&a| (a + 5).abs());
14-
vec.sort_by_key(|&a| id(-a));
15+
vec.sort_unstable_by_key(|&a| id(-a));
1516
// Reverse examples
1617
vec.sort_by_key(|&b| Reverse(b));
1718
vec.sort_by_key(|&b| Reverse((b + 5).abs()));
18-
vec.sort_by_key(|&b| Reverse(id(-b)));
19+
vec.sort_unstable_by_key(|&b| Reverse(id(-b)));
1920
// Negative examples (shouldn't be changed)
2021
let c = &7;
2122
vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
2223
vec.sort_by(|_, b| b.cmp(&5));
2324
vec.sort_by(|_, b| b.cmp(c));
24-
vec.sort_by(|a, _| a.cmp(c));
25+
vec.sort_unstable_by(|a, _| a.cmp(c));
2526
}

‎tests/ui/unnecessary_sort_by.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,17 @@ fn main() {
1010
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
1111
// Forward examples
1212
vec.sort_by(|a, b| a.cmp(b));
13+
vec.sort_unstable_by(|a, b| a.cmp(b));
1314
vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
14-
vec.sort_by(|a, b| id(-a).cmp(&id(-b)));
15+
vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
1516
// Reverse examples
1617
vec.sort_by(|a, b| b.cmp(a));
1718
vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
18-
vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
19+
vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
1920
// Negative examples (shouldn't be changed)
2021
let c = &7;
2122
vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
2223
vec.sort_by(|_, b| b.cmp(&5));
2324
vec.sort_by(|_, b| b.cmp(c));
24-
vec.sort_by(|a, _| a.cmp(c));
25+
vec.sort_unstable_by(|a, _| a.cmp(c));
2526
}

‎tests/ui/unnecessary_sort_by.stderr‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,35 +6,41 @@ LL | vec.sort_by(|a, b| a.cmp(b));
66
|
77
= note: `-D clippy::unnecessary-sort-by` implied by `-D warnings`
88

9-
error: use Vec::sort_by_key here instead
9+
error: use Vec::sort here instead
1010
--> $DIR/unnecessary_sort_by.rs:13:5
1111
|
12+
LL | vec.sort_unstable_by(|a, b| a.cmp(b));
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable()`
14+
15+
error: use Vec::sort_by_key here instead
16+
--> $DIR/unnecessary_sort_by.rs:14:5
17+
|
1218
LL | vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs()));
1319
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| (a + 5).abs())`
1420

1521
error: use Vec::sort_by_key here instead
16-
--> $DIR/unnecessary_sort_by.rs:14:5
22+
--> $DIR/unnecessary_sort_by.rs:15:5
1723
|
18-
LL | vec.sort_by(|a, b| id(-a).cmp(&id(-b)));
19-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| id(-a))`
24+
LL | vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b)));
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&a| id(-a))`
2026

2127
error: use Vec::sort_by_key here instead
22-
--> $DIR/unnecessary_sort_by.rs:16:5
28+
--> $DIR/unnecessary_sort_by.rs:17:5
2329
|
2430
LL | vec.sort_by(|a, b| b.cmp(a));
2531
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(b))`
2632

2733
error: use Vec::sort_by_key here instead
28-
--> $DIR/unnecessary_sort_by.rs:17:5
34+
--> $DIR/unnecessary_sort_by.rs:18:5
2935
|
3036
LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
3137
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))`
3238

3339
error: use Vec::sort_by_key here instead
34-
--> $DIR/unnecessary_sort_by.rs:18:5
40+
--> $DIR/unnecessary_sort_by.rs:19:5
3541
|
36-
LL | vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
37-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))`
42+
LL | vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a)));
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&b| Reverse(id(-b)))`
3844

39-
error: aborting due to 6 previous errors
45+
error: aborting due to 7 previous errors
4046

0 commit comments

Comments
 (0)