From 8efc6acc05387738313798069b8553d0ab9c92e5 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 16 Jan 2021 14:04:14 +0100 Subject: [PATCH] Improved shared_code_in_if_blocks output readability and added tests --- clippy_lints/src/copies.rs | 69 ++++++++----- .../shared_code_in_if_blocks/shared_at_bot.rs | 35 +++++++ .../shared_at_top_and_bot.rs | 96 ++++++++++++++++--- 3 files changed, 160 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 7162d809ec6..089f8c3ab0d 100644 --- a/clippy_lints/src/copies.rs +++ b/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, + ); }, ); } diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs b/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs index b85bb2e1f96..a586a1c9d45 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs +++ b/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs @@ -33,6 +33,7 @@ fn simple_examples() { result }; + // Else if block if x == 9 { println!("The index is: 6"); @@ -40,9 +41,32 @@ fn simple_examples() { } 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() {} diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs index 8de69623e5d..70f969aaf2e 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs +++ b/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, } -// General TODOs By xFrednet: +fn overlapping_eq_regions() { + let x = 9; -// -// * 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 + // 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() {}