The shared_code_in_if_blocks lint only operats on entire if expr not else ifs

Author: xFrednet

clippy_lints/src/copies.rs

@@ -1,7 +1,12 @@
-use clippy_utils::diagnostics::span_lint_and_note;
-use clippy_utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
-use clippy_utils::{get_parent_expr, if_sequence};
-use rustc_hir::{Block, Expr, ExprKind};
+use crate::utils::{both, count_eq, eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
+use crate::utils::{
+    first_line_of_span, get_parent_expr, higher, if_sequence, indent_of, parent_node_is_if_expr, reindent_multiline,
+    snippet, span_lint_and_note, span_lint_and_sugg, span_lint_and_then,
+};
+use rustc_data_structures::fx::FxHashSet;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_hir::{Block, Expr, HirId};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -136,7 +141,7 @@ declare_clippy_lint! {
     /// };
     /// ```
     pub SHARED_CODE_IN_IF_BLOCKS,
-    pedantic,
+    nursery,
     "`if` statement with shared code in all blocks"
 }
 
@@ -180,7 +185,7 @@ fn lint_same_then_else<'tcx>(
 ) {
     // We only lint ifs with multiple blocks
     // TODO xFrednet 2021-01-01: Check if it's an else if block
-    if blocks.len() < 2 {
+    if blocks.len() < 2 || parent_node_is_if_expr(expr, cx) {
         return;
     }
 
@@ -190,7 +195,7 @@ fn lint_same_then_else<'tcx>(
     let mut start_eq = usize::MAX;
     let mut end_eq = usize::MAX;
     let mut expr_eq = true;
-    for (index, win) in blocks.windows(2).enumerate() {
+    for win in blocks.windows(2) {
         let l_stmts = win[0].stmts;
         let r_stmts = win[1].stmts;
 
@@ -202,9 +207,7 @@ fn lint_same_then_else<'tcx>(
         let 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 && block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq {
+        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,
@@ -215,16 +218,24 @@ fn lint_same_then_else<'tcx>(
             );
 
             return;
+        } else {
+            println!(
+                "{:?}\n - expr_eq: {:10}, l_stmts.len(): {:10}, r_stmts.len(): {:10}",
+                win[0].span,
+                block_expr_eq,
+                l_stmts.len(),
+                r_stmts.len()
+            )
         }
 
         start_eq = start_eq.min(current_start_eq);
         end_eq = end_eq.min(current_end_eq);
         expr_eq &= block_expr_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 && (has_expr && !expr_eq)) {
-            return;
-        }
+    // 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 {
@@ -275,11 +286,14 @@ fn lint_same_then_else<'tcx>(
             end_eq -= moved_start;
         }
 
-        let mut end_linable = true;
-        if let Some(expr) = block.expr {
+        let end_linable = if let Some(expr) = block.expr {
             intravisit::walk_expr(&mut walker, expr);
-            end_linable = walker.uses.iter().any(|x| !block_defs.contains(x));
-        }
+            walker.uses.iter().any(|x| !block_defs.contains(x))
+        } else if end_eq == 0 {
+            false
+        } else {
+            true
+        };
 
         emit_shared_code_in_if_blocks_lint(cx, start_eq, end_eq, end_linable, blocks, expr);
     }
@@ -351,7 +365,7 @@ fn emit_shared_code_in_if_blocks_lint(
             SHARED_CODE_IN_IF_BLOCKS,
             *span,
             "All code blocks contain the same code",
-            "Consider moving the code out like this",
+            "Consider moving the statements out like this",
             sugg.clone(),
             Applicability::Unspecified,
         );

tests/ui/shared_code_in_if_blocks/shared_at_bot.rs

@@ -32,16 +32,119 @@ fn simple_examples() {
         println!("Block end!");
         result
     };
+
+    if x == 9 {
+        println!("The index is: 6");
+
+        println!("Same end of block");
+    } else if x == 8 {
+        println!("The index is: 4");
+
+        println!("Same end of block");
+    } else {
+        println!("Same end of block");
+    }
+
+    // TODO xFrednet 2021-01.13: Fix lint for `if let`
+    let index = Some(8);
+    if let Some(index) = index {
+        println!("The index is: {}", index);
+
+        println!("Same end of block");
+    } else {
+        println!("Same end of block");
+    }
+
+    if x == 9 {
+        if x == 8 {
+            // No parent!!
+            println!("Hello World");
+            println!("---");
+        } else {
+            println!("Hello World");
+        }
+    }
 }
 
 /// Simple examples where the move can cause some problems due to moved values
 fn simple_but_suggestion_is_invalid() {
-    // TODO xFrednet 2021-01-12: This
+    let x = 16;
+
+    // Local value
+    let later_used_value = 17;
+    if x == 9 {
+        let _ = 9;
+        let later_used_value = "A string value";
+        println!("{}", later_used_value);
+    } else {
+        let later_used_value = "A string value";
+        println!("{}", later_used_value);
+        // I'm expecting a note about this
+    }
+    println!("{}", later_used_value);
+
+    // outer function
+    if x == 78 {
+        let simple_examples = "I now identify as a &str :)";
+        println!("This is the new simple_example: {}", simple_examples);
+    } else {
+        println!("Separator print statement");
+
+        let simple_examples = "I now identify as a &str :)";
+        println!("This is the new simple_example: {}", simple_examples);
+    }
+    simple_examples();
 }
 
 /// Tests where the blocks are not linted due to the used value scope
 fn not_moveable_due_to_value_scope() {
-    // TODO xFrednet 2021-01-12: This
+    let x = 18;
+
+    // Using a local value in the moved code
+    if x == 9 {
+        let y = 18;
+        println!("y is: `{}`", y);
+    } else {
+        let y = "A string";
+        println!("y is: `{}`", y);
+    }
+
+    // Using a local value in the expression
+    let _ = if x == 0 {
+        let mut result = x + 1;
+
+        println!("1. Doing some calculations");
+        println!("2. Some more calculations");
+        println!("3. Setting result");
+
+        result
+    } else {
+        let mut result = x - 1;
+
+        println!("1. Doing some calculations");
+        println!("2. Some more calculations");
+        println!("3. Setting result");
+
+        result
+    };
+
+    let _ = if x == 7 {
+        let z1 = 100;
+        println!("z1: {}", z1);
+
+        let z2 = z1;
+        println!("z2: {}", z2);
+
+        z2
+    } else {
+        let z1 = 300;
+        println!("z1: {}", z1);
+
+        let z2 = z1;
+        println!("z2: {}", z2);
+
+        z2
+    };
 }
 
 fn main() {}

tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs

@@ -152,6 +152,16 @@ fn trigger_other_lint() {
             ":D"
         }
     };
+
+    if x == 0 {
+        println!("I'm single");
+    } else if x == 68 {
+        println!("I'm a doppelgänger");
+        // Don't listen to my clone below
+    } else {
+        // Don't listen to my clone above
+        println!("I'm a doppelgänger");
+    }
 }
 
 fn main() {}