From f29e9a5cb83ef6dca14652b323e2c00c36997a54 Mon Sep 17 00:00:00 2001
From: Alexey Shmalko <rasen.dubi@gmail.com>
Date: Thu, 25 Apr 2019 01:44:28 +0300
Subject: [PATCH] Handle common assert! misuses

---
 src/libsyntax_ext/assert.rs                   | 74 +++++++++++++++----
 src/test/ui/macros/assert-trailing-junk.rs    | 10 +++
 .../ui/macros/assert-trailing-junk.stderr     | 40 +++++++++-
 3 files changed, 109 insertions(+), 15 deletions(-)

diff --git a/src/libsyntax_ext/assert.rs b/src/libsyntax_ext/assert.rs
index f1974ed30f6..cd69733571d 100644
--- a/src/libsyntax_ext/assert.rs
+++ b/src/libsyntax_ext/assert.rs
@@ -1,10 +1,11 @@
-use errors::DiagnosticBuilder;
+use errors::{Applicability, DiagnosticBuilder};
 
 use syntax::ast::{self, *};
 use syntax::source_map::Spanned;
 use syntax::ext::base::*;
 use syntax::ext::build::AstBuilder;
 use syntax::parse::token;
+use syntax::parse::parser::Parser;
 use syntax::print::pprust;
 use syntax::ptr::P;
 use syntax::symbol::Symbol;
@@ -74,18 +75,54 @@ fn parse_assert<'a>(
         return Err(err);
     }
 
-    let assert = Assert {
-        cond_expr: parser.parse_expr()?,
-        custom_message: if parser.eat(&token::Comma) {
-            let ts = parser.parse_tokens();
-            if !ts.is_empty() {
-                Some(ts)
-            } else {
-                None
-            }
-        } else {
-            None
-        },
+    let cond_expr = parser.parse_expr()?;
+
+    // Some crates use the `assert!` macro in the following form (note extra semicolon):
+    //
+    // assert!(
+    //     my_function();
+    // );
+    //
+    // Warn about semicolon and suggest removing it. Eventually, this should be turned into an
+    // error.
+    if parser.token == token::Semi {
+        let mut err = cx.struct_span_warn(sp, "macro requires an expression as an argument");
+        err.span_suggestion(
+            parser.span,
+            "try removing semicolon",
+            String::new(),
+            Applicability::MaybeIncorrect
+        );
+        err.note("this is going to be an error in the future");
+        err.emit();
+
+        parser.bump();
+    }
+
+    // Some crates use the `assert!` macro in the following form (note missing comma before
+    // message):
+    //
+    // assert!(true "error message");
+    //
+    // Parse this as an actual message, and suggest inserting a comma. Eventually, this should be
+    // turned into an error.
+    let custom_message = if let token::Literal(token::Lit::Str_(_), _) = parser.token {
+        let mut err = cx.struct_span_warn(parser.span, "unexpected string literal");
+        let comma_span = cx.source_map().next_point(parser.prev_span);
+        err.span_suggestion_short(
+            comma_span,
+            "try adding a comma",
+            ", ".to_string(),
+            Applicability::MaybeIncorrect
+        );
+        err.note("this is going to be an error in the future");
+        err.emit();
+
+        parse_custom_message(&mut parser)
+    } else if parser.eat(&token::Comma) {
+        parse_custom_message(&mut parser)
+    } else {
+        None
     };
 
     if parser.token != token::Eof {
@@ -93,5 +130,14 @@ fn parse_assert<'a>(
         unreachable!();
     }
 
-    Ok(assert)
+    Ok(Assert { cond_expr, custom_message })
+}
+
+fn parse_custom_message<'a>(parser: &mut Parser<'a>) -> Option<TokenStream> {
+    let ts = parser.parse_tokens();
+    if !ts.is_empty() {
+        Some(ts)
+    } else {
+        None
+    }
 }
diff --git a/src/test/ui/macros/assert-trailing-junk.rs b/src/test/ui/macros/assert-trailing-junk.rs
index 66de6de3ee8..676ae05bf0f 100644
--- a/src/test/ui/macros/assert-trailing-junk.rs
+++ b/src/test/ui/macros/assert-trailing-junk.rs
@@ -11,4 +11,14 @@ fn main() {
 
     assert!(true, "whatever" blah);
     //~^ ERROR no rules expected
+
+    assert!(true "whatever" blah);
+    //~^ WARN unexpected string literal
+    //~^^ ERROR no rules expected
+
+    assert!(true;);
+    //~^ WARN macro requires an expression
+
+    assert!(false || true "error message");
+    //~^ WARN unexpected string literal
 }
diff --git a/src/test/ui/macros/assert-trailing-junk.stderr b/src/test/ui/macros/assert-trailing-junk.stderr
index 3f7d8dcaf79..6fc0a278461 100644
--- a/src/test/ui/macros/assert-trailing-junk.stderr
+++ b/src/test/ui/macros/assert-trailing-junk.stderr
@@ -18,5 +18,43 @@ LL |     assert!(true, "whatever" blah);
    |                             |
    |                             help: missing comma here
 
-error: aborting due to 3 previous errors
+warning: unexpected string literal
+  --> $DIR/assert-trailing-junk.rs:15:18
+   |
+LL |     assert!(true "whatever" blah);
+   |                 -^^^^^^^^^^
+   |                 |
+   |                 help: try adding a comma
+   |
+   = note: this is going to be an error in the future
+
+error: no rules expected the token `blah`
+  --> $DIR/assert-trailing-junk.rs:15:29
+   |
+LL |     assert!(true "whatever" blah);
+   |                            -^^^^ no rules expected this token in macro call
+   |                            |
+   |                            help: missing comma here
+
+warning: macro requires an expression as an argument
+  --> $DIR/assert-trailing-junk.rs:19:5
+   |
+LL |     assert!(true;);
+   |     ^^^^^^^^^^^^-^^
+   |                 |
+   |                 help: try removing semicolon
+   |
+   = note: this is going to be an error in the future
+
+warning: unexpected string literal
+  --> $DIR/assert-trailing-junk.rs:22:27
+   |
+LL |     assert!(false || true "error message");
+   |                          -^^^^^^^^^^^^^^^
+   |                          |
+   |                          help: try adding a comma
+   |
+   = note: this is going to be an error in the future
+
+error: aborting due to 4 previous errors
 
-- 
2.44.0