From 5461ed670eecedad74c82dd91ba532249d2073fb Mon Sep 17 00:00:00 2001 From: Jason Newcomb <jsnewcomb@pm.me> Date: Sun, 16 Jan 2022 09:46:02 -0500 Subject: [PATCH] Don't lint `if_same_then_else` with `if let` conditions --- clippy_lints/src/copies.rs | 29 +++++++++++++++++------------ tests/ui/if_same_then_else2.rs | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 73ce656ad15..f44d370a8fd 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -183,7 +183,7 @@ 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, &conds, &blocks, conds.len() == blocks.len(), expr); } } } @@ -192,6 +192,7 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { /// Implementation of `BRANCHES_SHARING_CODE` and `IF_SAME_THEN_ELSE` if the blocks are equal. fn lint_same_then_else<'tcx>( cx: &LateContext<'tcx>, + conds: &[&'tcx Expr<'_>], blocks: &[&Block<'tcx>], has_conditional_else: bool, expr: &'tcx Expr<'_>, @@ -204,7 +205,7 @@ fn lint_same_then_else<'tcx>( // Check if each block has shared code let has_expr = blocks[0].expr.is_some(); - let (start_eq, mut end_eq, expr_eq) = if let Some(block_eq) = scan_block_for_eq(cx, blocks) { + let (start_eq, mut end_eq, expr_eq) = if let Some(block_eq) = scan_block_for_eq(cx, conds, blocks) { (block_eq.start_eq, block_eq.end_eq, block_eq.expr_eq) } else { return; @@ -316,14 +317,14 @@ struct BlockEqual { /// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `None` to /// abort any further processing and avoid duplicate lint triggers. -fn scan_block_for_eq(cx: &LateContext<'_>, blocks: &[&Block<'_>]) -> Option<BlockEqual> { +fn scan_block_for_eq(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[&Block<'_>]) -> Option<BlockEqual> { let mut start_eq = usize::MAX; let mut end_eq = usize::MAX; let mut expr_eq = true; - let mut iter = blocks.windows(2); - while let Some(&[win0, win1]) = iter.next() { - let l_stmts = win0.stmts; - let r_stmts = win1.stmts; + let mut iter = blocks.windows(2).enumerate(); + while let Some((i, &[block0, block1])) = iter.next() { + let l_stmts = block0.stmts; + let r_stmts = block1.stmts; // `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. @@ -340,22 +341,26 @@ fn scan_block_for_eq(cx: &LateContext<'_>, blocks: &[&Block<'_>]) -> Option<Bloc it1.zip(it2) .fold(0, |acc, (l, r)| if evaluator.eq_stmt(l, r) { acc + 1 } else { 0 }) }; - let block_expr_eq = both(&win0.expr, &win1.expr, |l, r| evaluator.eq_expr(l, r)); + let block_expr_eq = both(&block0.expr, &block1.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 !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win0.hir_id); - if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win1.hir_id); + // `conds` may have one last item than `blocks`. + // Any `i` from `blocks.windows(2)` will exist in `conds`, but `i+1` may not exist on the last iteration. + if !matches!(conds[i].kind, ExprKind::Let(..)); + if !matches!(conds.get(i + 1).map(|e| &e.kind), Some(ExprKind::Let(..))); + if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, block0.hir_id); + if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, block1.hir_id); then { span_lint_and_note( cx, IF_SAME_THEN_ELSE, - win0.span, + block0.span, "this `if` has identical blocks", - Some(win1.span), + Some(block1.span), "same as this", ); diff --git a/tests/ui/if_same_then_else2.rs b/tests/ui/if_same_then_else2.rs index 69189d9e0c0..0016009a02f 100644 --- a/tests/ui/if_same_then_else2.rs +++ b/tests/ui/if_same_then_else2.rs @@ -138,6 +138,23 @@ fn if_same_then_else2() -> Result<&'static str, ()> { let (y, x) = (1, 2); return Ok(&foo[x..y]); } + + // Issue #7579 + let _ = if let Some(0) = None { 0 } else { 0 }; + + if true { + return Err(()); + } else if let Some(0) = None { + return Err(()); + } + + let _ = if let Some(0) = None { + 0 + } else if let Some(1) = None { + 0 + } else { + 0 + }; } fn main() {}