Auto merge of #4569 - james9909:add-comparison-chain, r=oli-obk

Add a new lint for comparison chains

changelog: Adds a new lint: `comparison_chain`.

`comparison_chain` lints all `if` conditional chains where all the conditions are binary comparisons on the same two operands and will suggest a rewrite with `match`.

Closes #4531.

Author: bors

CHANGELOG.md

@@ -962,6 +962,7 @@ Released 2018-09-13
 [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned
 [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
 [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
+[`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
 [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
 [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute
 [`dbg_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 316 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 317 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/comparison_chain.rs

@@ -0,0 +1,107 @@
+use crate::utils::{if_sequence, parent_node_is_if_expr, span_help_and_lint, SpanlessEq};
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks comparison chains written with `if` that can be
+    /// rewritten with `match` and `cmp`.
+    ///
+    /// **Why is this bad?** `if` is not guaranteed to be exhaustive and conditionals can get
+    /// repetitive
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust,ignore
+    /// # fn a() {}
+    /// # fn b() {}
+    /// # fn c() {}
+    /// fn f(x: u8, y: u8) {
+    ///     if x > y {
+    ///         a()
+    ///     } else if x < y {
+    ///         b()
+    ///     } else {
+    ///         c()
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// Could be written:
+    ///
+    /// ```rust,ignore
+    /// use std::cmp::Ordering;
+    /// # fn a() {}
+    /// # fn b() {}
+    /// # fn c() {}
+    /// fn f(x: u8, y: u8) {
+    ///      match x.cmp(&y) {
+    ///          Ordering::Greater => a(),
+    ///          Ordering::Less => b(),
+    ///          Ordering::Equal => c()
+    ///      }
+    /// }
+    /// ```
+    pub COMPARISON_CHAIN,
+    style,
+    "`if`s that can be rewritten with `match` and `cmp`"
+}
+
+declare_lint_pass!(ComparisonChain => [COMPARISON_CHAIN]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ComparisonChain {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if expr.span.from_expansion() {
+            return;
+        }
+
+        // We only care about the top-most `if` in the chain
+        if parent_node_is_if_expr(expr, cx) {
+            return;
+        }
+
+        // Check that there exists at least one explicit else condition
+        let (conds, _) = if_sequence(expr);
+        if conds.len() < 2 {
+            return;
+        }
+
+        for cond in conds.windows(2) {
+            if let (
+                &ExprKind::Binary(ref kind1, ref lhs1, ref rhs1),
+                &ExprKind::Binary(ref kind2, ref lhs2, ref rhs2),
+            ) = (&cond[0].node, &cond[1].node)
+            {
+                if !kind_is_cmp(kind1.node) || !kind_is_cmp(kind2.node) {
+                    return;
+                }
+
+                // Check that both sets of operands are equal
+                let mut spanless_eq = SpanlessEq::new(cx);
+                if (!spanless_eq.eq_expr(lhs1, lhs2) || !spanless_eq.eq_expr(rhs1, rhs2))
+                    && (!spanless_eq.eq_expr(lhs1, rhs2) || !spanless_eq.eq_expr(rhs1, lhs2))
+                {
+                    return;
+                }
+            } else {
+                // We only care about comparison chains
+                return;
+            }
+        }
+        span_help_and_lint(
+            cx,
+            COMPARISON_CHAIN,
+            expr.span,
+            "`if` chain can be rewritten with `match`",
+            "Consider rewriting the `if` chain to use `cmp` and `match`.",
+        )
+    }
+}
+
+fn kind_is_cmp(kind: BinOpKind) -> bool {
+    match kind {
+        BinOpKind::Lt | BinOpKind::Gt | BinOpKind::Eq => true,
+        _ => false,
+    }
+}

clippy_lints/src/copies.rs

@@ -1,11 +1,11 @@
-use crate::utils::{get_parent_expr, higher, same_tys, snippet, span_lint_and_then, span_note_and_lint};
+use crate::utils::{get_parent_expr, higher, if_sequence, same_tys, snippet, span_lint_and_then, span_note_and_lint};
 use crate::utils::{SpanlessEq, SpanlessHash};
 use rustc::hir::*;
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::ty::Ty;
 use rustc::{declare_lint_pass, declare_tool_lint};
 use rustc_data_structures::fx::FxHashMap;
-use smallvec::SmallVec;
+use std::cmp::Ordering;
 use std::collections::hash_map::Entry;
 use std::hash::BuildHasherDefault;
 use syntax::symbol::Symbol;
@@ -236,39 +236,6 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) {
     }
 }
 
-/// Returns the list of condition expressions and the list of blocks in a
-/// sequence of `if/else`.
-/// E.g., this returns `([a, b], [c, d, e])` for the expression
-/// `if a { c } else if b { d } else { e }`.
-fn if_sequence(mut expr: &Expr) -> (SmallVec<[&Expr; 1]>, SmallVec<[&Block; 1]>) {
-    let mut conds = SmallVec::new();
-    let mut blocks: SmallVec<[&Block; 1]> = SmallVec::new();
-
-    while let Some((ref cond, ref then_expr, ref else_expr)) = higher::if_block(&expr) {
-        conds.push(&**cond);
-        if let ExprKind::Block(ref block, _) = then_expr.node {
-            blocks.push(block);
-        } else {
-            panic!("ExprKind::If node is not an ExprKind::Block");
-        }
-
-        if let Some(ref else_expr) = *else_expr {
-            expr = else_expr;
-        } else {
-            break;
-        }
-    }
-
-    // final `else {..}`
-    if !blocks.is_empty() {
-        if let ExprKind::Block(ref block, _) = expr.node {
-            blocks.push(&**block);
-        }
-    }
-
-    (conds, blocks)
-}
-
 /// Returns the list of bindings in a pattern.
 fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> FxHashMap<Symbol, Ty<'tcx>> {
     fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut FxHashMap<Symbol, Ty<'tcx>>) {
@@ -333,14 +300,8 @@ fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a
 where
     Eq: Fn(&T, &T) -> bool,
 {
-    if exprs.len() < 2 {
-        None
-    } else if exprs.len() == 2 {
-        if eq(&exprs[0], &exprs[1]) {
-            Some((&exprs[0], &exprs[1]))
-        } else {
-            None
-        }
+    if exprs.len() == 2 && eq(&exprs[0], &exprs[1]) {
+        Some((&exprs[0], &exprs[1]))
     } else {
         None
     }

clippy_lints/src/lib.rs

@@ -159,6 +159,7 @@ pub mod cargo_common_metadata;
 pub mod checked_conversions;
 pub mod cognitive_complexity;
 pub mod collapsible_if;
+pub mod comparison_chain;
 pub mod copies;
 pub mod copy_iterator;
 pub mod dbg_macro;
@@ -600,6 +601,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
     reg.register_late_lint_pass(box integer_division::IntegerDivision);
     reg.register_late_lint_pass(box inherent_to_string::InherentToString);
     reg.register_late_lint_pass(box trait_bounds::TraitBounds);
+    reg.register_late_lint_pass(box comparison_chain::ComparisonChain);
 
     reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -706,6 +708,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
         bytecount::NAIVE_BYTECOUNT,
         cognitive_complexity::COGNITIVE_COMPLEXITY,
         collapsible_if::COLLAPSIBLE_IF,
+        comparison_chain::COMPARISON_CHAIN,
         copies::IFS_SAME_COND,
         copies::IF_SAME_THEN_ELSE,
         derive::DERIVE_HASH_XOR_EQ,
@@ -932,6 +935,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
         block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR,
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         collapsible_if::COLLAPSIBLE_IF,
+        comparison_chain::COMPARISON_CHAIN,
         doc::MISSING_SAFETY_DOC,
         enum_variants::ENUM_VARIANT_NAMES,
         enum_variants::MODULE_INCEPTION,

clippy_lints/src/needless_bool.rs

@@ -3,7 +3,7 @@
 //! This lint is **warn** by default
 
 use crate::utils::sugg::Sugg;
-use crate::utils::{higher, span_lint, span_lint_and_sugg};
+use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_sugg};
 use rustc::hir::*;
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_lint_pass, declare_tool_lint};
@@ -118,17 +118,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBool {
     }
 }
 
-fn parent_node_is_if_expr<'a, 'b>(expr: &Expr, cx: &LateContext<'a, 'b>) -> bool {
-    let parent_id = cx.tcx.hir().get_parent_node(expr.hir_id);
-    let parent_node = cx.tcx.hir().get(parent_id);
-
-    match parent_node {
-        rustc::hir::Node::Expr(e) => higher::if_block(&e).is_some(),
-        rustc::hir::Node::Arm(e) => higher::if_block(&e.body).is_some(),
-        _ => false,
-    }
-}
-
 declare_lint_pass!(BoolComparison => [BOOL_COMPARISON]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison {

clippy_lints/src/non_expressive_names.rs

@@ -1,6 +1,7 @@
 use crate::utils::{span_lint, span_lint_and_then};
 use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
 use rustc::{declare_tool_lint, impl_lint_pass};
+use std::cmp::Ordering;
 use syntax::ast::*;
 use syntax::attr;
 use syntax::source_map::Span;
@@ -206,63 +207,67 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
                 continue;
             }
             let mut split_at = None;
-            if existing_name.len > count {
-                if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) {
-                    continue;
-                }
-            } else if existing_name.len < count {
-                if count - existing_name.len != 1 || levenstein_not_1(&existing_name.interned, &interned_name) {
-                    continue;
-                }
-            } else {
-                let mut interned_chars = interned_name.chars();
-                let mut existing_chars = existing_name.interned.chars();
-                let first_i = interned_chars.next().expect("we know we have at least one char");
-                let first_e = existing_chars.next().expect("we know we have at least one char");
-                let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric();
+            match existing_name.len.cmp(&count) {
+                Ordering::Greater => {
+                    if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) {
+                        continue;
+                    }
+                },
+                Ordering::Less => {
+                    if count - existing_name.len != 1 || levenstein_not_1(&existing_name.interned, &interned_name) {
+                        continue;
+                    }
+                },
+                Ordering::Equal => {
+                    let mut interned_chars = interned_name.chars();
+                    let mut existing_chars = existing_name.interned.chars();
+                    let first_i = interned_chars.next().expect("we know we have at least one char");
+                    let first_e = existing_chars.next().expect("we know we have at least one char");
+                    let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric();
 
-                if eq_or_numeric((first_i, first_e)) {
-                    let last_i = interned_chars.next_back().expect("we know we have at least two chars");
-                    let last_e = existing_chars.next_back().expect("we know we have at least two chars");
-                    if eq_or_numeric((last_i, last_e)) {
-                        if interned_chars
-                            .zip(existing_chars)
-                            .filter(|&ie| !eq_or_numeric(ie))
-                            .count()
-                            != 1
-                        {
-                            continue;
+                    if eq_or_numeric((first_i, first_e)) {
+                        let last_i = interned_chars.next_back().expect("we know we have at least two chars");
+                        let last_e = existing_chars.next_back().expect("we know we have at least two chars");
+                        if eq_or_numeric((last_i, last_e)) {
+                            if interned_chars
+                                .zip(existing_chars)
+                                .filter(|&ie| !eq_or_numeric(ie))
+                                .count()
+                                != 1
+                            {
+                                continue;
+                            }
+                        } else {
+                            let second_last_i = interned_chars
+                                .next_back()
+                                .expect("we know we have at least three chars");
+                            let second_last_e = existing_chars
+                                .next_back()
+                                .expect("we know we have at least three chars");
+                            if !eq_or_numeric((second_last_i, second_last_e))
+                                || second_last_i == '_'
+                                || !interned_chars.zip(existing_chars).all(eq_or_numeric)
+                            {
+                                // allowed similarity foo_x, foo_y
+                                // or too many chars differ (foo_x, boo_y) or (foox, booy)
+                                continue;
+                            }
+                            split_at = interned_name.char_indices().rev().next().map(|(i, _)| i);
                         }
                     } else {
-                        let second_last_i = interned_chars
-                            .next_back()
-                            .expect("we know we have at least three chars");
-                        let second_last_e = existing_chars
-                            .next_back()
-                            .expect("we know we have at least three chars");
-                        if !eq_or_numeric((second_last_i, second_last_e))
-                            || second_last_i == '_'
+                        let second_i = interned_chars.next().expect("we know we have at least two chars");
+                        let second_e = existing_chars.next().expect("we know we have at least two chars");
+                        if !eq_or_numeric((second_i, second_e))
+                            || second_i == '_'
                             || !interned_chars.zip(existing_chars).all(eq_or_numeric)
                         {
-                            // allowed similarity foo_x, foo_y
-                            // or too many chars differ (foo_x, boo_y) or (foox, booy)
+                            // allowed similarity x_foo, y_foo
+                            // or too many chars differ (x_foo, y_boo) or (xfoo, yboo)
                             continue;
                         }
-                        split_at = interned_name.char_indices().rev().next().map(|(i, _)| i);
+                        split_at = interned_name.chars().next().map(char::len_utf8);
                     }
-                } else {
-                    let second_i = interned_chars.next().expect("we know we have at least two chars");
-                    let second_e = existing_chars.next().expect("we know we have at least two chars");
-                    if !eq_or_numeric((second_i, second_e))
-                        || second_i == '_'
-                        || !interned_chars.zip(existing_chars).all(eq_or_numeric)
-                    {
-                        // allowed similarity x_foo, y_foo
-                        // or too many chars differ (x_foo, y_boo) or (xfoo, yboo)
-                        continue;
-                    }
-                    split_at = interned_name.chars().next().map(char::len_utf8);
-                }
+                },
             }
             span_lint_and_then(
                 self.0.cx,