Auto merge of #7707 - Jarcho:suspicious_else_proc_mac, r=Manishearth

Don't lint `suspicious_else_formatting` inside proc-macros

fixes: #7650

I'll add a test for this one soon.

changelog: Don't lint `suspicious_else_formatting` inside proc-macros

clippy_lints/src/formatting.rs

@@ -286,34 +286,39 @@ fn check_array(cx: &EarlyContext<'_>, expr: &Expr) {
 }
 
 fn check_missing_else(cx: &EarlyContext<'_>, first: &Expr, second: &Expr) {
-    if !differing_macro_contexts(first.span, second.span)
-        && !first.span.from_expansion()
-        && is_if(first)
-        && (is_block(second) || is_if(second))
-    {
-        // where the else would be
+    if_chain! {
+        if !differing_macro_contexts(first.span, second.span);
+        if !first.span.from_expansion();
+        if let ExprKind::If(cond_expr, ..) = &first.kind;
+        if is_block(second) || is_if(second);
+
+        // Proc-macros can give weird spans. Make sure this is actually an `if`.
+        if let Some(if_snip) = snippet_opt(cx, first.span.until(cond_expr.span));
+        if if_snip.starts_with("if");
+
+        // If there is a line break between the two expressions, don't lint.
+        // If there is a non-whitespace character, this span came from a proc-macro.
         let else_span = first.span.between(second.span);
+        if let Some(else_snippet) = snippet_opt(cx, else_span);
+        if !else_snippet.chars().any(|c| c == '\n' || !c.is_whitespace());
+        then {
+            let (looks_like, next_thing) = if is_if(second) {
+                ("an `else if`", "the second `if`")
+            } else {
+                ("an `else {..}`", "the next block")
+            };
 
-        if let Some(else_snippet) = snippet_opt(cx, else_span) {
-            if !else_snippet.contains('\n') {
-                let (looks_like, next_thing) = if is_if(second) {
-                    ("an `else if`", "the second `if`")
-                } else {
-                    ("an `else {..}`", "the next block")
-                };
-
-                span_lint_and_note(
-                    cx,
-                    SUSPICIOUS_ELSE_FORMATTING,
-                    else_span,
-                    &format!("this looks like {} but the `else` is missing", looks_like),
-                    None,
-                    &format!(
-                        "to remove this lint, add the missing `else` or add a new line before {}",
-                        next_thing,
-                    ),
-                );
-            }
+            span_lint_and_note(
+                cx,
+                SUSPICIOUS_ELSE_FORMATTING,
+                else_span,
+                &format!("this looks like {} but the `else` is missing", looks_like),
+                None,
+                &format!(
+                    "to remove this lint, add the missing `else` or add a new line before {}",
+                    next_thing,
+                ),
+            );
         }
     }
 }

tests/ui/auxiliary/proc_macro_suspicious_else_formatting.rs

@@ -0,0 +1,75 @@
+// compile-flags: --emit=link
+// no-prefer-dynamic
+
+#![crate_type = "proc-macro"]
+
+extern crate proc_macro;
+use proc_macro::{token_stream, Delimiter, Group, Ident, Span, TokenStream, TokenTree};
+use std::iter::FromIterator;
+
+fn read_ident(iter: &mut token_stream::IntoIter) -> Ident {
+    match iter.next() {
+        Some(TokenTree::Ident(i)) => i,
+        _ => panic!("expected ident"),
+    }
+}
+
+#[proc_macro_derive(DeriveBadSpan)]
+pub fn derive_bad_span(input: TokenStream) -> TokenStream {
+    let mut input = input.into_iter();
+    assert_eq!(read_ident(&mut input).to_string(), "struct");
+    let ident = read_ident(&mut input);
+    let mut tys = match input.next() {
+        Some(TokenTree::Group(g)) if g.delimiter() == Delimiter::Parenthesis => g.stream().into_iter(),
+        _ => panic!(),
+    };
+    let field1 = read_ident(&mut tys);
+    tys.next();
+    let field2 = read_ident(&mut tys);
+
+    <TokenStream as FromIterator<TokenTree>>::from_iter(
+        [
+            Ident::new("impl", Span::call_site()).into(),
+            ident.into(),
+            Group::new(
+                Delimiter::Brace,
+                <TokenStream as FromIterator<TokenTree>>::from_iter(
+                    [
+                        Ident::new("fn", Span::call_site()).into(),
+                        Ident::new("_foo", Span::call_site()).into(),
+                        Group::new(Delimiter::Parenthesis, TokenStream::new()).into(),
+                        Group::new(
+                            Delimiter::Brace,
+                            <TokenStream as FromIterator<TokenTree>>::from_iter(
+                                [
+                                    Ident::new("if", field1.span()).into(),
+                                    Ident::new("true", field1.span()).into(),
+                                    {
+                                        let mut group = Group::new(Delimiter::Brace, TokenStream::new());
+                                        group.set_span(field1.span());
+                                        group.into()
+                                    },
+                                    Ident::new("if", field2.span()).into(),
+                                    Ident::new("true", field2.span()).into(),
+                                    {
+                                        let mut group = Group::new(Delimiter::Brace, TokenStream::new());
+                                        group.set_span(field2.span());
+                                        group.into()
+                                    },
+                                ]
+                                .iter()
+                                .cloned(),
+                            ),
+                        )
+                        .into(),
+                    ]
+                    .iter()
+                    .cloned(),
+                ),
+            )
+            .into(),
+        ]
+        .iter()
+        .cloned(),
+    )
+}

tests/ui/suspicious_else_formatting.rs

@@ -1,5 +1,10 @@
+// aux-build:proc_macro_suspicious_else_formatting.rs
+
 #![warn(clippy::suspicious_else_formatting)]
 
+extern crate proc_macro_suspicious_else_formatting;
+use proc_macro_suspicious_else_formatting::DeriveBadSpan;
+
 fn foo() -> bool {
     true
 }
@@ -103,3 +108,7 @@ fn main() {
     {
     }
 }
+
+// #7650 - Don't lint. Proc-macro using bad spans for `if` expressions.
+#[derive(DeriveBadSpan)]
+struct _Foo(u32, u32);

tests/ui/suspicious_else_formatting.stderr

@@ -1,5 +1,5 @@
 error: this looks like an `else {..}` but the `else` is missing
-  --> $DIR/suspicious_else_formatting.rs:11:6
+  --> $DIR/suspicious_else_formatting.rs:16:6
    |
 LL |     } {
    |      ^
@@ -8,7 +8,7 @@ LL |     } {
    = 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/suspicious_else_formatting.rs:15:6
+  --> $DIR/suspicious_else_formatting.rs:20:6
    |
 LL |     } if foo() {
    |      ^
@@ -16,7 +16,7 @@ LL |     } 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/suspicious_else_formatting.rs:22:10
+  --> $DIR/suspicious_else_formatting.rs:27:10
    |
 LL |         } if foo() {
    |          ^
@@ -24,7 +24,7 @@ LL |         } 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/suspicious_else_formatting.rs:30:10
+  --> $DIR/suspicious_else_formatting.rs:35:10
    |
 LL |         } if foo() {
    |          ^
@@ -32,7 +32,7 @@ LL |         } 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/suspicious_else_formatting.rs:39:6
+  --> $DIR/suspicious_else_formatting.rs:44:6
    |
 LL |       } else
    |  ______^
@@ -42,7 +42,7 @@ LL | |     {
    = 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/suspicious_else_formatting.rs:51:6
+  --> $DIR/suspicious_else_formatting.rs:56:6
    |
 LL |       } else
    |  ______^
@@ -52,7 +52,7 @@ LL | |     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/suspicious_else_formatting.rs:56:6
+  --> $DIR/suspicious_else_formatting.rs:61:6
    |
 LL |       }
    |  ______^
@@ -63,7 +63,7 @@ LL | |     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 {..}` but the formatting might hide it
-  --> $DIR/suspicious_else_formatting.rs:83:6
+  --> $DIR/suspicious_else_formatting.rs:88:6
    |
 LL |       }
    |  ______^
@@ -75,7 +75,7 @@ LL | |     {
    = 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/suspicious_else_formatting.rs:91:6
+  --> $DIR/suspicious_else_formatting.rs:96:6
    |
 LL |       }
    |  ______^