New lint: Use Reverse with sort_by_key

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

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),

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<A> = 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<A> = 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<LintTrigger> {
+    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 => {},
+        }
+    }
+}

src/lintlist/mod.rs

@@ -2292,6 +2292,13 @@ pub static ref ALL_LINTS: Vec<Lint> = 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",

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<isize> = 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));
+}

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<isize> = 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));
+}