Skip to content
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

Implement #[proc_macro_attribute] #38842

Merged
merged 4 commits into from
Jan 21, 2017
Merged

Conversation

abonander
Copy link
Contributor

@abonander abonander commented Jan 5, 2017

This implements #[proc_macro_attribute] as described in rust-lang/rfcs#1566

The following major (hopefully non-breaking) changes are included:

  • Refactor proc_macro::TokenStream to use syntax::tokenstream::TokenStream.

    • proc_macro::tokenstream::TokenStream no longer emits newlines between items, this can be trivially restored if desired
    • proc_macro::TokenStream::from_str does not try to parse an item anymore, moved to impl MultiItemModifier for CustomDerive with more informative error message
  • Implement #[proc_macro_attribute], which expects functions of the kind fn(TokenStream, TokenStream) -> TokenStream

    • Reactivated #![feature(proc_macro)] and gated #[proc_macro_attribute] under it
    • #![feature(proc_macro)] and #![feature(custom_attribute)] are mutually exclusive
    • adding #![feature(proc_macro)] makes the expansion pass assume that any attributes that are not built-in, or introduced by existing syntax extensions, are proc-macro attributes
  • Fix feature_gate::find_lang_feature_issue() to not use unwrap()

    • This change wasn't necessary for this PR, but it helped debugging a problem where I was using the wrong feature string.
  • Move "completed feature gate checking" pass to after "name resolution" pass

    • This was necessary for proper feature-gating of #[proc_macro_attribute] invocations when the proc_macro feature flag isn't set.

Prototype/Litmus Test: Implementation -- Usage

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@abonander
Copy link
Contributor Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned nikomatsakis Jan 5, 2017
@abonander
Copy link
Contributor Author

abonander commented Jan 5, 2017

Build failure looks to be unrelated. Nevermind, I was thrown off by only one builder failing but that's the PR builder.

@nikomatsakis
Copy link
Contributor

I haven't looked at the PR, but what is #[proc_macro_attribute]? (Is there some discussion somewhere about it...?)

@nrc
Copy link
Member

nrc commented Jan 5, 2017

@nikomatsakis its a proc macro that is used as an attribute, discussion is in the proc macro rfc

@abonander
Copy link
Contributor Author

@nikomatsakis rust-lang/rfcs#1566

That's why I reassigned @nrc since this is pretty much his baby already, I'm just putting the pieces together.

@abonander
Copy link
Contributor Author

@nrc, @jseyfried suggested moving the changes to proc_macro::TokenStream's implementation details to a separate PR to be merged ahead of this one. I'm not sure if those work either, as the testing is getting caught up on the OS error above, which looks to be occurring when the compiler tries to locate the registrar function.

@jseyfried
Copy link
Contributor

@abonander I'm just suggesting isolating the TokenStream changes to figure out what is introducing the error -- separate PR or not is orthogonal.

@abonander
Copy link
Contributor Author

@nrc @jseyfried I believe I have fixed most of the tests, it was due to not using proc_macro::__internal::set_parse_sess() properly in my additions to CustomDerive.

The remaining two failures have to do with the tests expecting proc_macro::TokenStream::to_string() to append a newline (\n) after each item, which can be trivially restored but I don't think is really necessary. I think we should just drop the end newline from those asserts in the tests.

@abonander abonander force-pushed the proc_macro_attribute branch from dfe362b to 3acf693 Compare January 6, 2017 02:50

fn register_custom_attribute(&mut self,
name: &str,
expand: fn(TokenStream, TokenStream) -> TokenStream) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting -- indent up to the (

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix to follow the style guidelines, but honestly I hate that one point. It's so ugly to have an island of arguments indented all the way over.

@@ -94,6 +95,18 @@ impl MultiItemModifier for CustomDerive {
}
};

let new_items = __internal::set_parse_sess(&ecx.parse_sess, ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: || { is more idiomatic (i.e. add a { before the line break)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So wrap the whole match in a redundant pair of braces?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

let annotated = __internal::token_stream_wrap(annotated);

let res = __internal::set_parse_sess(&ecx.parse_sess, || {
let inner = self.inner;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for a separate variable here -- || (self.inner)(annotation, annotated) should work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this pattern from CustomDerive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still be nice to clean up.

fn check_not_pub_in_root(&self, vis: &ast::Visibility, sp: Span) {
if self.is_proc_macro_crate &&
self.in_root &&
*vis == ast::Visibility::Public {
self.handler.span_err(sp,
"`proc-macro` crate types cannot \
export any items other than functions \
tagged with `#[proc_macro_derive]` \
tagged with `#[proc_macro_derive]` or `#[proc_macro_attribute]` \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid changing the diagnostics for users of stable macros 1.1 custom derives.

@@ -232,6 +190,126 @@ impl<'a> Visitor<'a> for CollectCustomDerives<'a> {
};
self.handler.span_err(item.span, msg);
}
}

fn collect_custom_attribute(&mut self, item: &'a ast::Item, attr: &'a ast::Attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collect_custom_attribute isn't clear to me -- how about collect_attr_proc_macro?

// Found `#[proc_macro_attribute]`
Attribute(&'a ast::Attribute),
None
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FoundAttr seems needless complex. Could we just use an Option<&'a ast::Attribute> instead?


let mut found_attr = FoundAttr::None;

for a in &item.attrs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I tend to avoid one letter names -- I think attr would be nicer here

},
FoundAttr::Derive(_) => {
self.handler.span_err(a.span(), "Cannot combine `#[proc_macro_attribute]`
and `#[proc_macro_derive]` on the same function");
Copy link
Contributor

@jseyfried jseyfried Jan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we wouldn't have to repeat these error messages for "proc_macro_derive" and "proc_macro_attribute" -- we would just error if let Some(found_attr) = found_attr and use a.name() == found_attr.name() to decide between the errors.

on bare functions",
"the `#[proc_macro_attribute` attribute is \
only usable with crates of the `proc-macro` \
crate type"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we also wouldn't need to repeat this error)

vec![registrar, name, cx.expr_path(path)])
}).map(|expr| {
cx.stmt_expr(expr)
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can use a single map and closure here (compose the current closures).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code was laid out like this, I presume to make some borrows work for the closure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it helps with borrows. Refactoring to something like this would be much nicer:

   stmts.extend(custom_attrs.iter().map(|ca| {
        let name = cx.expr_str(ca.span, ca.function_name.name),
        let path = cx.expr_path(cx.path_global(ca.span, vec![ca.function_name]));
        let registrar = cx.expr_ident(ca.span, registrar);
        let ufcs_path =
            cx.path(span, vec![proc_macro, __internal, registry, register_custom_attribute]);
        cx.stmt_expr(cx.expr_call(span, cx.expr_path(ufcs_path), vec![registrar, name, path]))
    }));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like there was a reason the original code was written with multiple closures; you can clean that up too for consistency if you want.

} else {
unreachable!("`found_attr` was `Some` but not one of \
`#[proc_macro_derive]` or `#[proc_macro_attribute]`: {:?}", attr);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this if chain just be

    let msg = format!("the `#[{}]` attribute may only be used on bare functions", attr.name());

} else {
unreachable!("`found_attr` was `Some` but not one of \
`#[proc_macro_derive]` or `#[proc_macro_attribute]`: {:?}", attr);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here -- just

    let msg = format!("the `#[{}]` attribute is only usable with crates \
                       of the `proc-macro` crate type",
                      attr.name());

@@ -610,6 +611,15 @@ impl<'a> CrateLoader<'a> {
);
self.0.push((Symbol::intern(trait_name), Rc::new(derive)));
}

fn register_custom_attribute(&mut self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also change these other uses of custom_attribute to attr_proc_macro (or something else if you want)?

@@ -232,6 +189,112 @@ impl<'a> Visitor<'a> for CollectCustomDerives<'a> {
};
self.handler.span_err(item.span, msg);
}
}

fn collect_attr_proc_macro(&mut self, item: &'a ast::Item, attr: &'a ast::Attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be merged with collect_custom_derive to avoid duplicating code (optional)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see a clean way to do that.

self.collect_attr_proc_macro(item, attr);
} else {
unreachable!("`found_attr` was `Some` but not one of \
`#[proc_macro_derive]` or `#[proc_macro_attribute]`: {:?}", attr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's easy enough to see that this is the only assignment of found_attrs (proving that this is unreachable) that the detailed message in the unreachable!() isn't warrented.

cx.stmt_expr(expr)

cx.stmt_expr(cx.expr_call(cd.span, cx.expr_path(ufcs_path),
vec![registrar, trait_name, cx.expr_path(path), attrs]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: strange indent (also twice, below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What level is that supposed to be indented to? It looks horrible any way I tried.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the line break is here, it should be indented up to the second (.
I would write this:

        let args = vec![registrar, trait_name, cx.expr_path(path), attrs];
        cx.stmt_expr(cx.expr_call(cd.span, cx.expr_path(ufcs_path), args));

let mut found_attr: Option<&'a ast::Attribute> = None;

for attr in &item.attrs {
if attr.check_name("proc_macro_derive") || attr.check_name("proc_macro_attribute"){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be a space between the ) and {

@@ -376,6 +378,7 @@ declare_features! (
(accepted, item_like_imports, "1.14.0", Some(35120)),
// Macros 1.1
(accepted, proc_macro, "1.15.0", Some(35900)),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stray added newline

for attr in &item.attrs {
if attr.check_name("proc_macro_derive") || attr.check_name("proc_macro_attribute"){
if let Some(prev_attr) = found_attr {
self.handler.span_err(attr.span(), "Only one `#[proc_macro_derive]` attribute \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be Only one `#[proc_macro]` attribute ... -- it might not be a #[proc_macro_derive].

Copy link
Contributor Author

@abonander abonander Jan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking #[proc_macro*] or something, presumably #[proc_macro] would be used for procedural macros invoked like normal ones, but you were saying we shouldn't change diagnostics messages for existing custom derive usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this error will be rare enough (and it should be easy for users to tell what's wrong) that
`#[proc_macro]` attribute would be clear enough.

You could also use attr.name() and prev_attr.name() to generate a more precise error message.

self.handler.span_err(attr.span(), "Only one `#[proc_macro_derive]` attribute \
is allowed on any given function");
self.handler.span_err(prev_attr.span(), "Previous procedural macro attribute \
here");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: strange ident (indent to the quote or move the newline to just before the quote and indent to the first (.

@abonander
Copy link
Contributor Author

So as far as test failures go, it's back to the two in the top-level post. Suggestions are welcome.

@nikomatsakis
Copy link
Contributor

@abonander @nrc

rust-lang/rfcs#1566

Indeed, I forgot we approved that RFC entirely! Very good, carry on. =)

@abonander
Copy link
Contributor Author

abonander commented Jan 7, 2017

@jseyfried @nrc Failing test on Travis looks to be due to the change to TokenStream, the panic isn't occurring within the deriving function anymore as TokenStream::from_str() doesn't check for parseability anymore, only lexability, and then the custom derive actually tries to parse items and fails and throws the error there. Should I just update that test to expect the new output?

And in the run-pass-fulldeps suite, the test I added is failing with a compiler error showing the #[attr_with_args] attribute as unused, I'm not sure how to fix that. I've been seeing mark_used(<some attr>) scattered around the codebase, is that what's supposed to be done here?

@keeperofdakeys
Copy link
Contributor

keeperofdakeys commented Jan 7, 2017

The error you are getting is probably from check_attribute in libsyntax/feature_gate.rs. The mark_known function will get you past this. I'm not sure if you'll also need mark_used (this is used for a lint IIRC).

@jseyfried
Copy link
Contributor

@abonander Updating the test is fine if the new behavior is reasonable (seems like an improvement here).

@keeperofdakeys I don't think mark_known is appropriate here. proc_macro_attribute is builtin and macro attribute should be removed before we get to custom attribute checking.

@abonander
Copy link
Contributor Author

abonander commented Jan 7, 2017

@jseyfried the error is occurring at the usage site, so the #[attr_with_args] attribute. It looks like attr::mark_used() is already called in MacroExpander::expand_attr_invoc(), but not attr::mark_known(). I'm gonna try adding that and see what it does.

@abonander abonander force-pushed the proc_macro_attribute branch 2 times, most recently from fcf77e5 to 77e8fc4 Compare January 8, 2017 05:05
@jseyfried jseyfried assigned jseyfried and unassigned nrc Jan 8, 2017
@abonander abonander force-pushed the proc_macro_attribute branch 2 times, most recently from 1374d45 to 03a3e7c Compare January 9, 2017 09:44
@abonander abonander changed the title [WIP] Implement #[proc_macro_attribute] Implement #[proc_macro_attribute] Jan 9, 2017
])}
}

pub fn token_stream_wrap(inner: TokenStream_) -> TokenStream {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we refactor away this function? Just using TokenStream constructor itself seems more ergonomic.

Copy link
Contributor Author

@abonander abonander Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would just mean moving this into the custom derive impl.

What do you mean? You can't construct TokenStream outside this crate otherwise, and wouldn't putting this in impl TokenStream make it visible outside the module?

Copy link
Contributor

@jseyfried jseyfried Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good point.


fn register_attr_proc_macro(&mut self,
name: &str,
expand: fn(TokenStream, TokenStream) -> TokenStream);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent to (

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e. same column as &)

@@ -125,11 +154,17 @@ pub mod __internal {
where F: FnOnce(&ParseSess) -> R
{
let p = CURRENT_SESS.with(|p| p.get());
assert!(!p.is_null());
assert!(!p.is_null(), "proc_macro::__internal::with_parse_sess() called \
before set_parse_sess()!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent to " (remove one space)

fn parse_to_lex_err(mut err: DiagnosticBuilder) -> LexError {
err.cancel();
LexError { _inner: () }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this belongs in __internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, it's not used outside the crate. __internal is for pub functions that have to be hidden behind a feature for use by other rustc crates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I guess since this is also used outside __internal in this crate it makes more sense to have it here.

@@ -1295,6 +1300,7 @@ impl<'a> Resolver<'a> {
invocations: invocations,
name_already_seen: FxHashMap(),
whitelisted_legacy_custom_derives: Vec::new(),
proc_macro_attribute_enabled: proc_macro_attribute_enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add trailing comma

fn check_not_pub_in_root(&self, vis: &ast::Visibility, sp: Span) {
if self.is_proc_macro_crate &&
self.in_root &&
*vis == ast::Visibility::Public {
self.handler.span_err(sp,
"`proc-macro` crate types cannot \
export any items other than functions \
tagged with `#[proc_macro_derive]` \
currently");
tagged with `#[proc_macro_derive]` currently");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we print proc_macro_attribute here when appropriate by adding an argument to check_no_pub_in_root? (a str or an attribute argument would work)

Copy link
Contributor Author

@abonander abonander Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this might be printed before we've ever discovered any items with #[proc_macro_derive] or #[proc_macro_attribute].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, ideally the error message would depend on whether the user has #![feature(proc_macro_attribute)] (adding an argument might not be appropriate).

It's OK as is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When stabilized, we can just say "#[proc_macro_derive], #[proc_macro_attribute] or #[proc_macro]"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jseyfried should we mention proc_macro_attribute if the proc_macro feature is enabled?

})
}

pub fn token_stream_inner(stream: TokenStream) -> TokenStream_ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here -- I don't think we need this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean make it a method of TokenStream or what?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm -- this is OK as is

if *attr == maybe_attr { return true; }
}

false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplification: BUILTIN_ATTRIBUTES.iter().any(|&(maybe_attr, _, _)| attr == maybe_attr)

"used internally by rustc",
NOT_A_FEATURE)),


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra newline

pub fn check_features(features: &Features, handler: &errors::Handler) {
if features.proc_macro_attribute && features.custom_attribute {
handler.err("Cannot use `#![feature(proc_macro_attribute)]` and \
`#![feature(custom_attribute)] at the same time");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent to " (add a space)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correctly indented, the backtick is in the same column as the quote.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indented to the ( now (i.e. how we would indent if it were the start of the second argument). Since it instead continues the quote in the first argument, we indent one more space to the " (same column as the C).

@bors
Copy link
Contributor

bors commented Jan 20, 2017

⌛ Testing commit 04ecee1 with merge bdf9361...

@bors
Copy link
Contributor

bors commented Jan 20, 2017

💔 Test failed - status-travis

@abonander
Copy link
Contributor Author

@alexcrichton @jseyfried Looks like this latest build breakage was caused by another PR merged ahead of mine interacting with the change in the ordering of passes. Should I just fix the failing test in a fixup to my last commit?

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 20, 2017
…seyfried

Implement `#[proc_macro_attribute]`

This implements `#[proc_macro_attribute]` as described in rust-lang/rfcs#1566

The following major (hopefully non-breaking) changes are included:

* Refactor `proc_macro::TokenStream` to use `syntax::tokenstream::TokenStream`.
    * `proc_macro::tokenstream::TokenStream` no longer emits newlines between items, this can be trivially restored if desired
    * `proc_macro::TokenStream::from_str` does not try to parse an item anymore, moved to `impl MultiItemModifier for CustomDerive` with more informative error message

* Implement `#[proc_macro_attribute]`, which expects functions of the kind `fn(TokenStream, TokenStream) -> TokenStream`
    * Reactivated `#![feature(proc_macro)]` and gated `#[proc_macro_attribute]` under it
    * `#![feature(proc_macro)]` and `#![feature(custom_attribute)]` are mutually exclusive
    * adding `#![feature(proc_macro)]` makes the expansion pass assume that any attributes that are not built-in, or introduced by existing syntax extensions, are proc-macro attributes

* Fix `feature_gate::find_lang_feature_issue()` to not use `unwrap()`

    * This change wasn't necessary for this PR, but it helped debugging a problem where I was using the wrong feature string.

* Move "completed feature gate checking" pass to after "name resolution" pass

    * This was necessary for proper feature-gating of `#[proc_macro_attribute]` invocations when the `proc_macro` feature flag isn't set.

Prototype/Litmus Test: [Implementation](https://github.com/abonander/anterofit/blob/proc_macro/service-attr/src/lib.rs#L13) -- [Usage](https://github.com/abonander/anterofit/blob/proc_macro/service-attr/examples/post_service.rs#L35)
@alexcrichton
Copy link
Member

alexcrichton commented Jan 20, 2017

@abonander naively I'd say the error is caused by this PR b/c master is always green, but I haven't looked too much into what's actually happening here. In any case I've fixed that test in the rollup (I believe) which will hopefully land soon and include this PR...

bors added a commit that referenced this pull request Jan 20, 2017
bors added a commit that referenced this pull request Jan 21, 2017
@bors bors merged commit 04ecee1 into rust-lang:master Jan 21, 2017
@jseyfried
Copy link
Contributor

@abonander Would you like to work on #[proc_macro]?

@abonander
Copy link
Contributor Author

@jseyfried I was waiting to see if anyone else wanted to take it on, but I guess I can.

@@ -172,7 +172,7 @@ pub fn check(path: &Path, bad: &mut bool) {
"use_extern_macros", "staged_api", "const_indexing",
"unboxed_closures", "stmt_expr_attributes",
"cfg_target_thread_local", "unwind_attributes",
"inclusive_range_syntax"
"inclusive_range_syntax", "proc_macro"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding entries to this list is not wanted, only removing them. I see though that you actually added a gate test, just only in the compile-fail-fulldeps directory, and the tidy check didn't run through that directory. So you only worked around a bug/missing feature in the tidy check, I'd say this is okay. I'll file a PR to remove it from the whitelist again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, proc macro tests need fulldeps to work because they need the TokenStream typedef from libproc_macro. Fixing tidy was a little bit out-of-scope.

@abonander
Copy link
Contributor Author

@jseyfried I guess I'll get started on #[proc_macro] when #39247 lands.

est31 added a commit to est31/rust that referenced this pull request Jan 22, 2017
PR rust-lang#38842 has exposed that we were missing the src/test/compile-fail-fulldeps
directory in the search for feature gate tests. Because the detection didn't
work despite the effort to name the test appropriately and add a correct
"// gate-test-proc_macro" comment, proc_macro was added to the whitelist.

We fix this little weakness in the feature gate tidy check and add
the src/test/compile-fail-fulldeps directory to the checked directories.
@jseyfried
Copy link
Contributor

@abonander Ok, great!
If you'd like to wait, that's fine too. I would like #[proc_macro] to land in the next couple of weeks though, so if no one has started it yet next weekend I'll start working on it myself.

@abonander
Copy link
Contributor Author

I don't mind, I just didn't want to hog all the glory for myself.

@est31
Copy link
Member

est31 commented Jan 22, 2017

If nobody r+'s an older PR in the next hour and a half or so, #39247 will land within the next 3-4 hours. The queue is quite short :)

bors added a commit that referenced this pull request Jan 23, 2017
Remove proc_macro from the tidy whitelist again

PR #38842 has exposed that we were missing the src/test/compile-fail-fulldeps
directory in the search for feature gate tests. Because the detection didn't
work despite the effort to name the test appropriately and add a correct
"// gate-test-proc_macro" comment, proc_macro was added to the whitelist.

We fix this little weakness in the feature gate tidy check and add
the src/test/compile-fail-fulldeps directory to the checked directories.

Part of issue #39059 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants