-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Do not re-export macros that are already at the crate root #88335
Conversation
This reverts commit d493c1b.
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
MacroNS, | ||
(res, vis, span, expansion, IsMacroExport), | ||
); | ||
} |
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 is incorrect, all macro_export
-ed macro_rules
should be "planted" into the root module (this is what define
does), without that they will remain non-modularized even if the macro_rules
itself is in the root module.
decoder.rs
must also filter away macro_rules
items in each_child_of_item
or at some level above, there's no way around that - macro_rules
items should not appear in the list of module childern from name resolution point of view.
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.
Other users of each_child_of_item
need to be audited on whether they want to see macro_rules
items as children, but fn build_reduced_graph_external
is the one use that certainly should filter them away.
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.
Test case for filtering in build_reduced_graph_external
:
macro m() {}
macro_rules! m { () => () }
If macro_rules
items are not skipped, then this will cause an error during decoding.
If macro_rules
items are not skipped, then paths like other_crate::some_mod::some_exported_macro_rules
will suddenly start working as well.
@@ -1392,7 +1392,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { | |||
// FIXME: Implement actual cross-crate hygiene. | |||
let is_good_import = | |||
binding.is_import() && !binding.is_ambiguity() && !ident.span.from_expansion(); | |||
if is_good_import || binding.is_macro_def() { | |||
if is_good_import || binding.is_macro_export() { |
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 bit of logic is correct - we don't need to represent any macros as reexports, except for the "root reexports" of macro_export
ed macro_rules
.
61122b1
to
fc67af1
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #88371) made this pull request unmergeable. Please resolve the merge conflicts. |
#88019 has been merged, unblocking. |
Triage: Merge conflicts |
I'm going to close this in favor of #91795. |
resolve/metadata: Stop encoding macros as reexports Supersedes rust-lang#88335. r? `@cjgillot`
Follow-up to #88019
Exported macro_rules were unconditionally reexported at the crate root. In the case of macro_rules that were already at the root, both the macro itself and its reexport appear with the same name in the same scope, which confuses the resolver for downstream crates.
This PR skips creating a re-export when the macro was already at the crate root.
r? @petrochenkov