Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested macro_rules! #34925

Merged
merged 2 commits into from
Jul 23, 2016
Merged

Support nested macro_rules! #34925

merged 2 commits into from
Jul 23, 2016

Conversation

jseyfried
Copy link
Contributor

Fixes #6994.
r? @eddyb

@@ -2663,7 +2665,7 @@ impl<'a> Parser<'a> {
}

pub fn check_unknown_macro_variable(&mut self) {
if self.quote_depth == 0 {
if self.quote_depth == 0 && !self.parsing_token_tree {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe quote_depth and parsing_token_tree have the exact same intended function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried just using quote_depth and it caused other problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, it caused problems with parsing the left hand side of a macro_rules! body. I didn't look into it in too much detail since I wanted to do this PR as simply as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove quote_depth then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so -- looking into it some more, the problem is that this condition needs to be false when parsing the lhs of a macro_rules! body.

Copy link
Member

@eddyb eddyb Jul 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always false because quote_depth is never anything other than 0.
EDIT: Oops, the modifications to quote_depth are in another module.

IMO, the whole distinction between $ ident (two tokens) and $ident (one token) is unnecessary and only creates complications.

Copy link
Contributor Author

@jseyfried jseyfried Jul 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed (out of the scope of this PR, though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed quote_depth and parsing_token_tree in #39419.

@jseyfried
Copy link
Contributor Author

cc @nrc

@eddyb
Copy link
Member

eddyb commented Jul 19, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 485e2df has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 20, 2016

⌛ Testing commit 485e2df with merge c3f1268...

@bors
Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 19, 2016 at 9:40 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1829


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34925 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95KCA_62xrH-TYQPIZjuTtGtzKDVJks5qXaa7gaJpZM4JQIp2
.

@bors
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit 485e2df with merge f83586d...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Jul 20, 2016 at 5:58 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1193


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34925 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95EzsjrA1E_e_PDW8RxbmHIkBnKV7ks5qXsQpgaJpZM4JQIp2
.

@bors
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit 485e2df with merge d62bb9d...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

sorry for the number of retries...

On Thu, Jul 21, 2016 at 12:37 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5022


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34925 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95FnwT1qmK2TntBL0juXRPERqL9_cks5qX8pugaJpZM4JQIp2
.

@bors
Copy link
Contributor

bors commented Jul 22, 2016

⌛ Testing commit 485e2df with merge 8445fd4...

@bors
Copy link
Contributor

bors commented Jul 22, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Jul 22, 2016 at 4:13 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5040


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34925 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95J5GyjVxyIgGDnJFdVeodLF5V5gDks5qYU60gaJpZM4JQIp2
.

@bors
Copy link
Contributor

bors commented Jul 23, 2016

⌛ Testing commit 485e2df with merge fd1d360...

bors added a commit that referenced this pull request Jul 23, 2016
Support nested `macro_rules!`

Fixes #6994.
r? @eddyb
@bors bors merged commit 485e2df into rust-lang:master Jul 23, 2016
@durka
Copy link
Contributor

durka commented Aug 3, 2016

Amazing.

This is kinda funny though...

macro_rules! outer {
    ($b:tt $c:ident) => {
        macro_rules! inner {
            ($a $b $c) => { }
        }
    }
}
outer!(: ident); // NB: must be `:`
inner!(foo);

@durka
Copy link
Contributor

durka commented Aug 3, 2016

This has added a deadlock:

macro_rules! outer {
    ($b:tt) => {
        macro_rules! inner {
            ($b) => { }
        }
    }
}

outer!($a); // OK
outer!($b); // hangs the compiler

@jseyfried
Copy link
Contributor Author

jseyfried commented Aug 3, 2016

@durka Thanks, I'll look into the NOTE and the deadlock tomorrow.

@durka
Copy link
Contributor

durka commented Aug 3, 2016

It's not a compiler NOTE, I meant if you don't pass : into the outer macro, then the inner one is malformed.

@jseyfried jseyfried deleted the nested_macros branch August 3, 2016 23:20
@eddyb
Copy link
Member

eddyb commented Aug 3, 2016

@durka I think that has to do with how we parse $x:y (it's a whole token right now, sadly).

@durka
Copy link
Contributor

durka commented Aug 3, 2016

It can't be in all cases, since I passed in the $x : y as separate tokens.

@jseyfried
Copy link
Contributor Author

I fixed the deadlock in #35453.

bors added a commit that referenced this pull request Aug 14, 2016
macros: Make metavariables hygienic

This PR makes metavariables hygienic. For example, consider:
```rust
macro_rules! foo {
    ($x:tt) => { // Suppose that this token tree argument is always a metavariable.
        macro_rules! bar { ($x:expr, $y:expr) => { ($x, $y) } }
    }
}

fn main() {
    foo!($z); // This currently compiles.
    foo!($y); // This is an error today but compiles after this PR.
}
```
Today, the `macro_rules! bar { ... }` definition is only valid when the metavariable passed to `foo` is not `$y` (since it unhygienically conflicts with the `$y` in the definition of `bar`) or `$x` (c.f. #35450).

After this PR, the definition of `bar` is always valid (and `bar!(a, b)` always expands to `(a, b)` as expected).

This can break code that was allowed in #34925 (landed two weeks ago). For example,
```rust
macro_rules! outer {
    ($t:tt) => {
        macro_rules! inner { ($i:item) => { $t } }
    }
}

outer!($i); // This `$i` should not interact with the `$i` in the definition of `inner!`.
inner!(fn main() {}); // After this PR, this is an error ("unknown macro variable `i`").
```

Due to the severe limitations on nested `macro_rules!` before #34925, this is not a breaking change for stable/beta.

Fixes #35450.

r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants