Simplify TokenTree and fix macro_rules! bugs by jseyfried · Pull Request #39419 · rust-lang/rust (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation37 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR
- fixes macros: tt fragments treat $(foo)* like foo #39390, fixes macros: simplify tt matchers #39403, and fixes macros: syntax errors in matches should fail on definition, not use #39404 (each is a [breaking-change], see issues for examples),
- fixes Regression in procedural macros emitting macro_rules macros #39889,
- simplifies and optimizes macro invocation parsing,
- cleans up
ext::tt::transcribe, - removes
tokenstream::TokenTree::SequenceandToken::MatchNt,- instead, adds a new type
ext::tt::quoted::TokenTreefor use bymacro_rules!(ext::tt)
- instead, adds a new type
- removes
parser.quote_depthandparser.parsing_token_tree, and - removes
quote_matcher!.- Instead, use
quote_tokens!andext::tt::quoted::parsethe result withexpect_matchers=true. - I found no outside uses of
quote_matcher!when searching Rust code on Github.
- Instead, use
r? @nrc
This is groundwork for procedural and declarative macros 2.0.
cc #38356 #39412
| /// A kleene-style repetition sequence with a span |
|---|
| Sequence(Span, Rc), |
| /// Matches a nonterminal. This is only used in the left hand side of MBE macros. |
| Match(Span, ast::Ident /* name to bind */, ast::Ident /* kind of nonterminal */), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be done on the fly? (i.e in parsing not lexing)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither parsing nor lexing knows about sequences and matchers at all. We lex directly into a Vec<TokenTree> and consume the token trees in the parser directly (e.g. parsing a token tree is constant time). Sequences and matchers are parsed by ext::tt::quoted::parse.
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only got halfway through reviewing this PR before the (long) weekend, and then my browser crashed and lost my comments. There wasn't anything major. Anyway, here is a single comment and I'll finish the review on Tuesday. Sorry for the delay.
| idx: usize, |
|---|
| dotdotdoted: bool, |
| sep: Option, |
| enum Frame { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would benefit from a comment
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| } |
|---|
| } |
| let mut stack = SmallVector::one(Frame::Delimited { |
| forest: Rc::new(tokenstream::Delimited { delim: token::NoDelim, tts: src }), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would factor this out into a ctor function
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| enum LockstepIterSize { |
|---|
| LisUnconstrained, |
| LisConstraint(usize, Ident), |
| LisContradiction(String), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd kill the Lis prefix
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @@ -0,0 +1,228 @@ |
|---|
| // Copyright 2017 The Rust Project Developers. See the COPYRIGHT |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make this module here? I'd have thought TTs were much more general than just quasi quoting
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have tokenstream::{TokenStream, TokenTree, ...}.
quoted::TokenTree is more general than tokenstream::TokenTree since it also supports "$-quoted tokens" from the macro_rules! syntax -- $e, $e:expr, and $(...)*. In other words, quoted::TokenTree is the parse tree for macro_rules! syntax.
Oh, GH didn't lose my earlier comments. Good.
nrc approved these changes Feb 7, 2017
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more comment, and that can be done later
| @@ -76,6 +76,8 @@ pub enum TokenTree { |
|---|
| Delimited(Span, Rc), |
| /// A kleene-style repetition sequence with a span |
| Sequence(Span, Rc), |
| /// Matches a nonterminal. This is only used in the left hand side of MBE macros. |
| Match(Span, ast::Ident /* name to bind */, ast::Ident /* kind of nonterminal */), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Match is a pretty bad name - it is not actually a match of a pattern to text (or anything to do with a match expression). Maybe PatternVariable or MacroVariable is better? Doesn't have to be done in this PR though.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a bit more refactoring I could also move Nonterminal::SubstNt here -- I think MacroVariable would be a better fit for that. MacroVariableDeclaration is too long though (imo); how about MetaVarDecl?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MetaVarDecl SGTM
☔ The latest upstream changes (presumably #39712) made this pull request unmergeable. Please resolve the merge conflicts.
☔ The latest upstream changes (presumably #39572) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb would you mind Cratering this?
☔ The latest upstream changes (presumably #39752) made this pull request unmergeable. Please resolve the merge conflicts.
The crater report shows 4 main regressions:
- brotli (failure)
- chipmunk (failure) - this one looks like a bug in
chipmunkitself - clap (failure) - this is very bad, as
claphas 454 other packages depending on it - multipart (failure)
These all look like macro-defining macros, and they seem like they should work, but something is perhaps doing too-eager parsing? I've never liked $foo:bar (or even just $foo) being a single token, it shouldn't.
It looks like all these errors are due to fixing #39404. That is, they are legitimate errors in the macro definition that didn't show up before because the erroneous pattern never ended up getting used to match anything. I'll start a warning cycle.
I've never liked
$foo:bar(or even just$foo) being a single token, it shouldn't.
Agreed, that's one of the reasons for this PR. After this PR, $foo:bar is never a single token (it used to be Token::Interpolated(Nonterminal::MatchNt(..)) sometimes; this PR removes Nonterminal::MatchNt).
Also, $foo is never a single token in a tokenstream::TokenTree after this PR, except on certain error paths (planning on removing Nonterminal::SubstNt later after refactoring the relevant errors).
Is it really #39404? It looked like the lone The fallout looks pretty bad if it's their bug, multiple patch releases may be needed.$foo was supposed to be interpolated from an outer macro, or am I imagining things?
EDIT: No, you're right 😞
Okay, this might not be that bad. clap is affected in 1.* and 2.* and since these are post-0.*, one release for each (of 1.* and 2.*) should be enough to fix most (ideally, all) of the regressions.
I've opened clap-rs/clap#857 and we can do another crater run once that's released.
📌 Commit 839398a has been approved by nrc
⌛ Testing commit 839398a with merge d35bcfa...
frewsxcv added a commit to frewsxcv/rust that referenced this pull request
Simplify TokenTree and fix macro_rules! bugs
This PR
- fixes rust-lang#39390, fixes rust-lang#39403, and fixes rust-lang#39404 (each is a [breaking-change], see issues for examples),
- fixes rust-lang#39889,
- simplifies and optimizes macro invocation parsing,
- cleans up
ext::tt::transcribe, - removes
tokenstream::TokenTree::SequenceandToken::MatchNt,- instead, adds a new type
ext::tt::quoted::TokenTreefor use bymacro_rules!(ext::tt)
- instead, adds a new type
- removes
parser.quote_depthandparser.parsing_token_tree, and - removes
quote_matcher!.- Instead, use
quote_tokens!andext::tt::quoted::parsethe result withexpect_matchers=true. - I found no outside uses of
quote_matcher!when searching Rust code on Github.
- Instead, use
r? @nrc
bors added a commit that referenced this pull request
bors added a commit that referenced this pull request
Simplify TokenTree and fix macro_rules! bugs
This PR
- fixes #39390, fixes #39403, and fixes #39404 (each is a [breaking-change], see issues for examples),
- fixes #39889,
- simplifies and optimizes macro invocation parsing,
- cleans up
ext::tt::transcribe, - removes
tokenstream::TokenTree::SequenceandToken::MatchNt,- instead, adds a new type
ext::tt::quoted::TokenTreefor use bymacro_rules!(ext::tt)
- instead, adds a new type
- removes
parser.quote_depthandparser.parsing_token_tree, and - removes
quote_matcher!.- Instead, use
quote_tokens!andext::tt::quoted::parsethe result withexpect_matchers=true. - I found no outside uses of
quote_matcher!when searching Rust code on Github.
- Instead, use
r? @nrc
bors added a commit that referenced this pull request
This was referenced
Mar 1, 2017
harpocrates added a commit to harpocrates/language-rust that referenced this pull request
Updated AST as per <rust-lang/rust#39419>, thereby closing #19.
Since those changes involved removing MathcNt and SubstNt, I've removed the unsafe parts of Quote that relied on that. Quote is now a safe module (where '$x' and '$x:ty' don't work), enabled by default.
Difference tests now work on 'ast.rs' as well as 'parser.rs'. Additional work was done on cleaning up tests around macros and tokens. Closes #16.
Labels
Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..)
Status: Waiting on a crater run to be completed.