Improved shared_code_in_if_blocks output readability and added tests

Author: xFrednet

clippy_lints/src/copies.rs

@@ -1,16 +1,16 @@
 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,
+    first_line_of_span, get_parent_expr, if_sequence, indent_of, parent_node_is_if_expr, reindent_multiline, snippet,
+    snippet_opt, 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_hir::{Block, Expr, ExprKind, HirId};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::source_map::Span;
+use rustc_span::{source_map::Span, BytePos};
 use std::borrow::Cow;
 
 declare_clippy_lint! {
@@ -218,14 +218,6 @@ 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);
@@ -328,7 +320,7 @@ fn emit_shared_code_in_if_blocks_lint(
         let suggestion = reindent_multiline(Cow::Borrowed(&suggestion), true, cond_indent);
 
         let span = span_start.to(span_end);
-        suggestions.push(("START HELP", span, suggestion.to_string()));
+        suggestions.push(("start", span, suggestion.to_string()));
     }
 
     if lint_end {
@@ -354,31 +346,56 @@ fn emit_shared_code_in_if_blocks_lint(
         let suggestion = "}\n".to_string() + &moved_snipped;
         let suggestion = reindent_multiline(Cow::Borrowed(&suggestion), true, indent);
 
-        let span = moved_start.to(span_end);
-        suggestions.push(("END_RANGE", span, suggestion.to_string()));
+        let mut span = moved_start.to(span_end);
+        // Improve formatting if the inner block has indention (i.e. normal Rust formatting)
+        let test_span = Span::new(span.lo() - BytePos(4), span.lo(), span.ctxt());
+        if snippet_opt(cx, test_span)
+            .map(|snip| snip == "    ")
+            .unwrap_or_default()
+        {
+            span = span.with_lo(test_span.lo());
+        }
+
+        suggestions.push(("end", span, suggestion.to_string()));
     }
 
     if suggestions.len() == 1 {
-        let (_, span, sugg) = &suggestions[0];
+        let (place_str, span, sugg) = suggestions.pop().unwrap();
+        let msg = format!("All if blocks contain the same code at the {}", place_str);
+        let help = format!("Consider moving the {} statements out like this", place_str);
         span_lint_and_sugg(
             cx,
             SHARED_CODE_IN_IF_BLOCKS,
-            *span,
-            "All code blocks contain the same code",
-            "Consider moving the statements out like this",
-            sugg.clone(),
+            span,
+            msg.as_str(),
+            help.as_str(),
+            sugg,
             Applicability::Unspecified,
         );
-    } else {
+    } else if suggestions.len() == 2 {
+        let (_, end_span, end_sugg) = suggestions.pop().unwrap();
+        let (_, start_span, start_sugg) = suggestions.pop().unwrap();
         span_lint_and_then(
             cx,
             SHARED_CODE_IN_IF_BLOCKS,
-            if_expr.span,
-            "All if blocks contain the same code",
+            start_span,
+            "All if blocks contain the same code at the start and the end. Here at the start:",
             move |diag| {
-                for (help, span, sugg) in suggestions {
-                    diag.span_suggestion(span, help, sugg, Applicability::Unspecified);
-                }
+                diag.span_note(end_span, "And here at the end:");
+
+                diag.span_suggestion(
+                    start_span,
+                    "Consider moving the start statements out like this:",
+                    start_sugg,
+                    Applicability::Unspecified,
+                );
+
+                diag.span_suggestion(
+                    end_span,
+                    "And consider moving the end statements out like this:",
+                    end_sugg,
+                    Applicability::Unspecified,
+                );
             },
         );
     }

clippy_lints/src/utils/mod.rs

@@ -1430,6 +1430,8 @@ pub fn if_sequence<'tcx>(
     (conds, blocks)
 }
 
+/// This function returns true if the given expression is the `else` or `if else` part of an if
+/// statement
 pub fn parent_node_is_if_expr(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool {
     let map = cx.tcx.hir();
     let parent_id = map.get_parent_node(expr.hir_id);

tests/ui/shared_code_in_if_blocks/shared_at_bot.rs

@@ -33,16 +33,40 @@ fn simple_examples() {
         result
     };
 
+    // Else if block
     if x == 9 {
         println!("The index is: 6");
 
         println!("Same end of block");
     } else if x == 8 {
         println!("The index is: 4");
 
+        // We should only get a lint trigger for the last statement
+        println!("This is also eq with the else block");
         println!("Same end of block");
     } else {
         println!("Same end of block");
+        println!("This is also eq with the else block");
+    }
+
+    // Use of outer scope value
+    let outer_scope_value = "I'm outside the if block";
+    if x < 99 {
+        let z = "How are you";
+        println!("I'm a local because I use the value `z`: `{}`", z);
+
+        println!(
+            "I'm moveable because I know: `outer_scope_value`: '{}'",
+            outer_scope_value
+        );
+    } else {
+        let z = 45678000;
+        println!("I'm a local because I use the value `z`: `{}`", z);
+
+        println!(
+            "I'm moveable because I know: `outer_scope_value`: '{}'",
+            outer_scope_value
+        );
     }
 
     // TODO xFrednet 2021-01.13: Fix lint for `if let`
@@ -147,4 +171,15 @@ fn not_moveable_due_to_value_scope() {
     };
 }
 
+#[rustfmt::skip]
+fn test_suggestion_with_weird_formatting() {
+    let x = 9;
+    let mut a = 0;
+    let mut b = 0;
+
+    // The error message still looks weird tbh but this is the best I can do
+    // for weird formatting
+    if x == 17 { b = 1; a = 0x99; } else { a = 0x99; }
+}
+
 fn main() {}

tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs

@@ -3,20 +3,88 @@
 
 // shared_code_in_if_blocks at the top and bottom of the if blocks
 
-fn main() {
-    // TODO xFrednet 2021-01-12: This
+struct DataPack {
+    id: u32,
+    name: String,
+    some_data: Vec<u8>,
 }
 
-// General TODOs By xFrednet:
-
-// 
-// * Make a test with overlapping eq regions (else ifs)
-// * Test if as function parameter, tuple constructor, index, while loop condition
-// * Test where only the expression is the same
-// * Test where the block only has an expression
-// * Test with let on a previous line let _ = \n if...
-// * Tests with unreadable formatting (Inline if, Not indented)
-// * Test multiline condition if x == 9 \n x == 8 {}
-// * Test if for return/break (Only move to front)
-// * Test in inline closures
-// * Test with structs and tuples
+fn overlapping_eq_regions() {
+    let x = 9;
+
+    // Overlap with separator
+    if x == 7 {
+        let t = 7;
+        let _overlap_start = t * 2;
+        let _overlap_end = 2 * t;
+        let _u = 9;
+    } else {
+        let t = 7;
+        let _overlap_start = t * 2;
+        let _overlap_end = 2 * t;
+        println!("Overlap separator");
+        let _overlap_start = t * 2;
+        let _overlap_end = 2 * t;
+        let _u = 9;
+    }
+
+    // Overlap with separator
+    if x == 99 {
+        let r = 7;
+        let _overlap_start = r;
+        let _overlap_middle = r * r;
+        let _overlap_end = r * r * r;
+        let z = "end";
+    } else {
+        let r = 7;
+        let _overlap_start = r;
+        let _overlap_middle = r * r;
+        let _overlap_middle = r * r;
+        let _overlap_end = r * r * r;
+        let z = "end";
+    }
+}
+
+fn complexer_example() {
+    fn gen_id(x: u32, y: u32) -> u32 {
+        let x = x & 0x0000_ffff;
+        let y = (y & 0xffff_0000) << 16;
+        x | y
+    }
+
+    fn process_data(data: DataPack) {
+        let _ = data;
+    }
+
+    let x = 8;
+    let y = 9;
+    if (x > 7 && y < 13) || (x + y) % 2 == 1 {
+        let a = 0xcafe;
+        let b = 0xffff00ff;
+        let e_id = gen_id(a, b);
+
+        println!("From the a `{}` to the b `{}`", a, b);
+
+        let pack = DataPack {
+            id: e_id,
+            name: "Player 1".to_string(),
+            some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90],
+        };
+        process_data(pack);
+    } else {
+        let a = 0xcafe;
+        let b = 0xffff00ff;
+        let e_id = gen_id(a, b);
+
+        println!("The new ID is '{}'", e_id);
+
+        let pack = DataPack {
+            id: e_id,
+            name: "Player 1".to_string(),
+            some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90],
+        };
+        process_data(pack);
+    }
+}
+
+fn main() {}