Fix suggestion for collapsible_if and collapsible_else_if when the inner if is enclosed in parentheses (rust-lang#15304)

changelog: [`collapsible_else_if`]: fix suggestion when inner `if` as
wrapped in parentheses
changelog: [`collapsible_if`]: fix suggestion when inner `if` as wrapped
in parentheses

fixes rust-lang#15303

I'm sure this is a bit dirty, but don't currently see a better way.

Author: llogiq

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
@@ -187,7 +190,8 @@ impl CollapsibleIf {
                             .with_leading_whitespace(cx)
                             .into_span()
                     };
-                    let inner_if = inner.span.split_at(2).0;
+                    let (paren_start, inner_if_span, paren_end) = peel_parens(cx.tcx.sess.source_map(), inner.span);
+                    let inner_if = inner_if_span.split_at(2).0;
                     let mut sugg = vec![
                         // Remove the outer then block `{`
                         (then_open_bracket, String::new()),
@@ -196,6 +200,17 @@ impl CollapsibleIf {
                         // Replace inner `if` by `&&`
                         (inner_if, String::from("&&")),
                     ];
+
+                    if !paren_start.is_empty() {
+                        // Remove any leading parentheses '('
+                        sugg.push((paren_start, String::new()));
+                    }
+
+                    if !paren_end.is_empty() {
+                        // Remove any trailing parentheses ')'
+                        sugg.push((paren_end, String::new()));
+                    }
+
                     sugg.extend(parens_around(check));
                     sugg.extend(parens_around(check_inner));
 
@@ -285,3 +300,37 @@ 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;
+
+    let start = span.shrink_to_lo();
+    let end = span.shrink_to_hi();
+
+    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();
+    }
+
+    (start.with_hi(span.lo()), span, end.with_lo(span.hi()))
+}
+
+fn peel_parens_str(snippet: &str) -> Option<(usize, &str, usize)> {
+    let trimmed = snippet.trim();
+    if !(trimmed.starts_with('(') && trimmed.ends_with(')')) {
+        return None;
+    }
+
+    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),
+    })
+}

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!("!") } }
+    }
+}

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!("!") } }
+    }
+}

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
 

tests/ui/collapsible_if.fixed

@@ -163,3 +163,21 @@ fn issue14799() {
         if true {}
     };
 }
+
+fn in_parens() {
+    if true
+        && true {
+            println!("In parens, linted");
+        }
+    //~^^^^^ collapsible_if
+}
+
+fn in_brackets() {
+    if true {
+        {
+            if true {
+                println!("In brackets, not linted");
+            }
+        }
+    }
+}

tests/ui/collapsible_if.rs

@@ -173,3 +173,22 @@ fn issue14799() {
         if true {}
     };
 }
+
+fn in_parens() {
+    if true {
+        (if true {
+            println!("In parens, linted");
+        })
+    }
+    //~^^^^^ collapsible_if
+}
+
+fn in_brackets() {
+    if true {
+        {
+            if true {
+                println!("In brackets, not linted");
+            }
+        }
+    }
+}

tests/ui/collapsible_if.stderr

@@ -190,5 +190,23 @@ LL |         // This is a comment, do not collapse code to it
 LL ~     ; 3
    |
 
-error: aborting due to 11 previous errors
+error: this `if` statement can be collapsed
+  --> tests/ui/collapsible_if.rs:178:5
+   |
+LL | /     if true {
+LL | |         (if true {
+LL | |             println!("In parens, linted");
+LL | |         })
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     if true
+LL ~         && true {
+LL |             println!("In parens, linted");
+LL ~         }
+   |
+
+error: aborting due to 12 previous errors