Teach suspicious_else_formatting about if .. {..} {..}

Author: HMPerson1

clippy_lints/src/formatting.rs

@@ -31,8 +31,8 @@ declare_clippy_lint! {
     "suspicious formatting of `*=`, `-=` or `!=`"
 }
 
-/// **What it does:** Checks for formatting of `else if`. It lints if the `else`
-/// and `if` are not on the same line or the `else` seems to be missing.
+/// **What it does:** Checks for formatting of `else`. It lints if the `else`
+/// is followed immediately by a newline or the `else` seems to be missing.
 ///
 /// **Why is this bad?** This is probably some refactoring remnant, even if the
 /// code is correct, it might look confusing.
@@ -42,19 +42,29 @@ declare_clippy_lint! {
 /// **Example:**
 /// ```rust,ignore
 /// if foo {
+/// } { // looks like an `else` is missing here
+/// }
+///
+/// if foo {
 /// } if bar { // looks like an `else` is missing here
 /// }
 ///
 /// if foo {
 /// } else
 ///
+/// { // this is the `else` block of the previous `if`, but should it be?
+/// }
+///
+/// if foo {
+/// } else
+///
 /// if bar { // this is the `else` block of the previous `if`, but should it be?
 /// }
 /// ```
 declare_clippy_lint! {
     pub SUSPICIOUS_ELSE_FORMATTING,
     style,
-    "suspicious formatting of `else if`"
+    "suspicious formatting of `else`"
 }
 
 /// **What it does:** Checks for possible missing comma in an array. It lints if
@@ -96,7 +106,7 @@ impl EarlyLintPass for Formatting {
             match (&w[0].node, &w[1].node) {
                 (&ast::StmtKind::Expr(ref first), &ast::StmtKind::Expr(ref second))
                 | (&ast::StmtKind::Expr(ref first), &ast::StmtKind::Semi(ref second)) => {
-                    check_consecutive_ifs(cx, first, second);
+                    check_missing_else(cx, first, second);
                 },
                 _ => (),
             }
@@ -105,7 +115,7 @@ impl EarlyLintPass for Formatting {
 
     fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
         check_assign(cx, expr);
-        check_else_if(cx, expr);
+        check_else(cx, expr);
         check_array(cx, expr);
     }
 }
@@ -139,10 +149,13 @@ fn check_assign(cx: &EarlyContext<'_>, expr: &ast::Expr) {
     }
 }
 
-/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for weird `else if`.
-fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) {
+/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for weird `else`.
+fn check_else(cx: &EarlyContext<'_>, expr: &ast::Expr) {
     if let Some((then, &Some(ref else_))) = unsugar_if(expr) {
-        if unsugar_if(else_).is_some() && !differing_macro_contexts(then.span, else_.span) && !in_macro(then.span) {
+        if (is_block(else_) || unsugar_if(else_).is_some())
+            && !differing_macro_contexts(then.span, else_.span)
+            && !in_macro(then.span)
+        {
             // this will be a span from the closing ‘}’ of the “then” block (excluding) to
             // the
             // “if” of the “else if” block (excluding)
@@ -154,14 +167,19 @@ fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) {
                 let else_pos = else_snippet.find("else").expect("there must be a `else` here");
 
                 if else_snippet[else_pos..].contains('\n') {
+                    let else_desc = if unsugar_if(else_).is_some() { "if" } else { "{..}" };
+
                     span_note_and_lint(
                         cx,
                         SUSPICIOUS_ELSE_FORMATTING,
                         else_span,
-                        "this is an `else if` but the formatting might hide it",
+                        &format!("this is an `else {}` but the formatting might hide it", else_desc),
                         else_span,
-                        "to remove this lint, remove the `else` or remove the new line between `else` \
-                         and `if`",
+                        &format!(
+                            "to remove this lint, remove the `else` or remove the new line between \
+                             `else` and `{}`",
+                            else_desc,
+                        ),
                     );
                 }
             }
@@ -200,32 +218,47 @@ fn check_array(cx: &EarlyContext<'_>, expr: &ast::Expr) {
     }
 }
 
-/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for consecutive ifs.
-fn check_consecutive_ifs(cx: &EarlyContext<'_>, first: &ast::Expr, second: &ast::Expr) {
+fn check_missing_else(cx: &EarlyContext<'_>, first: &ast::Expr, second: &ast::Expr) {
     if !differing_macro_contexts(first.span, second.span)
         && !in_macro(first.span)
         && unsugar_if(first).is_some()
-        && unsugar_if(second).is_some()
+        && (is_block(second) || unsugar_if(second).is_some())
     {
         // where the else would be
         let else_span = first.span.between(second.span);
 
         if let Some(else_snippet) = snippet_opt(cx, else_span) {
             if !else_snippet.contains('\n') {
+                let (looks_like, next_thing) = if unsugar_if(second).is_some() {
+                    ("an `else if`", "the second `if`")
+                } else {
+                    ("an `else {..}`", "the next block")
+                };
+
                 span_note_and_lint(
                     cx,
                     SUSPICIOUS_ELSE_FORMATTING,
                     else_span,
-                    "this looks like an `else if` but the `else` is missing",
+                    &format!("this looks like {} but the `else` is missing", looks_like),
                     else_span,
-                    "to remove this lint, add the missing `else` or add a new line before the second \
-                     `if`",
+                    &format!(
+                        "to remove this lint, add the missing `else` or add a new line before {}",
+                        next_thing,
+                    ),
                 );
             }
         }
     }
 }
 
+fn is_block(expr: &ast::Expr) -> bool {
+    if let ast::ExprKind::Block(..) = expr.node {
+        true
+    } else {
+        false
+    }
+}
+
 /// Match `if` or `if let` expressions and return the `then` and `else` block.
 fn unsugar_if(expr: &ast::Expr) -> Option<(&P<ast::Block>, &Option<P<ast::Expr>>)> {
     match expr.node {

tests/ui/formatting.rs

@@ -16,7 +16,11 @@
 fn foo() -> bool { true }
 
 fn main() {
-    // weird `else if` formatting:
+    // weird `else` formatting:
+    if foo() {
+    } {
+    }
+
     if foo() {
     } if foo() {
     }
@@ -41,6 +45,17 @@ fn main() {
         let _ = 0;
     };
 
+    if foo() {
+    } else
+    {
+    }
+
+    if foo() {
+    }
+    else
+    {
+    }
+
     if foo() {
     } else
     if foo() { // the span of the above error should continue here
@@ -53,6 +68,20 @@ fn main() {
     }
 
     // those are ok:
+    if foo() {
+    }
+    {
+    }
+
+    if foo() {
+    } else {
+    }
+
+    if foo() {
+    }
+    else {
+    }
+
     if foo() {
     }
     if foo() {

tests/ui/formatting.stderr

@@ -1,90 +1,119 @@
-error: this looks like an `else if` but the `else` is missing
+error: this looks like an `else {..}` but the `else` is missing
   --> $DIR/formatting.rs:21:6
    |
-21 |     } if foo() {
+21 |     } {
    |      ^
    |
    = note: `-D clippy::suspicious-else-formatting` implied by `-D warnings`
+   = note: to remove this lint, add the missing `else` or add a new line before the next block
+
+error: this looks like an `else if` but the `else` is missing
+  --> $DIR/formatting.rs:25:6
+   |
+25 |     } if foo() {
+   |      ^
+   |
    = note: to remove this lint, add the missing `else` or add a new line before the second `if`
 
 error: this looks like an `else if` but the `else` is missing
-  --> $DIR/formatting.rs:28:10
+  --> $DIR/formatting.rs:32:10
    |
-28 |         } if foo() {
+32 |         } if foo() {
    |          ^
    |
    = note: to remove this lint, add the missing `else` or add a new line before the second `if`
 
 error: this looks like an `else if` but the `else` is missing
-  --> $DIR/formatting.rs:36:10
+  --> $DIR/formatting.rs:40:10
    |
-36 |         } if foo() {
+40 |         } if foo() {
    |          ^
    |
    = note: to remove this lint, add the missing `else` or add a new line before the second `if`
 
+error: this is an `else {..}` but the formatting might hide it
+  --> $DIR/formatting.rs:49:6
+   |
+49 |       } else
+   |  ______^
+50 | |     {
+   | |____^
+   |
+   = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}`
+
+error: this is an `else {..}` but the formatting might hide it
+  --> $DIR/formatting.rs:54:6
+   |
+54 |       }
+   |  ______^
+55 | |     else
+56 | |     {
+   | |____^
+   |
+   = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}`
+
 error: this is an `else if` but the formatting might hide it
-  --> $DIR/formatting.rs:45:6
+  --> $DIR/formatting.rs:60:6
    |
-45 |       } else
+60 |       } else
    |  ______^
-46 | |     if foo() { // the span of the above error should continue here
+61 | |     if foo() { // the span of the above error should continue here
    | |____^
    |
    = note: to remove this lint, remove the `else` or remove the new line between `else` and `if`
 
 error: this is an `else if` but the formatting might hide it
-  --> $DIR/formatting.rs:50:6
+  --> $DIR/formatting.rs:65:6
    |
-50 |       }
+65 |       }
    |  ______^
-51 | |     else
-52 | |     if foo() { // the span of the above error should continue here
+66 | |     else
+67 | |     if foo() { // the span of the above error should continue here
    | |____^
    |
    = note: to remove this lint, remove the `else` or remove the new line between `else` and `if`
 
 error: this looks like you are trying to use `.. -= ..`, but you really are doing `.. = (- ..)`
-  --> $DIR/formatting.rs:77:6
-   |
-77 |     a =- 35;
-   |      ^^^^
-   |
-   = note: `-D clippy::suspicious-assignment-formatting` implied by `-D warnings`
-   = note: to remove this lint, use either `-=` or `= -`
+   --> $DIR/formatting.rs:106:6
+    |
+106 |     a =- 35;
+    |      ^^^^
+    |
+    = note: `-D clippy::suspicious-assignment-formatting` implied by `-D warnings`
+    = note: to remove this lint, use either `-=` or `= -`
 
 error: this looks like you are trying to use `.. *= ..`, but you really are doing `.. = (* ..)`
-  --> $DIR/formatting.rs:78:6
-   |
-78 |     a =* &191;
-   |      ^^^^
-   |
-   = note: to remove this lint, use either `*=` or `= *`
+   --> $DIR/formatting.rs:107:6
+    |
+107 |     a =* &191;
+    |      ^^^^
+    |
+    = note: to remove this lint, use either `*=` or `= *`
 
 error: this looks like you are trying to use `.. != ..`, but you really are doing `.. = (! ..)`
-  --> $DIR/formatting.rs:81:6
-   |
-81 |     b =! false;
-   |      ^^^^
-   |
-   = note: to remove this lint, use either `!=` or `= !`
+   --> $DIR/formatting.rs:110:6
+    |
+110 |     b =! false;
+    |      ^^^^
+    |
+    = note: to remove this lint, use either `!=` or `= !`
 
 error: possibly missing a comma here
-  --> $DIR/formatting.rs:90:19
-   |
-90 |         -1, -2, -3 // <= no comma here
-   |                   ^
-   |
-   = note: `-D clippy::possible-missing-comma` implied by `-D warnings`
-   = note: to remove this lint, add a comma or write the expr in a single line
+   --> $DIR/formatting.rs:119:19
+    |
+119 |         -1, -2, -3 // <= no comma here
+    |                   ^
+    |
+    = note: `-D clippy::possible-missing-comma` implied by `-D warnings`
+    = note: to remove this lint, add a comma or write the expr in a single line
 
 error: possibly missing a comma here
-  --> $DIR/formatting.rs:94:19
-   |
-94 |         -1, -2, -3 // <= no comma here
-   |                   ^
-   |
-   = note: to remove this lint, add a comma or write the expr in a single line
+   --> $DIR/formatting.rs:123:19
+    |
+123 |         -1, -2, -3 // <= no comma here
+    |                   ^
+    |
+    = note: to remove this lint, add a comma or write the expr in a single line
 
-error: aborting due to 10 previous errors
+error: aborting due to 13 previous errors