Add a lint about suspiciously formatted else if

README.md

@@ -113,6 +113,7 @@ name
 [string_lit_as_bytes](https://github.com/Manishearth/rust-clippy/wiki#string_lit_as_bytes)                           | warn    | calling `as_bytes` on a string literal; suggests using a byte string literal instead
 [string_to_string](https://github.com/Manishearth/rust-clippy/wiki#string_to_string)                                 | warn    | calling `String::to_string` which is inefficient
 [suspicious_assignment_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_assignment_formatting) | warn    | suspicious formatting of `*=`, `-=` or `!=`
+[suspicious_else_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_else_formatting)             | warn    | suspicious formatting of `else if`
 [temporary_assignment](https://github.com/Manishearth/rust-clippy/wiki#temporary_assignment)                         | warn    | assignments to temporaries
 [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg)                                 | warn    | An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take references with `&`.
 [trivial_regex](https://github.com/Manishearth/rust-clippy/wiki#trivial_regex)                                       | warn    | finds trivial regular expressions in `Regex::new(_)` invocations

src/formatting.rs

@@ -6,7 +6,7 @@ use syntax::ptr::P;
 
 /// **What it does:** This lint looks for use of the non-existent `=*`, `=!` and `=-` operators.
 ///
-/// **Why is this bad?** This either a typo of `*=`, `!=` or `-=` or confusing.
+/// **Why is this bad?** This is either a typo of `*=`, `!=` or `-=` or confusing.
 ///
 /// **Known problems:** None.
 ///
@@ -20,18 +20,65 @@ declare_lint! {
     "suspicious formatting of `*=`, `-=` or `!=`"
 }
 
+/// **What it does:** This lint 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.
+///
+/// **Why is this bad?** This is probably some refactoring remnant, even if the code is correct, it
+/// might look confusing.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust,ignore
+/// if foo {
+/// } if bar { // looks like an `else` is missing here
+/// }
+///
+/// if foo {
+/// } else
+///
+/// if bar { // this is the `else` block of the previous `if`, but should it be?
+/// }
+/// ```
+declare_lint! {
+    pub SUSPICIOUS_ELSE_FORMATTING,
+    Warn,
+    "suspicious formatting of `else if`"
+}
+
 #[derive(Copy,Clone)]
 pub struct Formatting;
 
 impl LintPass for Formatting {
     fn get_lints(&self) -> LintArray {
-        lint_array![SUSPICIOUS_ASSIGNMENT_FORMATTING]
+        lint_array![SUSPICIOUS_ASSIGNMENT_FORMATTING, SUSPICIOUS_ELSE_FORMATTING]
     }
 }
 
 impl EarlyLintPass for Formatting {
+    fn check_block(&mut self, cx: &EarlyContext, block: &ast::Block) {
+        for w in block.stmts.windows(2) {
+            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);
+                }
+                _ => (),
+            }
+        }
+
+        if let Some(ref expr) = block.expr {
+            if let Some(ref stmt) = block.stmts.iter().last() {
+                if let ast::StmtKind::Expr(ref first, _) = stmt.node {
+                    check_consecutive_ifs(cx, first, expr);
+                }
+            }
+        }
+    }
+
     fn check_expr(&mut self, cx: &EarlyContext, expr: &ast::Expr) {
         check_assign(cx, expr);
+        check_else_if(cx, expr);
     }
 }
 
@@ -64,3 +111,62 @@ fn check_unop(expr: &ast::Expr) -> Option<(&P<ast::Expr>, &'static str)> {
         _ => None,
     }
 }
+
+/// Implementation of the SUSPICIOUS_ELSE_FORMATTING lint for weird `else if`.
+fn check_else_if(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(cx, then.span) {
+            // this will be a span from the closing ‘}’ of the “then” block (excluding) to the
+            // “if” of the “else if” block (excluding)
+            let else_span = mk_sp(then.span.hi, else_.span.lo);
+
+            // the snippet should look like " else \n    " with maybe comments anywhere
+            // it’s bad when there is a ‘\n’ after the “else”
+            if let Some(else_snippet) = snippet_opt(cx, else_span) {
+                let else_pos = else_snippet.find("else").expect("there must be a `else` here");
+
+                if else_snippet[else_pos..].contains('\n') {
+                    span_note_and_lint(cx,
+                                       SUSPICIOUS_ELSE_FORMATTING,
+                                       else_span,
+                                       "this is an `else if` but the formatting might hide it",
+                                       else_span,
+                                       "to remove this lint, remove the `else` or remove the new line between `else` and `if`");
+                }
+            }
+        }
+    }
+}
+
+/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for consecutive ifs.
+fn check_consecutive_ifs(cx: &EarlyContext, first: &ast::Expr, second: &ast::Expr) {
+    if !differing_macro_contexts(first.span, second.span) &&
+    !in_macro(cx, first.span) &&
+    unsugar_if(first).is_some() &&
+    unsugar_if(second).is_some() {
+        // where the else would be
+        let else_span = mk_sp(first.span.hi, second.span.lo);
+
+        if let Some(else_snippet) = snippet_opt(cx, else_span) {
+            if !else_snippet.contains('\n') {
+                span_note_and_lint(cx,
+                                   SUSPICIOUS_ELSE_FORMATTING,
+                                   else_span,
+                                   "this looks like an `else if` but the `else` is missing",
+                                   else_span,
+                                   "to remove this lint, add the missing `else` or add a new line before the second `if`");
+            }
+        }
+    }
+}
+
+/// Match `if` or `else if` 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 {
+        ast::ExprKind::If(_, ref then, ref else_) |
+        ast::ExprKind::IfLet(_, _, ref then, ref else_) => Some((then, else_)),
+        _ => None,
+    }
+}

src/lib.rs

@@ -215,6 +215,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         eta_reduction::REDUNDANT_CLOSURE,
         format::USELESS_FORMAT,
         formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
+        formatting::SUSPICIOUS_ELSE_FORMATTING,
         identity_op::IDENTITY_OP,
         items_after_statements::ITEMS_AFTER_STATEMENTS,
         len_zero::LEN_WITHOUT_IS_EMPTY,

tests/compile-fail/formatting.rs

@@ -6,7 +6,55 @@
 #![allow(unused_assignments)]
 #![allow(if_same_then_else)]
 
+fn foo() -> bool { true }
+
 fn main() {
+    // weird `else if` formatting:
+    if foo() {
+    } if foo() { //~ERROR this looks like an `else if` but the `else` is missing
+    }
+
+    let _ = {
+        if foo() {
+        } if foo() { //~ERROR this looks like an `else if` but the `else` is missing
+        }
+        else {
+        }
+    };
+
+    if foo() {
+    } else //~ERROR this is an `else if` but the formatting might hide it
+    if foo() { // the span of the above error should continue here
+    }
+
+    if foo() {
+    } //~ERROR this is an `else if` but the formatting might hide it
+    else
+    if foo() { // the span of the above error should continue here
+    }
+
+    // those are ok:
+    if foo() {
+    }
+    if foo() {
+    }
+
+    if foo() {
+    } else if foo() {
+    }
+
+    if foo() {
+    }
+    else if foo() {
+    }
+
+    if foo() {
+    }
+
+    else if
+
+    foo() {}
+
     // weird op_eq formatting:
     let mut a = 42;
     a =- 35;