Expand macros in extern {} blocks
#49350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@petrochenkov Feel free to assign another reviewer if you have too much on your plate. |
petrochenkov
added
the
S-waiting-on-review
| @@ -6802,7 +6802,7 @@ impl<'a> Parser<'a> { | |||
| } | |||
|
|
|||
| /// Parse a foreign item. | |||
| fn parse_foreign_item(&mut self) -> PResult<'a, Option<ForeignItem>> { | |||
| pub fn parse_foreign_item(&mut self) -> PResult<'a, Option<ForeignItem>> { | |||
There was a problem hiding this comment.
Parser support for macros in foreign blocks is missing (the disambiguation is a bit tricky, but the approach can be copied from macros in traits/impls as well).
There was a problem hiding this comment.
I had covered this already but I lost a bunch of work somewhere.
src/libsyntax/feature_gate.rs
Outdated
| @@ -1599,6 +1599,9 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> { | |||
| gate_feature_post!(&self, extern_types, i.span, | |||
| "extern types are experimental"); | |||
| } | |||
| ast::ForeignItemKind::Macro(..) => { | |||
| // FIXME: do we allow macro invocs in `extern {}`? | |||
There was a problem hiding this comment.
Yes, this should be feature gated.
There was a problem hiding this comment.
Yeah, the feature gate is checked in the expansion code since this visitor is post-expansion.
There was a problem hiding this comment.
Thought it was, at least. Must have forgotten to push some changes from my laptop.
|
LGTM, but tests are missing and some existing tests fail. |
petrochenkov
added
S-waiting-on-author
|
Minimal set of tests: invocation of |
|
@petrochenkov I'm wondering the best way to add items to an Should I add a second crate that exports some symbols to link to, maybe? Or do you think this would be a reason to wait for #49321? |
Refer to something from |
|
I've got the compile-fail tests passing but I'm getting an ICE on the run-pass. Apparently there's a macro invoc making it through expansion, though it strangely has an empty path and invocation: I think it's getting parsed that way but I'm not sure. I need to rebuild in debug mode so there's not a load of intermediates optimized out. |
|
The However, expansion seems to work; I'm seeing two |
|
Okay, someone should leave a note somewhere to make sure to override the relevant |
|
@petrochenkov there's the prototype, I need to implement a UI feature-gate test still. And I should open a tracking issue, yeah? |
|
Added a UI test but the spans are wrong, I'll work on that. |
Yes, it'll need to be created sooner or later so it's better to do it now. |
|
@petrochenkov should be ready for review now. I'll squash afterwards. |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
abonander
force-pushed
the
macros-in-extern
branch
from
7d72d17
to
629b2f7
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
@petrochenkov the first two errors are parse breakages, but the last two changed error messages seem to be significant improvements. Thoughts? |
|
@TimNN there's some irrelevant information in that last log, and missing errors from two other tests. However, I don't need the bot in this thread as I'm checking the build at every opportunity. |
(I'll look at this PR tomorrow.) |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
r=me after squashing and removing |
|
@bors r+ |
|
📌 Commit 5be72bd has been approved by |
bors
added
S-waiting-on-bors
|
🔒 Merge conflict |
bors
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #49124) made this pull request unmergeable. Please resolve the merge conflicts. |
abonander
force-pushed
the
macros-in-extern
branch
from
5be72bd
to
38e4c77
Compare
|
@petrochenkov conflicts resolved |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
abonander
commented
Apr 3, 2018
| use test_macros::{nop_attr, no_output, emit_input}; | ||
|
|
||
| fn main() { | ||
| #[no_output] |
There was a problem hiding this comment.
@petrochenkov possible regression introduced by #49124? This invocation is being feature-gated by stmt_expr_attributes but I don't think it should be.
There was a problem hiding this comment.
Hmm, I thought the idea was to additionally gate macro expansions on statements/expressions with stmt_expr_attributes (in this case this error is correct).
I think we should pretend this is a "feature, not a bug" for now, land this PR and then possibly tweak feature gates so that #49124 becomes gated only by proc_macro.
There was a problem hiding this comment.
I think I saw Serde relying on this, but I realize now it had to have been using #[cfg] which has been allowed and is probably being tested elsewhere already. So it's not an issue for now.
abonander
force-pushed
the
macros-in-extern
branch
from
38e4c77
to
5d74990
Compare
|
@bors r+ |
|
📌 Commit 5d74990 has been approved by |
bors
added
S-waiting-on-bors
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
Apr 5, 2018
…chenkov
Expand macros in `extern {}` blocks
This permits macro and proc-macro and attribute invocations (the latter only with the `proc_macro` feature of course) in `extern {}` blocks, gated behind a new `macros_in_extern` feature.
A tracking issue is now open at rust-lang#49476
closes rust-lang#48747
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Apr 30, 2018
ExpansionKind::ForeignItems was added in rust-lang#49350, which is not included in the 1.26 beta.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This permits macro and proc-macro and attribute invocations (the latter only with the
proc_macrofeature of course) inextern {}blocks, gated behind a newmacros_in_externfeature.A tracking issue is now open at #49476
closes #48747