fix `collapsable_else_if` when the inner `if` is in parens

6 files changed, 102 insertions, 27 deletions

diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
index da0f0b2f1be..e3103e2d301 100644
--- a/clippy_lints/src/collapsible_if.rs
+++ b/clippy_lints/src/collapsible_if.rs
@@ -136,6 +136,9 @@ impl CollapsibleIf {
                         return;
                     }
 
+                    // Peel off any parentheses.
+                    let (_, else_block_span, _) = peel_parens(cx.tcx.sess.source_map(), else_.span);
+
                     // Prevent "elseif"
                     // Check that the "else" is followed by whitespace
                     let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() {
@@ -152,7 +155,7 @@ impl CollapsibleIf {
                             if requires_space { " " } else { "" },
                             snippet_block_with_applicability(
                                 cx,
-                                else_.span,
+                                else_block_span,
                                 "..",
                                 Some(else_block.span),
                                 &mut applicability
@@ -298,39 +301,36 @@ fn span_extract_keyword(sm: &SourceMap, span: Span, keyword: &str) -> Option<Spa
         .next()
 }
 
+/// Peel the parentheses from an `if` expression, e.g. `((if true {} else {}))`.
 fn peel_parens(sm: &SourceMap, mut span: Span) -> (Span, Span, Span) {
     use crate::rustc_span::Pos;
-    use rustc_span::SpanData;
 
     let start = span.shrink_to_lo();
     let end = span.shrink_to_hi();
 
-    loop {
-        let data = span.data();
-        let snippet = sm.span_to_snippet(span).unwrap();
-
-        let trim_start = snippet.len() - snippet.trim_start().len();
-        let trim_end = snippet.len() - snippet.trim_end().len();
-
-        let trimmed = snippet.trim();
-
-        if trimmed.starts_with('(') && trimmed.ends_with(')') {
-            // Try to remove one layer of parens by adjusting the span
-            span = SpanData {
-                lo: data.lo + BytePos::from_usize(trim_start + 1),
-                hi: data.hi - BytePos::from_usize(trim_end + 1),
-                ctxt: data.ctxt,
-                parent: data.parent,
-            }
-            .span();
+    let snippet = sm.span_to_snippet(span).unwrap();
+    if let Some((trim_start, _, trim_end)) = peel_parens_str(&snippet) {
+        let mut data = span.data();
+        data.lo = data.lo + BytePos::from_usize(trim_start);
+        data.hi = data.hi - BytePos::from_usize(trim_end);
+        span = data.span();
+    }
 
-            continue;
-        }
+    (start.with_hi(span.lo()), span, end.with_lo(span.hi()))
+}
 
-        break;
+fn peel_parens_str(snippet: &str) -> Option<(usize, &str, usize)> {
+    let trimmed = snippet.trim();
+    if !(trimmed.starts_with('(') && trimmed.ends_with(')')) {
+        return None;
     }
 
-    (start.with_hi(span.lo()),
-    span,
-    end.with_lo(span.hi()))
+    let trim_start = (snippet.len() - snippet.trim_start().len()) + 1;
+    let trim_end = (snippet.len() - snippet.trim_end().len()) + 1;
+
+    let inner = snippet.get(trim_start..snippet.len() - trim_end)?;
+    Some(match peel_parens_str(inner) {
+        None => (trim_start, inner, trim_end),
+        Some((start, inner, end)) => (trim_start + start, inner, trim_end + end),
+    })
 }
diff --git a/tests/ui/collapsible_else_if.fixed b/tests/ui/collapsible_else_if.fixed
index fed75244c6f..3d709fe9b8e 100644
--- a/tests/ui/collapsible_else_if.fixed
+++ b/tests/ui/collapsible_else_if.fixed
@@ -104,3 +104,25 @@ fn issue14799() {
         }
     }
 }
+
+fn in_parens() {
+    let x = "hello";
+    let y = "world";
+
+    if x == "hello" {
+        print!("Hello ");
+    } else if y == "world" { println!("world") } else { println!("!") }
+    //~^^^ collapsible_else_if
+}
+
+fn in_brackets() {
+    let x = "hello";
+    let y = "world";
+
+    // There is no lint when the inner `if` is in a block.
+    if x == "hello" {
+        print!("Hello ");
+    } else {
+        { if y == "world" { println!("world") } else { println!("!") } }
+    }
+}
diff --git a/tests/ui/collapsible_else_if.rs b/tests/ui/collapsible_else_if.rs
index e50e781fb69..51868e03908 100644
--- a/tests/ui/collapsible_else_if.rs
+++ b/tests/ui/collapsible_else_if.rs
@@ -120,3 +120,27 @@ fn issue14799() {
         }
     }
 }
+
+fn in_parens() {
+    let x = "hello";
+    let y = "world";
+
+    if x == "hello" {
+        print!("Hello ");
+    } else {
+        (if y == "world" { println!("world") } else { println!("!") })
+    }
+    //~^^^ collapsible_else_if
+}
+
+fn in_brackets() {
+    let x = "hello";
+    let y = "world";
+
+    // There is no lint when the inner `if` is in a block.
+    if x == "hello" {
+        print!("Hello ");
+    } else {
+        { if y == "world" { println!("world") } else { println!("!") } }
+    }
+}
diff --git a/tests/ui/collapsible_else_if.stderr b/tests/ui/collapsible_else_if.stderr
index 7d80894cadb..1a7bcec7fd5 100644
--- a/tests/ui/collapsible_else_if.stderr
+++ b/tests/ui/collapsible_else_if.stderr
@@ -150,5 +150,14 @@ LL | |         if false {}
 LL | |     }
    | |_____^ help: collapse nested if block: `if false {}`
 
-error: aborting due to 8 previous errors
+error: this `else { if .. }` block can be collapsed
+  --> tests/ui/collapsible_else_if.rs:130:12
+   |
+LL |       } else {
+   |  ____________^
+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
 
diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed
index f7c840c4cc1..78354c2d7cf 100644
--- a/tests/ui/collapsible_if.fixed
+++ b/tests/ui/collapsible_if.fixed
@@ -171,3 +171,13 @@ fn in_parens() {
         }
     //~^^^^^ collapsible_if
 }
+
+fn in_brackets() {
+    if true {
+        {
+            if true {
+                println!("In brackets, not linted");
+            }
+        }
+    }
+}
diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs
index 4faebcb0ca0..5d9afa10956 100644
--- a/tests/ui/collapsible_if.rs
+++ b/tests/ui/collapsible_if.rs
@@ -182,3 +182,13 @@ fn in_parens() {
     }
     //~^^^^^ collapsible_if
 }
+
+fn in_brackets() {
+    if true {
+        {
+            if true {
+                println!("In brackets, not linted");
+            }
+        }
+    }
+}