From 1b41c9a42e3d5231df8621ae4b900cddecc12a75 Mon Sep 17 00:00:00 2001 From: mark Date: Mon, 14 Jan 2019 19:14:02 -0600 Subject: [PATCH 1/5] error on duplicate matcher bindings --- src/libsyntax/ext/tt/macro_rules.rs | 54 ++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/ext/tt/macro_rules.rs b/src/libsyntax/ext/tt/macro_rules.rs index b3ecaeaedbb7a..61a5b2cb0d200 100644 --- a/src/libsyntax/ext/tt/macro_rules.rs +++ b/src/libsyntax/ext/tt/macro_rules.rs @@ -17,10 +17,10 @@ use crate::parse::token::Token::*; use crate::symbol::Symbol; use crate::tokenstream::{DelimSpan, TokenStream, TokenTree}; -use syntax_pos::{Span, DUMMY_SP}; +use syntax_pos::{Span, DUMMY_SP, symbol::Ident}; use log::debug; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap}; use std::borrow::Cow; use std::collections::hash_map::Entry; @@ -246,8 +246,12 @@ fn generic_extension<'cx>(cx: &'cx mut ExtCtxt<'_>, // Holy self-referential! /// Converts a `macro_rules!` invocation into a syntax extension. -pub fn compile(sess: &ParseSess, features: &Features, def: &ast::Item, edition: Edition) - -> SyntaxExtension { +pub fn compile( + sess: &ParseSess, + features: &Features, + def: &ast::Item, + edition: Edition +) -> SyntaxExtension { let lhs_nm = ast::Ident::with_empty_ctxt(Symbol::gensym("lhs")); let rhs_nm = ast::Ident::with_empty_ctxt(Symbol::gensym("rhs")); @@ -355,7 +359,9 @@ pub fn compile(sess: &ParseSess, features: &Features, def: &ast::Item, edition: // don't abort iteration early, so that errors for multiple lhses can be reported for lhs in &lhses { - valid &= check_lhs_no_empty_seq(sess, &[lhs.clone()]) + valid &= check_lhs_no_empty_seq(sess, &[lhs.clone()]); + valid &= + check_lhs_duplicate_matcher_bindings(sess, &[lhs.clone()], &mut FxHashMap::default()); } let expander: Box<_> = Box::new(MacroRulesMacroExpander { @@ -456,6 +462,44 @@ fn check_lhs_no_empty_seq(sess: &ParseSess, tts: &[quoted::TokenTree]) -> bool { true } +/// Check that the LHS contains no duplicate matcher bindings. e.g. `$a:expr, $a:expr` would be +/// illegal, since it would be ambiguous which `$a` to use if we ever needed to. +fn check_lhs_duplicate_matcher_bindings( + sess: &ParseSess, + tts: &[quoted::TokenTree], + metavar_names: &mut FxHashMap +) -> bool { + use self::quoted::TokenTree; + for tt in tts { + match *tt { + TokenTree::MetaVarDecl(span, name, _kind) => { + if let Some(&prev_span) = metavar_names.get(&name) { + sess.span_diagnostic + .struct_span_err(span, "duplicate matcher binding") + .span_note(prev_span, "previous declaration was here") + .emit(); + return false; + } else { + metavar_names.insert(name, span); + } + } + TokenTree::Delimited(_, ref del) => { + if !check_lhs_duplicate_matcher_bindings(sess, &del.tts, metavar_names) { + return false; + } + }, + TokenTree::Sequence(_, ref seq) => { + if !check_lhs_duplicate_matcher_bindings(sess, &seq.tts, metavar_names) { + return false; + } + } + _ => {} + } + } + + true +} + fn check_rhs(sess: &ParseSess, rhs: "ed::TokenTree) -> bool { match *rhs { quoted::TokenTree::Delimited(..) => return true, From 1d94cc2a2249b65f62e78e4998761193c3bfed23 Mon Sep 17 00:00:00 2001 From: mark Date: Tue, 15 Jan 2019 12:25:41 -0600 Subject: [PATCH 2/5] fix existing tests --- src/test/run-pass/macros/macro-follow.rs | 16 ++++++------ src/test/ui/macros/macro-follow.rs | 16 ++++++------ src/test/ui/macros/macro-follow.stderr | 32 ++++++++++++------------ 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/test/run-pass/macros/macro-follow.rs b/src/test/run-pass/macros/macro-follow.rs index f90afce90ee30..488339b025dce 100644 --- a/src/test/run-pass/macros/macro-follow.rs +++ b/src/test/run-pass/macros/macro-follow.rs @@ -73,7 +73,7 @@ macro_rules! follow_block { ($b:block $t:ty) => {}; ($b:block $s:stmt) => {}; ($b:block $p:path) => {}; - ($b:block $b:block) => {}; + ($b:block $c:block) => {}; ($b:block $i:ident) => {}; ($b:block $t:tt) => {}; ($b:block $i:item) => {}; @@ -99,9 +99,9 @@ macro_rules! follow_ident { ($i:ident $s:stmt) => {}; ($i:ident $p:path) => {}; ($i:ident $b:block) => {}; - ($i:ident $i:ident) => {}; + ($i:ident $j:ident) => {}; ($i:ident $t:tt) => {}; - ($i:ident $i:item) => {}; + ($i:ident $j:item) => {}; ($i:ident $m:meta) => {}; } // FOLLOW(tt) = any token @@ -120,12 +120,12 @@ macro_rules! follow_tt { ($t:tt ident) => {}; ($t:tt $p:pat) => {}; ($t:tt $e:expr) => {}; - ($t:tt $t:ty) => {}; + ($t:tt $v:ty) => {}; ($t:tt $s:stmt) => {}; ($t:tt $p:path) => {}; ($t:tt $b:block) => {}; ($t:tt $i:ident) => {}; - ($t:tt $t:tt) => {}; + ($t:tt $v:tt) => {}; ($t:tt $i:item) => {}; ($t:tt $m:meta) => {}; } @@ -149,9 +149,9 @@ macro_rules! follow_item { ($i:item $s:stmt) => {}; ($i:item $p:path) => {}; ($i:item $b:block) => {}; - ($i:item $i:ident) => {}; + ($i:item $j:ident) => {}; ($i:item $t:tt) => {}; - ($i:item $i:item) => {}; + ($i:item $j:item) => {}; ($i:item $m:meta) => {}; } // FOLLOW(meta) = any token @@ -177,7 +177,7 @@ macro_rules! follow_meta { ($m:meta $i:ident) => {}; ($m:meta $t:tt) => {}; ($m:meta $i:item) => {}; - ($m:meta $m:meta) => {}; + ($m:meta $n:meta) => {}; } fn main() {} diff --git a/src/test/ui/macros/macro-follow.rs b/src/test/ui/macros/macro-follow.rs index f4a1931da5aec..10b44e0017532 100644 --- a/src/test/ui/macros/macro-follow.rs +++ b/src/test/ui/macros/macro-follow.rs @@ -12,11 +12,11 @@ macro_rules! follow_pat { ($p:pat >) => {}; //~ERROR `$p:pat` is followed by `>` ($p:pat +) => {}; //~ERROR `$p:pat` is followed by `+` ($p:pat ident) => {}; //~ERROR `$p:pat` is followed by `ident` - ($p:pat $p:pat) => {}; //~ERROR `$p:pat` is followed by `$p:pat` + ($p:pat $q:pat) => {}; //~ERROR `$p:pat` is followed by `$q:pat` ($p:pat $e:expr) => {}; //~ERROR `$p:pat` is followed by `$e:expr` ($p:pat $t:ty) => {}; //~ERROR `$p:pat` is followed by `$t:ty` ($p:pat $s:stmt) => {}; //~ERROR `$p:pat` is followed by `$s:stmt` - ($p:pat $p:path) => {}; //~ERROR `$p:pat` is followed by `$p:path` + ($p:pat $q:path) => {}; //~ERROR `$p:pat` is followed by `$q:path` ($p:pat $b:block) => {}; //~ERROR `$p:pat` is followed by `$b:block` ($p:pat $i:ident) => {}; //~ERROR `$p:pat` is followed by `$i:ident` ($p:pat $t:tt) => {}; //~ERROR `$p:pat` is followed by `$t:tt` @@ -37,7 +37,7 @@ macro_rules! follow_expr { ($e:expr if) => {}; //~ERROR `$e:expr` is followed by `if` ($e:expr in) => {}; //~ERROR `$e:expr` is followed by `in` ($e:expr $p:pat) => {}; //~ERROR `$e:expr` is followed by `$p:pat` - ($e:expr $e:expr) => {}; //~ERROR `$e:expr` is followed by `$e:expr` + ($e:expr $f:expr) => {}; //~ERROR `$e:expr` is followed by `$f:expr` ($e:expr $t:ty) => {}; //~ERROR `$e:expr` is followed by `$t:ty` ($e:expr $s:stmt) => {}; //~ERROR `$e:expr` is followed by `$s:stmt` ($e:expr $p:path) => {}; //~ERROR `$e:expr` is followed by `$p:path` @@ -57,12 +57,12 @@ macro_rules! follow_ty { ($t:ty if) => {}; //~ERROR `$t:ty` is followed by `if` ($t:ty $p:pat) => {}; //~ERROR `$t:ty` is followed by `$p:pat` ($t:ty $e:expr) => {}; //~ERROR `$t:ty` is followed by `$e:expr` - ($t:ty $t:ty) => {}; //~ERROR `$t:ty` is followed by `$t:ty` + ($t:ty $r:ty) => {}; //~ERROR `$t:ty` is followed by `$r:ty` ($t:ty $s:stmt) => {}; //~ERROR `$t:ty` is followed by `$s:stmt` ($t:ty $p:path) => {}; //~ERROR `$t:ty` is followed by `$p:path` ($t:ty $b:block) => {}; // ok (RFC 1494) ($t:ty $i:ident) => {}; //~ERROR `$t:ty` is followed by `$i:ident` - ($t:ty $t:tt) => {}; //~ERROR `$t:ty` is followed by `$t:tt` + ($t:ty $r:tt) => {}; //~ERROR `$t:ty` is followed by `$r:tt` ($t:ty $i:item) => {}; //~ERROR `$t:ty` is followed by `$i:item` ($t:ty $m:meta) => {}; //~ERROR `$t:ty` is followed by `$m:meta` } @@ -82,7 +82,7 @@ macro_rules! follow_stmt { ($s:stmt $p:pat) => {}; //~ERROR `$s:stmt` is followed by `$p:pat` ($s:stmt $e:expr) => {}; //~ERROR `$s:stmt` is followed by `$e:expr` ($s:stmt $t:ty) => {}; //~ERROR `$s:stmt` is followed by `$t:ty` - ($s:stmt $s:stmt) => {}; //~ERROR `$s:stmt` is followed by `$s:stmt` + ($s:stmt $t:stmt) => {}; //~ERROR `$s:stmt` is followed by `$t:stmt` ($s:stmt $p:path) => {}; //~ERROR `$s:stmt` is followed by `$p:path` ($s:stmt $b:block) => {}; //~ERROR `$s:stmt` is followed by `$b:block` ($s:stmt $i:ident) => {}; //~ERROR `$s:stmt` is followed by `$i:ident` @@ -97,11 +97,11 @@ macro_rules! follow_path { ($p:path +) => {}; //~ERROR `$p:path` is followed by `+` ($p:path ident) => {}; //~ERROR `$p:path` is followed by `ident` ($p:path if) => {}; //~ERROR `$p:path` is followed by `if` - ($p:path $p:pat) => {}; //~ERROR `$p:path` is followed by `$p:pat` + ($p:path $q:pat) => {}; //~ERROR `$p:path` is followed by `$q:pat` ($p:path $e:expr) => {}; //~ERROR `$p:path` is followed by `$e:expr` ($p:path $t:ty) => {}; //~ERROR `$p:path` is followed by `$t:ty` ($p:path $s:stmt) => {}; //~ERROR `$p:path` is followed by `$s:stmt` - ($p:path $p:path) => {}; //~ERROR `$p:path` is followed by `$p:path` + ($p:path $q:path) => {}; //~ERROR `$p:path` is followed by `$q:path` ($p:path $b:block) => {}; // ok (RFC 1494) ($p:path $i:ident) => {}; //~ERROR `$p:path` is followed by `$i:ident` ($p:path $t:tt) => {}; //~ERROR `$p:path` is followed by `$t:tt` diff --git a/src/test/ui/macros/macro-follow.stderr b/src/test/ui/macros/macro-follow.stderr index 4aea5cc5de6bd..e3302eac4ac08 100644 --- a/src/test/ui/macros/macro-follow.stderr +++ b/src/test/ui/macros/macro-follow.stderr @@ -54,10 +54,10 @@ LL | ($p:pat ident) => {}; //~ERROR `$p:pat` is followed by `ident` | = note: allowed there are: `=>`, `,`, `=`, `|`, `if` or `in` -error: `$p:pat` is followed by `$p:pat`, which is not allowed for `pat` fragments +error: `$p:pat` is followed by `$q:pat`, which is not allowed for `pat` fragments --> $DIR/macro-follow.rs:15:13 | -LL | ($p:pat $p:pat) => {}; //~ERROR `$p:pat` is followed by `$p:pat` +LL | ($p:pat $q:pat) => {}; //~ERROR `$p:pat` is followed by `$q:pat` | ^^^^^^ not allowed after `pat` fragments | = note: allowed there are: `=>`, `,`, `=`, `|`, `if` or `in` @@ -86,10 +86,10 @@ LL | ($p:pat $s:stmt) => {}; //~ERROR `$p:pat` is followed by `$s:stmt` | = note: allowed there are: `=>`, `,`, `=`, `|`, `if` or `in` -error: `$p:pat` is followed by `$p:path`, which is not allowed for `pat` fragments +error: `$p:pat` is followed by `$q:path`, which is not allowed for `pat` fragments --> $DIR/macro-follow.rs:19:13 | -LL | ($p:pat $p:path) => {}; //~ERROR `$p:pat` is followed by `$p:path` +LL | ($p:pat $q:path) => {}; //~ERROR `$p:pat` is followed by `$q:path` | ^^^^^^^ not allowed after `pat` fragments | = note: allowed there are: `=>`, `,`, `=`, `|`, `if` or `in` @@ -230,10 +230,10 @@ LL | ($e:expr $p:pat) => {}; //~ERROR `$e:expr` is followed by `$p:pat` | = note: allowed there are: `=>`, `,` or `;` -error: `$e:expr` is followed by `$e:expr`, which is not allowed for `expr` fragments +error: `$e:expr` is followed by `$f:expr`, which is not allowed for `expr` fragments --> $DIR/macro-follow.rs:40:14 | -LL | ($e:expr $e:expr) => {}; //~ERROR `$e:expr` is followed by `$e:expr` +LL | ($e:expr $f:expr) => {}; //~ERROR `$e:expr` is followed by `$f:expr` | ^^^^^^^ not allowed after `expr` fragments | = note: allowed there are: `=>`, `,` or `;` @@ -350,10 +350,10 @@ LL | ($t:ty $e:expr) => {}; //~ERROR `$t:ty` is followed by `$e:expr` | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` -error: `$t:ty` is followed by `$t:ty`, which is not allowed for `ty` fragments +error: `$t:ty` is followed by `$r:ty`, which is not allowed for `ty` fragments --> $DIR/macro-follow.rs:60:12 | -LL | ($t:ty $t:ty) => {}; //~ERROR `$t:ty` is followed by `$t:ty` +LL | ($t:ty $r:ty) => {}; //~ERROR `$t:ty` is followed by `$r:ty` | ^^^^^ not allowed after `ty` fragments | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` @@ -382,10 +382,10 @@ LL | ($t:ty $i:ident) => {}; //~ERROR `$t:ty` is followed by `$i:ident` | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` -error: `$t:ty` is followed by `$t:tt`, which is not allowed for `ty` fragments +error: `$t:ty` is followed by `$r:tt`, which is not allowed for `ty` fragments --> $DIR/macro-follow.rs:65:12 | -LL | ($t:ty $t:tt) => {}; //~ERROR `$t:ty` is followed by `$t:tt` +LL | ($t:ty $r:tt) => {}; //~ERROR `$t:ty` is followed by `$r:tt` | ^^^^^ not allowed after `ty` fragments | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` @@ -518,10 +518,10 @@ LL | ($s:stmt $t:ty) => {}; //~ERROR `$s:stmt` is followed by `$t:ty` | = note: allowed there are: `=>`, `,` or `;` -error: `$s:stmt` is followed by `$s:stmt`, which is not allowed for `stmt` fragments +error: `$s:stmt` is followed by `$t:stmt`, which is not allowed for `stmt` fragments --> $DIR/macro-follow.rs:85:14 | -LL | ($s:stmt $s:stmt) => {}; //~ERROR `$s:stmt` is followed by `$s:stmt` +LL | ($s:stmt $t:stmt) => {}; //~ERROR `$s:stmt` is followed by `$t:stmt` | ^^^^^^^ not allowed after `stmt` fragments | = note: allowed there are: `=>`, `,` or `;` @@ -606,10 +606,10 @@ LL | ($p:path if) => {}; //~ERROR `$p:path` is followed by `if` | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` -error: `$p:path` is followed by `$p:pat`, which is not allowed for `path` fragments +error: `$p:path` is followed by `$q:pat`, which is not allowed for `path` fragments --> $DIR/macro-follow.rs:100:14 | -LL | ($p:path $p:pat) => {}; //~ERROR `$p:path` is followed by `$p:pat` +LL | ($p:path $q:pat) => {}; //~ERROR `$p:path` is followed by `$q:pat` | ^^^^^^ not allowed after `path` fragments | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` @@ -638,10 +638,10 @@ LL | ($p:path $s:stmt) => {}; //~ERROR `$p:path` is followed by `$s:stmt` | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` -error: `$p:path` is followed by `$p:path`, which is not allowed for `path` fragments +error: `$p:path` is followed by `$q:path`, which is not allowed for `path` fragments --> $DIR/macro-follow.rs:104:14 | -LL | ($p:path $p:path) => {}; //~ERROR `$p:path` is followed by `$p:path` +LL | ($p:path $q:path) => {}; //~ERROR `$p:path` is followed by `$q:path` | ^^^^^^^ not allowed after `path` fragments | = note: allowed there are: `{`, `[`, `=>`, `,`, `>`, `=`, `:`, `;`, `|`, `as` or `where` From 3e790a7c304b3bcb182c4fcb745ea288d3dc56f2 Mon Sep 17 00:00:00 2001 From: mark Date: Tue, 15 Jan 2019 16:55:23 -0600 Subject: [PATCH 3/5] add a test --- .../macros/macro-multiple-matcher-bindings.rs | 19 +++++++ .../macro-multiple-matcher-bindings.stderr | 50 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 src/test/ui/macros/macro-multiple-matcher-bindings.rs create mode 100644 src/test/ui/macros/macro-multiple-matcher-bindings.stderr diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.rs b/src/test/ui/macros/macro-multiple-matcher-bindings.rs new file mode 100644 index 0000000000000..ea57d66b565d9 --- /dev/null +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.rs @@ -0,0 +1,19 @@ +// Test that duplicate matcher binding names are caught at declaration time, rather than at macro +// invocation time. + +macro_rules! foo1 { + ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding + ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding +} + +macro_rules! foo2 { + ($a:ident) => {}; // OK + ($a:path) => {}; // OK +} + +macro_rules! foo3 { + ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding + ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding +} + +fn main() {} diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.stderr b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr new file mode 100644 index 0000000000000..3e3e10245870f --- /dev/null +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr @@ -0,0 +1,50 @@ +error: duplicate matcher binding + --> $DIR/macro-multiple-matcher-bindings.rs:5:16 + | +LL | ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + | +note: previous declaration was here + --> $DIR/macro-multiple-matcher-bindings.rs:5:6 + | +LL | ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + +error: duplicate matcher binding + --> $DIR/macro-multiple-matcher-bindings.rs:6:16 + | +LL | ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^ + | +note: previous declaration was here + --> $DIR/macro-multiple-matcher-bindings.rs:6:6 + | +LL | ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + +error: duplicate matcher binding + --> $DIR/macro-multiple-matcher-bindings.rs:15:18 + | +LL | ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + | +note: previous declaration was here + --> $DIR/macro-multiple-matcher-bindings.rs:15:6 + | +LL | ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + +error: duplicate matcher binding + --> $DIR/macro-multiple-matcher-bindings.rs:16:25 + | +LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^ + | +note: previous declaration was here + --> $DIR/macro-multiple-matcher-bindings.rs:16:8 + | +LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding + | ^^^^^^^^ + +error: aborting due to 4 previous errors + From 802b256283fe7dd0f0e72499ba77d18f2838e817 Mon Sep 17 00:00:00 2001 From: Mark Mansi Date: Thu, 17 Jan 2019 21:19:56 -0600 Subject: [PATCH 4/5] Make it an incompatibility lint for now --- src/librustc/lint/builtin.rs | 6 ++ src/librustc/lint/mod.rs | 3 +- src/librustc_lint/lib.rs | 5 ++ src/libsyntax/early_buffered_lints.rs | 2 + src/libsyntax/ext/tt/macro_rules.rs | 31 +++++--- .../macros/macro-multiple-matcher-bindings.rs | 10 +-- .../macro-multiple-matcher-bindings.stderr | 71 ++++++++----------- 7 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 6ae7448645a20..3ff76e98d7b89 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -352,6 +352,12 @@ declare_lint! { "outlives requirements can be inferred" } +declare_lint! { + pub DUPLICATE_MATCHER_BINDING_NAME, + Warn, + "duplicate macro matcher binding name" +} + /// Some lints that are buffered from `libsyntax`. See `syntax::early_buffered_lints`. pub mod parser { declare_lint! { diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 4e6bf753b01aa..8952ae98e597e 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -27,7 +27,7 @@ use crate::errors::{DiagnosticBuilder, DiagnosticId}; use crate::hir::def_id::{CrateNum, LOCAL_CRATE}; use crate::hir::intravisit; use crate::hir; -use crate::lint::builtin::BuiltinLintDiagnostics; +use crate::lint::builtin::{BuiltinLintDiagnostics, DUPLICATE_MATCHER_BINDING_NAME}; use crate::lint::builtin::parser::{QUESTION_MARK_MACRO_SEP, ILL_FORMED_ATTRIBUTE_INPUT}; use crate::session::{Session, DiagnosticMessageId}; use std::{hash, ptr}; @@ -82,6 +82,7 @@ impl Lint { match lint_id { BufferedEarlyLintId::QuestionMarkMacroSep => QUESTION_MARK_MACRO_SEP, BufferedEarlyLintId::IllFormedAttributeInput => ILL_FORMED_ATTRIBUTE_INPUT, + BufferedEarlyLintId::DuplicateMacroMatcherBindingName => DUPLICATE_MATCHER_BINDING_NAME, } } diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs index fd5e68d5ae60a..460cb977346a9 100644 --- a/src/librustc_lint/lib.rs +++ b/src/librustc_lint/lib.rs @@ -354,6 +354,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { reference: "issue #57644 ", edition: None, }, + FutureIncompatibleInfo { + id: LintId::of(DUPLICATE_MATCHER_BINDING_NAME), + reference: "issue #57593 ", + edition: None, + }, ]); // Register renamed and removed lints. diff --git a/src/libsyntax/early_buffered_lints.rs b/src/libsyntax/early_buffered_lints.rs index 977e6d4587709..29cb9cd7f30b5 100644 --- a/src/libsyntax/early_buffered_lints.rs +++ b/src/libsyntax/early_buffered_lints.rs @@ -12,6 +12,8 @@ pub enum BufferedEarlyLintId { /// Usage of `?` as a macro separator is deprecated. QuestionMarkMacroSep, IllFormedAttributeInput, + /// Usage of a duplicate macro matcher binding name. + DuplicateMacroMatcherBindingName, } /// Stores buffered lint info which can later be passed to `librustc`. diff --git a/src/libsyntax/ext/tt/macro_rules.rs b/src/libsyntax/ext/tt/macro_rules.rs index 61a5b2cb0d200..33ea675f9d1bb 100644 --- a/src/libsyntax/ext/tt/macro_rules.rs +++ b/src/libsyntax/ext/tt/macro_rules.rs @@ -360,8 +360,12 @@ pub fn compile( // don't abort iteration early, so that errors for multiple lhses can be reported for lhs in &lhses { valid &= check_lhs_no_empty_seq(sess, &[lhs.clone()]); - valid &= - check_lhs_duplicate_matcher_bindings(sess, &[lhs.clone()], &mut FxHashMap::default()); + valid &= check_lhs_duplicate_matcher_bindings( + sess, + &[lhs.clone()], + &mut FxHashMap::default(), + def.id + ); } let expander: Box<_> = Box::new(MacroRulesMacroExpander { @@ -467,29 +471,38 @@ fn check_lhs_no_empty_seq(sess: &ParseSess, tts: &[quoted::TokenTree]) -> bool { fn check_lhs_duplicate_matcher_bindings( sess: &ParseSess, tts: &[quoted::TokenTree], - metavar_names: &mut FxHashMap + metavar_names: &mut FxHashMap, + node_id: ast::NodeId, ) -> bool { use self::quoted::TokenTree; + use crate::early_buffered_lints::BufferedEarlyLintId; for tt in tts { match *tt { TokenTree::MetaVarDecl(span, name, _kind) => { if let Some(&prev_span) = metavar_names.get(&name) { - sess.span_diagnostic - .struct_span_err(span, "duplicate matcher binding") - .span_note(prev_span, "previous declaration was here") - .emit(); + // FIXME(mark-i-m): in a few cycles, make this a hard error. + // sess.span_diagnostic + // .struct_span_err(span, "duplicate matcher binding") + // .span_note(prev_span, "previous declaration was here") + // .emit(); + sess.buffer_lint( + BufferedEarlyLintId::DuplicateMacroMatcherBindingName, + crate::source_map::MultiSpan::from(vec![prev_span, span]), + node_id, + "duplicate matcher binding" + ); return false; } else { metavar_names.insert(name, span); } } TokenTree::Delimited(_, ref del) => { - if !check_lhs_duplicate_matcher_bindings(sess, &del.tts, metavar_names) { + if !check_lhs_duplicate_matcher_bindings(sess, &del.tts, metavar_names, node_id) { return false; } }, TokenTree::Sequence(_, ref seq) => { - if !check_lhs_duplicate_matcher_bindings(sess, &seq.tts, metavar_names) { + if !check_lhs_duplicate_matcher_bindings(sess, &seq.tts, metavar_names, node_id) { return false; } } diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.rs b/src/test/ui/macros/macro-multiple-matcher-bindings.rs index ea57d66b565d9..3deae3eacecb2 100644 --- a/src/test/ui/macros/macro-multiple-matcher-bindings.rs +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.rs @@ -1,9 +1,11 @@ // Test that duplicate matcher binding names are caught at declaration time, rather than at macro // invocation time. +#![allow(unused_macros)] + macro_rules! foo1 { - ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding - ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding + ($a:ident, $a:ident) => {}; //~WARN duplicate matcher binding + ($a:ident, $a:path) => {}; //~WARN duplicate matcher binding } macro_rules! foo2 { @@ -12,8 +14,8 @@ macro_rules! foo2 { } macro_rules! foo3 { - ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding - ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding + ($a:ident, $($a:ident),*) => {}; //~WARN duplicate matcher binding + ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARN duplicate matcher binding } fn main() {} diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.stderr b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr index 3e3e10245870f..04b27e88a1f05 100644 --- a/src/test/ui/macros/macro-multiple-matcher-bindings.stderr +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr @@ -1,50 +1,37 @@ -error: duplicate matcher binding - --> $DIR/macro-multiple-matcher-bindings.rs:5:16 - | -LL | ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ - | -note: previous declaration was here - --> $DIR/macro-multiple-matcher-bindings.rs:5:6 - | -LL | ($a:ident, $a:ident) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ +warning: duplicate matcher binding + --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:7:6 + | +7 | ($a:ident, $a:ident) => {}; //~WARN duplicate matcher binding + | ^^^^^^^^ ^^^^^^^^ + | + = note: #[warn(duplicate_matcher_binding_name)] on by default + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 -error: duplicate matcher binding - --> $DIR/macro-multiple-matcher-bindings.rs:6:16 - | -LL | ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^ - | -note: previous declaration was here - --> $DIR/macro-multiple-matcher-bindings.rs:6:6 - | -LL | ($a:ident, $a:path) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ +warning: duplicate matcher binding + --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:8:6 + | +8 | ($a:ident, $a:path) => {}; //~WARN duplicate matcher binding + | ^^^^^^^^ ^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 -error: duplicate matcher binding - --> $DIR/macro-multiple-matcher-bindings.rs:15:18 - | -LL | ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ +warning: duplicate matcher binding + --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:17:6 | -note: previous declaration was here - --> $DIR/macro-multiple-matcher-bindings.rs:15:6 +LL | ($a:ident, $($a:ident),*) => {}; //~WARN duplicate matcher binding + | ^^^^^^^^ ^^^^^^^^ | -LL | ($a:ident, $($a:ident),*) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 -error: duplicate matcher binding - --> $DIR/macro-multiple-matcher-bindings.rs:16:25 +warning: duplicate matcher binding + --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:18:8 | -LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^ +LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARN duplicate matcher binding + | ^^^^^^^^ ^^^^^^^ | -note: previous declaration was here - --> $DIR/macro-multiple-matcher-bindings.rs:16:8 - | -LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~ERROR duplicate matcher binding - | ^^^^^^^^ - -error: aborting due to 4 previous errors + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 From c25d6b83441e0c060ee0273193ef27b29e1318cd Mon Sep 17 00:00:00 2001 From: Mark Mansi Date: Thu, 17 Jan 2019 21:30:06 -0600 Subject: [PATCH 5/5] update test --- .../macros/macro-multiple-matcher-bindings.rs | 12 ++++-- .../macro-multiple-matcher-bindings.stderr | 38 +++++++++---------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.rs b/src/test/ui/macros/macro-multiple-matcher-bindings.rs index 3deae3eacecb2..9e0fa3887163d 100644 --- a/src/test/ui/macros/macro-multiple-matcher-bindings.rs +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.rs @@ -1,11 +1,15 @@ // Test that duplicate matcher binding names are caught at declaration time, rather than at macro // invocation time. +// +// FIXME(mark-i-m): Update this when it becomes a hard error. + +// compile-pass #![allow(unused_macros)] macro_rules! foo1 { - ($a:ident, $a:ident) => {}; //~WARN duplicate matcher binding - ($a:ident, $a:path) => {}; //~WARN duplicate matcher binding + ($a:ident, $a:ident) => {}; //~WARNING duplicate matcher binding + ($a:ident, $a:path) => {}; //~WARNING duplicate matcher binding } macro_rules! foo2 { @@ -14,8 +18,8 @@ macro_rules! foo2 { } macro_rules! foo3 { - ($a:ident, $($a:ident),*) => {}; //~WARN duplicate matcher binding - ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARN duplicate matcher binding + ($a:ident, $($a:ident),*) => {}; //~WARNING duplicate matcher binding + ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARNING duplicate matcher binding } fn main() {} diff --git a/src/test/ui/macros/macro-multiple-matcher-bindings.stderr b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr index 04b27e88a1f05..bc78b471a2d1e 100644 --- a/src/test/ui/macros/macro-multiple-matcher-bindings.stderr +++ b/src/test/ui/macros/macro-multiple-matcher-bindings.stderr @@ -1,35 +1,35 @@ warning: duplicate matcher binding - --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:7:6 - | -7 | ($a:ident, $a:ident) => {}; //~WARN duplicate matcher binding - | ^^^^^^^^ ^^^^^^^^ - | - = note: #[warn(duplicate_matcher_binding_name)] on by default - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #57593 + --> $DIR/macro-multiple-matcher-bindings.rs:11:6 + | +LL | ($a:ident, $a:ident) => {}; //~WARNING duplicate matcher binding + | ^^^^^^^^ ^^^^^^^^ + | + = note: #[warn(duplicate_matcher_binding_name)] on by default + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 warning: duplicate matcher binding - --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:8:6 - | -8 | ($a:ident, $a:path) => {}; //~WARN duplicate matcher binding - | ^^^^^^^^ ^^^^^^^ - | - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #57593 + --> $DIR/macro-multiple-matcher-bindings.rs:12:6 + | +LL | ($a:ident, $a:path) => {}; //~WARNING duplicate matcher binding + | ^^^^^^^^ ^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #57593 warning: duplicate matcher binding - --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:17:6 + --> $DIR/macro-multiple-matcher-bindings.rs:21:6 | -LL | ($a:ident, $($a:ident),*) => {}; //~WARN duplicate matcher binding +LL | ($a:ident, $($a:ident),*) => {}; //~WARNING duplicate matcher binding | ^^^^^^^^ ^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #57593 warning: duplicate matcher binding - --> src/test/ui/macros/macro-multiple-matcher-bindings.rs:18:8 + --> $DIR/macro-multiple-matcher-bindings.rs:22:8 | -LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARN duplicate matcher binding +LL | ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARNING duplicate matcher binding | ^^^^^^^^ ^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!