Auto merge of #57617 - mark-i-m:multiple-matcher-bindings, r=petroche…

…nkov

Error on duplicate matcher bindings

fix  #57593

This should not be merged without a crater run and maybe an FCP. Discussion is ongoing at  #57593.

TODO:
- [x] write tests
- [x] crater run
- [x] ~maybe need edition gating?~ not for 1 regression /centril

r? @petrochenkov

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! {

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,
         }
     }
 

src/librustc_lint/lib.rs

@@ -348,6 +348,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             reference: "issue #57644 <https://github.com/rust-lang/rust/issues/57644>",
             edition: None,
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(DUPLICATE_MATCHER_BINDING_NAME),
+            reference: "issue #57593 <https://github.com/rust-lang/rust/issues/57593>",
+            edition: None,
+        },
         ]);
 
     // Register renamed and removed lints.

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`.

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,13 @@ 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(),
+            def.id
+        );
     }
 
     let expander: Box<_> = Box::new(MacroRulesMacroExpander {
@@ -456,6 +466,53 @@ 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<Ident, Span>,
+    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) {
+                    // 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, node_id) {
+                    return false;
+                }
+            },
+            TokenTree::Sequence(_, ref seq) => {
+                if !check_lhs_duplicate_matcher_bindings(sess, &seq.tts, metavar_names, node_id) {
+                    return false;
+                }
+            }
+            _ => {}
+        }
+    }
+
+    true
+}
+
 fn check_rhs(sess: &ParseSess, rhs: &quoted::TokenTree) -> bool {
     match *rhs {
         quoted::TokenTree::Delimited(..) => return true,

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() {}

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`

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`

src/test/ui/macros/macro-multiple-matcher-bindings.rs

@@ -0,0 +1,25 @@
+// 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) => {}; //~WARNING duplicate matcher binding
+    ($a:ident, $a:path) => {};  //~WARNING duplicate matcher binding
+}
+
+macro_rules! foo2 {
+    ($a:ident) => {}; // OK
+    ($a:path) => {};  // OK
+}
+
+macro_rules! foo3 {
+    ($a:ident, $($a:ident),*) => {}; //~WARNING duplicate matcher binding
+    ($($a:ident)+ # $($($a:path),+);*) => {}; //~WARNING duplicate matcher binding
+}
+
+fn main() {}