A new lint for shared code in if blocks

* WIP working front and and detection
* A working lint
* Fixed the last test
* Cleaning up some code
* Fixed another test
* Ret renamed the lint
* Formatting

CHANGELOG.md

@@ -2049,6 +2049,7 @@ Released 2018-09-13
 [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse
 [`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same
 [`shadow_unrelated`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_unrelated
+[`shared_code_in_if_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#shared_code_in_if_blocks
 [`short_circuit_statement`]: https://rust-lang.github.io/rust-clippy/master/index.html#short_circuit_statement
 [`should_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_assert_eq
 [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait

clippy_lints/src/copies.rs

@@ -1,8 +1,9 @@
-use crate::utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
-use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_note};
+use crate::utils::{both, count_eq, eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
+use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_help, span_lint_and_note};
 use rustc_hir::{Block, Expr};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::source_map::Span;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for consecutive `if`s with the same condition.
@@ -103,7 +104,40 @@ declare_clippy_lint! {
     "`if` with the same `then` and `else` blocks"
 }
 
-declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE]);
+declare_clippy_lint! {
+    /// **What it does:** Checks if the `if` and `else` block contain shared that can be
+    /// moved out of the blocks.
+    ///
+    /// **Why is this bad?** Duplicate code is less maintainable.
+    ///
+    /// **Known problems:** Hopefully none.
+    ///
+    /// **Example:**
+    /// ```ignore
+    /// let foo = if … {
+    ///     println!("Hello World");
+    ///     13
+    /// } else {
+    ///     println!("Hello World");
+    ///     42
+    /// };
+    /// ```
+    ///
+    /// Could be written as:
+    /// ```ignore
+    /// println!("Hello World");
+    /// let foo = if … {
+    ///     13
+    /// } else {
+    ///     42
+    /// };
+    /// ```
+    pub SHARED_CODE_IN_IF_BLOCKS,
+    complexity,
+    "`if` statement with shared code in all blocks"
+}
+
+declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, SHARED_CODE_IN_IF_BLOCKS]);
 
 impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
@@ -118,28 +152,112 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
             }
 
             let (conds, blocks) = if_sequence(expr);
-            lint_same_then_else(cx, &blocks);
+            // Conditions
             lint_same_cond(cx, &conds);
             lint_same_fns_in_if_cond(cx, &conds);
+            // Block duplication
+            lint_same_then_else(cx, &blocks, conds.len() != blocks.len());
         }
     }
 }
 
-/// Implementation of `IF_SAME_THEN_ELSE`.
-fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>]) {
-    let eq: &dyn Fn(&&Block<'_>, &&Block<'_>) -> bool =
-        &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).eq_block(lhs, rhs) };
+/// Implementation of `SHARED_CODE_IN_IF_BLOCKS` and `IF_SAME_THEN_ELSE` if the blocks are equal.
+fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_unconditional_else: bool) {
+    /// This retrieves the span of the actual call site.
+    fn get_source_span(span: Span) -> Span {
+        if span.from_expansion() {
+            span.source_callee().unwrap().call_site
+        } else {
+            span
+        }
+    }
 
-    if let Some((i, j)) = search_same_sequenced(blocks, eq) {
-        span_lint_and_note(
+    fn min(a: usize, b: usize) -> usize {
+        if a < b {
+            a
+        } else {
+            b
+        }
+    }
+
+    fn lint_duplicate_code(cx: &LateContext<'_>, position: &str, lint_span: Span) {
+        span_lint_and_help(
             cx,
-            IF_SAME_THEN_ELSE,
-            j.span,
-            "this `if` has identical blocks",
-            Some(i.span),
-            "same as this",
+            SHARED_CODE_IN_IF_BLOCKS,
+            lint_span,
+            format!("All if blocks contain the same code at the {}", position).as_str(),
+            None,
+            "Consider moving the code out of the if statement to prevent code duplication",
         );
     }
+
+    // We only lint ifs with multiple blocks
+    if blocks.len() < 2 {
+        return;
+    }
+
+    // Check if each block has shared code
+    let mut start_eq = usize::MAX;
+    let mut end_eq = usize::MAX;
+    for (index, win) in blocks.windows(2).enumerate() {
+        let l_stmts = win[0].stmts;
+        let r_stmts = win[1].stmts;
+
+        let mut evaluator = SpanlessEq::new(cx);
+        let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r));
+        let current_end_eq = count_eq(&mut l_stmts.iter().rev(), &mut r_stmts.iter().rev(), |l, r| {
+            evaluator.eq_stmt(l, r)
+        });
+        let current_block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
+
+        // IF_SAME_THEN_ELSE
+        // We only lint the first two blocks (index == 0). Further blocks will be linted when that if
+        // statement is checked
+        if index == 0 && current_block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq {
+            span_lint_and_note(
+                cx,
+                IF_SAME_THEN_ELSE,
+                win[0].span,
+                "this `if` has identical blocks",
+                Some(win[1].span),
+                "same as this",
+            );
+
+            return;
+        }
+
+        start_eq = min(start_eq, current_start_eq);
+        end_eq = min(end_eq, current_end_eq);
+
+        // We can return if the eq count is 0 from both sides or if it has no unconditional else case
+        if !has_unconditional_else || start_eq == 0 && end_eq == 0 {
+            return;
+        };
+    }
+
+    let first_block = blocks[0];
+
+    // prevent double lint if the `start_eq` and `end_eq` cover the entire block
+    if start_eq == first_block.stmts.len() {
+        end_eq = 0;
+    }
+
+    if start_eq != 0 {
+        let start = first_block.span.shrink_to_lo();
+        let end = get_source_span(first_block.stmts[start_eq - 1].span);
+        let lint_span = start.to(end);
+
+        lint_duplicate_code(cx, "start", lint_span);
+    }
+
+    if end_eq != 0 {
+        let index = first_block.stmts.len() - end_eq;
+        let start = get_source_span(first_block.stmts[index].span);
+        let end = first_block.span.shrink_to_hi();
+        let lint_span = start.to(end);
+
+        lint_duplicate_code(cx, "end", lint_span);
+    }
 }
 
 /// Implementation of `IFS_SAME_COND`.
@@ -195,15 +313,3 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) {
         );
     }
 }
-
-fn search_same_sequenced<T, Eq>(exprs: &[T], eq: Eq) -> Option<(&T, &T)>
-where
-    Eq: Fn(&T, &T) -> bool,
-{
-    for win in exprs.windows(2) {
-        if eq(&win[0], &win[1]) {
-            return Some((&win[0], &win[1]));
-        }
-    }
-    None
-}

clippy_lints/src/lib.rs

@@ -558,6 +558,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &copies::IFS_SAME_COND,
         &copies::IF_SAME_THEN_ELSE,
         &copies::SAME_FUNCTIONS_IN_IF_CONDITION,
+        &copies::SHARED_CODE_IN_IF_BLOCKS,
         &copy_iterator::COPY_ITERATOR,
         &create_dir::CREATE_DIR,
         &dbg_macro::DBG_MACRO,
@@ -1381,6 +1382,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&comparison_chain::COMPARISON_CHAIN),
         LintId::of(&copies::IFS_SAME_COND),
         LintId::of(&copies::IF_SAME_THEN_ELSE),
+        LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS),
         LintId::of(&default::FIELD_REASSIGN_WITH_DEFAULT),
         LintId::of(&derive::DERIVE_HASH_XOR_EQ),
         LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
@@ -1751,6 +1753,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&assign_ops::MISREFACTORED_ASSIGN_OP),
         LintId::of(&attrs::DEPRECATED_CFG_ATTR),
         LintId::of(&booleans::NONMINIMAL_BOOL),
+        LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS),
         LintId::of(&double_comparison::DOUBLE_COMPARISONS),
         LintId::of(&double_parens::DOUBLE_PARENS),
         LintId::of(&duration_subsec::DURATION_SUBSEC),

clippy_lints/src/utils/hir_utils.rs

@@ -368,6 +368,15 @@ pub fn over<X>(left: &[X], right: &[X], mut eq_fn: impl FnMut(&X, &X) -> bool) -
     left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y))
 }
 
+/// Counts how many elements of the slices are equal as per `eq_fn`.
+pub fn count_eq<X: Sized>(
+    left: &mut dyn Iterator<Item = X>,
+    right: &mut dyn Iterator<Item = X>,
+    mut eq_fn: impl FnMut(&X, &X) -> bool,
+) -> usize {
+    left.zip(right).take_while(|(l, r)| eq_fn(l, r)).count()
+}
+
 /// Checks if two expressions evaluate to the same value, and don't contain any side effects.
 pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> bool {
     SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right)

clippy_lints/src/utils/mod.rs

@@ -26,7 +26,7 @@ pub mod visitors;
 
 pub use self::attrs::*;
 pub use self::diagnostics::*;
-pub use self::hir_utils::{both, eq_expr_value, over, SpanlessEq, SpanlessHash};
+pub use self::hir_utils::{both, count_eq, eq_expr_value, over, SpanlessEq, SpanlessHash};
 
 use std::borrow::Cow;
 use std::collections::hash_map::Entry;

tests/ui/checked_unwrap/complex_conditionals.rs

@@ -1,5 +1,5 @@
 #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
-#![allow(clippy::if_same_then_else)]
+#![allow(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)]
 
 fn test_complex_conditions() {
     let x: Result<(), ()> = Ok(());

tests/ui/if_same_then_else.rs

@@ -5,7 +5,8 @@
     clippy::never_loop,
     clippy::no_effect,
     clippy::unused_unit,
-    clippy::zero_divided_by_zero
+    clippy::zero_divided_by_zero,
+    clippy::shared_code_in_if_blocks
 )]
 
 struct Foo {