Skip to content

/rust

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 `#[macro_use]` on macro-expanded crates #34032

Merged
merged 3 commits into from Jun 9, 2016

Conversation

Reviewers
No reviews
Assignees

@nrc nrc

Labels
relnotes
Projects
None yet
Milestone
No milestone
5 participants
@jseyfried @LeoTestard @nrc @bors @alexcrichton
@jseyfried
Copy link
Contributor

jseyfried commented Jun 2, 2016 •
edited

This PR loads macros from #[macro_use] crates during expansion so that

  • macro-expanded #[macro_use] crates work (fixes #33936, fixes #28071), and
  • macros imported from crates have the same scope as macros imported from modules.
    (EDIT: reverted in #34239)

This is a [breaking-change]. For example, this will break:

macro_rules! m {
    () => { #[macro_use(foo)] extern crate core; } //~ ERROR imported macro not found
}
m!();

Also, this will break:

macro_rules! try { () => {} }
#[macro_use] extern crate core; //< This will now "overwrite" the above `try!` ...
// #[macro_use] mod bar { macro_rules! try { ... } } //< ... just like this would ...
fn main() { try!(); } //< ... making this an error

r? @nrc

@rust-highfive rust-highfive assigned nrc Jun 2, 2016

@jseyfried

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jseyfried commented Jun 2, 2016

cc @durka

@jseyfried jseyfried force-pushed the jseyfried:load_macros_in_expansion branch from 1b05450 to 38166c8 Jun 3, 2016

src/libsyntax/ext/base.rs Outdated
@@ -925,4 +940,8 @@ impl SyntaxEnv {
let last_chain_index = self.chain.len() - 1;
&mut self.chain[last_chain_index].info
}

pub fn is_crate_root(&mut self) -> bool {
self.chain.len() == 2

This comment has been minimized.

Sign in to view
Copy link
@LeoTestard

LeoTestard Jun 4, 2016

Contributor

Huh? Why 2? This probably needs a comment. :d

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Jun 5, 2016 •
edited

Author Contributor

Done.
src/libsyntax/ext/expand.rs Outdated
for def in fld.cx.loader.load_crate(&it, allows_macros) {
fld.cx.insert_macro(def);
}
SmallVector::one(Annotatable::Item(it))

This comment has been minimized.

Sign in to view
Copy link
@LeoTestard

LeoTestard Jun 4, 2016

Contributor

Is it normal that new_attrs aren't attached back?

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Jun 5, 2016

Author Contributor

Before this PR, new_attrs wasn't attached back (so the expanded decorator attributes weren't removed) when folding modules, trait items, and impl items, so I don't think it matters much.

That being said, I think the decorator attributes should always be removed since macro expansion should be idempotent, so I added a commit that always reattaches new_attrs.
src/libsyntax/ext/base.rs
@@ -546,6 +557,7 @@ pub struct ExtCtxt<'a> {
pub ecfg: expand::ExpansionConfig<'a>,
pub crate_root: Option<&'static str>,
pub feature_gated_cfgs: &'a mut Vec<GatedCfgAttr>,
pub loader: &'a mut MacroLoader,

This comment has been minimized.

Sign in to view
Copy link
@LeoTestard

LeoTestard Jun 4, 2016

Contributor

It's a sad thing that we have to use a trait and a trait object just for tests. Is there no other way? Can't we just create a dummy macro_import::MacroLoader?

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Jun 5, 2016

Author Contributor

My main reason for using a trait object is that macro_import::MacroLoader is defined and implemented in librustc_metadata, so libsyntax can't use it directly.
@LeoTestard

This comment has been minimized.

Sign in to view
Copy link
Contributor

LeoTestard commented Jun 4, 2016

(Otherwise: 👍 💯)

@jseyfried jseyfried force-pushed the jseyfried:load_macros_in_expansion branch from f953a79 to acbff87 Jun 5, 2016

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Jun 7, 2016

@bors: r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit acbff87 has been approved by nrc
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 8, 2016

🔒 Merge conflict
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 8, 2016

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

@jseyfried jseyfried force-pushed the jseyfried:load_macros_in_expansion branch from acbff87 to dbf0326 Jun 9, 2016

@jseyfried

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jseyfried commented Jun 9, 2016

Rebased. @bors r=nrc
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 9, 2016

📌 Commit dbf0326 has been approved by nrc
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 9, 2016

⌛️ Testing commit dbf0326 with merge dc77c5e...

bors added a commit that referenced this pull request Jun 9, 2016

@bors
Auto merge of #34032 - jseyfried:load_macros_in_expansion, r=nrc 
Support `#[macro_use]` on macro-expanded crates

This PR loads macros from `#[macro_use]` crates during expansion so that
 - macro-expanded `#[macro_use]` crates work (fixes #33936, fixes #28071), and
 - macros imported from crates have the same scope as macros imported from modules.

This is a [breaking-change]. For example, this will break:
```rust
macro_rules! m {
    () => { #[macro_use(foo)] extern crate core; } //~ ERROR imported macro not found
}
m!();
```
Also, this will break:
```rust
macro_rules! try { () => {} }
// #[macro_use] mod bar { macro_rules! try { ... } } //< ... just like this would ...
fn main() { try!(); } //< ... making this an error
```

r? @nrc
dc77c5e
@bors

@bors bors merged commit dbf0326 into rust-lang:master Jun 9, 2016
2 checks passed

2 checks passed

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jseyfried commented Jun 10, 2016

@nrc This may need a relnotes tag (c.f. #33458).

@alexcrichton alexcrichton added the relnotes label Jun 10, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jun 10, 2016

Added

@TimNN TimNN referenced this pull request Jun 11, 2016

Closed

Order of extern crate/pub mod matters suddenly #34212

@jseyfried jseyfried referenced this pull request Jun 12, 2016

Merged

Revert a change in the scope of macros imported from crates to fix a regression #34239

bors added a commit that referenced this pull request Jun 16, 2016

@bors
Auto merge of #34239 - jseyfried:fix_macro_use_scope_regression, r=nrc 
Revert a change in the scope of macros imported from crates to fix a regression

Fixes #34212.
The regression was caused by #34032, which changed the scope of macros imported from extern crates to match the scope of macros imported from modules.
r? @nrc
7aba683

@jseyfried jseyfried deleted the jseyfried:load_macros_in_expansion branch Sep 25, 2016

@jseyfried jseyfried referenced this pull request Sep 27, 2016

Merged

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.