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

Enforce the shadowing restrictions from RFC 1560 for today's macros #36767

Merged
merged 9 commits into from Oct 3, 2016

Conversation

Projects
None yet
5 participants
@jseyfried
Contributor

jseyfried commented Sep 27, 2016

This PR enforces a weakened version of the shadowing restrictions from RFC 1560. More specifically,

  • If a macro expansion contains a macro_rules! macro definition that is used outside of the expansion, the defined macro may not shadow an existing macro.
  • If a macro expansion contains a #[macro_use] extern crate macro import that is used outside of the expansion, the imported macro may not shadow an existing macro.

This is a [breaking-change]. For example,

macro_rules! m { () => {} }
macro_rules! n { () => {
    macro_rules! m { () => {} } //< This shadows an existing macro.
    m!(); //< This is inside the expansion that generated `m`'s definition, so it is OK.
} }
n!();
m!(); //< This use of `m` is outside the expansion, so it causes the shadowing to be an error.

r? @nrc

@jseyfried jseyfried force-pushed the jseyfried:enforce_rfc_1560_shadowing branch from cc0b377 to 987a536 Sep 27, 2016

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Sep 27, 2016

cc #35896, cc @eddyb (this is helpful for def/use orthogonality)
Also, this will need a crater run.

@jseyfried jseyfried force-pushed the jseyfried:enforce_rfc_1560_shadowing branch from 987a536 to 7f49798 Sep 28, 2016

@nrc

This comment has been minimized.

Member

nrc commented Sep 28, 2016

@brson, @nikomatsakis, @eddyb could one of you schedule a crater run please?

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 28, 2016

@nrc Starting now.

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 29, 2016

Crater run shows two regressions (conv and tokei).

@nrc

This comment has been minimized.

Member

nrc commented Sep 29, 2016

Code looks good, but I'm not sure about the Crater regressions. They certainly seem real and could indicate more uses in the wild. It is probably prudent to have a warning cycle for this if it is possible.

@jseyfried jseyfried force-pushed the jseyfried:enforce_rfc_1560_shadowing branch from 7f49798 to 3dd5981 Sep 29, 2016

@durka

This comment has been minimized.

Contributor

durka commented Sep 29, 2016

  • A macro defined by a macro-expanded macro_rules! may not shadow an existing macro.

Can it shadow a macro which was previously defined by a macro?

I use the above trick in static-cond. If it's not permitted, you can only invoke static_cond! once per crate, and there's no way to create unique identifiers, so... this sucks.

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Sep 29, 2016

@durka Yeah, looks like this PR and unhygienic macro names are a bad combination.

I wonder if it would be backwards compatible in practice to make macro names hygienic (EDIT: it isn't). That would avoid breaking static-cond as well as the two Crater-detected crates.

If we can't make macro names hygienic, we could instead only disallow macro-expanded shadowing if the shadowing macro is used outside of the expansion. This would also avoid breaking static-cond and the two Crater-detected crates, and it wouldn't compromise name resolution in any way.

@jseyfried jseyfried force-pushed the jseyfried:enforce_rfc_1560_shadowing branch from 3dd5981 to 057302b Oct 2, 2016

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Oct 2, 2016

I weakened the shadowing restrictions to avoid breakage in practice (see the edited initial comment).
This PR no longer breaks static-cond or the two crates from the Crater run.

@nrc

This comment has been minimized.

Member

nrc commented Oct 2, 2016

Code looks good, I'd like to get another Crater run before we land though. @eddyb, @brson, @nikomatsakis could someone oblige please?

@eddyb

This comment has been minimized.

Member

eddyb commented Oct 3, 2016

On it!

@eddyb

This comment has been minimized.

Member

eddyb commented Oct 3, 2016

Crater report looks good (the unrelated breakage is rust-num/num#231, I reused the "before" results).

@nrc

This comment has been minimized.

Member

nrc commented Oct 3, 2016

Thanks @eddyb !

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Oct 3, 2016

📌 Commit 057302b has been approved by nrc

@bors

This comment has been minimized.

Contributor

bors commented Oct 3, 2016

⌛️ Testing commit 057302b with merge f374565...

bors added a commit that referenced this pull request Oct 3, 2016

Auto merge of #36767 - jseyfried:enforce_rfc_1560_shadowing, r=nrc
Enforce the shadowing restrictions from RFC 1560 for today's macros

This PR enforces a weakened version of the shadowing restrictions from RFC 1560. More specifically,
 - If a macro expansion contains a `macro_rules!` macro definition that is used outside of the expansion, the defined macro may not shadow an existing macro.
 - If a macro expansion contains a `#[macro_use] extern crate` macro import that is used outside of the expansion, the imported macro may not shadow an existing macro.

This is a [breaking-change]. For example,
```rust
macro_rules! m { () => {} }
macro_rules! n { () => {
    macro_rules! m { () => {} } //< This shadows an existing macro.
    m!(); //< This is inside the expansion that generated `m`'s definition, so it is OK.
} }
n!();
m!(); //< This use of `m` is outside the expansion, so it causes the shadowing to be an error.
```

r? @nrc

@bors bors merged commit 057302b into rust-lang:master Oct 3, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@jseyfried jseyfried deleted the jseyfried:enforce_rfc_1560_shadowing branch Oct 3, 2016

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 12, 2016

Rollup merge of rust-lang#37084 - jseyfried:cleanup_expanded_macro_us…
…e_scopes, r=nrc

macros: clean up scopes of expanded `#[macro_use]` imports

This PR changes the scope of macro-expanded `#[macro_use]` imports to match that of unexpanded `#[macro_use]` imports. For example, this would be allowed:
```rust
example!();
macro_rules! m { () => { #[macro_use(example)] extern crate example_crate; } }
m!();
```

This PR also enforces the full shadowing restrictions from RFC 1560 on `#[macro_use]` imports (currently, we only enforce the weakened restrictions from rust-lang#36767).

This is a [breaking-change], but I believe it is highly unlikely to cause breakage in practice.
r? @nrc

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 12, 2016

Rollup merge of rust-lang#37084 - jseyfried:cleanup_expanded_macro_us…
…e_scopes, r=nrc

macros: clean up scopes of expanded `#[macro_use]` imports

This PR changes the scope of macro-expanded `#[macro_use]` imports to match that of unexpanded `#[macro_use]` imports. For example, this would be allowed:
```rust
example!();
macro_rules! m { () => { #[macro_use(example)] extern crate example_crate; } }
m!();
```

This PR also enforces the full shadowing restrictions from RFC 1560 on `#[macro_use]` imports (currently, we only enforce the weakened restrictions from rust-lang#36767).

This is a [breaking-change], but I believe it is highly unlikely to cause breakage in practice.
r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment