From 059e8edd15401d5544260e4058731dc8818578d5 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sun, 24 May 2020 19:45:41 -0700 Subject: [PATCH] Detect also a non-reversed comparison --- clippy_lints/src/lib.rs | 10 ++-- ...{sort_by_key_reverse.rs => sort_by_key.rs} | 52 +++++++++++-------- ...by_key_reverse.fixed => sort_by_key.fixed} | 8 ++- ...{sort_by_key_reverse.rs => sort_by_key.rs} | 6 ++- tests/ui/sort_by_key.stderr | 48 +++++++++++++++++ tests/ui/sort_by_key_reverse.stderr | 22 -------- 6 files changed, 94 insertions(+), 52 deletions(-) rename clippy_lints/src/{sort_by_key_reverse.rs => sort_by_key.rs} (83%) rename tests/ui/{sort_by_key_reverse.fixed => sort_by_key.fixed} (72%) rename tests/ui/{sort_by_key_reverse.rs => sort_by_key.rs} (78%) create mode 100644 tests/ui/sort_by_key.stderr delete mode 100644 tests/ui/sort_by_key_reverse.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e7a4c1ecaa9..9e826316f21 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -304,7 +304,7 @@ mod serde_api; mod shadow; mod single_component_path_imports; mod slow_vector_initialization; -mod sort_by_key_reverse; +mod sort_by_key; mod strings; mod suspicious_trait_impl; mod swap; @@ -780,7 +780,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &shadow::SHADOW_UNRELATED, &single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS, &slow_vector_initialization::SLOW_VECTOR_INITIALIZATION, - &sort_by_key_reverse::SORT_BY_KEY_REVERSE, + &sort_by_key::SORT_BY_KEY, &strings::STRING_ADD, &strings::STRING_ADD_ASSIGN, &strings::STRING_LIT_AS_BYTES, @@ -998,7 +998,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box ptr_offset_with_cast::PtrOffsetWithCast); store.register_late_pass(|| box redundant_clone::RedundantClone); store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit); - store.register_late_pass(|| box sort_by_key_reverse::SortByKeyReverse); + store.register_late_pass(|| box sort_by_key::SortByKey); store.register_late_pass(|| box types::RefToMut); store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants); store.register_late_pass(|| box missing_const_for_fn::MissingConstForFn); @@ -1394,7 +1394,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&serde_api::SERDE_API_MISUSE), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), - LintId::of(&sort_by_key_reverse::SORT_BY_KEY_REVERSE), + LintId::of(&sort_by_key::SORT_BY_KEY), LintId::of(&strings::STRING_LIT_AS_BYTES), LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), @@ -1596,7 +1596,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&ranges::RANGE_ZIP_WITH_LEN), LintId::of(&reference::DEREF_ADDROF), LintId::of(&reference::REF_IN_DEREF), - LintId::of(&sort_by_key_reverse::SORT_BY_KEY_REVERSE), + LintId::of(&sort_by_key::SORT_BY_KEY), LintId::of(&swap::MANUAL_SWAP), LintId::of(&temporary_assignment::TEMPORARY_ASSIGNMENT), LintId::of(&transmute::CROSSPOINTER_TRANSMUTE), diff --git a/clippy_lints/src/sort_by_key_reverse.rs b/clippy_lints/src/sort_by_key.rs similarity index 83% rename from clippy_lints/src/sort_by_key_reverse.rs rename to clippy_lints/src/sort_by_key.rs index ea850955db1..109845a28f4 100644 --- a/clippy_lints/src/sort_by_key_reverse.rs +++ b/clippy_lints/src/sort_by_key.rs @@ -11,33 +11,35 @@ use rustc_span::symbol::Ident; declare_clippy_lint! { /// **What it does:** /// Detects when people use `Vec::sort_by` and pass in a function - /// which compares the second argument to the first. + /// which compares the two arguments, either directly or indirectly. /// /// **Why is this bad?** - /// It is more clear to use `Vec::sort_by_key` and `std::cmp::Reverse` + /// It is more clear to use `Vec::sort_by_key` (or + /// `Vec::sort_by_key` and `std::cmp::Reverse` if necessary) than + /// using /// /// **Known problems:** None. /// /// **Example:** /// /// ```rust - /// vec.sort_by(|a, b| b.foo().cmp(&a.foo())); + /// vec.sort_by(|a, b| a.foo().cmp(b.foo())); /// ``` /// Use instead: /// ```rust - /// vec.sort_by_key(|e| Reverse(e.foo())); + /// vec.sort_by_key(|a| a.foo()); /// ``` - pub SORT_BY_KEY_REVERSE, + pub SORT_BY_KEY, complexity, "Use of `Vec::sort_by` when `Vec::sort_by_key` would be clearer" } -declare_lint_pass!(SortByKeyReverse => [SORT_BY_KEY_REVERSE]); +declare_lint_pass!(SortByKey => [SORT_BY_KEY]); struct LintTrigger { vec_name: String, closure_arg: String, - closure_reverse_body: String, + closure_body: String, unstable: bool, } @@ -154,43 +156,49 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option if utils::match_type(cx, &cx.tables.expr_ty(vec), &paths::VEC); if let closure_body = cx.tcx.hir().body(*closure_body_id); if let &[ - Param { pat: Pat { kind: PatKind::Binding(_, _, a_ident, _), .. }, ..}, - Param { pat: Pat { kind: PatKind::Binding(_, _, b_ident, _), .. }, .. } + Param { pat: Pat { kind: PatKind::Binding(_, _, left_ident, _), .. }, ..}, + Param { pat: Pat { kind: PatKind::Binding(_, _, right_ident, _), .. }, .. } ] = &closure_body.params; - if let ExprKind::MethodCall(method_path, _, [ref b_expr, ref a_expr]) = &closure_body.value.kind; + 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"; - if mirrored_exprs(&cx, &a_expr, &a_ident, &b_expr, &b_ident); then { + let (closure_body, closure_arg) = if mirrored_exprs( + &cx, + &left_expr, + &left_ident, + &right_expr, + &right_ident + ) { + (Sugg::hir(cx, &left_expr, "..").to_string(), left_ident.name.to_string()) + } 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()) + } else { + return None; + }; let vec_name = Sugg::hir(cx, &args[0], "..").to_string(); let unstable = name == "sort_unstable_by"; - let closure_arg = format!("&{}", b_ident.name.to_ident_string()); - let closure_reverse_body = Sugg::hir(cx, &b_expr, "..").to_string(); - // Get rid of parentheses, because they aren't needed anymore - // while closure_reverse_body.chars().next() == Some('(') && closure_reverse_body.chars().last() == Some(')') { - // closure_reverse_body = String::from(&closure_reverse_body[1..closure_reverse_body.len()-1]); - // } - Some(LintTrigger { vec_name, unstable, closure_arg, closure_reverse_body }) + Some(LintTrigger { vec_name, unstable, closure_arg, closure_body }) } else { None } } } -impl LateLintPass<'_, '_> for SortByKeyReverse { +impl LateLintPass<'_, '_> for SortByKey { fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) { if let Some(trigger) = detect_lint(cx, expr) { utils::span_lint_and_sugg( cx, - SORT_BY_KEY_REVERSE, + SORT_BY_KEY, expr.span, "use Vec::sort_by_key here instead", "try", format!( - "{}.sort{}_by_key(|{}| Reverse({}))", + "{}.sort{}_by_key(|&{}| {})", trigger.vec_name, if trigger.unstable { "_unstable" } else { "" }, trigger.closure_arg, - trigger.closure_reverse_body, + trigger.closure_body, ), Applicability::MachineApplicable, ); diff --git a/tests/ui/sort_by_key_reverse.fixed b/tests/ui/sort_by_key.fixed similarity index 72% rename from tests/ui/sort_by_key_reverse.fixed rename to tests/ui/sort_by_key.fixed index 722675a6b71..f6535c8d8f5 100644 --- a/tests/ui/sort_by_key_reverse.fixed +++ b/tests/ui/sort_by_key.fixed @@ -1,5 +1,5 @@ // run-rustfix -#![warn(clippy::sort_by_key_reverse)] +#![warn(clippy::sort_by_key)] use std::cmp::Reverse; @@ -9,6 +9,11 @@ fn id(x: isize) -> isize { fn main() { let mut vec: Vec = vec![3, 6, 1, 2, 5]; + // Forward examples + vec.sort_by_key(|&a| a); + vec.sort_by_key(|&a| (a + 5).abs()); + vec.sort_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))); @@ -18,5 +23,4 @@ fn main() { vec.sort_by(|_, b| b.cmp(&5)); vec.sort_by(|_, b| b.cmp(c)); vec.sort_by(|a, _| a.cmp(c)); - vec.sort_by(|a, b| a.cmp(b)); } diff --git a/tests/ui/sort_by_key_reverse.rs b/tests/ui/sort_by_key.rs similarity index 78% rename from tests/ui/sort_by_key_reverse.rs rename to tests/ui/sort_by_key.rs index 601621ffa9f..953c573d406 100644 --- a/tests/ui/sort_by_key_reverse.rs +++ b/tests/ui/sort_by_key.rs @@ -9,6 +9,11 @@ fn id(x: isize) -> isize { fn main() { let mut vec: Vec = vec![3, 6, 1, 2, 5]; + // Forward examples + vec.sort_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))); + // 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))); @@ -18,5 +23,4 @@ fn main() { vec.sort_by(|_, b| b.cmp(&5)); vec.sort_by(|_, b| b.cmp(c)); vec.sort_by(|a, _| a.cmp(c)); - vec.sort_by(|a, b| a.cmp(b)); } diff --git a/tests/ui/sort_by_key.stderr b/tests/ui/sort_by_key.stderr new file mode 100644 index 00000000000..fa6a9a0fb10 --- /dev/null +++ b/tests/ui/sort_by_key.stderr @@ -0,0 +1,48 @@ +error: use Vec::sort_by_key here instead + --> $DIR/sort_by_key.rs:13:5 + | +LL | vec.sort_by(|a, b| a.cmp(b)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| a)` + | + = note: `-D clippy::sort-by-key` implied by `-D warnings` + +error: use Vec::sort_by_key here instead + --> $DIR/sort_by_key.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/sort_by_key.rs:15:5 + | +LL | vec.sort_by(|a, b| id(-a).cmp(&id(-b))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&a| id(-a))` + +error: use Vec::sort_by_key here instead + --> $DIR/sort_by_key.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/sort_by_key.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/sort_by_key.rs:19:5 + | +LL | vec.sort_by(|a, b| id(-b).cmp(&id(-a))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))` + +error: unknown clippy lint: clippy::sort_by_key_reverse + --> $DIR/sort_by_key.rs:2:9 + | +LL | #![warn(clippy::sort_by_key_reverse)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: did you mean: `clippy::sort_by_key` + | + = note: `-D clippy::unknown-clippy-lints` implied by `-D warnings` + +error: aborting due to 7 previous errors + diff --git a/tests/ui/sort_by_key_reverse.stderr b/tests/ui/sort_by_key_reverse.stderr deleted file mode 100644 index b757c8a6176..00000000000 --- a/tests/ui/sort_by_key_reverse.stderr +++ /dev/null @@ -1,22 +0,0 @@ -error: use Vec::sort_by_key here instead - --> $DIR/sort_by_key_reverse.rs:12:5 - | -LL | vec.sort_by(|a, b| b.cmp(a)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(b))` - | - = note: `-D clippy::sort-by-key-reverse` implied by `-D warnings` - -error: use Vec::sort_by_key here instead - --> $DIR/sort_by_key_reverse.rs:13: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/sort_by_key_reverse.rs:14:5 - | -LL | vec.sort_by(|a, b| id(-b).cmp(&id(-a))); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse(id(-b)))` - -error: aborting due to 3 previous errors -