diff --git a/CHANGELOG.md b/CHANGELOG.md index fb4a0b500f22..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 diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 46093a16571b..1b982221b065 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,9 +1,19 @@ -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 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; +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, symbol::Symbol, BytePos}; +use std::borrow::Cow; declare_clippy_lint! { /// **What it does:** Checks for consecutive `if`s with the same condition. @@ -104,47 +114,457 @@ 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 BRANCHES_SHARING_CODE, + complexity, + "`if` statement with shared code in all blocks" +} + +declare_lint_pass!(CopyAndPaste => [ + IFS_SAME_COND, + SAME_FUNCTIONS_IN_IF_CONDITION, + IF_SAME_THEN_ELSE, + BRANCHES_SHARING_CODE +]); 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); - lint_same_then_else(cx, &blocks); - lint_same_cond(cx, &conds); - lint_same_fns_in_if_cond(cx, &conds); +/// 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_conditional_else: bool, + expr: &'tcx Expr<'_>, +) { + // We only lint ifs with multiple blocks + if blocks.len() < 2 || parent_node_is_if_expr(expr, cx) { + return; + } + + // Check if each block has shared code + let has_expr = blocks[0].expr.is_some(); + let (start_eq, mut end_eq, expr_eq) = scan_block_for_eq(cx, blocks); + + // BRANCHES_SHARING_CODE prerequisites + if has_conditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) { + return; + } + + // Only the start is the same + if start_eq != 0 && 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_branches_sharing_code_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 (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 block_walker = UsedValueFinderVisitor::new(cx); + for stmt in block_stmts { + intravisit::walk_stmt(&mut block_walker, stmt); + } + let mut block_defs = block_walker.defs; + + // Scan moved stmts + let mut moved_start: Option = None; + 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 &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); + end_walker.defs.drain().for_each(|x| { + block_defs.insert(x); + }); + + end_walker.def_symbols.clear(); + } + } + + end_walker.uses.clear(); + } + + if let Some(moved_start) = moved_start { + end_eq -= moved_start; + } + + 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| { + moved_syms.insert(x); + }); + } + + emit_branches_sharing_code_lint( + cx, + start_eq, + end_eq, + end_linable, + check_for_warn_of_moved_symbol(cx, &moved_syms, expr), + blocks, + 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) }; +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; - if let Some((i, j)) = search_same_sequenced(blocks, eq) { - span_lint_and_note( + // `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. + // 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(); + + 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 = { + // 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 + 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 { + 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 + .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 + }) + }) +} + +fn emit_branches_sharing_code_lint( + cx: &LateContext<'tcx>, + start_stmts: usize, + end_stmts: usize, + lint_end: bool, + warn_about_moved_symbol: 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![]; + 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(); + 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", 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 = 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); + 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 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())); + 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_then(cx, BRANCHES_SHARING_CODE, 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(); + span_lint_and_then( cx, - IF_SAME_THEN_ELSE, - j.span, - "this `if` has identical blocks", - Some(i.span), - "same as this", + BRANCHES_SHARING_CODE, + start_span, + "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_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, + ); + + add_optional_msgs(diag); + }, ); } } +/// This visitor collects `HirId`s and Symbols of defined symbols and `HirId`s 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> UsedValueFinderVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'tcx>) -> Self { + UsedValueFinderVisitor { + cx, + defs: FxHashSet::default(), + def_symbols: FxHashSet::default(), + uses: FxHashSet::default(), + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for UsedValueFinderVisitor<'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(sym) = l.pat.simple_ident() { + self.def_symbols.insert(sym.name); + } + + 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 +618,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..43e89c2475f3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -613,6 +613,7 @@ 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, @@ -1482,6 +1483,7 @@ 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(&default::FIELD_REASSIGN_WITH_DEFAULT), @@ -1870,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_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/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, diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 0c3b8b89171d..571eec9e530f 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,12 +93,14 @@ 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) + // 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) @@ -159,7 +162,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; } @@ -483,6 +486,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/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index d6364625e704..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; @@ -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/branches_sharing_code/shared_at_bottom.rs b/tests/ui/branches_sharing_code/shared_at_bottom.rs new file mode 100644 index 000000000000..c389c243d447 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_bottom.rs @@ -0,0 +1,209 @@ +#![allow(dead_code)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + +// This tests the branches_sharing_code lint at the end of blocks + +fn simple_examples() { + 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 + }; + + // Else if block + if x == 9 { + println!("The index is: 6"); + + println!("Same end of block"); + } else if x == 8 { + println!("The index is: 4"); + + // We should only get a lint trigger for the last statement + println!("This is also eq with the else block"); + println!("Same end of block"); + } else { + println!("This is also eq with the else block"); + println!("Same end of 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 + ); + } + + if x == 9 { + if x == 8 { + // No parent!! + println!("---"); + println!("Hello World"); + } else { + println!("Hello World"); + } + } +} + +/// Simple examples where the move can cause some problems due to moved values +fn simple_but_suggestion_is_invalid() { + 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() { + 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 + }; +} + +/// 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; + 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 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/branches_sharing_code/shared_at_bottom.stderr b/tests/ui/branches_sharing_code/shared_at_bottom.stderr new file mode 100644 index 000000000000..271fcd8b6c12 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_bottom.stderr @@ -0,0 +1,143 @@ +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bottom.rs:30:5 + | +LL | / let result = false; +LL | | println!("Block end!"); +LL | | result +LL | | }; + | |_____^ + | +note: the lint level is defined here + --> $DIR/shared_at_bottom.rs:2:36 + | +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 + | +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_bottom.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_bottom.rs:65: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 end + --> $DIR/shared_at_bottom.rs:77:9 + | +LL | / println!("Hello World"); +LL | | } + | |_________^ + | +help: consider moving the end statements out like this + | +LL | } +LL | println!("Hello World"); + | + +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bottom.rs:93: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_bottom.rs:106: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_bottom.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 + | +LL | } +LL | x << 2; + | + +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bottom.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 + | +LL | } +LL | x * 4 + | + +error: all if blocks contain the same code at the end + --> $DIR/shared_at_bottom.rs:190: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 9 previous errors + diff --git a/tests/ui/branches_sharing_code/shared_at_top.rs b/tests/ui/branches_sharing_code/shared_at_top.rs new file mode 100644 index 000000000000..e65bcfd78737 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_top.rs @@ -0,0 +1,103 @@ +#![allow(dead_code, clippy::eval_order_dependence)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + +// This tests the branches_sharing_code 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; + } +} + +/// 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/branches_sharing_code/shared_at_top.stderr b/tests/ui/branches_sharing_code/shared_at_top.stderr new file mode 100644 index 000000000000..15867e9ea020 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_top.stderr @@ -0,0 +1,121 @@ +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::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +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: 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::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +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 + diff --git a/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs new file mode 100644 index 000000000000..deefdad32c9d --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs @@ -0,0 +1,119 @@ +#![allow(dead_code)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + +// branches_sharing_code at the top and bottom of the if blocks + +struct DataPack { + id: u32, + name: String, + some_data: Vec, +} + +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); + } +} + +/// 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/branches_sharing_code/shared_at_top_and_bottom.stderr b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr new file mode 100644 index 000000000000..212cfb2f1d18 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.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_bottom.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_bottom.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: and here at the end + --> $DIR/shared_at_top_and_bottom.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_bottom.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_bottom.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_bottom.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_bottom.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_bottom.rs:94:5 + | +LL | / let _ = if x == 7 { +LL | | let _ = 19; + | |___________________^ + | +note: and here at the end + --> $DIR/shared_at_top_and_bottom.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_bottom.rs:106:5 + | +LL | / if x == 9 { +LL | | let _ = 17; + | |___________________^ + | +note: and here at the end + --> $DIR/shared_at_top_and_bottom.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/branches_sharing_code/valid_if_blocks.rs b/tests/ui/branches_sharing_code/valid_if_blocks.rs new file mode 100644 index 000000000000..0c70e3748ec1 --- /dev/null +++ b/tests/ui/branches_sharing_code/valid_if_blocks.rs @@ -0,0 +1,155 @@ +#![allow(dead_code, clippy::eval_order_dependence)] +#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + +// This tests valid if blocks that shouldn't trigger the lint + +// Tests with value references are includes in "shared_code_at_bottom.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 + } + + let x = 1; + if true { + println!("{}", x); + } else { + 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 +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"; + } + + // 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" } + }; + + 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() {} diff --git a/tests/ui/branches_sharing_code/valid_if_blocks.stderr b/tests/ui/branches_sharing_code/valid_if_blocks.stderr new file mode 100644 index 000000000000..a815995e7172 --- /dev/null +++ b/tests/ui/branches_sharing_code/valid_if_blocks.stderr @@ -0,0 +1,101 @@ +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:104:14 + | +LL | if false { + | ______________^ +LL | | } else { + | |_____^ + | +note: the lint level is defined here + --> $DIR/valid_if_blocks.rs:2:9 + | +LL | #![deny(clippy::if_same_then_else, clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +note: same as this + --> $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 { + | ____________^ +LL | | let u = 19; +LL | | println!("How are u today?"); +LL | | let _ = "This is a string"; +LL | | } + | |_____^ + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:126:23 + | +LL | let _ = if x == 6 { 7 } else { 7 }; + | ^^^^^ + | +note: same as this + --> $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:132:23 + | +LL | } else if x == 68 { + | _______________________^ +LL | | println!("I'm a doppelgänger"); +LL | | // Don't listen to my clone below +LL | | +LL | | if y == 90 { "=^.^=" } else { ":D" } +LL | | } else { + | |_____^ + | +note: same as this + --> $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 | | if y == 90 { "=^.^=" } else { ":D" } +LL | | }; + | |_____^ + +error: this `if` has identical blocks + --> $DIR/valid_if_blocks.rs:146: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:149: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 + diff --git a/tests/ui/checked_unwrap/complex_conditionals.rs b/tests/ui/checked_unwrap/complex_conditionals.rs index c986c992a07a..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)] +#![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 2307996a48ff..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)] +#![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 369676308348..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)] +#![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 0b3758952ac6..43468872db0b 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::branches_sharing_code)] 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.rs b/tests/ui/if_same_then_else.rs index 9c5fe02f7519..ef9567455008 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::branches_sharing_code )] 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..e4dc5b647dfd 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::branches_sharing_code )] 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..6524be0af851 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 { | _____________^ @@ -24,95 +10,99 @@ 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:22:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | if let Some(a) = Some(42) {} +LL | | for _ in &[42] { +LL | | let bar: &Option<_> = &Some::(42); +... | +LL | | } LL | | } | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:33:13 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:34:13 | 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:36: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 - --> $DIR/if_same_then_else2.rs:40:13 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:41:13 | 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:43: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 - --> $DIR/if_same_then_else2.rs:90:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:91:21 | 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:93:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | Ok("foo")?; -LL | | } +LL | | f32::NAN +LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:97:13 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:98:13 | 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:100: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 - --> $DIR/if_same_then_else2.rs:121:20 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:122:20 | LL | } else if true { | ____________________^ @@ -120,6 +110,16 @@ 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/let_if_seq.rs b/tests/ui/let_if_seq.rs index 32a67f181df4..2d8f3c2f0e7a 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::branches_sharing_code )] #![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/needless_bool/simple.rs b/tests/ui/needless_bool/simple.rs index e9f1428fc3a4..588bb88f4461 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::branches_sharing_code )] 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..82d95cc041fb 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::branches_sharing_code, + 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..8a471f802e11 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::branches_sharing_code, + 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`