Merge remote-tracking branch 'popzxc/if-same-cond-fn' into rollup-new-lints

CHANGELOG.md

@@ -1030,6 +1030,7 @@ Released 2018-09-13
 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
+[`ifs_same_cond_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond_fn
 [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return
 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping

clippy_lints/src/copies.rs

@@ -40,6 +40,53 @@ declare_clippy_lint! {
     "consecutive `ifs` with the same condition"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for consecutive `if`s with the same function call.
+    ///
+    /// **Why is this bad?** This is probably a copy & paste error.
+    /// Despite the fact that function can have side effects and `if` works as
+    /// intended, such an approach is implicit and can be considered a "code smell".
+    ///
+    /// **Known problems:** Hopefully none.
+    ///
+    /// **Example:**
+    /// ```ignore
+    /// if foo() == bar {
+    ///     …
+    /// } else if foo() == bar {
+    ///     …
+    /// }
+    /// ```
+    ///
+    /// This probably should be:
+    /// ```ignore
+    /// if foo() == bar {
+    ///     …
+    /// } else if foo() == baz {
+    ///     …
+    /// }
+    /// ```
+    ///
+    /// or if the original code was not a typo and called function mutates a state,
+    /// consider move the mutation out of the `if` condition to avoid similarity to
+    /// a copy & paste error:
+    ///
+    /// ```ignore
+    /// let first = foo();
+    /// if first == bar {
+    ///     …
+    /// } else {
+    ///     let second = foo();
+    ///     if second == bar {
+    ///     …
+    ///     }
+    /// }
+    /// ```
+    pub SAME_FUNCTIONS_IN_IF_CONDITION,
+    pedantic,
+    "consecutive `ifs` with the same function call"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for `if/else` with the same body as the *then* part
     /// and the *else* part.
@@ -102,7 +149,7 @@ declare_clippy_lint! {
     "`match` with identical arm bodies"
 }
 
-declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
+declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
@@ -119,6 +166,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste {
             let (conds, blocks) = if_sequence(expr);
             lint_same_then_else(cx, &blocks);
             lint_same_cond(cx, &conds);
+            lint_same_fns_in_if_cond(cx, &conds);
             lint_match_arms(cx, expr);
         }
     }
@@ -163,6 +211,34 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
     }
 }
 
+/// Implementation of `SAME_FUNCTIONS_IN_IF_CONDITION`.
+fn lint_same_fns_in_if_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
+    let hash: &dyn Fn(&&Expr) -> u64 = &|expr| -> u64 {
+        let mut h = SpanlessHash::new(cx, cx.tables);
+        h.hash_expr(expr);
+        h.finish()
+    };
+
+    let eq: &dyn Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool {
+        // Do not spawn warning if `IFS_SAME_COND` already produced it.
+        if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) {
+            return false;
+        }
+        SpanlessEq::new(cx).eq_expr(lhs, rhs)
+    };
+
+    for (i, j) in search_same(conds, hash, eq) {
+        span_note_and_lint(
+            cx,
+            SAME_FUNCTIONS_IN_IF_CONDITION,
+            j.span,
+            "this `if` has the same function call as a previous if",
+            i.span,
+            "same as this",
+        );
+    }
+}
+
 /// Implementation of `MATCH_SAME_ARMS`.
 fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) {
     fn same_bindings<'tcx>(

clippy_lints/src/lib.rs

@@ -470,6 +470,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         &collapsible_if::COLLAPSIBLE_IF,
         &comparison_chain::COMPARISON_CHAIN,
         &copies::IFS_SAME_COND,
+        &copies::SAME_FUNCTIONS_IN_IF_CONDITION,
         &copies::IF_SAME_THEN_ELSE,
         &copies::MATCH_SAME_ARMS,
         &copy_iterator::COPY_ITERATOR,
@@ -989,6 +990,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
     store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
         LintId::of(&attrs::INLINE_ALWAYS),
         LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
+        LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),
         LintId::of(&copies::MATCH_SAME_ARMS),
         LintId::of(&copy_iterator::COPY_ITERATOR),
         LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS),

src/lintlist/mod.rs

@@ -714,6 +714,13 @@ pub const ALL_LINTS: [Lint; 334] = [
         deprecation: None,
         module: "copies",
     },
+    Lint {
+        name: "ifs_same_cond_fn",
+        group: "pedantic",
+        desc: "consecutive `ifs` with the same function call",
+        deprecation: None,
+        module: "copies",
+    },
     Lint {
         name: "implicit_hasher",
         group: "style",

tests/ui/same_functions_in_if_condition.rs

@@ -0,0 +1,80 @@
+#![warn(clippy::same_functions_in_if_condition)]
+#![allow(clippy::ifs_same_cond)] // This warning is different from `ifs_same_cond`.
+#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks
+
+fn function() -> bool {
+    true
+}
+
+fn fn_arg(_arg: u8) -> bool {
+    true
+}
+
+struct Struct;
+
+impl Struct {
+    fn method(&self) -> bool {
+        true
+    }
+    fn method_arg(&self, _arg: u8) -> bool {
+        true
+    }
+}
+
+fn ifs_same_cond_fn() {
+    let a = 0;
+    let obj = Struct;
+
+    if function() {
+    } else if function() {
+        //~ ERROR ifs same condition
+    }
+
+    if fn_arg(a) {
+    } else if fn_arg(a) {
+        //~ ERROR ifs same condition
+    }
+
+    if obj.method() {
+    } else if obj.method() {
+        //~ ERROR ifs same condition
+    }
+
+    if obj.method_arg(a) {
+    } else if obj.method_arg(a) {
+        //~ ERROR ifs same condition
+    }
+
+    let mut v = vec![1];
+    if v.pop() == None {
+        //~ ERROR ifs same condition
+    } else if v.pop() == None {
+    }
+
+    if v.len() == 42 {
+        //~ ERROR ifs same condition
+    } else if v.len() == 42 {
+    }
+
+    if v.len() == 1 {
+        // ok, different conditions
+    } else if v.len() == 2 {
+    }
+
+    if fn_arg(0) {
+        // ok, different arguments.
+    } else if fn_arg(1) {
+    }
+
+    if obj.method_arg(0) {
+        // ok, different arguments.
+    } else if obj.method_arg(1) {
+    }
+
+    if a == 1 {
+        // ok, warning is on `ifs_same_cond` behalf.
+    } else if a == 1 {
+    }
+}
+
+fn main() {}

tests/ui/same_functions_in_if_condition.stderr

@@ -0,0 +1,75 @@
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:29:15
+   |
+LL |     } else if function() {
+   |               ^^^^^^^^^^
+   |
+   = note: `-D clippy::same-functions-in-if-condition` implied by `-D warnings`
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:28:8
+   |
+LL |     if function() {
+   |        ^^^^^^^^^^
+
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:34:15
+   |
+LL |     } else if fn_arg(a) {
+   |               ^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:33:8
+   |
+LL |     if fn_arg(a) {
+   |        ^^^^^^^^^
+
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:39:15
+   |
+LL |     } else if obj.method() {
+   |               ^^^^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:38:8
+   |
+LL |     if obj.method() {
+   |        ^^^^^^^^^^^^
+
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:44:15
+   |
+LL |     } else if obj.method_arg(a) {
+   |               ^^^^^^^^^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:43:8
+   |
+LL |     if obj.method_arg(a) {
+   |        ^^^^^^^^^^^^^^^^^
+
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:51:15
+   |
+LL |     } else if v.pop() == None {
+   |               ^^^^^^^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:49:8
+   |
+LL |     if v.pop() == None {
+   |        ^^^^^^^^^^^^^^^
+
+error: this `if` has the same function call as a previous if
+  --> $DIR/same_functions_in_if_condition.rs:56:15
+   |
+LL |     } else if v.len() == 42 {
+   |               ^^^^^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/same_functions_in_if_condition.rs:54:8
+   |
+LL |     if v.len() == 42 {
+   |        ^^^^^^^^^^^^^
+
+error: aborting due to 6 previous errors
+