From 5efd6bc6c3781f28f762fdf781d49e3db7fcc9d3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 23 Sep 2021 00:22:27 -0400 Subject: [PATCH 1/2] Don't lint `suspicious_else_formatting` inside proc-macros --- clippy_lints/src/formatting.rs | 57 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs index 4dd0ffe77ea4..b4f186525c56 100644 --- a/clippy_lints/src/formatting.rs +++ b/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 - let else_span = first.span.between(second.span); + 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); - 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") - }; + // 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"); - 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, - ), - ); - } + // 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") + }; + + 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, + ), + ); } } } From e69154f3705af1f69d61469d581f4c20ac5efd9d Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 23 Sep 2021 11:17:54 -0400 Subject: [PATCH 2/2] Add test for #7650 --- .../proc_macro_suspicious_else_formatting.rs | 75 +++++++++++++++++++ tests/ui/suspicious_else_formatting.rs | 9 +++ tests/ui/suspicious_else_formatting.stderr | 18 ++--- 3 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 tests/ui/auxiliary/proc_macro_suspicious_else_formatting.rs diff --git a/tests/ui/auxiliary/proc_macro_suspicious_else_formatting.rs b/tests/ui/auxiliary/proc_macro_suspicious_else_formatting.rs new file mode 100644 index 000000000000..26c88489b03c --- /dev/null +++ b/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); + + >::from_iter( + [ + Ident::new("impl", Span::call_site()).into(), + ident.into(), + Group::new( + Delimiter::Brace, + >::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, + >::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(), + ) +} diff --git a/tests/ui/suspicious_else_formatting.rs b/tests/ui/suspicious_else_formatting.rs index 547615b10d9f..be8bc22bf98a 100644 --- a/tests/ui/suspicious_else_formatting.rs +++ b/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); diff --git a/tests/ui/suspicious_else_formatting.stderr b/tests/ui/suspicious_else_formatting.stderr index d8d67b4138ab..d1db195cbb87 100644 --- a/tests/ui/suspicious_else_formatting.stderr +++ b/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 | } | ______^