Expand items before their derives

[abonander]

continuation of #41029

closes #47358

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[abonander]

r? @nrc

cc @keeperofdakeys @dtolnay @sgrif @alexcrichton @jseyfried (users participating on original PR)

[abonander]

@nrc Build passed, will squash after review

[abonander]

cc #38356

[bors]

☔️ The latest upstream changes (presumably #48449) made this pull request unmergeable. Please resolve the merge conflicts.

[abonander]

@petrochenkov perhaps you'd like to look at this one too?

[petrochenkov]

Started reviewing, will finish tomorrow.

[bors]

☔️ The latest upstream changes (presumably #48586) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

@abonander
Do you know why these attributes are filtered away here?
They are normally produced by builtin derives, but I'm not sure how they affect custom derives and why they need to be filtered away before applying a custom derive.

[abonander]

@jseyfried added this in the original PR. Naive guess, it's avoiding exposing implementation details to the custom derive? Because these would be retained in the actual item since custom derives don't re-emit the item they're applied to.

[petrochenkov]

custom derives don't re-emit the item they're applied to

Aha, this must be the key observation. Everything looks reasonable then.

[petrochenkov]

@abonander
Did you figure out what happens here and above in fn expand?
I can't understand it just by reading the code, but it usually becomes more clear after actively doing some refactoring or modifications.

[abonander]

I'm not sure what you mean by "figure out what happens here". It seems to do what it says on the tin; it accumulates bang/attribute macro invocations and then expand expands them. It has to be done in a loop because macro expansions can produce more invocations.

[petrochenkov]

Nit: comment style, // please.

[petrochenkov]

The original PR removed these fields and fn visit_mac below.
Are they needed again for some reason?

[petrochenkov]

Hmm, this commit also reverts some changes done in the original PR.

[abonander]

This and the other nonsensical changes are probably due to a naive rebase where I just applied all nonconflicting changes from both sides. I'll have to go through and fix these or redo the rebase.

[petrochenkov]

Ok, r=me once Travis is green and all the rebase issues are cleaned up (ideally by doing a "less naive" rebase).

EDIT: Also squashing non-@jseyfried commits would be nice.

[abonander]

@petrochenkov Since I don't completely understand everything that this code is doing, I'm slightly concerned about derives that declare custom attributes: it seems that this should break it since we would always end up looking at the attributes first and throw an error since they don't resolve to anything. However, the following tests using custom attributes are passing:

• https://github.com/rust-lang/rust/blob/master/src/test/run-pass-fulldeps/proc-macro/derive-attr-cfg.rs
• https://github.com/rust-lang/rust/blob/master/src/test/run-pass-fulldeps/proc-macro/derive-b.rs

Do you think these are enough of a smoke test that we don't need a separate test to ensure this hasn't broken custom attributes for derives?

@jseyfried could you weigh in on this since it's mostly your code?

[aidanhs]

Just to give an update, crater run will start in ~4 days. Sorry :( This PR got pushed down because a more time-critical one cropped up.

[petrochenkov]

@abonander

I'm slightly concerned about derives that declare custom attributes: it seems that this should break it since we would always end up looking at the attributes first and throw an error since they don't resolve to anything

I'm not sure what is this about exactly.
The linked tests don't contain attributes declared by derives? (I.e. produced by expansion of derive.)
IIRC, attributes are processed in the same order they are written? (Except for maybe cfg?) I.e. #[foo] is processed after #[derive(Foo)] in the linked example. Does this PR change this behavior?

[abonander]

The linked tests don't contain attributes declared by derives? (I.e. produced by expansion of derive.)

I thought attributes declared by derives are meant to be applied by the user and consumed by the derive, i.e. removed after the derive completes. This is how Serde uses them anyway: https://serde.rs/attributes.html

This PR, at least nominally, expands derives last on an item, processing other macros first: https://github.com/rust-lang/rust/pull/48465/files#diff-54079e90040377e52f969c857e3f558aR21

If we expand the item itself first before its derives, the expander would look at these custom attributes and find that they don't resolve to anything and throw an error. However, the tests I linked are passing which suggests this has already been accounted for.

[petrochenkov]

I suspect that the expander classifies the attributes first (into builtin, unknown, whitelisted, macro-attributes of various kinds of legacy-ness) and expands only those that are actually macros.

[keeperofdakeys]

The ProcMacroDerive::expand function marks all matching attributes as used and known when expanding a proc_macro_derive, so the compiler ignores them. They were once removed, but that prevented multiple derives using the attribute, and meant the derive had to return a modified item.

IIRC this works because errors regarding macro resolution only become a hard error once everything has expanded.

This change doesn't make derives happen last, it makes bang macros happen first (so called partial expansions).

[abonander]

Mmm, good assessment. I've only recently started getting back into macro expansion stuff so a lot of these changes went over my head.

[aidanhs]

Crater run started

[nikomatsakis]

This came up again today when discussing procedural macros (e.g., Macros 1.2). I'm still pretty hesitant to make changes here -- it's not clear that expanding before derive is what we want to do.

[jseyfried]

(Re)reviewed, LGTM. Were there any other questions about the code not answered by #48465 (comment) or #48465 (comment)? (thanks @keeperofdakeys!)

[jseyfried]

@nikomatsakis Has @nrc changed his mind?

The main motivation for this was that we already "expand" #[cfg] and #[cfg_attr] before derive, and this is widely relied upon today. If we expand everything, then we can think of cfg and cfg_attr as just other attribute macros. This is useful e.g. so that people can implement their own customized my_cfg attribute macro that behaves exactly like cfg where desired.

[nrc]

Has @nrc changed his mind?

Yes. Sorry, I did not at all understand what is going on here. I don't think we can land this because changing expansion order is a breaking change, but also because the more complex the expansion order, the harder it is to understand what is going on. The fact that cfgs are evaluated out of order with respect to macros is an unfortunate historical artefact and shouldn't be considered precedent here.

The trouble I think is that one can create examples which benefit from either expansion order. Given that, I think we must go for the most predictable and consistent behaviour, which I believe is the current behaviour.

Apologies that I didn't get to this conclusion earlier.

[abonander]

@nrc What about making the expansion order opt-in? We could apply some derives pre-expansion and some derives post-expansion.

[jseyfried]

@nrc Ok.

To be clear, cfgs are only evaluated out of order with respect to derive macros, not any other kind.

I don't think we can land this because changing expansion order is a breaking change

I doubt this is breaking in practice, but yeah back-compat could complicate things...

The fact that cfgs are evaluated out of order with respect to macros is an unfortunate historical artefact and shouldn't be considered precedent here.

I think it's worth noting though that just about every major derive macro today needs cfg to be expanded first. That is, if the historical artifact had not existed, just about every derive macro would need to opt in to (at least) cfg getting expanded first (via expand: Tokenstream -> Tokenstream or some other mechanism) to support people deriving things on structs with conditionally compiled fields.

The trouble I think is that one can create examples which benefit from either expansion order

Can you think of examples of derives that don't want their input fully expanded? These seem like a far smaller class, especially if we assume that attribute macros will eventually be allowed on struct fields.

Given that, I think we must go for the most predictable and consistent behaviour

I think most predictable/consistent would be either everything is expanded first, or the macro sees the original, raw, unmodified Tokenstream from the source code. Halfway seems weird.

I like @abonander's idea of allowing the macro author to request either unexpanded or fully expanded input. If derive back-compat is an issue, not requesting either could default to today's behavior.

[abonander]

Something like

#[proc_macro_derive(MyTrait, expand_input = "true")]

Or do we want to provide it as a general mechanism in libproc_macro like @jseyfried suggested with pub fn expand(TokenStream) -> TokenStream?

[jseyfried]

Just providing expand: TokenStream -> TokenStream wouldn't be enough to opt out, which presumably we'd want since at least cfg will continue to be expanded first for derives by default.

[abonander]

I meant for that to be the way to opt-in to input expansion, by programmatically asking for it:

#[proc_macro_derive(MyTrait)]
pub fn my_trait_derive(input: TokenStream) -> TokenStream {
    let expanded = proc_macro::expand(input);
    // ...
}

[nrc]

What about making the expansion order opt-in?

So, for proper proc macros my preferred way to opt-in is to let macros eagerly expand macros in their body by adding expand functionality to the API available to proc macros. This also helps custom derive in the long term if you rewrite your custom derive to be token-based rather than string-based. However, this is kind of a long way off. Personally I'm happy to wait. Custom derive was always meant to be an 80/20 solution and very rough and ready. We could have some opt-in as part of the custom derive attribute, but the motivation would have to be very strong IMO.

Can you think of examples of derives that don't want their input fully expanded?

macro_rules! methods {
    ($($m:item,)*) => { () }
}

#[derive(Foo)]
struct S {
    f: Bar,
    methods! {
        fn baz() {}
    }
}

Where Foo expands to the struct and an impl wrapping the methods. This is clearly a hack, but I believe is used in the wild already.

[jseyfried]

@nrc

Ok, expand instead of opt-in makes sense. Do you think derives will ever be able to opt out of today's cfg expansion then?

This is clearly a hack, but I believe is used in the wild already.

Interesting, ideally I would have hoped "inert attributes" would be used for that:
#[derive(Foo)] #[methods(...)] struct S { ... } (c.f. serde).

[nrc]

Do you think derives will ever be able to opt out of today's cfg expansion then?

Hmm, this is an excellent question. I'm not really sure - I don't see why from a design perspective, however, from an implementation point of view messing with the order of 'expansion' of cfgs c.f. macros might be hard.

Interesting, ideally I would have hoped "inert attributes" would be used for that

iiuc, the problem there is that writing a lot of code inside an attribute is not so nice to look at.

[jseyfried]

from an implementation point of view messing with the order of 'expansion' of cfgs c.f. macros might be hard

Nah, it'd be pretty straightforward -- quite a bit simpler than this PR.

iiuc, the problem there is that writing a lot of code inside an attribute is not so nice to look at.

Yeah... I think with the right formatting it'd still be nicer, but if that's what's being done in the wild then so be it.

[aidanhs]

Hi @petrochenkov (crater requester/PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-48465/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

[abonander]

We've got some significant breakages in derive crates:

• phf_macros
• rental
• cssparser
• data-encoding-macro (used by trust-dns)

Several of these derives were expecting stringify!() in their input. They are probably expanding it internally anyways, so this change would probably be beneficial to them, but it looks like making this opt-in is our only option to avoid breakage.

[petrochenkov]

@abonander
So, should we close this PR then?
I think the opt-it/opt-out scheme deserves an RFC.
Also, the expansion order (as it works now) badly needs documentation - this is something that can be solved by the same RFC as well.

[abonander]

Thing with RFCs is that they're primarily for proposing solutions and I don't think we've gotten that far. I think exposing input expansion programmatically (in libproc_macro) is the most future-forward approach since it allows bang and attribute macros to utilize it as well. And it wouldn't take any effort to feature-gate it. Either way we should probably move this discussion to IRLO and close unless someone suggests something immediately actionable.

[petrochenkov]

Either way we should probably move this discussion to IRLO

Yeah, a pre-RFC on IRLO would probably be even better.

[abonander]

Created a topic on IRLO: https://internals.rust-lang.org/t/pre-rfc-input-expansion-for-proc-macros-derives-primarily/7210