-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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: improve reexports #37463
macros: improve reexports #37463
Conversation
670dde0
to
6a83458
Compare
cc @eddyb @alexcrichton |
6a83458
to
70eca75
Compare
name: macro_def.name, | ||
attrs: macro_def.attrs.to_vec(), | ||
span: macro_def.span, | ||
body: ::syntax::print::pprust::tts_to_string(¯o_def.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just encode the raw tokens - which would get us proper spans from the original source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I'd rather change that in a separate PR.
9399bd2
to
ea80c32
Compare
☔ The latest upstream changes (presumably #37431) made this pull request unmergeable. Please resolve the merge conflicts. |
ea80c32
to
b0babc8
Compare
b3bb0a7
to
bac13dc
Compare
if let Def::Macro(..) = exp.def { | ||
} else if macros_only { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be clearer using else if !macros_only { callback(exp) }
and/or using a match rather than if let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use a match
.
@@ -533,6 +533,7 @@ impl PatternSource { | |||
pub enum Namespace { | |||
TypeNS, | |||
ValueNS, | |||
MacroNS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to use a qualified name rather than have this NS
suffix on each variant, but not essential to landing
@@ -226,4 +226,8 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> { | |||
fn visit_lifetime(&mut self, lifetime: &'ast Lifetime) { | |||
self.insert(lifetime.id, NodeLifetime(lifetime)); | |||
} | |||
|
|||
fn visit_macro_def(&mut self, macro_def: &'ast MacroDef) { | |||
self.insert_entry(macro_def.id, NotPresent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we record macros in the HIR and start with HIR to make the metadata? I'd have thought the HIR should be totally ignorant of macros and we could generate metadata from the AST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AST is no longer around when we generate metadata.
If we don't want to include exported macros in the HIR, I think the best alternative would be to move the AST's exported macros into a local in driver::compile_input
and pass them directly to the metadata encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see any improvement from that. OTOH, HIR is nicer for incremental recompilation.
☔ The latest upstream changes (presumably #37506) made this pull request unmergeable. Please resolve the merge conflicts. |
ed42dfc
to
57d781d
Compare
@bors r=nrc |
📌 Commit 57d781d has been approved by |
⌛ Testing commit 57d781d with merge 8840d92... |
💔 Test failed - auto-linux-cross-opt |
57d781d
to
f967566
Compare
@bors r=nrc |
📌 Commit f967566 has been approved by |
f967566
to
3d760d7
Compare
that is referenced by multiple `extern crate` items.
3d760d7
to
b2fa1b6
Compare
@bors r=nrc |
📌 Commit b2fa1b6 has been approved by |
⌛ Testing commit b2fa1b6 with merge 8510b2e... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
b2fa1b6
to
a0a9f8c
Compare
@bors r=nrc |
📌 Commit a0a9f8c has been approved by |
macros: improve reexports This PR - avoids building multiple module graphs for a crate that is referenced by multiple `extern crate` items, - registers `#[no_link] extern crate`s to avoid loading the same crate metadata twice, - stability checks `#[no_link] extern crate`s, - [breaking-chage]: `#[no_link] #[macro_use] extern crate syntax;` is allowed on stable today - fixes `$crate` in `#[macro_reexport]`ed macros, - [breaking-change] for `#[feature(macro_reexport)]` (technically) - allows selective macro importing (i.e. `#[macro_use(foo, bar)]`) from custom derive crates, and - refactors the crate metadata to support re-exported macros in arbitrary modules (not yet needed). r? @nrc
rustc_metadata: don't break the version check when CrateRoot changes. In #36551 I made `rustc_version` a field of `CrateRoot`, but despite it being the first field, one could still break the version check by changing `CrateRoot` so older compilers couldn't fully decode it (e.g. #37463). This PR fixes #37803 by moving the version string back at the beginning of metadata, right after the 32-bit big-endian absolute position of `CrateRoot`, and by incrementing `METADATA_VERSION`.
This PR
$crate
in#[macro_reexport]
ed macros,#[feature(macro_reexport)]
(technically)#[no_link] extern crate
s,#[no_link] #[macro_use] extern crate syntax;
is allowed on stable today#[macro_use(foo, bar)]
) from custom derive crates,extern crate
items,#[no_link] extern crate
s to avoid loading the same crate metadata twice, andr? @nrc