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

New RFC: proc-macro-attribute-recursion #2628

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@llogiq
Copy link
Contributor

llogiq commented Jan 23, 2019

This breaks out a small part of #2320 that is simple to implement and reason about and will give us a simple workable solution to macro expansion in proc macros while we wait for the more complete solution to emerge.

Rendered

@llogiq llogiq force-pushed the llogiq:proc-macro-attribute-recursion branch from 8b017e6 to 91ac6d3 Jan 23, 2019

@llogiq llogiq force-pushed the llogiq:proc-macro-attribute-recursion branch from ae7e822 to 665feb4 Jan 23, 2019

@ExpHP

This comment has been minimized.

Copy link

ExpHP commented Jan 23, 2019

So.... what currently happens if a proc_macro_attribute adds another proc_macro_attribute attribute to the output during expansion?

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 24, 2019

It just gets ignored.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

The expander is extended to search the expansion of `proc_macro` and `proc_macro_attributes` for other macro invocations. Those are then expanded until there are no more attributes or macro invocations left or the macro expansion limit is reached, whichever comes first.

This comment has been minimized.

@Centril

Centril Jan 24, 2019

Contributor

This whole section (and the RFC in general) seems rather underspecified; I'd like to see examples of proc_macro added to the previous section at least. This also doesn't seem like a full description of behavior since #[flame] gets applied to macro expansions. This also doesn't say what happens if you write #[flame(alpha, beta)]. This should be specified including with examples.

This comment has been minimized.

@llogiq

llogiq Jan 24, 2019

Author Contributor

I didn't detail what #[flame(alpha, beta)] does (currently it should give you an error), because this RFC does not change this part of functionality. As for bang-macros, it doesn't matter if they are defined via macro, macro_rules! or as a proc macro. Again the functionality of expansion is unchanged.

The only two things this RFC defines: That the output of proc_macro_attributes get expanded and that the order of expansion follows the order of appearance (so that things from the original code get expanded before things added by the expansion).

This comment has been minimized.

@Centril

Centril Jan 24, 2019

Contributor

Well, from what I can tell, the reference says nothing about how #[flame] gets attached to this_is_fun.

That the output of proc_macro_attributes get expanded and that the order of expansion follows the order of appearance (so that things from the original code get expanded before things added by the expansion).

That's not what the text says; it says "The expander is extended to search the expansion of proc_macro and proc_macro_attributes for other macro invocations." -- you've added examples for the latter but not the former.

Show resolved Hide resolved text/0000-proc-macro-attribute-recursion.md Outdated
Show resolved Hide resolved text/0000-proc-macro-attribute-recursion.md Outdated
Implementors will have to make sure to order the expansions within expanded output by their origin: macros which are in the `proc_macro_attribute`s' input need to be expanded before expanding macros that have been added by the `proc_macro_attribute`s themselves. This can easily be done by examining the `Span`s of the expansion and ordering them by `SyntaxContext`.

# Drawbacks
[drawbacks]: #drawbacks

This comment has been minimized.

@Centril

Centril Jan 24, 2019

Contributor

The behavior of attaching #[flame] to expansion of macros is, as far as I can see, theoretically a breaking change if attaching the attribute has an effect on static or dynamic semantics of the expansion. I'm surprised that this behavior is not opt-in. It seems the proc macro author should request this behavior.

This comment has been minimized.

@llogiq

llogiq Jan 24, 2019

Author Contributor

I agree that in theory this is a breaking change. I' would however be surprised if anyone relied on the current behavior, as it doesn't do anything useful. As I commented on #2320, this was the thing I tried because it felt natural and just might have worked – so in effect by adding the attribute, I am opting in to the expansion. What else would a proc macro author expect when adding an attribute that is expanded by a proc macro?

This comment has been minimized.

@Centril

Centril Jan 24, 2019

Contributor

I' would however be surprised if anyone relied on the current behavior, as it doesn't do anything useful.

We should at least crater run it and the theoretical breakage should be added to the text with a reasoning about why you would be surprised and why it doesn't do anything useful.

As I commented on #2320, this was the thing I tried because it felt natural and just might have worked – so in effect by adding the attribute, I am opting in to the expansion. What else would a proc macro author expect when adding an attribute that is expanded by a proc macro?

It could either expand, as per your RFC, to:

mod fun {
    #[flame]
    fn this_is_fun(x: u64) -> u64 { x + 1 }
}

or alternatively:

mod fun {
    fn this_is_fun(x: u64) -> u64 { x + 1 } // This is what I'd expect.
}

this might make a world of difference if #[flame] does transformations to this_is_fun or not.

This comment has been minimized.

@llogiq

llogiq Jan 24, 2019

Author Contributor

OK, I added some text to this effect. Please be aware that this is a very language-lawyerly perspective (not that there's anything wrong with that) – as a proc macro author I certainly wouldn't write code to add an attribute that does nothing. In fact, I wrote this RFC because this was what I tried to get macros expanded in flamer and I don't want to wait for a more general solution that may take far more effort.

Anyway, a crater run certainly won't hurt.

@llogiq llogiq force-pushed the llogiq:proc-macro-attribute-recursion branch from 5555c94 to a08b6cd Jan 24, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 25, 2019

For things to work as described three somewhat independent changes are needed:

  • Change expansion order for attributes.
    Currently attributes are expanded in straightforward left-to-right order.
    With the new expansion order each attribute would get an associated "parent expansion" and attributes would be expanded in the order of (parent_expansion, left_to_right) tuple. One possible issue is that expansions are not totally ordered in general, but perhaps it's not an issue for attributes on a single item? Not sure.
  • Change the rules for inert attributes on macro items/expressions/etc.
    Currently inert attributes (and macro attributes for which expansion is delayed in the new expansion order are effectively inert for some time) attached to a macro invocation are lost (kind of cfg-ed out) when the macro is expanded.
    Instead they need to be spread across the expansion results (cloned if necessary), like #[attr] gen_items!() -> #[attr] item1, #[attr] item2, ....
    Note how this is a fundamentally AST-based operation - the macro can't "just" produce tokens (like in the TokenStream model), the tokens must be immediately interpreted as AST fragments to complete expansion of #[attr] gen_items!() and "distribute" the attributes.
  • Include macro invocations into the attribute expansion order.
    Currently #[attr1] ... #[attrN] ITEM means "expand attributes in some order, then proceed to the resulting item(s)". Perhaps the items is a macro, in this case it will be expanded as well after all the attributes.
    Instead the ITEM needs to be included as N + 1th element into the expansion order, get its "parent expansion", and be ordered and expanded using the same rule as its attributes, i.e. (parent_expansion, left_to_right) mentioned above.

TLDR: This is probably not a small and simple change, and it may have implications of the TokenStream model.

"Small and simple change" would be to provide fn fully_expand_item(invocation: TokenStream) -> Result<TokenStream, SomeErrorType> delegating to the current ad hoc eager expansion mechanism in the compiler used for expanding arguments of build-in macros like env!(...).
It would certainly not be stabilize-able as is, and I'm not sure how convenient and usable at all it would be for proc macro authors in practice, but it would be at least something to get experience with, because right now proc macro authors cannot expand their input at all.

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 26, 2019

Good point; while this RFC is conceptually simple, the implementation has some subtle details.

  • I don't specify the expansion order beyond requiring that parent expansions be expanded before recursively expanding their result because it is not necessary. Implementing this in a straightforward front-to-back order and keeping a queue of things that need expansion in the future should provide a sufficiently simple implementation of the expansion ordering.
  • It is true that currently inert attributes are removed, however I disagree that changing this is hard. Attributes are just AST nodes, and we can already clone those when necessary. We may choose to implement a MacroExpansion AST node that holds the attributes of the macro expansion to avoid having to clone the attributes, but this is an implementation detail I don't think should be spedified as part of the RFC.
  • Your third point means that we unify macro expansion – where we currently have multiple expansion phases for bang- and attribute macros, those are now interleaved. While I agree with this, every possible solution (such as your fully_expand_item function) shares this trait. I still may want to add some text detailing this change, though.

Also your proposal is incomplete – we'd need multiple functions, for macros in item, expr, pat and ty positions (and I'm not sure if I forgot one). Given this, it is far from simple, too. While I agree that it would indeed be somewhat usable, I'd rather not create such a strong dependency on resolve from expansion. Better to keep the API boundary small.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 26, 2019

@llogiq
Regarding

we'd need multiple functions, for macros in item, expr, pat and ty positions

, is expand_item is a necessary minimum because you can wrap everything else into an item and then unwrap - this is what #2320 suggests too.

(The proper solution should probably be a position agnostic TokenStream -> TokenStream function, but the current compiler machinery to which my minimal fully_expand_item is supposed to delegate to hasn't fully migrated from AST to token streams yet and still needs the "item" part.)

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 27, 2019

True. Fair enough, if I get to pull this trick, you may too. 😄 I still maintain that this would create an API burden that we will never want to stabilize, whereas my proposal, even if more complex, would be completely forwards-compatible.

Apart from that, I outlined how, given a support library, this can be quite usable from proc_macros, too. I imagine that we might extend proc_macro_rules to do argument expansion automatically.

@Centril Centril self-assigned this Jan 31, 2019

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 31, 2019

cc @nrc

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Feb 13, 2019

cc @matklad and @jonathandturner who might be reimplementing macro expansion for their respective IDE support projects. What do you think? Would you prefer a magic function that lets proc macros call back into resolve+macro expansion or this recursive method?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Feb 13, 2019

Heh, as an IDE writer, I would prefer neither of those options :-)

The "magic function" approach frightens me, because macro expansion is no longer a pure TokenStream -> TokenStream function, but depends on the global compiler state (for example, on which names are defined in the scope). That I think breaks caching of macro expansions.

This suggestion seems more amendable to caching, probably, but it seems to change how expansion works pretty significantly. The "copy-paste attributes on macros onto expansions" and "the order of expansion of #[attr] m!() depends on how did we get here" seems like a big change.

Could we instead tag macro definitions with #[needs_eager_expansion], which guarantees that the input to this macro will be valid Rust code, free from macro invocations? This is probably covered in some comment somewhere, but a quick glance to the alternatives section hasn't answered the question.

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Feb 13, 2019

@matklad so this tag would also apply to proc_macro_attributes? That would make my suggestion from above feasible: we could use a proc_macro_attribute to have 'inner' macro invocations expanded (or perhaps we can do this in general, but there may be some problems with expansion order).

Unless there is some snag I'm overlooking, I'm totally for it!

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Feb 13, 2019

@matklad thinking a bit more about it, wouldn't that be functionally equivalent to my proposal? What would happen if I create a proc_macro that returns a macro by example invocation to be eagerly expanded that will force-expand the innards and call-back a proc-macro?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Feb 14, 2019

I have a fuzzy understanding of proc macros, so I don't really know what exactly either proposal means, but I think "eager expand all inputs" is different on the implementation side in that it

  • doesn't reimplementer POV thequire to change ordering of macro expansion
  • does not need "macro attribute on macro invocation" thing

From the macro author POV, I think this is less flexible: you can't selectively process some macro invocations as token trees and others as expanded code, it's all or nothing.

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Feb 14, 2019

True, that is a difference. However, for my use cases your suggestion works, too. @petrochenkov what do you think? Should we close this RFC and setup another? @matklad do you have time to write up a new RFC or should I do it?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Feb 14, 2019

I don't have neither time, nor the required knowledge here :-)

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Feb 16, 2019

@petrochenkov you know more about the current macro expansion implementation – how complex would you rate @matklad 's suggestion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment