diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 0efcb3c77ff6..be07ce1272bd 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -267,6 +267,9 @@ impl LateLintPass<'_> for CollapsibleIf { && !expr.span.from_expansion() { if let Some(else_) = else_ + // Short circuit if both `if` branches contain only a single `if {..} else {}`, as + // collapsing such blocks can lead to less readable code (#4971) + && !(single_inner_if_else(then) && single_inner_if_else(else_)) && let ExprKind::Block(else_, None) = else_.kind { self.check_collapsible_else_if(cx, then.span, else_); @@ -280,6 +283,19 @@ impl LateLintPass<'_> for CollapsibleIf { } } +/// Returns true if `expr` is a block that contains only one `if {..} else {}` statement +fn single_inner_if_else(expr: &Expr<'_>) -> bool { + if let ExprKind::Block(block, None) = expr.kind + && let Some(inner_expr) = expr_block(block) + && let ExprKind::If(_, _, else_) = inner_expr.kind + && else_.is_some() + { + true + } else { + false + } +} + /// If `block` is a block with either one expression or a statement containing an expression, /// return the expression. We don't peel blocks recursively, as extra blocks might be intentional. fn expr_block<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> { diff --git a/tests/ui/collapsible_else_if.fixed b/tests/ui/collapsible_else_if.fixed index d85bbf189075..cd2d9be9f433 100644 --- a/tests/ui/collapsible_else_if.fixed +++ b/tests/ui/collapsible_else_if.fixed @@ -70,6 +70,17 @@ fn main() { } //~^^^^^^^^ collapsible_else_if + if x == "hello" { + if y == "world" { + print!("Hello "); + } else { + println!("world"); + } + } else if let Some(42) = Some(42) { + println!("42"); + } + //~^^^^^ collapsible_else_if + if x == "hello" { print!("Hello "); } else { @@ -78,6 +89,21 @@ fn main() { println!("world!") } } + + if x == "hello" { + if y == "world" { + print!("Hello "); + } else { + println!("world"); + } + } else { + if let Some(42) = Some(42) { + println!("42"); + } else { + println!("!"); + } + } + } #[rustfmt::skip] diff --git a/tests/ui/collapsible_else_if.rs b/tests/ui/collapsible_else_if.rs index 10aef2792adb..75f204328538 100644 --- a/tests/ui/collapsible_else_if.rs +++ b/tests/ui/collapsible_else_if.rs @@ -84,6 +84,19 @@ fn main() { } //~^^^^^^^^ collapsible_else_if + if x == "hello" { + if y == "world" { + print!("Hello "); + } else { + println!("world"); + } + } else { + if let Some(42) = Some(42) { + println!("42"); + } + } + //~^^^^^ collapsible_else_if + if x == "hello" { print!("Hello "); } else { @@ -92,6 +105,21 @@ fn main() { println!("world!") } } + + if x == "hello" { + if y == "world" { + print!("Hello "); + } else { + println!("world"); + } + } else { + if let Some(42) = Some(42) { + println!("42"); + } else { + println!("!"); + } + } + } #[rustfmt::skip] diff --git a/tests/ui/collapsible_else_if.stderr b/tests/ui/collapsible_else_if.stderr index 98109099bae6..ebd78d2b1ffe 100644 --- a/tests/ui/collapsible_else_if.stderr +++ b/tests/ui/collapsible_else_if.stderr @@ -142,7 +142,25 @@ LL + } | error: this `else { if .. }` block can be collapsed - --> tests/ui/collapsible_else_if.rs:100:10 + --> tests/ui/collapsible_else_if.rs:93:12 + | +LL | } else { + | ____________^ +LL | | if let Some(42) = Some(42) { +LL | | println!("42"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ } else if let Some(42) = Some(42) { +LL + println!("42"); +LL + } + | + +error: this `else { if .. }` block can be collapsed + --> tests/ui/collapsible_else_if.rs:128:10 | LL | }else{ | __________^ @@ -151,7 +169,7 @@ LL | | } | |_____^ help: collapse nested if block: `if false {}` error: this `else { if .. }` block can be collapsed - --> tests/ui/collapsible_else_if.rs:139:12 + --> tests/ui/collapsible_else_if.rs:167:12 | LL | } else { | ____________^ @@ -159,5 +177,5 @@ LL | | (if y == "world" { println!("world") } else { println!("!") }) LL | | } | |_____^ help: collapse nested if block: `if y == "world" { println!("world") } else { println!("!") }` -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors