Ambiguity between custom derive attributes and proc macro attributes

[alexcrichton]

Discovered by @alercah here
it's not clear what to do with this crate right now:

#![feature(proc_macro)]

extern crate proc_macro;
#[macro_use]
extern crate quote;

use proc_macro::TokenStream;

#[proc_macro_derive(Hello, attributes(Bye))]
pub fn Hello(_: TokenStream) -> TokenStream {
    (quote! { fn hello() { println!("hello") }}).into()
}

#[proc_macro_attribute]
pub fn Bye(_: TokenStream, _: TokenStream) -> TokenStream {
    TokenStream::new()
}

along with:

#[Bye]
#[derive(Hello)]
struct T()

cc @petrochenkov

[alexcrichton]

Some immediate mitigations that look like they should be in place are:

• A proc-macro crate should not be able to export an attribute which conflicts with any attributes on a custom derive.
• A module should not be able to bring an attribute in scope with a custom derive in scope where the two conflict (the name of the macro attribute and the name of the derive's custom attributes)

@alercah and @petrochenkov, can y'all comment to whether this fixes the issue at hand or not? This is the same as I mentioned here but @alercah I wanted to clarify that the importing case is hopefully handled by the second bullet.

In general I also think the most pressing part here is future-proofing ourselves. We don't necessarily need a fully fleshed out and fully rationalized system from day 1, we mostly just need to buy us as much runway as we can.

[alercah]

Now that I've done the hard work, I'm pretty sure conflicts will be resolved if you add:

• A module should not be able to bring a custom derive into scope if any of its attributes conflict with the custom derive attributes of another custom derive from a different module (it is okay if two custom derives from the same module share an attribute, like how serde_derive::Deserialize and serde_derive::Serialize share serde).

But I worry that this isn't enough future-proofing, because the implicit import behaviour will be stabilized if we go ahead as-is. As I understand it, right now, if you import serde_derive::Deserialize, you get serde_derive::serde whether you like it or not. If people like it, then we can't take it away later.

[alercah]

Ah, wait, I think that since that's only for imports, proc_macro could be stabilized with the export restriction, and then things could be fixed later for use_extern_macro. I'd appreciate another opinion on that, though.

[alexcrichton]

@alercah I'm gonna respond to your other comment over here to try to keep discussion centralized, copied below in case anyone needs it!

So the main point of the inert attributes is to provide a way for proc_macro_derive to be passed additional information that cannot otherwise be passed in. There is otherwise no way, barring trickery like custom-derive or encoding it in silly ways like redundant groupings, for extra information to be provided to a proc macro. This is especially important given that proc_macro_derive does not support arguments, but additionally it lets you tag fields or variants in a way that is both natural and coherent.

If we imagined a world where proc_macro_derive accepted arguments but didn't have inert associated attributes, we couldn't replicate all of today's functionality exactly. While attribute macros are evaluated in source order (and can create or remove attributes appearing after, but not before, them in the source), derives are only run after all attribute macros, including nested ones. And attribute macros are not yet supported on fields or on enum variants, it appears; even if they were, however, the macros would need to leave some residue behind for the derive macro.

On the other side, derive macros can't (currently) edit the token stream of the struct they are deriving for, so they cannot remove their residue. Therefore, we can't have these inert attributes always be an error. Based on testing, the current resolution system is this, modulo some bugs:

1. In order, all attributes are evaluated, with macros replacing the rest of the declaration, including following attributes. If an in-scope inert attribute (putting aside questions of shadowing and what exactly brings it into scope) is encountered, then the declaration is checked to see if it has a corresponding derive. If it does, it is left in the token stream. If not, this is an error (the diagnostic sucks, currently, but it is an error nonetheless). Note that the derive must exist at the time the macro is checked; it cannot be added by a subsequent attribute macro.
2. cfg, but no other, attributes are processed inside the declaration.
3. Derive macros are processed.
4. Built-in derives are processed.
5. The rest of the declaration is processed normally.

This is a bit of a mess.

If we imagine ourselves trying to reconcile this against future plans like scoped attribute names, we run into issues because inert attribute processing in step 3 is unable to accommodate paths: the derive macro has no way of knowing if foo::inert refers to its own inert attribute or another macro's, because it has no way to understand paths or name lookup. As it is, they cannot be scoped and so if two macros collide on magic inert attribute names, you are SOL. This isn't entirely out of line with, say, macros that have magic field names, but it feels quite inconsistent given that attribute macros are expected to get first-class names; why not inert attributes?

Regular attribute macros run into the same problem, if you try to coordinate between two of them (again, suppose you want to mark fields). Currently, attribute macros can fake inert attributes by removing them from fields as they are encountered, but they would run into the same issue of trying to identify other attributes by way of scoping.

Additionally, the lack of explicit declarations for inert attributes is problematic.

At the risk of relitigating #35900 and #37614, I'm going to consider what I would do if I was designing from scratch:

1. Give procedural macros an explicit way to compare a def-site path and a call-site path to determine if they refer to the same item. All macros can use this to identify inert attributes.
2. Make it so that macros are expanded in the following order:

1. Attribute macros and other macro-like attributes like `cfg` on the item.
2. Attribute macros and other macro-like attributes within the item.
3. Derive macros.
4. Built-in derives.

This gives derives a clear power that custom attributes do not: they happen after the structure is locked in. It also makes it safe for attribute macros to modify or remove fields, because deriving always happens last.

3. Make inert attributes require explicit declaration and are declared in the macro namespace. I'm not sure what form the declaration should take. It may be nice to define the syntax of arguments, so that they can be vetted proactively by the compiler. The effect of attributes in proc_macro_derive is changed from a declaration, to simply an indication that the derive macro uses the attribute. It is also supported on other proc_macro* attributes, in case they wish to leave inert attributes in place to pass on to later macros, they can do so without having to figure out whether they are invoked.
4. Inert attributes are, as now, left unexpanded. The semantics are simply that they error if they are not used. An attribute is considered used if it was the result of macro expansion by a macro which is declared as using it, or if it is inside or pertaining to a declaration

The first one actually seems like a fairly conservative extension to the proc_macro API. The second is related to RFC 2320, which proposes an opt-in way for derives and other proc macros to expand macros in their input. In practice, I imagine that custom derives will always want to fully expand their input of unknown macros, since otherwise they cannot trust that fields are not going to appear or disappear after they have declared the deriving impls. But it is not a deal-breaker.

The third is a bigger change, and would break existing users of proc_macro_derive. But if we make inert attributes into first-class names, then that in turn implies that they should really have declarations rather than weird implicit declarations inside the proc_macro* attributes. It could allow for piggybacking on other packages' attributes in a way fully supported by t, for instance (e.g. attributes(serde_derive::serde)), and it would definitely serve to make scoping clear.

The fourth is just a slight semantic reordering to let the derive that uses an inert attribute be added after the attribute, reducing ordering dependence.

In general, it strikes me as weird that proc_macro_derive doesn't take arguments, but perhaps that was never considered. In any case, I think this makes my preferred proposal as follows:

• Inert attributes become a separate kind of entity in the macro namespace, in 2018. Currently the only way to declare them is with attributes on proc_macro_derive, which declares them in the current namespace.
• The proc_macro API is augmented with a way to compare paths for equality.
• As a subsequent feature, inert attributes become explicitly declarable, and proc_macro and proc_macro_attribute are able to refer to them. They cannot refer to implicitly declared ones.
• Once the above feature and RFC 2320 are available, a new version of proc_macro_derive is made which offers arguments to derives, as well as two breaking changes: they no longer implicitly declare inert attributes, and they are applied later in the expansion order per the above.
• The new version of proc_macro_derive is made standard in the next epoch.

(Or, alternatively, we decide it should be part of 2018 but will be late, so we require 2018 code to use #[feature(proc_macro_derive_2015)] or something to preserve old behaviour).

---

It's true yeah that the macros 1.1 custom derive inert attributes aren't necessarily going to mesh perfectly with the module system, but the fact of the matter is that macros 1.1 is stable in Rust and we don't currently plan to change/deprecate any portions of its design. While it's true that weird cases can come up they should also be weighed against how common they're expected to be to provide a signal as to how urgent it'd be to resolve them.

In that sense we don't really have an opportunity to design the system from scratch, and it's basically already stable that use serde_derive::Serialize imports the ability to use #[serde] on fields and such. I think we'll basically have to rely on community norms to say "don't define a custom attribute #[serde] and expect it to work smoothly".

I think it's possible to implement the extra restriction you mentioned but it affects stable code so we'd need to be very careful about making such a change. Additionally it doesn't actually impact macros 1.2 stabilization (it only affects stable code already) so it may mean we don't necessarily need to tackle this for the 2018 edition!

[alercah]

Yes, sorry, I have a habit of writing "here's what I would do designing from scratch" as a way to sort through my thoughts, rather than actually proposing a ground-up rewrite. I think that the proposed changes to macros 1.1 are pretty straightforward, enough to maybe sneak along an epoch boundary to unify everything, but not urgent.

I realized I didn't really think through how this all interacts with declarative macros, and I'm now worried that will also cause futureproofing issues. If we're going to consider putting those into the macro namespace at some point, then we have to prepare for that rather than having pseudo-namespaced identifiers. Part of the arguments I read today about the stabilization of proc_macro_derive involved namespacing, and they decided to keep them in the same namespace as macros because it's easier to separate them than merge them. In theory, attributes are supposed to be the same, but more experimentation seems to indicate they aren't always. I just tested, and it's impossible to shadow the built-in derive attribute, and I wouldn't be surprised if this was true for other attributes as well.

[alercah]

You can, however, shadow built-in derives, even with non-derive macros.

[alercah]

Ah, but you can't shadow them with other derives. That one's an error.

[aturon]

Bumping to RC2 milestone

[alexcrichton]

@petrochenkov hey I was curious to check in on this issue, I believe the current state is that whitelisted attributes are not macro-expanded, even if there's a macro in scope, right?

[petrochenkov]

@alexcrichton
They are shadowed by macros in scope right now.

macro serde() {}

#[derive(Serialize)]
#[serde] // ERROR macro `serde` may not be used in attributes
struct S;

fn main() {}

This causes regressions like #53583 and #53898.

My intention so far is to bump priority of derive helpers so they shadow other macros instead (#53583 (comment)).
Given that introduction of serde into scope (#[derive(Serialize)]) is closer to the use-site (#[serde]) than macro serde() {}, so it's reasonable to expect that closer definition shadows definitions from more remote scopes.

[alexcrichton]

Ok cool, sounds good to me!

[petrochenkov]

Status update: future proofed in #54086.

The model is largely the same as previously, the helper attribute (e.g. #[serde]) is not defined by the proc-macro crate and imported, but introduced into scope by expansion of derive macro (e.g. #[derive(Serialize)]).

Future proofing: If derive helper attribute is ambiguous with any other macro name in scope (taking sub-namespacing into account), then ambiguity error is reported.

Compatibility: to avoid regressions, helper attributes are succesfully resolved even if they are written before the derive

#[serde] // OK for compatibility
#[derive(Serialize)]
struct S;

, this is not good - the attribute is not in scope yet at that point if we want to pursue straightforward left-to-right expansion model.

Future plans:

• Possibly relax the ambiguity errors, derive helpers should probably shadow anything else since they are introduced closer to the point of use than anything else.
  However, they are effectively macro expanded from their corresponding derive (derive itself must be resolved for the helper to be introduced into scope), so restricted shadowing should apply.
• Report deprecation warnings for helper attributes placed before their corresponding derives.

I think we can close this generic issue now and perhaps create new more focused issues for "future plans" (if those even need tracking).