From dfc08610850bc05a3bc31699b592d789a3c71911 Mon Sep 17 00:00:00 2001 From: Alexey Shmalko Date: Wed, 17 Apr 2019 14:01:57 +0300 Subject: [PATCH 1/2] Make assert! ensure the macro is parsed completely --- src/libsyntax_ext/assert.rs | 11 ++++++++-- src/test/ui/macros/assert-trailing-junk.rs | 14 ++++++++++++ .../ui/macros/assert-trailing-junk.stderr | 22 +++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/macros/assert-trailing-junk.rs create mode 100644 src/test/ui/macros/assert-trailing-junk.stderr diff --git a/src/libsyntax_ext/assert.rs b/src/libsyntax_ext/assert.rs index d2c397e0ecc..f1974ed30f6 100644 --- a/src/libsyntax_ext/assert.rs +++ b/src/libsyntax_ext/assert.rs @@ -74,7 +74,7 @@ fn parse_assert<'a>( return Err(err); } - Ok(Assert { + let assert = Assert { cond_expr: parser.parse_expr()?, custom_message: if parser.eat(&token::Comma) { let ts = parser.parse_tokens(); @@ -86,5 +86,12 @@ fn parse_assert<'a>( } else { None }, - }) + }; + + if parser.token != token::Eof { + parser.expect_one_of(&[], &[])?; + unreachable!(); + } + + Ok(assert) } diff --git a/src/test/ui/macros/assert-trailing-junk.rs b/src/test/ui/macros/assert-trailing-junk.rs new file mode 100644 index 00000000000..66de6de3ee8 --- /dev/null +++ b/src/test/ui/macros/assert-trailing-junk.rs @@ -0,0 +1,14 @@ +// 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 +} diff --git a/src/test/ui/macros/assert-trailing-junk.stderr b/src/test/ui/macros/assert-trailing-junk.stderr new file mode 100644 index 00000000000..3f7d8dcaf79 --- /dev/null +++ b/src/test/ui/macros/assert-trailing-junk.stderr @@ -0,0 +1,22 @@ +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 + +error: aborting due to 3 previous errors + From f29e9a5cb83ef6dca14652b323e2c00c36997a54 Mon Sep 17 00:00:00 2001 From: Alexey Shmalko Date: Thu, 25 Apr 2019 01:44:28 +0300 Subject: [PATCH 2/2] 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 { + 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