Updated code for dogfood

Author: xFrednet

clippy_lints/src/copies.rs

@@ -1,8 +1,9 @@
-use crate::utils::{both, count_eq, eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
 use crate::utils::{
-    first_line_of_span, get_enclosing_block, get_parent_expr, if_sequence, indent_of, parent_node_is_if_expr,
-    reindent_multiline, snippet, snippet_opt, span_lint_and_note, span_lint_and_then, ContainsName,
+    both, count_eq, eq_expr_value, first_line_of_span, get_enclosing_block, get_parent_expr, if_sequence, in_macro,
+    indent_of, parent_node_is_if_expr, reindent_multiline, run_lints, search_same, snippet, snippet_opt,
+    span_lint_and_note, span_lint_and_then, ContainsName, SpanlessEq, SpanlessHash,
 };
+use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
@@ -188,57 +189,15 @@ fn lint_same_then_else<'tcx>(
         return;
     }
 
-    let has_expr = blocks[0].expr.is_some();
-
     // Check if each block has shared code
-    let mut start_eq = usize::MAX;
-    let mut end_eq = usize::MAX;
-    let mut expr_eq = true;
-    for win in blocks.windows(2) {
-        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 block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
-
-        // IF_SAME_THEN_ELSE
-        if 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 = start_eq.min(current_start_eq);
-        end_eq = end_eq.min(current_end_eq);
-        expr_eq &= block_expr_eq;
-    }
+    let has_expr = blocks[0].expr.is_some();
+    let (start_eq, mut end_eq, expr_eq) = scan_block_for_eq(cx, blocks);
 
     // SHARED_CODE_IN_IF_BLOCKS prerequisites
     if !has_unconditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) {
         return;
     }
 
-    if has_expr && !expr_eq {
-        end_eq = 0;
-    }
-
-    // Check if the regions are overlapping. Set `end_eq` to prevent the overlap
-    let min_block_size = blocks.iter().map(|x| x.stmts.len()).min().unwrap();
-    if (start_eq + end_eq) > min_block_size {
-        end_eq = min_block_size - start_eq;
-    }
-
     // Only the start is the same
     if start_eq != 0 && end_eq == 0 && (!has_expr || !expr_eq) {
         let block = blocks[0];
@@ -302,14 +261,13 @@ fn lint_same_then_else<'tcx>(
             end_eq -= moved_start;
         }
 
-        let end_linable = if let Some(expr) = block.expr {
-            intravisit::walk_expr(&mut end_walker, expr);
-            end_walker.uses.iter().any(|x| !block_defs.contains(x))
-        } else if end_eq == 0 {
-            false
-        } else {
-            true
-        };
+        let end_linable = block.expr.map_or_else(
+            || end_eq != 0,
+            |expr| {
+                intravisit::walk_expr(&mut end_walker, expr);
+                end_walker.uses.iter().any(|x| !block_defs.contains(x))
+            },
+        );
 
         if end_linable {
             end_walker.def_symbols.drain().for_each(|x| {
@@ -329,13 +287,67 @@ fn lint_same_then_else<'tcx>(
     }
 }
 
+fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, usize, bool) {
+    let mut start_eq = usize::MAX;
+    let mut end_eq = usize::MAX;
+    let mut expr_eq = true;
+    for win in blocks.windows(2) {
+        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 block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
+
+        // IF_SAME_THEN_ELSE
+        if_chain! {
+            if block_expr_eq;
+            if l_stmts.len() == r_stmts.len();
+            if l_stmts.len() == current_start_eq;
+            if run_lints(cx, &[IF_SAME_THEN_ELSE], win[0].hir_id);
+            if run_lints(cx, &[IF_SAME_THEN_ELSE], win[1].hir_id);
+            then {
+                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 (0, 0, false);
+            }
+        }
+
+        start_eq = start_eq.min(current_start_eq);
+        end_eq = end_eq.min(current_end_eq);
+        expr_eq &= block_expr_eq;
+    }
+
+    let has_expr = blocks[0].expr.is_some();
+    if has_expr && !expr_eq {
+        end_eq = 0;
+    }
+
+    // Check if the regions are overlapping. Set `end_eq` to prevent the overlap
+    let min_block_size = blocks.iter().map(|x| x.stmts.len()).min().unwrap();
+    if (start_eq + end_eq) > min_block_size {
+        end_eq = min_block_size - start_eq;
+    }
+
+    (start_eq, end_eq, expr_eq)
+}
+
 fn check_for_warn_of_moved_symbol(
     cx: &LateContext<'tcx>,
     symbols: &FxHashSet<Symbol>,
     if_expr: &'tcx Expr<'_>,
 ) -> bool {
-    // Obs true as we include the current if block
-    if let Some(block) = get_enclosing_block(cx, if_expr.hir_id) {
+    get_enclosing_block(cx, if_expr.hir_id).map_or(false, |block| {
         let ignore_span = block.span.shrink_to_lo().to(if_expr.span);
 
         symbols
@@ -360,9 +372,7 @@ fn check_for_warn_of_moved_symbol(
 
                 walker.result
             })
-    } else {
-        false
-    }
+    })
 }
 
 fn emit_shared_code_in_if_blocks_lint(
@@ -410,12 +420,10 @@ fn emit_shared_code_in_if_blocks_lint(
             block.stmts[block.stmts.len() - end_stmts].span
         }
         .source_callsite();
-        let moved_end = if let Some(expr) = block.expr {
-            expr.span
-        } else {
-            block.stmts[block.stmts.len() - 1].span
-        }
-        .source_callsite();
+        let moved_end = block
+            .expr
+            .map_or_else(|| block.stmts[block.stmts.len() - 1].span, |expr| expr.span)
+            .source_callsite();
 
         let moved_span = moved_start.to(moved_end);
         let moved_snipped = reindent_multiline(snippet(cx, moved_span, "_"), true, None);
@@ -488,7 +496,7 @@ fn emit_shared_code_in_if_blocks_lint(
     }
 }
 
-/// This visitor collects HirIds and Symbols of defined symbols and HirIds of used values.
+/// This visitor collects `HirId`s and Symbols of defined symbols and `HirId`s of used values.
 struct UsedValueFinderVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
 

tests/ui/shared_code_in_if_blocks/shared_at_top.rs

@@ -80,4 +80,24 @@ fn simple_but_suggestion_is_invalid() {
     }
 }
 
+/// This function tests that the `IS_SAME_THAN_ELSE` only covers the lint if it's enabled.
+fn check_if_same_than_else_mask() {
+    let x = 2021;
+
+    #[allow(clippy::if_same_then_else)]
+    if x == 2020 {
+        println!("This should trigger the `SHARED_CODE_IN_IF_BLOCKS` lint.");
+        println!("Because `IF_SAME_THEN_ELSE` is allowed here");
+    } else {
+        println!("This should trigger the `SHARED_CODE_IN_IF_BLOCKS` lint.");
+        println!("Because `IF_SAME_THEN_ELSE` is allowed here");
+    }
+
+    if x == 2019 {
+        println!("This should trigger `IS_SAME_THAN_ELSE` as usual");
+    } else {
+        println!("This should trigger `IS_SAME_THAN_ELSE` as usual");
+    }
+}
+
 fn main() {}

tests/ui/shared_code_in_if_blocks/shared_at_top.stderr

@@ -79,5 +79,43 @@ LL |     println!("I'm also moveable");
 LL |     if x == 11 {
    |
 
-error: aborting due to 5 previous errors
+error: All if blocks contain the same code at the start
+  --> $DIR/shared_at_top.rs:88:5
+   |
+LL | /     if x == 2020 {
+LL | |         println!("This should trigger the `SHARED_CODE_IN_IF_BLOCKS` lint.");
+LL | |         println!("Because `IF_SAME_THEN_ELSE` is allowed here");
+   | |________________________________________________________________^
+   |
+help: Consider moving the start statements out like this
+   |
+LL |     println!("This should trigger the `SHARED_CODE_IN_IF_BLOCKS` lint.");
+LL |     println!("Because `IF_SAME_THEN_ELSE` is allowed here");
+LL |     if x == 2020 {
+   |
+
+error: this `if` has identical blocks
+  --> $DIR/shared_at_top.rs:96:18
+   |
+LL |       if x == 2019 {
+   |  __________________^
+LL | |         println!("This should trigger `IS_SAME_THAN_ELSE` as usual");
+LL | |     } else {
+   | |_____^
+   |
+note: the lint level is defined here
+  --> $DIR/shared_at_top.rs:2:9
+   |
+LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
+note: same as this
+  --> $DIR/shared_at_top.rs:98:12
+   |
+LL |       } else {
+   |  ____________^
+LL | |         println!("This should trigger `IS_SAME_THAN_ELSE` as usual");
+LL | |     }
+   | |_____^
+
+error: aborting due to 7 previous errors