Apply attr proc macros before cfg processing #44528
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jseyfried (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Thanks for the PR @tmnilsson, we'll check in now and again to make sure @jseyfried (or another reviewer) gets to this soon! |
alexcrichton
added
the
S-waiting-on-review
|
review ping @jseyfried! pinging you on IRC too.... or not because either I don't know your nick or you're not online :) sneaky! |
src/libsyntax/ext/expand.rs
Outdated
| let item = if !attr.is_some() { | ||
| Annotatable::Item(fully_configure!(self, item, noop_fold_item)) | ||
| } | ||
| else { |
There was a problem hiding this comment.
nit: } else { is more idiomatic
src/libsyntax/ext/expand.rs
Outdated
| @@ -867,7 +867,12 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { | |||
|
|
|||
| let (attr, traits, mut item) = self.classify_item(item); | |||
| if attr.is_some() || !traits.is_empty() { | |||
| let item = Annotatable::Item(fully_configure!(self, item, noop_fold_item)); | |||
| let item = if !attr.is_some() { | |||
There was a problem hiding this comment.
nit: !attr.is_some() -> attr.is_none()
|
@tmnilsson looks good so far! Could you test that #[proc_macro_derive(Foo, attributes(foo))]
fn derive_foo(input: TokenStream) -> TokenStream { input }The issue here is that when collecting If we learn that I believe you'll need to |
arielb1
added
S-waiting-on-author
|
Thanks for the feedback! That example would not get fully configured, as you expected. I wasn't even aware of inert attributes, actually. Anyway, I moved the full configure as you suggested, and that seems to make it work. |
src/libsyntax/ext/expand.rs
Outdated
| Annotatable::Item(cfg.fold_item(item).pop().unwrap()) | ||
| } | ||
| Annotatable::TraitItem(item) => { | ||
| Annotatable::TraitItem(P(cfg.fold_trait_item(item.unwrap()).pop().unwrap())) |
There was a problem hiding this comment.
nit: item.map(|item| cfg.fold_trait_item(item).pop().unwrap()) is more idiomatic and efficient here (i.e. with P). Same for ImplItem.
|
@tmnilsson Excellent, thanks! Also FYI we usually rebase PR branch over newer master and force push instead of merging master into PR branch. |
Now items are not fully configured until right before expanding derives.
tmnilsson
force-pushed
the
attr_proc_macro_cfg_process
branch
from
9998b2d
to
0f97b6b
Compare
|
@bors r+ |
|
📌 Commit 0f97b6b has been approved by |
…ried Apply attr proc macros before cfg processing Fixes #39336. r? @jseyfried
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #39336.
r? @jseyfried