diff --git a/CHANGELOG.md b/CHANGELOG.md
index f7dae3dcfff0..086a1141be55 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1601,6 +1601,7 @@ Released 2018-09-13
[`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold
[`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
[`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
+[`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by
[`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
[`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern
[`unneeded_wildcard_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_wildcard_pattern
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 38cfa212d9f4..03addf1f4a40 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -318,6 +318,7 @@ mod try_err;
mod types;
mod unicode;
mod unnamed_address;
+mod unnecessary_sort_by;
mod unsafe_removed_from_name;
mod unused_io_amount;
mod unused_self;
@@ -833,6 +834,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&unicode::ZERO_WIDTH_SPACE,
&unnamed_address::FN_ADDRESS_COMPARISONS,
&unnamed_address::VTABLE_ADDRESS_COMPARISONS,
+ &unnecessary_sort_by::UNNECESSARY_SORT_BY,
&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
&unused_io_amount::UNUSED_IO_AMOUNT,
&unused_self::UNUSED_SELF,
@@ -996,6 +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 unnecessary_sort_by::UnnecessarySortBy);
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);
@@ -1427,6 +1430,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&unicode::ZERO_WIDTH_SPACE),
LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS),
LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS),
+ LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
LintId::of(&unwrap::PANICKING_UNWRAP),
@@ -1608,6 +1612,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&types::UNIT_ARG),
LintId::of(&types::UNNECESSARY_CAST),
LintId::of(&types::VEC_BOX),
+ LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
LintId::of(&unwrap::UNNECESSARY_UNWRAP),
LintId::of(&useless_conversion::USELESS_CONVERSION),
LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO),
diff --git a/clippy_lints/src/unnecessary_sort_by.rs b/clippy_lints/src/unnecessary_sort_by.rs
new file mode 100644
index 000000000000..33d8331c2923
--- /dev/null
+++ b/clippy_lints/src/unnecessary_sort_by.rs
@@ -0,0 +1,267 @@
+use crate::utils;
+use crate::utils::paths;
+use crate::utils::sugg::Sugg;
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+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 two arguments, either directly or indirectly.
+ ///
+ /// **Why is this bad?**
+ /// It is more clear to use `Vec::sort_by_key` (or `Vec::sort` if
+ /// possible) than to use `Vec::sort_by` and and a more complicated
+ /// closure.
+ ///
+ /// **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
+ /// # 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,
+ complexity,
+ "Use of `Vec::sort_by` when `Vec::sort_by_key` or `Vec::sort` would be clearer"
+}
+
+declare_lint_pass!(UnnecessarySortBy => [UNNECESSARY_SORT_BY]);
+
+enum LintTrigger {
+ Sort(SortDetection),
+ SortByKey(SortByKeyDetection),
+}
+
+struct SortDetection {
+ vec_name: String,
+ unstable: bool,
+}
+
+struct SortByKeyDetection {
+ vec_name: String,
+ closure_arg: String,
+ closure_body: String,
+ reverse: bool,
+ unstable: bool,
+}
+
+/// Detect if the two expressions are mirrored (identical, except one
+/// contains a and the other replaces it with b)
+fn mirrored_exprs(
+ cx: &LateContext<'_, '_>,
+ a_expr: &Expr<'_>,
+ a_ident: &Ident,
+ b_expr: &Expr<'_>,
+ b_ident: &Ident,
+) -> bool {
+ match (&a_expr.kind, &b_expr.kind) {
+ // Two boxes with mirrored contents
+ (ExprKind::Box(left_expr), ExprKind::Box(right_expr)) => {
+ mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
+ },
+ // Two arrays with mirrored contents
+ (ExprKind::Array(left_exprs), ExprKind::Array(right_exprs)) => left_exprs
+ .iter()
+ .zip(right_exprs.iter())
+ .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
+ // The two exprs are function calls.
+ // Check to see that the function itself and its arguments are mirrored
+ (ExprKind::Call(left_expr, left_args), ExprKind::Call(right_expr, right_args)) => {
+ mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
+ && left_args
+ .iter()
+ .zip(right_args.iter())
+ .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
+ },
+ // The two exprs are method calls.
+ // Check to see that the function is the same and the arguments are mirrored
+ // This is enough because the receiver of the method is listed in the arguments
+ (ExprKind::MethodCall(left_segment, _, left_args), ExprKind::MethodCall(right_segment, _, right_args)) => {
+ left_segment.ident == right_segment.ident
+ && left_args
+ .iter()
+ .zip(right_args.iter())
+ .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
+ },
+ // Two tuples with mirrored contents
+ (ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs)) => left_exprs
+ .iter()
+ .zip(right_exprs.iter())
+ .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
+ // Two binary ops, which are the same operation and which have mirrored arguments
+ (ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right)) => {
+ left_op.node == right_op.node
+ && mirrored_exprs(cx, left_left, a_ident, right_left, b_ident)
+ && mirrored_exprs(cx, left_right, a_ident, right_right, b_ident)
+ },
+ // Two unary ops, which are the same operation and which have the same argument
+ (ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr)) => {
+ left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
+ },
+ // The two exprs are literals of some kind
+ (ExprKind::Lit(left_lit), ExprKind::Lit(right_lit)) => left_lit.node == right_lit.node,
+ (ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
+ (ExprKind::DropTemps(left_block), ExprKind::DropTemps(right_block)) => {
+ mirrored_exprs(cx, left_block, a_ident, right_block, b_ident)
+ },
+ (ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident)) => {
+ left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident)
+ },
+ // Two paths: either one is a and the other is b, or they're identical to each other
+ (
+ ExprKind::Path(QPath::Resolved(
+ _,
+ Path {
+ segments: left_segments,
+ ..
+ },
+ )),
+ ExprKind::Path(QPath::Resolved(
+ _,
+ Path {
+ segments: right_segments,
+ ..
+ },
+ )),
+ ) => {
+ (left_segments
+ .iter()
+ .zip(right_segments.iter())
+ .all(|(left, right)| left.ident == right.ident)
+ && left_segments
+ .iter()
+ .all(|seg| &seg.ident != a_ident && &seg.ident != b_ident))
+ || (left_segments.len() == 1
+ && &left_segments[0].ident == a_ident
+ && right_segments.len() == 1
+ && &right_segments[0].ident == b_ident)
+ },
+ // Matching expressions, but one or both is borrowed
+ (
+ ExprKind::AddrOf(left_kind, Mutability::Not, left_expr),
+ ExprKind::AddrOf(right_kind, Mutability::Not, right_expr),
+ ) => left_kind == right_kind && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
+ (_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => {
+ mirrored_exprs(cx, a_expr, a_ident, right_expr, b_ident)
+ },
+ (ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(cx, left_expr, a_ident, b_expr, b_ident),
+ _ => false,
+ }
+}
+
+fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option {
+ if_chain! {
+ if let ExprKind::MethodCall(name_ident, _, args) = &expr.kind;
+ if let name = name_ident.ident.name.to_ident_string();
+ if name == "sort_by" || name == "sort_unstable_by";
+ if let [vec, Expr { kind: ExprKind::Closure(_, _, closure_body_id, _, _), .. }] = args;
+ 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(_, _, left_ident, _), .. }, ..},
+ Param { pat: Pat { kind: PatKind::Binding(_, _, right_ident, _), .. }, .. }
+ ] = &closure_body.params;
+ 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, 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(), false)
+ } else if mirrored_exprs(&cx, &left_expr, &right_ident, &right_expr, &left_ident) {
+ (Sugg::hir(cx, &left_expr, "..").to_string(), right_ident.name.to_string(), true)
+ } else {
+ return None;
+ };
+ let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
+ let unstable = name == "sort_unstable_by";
+ if_chain! {
+ if let ExprKind::Path(QPath::Resolved(_, Path {
+ segments: [PathSegment { ident: left_name, .. }], ..
+ })) = &left_expr.kind;
+ if left_name == left_ident;
+ then {
+ Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
+ }
+ else {
+ Some(LintTrigger::SortByKey(SortByKeyDetection {
+ vec_name,
+ unstable,
+ closure_arg,
+ closure_body,
+ reverse
+ }))
+ }
+ }
+ } else {
+ None
+ }
+ }
+}
+
+impl LateLintPass<'_, '_> for UnnecessarySortBy {
+ fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
+ match detect_lint(cx, expr) {
+ Some(LintTrigger::SortByKey(trigger)) => utils::span_lint_and_sugg(
+ cx,
+ UNNECESSARY_SORT_BY,
+ expr.span,
+ "use Vec::sort_by_key here instead",
+ "try",
+ format!(
+ "{}.sort{}_by_key(|&{}| {})",
+ trigger.vec_name,
+ if trigger.unstable { "_unstable" } else { "" },
+ trigger.closure_arg,
+ if trigger.reverse {
+ format!("Reverse({})", trigger.closure_body)
+ } else {
+ trigger.closure_body.to_string()
+ },
+ ),
+ if trigger.reverse {
+ Applicability::MaybeIncorrect
+ } else {
+ Applicability::MachineApplicable
+ },
+ ),
+ Some(LintTrigger::Sort(trigger)) => utils::span_lint_and_sugg(
+ cx,
+ UNNECESSARY_SORT_BY,
+ expr.span,
+ "use Vec::sort here instead",
+ "try",
+ format!(
+ "{}.sort{}()",
+ trigger.vec_name,
+ if trigger.unstable { "_unstable" } else { "" },
+ ),
+ Applicability::MachineApplicable,
+ ),
+ None => {},
+ }
+ }
+}
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index 69578732898f..ab9542a7b9c8 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -2292,6 +2292,13 @@ pub static ref ALL_LINTS: Vec = vec![
deprecation: None,
module: "no_effect",
},
+ Lint {
+ name: "unnecessary_sort_by",
+ group: "complexity",
+ desc: "Use of `Vec::sort_by` when `Vec::sort_by_key` or `Vec::sort` would be clearer",
+ deprecation: None,
+ module: "unnecessary_sort_by",
+ },
Lint {
name: "unnecessary_unwrap",
group: "complexity",
diff --git a/tests/ui/unnecessary_sort_by.fixed b/tests/ui/unnecessary_sort_by.fixed
new file mode 100644
index 000000000000..779fd57707ad
--- /dev/null
+++ b/tests/ui/unnecessary_sort_by.fixed
@@ -0,0 +1,26 @@
+// run-rustfix
+
+use std::cmp::Reverse;
+
+fn id(x: isize) -> isize {
+ x
+}
+
+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_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_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_unstable_by(|a, _| a.cmp(c));
+}
diff --git a/tests/ui/unnecessary_sort_by.rs b/tests/ui/unnecessary_sort_by.rs
new file mode 100644
index 000000000000..0485a5630afe
--- /dev/null
+++ b/tests/ui/unnecessary_sort_by.rs
@@ -0,0 +1,26 @@
+// run-rustfix
+
+use std::cmp::Reverse;
+
+fn id(x: isize) -> isize {
+ x
+}
+
+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_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_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_unstable_by(|a, _| a.cmp(c));
+}
diff --git a/tests/ui/unnecessary_sort_by.stderr b/tests/ui/unnecessary_sort_by.stderr
new file mode 100644
index 000000000000..903b6e5099ce
--- /dev/null
+++ b/tests/ui/unnecessary_sort_by.stderr
@@ -0,0 +1,46 @@
+error: use Vec::sort here instead
+ --> $DIR/unnecessary_sort_by.rs:12:5
+ |
+LL | vec.sort_by(|a, b| a.cmp(b));
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort()`
+ |
+ = note: `-D clippy::unnecessary-sort-by` implied by `-D warnings`
+
+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:15:5
+ |
+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: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: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:19:5
+ |
+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 7 previous errors
+