Auto merge of #60039 - rasendubi:assert-trailing-junk, r=alexcrichton

Make assert! ensure the macro is parsed completely

Fixes #60024

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,17 +75,69 @@ fn parse_assert<'a>(
         return Err(err);
     }
 
-    Ok(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 {
+        parser.expect_one_of(&[], &[])?;
+        unreachable!();
+    }
+
+    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
+    }
 }

src/test/ui/macros/assert-trailing-junk.rs

@@ -0,0 +1,24 @@
+// Ensure assert macro does not ignore trailing garbage.
+//
+// See https://github.com/rust-lang/rust/issues/60024 for details.
+
+fn main() {
+    assert!(true some extra junk, "whatever");
+    //~^ ERROR expected one of
+
+    assert!(true some extra junk);
+    //~^ ERROR expected one of
+
+    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
+}

src/test/ui/macros/assert-trailing-junk.stderr

@@ -0,0 +1,60 @@
+error: expected one of `,`, `.`, `?`, or an operator, found `some`
+  --> $DIR/assert-trailing-junk.rs:6:18
+   |
+LL |     assert!(true some extra junk, "whatever");
+   |                  ^^^^ expected one of `,`, `.`, `?`, or an operator here
+
+error: expected one of `,`, `.`, `?`, or an operator, found `some`
+  --> $DIR/assert-trailing-junk.rs:9:18
+   |
+LL |     assert!(true some extra junk);
+   |                  ^^^^ expected one of `,`, `.`, `?`, or an operator here
+
+error: no rules expected the token `blah`
+  --> $DIR/assert-trailing-junk.rs:12:30
+   |
+LL |     assert!(true, "whatever" blah);
+   |                             -^^^^ no rules expected this token in macro call
+   |                             |
+   |                             help: missing comma here
+
+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
+