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

macros: Make metavariables hygienic #35453

Merged
merged 4 commits into from
Aug 14, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Aug 7, 2016

This PR makes metavariables hygienic. For example, consider:

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,

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

@jseyfried
Copy link
Contributor Author

jseyfried commented Aug 7, 2016

cc @durka @eddyb

@nrc
Copy link
Member

nrc commented Aug 8, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 8, 2016

📌 Commit cdbfe9f has been approved by nrc

@bors
Copy link
Contributor

bors commented Aug 8, 2016

⌛ Testing commit cdbfe9f with merge 26f17ba...

@bors
Copy link
Contributor

bors commented Aug 8, 2016

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

@durka
Copy link
Contributor

durka commented Aug 8, 2016

LLVM didn't build?

@alexcrichton
Copy link
Member

@bors: retry

  • not sure...

@bors
Copy link
Contributor

bors commented Aug 10, 2016

⌛ Testing commit cdbfe9f with merge 70bb60c...

@bors
Copy link
Contributor

bors commented Aug 10, 2016

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

@durka
Copy link
Contributor

durka commented Aug 10, 2016

 error[E0308]: mismatched types
   --> C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\run-pass-fulldeps\auxiliary\procedural_mbe_matching.rs:39:22
    |
 39 |             match (&*map[&str_to_ident("matched").name], &*map[&str_to_ident("pat").name]) {
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `syntax::ast::Ident`, found struct `syntax::ast::Name`
    |
    = note: expected type `&syntax::ast::Ident`
    = note:    found type `&syntax::ast::Name`

 error[E0308]: mismatched types
   --> C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\run-pass-fulldeps\auxiliary\procedural_mbe_matching.rs:39:60
    |
 39 |             match (&*map[&str_to_ident("matched").name], &*map[&str_to_ident("pat").name]) {
    |                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `syntax::ast::Ident`, found struct `syntax::ast::Name`
    |
    = note: expected type `&syntax::ast::Ident`
    = note:    found type `&syntax::ast::Name`

 error: aborting due to 2 previous errors

@jseyfried
Copy link
Contributor Author

@durka Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 12, 2016

📌 Commit b62ff25 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Aug 12, 2016

📌 Commit b62ff25 has been approved by nrc

@jseyfried jseyfried force-pushed the hygienize_metavariables branch from b62ff25 to 95b68aa Compare August 12, 2016 07:21
@bors
Copy link
Contributor

bors commented Aug 12, 2016

📌 Commit 95b68aa has been approved by nrc

@durka
Copy link
Contributor

durka commented Aug 12, 2016

So... this is a syntax-breaking change if there is test fallout (sorry).

@eddyb
Copy link
Member

eddyb commented Aug 12, 2016

cc @Manishearth

@jseyfried
Copy link
Contributor Author

@durka Good point.
@bors r- (unless @Manishearth's says this isn't actually a syntax-[breaking-change] in practice)

@Manishearth
Copy link
Member

Manishearth commented Aug 13, 2016

I think so. @dtolnay whatsay?

@Manishearth
Copy link
Member

We're gettign a pile of PRs that will break aster (and only aster) right now though. So it might be good to do a rollup. OTOH there's a bunch of diagnostics work coming up which I'd like to include.

@dtolnay
Copy link
Member

dtolnay commented Aug 13, 2016

This PR does not affect quasi/aster. For the others - I have time in the next two weeks to deal with fallout so this would be a good time. Thanks for the heads up.

@Manishearth
Copy link
Member

Cool. @GuillaumeGomez could you get all libsyntax refactors necessary for diagnostics in now?

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Aug 13, 2016

📌 Commit 95b68aa has been approved by nrc

@bors
Copy link
Contributor

bors commented Aug 14, 2016

⌛ Testing commit 95b68aa with merge f6ba0ad...

@alexcrichton
Copy link
Member

@bors: retry force clean

  • restarted buildbot

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
@bors
Copy link
Contributor

bors commented Aug 14, 2016

⌛ Testing commit 95b68aa with merge eec30ea...

@bors bors merged commit 95b68aa into rust-lang:master Aug 14, 2016
@jseyfried jseyfried deleted the hygienize_metavariables branch September 12, 2016 22:09
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.

8 participants