Skip to content

Commit

Permalink
Auto merge of #49719 - mark-i-m:no_sep, r=petrochenkov
Browse files Browse the repository at this point in the history 
Update `?` repetition disambiguation.

**Do not merge** (yet)

This is a test implementation of some ideas from discussion in #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
  • Loading branch information
@bors
bors committed Apr 16, 2018
2 parents 8de5353 + 54bba4c commit d6ba1b9
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 156 deletions.
89 changes: 22 additions & 67 deletions src/libsyntax/ext/tt/quoted.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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),

Expand All @@ -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 {
Expand Down
29 changes: 6 additions & 23 deletions src/test/run-pass/macro-at-most-once-rep.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
} }
Expand All @@ -62,7 +50,7 @@ macro_rules! barstar {

$(
x += $a;
)*
)?

assert_eq!(x, $num);
} }
Expand All @@ -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);
}
34 changes: 15 additions & 19 deletions src/test/ui/macros/macro-at-most-once-rep-ambig.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)?*) => {}
}
Expand All @@ -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
}
76 changes: 29 additions & 47 deletions src/test/ui/macros/macro-at-most-once-rep-ambig.stderr
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit d6ba1b9

Please sign in to comment.