From 9a5baed482b68e0d9806e19eb9e8676d7ff3e1c2 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sun, 31 May 2020 15:09:12 -0700 Subject: [PATCH] Implement suggestions from phansch --- clippy_lints/src/unnecessary_sort_by.rs | 41 ++++++++++++++++++++----- tests/ui/unnecessary_sort_by.fixed | 7 +++-- tests/ui/unnecessary_sort_by.rs | 7 +++-- tests/ui/unnecessary_sort_by.stderr | 26 ++++++++++------ 4 files changed, 57 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/unnecessary_sort_by.rs b/clippy_lints/src/unnecessary_sort_by.rs index c0858ec4c88..33d8331c292 100644 --- a/clippy_lints/src/unnecessary_sort_by.rs +++ b/clippy_lints/src/unnecessary_sort_by.rs @@ -18,15 +18,25 @@ declare_clippy_lint! { /// possible) than to use `Vec::sort_by` and and a more complicated /// closure. /// - /// **Known problems:** None. + /// **Known problems:** + /// If the suggested `Vec::sort_by_key` uses Reverse and it isn't + /// imported by a use statement in the current frame, then a `use` + /// statement that imports it will need to be added (which this lint + /// can't do). /// /// **Example:** /// /// ```rust - /// vec.sort_by(|a, b| a.foo().cmp(b.foo())); + /// # struct A; + /// # impl A { fn foo(&self) {} } + /// # let mut vec: Vec = Vec::new(); + /// vec.sort_by(|a, b| a.foo().cmp(&b.foo())); /// ``` /// Use instead: /// ```rust + /// # struct A; + /// # impl A { fn foo(&self) {} } + /// # let mut vec: Vec = Vec::new(); /// vec.sort_by_key(|a| a.foo()); /// ``` pub UNNECESSARY_SORT_BY, @@ -50,6 +60,7 @@ struct SortByKeyDetection { vec_name: String, closure_arg: String, closure_body: String, + reverse: bool, unstable: bool, } @@ -172,16 +183,16 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option if let ExprKind::MethodCall(method_path, _, [ref left_expr, ref right_expr]) = &closure_body.value.kind; if method_path.ident.name.to_ident_string() == "cmp"; then { - let (closure_body, closure_arg) = if mirrored_exprs( + let (closure_body, closure_arg, reverse) = if mirrored_exprs( &cx, &left_expr, &left_ident, &right_expr, &right_ident ) { - (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string()) + (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string(), false) } else if mirrored_exprs(&cx, &left_expr, &right_ident, &right_expr, &left_ident) { - (format!("Reverse({})", Sugg::hir(cx, &left_expr, "..").to_string()), right_ident.name.to_string()) + (Sugg::hir(cx, &left_expr, "..").to_string(), right_ident.name.to_string(), true) } else { return None; }; @@ -196,7 +207,13 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option Some(LintTrigger::Sort(SortDetection { vec_name, unstable })) } else { - Some(LintTrigger::SortByKey(SortByKeyDetection { vec_name, unstable, closure_arg, closure_body })) + Some(LintTrigger::SortByKey(SortByKeyDetection { + vec_name, + unstable, + closure_arg, + closure_body, + reverse + })) } } } else { @@ -219,9 +236,17 @@ impl LateLintPass<'_, '_> for UnnecessarySortBy { trigger.vec_name, if trigger.unstable { "_unstable" } else { "" }, trigger.closure_arg, - trigger.closure_body, + if trigger.reverse { + format!("Reverse({})", trigger.closure_body) + } else { + trigger.closure_body.to_string() + }, ), - Applicability::MachineApplicable, + if trigger.reverse { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }, ), Some(LintTrigger::Sort(trigger)) => utils::span_lint_and_sugg( cx, diff --git a/tests/ui/unnecessary_sort_by.fixed b/tests/ui/unnecessary_sort_by.fixed index 4521ae38d49..779fd57707a 100644 --- a/tests/ui/unnecessary_sort_by.fixed +++ b/tests/ui/unnecessary_sort_by.fixed @@ -10,16 +10,17 @@ fn main() { let mut vec: Vec = vec![3, 6, 1, 2, 5]; // Forward examples vec.sort(); + vec.sort_unstable(); vec.sort_by_key(|&a| (a + 5).abs()); - vec.sort_by_key(|&a| id(-a)); + vec.sort_unstable_by_key(|&a| id(-a)); // Reverse examples vec.sort_by_key(|&b| Reverse(b)); vec.sort_by_key(|&b| Reverse((b + 5).abs())); - vec.sort_by_key(|&b| Reverse(id(-b))); + vec.sort_unstable_by_key(|&b| Reverse(id(-b))); // Negative examples (shouldn't be changed) let c = &7; vec.sort_by(|a, b| (b - a).cmp(&(a - b))); vec.sort_by(|_, b| b.cmp(&5)); vec.sort_by(|_, b| b.cmp(c)); - vec.sort_by(|a, _| a.cmp(c)); + vec.sort_unstable_by(|a, _| a.cmp(c)); } diff --git a/tests/ui/unnecessary_sort_by.rs b/tests/ui/unnecessary_sort_by.rs index fdb5a823369..0485a5630af 100644 --- a/tests/ui/unnecessary_sort_by.rs +++ b/tests/ui/unnecessary_sort_by.rs @@ -10,16 +10,17 @@ fn main() { let mut vec: Vec = vec![3, 6, 1, 2, 5]; // Forward examples vec.sort_by(|a, b| a.cmp(b)); + vec.sort_unstable_by(|a, b| a.cmp(b)); vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs())); - vec.sort_by(|a, b| id(-a).cmp(&id(-b))); + vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b))); // Reverse examples vec.sort_by(|a, b| b.cmp(a)); vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs())); - vec.sort_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) let c = &7; vec.sort_by(|a, b| (b - a).cmp(&(a - b))); vec.sort_by(|_, b| b.cmp(&5)); vec.sort_by(|_, b| b.cmp(c)); - vec.sort_by(|a, _| a.cmp(c)); + vec.sort_unstable_by(|a, _| a.cmp(c)); } diff --git a/tests/ui/unnecessary_sort_by.stderr b/tests/ui/unnecessary_sort_by.stderr index b6365c1709d..903b6e5099c 100644 --- a/tests/ui/unnecessary_sort_by.stderr +++ b/tests/ui/unnecessary_sort_by.stderr @@ -6,35 +6,41 @@ LL | vec.sort_by(|a, b| a.cmp(b)); | = note: `-D clippy::unnecessary-sort-by` implied by `-D warnings` -error: use Vec::sort_by_key here instead +error: use Vec::sort here instead --> $DIR/unnecessary_sort_by.rs:13:5 | +LL | vec.sort_unstable_by(|a, b| a.cmp(b)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable()` + +error: use Vec::sort_by_key here instead + --> $DIR/unnecessary_sort_by.rs:14:5 + | LL | vec.sort_by(|a, b| (a + 5).abs().cmp(&(b + 5).abs())); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| (a + 5).abs())` error: use Vec::sort_by_key here instead - --> $DIR/unnecessary_sort_by.rs:14:5 + --> $DIR/unnecessary_sort_by.rs:15:5 | -LL | vec.sort_by(|a, b| id(-a).cmp(&id(-b))); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| id(-a))` +LL | vec.sort_unstable_by(|a, b| id(-a).cmp(&id(-b))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&a| id(-a))` error: use Vec::sort_by_key here instead - --> $DIR/unnecessary_sort_by.rs:16:5 + --> $DIR/unnecessary_sort_by.rs:17: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 - --> $DIR/unnecessary_sort_by.rs:17:5 + --> $DIR/unnecessary_sort_by.rs:18:5 | LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs())); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))` error: use Vec::sort_by_key here instead - --> $DIR/unnecessary_sort_by.rs:18:5 + --> $DIR/unnecessary_sort_by.rs:19:5 | -LL | vec.sort_by(|a, b| id(-b).cmp(&id(-a))); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))` +LL | vec.sort_unstable_by(|a, b| id(-b).cmp(&id(-a))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_unstable_by_key(|&b| Reverse(id(-b)))` -error: aborting due to 6 previous errors +error: aborting due to 7 previous errors