1
Fork 0

Auto merge of #8297 - Jarcho:if_same_then_else_7579, r=Manishearth

Don't lint `if_same_then_else` with `if let` conditions

fixes #7579

changelog: Don't lint `if_same_then_else` with `if let` conditions
This commit is contained in:
bors 2022-01-17 06:48:01 +00:00
commit 537a7f3e44
2 changed files with 34 additions and 12 deletions

View file

@ -183,7 +183,7 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
lint_same_cond(cx, &conds); lint_same_cond(cx, &conds);
lint_same_fns_in_if_cond(cx, &conds); lint_same_fns_in_if_cond(cx, &conds);
// Block duplication // 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. /// Implementation of `BRANCHES_SHARING_CODE` and `IF_SAME_THEN_ELSE` if the blocks are equal.
fn lint_same_then_else<'tcx>( fn lint_same_then_else<'tcx>(
cx: &LateContext<'tcx>, cx: &LateContext<'tcx>,
conds: &[&'tcx Expr<'_>],
blocks: &[&Block<'tcx>], blocks: &[&Block<'tcx>],
has_conditional_else: bool, has_conditional_else: bool,
expr: &'tcx Expr<'_>, expr: &'tcx Expr<'_>,
@ -204,7 +205,7 @@ fn lint_same_then_else<'tcx>(
// Check if each block has shared code // Check if each block has shared code
let has_expr = blocks[0].expr.is_some(); 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) (block_eq.start_eq, block_eq.end_eq, block_eq.expr_eq)
} else { } else {
return; 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 /// 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. /// 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 start_eq = usize::MAX;
let mut end_eq = usize::MAX; let mut end_eq = usize::MAX;
let mut expr_eq = true; let mut expr_eq = true;
let mut iter = blocks.windows(2); let mut iter = blocks.windows(2).enumerate();
while let Some(&[win0, win1]) = iter.next() { while let Some((i, &[block0, block1])) = iter.next() {
let l_stmts = win0.stmts; let l_stmts = block0.stmts;
let r_stmts = win1.stmts; let r_stmts = block1.stmts;
// `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. // `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. // 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) it1.zip(it2)
.fold(0, |acc, (l, r)| if evaluator.eq_stmt(l, r) { acc + 1 } else { 0 }) .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_SAME_THEN_ELSE
if_chain! { if_chain! {
if block_expr_eq; if block_expr_eq;
if l_stmts.len() == r_stmts.len(); if l_stmts.len() == r_stmts.len();
if l_stmts.len() == current_start_eq; if l_stmts.len() == current_start_eq;
if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win0.hir_id); // `conds` may have one last item than `blocks`.
if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win1.hir_id); // 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 { then {
span_lint_and_note( span_lint_and_note(
cx, cx,
IF_SAME_THEN_ELSE, IF_SAME_THEN_ELSE,
win0.span, block0.span,
"this `if` has identical blocks", "this `if` has identical blocks",
Some(win1.span), Some(block1.span),
"same as this", "same as this",
); );

View file

@ -138,6 +138,23 @@ fn if_same_then_else2() -> Result<&'static str, ()> {
let (y, x) = (1, 2); let (y, x) = (1, 2);
return Ok(&foo[x..y]); 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() {} fn main() {}