-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Eager Macro Expansion #2320
Conversation
So the idea would be to implement the |
@pierzchalski Incidentally, you probably want to CC/assign @jseyfried to this PR. |
@alexreg Whoops! Done. |
On second thought, maybe better to CC @petrochenkov given @jseyfried's long-term absence? |
Sorry, can't say anything useful here, I haven't written a single procedural macro in my life and didn't touch their implementation in the compiler either. |
This is a language/compiler RFC so I guess @nikomatsakis and @nrc are two other people to CC, anyone else who would be interested? |
@petrochenkov Oh, sorry. I gathered from your comments on the declarative macros 2.0 RFC that you knew something of the macros system in general. My bad. |
|
||
* Greatly increases the potential for hairy interactions between macro calls. This opens up more of the implementation to be buggy (that is, by restricting how macros can be expanded, we might keep implementation complexity in check). | ||
|
||
* Relies on proc macros being in a separate crate, as discussed in the reference level explanation [above](#reference-level-explanation). This makes it harder to implement any future plans of letting proc macros be defined and used in the same crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to highlight this drawback. Are the gains in this RFC enough to outweigh this drawback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, why does it require a separate crate for proc macros? Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, this expansion API doesn't add any extra constraints to where a proc macro can be defined, so I guess this shouldn't really be here.
Originally I was worried about macro name resolution (I thought having proc macros in a separate crate at the call site would make that easier but given that there are other issues involving macro paths this seems redundant to worry about), and collecting definitions in an 'executable' form.
Declarative macros can basically be run immediately after they're parsed because they're all compositions of pre-existing built-in purely-syntactic compiler magic. Same-crate procedural macros would need to be 'pre-compiled' like they're tiny little inline build.rs
s scattered throughout your code. I thought this would interact poorly in situations line this:
#[macro_use]
extern crate some_crate;
#[proc_macro]
fn my_proc_macro(ts: TokenStream) -> TokenStream { ... }
fn main() {
some_crate::a_macro!(my_proc_macro!(foo));
}
How does some_crate::a_macro!
know how to expand my_proc_macro!
?
In hindsight, this is just a roundabout way of hitting an existing problem with same-crate proc macros:
// Not a proc-macro.
fn helper(ts: TokenStream) -> TokenStream { ... }
#[proc_macro]
fn a_macro(ts: TokenStream) -> TokenStream {
let helped_ts = helper(ts);
...
}
fn main() {
a_macro!(foo);
}
Same question: how does a_macro!
know how to evaluate helper
? I think whatever answer we find there will translate to this macro expansion problem.
Anyway, I'm now slightly more confident that that particular drawback isn't introduced by this RFC. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd tend to agree with that assessment. Is there an RFC open for same-crate proc macros currently? If so, I'd be curious to read it over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember reading some fleeting comments about it, but I just had a quick look around and I can't find anything about plans for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no expert wrt. proc macros.. I'd also be interested in any resources wrt. same-crate macros.
Thanks for the detailed review and changes =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pierzchalski On a related note, my WIP PR can be found here: rust-lang/rust#47992 (comment). I'm going to make another big commit & push in an hour I think.
Remove 'same crate proc macro' drawback and replace it with discussion under reference explanation, since it's an issue that isn't introduced by this RFC and will also probably share a solution.
|
||
Built-in macros already look more and more like proc macros (or at the very least could be massaged into acting like them), and so they can also be added to the definition map. | ||
|
||
Since proc macros and `macro` definitions are relative-path-addressable, the proc macro call context needs to keep track of what the path was at the call site. I'm not sure if this information is available at expansion time, but are there any issues getting it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this information is available at expansion time. Resolving the macro shouldn't be a problem.
I just realised that one of the motivations for this feature (the #[proc_macro]
fn lift(ts: TokenStream) -> TokenStream {
let mut mac_c = ...;
mac_c.call_from(...);
// ^^^
// This needs to be the span/scope/context of, in this
// example, `main`: the caller of `m`, which is the caller of `lift!`.
...
}
macro m() {
lift!(m_helper!()); // Should set the caller context of `m_helper!` to
// caller context of `m!`.
}
fn main() {
m!();
} But the current |
@pierzchalski Yeah, it looks like either we'd have to bake this |
Good job! I've wanted a solution for this for some time. I see but two possible problem with the solution this RFC PR suggests:
|
@llogiq sorry for the late reply! I'm not sure what point you're trying to make in (1) - if I change the order of two macro calls, I don't really expect the same result in general, similar to if I change the order of two function calls. Do you have a concrete example of a proc macro which wants to ignore/pass-through macro nodes but which also cares if an expression comes from a macro expansion? Also re. (1), I'm not overly familiar with the expansion process but as far as I understand and recall, the current setup is recursive fixpoint expansion, which makes it hard to have cleanly delineated pre- and post-expansion phases for macros to register themselves for. Can you clarify how these would work in that context? Regarding (2), one dodgy solution is to have the macro expansion utility functions be internals-aware by having a blacklist of "do not expand" macros, but that's pretty close to outright stabilising them. |
To answer (2), in mutagen, I'd like to avoid mutating I'm OK with getting the resulting code if I also get expansion info, and also get a way of expanding macros so I can look into them. |
So I don't know what changes @jseyfried is making to how contexts and scopes are handled, but I agree that sounds like the right place to put this information (about how a particular token was created or expanded). Putting it in spans definitely sounds more workable than trying to wrangle invocations to guarantee you see things pre- or post-expansion, but it also means doing a lot more design work to identify what information you need and in what form. |
One thing I think we need is a way for proc macros to mark what they changed (and for |
iiuc, |
re compiler internals and inspection, I would expect that the results of expansion would be a TokenStream and that could be inspected to see what macro was expanded (one could also inspect the macro before expansion to get some details too). I would expect that 'stability hygiene' would handle access to compiler internals, and that the implementation of that would not allow macro authors to arbitrarily apply that tokens. |
Thanks for this RFC @pierzchalski! I agree that this is definitely a facility we want to provide for macro authors. My primary concern is that this is a surprisingly complex feature and it might be better to try and handle a more minimal version as a first iteration. It might be a good idea to try and avoid any hygiene stuff in a first pass (but keep the API future-compatible in this direction), that would work well with the macros 1.2 work. It is worth considering how to handle expansion order (although it might be worth just making sure we are future-compatible, rather than spec'ing this completely). Consider the following macros uses:
If Then consider a macro that wants to expand two macros where one is defined by the other - it might be nice if the macro could try different expansion orders. I think all that is needed is for the compiler to tell the macro why expansion failed - is it due to a failed name lookup, or something going wrong during the actual expansion stage. Which brings to mind another possible problem - what happens if the macro we're expanding panics? Should that be caught by the compiler or the macro requesting expansion? |
Is there prior art for this? What do the Scheme APIs for this look like? |
The full API provided by `proc_macro` and used by `syn` is more flexible than suggested by the use of `parse_expand` and `parse_meta_expand` above. To begin, `proc_macro` defines a struct, `MacroCall`, with the following interface: | ||
|
||
```rust | ||
struct MacroCall {...}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without getting too deep into a bikeshed, I think something like ExpansionBuilder
would be a better name
|
||
fn new_attr(path: TokenStream, args: TokenStream, body: TokenStream) -> Self; | ||
|
||
fn call_from(self, from: Span) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this to a later iteration
|
||
fn call_from(self, from: Span) -> Self; | ||
|
||
fn expand(self) -> Result<TokenStream, Diagnostic>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error type should probably be an enum of different ways things can go wrong, and where there are compile errors we probably want a Vec
of Diagnostic
s, rather than just one.
``` | ||
|
||
The functions `new_proc` and `new_attr` create a procedural macro call and an attribute macro call, respectively. Both expect `path` to parse as a [path](https://docs.rs/syn/0.12/syn/struct.Path.html) like `println` or `::std::println`. The scope of the spans of `path` are used to resolve the macro definition. This is unlikely to work unless all the tokens have the same scope. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I really like the idea of using a Builder API - it keeps things simple and is future-proof
@nrc yeah, I wrote this before the Macros 1.2 stuff came out and so I assumed strong support for hygiene would be a requirement for making any progress. Having seen how complicated that can be just from a design standpoint, I can understand why it's taking a back seat for the next round of stabilisation! Hopefully I'll strip out the hygiene/ Points, in order:
|
Thanks @chris-morgan! Co-Authored-By: Chris Morgan <[email protected]>
This thread is very long, so apologies if it's been discussed already, but at least it doesn't seem to be covered in the RFC: Why is "Global eager expansion" the only option for having the "eagerness" be a property of the macro definition? Why not have an option when defining a macro, eg. #[proc_macro(expand_input)] If this option is specified, the compiler takes the TokenStream that would be passed as input to the proc-macro and expands it first, before passing it in. |
Indeed, I'm not sure what's the best way to deal with that.
This is a good point! It's still subject to the same limitations as global eagerness, because the input token stream is an arbitrary token stream; giving the compiler the responsibility of eagerly expanding input means either 1) restricting the input to well-formed Rust terms or 2) being able to specify where in the input expansion should occur. The former is too restrictive, and the latter is probably easiest via the proc macro API. |
Is it actually restrictive though? Couldn't you combine this with recursive calls, so for macros where you need to accept tokenstreams that are invalid Rust, and also need to expand macros, define a normal proc-macro that expands to a set of calls to proc-macros that are themselves declared as "eager". This has several advantages over the proposed approach:
|
I don't see how this covers any more use-cases. It would prevent the need for more IPC calls when expanding though. Despite this, I still think the RFC's solution is preferable as it gives macro creators more flexibility. I also think it's better to have eager expansion be more verbose, simply expanding all inputs would probably be done right at the top of the function before anything else, which to me is easier to notice than a flag in the |
@rfcbot fcp postpone Hello everyone; we discussed this RFC in our backlog bonanza. The consensus was that we that we should postpone it, as we don't think we have the bandwidth to see to it right now. We do think that macros need some more work, though, and that this RFC in particular is looking at real problems (even if we're not sure whether it's the right solution or not). We would like to encourage folks to discuss "macros 2.0" when the time comes for us to discuss our upcoming roadmap (one of the procedural changes we have in mind is to make it clearer when we'd be open to bigger proposals). |
Team member @nikomatsakis has proposed to postpone this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to postpone, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC is now postponed. |
Rendered.