Auto merge of #49719 - mark-i-m:no_sep, r=petrochenkov

Update `?` repetition disambiguation.

**Do not merge** (yet)

This is a test implementation of some ideas from discussion in https://github.com/rust-lang/rust/issues/48075 . This PR
- disallows `?` repetition from taking a separator, since the separator is never used.
- disallows the use of `?` as a separator. This allows patterns like `$(a)?+` to match `+` and `a+` rather than `a?a?a`. This is a _breaking change_, but maybe that's ok? Perhaps a crater run is the right approach?

cc @durka @alexreg @nikomatsakis

4 files changed, 72 insertions, 156 deletions

diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs
index 01b971976a7..77c6afa1c64 100644
--- a/src/libsyntax/ext/tt/quoted.rs
+++ b/src/libsyntax/ext/tt/quoted.rs
@@ -386,72 +386,26 @@ where
 {
     // We basically look at two token trees here, denoted as #1 and #2 below
     let span = match parse_kleene_op(input, span) {
-        // #1 is a `+` or `*` KleeneOp
-        //
-        // `?` is ambiguous: it could be a separator or a Kleene::ZeroOrOne, so we need to look
-        // ahead one more token to be sure.
-        Ok(Ok(op)) if op != KleeneOp::ZeroOrOne => return (None, op),
-
-        // #1 is `?` token, but it could be a Kleene::ZeroOrOne without a separator or it could
-        // be a `?` separator followed by any Kleene operator. We need to look ahead 1 token to
-        // find out which.
-        Ok(Ok(op)) => {
-            assert_eq!(op, KleeneOp::ZeroOrOne);
-
-            // Lookahead at #2. If it is a KleenOp, then #1 is a separator.
-            let is_1_sep = if let Some(&tokenstream::TokenTree::Token(_, ref tok2)) = input.peek() {
-                kleene_op(tok2).is_some()
-            } else {
-                false
-            };
-
-            if is_1_sep {
-                // #1 is a separator and #2 should be a KleepeOp::*
-                // (N.B. We need to advance the input iterator.)
-                match parse_kleene_op(input, span) {
-                    // #2 is a KleeneOp (this is the only valid option) :)
-                    Ok(Ok(op)) if op == KleeneOp::ZeroOrOne => {
-                        if !features.macro_at_most_once_rep
-                            && !attr::contains_name(attrs, "allow_internal_unstable")
-                        {
-                            let explain = feature_gate::EXPLAIN_MACRO_AT_MOST_ONCE_REP;
-                            emit_feature_err(
-                                sess,
-                                "macro_at_most_once_rep",
-                                span,
-                                GateIssue::Language,
-                                explain,
-                            );
-                        }
-                        return (Some(token::Question), op);
-                    }
-                    Ok(Ok(op)) => return (Some(token::Question), op),
-
-                    // #2 is a random token (this is an error) :(
-                    Ok(Err((_, span))) => span,
-
-                    // #2 is not even a token at all :(
-                    Err(span) => span,
-                }
-            } else {
-                if !features.macro_at_most_once_rep
-                    && !attr::contains_name(attrs, "allow_internal_unstable")
-                {
-                    let explain = feature_gate::EXPLAIN_MACRO_AT_MOST_ONCE_REP;
-                    emit_feature_err(
-                        sess,
-                        "macro_at_most_once_rep",
-                        span,
-                        GateIssue::Language,
-                        explain,
-                    );
-                }
-
-                // #2 is a random tree and #1 is KleeneOp::ZeroOrOne
-                return (None, op);
+        // #1 is any KleeneOp (`?`)
+        Ok(Ok(op)) if op == KleeneOp::ZeroOrOne => {
+            if !features.macro_at_most_once_rep
+                && !attr::contains_name(attrs, "allow_internal_unstable")
+            {
+                let explain = feature_gate::EXPLAIN_MACRO_AT_MOST_ONCE_REP;
+                emit_feature_err(
+                    sess,
+                    "macro_at_most_once_rep",
+                    span,
+                    GateIssue::Language,
+                    explain,
+                );
             }
+            return (None, op);
         }
 
+        // #1 is any KleeneOp (`+`, `*`)
+        Ok(Ok(op)) => return (None, op),
+
         // #1 is a separator followed by #2, a KleeneOp
         Ok(Err((tok, span))) => match parse_kleene_op(input, span) {
             // #2 is a KleeneOp :D
@@ -467,8 +421,11 @@ where
                         GateIssue::Language,
                         explain,
                     );
+                } else {
+                    sess.span_diagnostic
+                        .span_err(span, "`?` macro repetition does not allow a separator");
                 }
-                return (Some(tok), op);
+                return (None, op);
             }
             Ok(Ok(op)) => return (Some(tok), op),
 
@@ -483,9 +440,7 @@ where
         Err(span) => span,
     };
 
-    if !features.macro_at_most_once_rep
-        && !attr::contains_name(attrs, "allow_internal_unstable")
-    {
+    if !features.macro_at_most_once_rep && !attr::contains_name(attrs, "allow_internal_unstable") {
         sess.span_diagnostic
             .span_err(span, "expected one of: `*`, `+`, or `?`");
     } else {
diff --git a/src/test/run-pass/macro-at-most-once-rep.rs b/src/test/run-pass/macro-at-most-once-rep.rs
index b7e942f9383..c08effe5493 100644
--- a/src/test/run-pass/macro-at-most-once-rep.rs
+++ b/src/test/run-pass/macro-at-most-once-rep.rs
@@ -32,25 +32,13 @@ macro_rules! foo {
     } }
 }
 
-macro_rules! baz {
-    ($($a:ident),? ; $num:expr) => { { // comma separator is meaningless for `?`
-        let mut x = 0;
-
-        $(
-            x += $a;
-         )?
-
-        assert_eq!(x, $num);
-    } }
-}
-
 macro_rules! barplus {
     ($($a:ident)?+ ; $num:expr) => { {
         let mut x = 0;
 
         $(
             x += $a;
-         )+
+         )?
 
         assert_eq!(x, $num);
     } }
@@ -62,7 +50,7 @@ macro_rules! barstar {
 
         $(
             x += $a;
-         )*
+         )?
 
         assert_eq!(x, $num);
     } }
@@ -74,15 +62,10 @@ pub fn main() {
     // accept 0 or 1 repetitions
     foo!( ; 0);
     foo!(a ; 1);
-    baz!( ; 0);
-    baz!(a ; 1);
 
     // Make sure using ? as a separator works as before
-    barplus!(a ; 1);
-    barplus!(a?a ; 2);
-    barplus!(a?a?a ; 3);
-    barstar!( ; 0);
-    barstar!(a ; 1);
-    barstar!(a?a ; 2);
-    barstar!(a?a?a ; 3);
+    barplus!(+ ; 0);
+    barplus!(a + ; 1);
+    barstar!(* ; 0);
+    barstar!(a * ; 1);
 }
diff --git a/src/test/ui/macros/macro-at-most-once-rep-ambig.rs b/src/test/ui/macros/macro-at-most-once-rep-ambig.rs
index a5660f8b41f..e25c3ccfcd9 100644
--- a/src/test/ui/macros/macro-at-most-once-rep-ambig.rs
+++ b/src/test/ui/macros/macro-at-most-once-rep-ambig.rs
@@ -8,30 +8,26 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// The logic for parsing Kleene operators in macros has a special case to disambiguate `?`.
-// Specifically, `$(pat)?` is the ZeroOrOne operator whereas `$(pat)?+` or `$(pat)?*` are the
-// ZeroOrMore and OneOrMore operators using `?` as a separator. These tests are intended to
-// exercise that logic in the macro parser.
-//
-// Moreover, we also throw in some tests for using a separator with `?`, which is meaningless but
-// included for consistency with `+` and `*`.
-//
-// This test focuses on error cases.
+// Tests the behavior of various Kleene operators in macros with respect to `?` terminals. In
+// particular, `?` in the position of a separator and of a Kleene operator is tested.
 
 #![feature(macro_at_most_once_rep)]
 
+// should match `` and `a`
 macro_rules! foo {
     ($(a)?) => {}
 }
 
 macro_rules! baz {
-    ($(a),?) => {} // comma separator is meaningless for `?`
+    ($(a),?) => {} //~ ERROR `?` macro repetition does not allow a separator
 }
 
+// should match `+` and `a+`
 macro_rules! barplus {
     ($(a)?+) => {}
 }
 
+// should match `*` and `a*`
 macro_rules! barstar {
     ($(a)?*) => {}
 }
@@ -40,14 +36,14 @@ pub fn main() {
     foo!(a?a?a); //~ ERROR no rules expected the token `?`
     foo!(a?a); //~ ERROR no rules expected the token `?`
     foo!(a?); //~ ERROR no rules expected the token `?`
-    baz!(a?a?a); //~ ERROR no rules expected the token `?`
-    baz!(a?a); //~ ERROR no rules expected the token `?`
-    baz!(a?); //~ ERROR no rules expected the token `?`
-    baz!(a,); //~ ERROR unexpected end of macro invocation
-    baz!(a?a?a,); //~ ERROR no rules expected the token `?`
-    baz!(a?a,); //~ ERROR no rules expected the token `?`
-    baz!(a?,); //~ ERROR no rules expected the token `?`
     barplus!(); //~ ERROR unexpected end of macro invocation
-    barplus!(a?); //~ ERROR unexpected end of macro invocation
-    barstar!(a?); //~ ERROR unexpected end of macro invocation
+    barstar!(); //~ ERROR unexpected end of macro invocation
+    barplus!(a?); //~ ERROR no rules expected the token `?`
+    barplus!(a); //~ ERROR unexpected end of macro invocation
+    barstar!(a?); //~ ERROR no rules expected the token `?`
+    barstar!(a); //~ ERROR unexpected end of macro invocation
+    barplus!(+); // ok
+    barstar!(*); // ok
+    barplus!(a+); // ok
+    barstar!(a*); // ok
 }
diff --git a/src/test/ui/macros/macro-at-most-once-rep-ambig.stderr b/src/test/ui/macros/macro-at-most-once-rep-ambig.stderr
index d382082a575..cb1e360471c 100644
--- a/src/test/ui/macros/macro-at-most-once-rep-ambig.stderr
+++ b/src/test/ui/macros/macro-at-most-once-rep-ambig.stderr
@@ -1,80 +1,62 @@
+error: `?` macro repetition does not allow a separator
+  --> $DIR/macro-at-most-once-rep-ambig.rs:22:10
+   |
+LL |     ($(a),?) => {} //~ ERROR `?` macro repetition does not allow a separator
+   |          ^
+
 error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:40:11
+  --> $DIR/macro-at-most-once-rep-ambig.rs:36:11
    |
 LL |     foo!(a?a?a); //~ ERROR no rules expected the token `?`
    |           ^
 
 error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:41:11
+  --> $DIR/macro-at-most-once-rep-ambig.rs:37:11
    |
 LL |     foo!(a?a); //~ ERROR no rules expected the token `?`
    |           ^
 
 error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:42:11
+  --> $DIR/macro-at-most-once-rep-ambig.rs:38:11
    |
 LL |     foo!(a?); //~ ERROR no rules expected the token `?`
    |           ^
 
-error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:43:11
-   |
-LL |     baz!(a?a?a); //~ ERROR no rules expected the token `?`
-   |           ^
-
-error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:44:11
-   |
-LL |     baz!(a?a); //~ ERROR no rules expected the token `?`
-   |           ^
-
-error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:45:11
-   |
-LL |     baz!(a?); //~ ERROR no rules expected the token `?`
-   |           ^
-
 error: unexpected end of macro invocation
-  --> $DIR/macro-at-most-once-rep-ambig.rs:46:11
-   |
-LL |     baz!(a,); //~ ERROR unexpected end of macro invocation
-   |           ^
-
-error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:47:11
+  --> $DIR/macro-at-most-once-rep-ambig.rs:39:5
    |
-LL |     baz!(a?a?a,); //~ ERROR no rules expected the token `?`
-   |           ^
+LL |     barplus!(); //~ ERROR unexpected end of macro invocation
+   |     ^^^^^^^^^^^
 
-error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:48:11
+error: unexpected end of macro invocation
+  --> $DIR/macro-at-most-once-rep-ambig.rs:40:5
    |
-LL |     baz!(a?a,); //~ ERROR no rules expected the token `?`
-   |           ^
+LL |     barstar!(); //~ ERROR unexpected end of macro invocation
+   |     ^^^^^^^^^^^
 
 error: no rules expected the token `?`
-  --> $DIR/macro-at-most-once-rep-ambig.rs:49:11
+  --> $DIR/macro-at-most-once-rep-ambig.rs:41:15
    |
-LL |     baz!(a?,); //~ ERROR no rules expected the token `?`
-   |           ^
+LL |     barplus!(a?); //~ ERROR no rules expected the token `?`
+   |               ^
 
 error: unexpected end of macro invocation
-  --> $DIR/macro-at-most-once-rep-ambig.rs:50:5
+  --> $DIR/macro-at-most-once-rep-ambig.rs:42:14
    |
-LL |     barplus!(); //~ ERROR unexpected end of macro invocation
-   |     ^^^^^^^^^^^
+LL |     barplus!(a); //~ ERROR unexpected end of macro invocation
+   |              ^
 
-error: unexpected end of macro invocation
-  --> $DIR/macro-at-most-once-rep-ambig.rs:51:15
+error: no rules expected the token `?`
+  --> $DIR/macro-at-most-once-rep-ambig.rs:43:15
    |
-LL |     barplus!(a?); //~ ERROR unexpected end of macro invocation
+LL |     barstar!(a?); //~ ERROR no rules expected the token `?`
    |               ^
 
 error: unexpected end of macro invocation
-  --> $DIR/macro-at-most-once-rep-ambig.rs:52:15
+  --> $DIR/macro-at-most-once-rep-ambig.rs:44:14
    |
-LL |     barstar!(a?); //~ ERROR unexpected end of macro invocation
-   |               ^
+LL |     barstar!(a); //~ ERROR unexpected end of macro invocation
+   |              ^
 
-error: aborting due to 13 previous errors
+error: aborting due to 10 previous errors