From 232e2b79f2e76cc7e1c76e0d35968711960dedee Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 5 Jan 2021 23:59:14 +0100 Subject: [PATCH 01/11] Added documentation to the multispan_sugg method --- clippy_utils/src/diagnostics.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 73a2fa992b3f..7f827f1759d2 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -218,6 +218,11 @@ where multispan_sugg_with_applicability(diag, help_msg, Applicability::Unspecified, sugg) } +/// Create a suggestion made from several `span → replacement`. +/// +/// rustfix currently doesn't support the automatic application of suggestions with +/// multiple spans. This is tracked in issue [rustfix#141](https://github.com/rust-lang/rustfix/issues/141). +/// Suggestions with multiple spans will be silently ignored. pub fn multispan_sugg_with_applicability( diag: &mut DiagnosticBuilder<'_>, help_msg: &str, From d1df73228a7cdadb4d635c6ed77daeeaebbe10ff Mon Sep 17 00:00:00 2001 From: xFrednet Date: Fri, 11 Dec 2020 22:29:53 +0000 Subject: [PATCH 02/11] A new lint for shared code in if blocks * Added expression check for shared_code_in_if_blocks * Finishing touches for the shared_code_in_if_blocks lint * Applying PR suggestions * Update lints yay * Moved test into subfolder --- CHANGELOG.md | 1 + clippy_lints/src/copies.rs | 305 ++++++++++++++++-- clippy_lints/src/lib.rs | 2 + clippy_lints/src/literal_representation.rs | 40 ++- .../methods/manual_saturating_arithmetic.rs | 48 +-- clippy_utils/src/hir_utils.rs | 9 + .../ui/checked_unwrap/complex_conditionals.rs | 2 +- tests/ui/if_same_then_else.rs | 3 +- tests/ui/if_same_then_else.stderr | 88 ++--- tests/ui/if_same_then_else2.rs | 3 +- tests/ui/if_same_then_else2.stderr | 80 +++-- tests/ui/let_if_seq.rs | 3 +- tests/ui/let_if_seq.stderr | 8 +- .../shared_code_in_if_blocks/shared_at_bot.rs | 47 +++ .../shared_code_in_if_blocks/shared_at_top.rs | 83 +++++ .../shared_at_top_and_bot.rs | 22 ++ .../valid_if_blocks.rs | 157 +++++++++ 17 files changed, 737 insertions(+), 164 deletions(-) create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_bot.rs create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_top.rs create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs create mode 100644 tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fb4a0b500f22..790bca4ac186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2455,6 +2455,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 diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 46093a16571b..29ff19707055 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -3,7 +3,10 @@ use clippy_utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHas use clippy_utils::{get_parent_expr, if_sequence}; use rustc_hir::{Block, Expr, ExprKind}; 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 std::borrow::Cow; declare_clippy_lint! { /// **What it does:** Checks for consecutive `if`s with the same condition. @@ -104,7 +107,45 @@ 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 code 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, + pedantic, + "`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<'_>) { @@ -121,30 +162,256 @@ 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(), expr); } } } -/// 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<'tcx>( + cx: &LateContext<'tcx>, + blocks: &[&Block<'tcx>], + has_unconditional_else: bool, + expr: &'tcx Expr<'_>, +) { + // We only lint ifs with multiple blocks + // TODO xFrednet 2021-01-01: Check if it's an else if block + if blocks.len() < 2 { + return; + } - if let Some((i, j)) = search_same_sequenced(blocks, eq) { - span_lint_and_note( + 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 (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 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 { + 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; + + // 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; + } + } + + 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) { + emit_shared_code_in_if_blocks_lint(cx, start_eq, 0, false, blocks, expr); + } else if end_eq != 0 && (!has_expr || !expr_eq) { + let block = blocks[blocks.len() - 1]; + let stmts = block.stmts.split_at(start_eq).1; + let (block_stmts, moved_stmts) = stmts.split_at(stmts.len() - end_eq); + + // Scan block + let mut walker = SymbolFinderVisitor::new(cx); + for stmt in block_stmts { + intravisit::walk_stmt(&mut walker, stmt); + } + let mut block_defs = walker.defs; + + // Scan moved stmts + let mut moved_start: Option = None; + let mut walker = SymbolFinderVisitor::new(cx); + for (index, stmt) in moved_stmts.iter().enumerate() { + intravisit::walk_stmt(&mut walker, stmt); + + for value in &walker.uses { + // Well we can't move this and all prev statements. So reset + if block_defs.contains(&value) { + moved_start = Some(index + 1); + walker.defs.drain().for_each(|x| { + block_defs.insert(x); + }); + } + } + + walker.uses.clear(); + } + + if let Some(moved_start) = moved_start { + end_eq -= moved_start; + } + + let mut end_linable = true; + if let Some(expr) = block.expr { + intravisit::walk_expr(&mut walker, expr); + end_linable = walker.uses.iter().any(|x| !block_defs.contains(x)); + } + + emit_shared_code_in_if_blocks_lint(cx, start_eq, end_eq, end_linable, blocks, expr); + } +} + +fn emit_shared_code_in_if_blocks_lint( + cx: &LateContext<'tcx>, + start_stmts: usize, + end_stmts: usize, + lint_end: bool, + blocks: &[&Block<'tcx>], + if_expr: &'tcx Expr<'_>, +) { + if start_stmts == 0 && !lint_end { + return; + } + + // (help, span, suggestion) + let mut suggestions: Vec<(&str, Span, String)> = vec![]; + + if start_stmts > 0 { + let block = blocks[0]; + let span_start = first_line_of_span(cx, if_expr.span).shrink_to_lo(); + let span_end = block.stmts[start_stmts - 1].span.source_callsite(); + + let cond_span = first_line_of_span(cx, if_expr.span).until(block.span); + let cond_snippet = reindent_multiline(snippet(cx, cond_span, "_"), false, None); + let cond_indent = indent_of(cx, cond_span); + let moved_span = block.stmts[0].span.source_callsite().to(span_end); + let moved_snippet = reindent_multiline(snippet(cx, moved_span, "_"), true, None); + let suggestion = moved_snippet.to_string() + "\n" + &cond_snippet + "{"; + 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())); + } + + if lint_end { + let block = blocks[blocks.len() - 1]; + let span_end = block.span.shrink_to_hi(); + + let moved_start = if end_stmts == 0 && block.expr.is_some() { + block.expr.unwrap().span + } else { + 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_span = moved_start.to(moved_end); + let moved_snipped = reindent_multiline(snippet(cx, moved_span, "_"), true, None); + let indent = indent_of(cx, if_expr.span.shrink_to_hi()); + 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())); + } + + if suggestions.len() == 1 { + let (_, span, sugg) = &suggestions[0]; + span_lint_and_sugg( cx, - IF_SAME_THEN_ELSE, - j.span, - "this `if` has identical blocks", - Some(i.span), - "same as this", + SHARED_CODE_IN_IF_BLOCKS, + *span, + "All code blocks contain the same code", + "Consider moving the code out like this", + sugg.clone(), + Applicability::Unspecified, + ); + } else { + span_lint_and_then( + cx, + SHARED_CODE_IN_IF_BLOCKS, + if_expr.span, + "All if blocks contain the same code", + move |diag| { + for (help, span, sugg) in suggestions { + diag.span_suggestion(span, help, sugg, Applicability::Unspecified); + } + }, ); } } +pub struct SymbolFinderVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + defs: FxHashSet, + uses: FxHashSet, +} + +impl<'a, 'tcx> SymbolFinderVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'tcx>) -> Self { + SymbolFinderVisitor { + cx, + defs: FxHashSet::default(), + uses: FxHashSet::default(), + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for SymbolFinderVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::All(self.cx.tcx.hir()) + } + + fn visit_local(&mut self, l: &'tcx rustc_hir::Local<'tcx>) { + let local_id = l.pat.hir_id; + self.defs.insert(local_id); + if let Some(expr) = l.init { + intravisit::walk_expr(self, expr); + } + } + + fn visit_qpath(&mut self, qpath: &'tcx rustc_hir::QPath<'tcx>, id: HirId, _span: rustc_span::Span) { + if let rustc_hir::QPath::Resolved(_, ref path) = *qpath { + if path.segments.len() == 1 { + if let rustc_hir::def::Res::Local(var) = self.cx.qpath_res(qpath, id) { + self.uses.insert(var); + } + } + } + } +} + /// Implementation of `IFS_SAME_COND`. fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { let hash: &dyn Fn(&&Expr<'_>) -> u64 = &|expr| -> u64 { @@ -198,15 +465,3 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { ); } } - -fn search_same_sequenced(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 -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b93e6b5b2e8e..ba98a2a2607d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -616,6 +616,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, ©_iterator::COPY_ITERATOR, &create_dir::CREATE_DIR, &dbg_macro::DBG_MACRO, @@ -1365,6 +1366,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&casts::PTR_AS_PTR), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), + LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(©_iterator::COPY_ITERATOR), LintId::of(&default::DEFAULT_TRAIT_ACCESS), LintId::of(&dereference::EXPLICIT_DEREF_METHODS), diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 7fd55151226b..b736eeff6bf7 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -464,32 +464,28 @@ impl DecimalLiteralRepresentation { { return Err(WarningType::DecimalRepresentation); } - } else if digits.len() < 4 { - // Lint for Literals with a hex-representation of 2 or 3 digits - let f = &digits[0..1]; // first digit - let s = &digits[1..]; // suffix - - // Powers of 2 - if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && s.chars().all(|c| c == '0')) - // Powers of 2 minus 1 - || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && s.chars().all(|c| c == 'F')) - { - return Err(WarningType::DecimalRepresentation); - } } else { - // Lint for Literals with a hex-representation of 4 digits or more let f = &digits[0..1]; // first digit let m = &digits[1..digits.len() - 1]; // middle digits, except last let s = &digits[1..]; // suffix - - // Powers of 2 with a margin of +15/-16 - if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && m.chars().all(|c| c == '0')) - || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && m.chars().all(|c| c == 'F')) - // Lint for representations with only 0s and Fs, while allowing 7 as the first - // digit - || ((f.eq("7") || f.eq("F")) && s.chars().all(|c| c == '0' || c == 'F')) - { - return Err(WarningType::DecimalRepresentation); + if digits.len() < 4 { + // Powers of 2 + if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && s.chars().all(|c| c == '0')) + // Powers of 2 minus 1 + || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && s.chars().all(|c| c == 'F')) + { + return Err(WarningType::DecimalRepresentation); + } + } else { + // Powers of 2 with a margin of +15/-16 + if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && m.chars().all(|c| c == '0')) + || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && m.chars().all(|c| c == 'F')) + // Lint for representations with only 0s and Fs, while allowing 7 as the first + // digit + || ((f.eq("7") || f.eq("F")) && s.chars().all(|c| c == '0' || c == 'F')) + { + return Err(WarningType::DecimalRepresentation); + } } } diff --git a/clippy_lints/src/methods/manual_saturating_arithmetic.rs b/clippy_lints/src/methods/manual_saturating_arithmetic.rs index 6c5a842a9128..ecb8b72ef461 100644 --- a/clippy_lints/src/methods/manual_saturating_arithmetic.rs +++ b/clippy_lints/src/methods/manual_saturating_arithmetic.rs @@ -44,44 +44,28 @@ pub fn check( // "mul" is omitted because lhs can be negative. _ => return, } - - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - super::MANUAL_SATURATING_ARITHMETIC, - expr.span, - "manual saturating arithmetic", - &format!("try using `saturating_{}`", arith), - format!( - "{}.saturating_{}({})", - snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability), - arith, - snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability), - ), - applicability, - ); } else { match (mm, arith) { (MinMax::Max, "add" | "mul") | (MinMax::Min, "sub") => (), _ => return, } - - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - super::MANUAL_SATURATING_ARITHMETIC, - expr.span, - "manual saturating arithmetic", - &format!("try using `saturating_{}`", arith), - format!( - "{}.saturating_{}({})", - snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability), - arith, - snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability), - ), - applicability, - ); } + + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + super::MANUAL_SATURATING_ARITHMETIC, + expr.span, + "manual saturating arithmetic", + &format!("try using `saturating_{}`", arith), + format!( + "{}.saturating_{}({})", + snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability), + arith, + snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability), + ), + applicability, + ); } #[derive(PartialEq, Eq)] diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 0c3b8b89171d..8b90fedbafef 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -483,6 +483,15 @@ pub fn over(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( + left: &mut dyn Iterator, + right: &mut dyn Iterator, + 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) diff --git a/tests/ui/checked_unwrap/complex_conditionals.rs b/tests/ui/checked_unwrap/complex_conditionals.rs index c986c992a07a..bb5b6f5ec043 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.rs +++ b/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(()); diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 9c5fe02f7519..35a2e139c11d 100644 --- a/tests/ui/if_same_then_else.rs +++ b/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 { diff --git a/tests/ui/if_same_then_else.stderr b/tests/ui/if_same_then_else.stderr index d9fdf06fa8b7..2f38052fc209 100644 --- a/tests/ui/if_same_then_else.stderr +++ b/tests/ui/if_same_then_else.stderr @@ -1,111 +1,111 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:28:12 + --> $DIR/if_same_then_else.rs:21:13 | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block +LL | if true { + | _____________^ LL | | Foo { bar: 42 }; LL | | 0..10; +LL | | ..; ... | LL | | foo(); -LL | | } +LL | | } else { | |_____^ | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else.rs:20:13 + --> $DIR/if_same_then_else.rs:29:12 | -LL | if true { - | _____________^ +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block LL | | Foo { bar: 42 }; LL | | 0..10; -LL | | ..; ... | LL | | foo(); -LL | | } else { +LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:66:12 - | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block -LL | | 0.0 -LL | | }; - | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:64:21 + --> $DIR/if_same_then_else.rs:65:21 | LL | let _ = if true { | _____________________^ LL | | 0.0 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:73:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:67:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | -0.0 +LL | | 0.0 LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:71:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:72:21 | LL | let _ = if true { | _____________________^ LL | | -0.0 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:89:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:74:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | 42 +LL | | -0.0 LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:87:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:88:21 | LL | let _ = if true { | _____________________^ LL | | 42 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:101:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:90:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block +LL | | 42 +LL | | }; + | |_____^ + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:95:13 + | +LL | if true { + | _____________^ LL | | let bar = if true { 42 } else { 43 }; LL | | +LL | | while foo() { ... | LL | | bar + 1; -LL | | } +LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:94:13 + --> $DIR/if_same_then_else.rs:102:12 | -LL | if true { - | _____________^ +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block LL | | let bar = if true { 42 } else { 43 }; LL | | -LL | | while foo() { ... | LL | | bar + 1; -LL | | } else { +LL | | } | |_____^ error: aborting due to 5 previous errors diff --git a/tests/ui/if_same_then_else2.rs b/tests/ui/if_same_then_else2.rs index a2ff1b741ca2..8164b125570d 100644 --- a/tests/ui/if_same_then_else2.rs +++ b/tests/ui/if_same_then_else2.rs @@ -5,7 +5,8 @@ clippy::collapsible_if, clippy::ifs_same_cond, clippy::needless_return, - clippy::single_element_loop + clippy::single_element_loop, + clippy::shared_code_in_if_blocks )] fn if_same_then_else2() -> Result<&'static str, ()> { diff --git a/tests/ui/if_same_then_else2.stderr b/tests/ui/if_same_then_else2.stderr index 454322d8aacd..941b92f23f35 100644 --- a/tests/ui/if_same_then_else2.stderr +++ b/tests/ui/if_same_then_else2.stderr @@ -24,18 +24,22 @@ LL | | if foo.is_some() { LL | | } LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:35:12 + | + = note: `-D clippy::if-same-then-else` implied by `-D warnings` +note: same as this + --> $DIR/if_same_then_else2.rs:21:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | if let Some(a) = Some(42) {} +LL | | for _ in &[42] { +LL | | let foo: &Option<_> = &Some::(42); +... | +LL | | } LL | | } | |_____^ - | -note: same as this + +error: this `if` has identical blocks --> $DIR/if_same_then_else2.rs:33:13 | LL | if true { @@ -43,18 +47,18 @@ LL | if true { LL | | if let Some(a) = Some(42) {} LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:42:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:35:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | if let (1, .., 3) = (1, 2, 3) {} +LL | | if let Some(a) = Some(42) {} LL | | } | |_____^ - | -note: same as this + +error: this `if` has identical blocks --> $DIR/if_same_then_else2.rs:40:13 | LL | if true { @@ -62,18 +66,18 @@ LL | if true { LL | | if let (1, .., 3) = (1, 2, 3) {} LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:92:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:42:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | f32::NAN -LL | | }; +LL | | if let (1, .., 3) = (1, 2, 3) {} +LL | | } | |_____^ - | -note: same as this + +error: this `if` has identical blocks --> $DIR/if_same_then_else2.rs:90:21 | LL | let _ = if true { @@ -81,18 +85,18 @@ LL | let _ = if true { LL | | f32::NAN LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:99:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:92:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | Ok("foo")?; -LL | | } +LL | | f32::NAN +LL | | }; | |_____^ - | -note: same as this + +error: this `if` has identical blocks --> $DIR/if_same_then_else2.rs:97:13 | LL | if true { @@ -100,18 +104,18 @@ LL | if true { LL | | Ok("foo")?; LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:124:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:99:12 | LL | } else { | ____________^ -LL | | let foo = ""; -LL | | return Ok(&foo[0..]); +LL | | //~ ERROR same body as `if` block +LL | | Ok("foo")?; LL | | } | |_____^ - | -note: same as this + +error: this `if` has identical blocks --> $DIR/if_same_then_else2.rs:121:20 | LL | } else if true { @@ -120,6 +124,16 @@ LL | | let foo = ""; LL | | return Ok(&foo[0..]); LL | | } else { | |_____^ + | +note: same as this + --> $DIR/if_same_then_else2.rs:124:12 + | +LL | } else { + | ____________^ +LL | | let foo = ""; +LL | | return Ok(&foo[0..]); +LL | | } + | |_____^ error: aborting due to 6 previous errors diff --git a/tests/ui/let_if_seq.rs b/tests/ui/let_if_seq.rs index 32a67f181df4..9fd3f875a5f1 100644 --- a/tests/ui/let_if_seq.rs +++ b/tests/ui/let_if_seq.rs @@ -2,7 +2,8 @@ unused_variables, unused_assignments, clippy::similar_names, - clippy::blacklisted_name + clippy::blacklisted_name, + clippy::shared_code_in_if_blocks )] #![warn(clippy::useless_let_if_seq)] diff --git a/tests/ui/let_if_seq.stderr b/tests/ui/let_if_seq.stderr index 7de560c73486..9cf2e10a5ee5 100644 --- a/tests/ui/let_if_seq.stderr +++ b/tests/ui/let_if_seq.stderr @@ -1,5 +1,5 @@ error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:64:5 + --> $DIR/let_if_seq.rs:65:5 | LL | / let mut foo = 0; LL | | if f() { @@ -11,7 +11,7 @@ LL | | } = note: you might not need `mut` at all error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:69:5 + --> $DIR/let_if_seq.rs:70:5 | LL | / let mut bar = 0; LL | | if f() { @@ -25,7 +25,7 @@ LL | | } = note: you might not need `mut` at all error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:77:5 + --> $DIR/let_if_seq.rs:78:5 | LL | / let quz; LL | | if f() { @@ -36,7 +36,7 @@ LL | | } | |_____^ help: it is more idiomatic to write: `let quz = if f() { 42 } else { 0 };` error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:106:5 + --> $DIR/let_if_seq.rs:107:5 | LL | / let mut baz = 0; LL | | if f() { 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 new file mode 100644 index 000000000000..22f1fe95f792 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs @@ -0,0 +1,47 @@ +#![allow(dead_code)] +#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + +// This tests the shared_code_in_if_blocks lint at the end of blocks + +fn simple_examples() { + // TODO xFrednet 2021-01-06: Test with const literals at the end + let x = 1; + + let _ = if x == 7 { + println!("Branch I"); + let start_value = 0; + println!("=^.^="); + + // Same but not moveable due to `start_value` + let _ = start_value; + + // The rest is self contained and moveable => Only lint the rest + let result = false; + println!("Block end!"); + result + } else { + println!("Branch II"); + let start_value = 8; + println!("xD"); + + // Same but not moveable due to `start_value` + let _ = start_value; + + // The rest is self contained and moveable => Only lint the rest + let result = false; + println!("Block end!"); + result + }; +} + +/// 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 +} + +/// 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 +} + +fn main() {} diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.rs b/tests/ui/shared_code_in_if_blocks/shared_at_top.rs new file mode 100644 index 000000000000..0a6828127e99 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top.rs @@ -0,0 +1,83 @@ +#![allow(dead_code, clippy::eval_order_dependence)] +#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + +// This tests the shared_code_in_if_blocks lint at the start of blocks + +fn simple_examples() { + let x = 0; + + // Simple + if true { + println!("Hello World!"); + println!("I'm branch nr: 1"); + } else { + println!("Hello World!"); + println!("I'm branch nr: 2"); + } + + // Else if + if x == 0 { + let y = 9; + println!("The value y was set to: `{}`", y); + let _z = y; + + println!("I'm the true start index of arrays"); + } else if x == 1 { + let y = 9; + println!("The value y was set to: `{}`", y); + let _z = y; + + println!("I start counting from 1 so my array starts from `1`"); + } else { + let y = 9; + println!("The value y was set to: `{}`", y); + let _z = y; + + println!("Ha, Pascal allows you to start the array where you want") + } + + // Return a value + let _ = if x == 7 { + let y = 16; + println!("What can I say except: \"you're welcome?\""); + let _ = y; + x + } else { + let y = 16; + println!("Thank you"); + y + }; +} + +/// Simple examples where the move can cause some problems due to moved values +fn simple_but_suggestion_is_invalid() { + let x = 10; + + // Can't be automatically moved because used_value_name is getting used again + let used_value_name = 19; + if x == 10 { + let used_value_name = "Different type"; + println!("Str: {}", used_value_name); + let _ = 1; + } else { + let used_value_name = "Different type"; + println!("Str: {}", used_value_name); + let _ = 2; + } + let _ = used_value_name; + + // This can be automatically moved as `can_be_overridden` is not used again + let can_be_overridden = 8; + let _ = can_be_overridden; + if x == 11 { + let can_be_overridden = "Move me"; + println!("I'm also moveable"); + let _ = 111; + } else { + let can_be_overridden = "Move me"; + println!("I'm also moveable"); + let _ = 222; + } +} + +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 new file mode 100644 index 000000000000..8de69623e5d2 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs @@ -0,0 +1,22 @@ +#![allow(dead_code)] +#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + +// shared_code_in_if_blocks at the top and bottom of the if blocks + +fn main() { + // TODO xFrednet 2021-01-12: This +} + +// 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 diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs new file mode 100644 index 000000000000..1f12e9348d47 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs @@ -0,0 +1,157 @@ +#![allow(dead_code, clippy::eval_order_dependence)] +#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + +// This tests the shared_code_in_if_blocks lint at the start of blocks + +// Tests with value references are includes in "shared_code_at_bot.rs" + +fn valid_examples() { + let x = 2; + + // The edge statements are different + if x == 9 { + let y = 1 << 5; + + println!("This is the same: vvv"); + let _z = y; + println!("The block expression is different"); + + println!("Different end 1"); + } else { + let y = 1 << 7; + + println!("This is the same: vvv"); + let _z = y; + println!("The block expression is different"); + + println!("Different end 2"); + } + + // No else + if x == 2 { + println!("Hello world!"); + println!("Hello back, how are you?"); + + // This is different vvvv + println!("Howdy stranger =^.^="); + + println!("Bye Bye World"); + } else if x == 9 { + println!("Hello world!"); + println!("Hello back, how are you?"); + + // This is different vvvv + println!("Hello reviewer :D"); + + println!("Bye Bye World"); + } + + // Overlapping statements only in else if blocks -> Don't lint + if x == 0 { + println!("I'm important!") + } else if x == 17 { + println!("I share code in else if"); + + println!("x is 17"); + } else { + println!("I share code in else if"); + + println!("x is nether x nor 17"); + } + + // Mutability is different + if x == 13 { + let mut y = 9; + println!("Value y is: {}", y); + y += 16; + let _z1 = y; + } else { + let y = 9; + println!("Value y is: {}", y); + let _z2 = y; + } + + // Same blocks but at start and bottom so no `if_same_then_else` lint + if x == 418 { + let y = 9; + let z = 8; + let _ = (x, y, z); + // Don't tell the programmer, my code is also in the else block + } else if x == 419 { + println!("+-----------+"); + println!("| |"); + println!("| O O |"); + println!("| ° |"); + println!("| \\_____/ |"); + println!("| |"); + println!("+-----------+"); + } else { + let y = 9; + let z = 8; + let _ = (x, y, z); + // I'm so much better than the x == 418 block. Trust me + } +} + +/// This makes sure that the `if_same_then_else` masks the `shared_code_in_if_blocks` lint +fn trigger_other_lint() { + let x = 0; + let y = 1; + + // Same block + if x == 0 { + let u = 19; + println!("How are u today?"); + let _ = "This is a string"; + } else { + let u = 19; + println!("How are u today?"); + let _ = "This is a string"; + } + + // More complex same blocks + if x == 17 { + #[derive(Debug)] + struct Duck { + num: u64, + }; + let pet = Duck { num: 18 }; + println!("{:?}", pet); + } else { + #[derive(Debug)] + struct Duck { + num: u64, + }; + let pet = Duck { num: 18 }; + println!("{:?}", pet); + } + + // Only same expression + let _ = if x == 6 { 7 } else { 7 }; + + // Same in else if block + let _ = if x == 67 { + println!("Well I'm the most important block"); + "I'm a pretty string" + } else if x == 68 { + println!("I'm a doppelgänger"); + // Don't listen to my clone below + + if y == 90 { + "=^.^=" + } else { + ":D" + } + } else { + // Don't listen to my clone above + println!("I'm a doppelgänger"); + + if y == 90 { + "=^.^=" + } else { + ":D" + } + }; +} + +fn main() {} From 469ff96db3cc397be46ea9d19c72148c6a95eb6a Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 13 Jan 2021 20:01:15 +0100 Subject: [PATCH 03/11] The shared_code_in_if_blocks lint only operats on entire if expr not else ifs --- clippy_lints/src/copies.rs | 52 +++++---- .../shared_code_in_if_blocks/shared_at_bot.rs | 107 +++++++++++++++++- .../valid_if_blocks.rs | 10 ++ 3 files changed, 148 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 29ff19707055..7162d809ec67 100644 --- a/clippy_lints/src/copies.rs +++ b/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, ); 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 22f1fe95f792..b85bb2e1f96f 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 @@ -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() {} diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs index 1f12e9348d47..480758777f16 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs +++ b/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() {} From 8efc6acc05387738313798069b8553d0ab9c92e5 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 16 Jan 2021 14:04:14 +0100 Subject: [PATCH 04/11] 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 | 98 ++++++++++++++++--- 3 files changed, 161 insertions(+), 41 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 7162d809ec67..089f8c3ab0de 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 b85bb2e1f96f..a586a1c9d45a 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 8de69623e5d2..70f969aaf2ef 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: - -// -// * 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() {} From b1d26e544f10b814d9793294d0c05dd2ace5d0dc Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 16 Jan 2021 20:37:50 +0100 Subject: [PATCH 05/11] Improved shared_code_in_if_blocks message and added test stderrs --- clippy_lints/src/copies.rs | 169 ++++++++++++++---- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/literal_representation.rs | 40 +++-- tests/ui/if_same_then_else2.stderr | 22 +-- .../shared_code_in_if_blocks/shared_at_bot.rs | 19 ++ .../shared_at_bot.stderr | 131 ++++++++++++++ .../shared_at_top.stderr | 83 +++++++++ .../shared_at_top_and_bot.rs | 29 +++ .../shared_at_top_and_bot.stderr | 154 ++++++++++++++++ .../valid_if_blocks.stderr | 107 +++++++++++ 10 files changed, 692 insertions(+), 64 deletions(-) create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_top.stderr create mode 100644 tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr create mode 100644 tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 089f8c3ab0de..47732f4766db 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, 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, + 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, }; use rustc_data_structures::fx::FxHashSet; -use rustc_errors::Applicability; +use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; 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, BytePos}; +use rustc_span::{source_map::Span, symbol::Symbol, BytePos}; use std::borrow::Cow; declare_clippy_lint! { @@ -184,7 +184,6 @@ fn lint_same_then_else<'tcx>( expr: &'tcx Expr<'_>, ) { // We only lint ifs with multiple blocks - // TODO xFrednet 2021-01-01: Check if it's an else if block if blocks.len() < 2 || parent_node_is_if_expr(expr, cx) { return; } @@ -242,36 +241,61 @@ fn lint_same_then_else<'tcx>( // Only the start is the same if start_eq != 0 && end_eq == 0 && (!has_expr || !expr_eq) { - emit_shared_code_in_if_blocks_lint(cx, start_eq, 0, false, blocks, expr); - } else if end_eq != 0 && (!has_expr || !expr_eq) { + let block = blocks[0]; + let start_stmts = block.stmts.split_at(start_eq).0; + + let mut start_walker = UsedValueFinderVisitor::new(cx); + for stmt in start_stmts { + intravisit::walk_stmt(&mut start_walker, stmt); + } + + emit_shared_code_in_if_blocks_lint( + cx, + start_eq, + 0, + false, + check_for_warn_of_moved_symbol(cx, &start_walker.def_symbols, expr), + blocks, + expr, + ); + } else if end_eq != 0 || (has_expr && expr_eq) { let block = blocks[blocks.len() - 1]; - let stmts = block.stmts.split_at(start_eq).1; - let (block_stmts, moved_stmts) = stmts.split_at(stmts.len() - end_eq); + let (start_stmts, block_stmts) = block.stmts.split_at(start_eq); + let (block_stmts, end_stmts) = block_stmts.split_at(block_stmts.len() - end_eq); + + // Scan start + let mut start_walker = UsedValueFinderVisitor::new(cx); + for stmt in start_stmts { + intravisit::walk_stmt(&mut start_walker, stmt); + } + let mut moved_syms = start_walker.def_symbols; // Scan block - let mut walker = SymbolFinderVisitor::new(cx); + let mut block_walker = UsedValueFinderVisitor::new(cx); for stmt in block_stmts { - intravisit::walk_stmt(&mut walker, stmt); + intravisit::walk_stmt(&mut block_walker, stmt); } - let mut block_defs = walker.defs; + let mut block_defs = block_walker.defs; // Scan moved stmts let mut moved_start: Option = None; - let mut walker = SymbolFinderVisitor::new(cx); - for (index, stmt) in moved_stmts.iter().enumerate() { - intravisit::walk_stmt(&mut walker, stmt); + let mut end_walker = UsedValueFinderVisitor::new(cx); + for (index, stmt) in end_stmts.iter().enumerate() { + intravisit::walk_stmt(&mut end_walker, stmt); - for value in &walker.uses { + for value in &end_walker.uses { // Well we can't move this and all prev statements. So reset if block_defs.contains(&value) { moved_start = Some(index + 1); - walker.defs.drain().for_each(|x| { + end_walker.defs.drain().for_each(|x| { block_defs.insert(x); }); + + end_walker.def_symbols.clear(); } } - walker.uses.clear(); + end_walker.uses.clear(); } if let Some(moved_start) = moved_start { @@ -279,15 +303,65 @@ fn lint_same_then_else<'tcx>( } let end_linable = if let Some(expr) = block.expr { - intravisit::walk_expr(&mut walker, expr); - walker.uses.iter().any(|x| !block_defs.contains(x)) + 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 }; - emit_shared_code_in_if_blocks_lint(cx, start_eq, end_eq, end_linable, blocks, expr); + if end_linable { + end_walker.def_symbols.drain().for_each(|x| { + moved_syms.insert(x); + }); + } + + emit_shared_code_in_if_blocks_lint( + cx, + start_eq, + end_eq, + end_linable, + check_for_warn_of_moved_symbol(cx, &moved_syms, expr), + blocks, + expr, + ); + } +} + +fn check_for_warn_of_moved_symbol( + cx: &LateContext<'tcx>, + symbols: &FxHashSet, + 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) { + let ignore_span = block.span.shrink_to_lo().to(if_expr.span); + + symbols + .iter() + .filter(|sym| !sym.as_str().starts_with('_')) + .any(move |sym| { + let mut walker = ContainsName { + name: *sym, + result: false, + }; + + // Scan block + block + .stmts + .iter() + .filter(|stmt| !ignore_span.overlaps(stmt.span)) + .for_each(|stmt| intravisit::walk_stmt(&mut walker, stmt)); + + if let Some(expr) = block.expr { + intravisit::walk_expr(&mut walker, expr); + } + + walker.result + }) + } else { + false } } @@ -296,6 +370,7 @@ fn emit_shared_code_in_if_blocks_lint( start_stmts: usize, end_stmts: usize, lint_end: bool, + warn_about_moved_symbol: bool, blocks: &[&Block<'tcx>], if_expr: &'tcx Expr<'_>, ) { @@ -305,7 +380,9 @@ fn emit_shared_code_in_if_blocks_lint( // (help, span, suggestion) let mut suggestions: Vec<(&str, Span, String)> = vec![]; + let mut add_expr_note = false; + // Construct suggestions if start_stmts > 0 { let block = blocks[0]; let span_start = first_line_of_span(cx, if_expr.span).shrink_to_lo(); @@ -357,21 +434,29 @@ fn emit_shared_code_in_if_blocks_lint( } suggestions.push(("end", span, suggestion.to_string())); + add_expr_note = !cx.typeck_results().expr_ty(if_expr).is_unit() } + let add_optional_msgs = |diag: &mut DiagnosticBuilder<'_>| { + if add_expr_note { + diag.note("The end suggestion probably needs some adjustments to use the expression result correctly."); + } + + if warn_about_moved_symbol { + diag.warn("Some moved values might need to be renamed to avoid wrong references."); + } + }; + + // Emit lint if suggestions.len() == 1 { 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, - msg.as_str(), - help.as_str(), - sugg, - Applicability::Unspecified, - ); + span_lint_and_then(cx, SHARED_CODE_IN_IF_BLOCKS, span, msg.as_str(), |diag| { + diag.span_suggestion(span, help.as_str(), sugg, Applicability::Unspecified); + + add_optional_msgs(diag); + }); } else if suggestions.len() == 2 { let (_, end_span, end_sugg) = suggestions.pop().unwrap(); let (_, start_span, start_sugg) = suggestions.pop().unwrap(); @@ -396,28 +481,39 @@ fn emit_shared_code_in_if_blocks_lint( end_sugg, Applicability::Unspecified, ); + + add_optional_msgs(diag); }, ); } } -pub struct SymbolFinderVisitor<'a, 'tcx> { +/// This visitor collects HirIds and Symbols of defined symbols and HirIds of used values. +struct UsedValueFinderVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, + + /// The `HirId`s of defined values in the scanned statements defs: FxHashSet, + + /// The Symbols of the defined symbols in the scanned statements + def_symbols: FxHashSet, + + /// The `HirId`s of the used values uses: FxHashSet, } -impl<'a, 'tcx> SymbolFinderVisitor<'a, 'tcx> { +impl<'a, 'tcx> UsedValueFinderVisitor<'a, 'tcx> { fn new(cx: &'a LateContext<'tcx>) -> Self { - SymbolFinderVisitor { + UsedValueFinderVisitor { cx, defs: FxHashSet::default(), + def_symbols: FxHashSet::default(), uses: FxHashSet::default(), } } } -impl<'a, 'tcx> Visitor<'tcx> for SymbolFinderVisitor<'a, 'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for UsedValueFinderVisitor<'a, 'tcx> { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { @@ -427,6 +523,11 @@ impl<'a, 'tcx> Visitor<'tcx> for SymbolFinderVisitor<'a, 'tcx> { fn visit_local(&mut self, l: &'tcx rustc_hir::Local<'tcx>) { let local_id = l.pat.hir_id; self.defs.insert(local_id); + + if let Some(sym) = l.pat.simple_ident() { + self.def_symbols.insert(sym.name); + } + if let Some(expr) = l.init { intravisit::walk_expr(self, expr); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ba98a2a2607d..9afbf95a342c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1366,7 +1366,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&casts::PTR_AS_PTR), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), - LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(©_iterator::COPY_ITERATOR), LintId::of(&default::DEFAULT_TRAIT_ACCESS), LintId::of(&dereference::EXPLICIT_DEREF_METHODS), @@ -2064,6 +2063,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(&attrs::EMPTY_LINE_AFTER_OUTER_ATTR), LintId::of(&cognitive_complexity::COGNITIVE_COMPLEXITY), + LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(&disallowed_method::DISALLOWED_METHOD), LintId::of(&fallible_impl_from::FALLIBLE_IMPL_FROM), LintId::of(&floating_point_arithmetic::IMPRECISE_FLOPS), diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index b736eeff6bf7..7fd55151226b 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -464,28 +464,32 @@ impl DecimalLiteralRepresentation { { return Err(WarningType::DecimalRepresentation); } + } else if digits.len() < 4 { + // Lint for Literals with a hex-representation of 2 or 3 digits + let f = &digits[0..1]; // first digit + let s = &digits[1..]; // suffix + + // Powers of 2 + if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && s.chars().all(|c| c == '0')) + // Powers of 2 minus 1 + || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && s.chars().all(|c| c == 'F')) + { + return Err(WarningType::DecimalRepresentation); + } } else { + // Lint for Literals with a hex-representation of 4 digits or more let f = &digits[0..1]; // first digit let m = &digits[1..digits.len() - 1]; // middle digits, except last let s = &digits[1..]; // suffix - if digits.len() < 4 { - // Powers of 2 - if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && s.chars().all(|c| c == '0')) - // Powers of 2 minus 1 - || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && s.chars().all(|c| c == 'F')) - { - return Err(WarningType::DecimalRepresentation); - } - } else { - // Powers of 2 with a margin of +15/-16 - if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && m.chars().all(|c| c == '0')) - || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && m.chars().all(|c| c == 'F')) - // Lint for representations with only 0s and Fs, while allowing 7 as the first - // digit - || ((f.eq("7") || f.eq("F")) && s.chars().all(|c| c == '0' || c == 'F')) - { - return Err(WarningType::DecimalRepresentation); - } + + // Powers of 2 with a margin of +15/-16 + if ((f.eq("1") || f.eq("2") || f.eq("4") || f.eq("8")) && m.chars().all(|c| c == '0')) + || ((f.eq("1") || f.eq("3") || f.eq("7") || f.eq("F")) && m.chars().all(|c| c == 'F')) + // Lint for representations with only 0s and Fs, while allowing 7 as the first + // digit + || ((f.eq("7") || f.eq("F")) && s.chars().all(|c| c == '0' || c == 'F')) + { + return Err(WarningType::DecimalRepresentation); } } diff --git a/tests/ui/if_same_then_else2.stderr b/tests/ui/if_same_then_else2.stderr index 941b92f23f35..b31eb46a9826 100644 --- a/tests/ui/if_same_then_else2.stderr +++ b/tests/ui/if_same_then_else2.stderr @@ -27,7 +27,7 @@ LL | | } else { | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else2.rs:21:12 + --> $DIR/if_same_then_else2.rs:22:12 | LL | } else { | ____________^ @@ -40,7 +40,7 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:33:13 + --> $DIR/if_same_then_else2.rs:34:13 | LL | if true { | _____________^ @@ -49,7 +49,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:35:12 + --> $DIR/if_same_then_else2.rs:36:12 | LL | } else { | ____________^ @@ -59,7 +59,7 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:40:13 + --> $DIR/if_same_then_else2.rs:41:13 | LL | if true { | _____________^ @@ -68,7 +68,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:42:12 + --> $DIR/if_same_then_else2.rs:43:12 | LL | } else { | ____________^ @@ -78,7 +78,7 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:90:21 + --> $DIR/if_same_then_else2.rs:91:21 | LL | let _ = if true { | _____________________^ @@ -87,7 +87,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:92:12 + --> $DIR/if_same_then_else2.rs:93:12 | LL | } else { | ____________^ @@ -97,7 +97,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:97:13 + --> $DIR/if_same_then_else2.rs:98:13 | LL | if true { | _____________^ @@ -106,7 +106,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:99:12 + --> $DIR/if_same_then_else2.rs:100:12 | LL | } else { | ____________^ @@ -116,7 +116,7 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:121:20 + --> $DIR/if_same_then_else2.rs:122:20 | LL | } else if true { | ____________________^ @@ -126,7 +126,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:124:12 + --> $DIR/if_same_then_else2.rs:125:12 | LL | } else { | ____________^ 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 a586a1c9d45a..5392da7ac7ac 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 @@ -171,6 +171,25 @@ fn not_moveable_due_to_value_scope() { }; } +/// This should add a note to the lint msg since the moved expression is not `()` +fn added_note_for_expression_use() -> u32 { + let x = 9; + + let _ = if x == 7 { + x << 2 + } else { + let _ = 6; + x << 2 + }; + + if x == 9 { + x * 4 + } else { + let _ = 17; + x * 4 + } +} + #[rustfmt::skip] fn test_suggestion_with_weird_formatting() { let x = 9; diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr new file mode 100644 index 000000000000..62a95157d618 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr @@ -0,0 +1,131 @@ +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:31:5 + | +LL | / let result = false; +LL | | println!("Block end!"); +LL | | result +LL | | }; + | |_____^ + | +note: the lint level is defined here + --> $DIR/shared_at_bot.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: The end suggestion probably needs some adjustments to use the expression result correctly. +help: Consider moving the end statements out like this + | +LL | } +LL | let result = false; +LL | println!("Block end!"); +LL | result; + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:66:5 + | +LL | / println!( +LL | | "I'm moveable because I know: `outer_scope_value`: '{}'", +LL | | outer_scope_value +LL | | ); +LL | | } + | |_____^ + | +help: Consider moving the end statements out like this + | +LL | } +LL | println!( +LL | "I'm moveable because I know: `outer_scope_value`: '{}'", +LL | outer_scope_value +LL | ); + | + +error: All if blocks contain the same code at the start + --> $DIR/shared_at_bot.rs:83:9 + | +LL | / if x == 8 { +LL | | // No parent!! +LL | | println!("Hello World"); + | |____________________________________^ + | +help: Consider moving the start statements out like this + | +LL | println!("Hello World"); +LL | if x == 8 { + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:104:5 + | +LL | / let later_used_value = "A string value"; +LL | | println!("{}", later_used_value); +LL | | // I'm expecting a note about this +LL | | } + | |_____^ + | + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the end statements out like this + | +LL | } +LL | let later_used_value = "A string value"; +LL | println!("{}", later_used_value); + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:117:5 + | +LL | / let simple_examples = "I now identify as a &str :)"; +LL | | println!("This is the new simple_example: {}", simple_examples); +LL | | } + | |_____^ + | + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the end statements out like this + | +LL | } +LL | let simple_examples = "I now identify as a &str :)"; +LL | println!("This is the new simple_example: {}", simple_examples); + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:182:5 + | +LL | / x << 2 +LL | | }; + | |_____^ + | + = note: The end suggestion probably needs some adjustments to use the expression result correctly. +help: Consider moving the end statements out like this + | +LL | } +LL | x << 2; + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:189:5 + | +LL | / x * 4 +LL | | } + | |_____^ + | + = note: The end suggestion probably needs some adjustments to use the expression result correctly. +help: Consider moving the end statements out like this + | +LL | } +LL | x * 4 + | + +error: All if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:201:44 + | +LL | if x == 17 { b = 1; a = 0x99; } else { a = 0x99; } + | ^^^^^^^^^^^ + | +help: Consider moving the end statements out like this + | +LL | if x == 17 { b = 1; a = 0x99; } else { } +LL | a = 0x99; + | + +error: aborting due to 8 previous errors + diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr new file mode 100644 index 000000000000..c2bd8a070edb --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr @@ -0,0 +1,83 @@ +error: All if blocks contain the same code at the start + --> $DIR/shared_at_top.rs:10:5 + | +LL | / if true { +LL | | println!("Hello World!"); + | |_________________________________^ + | +note: the lint level is defined here + --> $DIR/shared_at_top.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: Consider moving the start statements out like this + | +LL | println!("Hello World!"); +LL | if true { + | + +error: All if blocks contain the same code at the start + --> $DIR/shared_at_top.rs:19:5 + | +LL | / if x == 0 { +LL | | let y = 9; +LL | | println!("The value y was set to: `{}`", y); +LL | | let _z = y; + | |___________________^ + | + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the start statements out like this + | +LL | let y = 9; +LL | println!("The value y was set to: `{}`", y); +LL | let _z = y; +LL | if x == 0 { + | + +error: All if blocks contain the same code at the start + --> $DIR/shared_at_top.rs:40:5 + | +LL | / let _ = if x == 7 { +LL | | let y = 16; + | |___________________^ + | +help: Consider moving the start statements out like this + | +LL | let y = 16; +LL | let _ = if x == 7 { + | + +error: All if blocks contain the same code at the start + --> $DIR/shared_at_top.rs:58:5 + | +LL | / if x == 10 { +LL | | let used_value_name = "Different type"; +LL | | println!("Str: {}", used_value_name); + | |_____________________________________________^ + | + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the start statements out like this + | +LL | let used_value_name = "Different type"; +LL | println!("Str: {}", used_value_name); +LL | if x == 10 { + | + +error: All if blocks contain the same code at the start + --> $DIR/shared_at_top.rs:72:5 + | +LL | / if x == 11 { +LL | | let can_be_overridden = "Move me"; +LL | | println!("I'm also moveable"); + | |______________________________________^ + | + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the start statements out like this + | +LL | let can_be_overridden = "Move me"; +LL | println!("I'm also moveable"); +LL | if x == 11 { + | + +error: aborting due to 5 previous errors + 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 70f969aaf2ef..46a8f931aaff 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 @@ -87,4 +87,33 @@ fn complexer_example() { } } +/// This should add a note to the lint msg since the moved expression is not `()` +fn added_note_for_expression_use() -> u32 { + let x = 9; + + let _ = if x == 7 { + let _ = 19; + + let _splitter = 6; + + x << 2 + } else { + let _ = 19; + + x << 2 + }; + + if x == 9 { + let _ = 17; + + let _splitter = 6; + + x * 4 + } else { + let _ = 17; + + x * 4 + } +} + fn main() {} diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr new file mode 100644 index 000000000000..1ba7211b469d --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr @@ -0,0 +1,154 @@ +error: All if blocks contain the same code at the start and the end. Here at the start: + --> $DIR/shared_at_top_and_bot.rs:16:5 + | +LL | / if x == 7 { +LL | | let t = 7; +LL | | let _overlap_start = t * 2; +LL | | let _overlap_end = 2 * t; + | |_________________________________^ + | +note: the lint level is defined here + --> $DIR/shared_at_top_and_bot.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: And here at the end: + --> $DIR/shared_at_top_and_bot.rs:28:5 + | +LL | / let _u = 9; +LL | | } + | |_____^ +help: Consider moving the start statements out like this: + | +LL | let t = 7; +LL | let _overlap_start = t * 2; +LL | let _overlap_end = 2 * t; +LL | if x == 7 { + | +help: And consider moving the end statements out like this: + | +LL | } +LL | let _u = 9; + | + +error: All if blocks contain the same code at the start and the end. Here at the start: + --> $DIR/shared_at_top_and_bot.rs:32:5 + | +LL | / if x == 99 { +LL | | let r = 7; +LL | | let _overlap_start = r; +LL | | let _overlap_middle = r * r; + | |____________________________________^ + | +note: And here at the end: + --> $DIR/shared_at_top_and_bot.rs:43:5 + | +LL | / let _overlap_end = r * r * r; +LL | | let z = "end"; +LL | | } + | |_____^ + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the start statements out like this: + | +LL | let r = 7; +LL | let _overlap_start = r; +LL | let _overlap_middle = r * r; +LL | if x == 99 { + | +help: And consider moving the end statements out like this: + | +LL | } +LL | let _overlap_end = r * r * r; +LL | let z = "end"; + | + +error: All if blocks contain the same code at the start and the end. Here at the start: + --> $DIR/shared_at_top_and_bot.rs:61:5 + | +LL | / if (x > 7 && y < 13) || (x + y) % 2 == 1 { +LL | | let a = 0xcafe; +LL | | let b = 0xffff00ff; +LL | | let e_id = gen_id(a, b); + | |________________________________^ + | +note: And here at the end: + --> $DIR/shared_at_top_and_bot.rs:81:5 + | +LL | / let pack = DataPack { +LL | | id: e_id, +LL | | name: "Player 1".to_string(), +LL | | some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90], +LL | | }; +LL | | process_data(pack); +LL | | } + | |_____^ + = warning: Some moved values might need to be renamed to avoid wrong references. +help: Consider moving the start statements out like this: + | +LL | let a = 0xcafe; +LL | let b = 0xffff00ff; +LL | let e_id = gen_id(a, b); +LL | if (x > 7 && y < 13) || (x + y) % 2 == 1 { + | +help: And consider moving the end statements out like this: + | +LL | } +LL | let pack = DataPack { +LL | id: e_id, +LL | name: "Player 1".to_string(), +LL | some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90], +LL | }; + ... + +error: All if blocks contain the same code at the start and the end. Here at the start: + --> $DIR/shared_at_top_and_bot.rs:94:5 + | +LL | / let _ = if x == 7 { +LL | | let _ = 19; + | |___________________^ + | +note: And here at the end: + --> $DIR/shared_at_top_and_bot.rs:103:5 + | +LL | / x << 2 +LL | | }; + | |_____^ + = note: The end suggestion probably needs some adjustments to use the expression result correctly. +help: Consider moving the start statements out like this: + | +LL | let _ = 19; +LL | let _ = if x == 7 { + | +help: And consider moving the end statements out like this: + | +LL | } +LL | x << 2; + | + +error: All if blocks contain the same code at the start and the end. Here at the start: + --> $DIR/shared_at_top_and_bot.rs:106:5 + | +LL | / if x == 9 { +LL | | let _ = 17; + | |___________________^ + | +note: And here at the end: + --> $DIR/shared_at_top_and_bot.rs:115:5 + | +LL | / x * 4 +LL | | } + | |_____^ + = note: The end suggestion probably needs some adjustments to use the expression result correctly. +help: Consider moving the start statements out like this: + | +LL | let _ = 17; +LL | if x == 9 { + | +help: And consider moving the end statements out like this: + | +LL | } +LL | x * 4 + | + +error: aborting due to 5 previous errors + diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr new file mode 100644 index 000000000000..003c060f0723 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr @@ -0,0 +1,107 @@ +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:102:15 + | +LL | if x == 0 { + | _______________^ +LL | | let u = 19; +LL | | println!("How are u today?"); +LL | | let _ = "This is a string"; +LL | | } else { + | |_____^ + | +note: the lint level is defined here + --> $DIR/valid_if_blocks.rs:2:9 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +note: same as this + --> $DIR/valid_if_blocks.rs:106:12 + | +LL | } else { + | ____________^ +LL | | let u = 19; +LL | | println!("How are u today?"); +LL | | let _ = "This is a string"; +LL | | } + | |_____^ + +error: All if blocks contain the same code at the end + --> $DIR/valid_if_blocks.rs:125:5 + | +LL | / let pet = Duck { num: 18 }; +LL | | println!("{:?}", pet); +LL | | } + | |_____^ + | +note: the lint level is defined here + --> $DIR/valid_if_blocks.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: Consider moving the end statements out like this + | +LL | } +LL | let pet = Duck { num: 18 }; +LL | println!("{:?}", pet); + | + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:130:23 + | +LL | let _ = if x == 6 { 7 } else { 7 }; + | ^^^^^ + | +note: same as this + --> $DIR/valid_if_blocks.rs:130:34 + | +LL | let _ = if x == 6 { 7 } else { 7 }; + | ^^^^^ + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:136:23 + | +LL | } else if x == 68 { + | _______________________^ +LL | | println!("I'm a doppelgänger"); +LL | | // Don't listen to my clone below +LL | | +... | +LL | | } +LL | | } else { + | |_____^ + | +note: same as this + --> $DIR/valid_if_blocks.rs:145:12 + | +LL | } else { + | ____________^ +LL | | // Don't listen to my clone above +LL | | println!("I'm a doppelgänger"); +LL | | +... | +LL | | } +LL | | }; + | |_____^ + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:158:23 + | +LL | } else if x == 68 { + | _______________________^ +LL | | println!("I'm a doppelgänger"); +LL | | // Don't listen to my clone below +LL | | } else { + | |_____^ + | +note: same as this + --> $DIR/valid_if_blocks.rs:161:12 + | +LL | } else { + | ____________^ +LL | | // Don't listen to my clone above +LL | | println!("I'm a doppelgänger"); +LL | | } + | |_____^ + +error: aborting due to 5 previous errors + From 65ed5a632f5a5fc99a015e7ad0537c401fb61023 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 16 Jan 2021 21:04:47 +0100 Subject: [PATCH 06/11] Updated code for dogfood --- clippy_lints/src/copies.rs | 142 +++++++++--------- .../shared_code_in_if_blocks/shared_at_top.rs | 20 +++ .../shared_at_top.stderr | 40 ++++- 3 files changed, 134 insertions(+), 68 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 47732f4766db..4a701dc8898d 100644 --- a/clippy_lints/src/copies.rs +++ b/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, 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>, diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.rs b/tests/ui/shared_code_in_if_blocks/shared_at_top.rs index 0a6828127e99..496939f2a5e8 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.rs +++ b/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() {} diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr index c2bd8a070edb..d217083c413a 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr +++ b/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 From c74e49eab911ce5d55b3692324dea64905323a88 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 23 Feb 2021 21:16:19 +0100 Subject: [PATCH 07/11] Adapted the lint to use the new SpanlessEq --- clippy_lints/src/copies.rs | 28 +++++--- clippy_utils/src/hir_utils.rs | 9 +-- tests/ui/if_same_then_else.stderr | 28 +------- tests/ui/if_same_then_else2.stderr | 40 +---------- .../shared_code_in_if_blocks/shared_at_bot.rs | 15 +--- .../shared_at_bot.stderr | 72 +++++++++++-------- .../shared_at_top.stderr | 24 +++---- .../shared_at_top_and_bot.stderr | 40 +++++------ .../valid_if_blocks.rs | 17 ----- .../valid_if_blocks.stderr | 34 ++------- 10 files changed, 111 insertions(+), 196 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 4a701dc8898d..7a9f299d8e04 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -295,11 +295,21 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, let l_stmts = win[0].stmts; let r_stmts = win[1].stmts; + // `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. + // The comparison therefor needs to be done in a way that builds the correct context. let mut evaluator = SpanlessEq::new(cx); + let mut evaluator = evaluator.inter_expr(); + 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_end_eq = { + // We skip the middle statements which can't be equal + let end_comparison_count = l_stmts.len().min(r_stmts.len()) - current_start_eq; + let it1 = l_stmts.iter().skip(l_stmts.len() - end_comparison_count); + let it2 = r_stmts.iter().skip(r_stmts.len() - end_comparison_count); + it1.zip(it2) + .fold(0, |acc, (l, r)| if evaluator.eq_stmt(l, r) { acc + 1 } else { 0 }) + }; let block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r)); // IF_SAME_THEN_ELSE @@ -458,8 +468,8 @@ fn emit_shared_code_in_if_blocks_lint( // Emit lint if suggestions.len() == 1 { 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); + 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_then(cx, SHARED_CODE_IN_IF_BLOCKS, span, msg.as_str(), |diag| { diag.span_suggestion(span, help.as_str(), sugg, Applicability::Unspecified); @@ -472,20 +482,20 @@ fn emit_shared_code_in_if_blocks_lint( cx, SHARED_CODE_IN_IF_BLOCKS, start_span, - "All if blocks contain the same code at the start and the end. Here at the start:", + "all if blocks contain the same code at the start and the end. Here at the start", move |diag| { - diag.span_note(end_span, "And here at the end:"); + diag.span_note(end_span, "and here at the end"); diag.span_suggestion( start_span, - "Consider moving the start statements out like this:", + "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:", + "and consider moving the end statements out like this", end_sugg, Applicability::Unspecified, ); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 8b90fedbafef..000c249bb0ff 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -58,13 +58,14 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { /// Use this method to wrap comparisons that may involve inter-expression context. /// See `self.locals`. - fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> { + pub fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> { HirEqInterExpr { inner: self, locals: HirIdMap::default(), } } + #[allow(dead_code)] pub fn eq_block(&mut self, left: &Block<'_>, right: &Block<'_>) -> bool { self.inter_expr().eq_block(left, right) } @@ -82,7 +83,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } } -struct HirEqInterExpr<'a, 'b, 'tcx> { +pub struct HirEqInterExpr<'a, 'b, 'tcx> { inner: &'a mut SpanlessEq<'b, 'tcx>, // When binding are declared, the binding ID in the left expression is mapped to the one on the @@ -92,7 +93,7 @@ struct HirEqInterExpr<'a, 'b, 'tcx> { } impl HirEqInterExpr<'_, '_, '_> { - fn eq_stmt(&mut self, left: &Stmt<'_>, right: &Stmt<'_>) -> bool { + pub fn eq_stmt(&mut self, left: &Stmt<'_>, right: &Stmt<'_>) -> bool { match (&left.kind, &right.kind) { (&StmtKind::Local(ref l), &StmtKind::Local(ref r)) => { self.eq_pat(&l.pat, &r.pat) @@ -159,7 +160,7 @@ impl HirEqInterExpr<'_, '_, '_> { } #[allow(clippy::similar_names)] - fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool { + pub fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool { if !self.inner.allow_side_effects && differing_macro_contexts(left.span, right.span) { return false; } diff --git a/tests/ui/if_same_then_else.stderr b/tests/ui/if_same_then_else.stderr index 2f38052fc209..74b11bac487e 100644 --- a/tests/ui/if_same_then_else.stderr +++ b/tests/ui/if_same_then_else.stderr @@ -82,31 +82,5 @@ LL | | 42 LL | | }; | |_____^ -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:95:13 - | -LL | if true { - | _____________^ -LL | | let bar = if true { 42 } else { 43 }; -LL | | -LL | | while foo() { -... | -LL | | bar + 1; -LL | | } else { - | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:102:12 - | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block -LL | | let bar = if true { 42 } else { 43 }; -LL | | -... | -LL | | bar + 1; -LL | | } - | |_____^ - -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/if_same_then_else2.stderr b/tests/ui/if_same_then_else2.stderr index b31eb46a9826..a0e636e3a611 100644 --- a/tests/ui/if_same_then_else2.stderr +++ b/tests/ui/if_same_then_else2.stderr @@ -1,19 +1,5 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:21:12 - | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block -LL | | for _ in &[42] { -LL | | let bar: &Option<_> = &Some::(42); -... | -LL | | } -LL | | } - | |_____^ - | - = note: `-D clippy::if-same-then-else` implied by `-D warnings` -note: same as this - --> $DIR/if_same_then_else2.rs:12:13 + --> $DIR/if_same_then_else2.rs:13:13 | LL | if true { | _____________^ @@ -33,7 +19,7 @@ LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block LL | | for _ in &[42] { -LL | | let foo: &Option<_> = &Some::(42); +LL | | let bar: &Option<_> = &Some::(42); ... | LL | | } LL | | } @@ -115,25 +101,5 @@ LL | | Ok("foo")?; LL | | } | |_____^ -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:122:20 - | -LL | } else if true { - | ____________________^ -LL | | let foo = ""; -LL | | return Ok(&foo[0..]); -LL | | } else { - | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:125:12 - | -LL | } else { - | ____________^ -LL | | let foo = ""; -LL | | return Ok(&foo[0..]); -LL | | } - | |_____^ - -error: aborting due to 6 previous errors +error: aborting due to 5 previous errors 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 5392da7ac7ac..7974ea2f59c8 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 @@ -4,7 +4,6 @@ // This tests the shared_code_in_if_blocks lint at the end of blocks fn simple_examples() { - // TODO xFrednet 2021-01-06: Test with const literals at the end let x = 1; let _ = if x == 7 { @@ -45,8 +44,8 @@ fn simple_examples() { 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"); + println!("Same end of block"); } // Use of outer scope value @@ -69,21 +68,11 @@ fn simple_examples() { ); } - // 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!("---"); + println!("Hello World"); } else { println!("Hello World"); } diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr index 62a95157d618..5ecf47bf9206 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr @@ -1,5 +1,5 @@ -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:31:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:30:5 | LL | / let result = false; LL | | println!("Block end!"); @@ -13,7 +13,7 @@ note: the lint level is defined here LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: The end suggestion probably needs some adjustments to use the expression result correctly. -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | let result = false; @@ -21,8 +21,21 @@ LL | println!("Block end!"); LL | result; | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:66:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:48:5 + | +LL | / println!("Same end of block"); +LL | | } + | |_____^ + | +help: consider moving the end statements out like this + | +LL | } +LL | println!("Same end of block"); + | + +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:65:5 | LL | / println!( LL | | "I'm moveable because I know: `outer_scope_value`: '{}'", @@ -31,7 +44,7 @@ LL | | ); LL | | } | |_____^ | -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | println!( @@ -40,22 +53,21 @@ LL | outer_scope_value LL | ); | -error: All if blocks contain the same code at the start - --> $DIR/shared_at_bot.rs:83:9 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:77:9 | -LL | / if x == 8 { -LL | | // No parent!! -LL | | println!("Hello World"); - | |____________________________________^ +LL | / println!("Hello World"); +LL | | } + | |_________^ | -help: Consider moving the start statements out like this +help: consider moving the end statements out like this | +LL | } LL | println!("Hello World"); -LL | if x == 8 { | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:104:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:93:5 | LL | / let later_used_value = "A string value"; LL | | println!("{}", later_used_value); @@ -64,15 +76,15 @@ LL | | } | |_____^ | = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | let later_used_value = "A string value"; LL | println!("{}", later_used_value); | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:117:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:106:5 | LL | / let simple_examples = "I now identify as a &str :)"; LL | | println!("This is the new simple_example: {}", simple_examples); @@ -80,52 +92,52 @@ LL | | } | |_____^ | = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | let simple_examples = "I now identify as a &str :)"; LL | println!("This is the new simple_example: {}", simple_examples); | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:182:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:171:5 | LL | / x << 2 LL | | }; | |_____^ | = note: The end suggestion probably needs some adjustments to use the expression result correctly. -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | x << 2; | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:189:5 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:178:5 | LL | / x * 4 LL | | } | |_____^ | = note: The end suggestion probably needs some adjustments to use the expression result correctly. -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | } LL | x * 4 | -error: All if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:201:44 +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bot.rs:190:44 | LL | if x == 17 { b = 1; a = 0x99; } else { a = 0x99; } | ^^^^^^^^^^^ | -help: Consider moving the end statements out like this +help: consider moving the end statements out like this | LL | if x == 17 { b = 1; a = 0x99; } else { } LL | a = 0x99; | -error: aborting due to 8 previous errors +error: aborting due to 9 previous errors diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr index d217083c413a..1ad924aba6a7 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr @@ -1,4 +1,4 @@ -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:10:5 | LL | / if true { @@ -10,13 +10,13 @@ note: the lint level is defined here | LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: Consider moving the start statements out like this +help: consider moving the start statements out like this | LL | println!("Hello World!"); LL | if true { | -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:19:5 | LL | / if x == 0 { @@ -26,7 +26,7 @@ LL | | let _z = y; | |___________________^ | = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the start statements out like this +help: consider moving the start statements out like this | LL | let y = 9; LL | println!("The value y was set to: `{}`", y); @@ -34,20 +34,20 @@ LL | let _z = y; LL | if x == 0 { | -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:40:5 | LL | / let _ = if x == 7 { LL | | let y = 16; | |___________________^ | -help: Consider moving the start statements out like this +help: consider moving the start statements out like this | LL | let y = 16; LL | let _ = if x == 7 { | -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:58:5 | LL | / if x == 10 { @@ -56,14 +56,14 @@ LL | | println!("Str: {}", used_value_name); | |_____________________________________________^ | = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the start statements out like this +help: consider moving the start statements out like this | LL | let used_value_name = "Different type"; LL | println!("Str: {}", used_value_name); LL | if x == 10 { | -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:72:5 | LL | / if x == 11 { @@ -72,14 +72,14 @@ LL | | println!("I'm also moveable"); | |______________________________________^ | = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the start statements out like this +help: consider moving the start statements out like this | LL | let can_be_overridden = "Move me"; LL | println!("I'm also moveable"); LL | if x == 11 { | -error: All if blocks contain the same code at the start +error: all if blocks contain the same code at the start --> $DIR/shared_at_top.rs:88:5 | LL | / if x == 2020 { @@ -87,7 +87,7 @@ 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 +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"); diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr index 1ba7211b469d..9f675a20a6d9 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr @@ -1,4 +1,4 @@ -error: All if blocks contain the same code at the start and the end. Here at the start: +error: all if blocks contain the same code at the start and the end. Here at the start --> $DIR/shared_at_top_and_bot.rs:16:5 | LL | / if x == 7 { @@ -12,26 +12,26 @@ note: the lint level is defined here | LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: And here at the end: +note: and here at the end --> $DIR/shared_at_top_and_bot.rs:28:5 | LL | / let _u = 9; LL | | } | |_____^ -help: Consider moving the start statements out like this: +help: consider moving the start statements out like this | LL | let t = 7; LL | let _overlap_start = t * 2; LL | let _overlap_end = 2 * t; LL | if x == 7 { | -help: And consider moving the end statements out like this: +help: and consider moving the end statements out like this | LL | } LL | let _u = 9; | -error: All if blocks contain the same code at the start and the end. Here at the start: +error: all if blocks contain the same code at the start and the end. Here at the start --> $DIR/shared_at_top_and_bot.rs:32:5 | LL | / if x == 99 { @@ -40,7 +40,7 @@ LL | | let _overlap_start = r; LL | | let _overlap_middle = r * r; | |____________________________________^ | -note: And here at the end: +note: and here at the end --> $DIR/shared_at_top_and_bot.rs:43:5 | LL | / let _overlap_end = r * r * r; @@ -48,21 +48,21 @@ LL | | let z = "end"; LL | | } | |_____^ = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the start statements out like this: +help: consider moving the start statements out like this | LL | let r = 7; LL | let _overlap_start = r; LL | let _overlap_middle = r * r; LL | if x == 99 { | -help: And consider moving the end statements out like this: +help: and consider moving the end statements out like this | LL | } LL | let _overlap_end = r * r * r; LL | let z = "end"; | -error: All if blocks contain the same code at the start and the end. Here at the start: +error: all if blocks contain the same code at the start and the end. Here at the start --> $DIR/shared_at_top_and_bot.rs:61:5 | LL | / if (x > 7 && y < 13) || (x + y) % 2 == 1 { @@ -71,7 +71,7 @@ LL | | let b = 0xffff00ff; LL | | let e_id = gen_id(a, b); | |________________________________^ | -note: And here at the end: +note: and here at the end --> $DIR/shared_at_top_and_bot.rs:81:5 | LL | / let pack = DataPack { @@ -83,14 +83,14 @@ LL | | process_data(pack); LL | | } | |_____^ = warning: Some moved values might need to be renamed to avoid wrong references. -help: Consider moving the start statements out like this: +help: consider moving the start statements out like this | LL | let a = 0xcafe; LL | let b = 0xffff00ff; LL | let e_id = gen_id(a, b); LL | if (x > 7 && y < 13) || (x + y) % 2 == 1 { | -help: And consider moving the end statements out like this: +help: and consider moving the end statements out like this | LL | } LL | let pack = DataPack { @@ -100,51 +100,51 @@ LL | some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90], LL | }; ... -error: All if blocks contain the same code at the start and the end. Here at the start: +error: all if blocks contain the same code at the start and the end. Here at the start --> $DIR/shared_at_top_and_bot.rs:94:5 | LL | / let _ = if x == 7 { LL | | let _ = 19; | |___________________^ | -note: And here at the end: +note: and here at the end --> $DIR/shared_at_top_and_bot.rs:103:5 | LL | / x << 2 LL | | }; | |_____^ = note: The end suggestion probably needs some adjustments to use the expression result correctly. -help: Consider moving the start statements out like this: +help: consider moving the start statements out like this | LL | let _ = 19; LL | let _ = if x == 7 { | -help: And consider moving the end statements out like this: +help: and consider moving the end statements out like this | LL | } LL | x << 2; | -error: All if blocks contain the same code at the start and the end. Here at the start: +error: all if blocks contain the same code at the start and the end. Here at the start --> $DIR/shared_at_top_and_bot.rs:106:5 | LL | / if x == 9 { LL | | let _ = 17; | |___________________^ | -note: And here at the end: +note: and here at the end --> $DIR/shared_at_top_and_bot.rs:115:5 | LL | / x * 4 LL | | } | |_____^ = note: The end suggestion probably needs some adjustments to use the expression result correctly. -help: Consider moving the start statements out like this: +help: consider moving the start statements out like this | LL | let _ = 17; LL | if x == 9 { | -help: And consider moving the end statements out like this: +help: and consider moving the end statements out like this | LL | } LL | x * 4 diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs index 480758777f16..a564b30cb1cc 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs @@ -109,23 +109,6 @@ fn trigger_other_lint() { let _ = "This is a string"; } - // More complex same blocks - if x == 17 { - #[derive(Debug)] - struct Duck { - num: u64, - }; - let pet = Duck { num: 18 }; - println!("{:?}", pet); - } else { - #[derive(Debug)] - struct Duck { - num: u64, - }; - let pet = Duck { num: 18 }; - println!("{:?}", pet); - } - // Only same expression let _ = if x == 6 { 7 } else { 7 }; diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr index 003c060f0723..d290c65822b3 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr @@ -25,40 +25,20 @@ LL | | let _ = "This is a string"; LL | | } | |_____^ -error: All if blocks contain the same code at the end - --> $DIR/valid_if_blocks.rs:125:5 - | -LL | / let pet = Duck { num: 18 }; -LL | | println!("{:?}", pet); -LL | | } - | |_____^ - | -note: the lint level is defined here - --> $DIR/valid_if_blocks.rs:2:36 - | -LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: Consider moving the end statements out like this - | -LL | } -LL | let pet = Duck { num: 18 }; -LL | println!("{:?}", pet); - | - error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:130:23 + --> $DIR/valid_if_blocks.rs:113:23 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ | note: same as this - --> $DIR/valid_if_blocks.rs:130:34 + --> $DIR/valid_if_blocks.rs:113:34 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:136:23 + --> $DIR/valid_if_blocks.rs:119:23 | LL | } else if x == 68 { | _______________________^ @@ -71,7 +51,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:145:12 + --> $DIR/valid_if_blocks.rs:128:12 | LL | } else { | ____________^ @@ -84,7 +64,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:158:23 + --> $DIR/valid_if_blocks.rs:141:23 | LL | } else if x == 68 { | _______________________^ @@ -94,7 +74,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:161:12 + --> $DIR/valid_if_blocks.rs:144:12 | LL | } else { | ____________^ @@ -103,5 +83,5 @@ LL | | println!("I'm a doppelgänger"); LL | | } | |_____^ -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors From 617c65baa90cdbd9228cacad4f2079a9d868d070 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 25 Feb 2021 23:33:46 +0100 Subject: [PATCH 08/11] Moving shared_code_in_if_blocks to clippy::complexity and running lintcheck --- clippy_lints/src/copies.rs | 6 +-- clippy_lints/src/lib.rs | 2 +- clippy_utils/src/hir_utils.rs | 6 ++- clippy_utils/src/lib.rs | 18 +++++-- .../complex_conditionals_nested.rs | 2 +- .../ui/checked_unwrap/simple_conditionals.rs | 2 +- tests/ui/default_numeric_fallback.rs | 1 + tests/ui/default_numeric_fallback.stderr | 48 +++++++++---------- tests/ui/if_same_then_else.stderr | 28 ++++++++++- tests/ui/if_same_then_else2.stderr | 22 ++++++++- tests/ui/needless_bool/simple.rs | 3 +- tests/ui/needless_bool/simple.stderr | 8 ++-- tests/ui/needless_return.fixed | 9 +++- tests/ui/needless_return.rs | 9 +++- tests/ui/needless_return.stderr | 36 +++++++------- .../shared_code_in_if_blocks/shared_at_bot.rs | 16 +++++++ .../shared_at_bot.stderr | 10 ++-- .../shared_at_top.stderr | 6 +-- .../shared_at_top_and_bot.stderr | 8 ++-- .../valid_if_blocks.rs | 8 ++++ .../valid_if_blocks.stderr | 16 +++---- 21 files changed, 180 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 7a9f299d8e04..8b94ac96880d 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -142,7 +142,7 @@ declare_clippy_lint! { /// }; /// ``` pub SHARED_CODE_IN_IF_BLOCKS, - nursery, + complexity, "`if` statement with shared code in all blocks" } @@ -457,11 +457,11 @@ fn emit_shared_code_in_if_blocks_lint( let add_optional_msgs = |diag: &mut DiagnosticBuilder<'_>| { if add_expr_note { - diag.note("The end suggestion probably needs some adjustments to use the expression result correctly."); + diag.note("The end suggestion probably needs some adjustments to use the expression result correctly"); } if warn_about_moved_symbol { - diag.warn("Some moved values might need to be renamed to avoid wrong references."); + diag.warn("Some moved values might need to be renamed to avoid wrong references"); } }; diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9afbf95a342c..3f5c1c7c5269 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1485,6 +1485,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), @@ -2063,7 +2064,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(&attrs::EMPTY_LINE_AFTER_OUTER_ATTR), LintId::of(&cognitive_complexity::COGNITIVE_COMPLEXITY), - LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(&disallowed_method::DISALLOWED_METHOD), LintId::of(&fallible_impl_from::FALLIBLE_IMPL_FROM), LintId::of(&floating_point_arithmetic::IMPRECISE_FLOPS), diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 000c249bb0ff..571eec9e530f 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -96,9 +96,11 @@ impl HirEqInterExpr<'_, '_, '_> { pub fn eq_stmt(&mut self, left: &Stmt<'_>, right: &Stmt<'_>) -> bool { match (&left.kind, &right.kind) { (&StmtKind::Local(ref l), &StmtKind::Local(ref r)) => { - self.eq_pat(&l.pat, &r.pat) + // eq_pat adds the HirIds to the locals map. We therefor call it last to make sure that + // these only get added if the init and type is equal. + both(&l.init, &r.init, |l, r| self.eq_expr(l, r)) && both(&l.ty, &r.ty, |l, r| self.eq_ty(l, r)) - && both(&l.init, &r.init, |l, r| self.eq_expr(l, r)) + && self.eq_pat(&l.pat, &r.pat) }, (&StmtKind::Expr(ref l), &StmtKind::Expr(ref r)) | (&StmtKind::Semi(ref l), &StmtKind::Semi(ref r)) => { self.eq_expr(l, r) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index d6364625e704..f9576068dd65 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -609,9 +609,9 @@ pub fn get_pat_name(pat: &Pat<'_>) -> Option { } } -struct ContainsName { - name: Symbol, - result: bool, +pub struct ContainsName { + pub name: Symbol, + pub result: bool, } impl<'tcx> Visitor<'tcx> for ContainsName { @@ -1216,6 +1216,8 @@ pub fn if_sequence<'tcx>(mut expr: &'tcx Expr<'tcx>) -> (Vec<&'tcx Expr<'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); @@ -1326,6 +1328,16 @@ pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { } } +/// This function checks if any of the lints in the slice is enabled for the provided `HirId`. +/// A lint counts as enabled with any of the levels: `Level::Forbid` | `Level::Deny` | `Level::Warn` +/// +/// ```ignore +/// #[deny(clippy::YOUR_AWESOME_LINT)] +/// println!("Hello, World!"); // <- Clippy code: run_lints(cx, &[YOUR_AWESOME_LINT], id) == true +/// +/// #[allow(clippy::YOUR_AWESOME_LINT)] +/// println!("See you soon!"); // <- Clippy code: run_lints(cx, &[YOUR_AWESOME_LINT], id) == false +/// ``` pub fn run_lints(cx: &LateContext<'_>, lints: &[&'static Lint], id: HirId) -> bool { lints.iter().any(|lint| { matches!( diff --git a/tests/ui/checked_unwrap/complex_conditionals_nested.rs b/tests/ui/checked_unwrap/complex_conditionals_nested.rs index 2307996a48ff..99e8fb954665 100644 --- a/tests/ui/checked_unwrap/complex_conditionals_nested.rs +++ b/tests/ui/checked_unwrap/complex_conditionals_nested.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_nested() { fn nested() { diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index 369676308348..0459100d88ac 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_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)] macro_rules! m { ($a:expr) => { diff --git a/tests/ui/default_numeric_fallback.rs b/tests/ui/default_numeric_fallback.rs index 0b3758952ac6..de89f806c585 100644 --- a/tests/ui/default_numeric_fallback.rs +++ b/tests/ui/default_numeric_fallback.rs @@ -3,6 +3,7 @@ #![allow(clippy::never_loop)] #![allow(clippy::no_effect)] #![allow(clippy::unnecessary_operation)] +#![allow(clippy::shared_code_in_if_blocks)] mod basic_expr { fn test() { diff --git a/tests/ui/default_numeric_fallback.stderr b/tests/ui/default_numeric_fallback.stderr index b31aa4ebcf8e..d1c4c8203dd8 100644 --- a/tests/ui/default_numeric_fallback.stderr +++ b/tests/ui/default_numeric_fallback.stderr @@ -1,5 +1,5 @@ error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:10:17 + --> $DIR/default_numeric_fallback.rs:11:17 | LL | let x = 22; | ^^ help: consider adding suffix: `22_i32` @@ -7,139 +7,139 @@ LL | let x = 22; = note: `-D clippy::default-numeric-fallback` implied by `-D warnings` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:11:18 + --> $DIR/default_numeric_fallback.rs:12:18 | LL | let x = [1, 2, 3]; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:11:21 + --> $DIR/default_numeric_fallback.rs:12:21 | LL | let x = [1, 2, 3]; | ^ help: consider adding suffix: `2_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:11:24 + --> $DIR/default_numeric_fallback.rs:12:24 | LL | let x = [1, 2, 3]; | ^ help: consider adding suffix: `3_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:12:28 + --> $DIR/default_numeric_fallback.rs:13:28 | LL | let x = if true { (1, 2) } else { (3, 4) }; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:12:31 + --> $DIR/default_numeric_fallback.rs:13:31 | LL | let x = if true { (1, 2) } else { (3, 4) }; | ^ help: consider adding suffix: `2_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:12:44 + --> $DIR/default_numeric_fallback.rs:13:44 | LL | let x = if true { (1, 2) } else { (3, 4) }; | ^ help: consider adding suffix: `3_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:12:47 + --> $DIR/default_numeric_fallback.rs:13:47 | LL | let x = if true { (1, 2) } else { (3, 4) }; | ^ help: consider adding suffix: `4_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:13:23 + --> $DIR/default_numeric_fallback.rs:14:23 | LL | let x = match 1 { | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:14:13 + --> $DIR/default_numeric_fallback.rs:15:13 | LL | 1 => 1, | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:14:18 + --> $DIR/default_numeric_fallback.rs:15:18 | LL | 1 => 1, | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:15:18 + --> $DIR/default_numeric_fallback.rs:16:18 | LL | _ => 2, | ^ help: consider adding suffix: `2_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:19:17 + --> $DIR/default_numeric_fallback.rs:20:17 | LL | let x = 0.12; | ^^^^ help: consider adding suffix: `0.12_f64` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:37:21 + --> $DIR/default_numeric_fallback.rs:38:21 | LL | let y = 1; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:45:21 + --> $DIR/default_numeric_fallback.rs:46:21 | LL | let y = 1; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:51:21 + --> $DIR/default_numeric_fallback.rs:52:21 | LL | let y = 1; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:63:9 + --> $DIR/default_numeric_fallback.rs:64:9 | LL | 1 | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:69:27 + --> $DIR/default_numeric_fallback.rs:70:27 | LL | let f = || -> _ { 1 }; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:73:29 + --> $DIR/default_numeric_fallback.rs:74:29 | LL | let f = || -> i32 { 1 }; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:87:21 + --> $DIR/default_numeric_fallback.rs:88:21 | LL | generic_arg(1); | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:90:32 + --> $DIR/default_numeric_fallback.rs:91:32 | LL | let x: _ = generic_arg(1); | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:108:28 + --> $DIR/default_numeric_fallback.rs:109:28 | LL | GenericStruct { x: 1 }; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:111:36 + --> $DIR/default_numeric_fallback.rs:112:36 | LL | let _ = GenericStruct { x: 1 }; | ^ help: consider adding suffix: `1_i32` error: default numeric fallback might occur - --> $DIR/default_numeric_fallback.rs:131:23 + --> $DIR/default_numeric_fallback.rs:132:23 | LL | s.generic_arg(1); | ^ help: consider adding suffix: `1_i32` diff --git a/tests/ui/if_same_then_else.stderr b/tests/ui/if_same_then_else.stderr index 74b11bac487e..2f38052fc209 100644 --- a/tests/ui/if_same_then_else.stderr +++ b/tests/ui/if_same_then_else.stderr @@ -82,5 +82,31 @@ LL | | 42 LL | | }; | |_____^ -error: aborting due to 4 previous errors +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:95:13 + | +LL | if true { + | _____________^ +LL | | let bar = if true { 42 } else { 43 }; +LL | | +LL | | while foo() { +... | +LL | | bar + 1; +LL | | } else { + | |_____^ + | +note: same as this + --> $DIR/if_same_then_else.rs:102:12 + | +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block +LL | | let bar = if true { 42 } else { 43 }; +LL | | +... | +LL | | bar + 1; +LL | | } + | |_____^ + +error: aborting due to 5 previous errors diff --git a/tests/ui/if_same_then_else2.stderr b/tests/ui/if_same_then_else2.stderr index a0e636e3a611..6524be0af851 100644 --- a/tests/ui/if_same_then_else2.stderr +++ b/tests/ui/if_same_then_else2.stderr @@ -101,5 +101,25 @@ LL | | Ok("foo")?; LL | | } | |_____^ -error: aborting due to 5 previous errors +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:122:20 + | +LL | } else if true { + | ____________________^ +LL | | let foo = ""; +LL | | return Ok(&foo[0..]); +LL | | } else { + | |_____^ + | +note: same as this + --> $DIR/if_same_then_else2.rs:125:12 + | +LL | } else { + | ____________^ +LL | | let foo = ""; +LL | | return Ok(&foo[0..]); +LL | | } + | |_____^ + +error: aborting due to 6 previous errors diff --git a/tests/ui/needless_bool/simple.rs b/tests/ui/needless_bool/simple.rs index e9f1428fc3a4..678040fa98b3 100644 --- a/tests/ui/needless_bool/simple.rs +++ b/tests/ui/needless_bool/simple.rs @@ -4,7 +4,8 @@ dead_code, clippy::no_effect, clippy::if_same_then_else, - clippy::needless_return + clippy::needless_return, + clippy::shared_code_in_if_blocks )] fn main() { diff --git a/tests/ui/needless_bool/simple.stderr b/tests/ui/needless_bool/simple.stderr index c57a8a042fb8..0ccc9416bcd5 100644 --- a/tests/ui/needless_bool/simple.stderr +++ b/tests/ui/needless_bool/simple.stderr @@ -1,5 +1,5 @@ error: this if-then-else expression will always return true - --> $DIR/simple.rs:13:5 + --> $DIR/simple.rs:14:5 | LL | / if x { LL | | true @@ -11,7 +11,7 @@ LL | | }; = note: `-D clippy::needless-bool` implied by `-D warnings` error: this if-then-else expression will always return false - --> $DIR/simple.rs:18:5 + --> $DIR/simple.rs:19:5 | LL | / if x { LL | | false @@ -21,7 +21,7 @@ LL | | }; | |_____^ error: this if-then-else expression will always return true - --> $DIR/simple.rs:33:5 + --> $DIR/simple.rs:34:5 | LL | / if x { LL | | return true; @@ -31,7 +31,7 @@ LL | | }; | |_____^ error: this if-then-else expression will always return false - --> $DIR/simple.rs:41:5 + --> $DIR/simple.rs:42:5 | LL | / if x { LL | | return false; diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 990475fcb587..ebf74cfef126 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -1,7 +1,12 @@ // run-rustfix -#![allow(unused, clippy::needless_bool)] -#![allow(clippy::if_same_then_else, clippy::single_match)] +#![allow(unused)] +#![allow( + clippy::if_same_then_else, + clippy::single_match, + clippy::shared_code_in_if_blocks, + clippy::needless_bool +)] #![warn(clippy::needless_return)] macro_rules! the_answer { diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index dec3d84a0204..2bebccdabca5 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -1,7 +1,12 @@ // run-rustfix -#![allow(unused, clippy::needless_bool)] -#![allow(clippy::if_same_then_else, clippy::single_match)] +#![allow(unused)] +#![allow( + clippy::if_same_then_else, + clippy::single_match, + clippy::shared_code_in_if_blocks, + clippy::needless_bool +)] #![warn(clippy::needless_return)] macro_rules! the_answer { diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index ae31d6075416..075db22456f7 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -1,5 +1,5 @@ error: unneeded `return` statement - --> $DIR/needless_return.rs:18:5 + --> $DIR/needless_return.rs:23:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` @@ -7,103 +7,103 @@ LL | return true; = note: `-D clippy::needless-return` implied by `-D warnings` error: unneeded `return` statement - --> $DIR/needless_return.rs:22:5 + --> $DIR/needless_return.rs:27:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:27:9 + --> $DIR/needless_return.rs:32:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:29:9 + --> $DIR/needless_return.rs:34:9 | LL | return false; | ^^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded `return` statement - --> $DIR/needless_return.rs:35:17 + --> $DIR/needless_return.rs:40:17 | LL | true => return false, | ^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded `return` statement - --> $DIR/needless_return.rs:37:13 + --> $DIR/needless_return.rs:42:13 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:44:9 + --> $DIR/needless_return.rs:49:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:46:16 + --> $DIR/needless_return.rs:51:16 | LL | let _ = || return true; | ^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:54:5 + --> $DIR/needless_return.rs:59:5 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:59:9 + --> $DIR/needless_return.rs:64:9 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:61:9 + --> $DIR/needless_return.rs:66:9 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:68:14 + --> $DIR/needless_return.rs:73:14 | LL | _ => return, | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:83:9 + --> $DIR/needless_return.rs:88:9 | LL | return String::from("test"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")` error: unneeded `return` statement - --> $DIR/needless_return.rs:85:9 + --> $DIR/needless_return.rs:90:9 | LL | return String::new(); | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` error: unneeded `return` statement - --> $DIR/needless_return.rs:106:32 + --> $DIR/needless_return.rs:111:32 | LL | bar.unwrap_or_else(|_| return) | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:111:13 + --> $DIR/needless_return.rs:116:13 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:113:20 + --> $DIR/needless_return.rs:118:20 | LL | let _ = || return; | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:119:32 + --> $DIR/needless_return.rs:124:32 | LL | res.unwrap_or_else(|_| return Foo) | ^^^^^^^^^^ help: remove `return`: `Foo` 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 7974ea2f59c8..6ff362cb7525 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 @@ -190,4 +190,20 @@ fn test_suggestion_with_weird_formatting() { if x == 17 { b = 1; a = 0x99; } else { a = 0x99; } } +fn fp_test() { + let x = 17; + + if x == 18 { + let y = 19; + if y < x { + println!("Trigger") + } + } else { + let z = 166; + if z < x { + println!("Trigger") + } + } +} + fn main() {} diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr index 5ecf47bf9206..2268d8f53655 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr @@ -12,7 +12,7 @@ note: the lint level is defined here | LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: The end suggestion probably needs some adjustments to use the expression result correctly. + = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the end statements out like this | LL | } @@ -75,7 +75,7 @@ LL | | // I'm expecting a note about this LL | | } | |_____^ | - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the end statements out like this | LL | } @@ -91,7 +91,7 @@ LL | | println!("This is the new simple_example: {}", simple_examples); LL | | } | |_____^ | - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the end statements out like this | LL | } @@ -106,7 +106,7 @@ LL | / x << 2 LL | | }; | |_____^ | - = note: The end suggestion probably needs some adjustments to use the expression result correctly. + = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the end statements out like this | LL | } @@ -120,7 +120,7 @@ LL | / x * 4 LL | | } | |_____^ | - = note: The end suggestion probably needs some adjustments to use the expression result correctly. + = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the end statements out like this | LL | } diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr index 1ad924aba6a7..76898a6ff02a 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr @@ -25,7 +25,7 @@ LL | | println!("The value y was set to: `{}`", y); LL | | let _z = y; | |___________________^ | - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the start statements out like this | LL | let y = 9; @@ -55,7 +55,7 @@ LL | | let used_value_name = "Different type"; LL | | println!("Str: {}", used_value_name); | |_____________________________________________^ | - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the start statements out like this | LL | let used_value_name = "Different type"; @@ -71,7 +71,7 @@ LL | | let can_be_overridden = "Move me"; LL | | println!("I'm also moveable"); | |______________________________________^ | - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the start statements out like this | LL | let can_be_overridden = "Move me"; diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr index 9f675a20a6d9..75c3d397f2e9 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr +++ b/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr @@ -47,7 +47,7 @@ LL | / let _overlap_end = r * r * r; LL | | let z = "end"; LL | | } | |_____^ - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the start statements out like this | LL | let r = 7; @@ -82,7 +82,7 @@ LL | | }; LL | | process_data(pack); LL | | } | |_____^ - = warning: Some moved values might need to be renamed to avoid wrong references. + = warning: Some moved values might need to be renamed to avoid wrong references help: consider moving the start statements out like this | LL | let a = 0xcafe; @@ -113,7 +113,7 @@ note: and here at the end LL | / x << 2 LL | | }; | |_____^ - = note: The end suggestion probably needs some adjustments to use the expression result correctly. + = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the start statements out like this | LL | let _ = 19; @@ -138,7 +138,7 @@ note: and here at the end LL | / x * 4 LL | | } | |_____^ - = note: The end suggestion probably needs some adjustments to use the expression result correctly. + = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the start statements out like this | LL | let _ = 17; diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs index a564b30cb1cc..cd397db47d0d 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs @@ -91,6 +91,14 @@ fn valid_examples() { let _ = (x, y, z); // I'm so much better than the x == 418 block. Trust me } + + let x = 1; + if true { + println!("{}", x); + } else { + let x = 2; + println!("{}", x); + } } /// This makes sure that the `if_same_then_else` masks the `shared_code_in_if_blocks` lint diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr index d290c65822b3..2061cc25d216 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr @@ -1,5 +1,5 @@ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:102:15 + --> $DIR/valid_if_blocks.rs:110:15 | LL | if x == 0 { | _______________^ @@ -15,7 +15,7 @@ note: the lint level is defined here LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: same as this - --> $DIR/valid_if_blocks.rs:106:12 + --> $DIR/valid_if_blocks.rs:114:12 | LL | } else { | ____________^ @@ -26,19 +26,19 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:113:23 + --> $DIR/valid_if_blocks.rs:121:23 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ | note: same as this - --> $DIR/valid_if_blocks.rs:113:34 + --> $DIR/valid_if_blocks.rs:121:34 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:119:23 + --> $DIR/valid_if_blocks.rs:127:23 | LL | } else if x == 68 { | _______________________^ @@ -51,7 +51,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:128:12 + --> $DIR/valid_if_blocks.rs:136:12 | LL | } else { | ____________^ @@ -64,7 +64,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:141:23 + --> $DIR/valid_if_blocks.rs:149:23 | LL | } else if x == 68 { | _______________________^ @@ -74,7 +74,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:144:12 + --> $DIR/valid_if_blocks.rs:152:12 | LL | } else { | ____________^ From 8c0b4d7f65802031f0131bc56de8fb8b275d70d7 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Fri, 26 Feb 2021 21:29:55 +0100 Subject: [PATCH 09/11] Only running shared_code_in_if_blocks only for if statements --- clippy_lints/src/copies.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 8b94ac96880d..ff99a9a09b3a 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -156,23 +156,25 @@ declare_lint_pass!(CopyAndPaste => [ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if !expr.span.from_expansion() { - // skip ifs directly in else, it will be checked in the parent if - if let Some(&Expr { - kind: ExprKind::If(_, _, Some(ref else_expr)), - .. - }) = get_parent_expr(cx, expr) - { - if else_expr.hir_id == expr.hir_id { - return; + if let ExprKind::If(_, _, _) = expr.kind { + // skip ifs directly in else, it will be checked in the parent if + if let Some(&Expr { + kind: ExprKind::If(_, _, Some(ref else_expr)), + .. + }) = get_parent_expr(cx, expr) + { + if else_expr.hir_id == expr.hir_id { + return; + } } - } - let (conds, blocks) = if_sequence(expr); - // 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(), expr); + let (conds, blocks) = if_sequence(expr); + // 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(), expr); + } } } } From 7c9e192e055f8b8a8a5f8b177c415440bc2333ce Mon Sep 17 00:00:00 2001 From: xFrednet Date: Fri, 5 Mar 2021 19:23:12 +0100 Subject: [PATCH 10/11] Test for empty blocks and update from master --- .../valid_if_blocks.rs | 17 +++---- .../valid_if_blocks.stderr | 50 ++++++++++++------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs index cd397db47d0d..e63490d5b079 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs @@ -99,6 +99,11 @@ fn valid_examples() { let x = 2; println!("{}", x); } + + // Let's test empty blocks + if false { + } else { + } } /// This makes sure that the `if_same_then_else` masks the `shared_code_in_if_blocks` lint @@ -128,20 +133,12 @@ fn trigger_other_lint() { println!("I'm a doppelgänger"); // Don't listen to my clone below - if y == 90 { - "=^.^=" - } else { - ":D" - } + if y == 90 { "=^.^=" } else { ":D" } } else { // Don't listen to my clone above println!("I'm a doppelgänger"); - if y == 90 { - "=^.^=" - } else { - ":D" - } + if y == 90 { "=^.^=" } else { ":D" } }; if x == 0 { diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr index 2061cc25d216..846581456dca 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr +++ b/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr @@ -1,11 +1,8 @@ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:110:15 + --> $DIR/valid_if_blocks.rs:104:14 | -LL | if x == 0 { - | _______________^ -LL | | let u = 19; -LL | | println!("How are u today?"); -LL | | let _ = "This is a string"; +LL | if false { + | ______________^ LL | | } else { | |_____^ | @@ -15,7 +12,26 @@ note: the lint level is defined here LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: same as this - --> $DIR/valid_if_blocks.rs:114:12 + --> $DIR/valid_if_blocks.rs:105:12 + | +LL | } else { + | ____________^ +LL | | } + | |_____^ + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:115:15 + | +LL | if x == 0 { + | _______________^ +LL | | let u = 19; +LL | | println!("How are u today?"); +LL | | let _ = "This is a string"; +LL | | } else { + | |_____^ + | +note: same as this + --> $DIR/valid_if_blocks.rs:119:12 | LL | } else { | ____________^ @@ -26,45 +42,43 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:121:23 + --> $DIR/valid_if_blocks.rs:126:23 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ | note: same as this - --> $DIR/valid_if_blocks.rs:121:34 + --> $DIR/valid_if_blocks.rs:126:34 | LL | let _ = if x == 6 { 7 } else { 7 }; | ^^^^^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:127:23 + --> $DIR/valid_if_blocks.rs:132:23 | LL | } else if x == 68 { | _______________________^ LL | | println!("I'm a doppelgänger"); LL | | // Don't listen to my clone below LL | | -... | -LL | | } +LL | | if y == 90 { "=^.^=" } else { ":D" } LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:136:12 + --> $DIR/valid_if_blocks.rs:137:12 | LL | } else { | ____________^ LL | | // Don't listen to my clone above LL | | println!("I'm a doppelgänger"); LL | | -... | -LL | | } +LL | | if y == 90 { "=^.^=" } else { ":D" } LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/valid_if_blocks.rs:149:23 + --> $DIR/valid_if_blocks.rs:146:23 | LL | } else if x == 68 { | _______________________^ @@ -74,7 +88,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/valid_if_blocks.rs:152:12 + --> $DIR/valid_if_blocks.rs:149:12 | LL | } else { | ____________^ @@ -83,5 +97,5 @@ LL | | println!("I'm a doppelgänger"); LL | | } | |_____^ -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors From a6f54f5dfdfdf0017ffecfbcd6f43352b8b71ca1 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 1 Apr 2021 18:30:47 +0200 Subject: [PATCH 11/11] Renaming the lint to branches_sharing_code and fixing typos --- CHANGELOG.md | 2 +- clippy_lints/src/copies.rs | 35 ++++++++++--------- clippy_lints/src/lib.rs | 5 +-- clippy_utils/src/lib.rs | 2 +- .../shared_at_bottom.rs} | 4 +-- .../shared_at_bottom.stderr} | 24 ++++++------- .../shared_at_top.rs | 4 +-- .../shared_at_top.stderr | 6 ++-- .../shared_at_top_and_bottom.rs} | 4 +-- .../shared_at_top_and_bottom.stderr} | 26 +++++++------- .../valid_if_blocks.rs | 6 ++-- .../valid_if_blocks.stderr | 2 +- .../ui/checked_unwrap/complex_conditionals.rs | 2 +- .../complex_conditionals_nested.rs | 2 +- .../ui/checked_unwrap/simple_conditionals.rs | 2 +- tests/ui/default_numeric_fallback.rs | 2 +- tests/ui/if_same_then_else.rs | 2 +- tests/ui/if_same_then_else2.rs | 2 +- tests/ui/let_if_seq.rs | 2 +- tests/ui/needless_bool/simple.rs | 2 +- tests/ui/needless_return.fixed | 2 +- tests/ui/needless_return.rs | 2 +- 22 files changed, 71 insertions(+), 69 deletions(-) rename tests/ui/{shared_code_in_if_blocks/shared_at_bot.rs => branches_sharing_code/shared_at_bottom.rs} (97%) rename tests/ui/{shared_code_in_if_blocks/shared_at_bot.stderr => branches_sharing_code/shared_at_bottom.stderr} (88%) rename tests/ui/{shared_code_in_if_blocks => branches_sharing_code}/shared_at_top.rs (95%) rename tests/ui/{shared_code_in_if_blocks => branches_sharing_code}/shared_at_top.stderr (95%) rename tests/ui/{shared_code_in_if_blocks/shared_at_top_and_bot.rs => branches_sharing_code/shared_at_top_and_bottom.rs} (94%) rename tests/ui/{shared_code_in_if_blocks/shared_at_top_and_bot.stderr => branches_sharing_code/shared_at_top_and_bottom.stderr} (87%) rename tests/ui/{shared_code_in_if_blocks => branches_sharing_code}/valid_if_blocks.rs (96%) rename tests/ui/{shared_code_in_if_blocks => branches_sharing_code}/valid_if_blocks.stderr (96%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 790bca4ac186..73997192ae0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2129,6 +2129,7 @@ Released 2018-09-13 [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box [`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local +[`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow [`bytes_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_nth [`cargo_common_metadata`]: https://rust-lang.github.io/rust-clippy/master/index.html#cargo_common_metadata @@ -2455,7 +2456,6 @@ 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 diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index ff99a9a09b3a..1b982221b065 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,7 +1,8 @@ -use crate::utils::{ - 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 clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then}; +use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt}; +use clippy_utils::{ + both, count_eq, eq_expr_value, get_enclosing_block, get_parent_expr, if_sequence, in_macro, parent_node_is_if_expr, + run_lints, search_same, ContainsName, SpanlessEq, SpanlessHash, }; use if_chain::if_chain; use rustc_data_structures::fx::FxHashSet; @@ -141,7 +142,7 @@ declare_clippy_lint! { /// 42 /// }; /// ``` - pub SHARED_CODE_IN_IF_BLOCKS, + pub BRANCHES_SHARING_CODE, complexity, "`if` statement with shared code in all blocks" } @@ -150,7 +151,7 @@ declare_lint_pass!(CopyAndPaste => [ IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, - SHARED_CODE_IN_IF_BLOCKS + BRANCHES_SHARING_CODE ]); impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { @@ -173,17 +174,17 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { 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(), expr); + lint_same_then_else(cx, &blocks, conds.len() == blocks.len(), expr); } } } } -/// Implementation of `SHARED_CODE_IN_IF_BLOCKS` and `IF_SAME_THEN_ELSE` if the blocks are equal. +/// Implementation of `BRANCHES_SHARING_CODE` and `IF_SAME_THEN_ELSE` if the blocks are equal. fn lint_same_then_else<'tcx>( cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>], - has_unconditional_else: bool, + has_conditional_else: bool, expr: &'tcx Expr<'_>, ) { // We only lint ifs with multiple blocks @@ -195,8 +196,8 @@ fn lint_same_then_else<'tcx>( 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)) { + // BRANCHES_SHARING_CODE prerequisites + if has_conditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) { return; } @@ -210,7 +211,7 @@ fn lint_same_then_else<'tcx>( intravisit::walk_stmt(&mut start_walker, stmt); } - emit_shared_code_in_if_blocks_lint( + emit_branches_sharing_code_lint( cx, start_eq, 0, @@ -277,7 +278,7 @@ fn lint_same_then_else<'tcx>( }); } - emit_shared_code_in_if_blocks_lint( + emit_branches_sharing_code_lint( cx, start_eq, end_eq, @@ -298,7 +299,7 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, let r_stmts = win[1].stmts; // `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. - // The comparison therefor needs to be done in a way that builds the correct context. + // The comparison therefore needs to be done in a way that builds the correct context. let mut evaluator = SpanlessEq::new(cx); let mut evaluator = evaluator.inter_expr(); @@ -387,7 +388,7 @@ fn check_for_warn_of_moved_symbol( }) } -fn emit_shared_code_in_if_blocks_lint( +fn emit_branches_sharing_code_lint( cx: &LateContext<'tcx>, start_stmts: usize, end_stmts: usize, @@ -472,7 +473,7 @@ fn emit_shared_code_in_if_blocks_lint( 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_then(cx, SHARED_CODE_IN_IF_BLOCKS, span, msg.as_str(), |diag| { + span_lint_and_then(cx, BRANCHES_SHARING_CODE, span, msg.as_str(), |diag| { diag.span_suggestion(span, help.as_str(), sugg, Applicability::Unspecified); add_optional_msgs(diag); @@ -482,7 +483,7 @@ fn emit_shared_code_in_if_blocks_lint( let (_, start_span, start_sugg) = suggestions.pop().unwrap(); span_lint_and_then( cx, - SHARED_CODE_IN_IF_BLOCKS, + BRANCHES_SHARING_CODE, start_span, "all if blocks contain the same code at the start and the end. Here at the start", move |diag| { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3f5c1c7c5269..43e89c2475f3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -613,10 +613,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &collapsible_if::COLLAPSIBLE_IF, &collapsible_match::COLLAPSIBLE_MATCH, &comparison_chain::COMPARISON_CHAIN, + &copies::BRANCHES_SHARING_CODE, &copies::IFS_SAME_COND, &copies::IF_SAME_THEN_ELSE, &copies::SAME_FUNCTIONS_IN_IF_CONDITION, - &copies::SHARED_CODE_IN_IF_BLOCKS, ©_iterator::COPY_ITERATOR, &create_dir::CREATE_DIR, &dbg_macro::DBG_MACRO, @@ -1483,9 +1483,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&collapsible_if::COLLAPSIBLE_IF), LintId::of(&collapsible_match::COLLAPSIBLE_MATCH), LintId::of(&comparison_chain::COMPARISON_CHAIN), + LintId::of(&copies::BRANCHES_SHARING_CODE), 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), @@ -1872,6 +1872,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&booleans::NONMINIMAL_BOOL), LintId::of(&casts::CHAR_LIT_AS_U8), LintId::of(&casts::UNNECESSARY_CAST), + LintId::of(&copies::BRANCHES_SHARING_CODE), LintId::of(&double_comparison::DOUBLE_COMPARISONS), LintId::of(&double_parens::DOUBLE_PARENS), LintId::of(&duration_subsec::DURATION_SUBSEC), diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index f9576068dd65..7ce90ffd9f0f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -47,7 +47,7 @@ pub mod usage; pub mod visitors; pub use self::attrs::*; -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::collections::hash_map::Entry; use std::hash::BuildHasherDefault; diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs b/tests/ui/branches_sharing_code/shared_at_bottom.rs similarity index 97% rename from tests/ui/shared_code_in_if_blocks/shared_at_bot.rs rename to tests/ui/branches_sharing_code/shared_at_bottom.rs index 6ff362cb7525..c389c243d447 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_bot.rs +++ b/tests/ui/branches_sharing_code/shared_at_bottom.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] -#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] -// This tests the shared_code_in_if_blocks lint at the end of blocks +// This tests the branches_sharing_code lint at the end of blocks fn simple_examples() { let x = 1; diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr b/tests/ui/branches_sharing_code/shared_at_bottom.stderr similarity index 88% rename from tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr rename to tests/ui/branches_sharing_code/shared_at_bottom.stderr index 2268d8f53655..271fcd8b6c12 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_bot.stderr +++ b/tests/ui/branches_sharing_code/shared_at_bottom.stderr @@ -1,5 +1,5 @@ error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:30:5 + --> $DIR/shared_at_bottom.rs:30:5 | LL | / let result = false; LL | | println!("Block end!"); @@ -8,10 +8,10 @@ LL | | }; | |_____^ | note: the lint level is defined here - --> $DIR/shared_at_bot.rs:2:36 + --> $DIR/shared_at_bottom.rs:2:36 | -LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: The end suggestion probably needs some adjustments to use the expression result correctly help: consider moving the end statements out like this | @@ -22,7 +22,7 @@ LL | result; | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:48:5 + --> $DIR/shared_at_bottom.rs:48:5 | LL | / println!("Same end of block"); LL | | } @@ -35,7 +35,7 @@ LL | println!("Same end of block"); | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:65:5 + --> $DIR/shared_at_bottom.rs:65:5 | LL | / println!( LL | | "I'm moveable because I know: `outer_scope_value`: '{}'", @@ -54,7 +54,7 @@ LL | ); | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:77:9 + --> $DIR/shared_at_bottom.rs:77:9 | LL | / println!("Hello World"); LL | | } @@ -67,7 +67,7 @@ LL | println!("Hello World"); | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:93:5 + --> $DIR/shared_at_bottom.rs:93:5 | LL | / let later_used_value = "A string value"; LL | | println!("{}", later_used_value); @@ -84,7 +84,7 @@ LL | println!("{}", later_used_value); | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:106:5 + --> $DIR/shared_at_bottom.rs:106:5 | LL | / let simple_examples = "I now identify as a &str :)"; LL | | println!("This is the new simple_example: {}", simple_examples); @@ -100,7 +100,7 @@ LL | println!("This is the new simple_example: {}", simple_examples); | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:171:5 + --> $DIR/shared_at_bottom.rs:171:5 | LL | / x << 2 LL | | }; @@ -114,7 +114,7 @@ LL | x << 2; | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:178:5 + --> $DIR/shared_at_bottom.rs:178:5 | LL | / x * 4 LL | | } @@ -128,7 +128,7 @@ LL | x * 4 | error: all if blocks contain the same code at the end - --> $DIR/shared_at_bot.rs:190:44 + --> $DIR/shared_at_bottom.rs:190:44 | LL | if x == 17 { b = 1; a = 0x99; } else { a = 0x99; } | ^^^^^^^^^^^ diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.rs b/tests/ui/branches_sharing_code/shared_at_top.rs similarity index 95% rename from tests/ui/shared_code_in_if_blocks/shared_at_top.rs rename to tests/ui/branches_sharing_code/shared_at_top.rs index 496939f2a5e8..e65bcfd78737 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.rs +++ b/tests/ui/branches_sharing_code/shared_at_top.rs @@ -1,7 +1,7 @@ #![allow(dead_code, clippy::eval_order_dependence)] -#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] -// This tests the shared_code_in_if_blocks lint at the start of blocks +// This tests the branches_sharing_code lint at the start of blocks fn simple_examples() { let x = 0; diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr b/tests/ui/branches_sharing_code/shared_at_top.stderr similarity index 95% rename from tests/ui/shared_code_in_if_blocks/shared_at_top.stderr rename to tests/ui/branches_sharing_code/shared_at_top.stderr index 76898a6ff02a..15867e9ea020 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top.stderr +++ b/tests/ui/branches_sharing_code/shared_at_top.stderr @@ -8,8 +8,8 @@ LL | | println!("Hello World!"); note: the lint level is defined here --> $DIR/shared_at_top.rs:2:36 | -LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider moving the start statements out like this | LL | println!("Hello World!"); @@ -106,7 +106,7 @@ 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)] +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: same as this --> $DIR/shared_at_top.rs:98:12 diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs similarity index 94% rename from tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs rename to tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs index 46a8f931aaff..deefdad32c9d 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.rs +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] -#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] -// shared_code_in_if_blocks at the top and bottom of the if blocks +// branches_sharing_code at the top and bottom of the if blocks struct DataPack { id: u32, diff --git a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr similarity index 87% rename from tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr rename to tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr index 75c3d397f2e9..212cfb2f1d18 100644 --- a/tests/ui/shared_code_in_if_blocks/shared_at_top_and_bot.stderr +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr @@ -1,5 +1,5 @@ error: all if blocks contain the same code at the start and the end. Here at the start - --> $DIR/shared_at_top_and_bot.rs:16:5 + --> $DIR/shared_at_top_and_bottom.rs:16:5 | LL | / if x == 7 { LL | | let t = 7; @@ -8,12 +8,12 @@ LL | | let _overlap_end = 2 * t; | |_________________________________^ | note: the lint level is defined here - --> $DIR/shared_at_top_and_bot.rs:2:36 + --> $DIR/shared_at_top_and_bottom.rs:2:36 | -LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: and here at the end - --> $DIR/shared_at_top_and_bot.rs:28:5 + --> $DIR/shared_at_top_and_bottom.rs:28:5 | LL | / let _u = 9; LL | | } @@ -32,7 +32,7 @@ LL | let _u = 9; | error: all if blocks contain the same code at the start and the end. Here at the start - --> $DIR/shared_at_top_and_bot.rs:32:5 + --> $DIR/shared_at_top_and_bottom.rs:32:5 | LL | / if x == 99 { LL | | let r = 7; @@ -41,7 +41,7 @@ LL | | let _overlap_middle = r * r; | |____________________________________^ | note: and here at the end - --> $DIR/shared_at_top_and_bot.rs:43:5 + --> $DIR/shared_at_top_and_bottom.rs:43:5 | LL | / let _overlap_end = r * r * r; LL | | let z = "end"; @@ -63,7 +63,7 @@ LL | let z = "end"; | error: all if blocks contain the same code at the start and the end. Here at the start - --> $DIR/shared_at_top_and_bot.rs:61:5 + --> $DIR/shared_at_top_and_bottom.rs:61:5 | LL | / if (x > 7 && y < 13) || (x + y) % 2 == 1 { LL | | let a = 0xcafe; @@ -72,7 +72,7 @@ LL | | let e_id = gen_id(a, b); | |________________________________^ | note: and here at the end - --> $DIR/shared_at_top_and_bot.rs:81:5 + --> $DIR/shared_at_top_and_bottom.rs:81:5 | LL | / let pack = DataPack { LL | | id: e_id, @@ -101,14 +101,14 @@ LL | }; ... error: all if blocks contain the same code at the start and the end. Here at the start - --> $DIR/shared_at_top_and_bot.rs:94:5 + --> $DIR/shared_at_top_and_bottom.rs:94:5 | LL | / let _ = if x == 7 { LL | | let _ = 19; | |___________________^ | note: and here at the end - --> $DIR/shared_at_top_and_bot.rs:103:5 + --> $DIR/shared_at_top_and_bottom.rs:103:5 | LL | / x << 2 LL | | }; @@ -126,14 +126,14 @@ LL | x << 2; | error: all if blocks contain the same code at the start and the end. Here at the start - --> $DIR/shared_at_top_and_bot.rs:106:5 + --> $DIR/shared_at_top_and_bottom.rs:106:5 | LL | / if x == 9 { LL | | let _ = 17; | |___________________^ | note: and here at the end - --> $DIR/shared_at_top_and_bot.rs:115:5 + --> $DIR/shared_at_top_and_bottom.rs:115:5 | LL | / x * 4 LL | | } diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs b/tests/ui/branches_sharing_code/valid_if_blocks.rs similarity index 96% rename from tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs rename to tests/ui/branches_sharing_code/valid_if_blocks.rs index e63490d5b079..0c70e3748ec1 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.rs +++ b/tests/ui/branches_sharing_code/valid_if_blocks.rs @@ -1,9 +1,9 @@ #![allow(dead_code, clippy::eval_order_dependence)] -#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] -// This tests the shared_code_in_if_blocks lint at the start of blocks +// This tests valid if blocks that shouldn't trigger the lint -// Tests with value references are includes in "shared_code_at_bot.rs" +// Tests with value references are includes in "shared_code_at_bottom.rs" fn valid_examples() { let x = 2; diff --git a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr b/tests/ui/branches_sharing_code/valid_if_blocks.stderr similarity index 96% rename from tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr rename to tests/ui/branches_sharing_code/valid_if_blocks.stderr index 846581456dca..a815995e7172 100644 --- a/tests/ui/shared_code_in_if_blocks/valid_if_blocks.stderr +++ b/tests/ui/branches_sharing_code/valid_if_blocks.stderr @@ -9,7 +9,7 @@ LL | | } else { note: the lint level is defined here --> $DIR/valid_if_blocks.rs:2:9 | -LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: same as this --> $DIR/valid_if_blocks.rs:105:12 diff --git a/tests/ui/checked_unwrap/complex_conditionals.rs b/tests/ui/checked_unwrap/complex_conditionals.rs index bb5b6f5ec043..ec082c73b44c 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.rs +++ b/tests/ui/checked_unwrap/complex_conditionals.rs @@ -1,5 +1,5 @@ #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![allow(clippy::if_same_then_else, clippy::branches_sharing_code)] fn test_complex_conditions() { let x: Result<(), ()> = Ok(()); diff --git a/tests/ui/checked_unwrap/complex_conditionals_nested.rs b/tests/ui/checked_unwrap/complex_conditionals_nested.rs index 99e8fb954665..043ea4148dc5 100644 --- a/tests/ui/checked_unwrap/complex_conditionals_nested.rs +++ b/tests/ui/checked_unwrap/complex_conditionals_nested.rs @@ -1,5 +1,5 @@ #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![allow(clippy::if_same_then_else, clippy::branches_sharing_code)] fn test_nested() { fn nested() { diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index 0459100d88ac..8f23fb28827a 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -1,5 +1,5 @@ #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] +#![allow(clippy::if_same_then_else, clippy::branches_sharing_code)] macro_rules! m { ($a:expr) => { diff --git a/tests/ui/default_numeric_fallback.rs b/tests/ui/default_numeric_fallback.rs index de89f806c585..43468872db0b 100644 --- a/tests/ui/default_numeric_fallback.rs +++ b/tests/ui/default_numeric_fallback.rs @@ -3,7 +3,7 @@ #![allow(clippy::never_loop)] #![allow(clippy::no_effect)] #![allow(clippy::unnecessary_operation)] -#![allow(clippy::shared_code_in_if_blocks)] +#![allow(clippy::branches_sharing_code)] mod basic_expr { fn test() { diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 35a2e139c11d..ef9567455008 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -6,7 +6,7 @@ clippy::no_effect, clippy::unused_unit, clippy::zero_divided_by_zero, - clippy::shared_code_in_if_blocks + clippy::branches_sharing_code )] struct Foo { diff --git a/tests/ui/if_same_then_else2.rs b/tests/ui/if_same_then_else2.rs index 8164b125570d..e4dc5b647dfd 100644 --- a/tests/ui/if_same_then_else2.rs +++ b/tests/ui/if_same_then_else2.rs @@ -6,7 +6,7 @@ clippy::ifs_same_cond, clippy::needless_return, clippy::single_element_loop, - clippy::shared_code_in_if_blocks + clippy::branches_sharing_code )] fn if_same_then_else2() -> Result<&'static str, ()> { diff --git a/tests/ui/let_if_seq.rs b/tests/ui/let_if_seq.rs index 9fd3f875a5f1..2d8f3c2f0e7a 100644 --- a/tests/ui/let_if_seq.rs +++ b/tests/ui/let_if_seq.rs @@ -3,7 +3,7 @@ unused_assignments, clippy::similar_names, clippy::blacklisted_name, - clippy::shared_code_in_if_blocks + clippy::branches_sharing_code )] #![warn(clippy::useless_let_if_seq)] diff --git a/tests/ui/needless_bool/simple.rs b/tests/ui/needless_bool/simple.rs index 678040fa98b3..588bb88f4461 100644 --- a/tests/ui/needless_bool/simple.rs +++ b/tests/ui/needless_bool/simple.rs @@ -5,7 +5,7 @@ clippy::no_effect, clippy::if_same_then_else, clippy::needless_return, - clippy::shared_code_in_if_blocks + clippy::branches_sharing_code )] fn main() { diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index ebf74cfef126..82d95cc041fb 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -4,7 +4,7 @@ #![allow( clippy::if_same_then_else, clippy::single_match, - clippy::shared_code_in_if_blocks, + clippy::branches_sharing_code, clippy::needless_bool )] #![warn(clippy::needless_return)] diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index 2bebccdabca5..8a471f802e11 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -4,7 +4,7 @@ #![allow( clippy::if_same_then_else, clippy::single_match, - clippy::shared_code_in_if_blocks, + clippy::branches_sharing_code, clippy::needless_bool )] #![warn(clippy::needless_return)]