Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement `#[macro_export(local_inner_macros)]` (a solution for the macro helper import problem) #51496
Conversation
rust-highfive
assigned
pnkfelix
Jun 11, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
Jun 11, 2018
This comment has been minimized.
This comment has been minimized.
|
cc @dtolnay @rust-lang/lang |
rust-highfive
assigned
nikomatsakis
and unassigned
pnkfelix
Jun 11, 2018
This comment has been minimized.
This comment has been minimized.
|
There's one subtle detail that affects how we treat the If we have a macro call using a macro variable, like // Library crate
#[macro_export(macro_helper_hack)]
macro_rules! public {
($helper: ident) => {
$helper!();
}
}
// User crate
// `my_helper` is not affected by the macro helper hack (not changed to `$crate::my_helper`) and is resolved in the user crate, not library crate
public!(my_helper);The primary alternative is to determine whether the change is applicable using context of the macro call |
petrochenkov
referenced this pull request
Jun 11, 2018
Closed
Tracking issue for "macro naming and modularisation" (RFC #1561) #35896
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
Currently building locally to play around with this, but I think this does the trick. Grabbing the span from the ident is the right behavior (as opposed to from the macro call). |
petrochenkov
reviewed
Jun 11, 2018
| if attr.check_name("macro_export") { | ||
| if let Some(content) = attr.meta_item_list() { | ||
| if attr::list_contains_name(&content, "macro_helper_hack") { | ||
| gate_feature_post!(&self, macro_helper_hack, attr.span, |
This comment has been minimized.
This comment has been minimized.
petrochenkov
Jun 11, 2018
Author
Contributor
Well, this must be a mistake, the attribute shouldn't be gated at the library side, but rather the effect should be enabled if feature(use_extern_macros) is enabled on the use site or stabilized.
This comment has been minimized.
This comment has been minimized.
dtolnay
Jun 11, 2018
Member
I was about to write the same thing. This should never happen:
error[E0658]: `macro_helper_hack` in `macro_export` is experimental (see issue #35896)
--> serde-json/src/macros.rs:69:1
|
69 | #[macro_export(macro_helper_hack)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add #![feature(macro_helper_hack)] to the crate attributes to enable
If not building with use_extern_macros, the macro_helper_hack should have no effect and should be ignored as it has been on all past stable compilers.
This comment has been minimized.
This comment has been minimized.
|
Overall I found that this PR works incredibly well. I tested it against some of my nastiest macro code and it behaved as desired every time, with nom-style passing of macro names across crates and everything. The one exception I found was it seems #![feature(use_extern_macros, macro_helper_hack)]
#[macro_export(macro_helper_hack)]
macro_rules! noop {
() => {
noop_helper!{}
};
}
#[doc(hidden)]
#[macro_export]
macro_rules! noop_helper {
() => {};
}
pub fn call() {
noop!{}
}
|
This comment has been minimized.
This comment has been minimized.
|
Ah, it appears the problem is not with this PR. The same thing happens invoking a macro through $crate just with #![feature(use_extern_macros)]
#[macro_export]
macro_rules! noop {
() => {
$crate::noop_helper!{}
};
}
#[doc(hidden)]
#[macro_export]
macro_rules! noop_helper {
() => {};
}
pub fn call() {
noop!{}
}
|
This comment has been minimized.
This comment has been minimized.
|
I guess this is a consequence of #35896 (comment).
Seems easy to work around in any case, and uncommon anyway. |
This comment has been minimized.
This comment has been minimized.
|
Yes, With duplicate macro_rules! foo { () => () } // (1)
mod m {
// maybe somewhere in deeply nested modules
#[macro_export] macro_rules! foo { () => () } (2)
}
macro_rules! foo { () => () } // (3), shadows (1) for the rest of the module
$crate::foo!(); // refers to (2), like from other crates, not to (1) or (3)This doesn't seem urgent and blocking |
This comment has been minimized.
This comment has been minimized.
|
Once |
petrochenkov
force-pushed the
petrochenkov:mhelper2
branch
from
26d10e8
to
81287c7
Jun 16, 2018
petrochenkov
changed the title
[Do not merge] Implement #[macro_export(macro_helper_hack)]
Implement `#[macro_export(local_inner_macros)]` (a solution for the macro helper import problem)
Jun 16, 2018
This comment has been minimized.
This comment has been minimized.
|
Updated.
Bikeshedding about the attribute name is welcome. |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov sorry for being slow to provide feedback. Mozilla All Hands was all consuming. I'll take a look and also skim the comments, but before that I wanted to say thanks for trying this out! |
petrochenkov
added
T-lang
S-waiting-on-team
and removed
S-waiting-on-review
labels
Jun 20, 2018
This comment has been minimized.
This comment has been minimized.
|
Let's notify people formally, there's a number of other things that should ideally get into 1.29 and are blocked on this PR. @rfcbot fcp merge This PR implements a solution for the macro helper issue described by dtolnay in #35896 (comment). Macros exported from libraries can be marked with If we have a fn-like macro call |
petrochenkov
added
the
I-nominated
label
Jun 20, 2018
This comment has been minimized.
This comment has been minimized.
|
@rfcbot still doesn't listen to me |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov This issue is tagged T-lang, so only lang team members can start FCP. I'll do it for you. @rfcbot fcp merge |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Jun 20, 2018
•
|
Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams: Concerns:
Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
removed
the
proposed-final-comment-period
label
Jun 26, 2018
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Jun 26, 2018
|
|
This comment has been minimized.
This comment has been minimized.
|
I checked @pnkfelix's box since they are not available just now, and this is not a stabilization decision anyhow. |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov I think we can go ahead and r+ without waiting for the full FCP, if you want to rebase. |
This comment has been minimized.
This comment has been minimized.
|
(As, again, this is not a final decision, and we could always revert) |
This comment has been minimized.
This comment has been minimized.
|
(Oh, and because time is somewhat of the essence here.) |
petrochenkov
force-pushed the
petrochenkov:mhelper2
branch
from
81287c7
to
d347270
Jun 27, 2018
This comment has been minimized.
This comment has been minimized.
|
@bors r=nikomatsakis |
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-team
labels
Jun 27, 2018
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jun 27, 2018
This comment has been minimized.
This comment has been minimized.
|
|
bors
merged commit d347270
into
rust-lang:master
Jun 27, 2018
rfcbot
added
finished-final-comment-period
and removed
final-comment-period
labels
Jul 6, 2018
This comment has been minimized.
This comment has been minimized.
|
Is there a writeup on the migration strategy for macro heavy crates, and how we can test with recent nightlies anywhere? |
Centril
referenced this pull request
Aug 6, 2018
Closed
(Modules) Tracking issue for `(use) crate_name::` paths without `extern crate` #53128
This comment has been minimized.
This comment has been minimized.
|
@sgrif I don't know of a writeup to point to. The rough summary:
|
This comment has been minimized.
This comment has been minimized.
Or use |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov Thanks, I've edited my comment to take that possibility into account. |
This comment has been minimized.
This comment has been minimized.
|
Do macros which take other macros as callbacks need this as well? |
petrochenkov commentedJun 11, 2018
•
edited
Implement a solution for the macro helper issue discussed in #35896 as described in #35896 (comment).
Macros exported from libraries can be marked with
#[macro_export(local_inner_macros)]and this annotation changes how nested macros in them are resolved.If we have a fn-like macro call
ident!(...)andidentcomes from amacro_rules!macro marked with#[macro_export(local_inner_macros)]then it's replaced with$crate::ident!(...)and resolved as such ($crategets the same context asident).