Simplify `TokenTree` and fix `macro_rules!` bugs by jseyfried · Pull Request #39419 · rust-lang/rust

jseyfried · 2017-01-31T06:34:42Z

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::Sequence and Token::MatchNt,
- instead, adds a new type ext::tt::quoted::TokenTree for use by macro_rules! (ext::tt)
removes parser.quote_depth and parser.parsing_token_tree, and
removes quote_matcher!.
- Instead, use quote_tokens! and ext::tt::quoted::parse the result with expect_matchers=true.
- I found no outside uses of quote_matcher! when searching Rust code on Github.

r? @nrc

jseyfried · 2017-01-31T06:37:02Z

This is groundwork for procedural and declarative macros 2.0.
cc #38356 #39412

eddyb · 2017-01-31T07:08:12Z

src/libsyntax/ext/tt/quoted.rs

Can't this be done on the fly? (i.e in parsing not lexing)

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.

nrc

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.

nrc · 2017-02-03T01:08:02Z

src/libsyntax/ext/tt/transcribe.rs

This would benefit from a comment

nrc · 2017-02-03T01:10:35Z

src/libsyntax/ext/tt/transcribe.rs

I would factor this out into a ctor function

nrc · 2017-02-03T01:11:35Z

src/libsyntax/ext/tt/transcribe.rs

nit: I'd kill the Lis prefix

nrc · 2017-02-03T01:27:22Z

src/libsyntax/ext/tt/quoted.rs

Why make this module here? I'd have thought TTs were much more general than just quasi quoting

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.

nrc · 2017-02-07T03:07:49Z

Oh, GH didn't lose my earlier comments. Good.

nrc

Just one more comment, and that can be done later

nrc · 2017-02-07T03:37:07Z

src/libsyntax/ext/tt/quoted.rs

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.

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?

MetaVarDecl SGTM

bors · 2017-02-10T07:31:31Z

☔ The latest upstream changes (presumably #39712) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2017-02-13T01:59:32Z

☔ The latest upstream changes (presumably #39572) made this pull request unmergeable. Please resolve the merge conflicts.

jseyfried · 2017-02-16T23:51:31Z

@eddyb would you mind Cratering this?

bors · 2017-02-17T06:50:47Z

☔ The latest upstream changes (presumably #39752) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2017-02-17T08:21:34Z

@jseyfried Can you ping me after a rebase?

jseyfried · 2017-02-17T08:30:36Z

@eddyb rebased.

eddyb · 2017-02-17T22:18:19Z

The crater report shows 4 main regressions:

brotli (failure)
chipmunk (failure) - this one looks like a bug in chipmunk itself
clap (failure) - this is very bad, as clap has 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.

jseyfried · 2017-02-18T03:08:12Z

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

eddyb · 2017-02-18T09:14:18Z

~~Is it really #39404? It looked like the lone $foo was supposed to be interpolated from an outer macro, or am I imagining things?~~ The fallout looks pretty bad if it's their bug, multiple patch releases may be needed.

EDIT: No, you're right 😞

eddyb · 2017-02-18T10:07:47Z

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.

jseyfried · 2017-02-28T22:18:46Z

@bors r=nrc

bors · 2017-02-28T22:18:47Z

📌 Commit 839398a has been approved by nrc

bors · 2017-02-28T22:19:48Z

⌛ Testing commit 839398a with merge d35bcfa...

bors · 2017-02-28T22:21:30Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-28T22:30:23Z

@bors: retry * network error

…

On Tue, Feb 28, 2017 at 4:21 PM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2168> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#39419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95GB7SjuZ5IPQajxpJNAzk0PKgfWqks5rhJ3sgaJpZM4LySVU> .

@nrc

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::Sequence` and `Token::MatchNt`, - instead, adds a new type `ext::tt::quoted::TokenTree` for use by `macro_rules!` (`ext::tt`) - removes `parser.quote_depth` and `parser.parsing_token_tree`, and - removes `quote_matcher!`. - Instead, use `quote_tokens!` and `ext::tt::quoted::parse` the result with `expect_matchers=true`. - I found no outside uses of `quote_matcher!` when searching Rust code on Github. r? @nrc

Rollup of 6 pull requests - Successful merges: #39419, #39936, #39944, #39960, #40028, #40128 - Failed merges:

bors · 2017-03-01T05:58:13Z

⌛ Testing commit 839398a with merge 7ce1fbe...

@nrc

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::Sequence` and `Token::MatchNt`, - instead, adds a new type `ext::tt::quoted::TokenTree` for use by `macro_rules!` (`ext::tt`) - removes `parser.quote_depth` and `parser.parsing_token_tree`, and - removes `quote_matcher!`. - Instead, use `quote_tokens!` and `ext::tt::quoted::parse` the result with `expect_matchers=true`. - I found no outside uses of `quote_matcher!` when searching Rust code on Github. r? @nrc

bors · 2017-03-01T07:57:04Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 7ce1fbe to master...

Rollup of 6 pull requests - Successful merges: #39419, #39936, #39944, #39960, #40028, #40128 - Failed merges:

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.

rust-highfive assigned nrc Jan 31, 2017

jseyfried mentioned this pull request Jan 31, 2017

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks

jseyfried mentioned this pull request Jan 31, 2017

Support nested macro_rules! #34925

Merged

jseyfried added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 31, 2017

eddyb reviewed Jan 31, 2017

View reviewed changes

jseyfried force-pushed the simplify_tokentree branch 2 times, most recently from ca90c32 to 04b8382 Compare January 31, 2017 08:41

nrc reviewed Feb 3, 2017

View reviewed changes

nrc approved these changes Feb 7, 2017

View reviewed changes

jseyfried mentioned this pull request Feb 9, 2017

parser: fix ICE when parsing token trees after an error #39674

Merged

jseyfried mentioned this pull request Feb 11, 2017

ICE with macro_rules! in block #39709

Closed

jseyfried force-pushed the simplify_tokentree branch from a4e7704 to ea1f84c Compare February 12, 2017 04:18

jseyfried force-pushed the simplify_tokentree branch 3 times, most recently from 67be452 to 88716e7 Compare February 16, 2017 12:38

jseyfried force-pushed the simplify_tokentree branch from 88716e7 to f46907c Compare February 17, 2017 08:30

eddyb mentioned this pull request Feb 18, 2017

Add missing fragment specifier to a clap_app! rule. clap-rs/clap#857

Merged

jseyfried force-pushed the simplify_tokentree branch from b6e60f9 to 839398a Compare February 28, 2017 22:15

frewsxcv mentioned this pull request Mar 1, 2017

Rollup of 6 pull requests #40167

Merged

bors added a commit that referenced this pull request Mar 1, 2017

Auto merge of #40167 - frewsxcv:rollup, r=frewsxcv

88f7016

Rollup of 6 pull requests - Successful merges: #39419, #39936, #39944, #39960, #40028, #40128 - Failed merges:

bors added a commit that referenced this pull request Mar 1, 2017

Auto merge of #40167 - frewsxcv:rollup, r=frewsxcv

b671c32

Rollup of 6 pull requests - Successful merges: #39419, #39936, #39944, #39960, #40028, #40128 - Failed merges:

bors merged commit 839398a into rust-lang:master Mar 1, 2017

This was referenced Mar 1, 2017

Refactor parsing of trait object types #40043

Merged

Better tokentree pretty printer #39079

Closed

dtolnay mentioned this pull request Mar 2, 2017

No syntax variables matched as repeating at this depth dtolnay/proc-macro-hack#1

Closed

sanmai-NL mentioned this pull request Mar 2, 2017

maud_macros build breaks with rustc 1.17.0-nightly (691eba135 2017-03-01) lambda-fairy/maud#77

Closed

golddranks mentioned this pull request Mar 9, 2017

Regression in binary_macros-0.6.3, Rust 1.16 #40381

Closed

jseyfried deleted the simplify_tokentree branch March 9, 2017 06:28

jseyfried mentioned this pull request Mar 10, 2017

[beta] macro: fix issue with sequence repetitions on beta #40411

Closed

aidanhs mentioned this pull request Mar 11, 2017

Update to nightly 2017-03-03 SkylerLipthay/interpolate_idents#17

Merged

harpocrates mentioned this pull request Apr 20, 2017

Fix macros harpocrates/language-rust#19

Closed

jseyfried mentioned this pull request May 8, 2017

macro_rules attempts to expand tokens it needs to only pass through. #28415

Closed

jseyfried mentioned this pull request May 18, 2017

Rust 1.17 regression - chipmunk-0.0.2, invalid fragment specifier #41795

Closed

jseyfried mentioned this pull request Jul 18, 2017

Tracking issue for future-incompatibility lint missing_fragment_specifier #40107

Closed

ehuss mentioned this pull request Jul 29, 2020

Metavariables can be keywords rust-lang/reference#863

Closed

ehuss mentioned this pull request Jun 12, 2022

mbe matchers can contain dollar ($) rust-lang/reference#1229

Open

Uh oh!

Conversation

jseyfried commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseyfried commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

nrc Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Feb 7, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

nrc Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

jseyfried Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

bors commented Feb 10, 2017

Uh oh!

bors commented Feb 13, 2017

Uh oh!

jseyfried commented Feb 16, 2017

Uh oh!

bors commented Feb 17, 2017

Uh oh!

eddyb commented Feb 17, 2017

Uh oh!

jseyfried commented Feb 17, 2017

Uh oh!

eddyb commented Feb 17, 2017 • edited by jseyfried Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseyfried commented Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jseyfried commented Jan 31, 2017 •

edited

Loading

jseyfried commented Jan 31, 2017 •

edited

Loading

jseyfried Jan 31, 2017 •

edited

Loading

jseyfried Feb 3, 2017 •

edited

Loading

jseyfried Feb 3, 2017 •

edited

Loading

jseyfried Feb 3, 2017 •

edited

Loading

jseyfried Feb 3, 2017 •

edited

Loading

jseyfried Feb 7, 2017 •

edited

Loading

eddyb commented Feb 17, 2017 •

edited by jseyfried

Loading

jseyfried commented Feb 18, 2017 •

edited

Loading

eddyb commented Feb 18, 2017 •

edited

Loading