Custom attributes from unstable plugins and procedural macro attributes are not compatible

[nox]

In servo/servo@master...nox:custom-derive I changed the attribute #[dom_struct] from our plugins crate (a plugin = true crate) into a proc_macro_attribute function. Unfortunately, the plugins crate also defines a whitelisted attribute #[must_root] and using both at the same time fails with the following compile error:

error: macro undefined: `must_root`
   --> /Users/nox/src/servo/components/script/serviceworkerjob.rs:115:1
    |
115 | #[must_root]
    | ^^^^^^^^^^^^

[nox]

15:25 <SimonSapin> nox: no sure you’ll get a response other than "don’t use a plugin", unfortunately

Note that #[must_root] is crucial in Servo, because that's the only thing that prevents us from doing very bad things with our JS engine garbage collector, as part of the unrooted_must_root plugin. There is currently absolutely no way to write that as something else than a compiler plugin, so "don't use a plugin" isn't a valid answer to this bug.

[abonander]

@nox Does the plugin remove the #[must_root] attribute after it passes over an item?

cc @jseyfried

[nox]

@abonander Which plugin do you mean? #[dom_struct] and #[must_root] are independent, but the former expands to a bunch of things, including #[must_root]. It should also be noted that both #[must_root] from #[dom_struct] and #[must_root] applied directly (on things which don't have #[dom_struct]`) trigger the error.

[SimonSapin]

This comment was a bit exaggerated, sorry :) The issue looks like some bad interaction between old-style plugins and new-style procedural macros. Given that plugins are on the way out, I’m skeptical that people will be interested in putting in work to fix it.

In the short term, that means we’ll need to keep dom_struct as a plugin as well. (Unless we can make it a macro_rules! macro, though that makes every use site more verbose, wrapping the entire type in an extra pair of {} braces.)

In medium term, the usual advice is to move everything to proc macros instead of plugins. But as far as I understand this doesn’t work for must_root which is tied to a custom lint check.

The "best" solution here would be proper GC support in the language, but it seems that there is still a lot of work to do before that happens. Maybe @pnkfelix can say more.

[keeperofdakeys]

Which nightly were you using for this? There have been some PRs playing with the expansion order recently, which may have fixed/introduced the issue. This is definitely an issue that should be fixed though.

[SimonSapin]

Servo is currently on rustc 1.17.0-nightly (025c328 2017-02-15)

[nox]

I just tried with (8a1ce40 2017-02-21), it didn't fix the problem.

[nox]

Actually the error is different now:

error: cannot find attribute macro `must_root` in this scope
   --> /Users/nox/src/servo/components/script/serviceworkerjob.rs:115:1
    |
115 | #[must_root]
    | ^^^^^^^^^^^^

I double checked whether I still do whitelist it, and whether I still have an plugin attribute for the plugin crate, and I'm pretty sure I do.

[nox]

I pushed the rustup commit to the branch if anyone wants to try.

[keeperofdakeys]

That was simply the error message being updated. If I have some free time tonight, I'll try looking into this. Does #[must_root] still work by itself? Or only not work when it's expanded through the proc_macro_attribute? Also does the order of attributes that #[dom_struct] expands to have any effect?

[nox]

It doesn't work anywhere, even on structs that don't use #[dom_struct] at all.

[keeperofdakeys]

I did some debugging, and I only start getting this error when I add the proc_macro feature.

[nox]

Well yes of course. This bug was discovered when #[dom_struct] was made into a procedural macro instead of an old-style compiler plugin.

[abonander]

This is because #![feature(proc_macro)] causes the compiler to consider all non-builtin or plugin attributes as procedural macros, which is why #![feature(proc_macro)] and #![feature(custom_attribute)] are mutually exclusive. This was to avoid major changes to the way expansion works.

Perhaps we can implement associated attributes similar to what custom derive does. @jseyfried What do you think?

[nox]

@abonander We don't use custom_attribute, so I don't see how that feature is related.

[nox]

Perhaps we can implement associated attributes similar to what custom derive does. @jseyfried What do you think?

Also note that #[must_root] is not associated to #[dom_struct] or any procedural macro or procedural macro attribute. It's only used as part of a lint, that can't be expressed by any stable code.

[abonander]

Regardless, that's what I believe is causing the problem here. It's trying to eagerly expand attributes as procedural macros because a kind-of chicken-or-egg problem in that part of expansion. I half expected we'd end up dealing with something like this but I was more eager to at least get the basic implementation going so I could use it with my own prototype.

[keeperofdakeys]

I think the right thing to do here is to modify this function to ignore plugin attributes. (It's only called when the proc_macro feature is enabled). Checking that feature_gate.plugin_attributes does not contain the name may be enough.

[abonander]

The problem is that #[must_root] isn't declared anywhere. It only worked without #![feature(custom_attribute)] because the unrooted_must_root lint marked it as used by checking for it on items.

Perhaps it is necessary to require lints to declare any attributes they use.

[nox]

It's declared and whitelisted in servo_plugins.

[nox]

https://github.com/servo/servo/blob/master/components/script_plugins/lib.rs#L47

[abonander]

Ah, so yeah, that needs to be checked when collecting attributes then.

[jseyfried]

@SimonSapin @nox

The issue looks like some bad interaction between old-style plugins and new-style procedural macros. Given that plugins are on the way out, I’m skeptical that people will be interested in putting in work to fix it.

I think it is important that old-style plugins interact as well as possible with procedural macros to ensure a smooth transition. At a bare minimum, adding #![feature(proc_macro)] should not break any existing uses of old-style plugins. Any issues involving interaction between plugins and procedural macros will be addressed/fixed quickly.

@abonander fixed this in #40039 (@nox @keeperofdakeys thanks for helping debug!).

[nox]

Thanks @abonander and @keeperofdakeys, and thanks to the Rust team for the neat per-PR nightlies which will allow me to try it in Servo as soon as the PR lands.