rustc: Implement custom derive (macros 1.1) #35957
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -947,6 +947,10 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session, | |||
| middle::dead::check_crate(tcx, &analysis.access_levels); | |||
| }); | |||
|
|
|||
| time(time_passes, "rustc-maro checking", || { | |||
|
☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So, I'm not sure what to do with this PR - there is a lot I'd like to change, and I'm not sure if that will be easier to do before or after landing (it is already quite large, as you say). Ideally we'd fix stuff up before landing, but I expect a lot of things won't affect the API and it will allow users to start iterating if we land early. I also don't want to hold up landing too much since it is going to be a pain to rebase and is important to move quickly here. So, issues:
All of these are things we can fix later, but would be easier to fix on a branch. So, I see three options:
All else being equal I'd prefer the last option. However, if clients are eager to get stuck into this asap and/or it will be really painful, then I don't mind too much going for the first option. I've had a read through the code and other than the points above, I'm pretty happy with it. If we do go for option 1, then I'd like to do a detailed pass, but there didn't seem to be much point in doing that just yet if we go for another option. |
I disagree, other than in the interest of saving time. The current plugin-oriented infrastructure makes a lot of assumptions about the simple nature of name resolution in macros 1.0, whereas using metadata is more akin to the behavior of existing item hierarchies. |
|
OK, it's seeming like there's a lack of communication and breakdown of process here. In particular:
At the moment, the only accepted RFC we have on this topic is Macros 1.1, which was very precise about the naming and APIs that would be included, and the stabilization path. In contrast, the macros 2.0 direction here is currently only in blog post form, and a PR establishing libproc_macro with no accepted RFC. I agree with @nrc that these two things need to be unified. But I am at a loss as to why this was never brought up in the comment thread for the Macros 1.1 RFC. AFAICT, the implementation in this PR matches precisely what the RFC said, which was very carefully written to ensure everyone was on the same page about the API details, including the name of the crate. Why weren't these issues raised at that point? I'm also a little surprised that we have started to land pieces of the larger libproc_macro without an RFC accepted about experimentation there (or even a check-in with the relevant teams). That's not normally how we go about adding major new unstable API surfaces. So, that's where we are. The question now is, what's the best way out of this quagmire? Personally, I think we should:
|
|
(I should say that conversely, I'm was also surprised that this PR ignores the existence of libproc_macro; but OTOH it seems that very few people even realized that libproc_macro had landed, myself included. Like I said, there seems to be a process/communication breakdown prior to this PR.) |
|
For what it's worth, there was a lot of dissent about the name of the initial macro crate [1] [2] [3]. @nrc and I setteld on As for the landing stuff, part of the 'lack-of-RFC' thing was that I spent a decent chunk of my internship on the project, and there was a push to get it landed the compiler before my internship was over (which happened just barely in time). As for actual tokensterams, there has been a |
Yeah, I certainly understand that, and I don't think you are at fault :) We do sometimes allow landing experimental things without an RFC, but we usually have some kind of heads-up to the relevant teams and consensus that the experiment is OK, as well as a broadcast to the larger community about what's happening. It's probably obvious why: once something lands, there's significant inertia around it, so if we really want to operate in a consensus-based way we need consensus even on experiments. Anyway, the people overseeing/reviewing this work should have ensured that communication happened, and planned ahead for the inevitable last-minute rush. It's water under the bridge at this point, regardless. |
|
Yes as @aturon mentioned I was hoping to just follow the design in the RFC, especially the contentious points about the API. I'd personally prefer to not deviate that far from what was accepted. I was also under the impression that we needed more plumbing to use something like libproc_macro for custom derive modes. To be clear, we're not talking about entirely new macro systems here. We're talking about one and only one feature, custom derive. The Once we move everything to token streams then we can fuse the libraries and/or build one on the other, and once we figure out the stable naming I figure we can move everything to that. In the meantime, though, |
|
To clarify my position (which, again, is opinion):
|
|
First, I'm sorry if I came down hard before. I'm mostly just frustrated about the lack of coordination for this PR, which is leading to friction and wasted work.
RFCs merge at a wide range of rates, depending on scope and priority. Narrow, high priority RFCs often move quickly; libs RFCs often move relatively quickly. I don't understand the relevance of this point, in any case.
Looking more closely at the history, you're correct: the name became an explicit "unresolved question", to be resolved prior to stabilization. However:
So, the other part of the process for landing features in Rust is stabilization, where we generally have a tracking issue where people can discuss final details before stabilization (and we have a widely-communicated final comment period, to ensure that everyone's aware). I believe the intent was to sort out the final naming there. Even so, we do need an initial name, and I agree that
I agree that the two should be sharing a definition in the long run. But nothing is leaking an I think the question right now is an engineering choice: given that we'd like to land Macros 1.1 relatively quickly, what's the best option for internal representation for now? Here, I don't know enough of the details to really weigh in; if the existing @alexcrichton, can you please spell out in a bit more detail the rationale for:
The above touches on two of the three issues @nrc raised. The dlsym issue, I can't really speak to. |
One thing to remember is that impls of stable traits on stable types are always stable (AFAIK), so any trait impl on |
That's a good point. I'm not sure how much of a risk that will be for this particular case, but it does seem prudent to newtype and otherwise be very careful about the promises we're making here. I would note that there's impetus to move somewhat quickly here, just in the sense that at a minimum this feature is 12 weeks out from hitting stable, and the earlier we can land it in nightly, the earlier we can start getting feedback. |
|
Gah, I somehow missed @alexcrichton's comment which already spells out some of the impetus for working with ASTs today. So cancel that question :-) |
|
Whoa yes ok I think this may need to slow down a bit. I'd like to clarify some things first:
I implement the name The name has already been debated on the RFC. I was hoping to not reopen debate about the name on this PR (as it just blocks everyone using the feature). The library can be named whatever, but to be clear the name
To reiterate from above again, nothing new is stable in this PR. The discussion on the RFC likely means the name will be changed, and it will be changed before stabilizing. We can do this in a manner that doesn't break any existing
Again, to be clear, I have absolutely no intention of supplanting this. It is a technical fact that token streams are not how I would want to move over to the internal In other words, we could land this today and then change basically everything about how
To reiterate again, this is not happening, nor is it ever planned to happen.
Sure! Today
This is somewhat covered above, primarily by the lack of conversion from an AST to a
I'm not sure I understand this? The |
|
@aturon @alexcrichton Er, I really didn't mean that comment to seem as harsh as it apparently came across. Sorry about that. :/ And also, I clearly didn't read the code closely enough. Sorry about that, too. Thanks for explaining why I was wrong. I'm definitely onboard with this PR now, and excited to have it land. |
|
@cgswords Do you think the quasi-quoting (& eventually other tools) can be in their own plugin crate, with everything that's not a syntax extension in the regular |
Sorry, I was a bit vague in my statement and also misunderstood a bit what was going on here. I think where/how we store the compiled macro isn't too important and the approach here (dlsym/metadata) seems fine. What I don't like is checking the signature of the macro by adding custom code to type checking. We can do what we do for MultiModifier, etc. and use a trait implementation for a function and use regular type checking to enforce the signature. Having read the PR in a bit more detail, this bit seems more orthogonal to the storage of the macro and the implementation of derive than I first thought. I think name resolution and the way we store the macros is mostly orthogonal, either way there seems to be some degree of work to do to store macros in either fashion for plugins 2.0. |
|
@nrc You can't do much better other than by enforcing a trait obligation instead of checking the individual type parts of the signature, i.e. something like this. |
Yes. Sorry for my part of this.
I certainly misunderstood parts of the RFC, it seems. The spec for librustc_macro's TokenStreams doesn't have any implementation, and given the reference to the existing TokenStream, I assumed the implementation would be simply wrapping that. More generally, my reading of the RFC was from the perspective of a maximally minimal slice of the proposed procedural macros functionality, and I assumed the implementation would follow that philosophy.
We talked explicitly about this (when discussing my q2 goals, iirc) and you told me we should experiment first before writing a libmacro RFC and that the level of detail in the blog post was enough to start that experimentation. My suggestion was to write an RFC and you told me not to. Perhaps I misunderstood where this experimentation should take place or you've changed your mind since the macros 1.1 RFC, but I thought evolving a small libmacro in-tree is the only way to do this. There is really only the beginning of the library there and I didn't think it was worth announcing until we had a TokenStream-based macro form to use it with. I missed an opportunity to announce at least to internals that something had landed, but I didn't know there was any other macro work going on, so didn't think it was urgent.
I don't think there is much difference of opinion on the API here, only in the implementation, which is not really touched on in the RFC beyond some discussion of the crate types and how macros are stored. In particular the implementation for TokenStreams is not given and I assumed we'd discuss this after the RFC was accepted and before implementation (which is usually what happens with RFCs, I think it is very rare to bring up implmentation details in an RFC thread). From the RFC:
See above.
I'm not actually too concerned about the name itself, more that we don't have two separate crates (my first proposal in this thread was to change the name of the existing libproc_macro to librustc_macro and merge in what is in this PR). We could also keep two separate crates, but have rustc_macro re-export a subset of proc_macro (since I think the two crates have different long term goals).
This is important, and we should.
Agreed. But I'm also happy to land as is, as long as there is momentum to address the issue quickly, rather than leave it open once we have something that clients like Serde can use. |
Hmm, so this seems to be key to the choice of implementation of TokenStream. We certainly have the constraints that the clients must manipulate Strings and the deriving infrastructure wants to see AST. I agree we should avoid changing too much of that. However, it seems that this does not necessarily inform the implementation of TokenStream - we must go from AST to String to AST somewhere, but I don't think it is necessarily easier to do it one way vs another. In particular in this PR we go from String to AST in |
When a macro is registered with the registrar, its implementation (usually a function) must satisfy a trait bound, that only happens if the function signature matches the function type for which there is an |
| //! new custom derive modes through `#[rustc_macro_derive]`. | ||
| //! | ||
| //! Added recently as part of [RFC 1681] this crate is currently *unstable* and | ||
| //! requires the `#![feature(rustc_macro)]` directive to use. Eventually, |
|
@bors: r=nrc |
|
📌 Commit 66ec18f has been approved by |
|
@bors: r- oh right haven't fixed tests yet. |
This commit is an implementation of [RFC 1681] which adds support to the compiler for first-class user-define custom `#[derive]` modes with a far more stable API than plugins have today. [RFC 1681]: https://github.com/rust-lang/rfcs/blob/master/text/1681-macros-1.1.md The main features added by this commit are: * A new `rustc-macro` crate-type. This crate type represents one which will provide custom `derive` implementations and perhaps eventually flower into the implementation of macros 2.0 as well. * A new `rustc_macro` crate in the standard distribution. This crate will provide the runtime interface between macro crates and the compiler. The API here is particularly conservative right now but has quite a bit of room to expand into any manner of APIs required by macro authors. * The ability to load new derive modes through the `#[macro_use]` annotations on other crates. All support added here is gated behind the `rustc_macro` feature gate, both for the library support (the `rustc_macro` crate) as well as the language features. There are a few minor differences from the implementation outlined in the RFC, such as the `rustc_macro` crate being available as a dylib and all symbols are `dlsym`'d directly instead of having a shim compiled. These should only affect the implementation, however, not the public interface. This commit also ended up touching a lot of code related to `#[derive]`, making a few notable changes: * Recognized derive attributes are no longer desugared to `derive_Foo`. Wasn't sure how to keep this behavior and *not* expose it to custom derive. * Derive attributes no longer have access to unstable features by default, they have to opt in on a granular level. * The `derive(Copy,Clone)` optimization is now done through another "obscure attribute" which is just intended to ferry along in the compiler that such an optimization is possible. The `derive(PartialEq,Eq)` optimization was also updated to do something similar. --- One part of this PR which needs to be improved before stabilizing are the errors and exact interfaces here. The error messages are relatively poor quality and there are surprising spects of this such as `#[derive(PartialEq, Eq, MyTrait)]` not working by default. The custom attributes added by the compiler end up becoming unstable again when going through a custom impl. Hopefully though this is enough to start allowing experimentation on crates.io! syntax-[breaking-change]
alexcrichton
force-pushed
the
macros-1.1
branch
from
66ec18f
to
ecc6c39
Compare
|
@bors: r=nrc |
|
📌 Commit ecc6c39 has been approved by |
rustc: Implement custom derive (macros 1.1) This commit is an implementation of [RFC 1681] which adds support to the compiler for first-class user-define custom `#[derive]` modes with a far more stable API than plugins have today. [RFC 1681]: https://github.com/rust-lang/rfcs/blob/master/text/1681-macros-1.1.md The main features added by this commit are: * A new `rustc-macro` crate-type. This crate type represents one which will provide custom `derive` implementations and perhaps eventually flower into the implementation of macros 2.0 as well. * A new `rustc_macro` crate in the standard distribution. This crate will provide the runtime interface between macro crates and the compiler. The API here is particularly conservative right now but has quite a bit of room to expand into any manner of APIs required by macro authors. * The ability to load new derive modes through the `#[macro_use]` annotations on other crates. All support added here is gated behind the `rustc_macro` feature gate, both for the library support (the `rustc_macro` crate) as well as the language features. There are a few minor differences from the implementation outlined in the RFC, such as the `rustc_macro` crate being available as a dylib and all symbols are `dlsym`'d directly instead of having a shim compiled. These should only affect the implementation, however, not the public interface. This commit also ended up touching a lot of code related to `#[derive]`, making a few notable changes: * Recognized derive attributes are no longer desugared to `derive_Foo`. Wasn't sure how to keep this behavior and *not* expose it to custom derive. * Derive attributes no longer have access to unstable features by default, they have to opt in on a granular level. * The `derive(Copy,Clone)` optimization is now done through another "obscure attribute" which is just intended to ferry along in the compiler that such an optimization is possible. The `derive(PartialEq,Eq)` optimization was also updated to do something similar. --- One part of this PR which needs to be improved before stabilizing are the errors and exact interfaces here. The error messages are relatively poor quality and there are surprising spects of this such as `#[derive(PartialEq, Eq, MyTrait)]` not working by default. The custom attributes added by the compiler end up becoming unstable again when going through a custom impl. Hopefully though this is enough to start allowing experimentation on crates.io!
| @@ -302,7 +305,6 @@ declare_features! ( | |||
| (removed, struct_inherit, "1.0.0", None), | |||
| (removed, test_removed_feature, "1.0.0", None), | |||
| (removed, visible_private_types, "1.0.0", None), | |||
| (removed, unsafe_no_drop_flag, "1.0.0", None) | |||
There was a problem hiding this comment.
I am curious, was this line removed intentionally?
There was a problem hiding this comment.
Heh, couple of pages above (line 94) there's a comment // Don't ever remove anything from this list; set them to 'Removed'., but people do this anyway.
There was a problem hiding this comment.
Oops yes this was not intentional, just a botched merge.
There was a problem hiding this comment.
FWIW library features do get removed all the time so I don't think we need to bother anymore with this list.
This feature was accidentally removed in rust-lang#35957.
…rielb1 Add back feature accidentally removed This feature was accidentally removed in rust-lang#35957.
Macros 1.1 Tested with serde-rs/serde#530. This should be able to merge independently of rust-lang/rust#35957. r? @alexcrichton
This feature was accidentally removed in rust-lang#35957.
Adds a `ProcMacro` form of syntax extension This commit adds syntax extension forms matching the types for procedural macros 2.0 (RFC #1566), these still require the usual syntax extension boiler plate, but this is a first step towards proper implementation and should be useful for macros 1.1 stuff too. Supports both attribute-like and function-like macros. Note that RFC #1566 has not been accepted yet, but I think there is consensus that we want to head in vaguely that direction and so this PR will be useful in any case. It is also fairly easy to undo and does not break any existing programs. This is related to #35957 in that I hope it can be used in the implementation of macros 1.1, however, there is no direct overlap and is more of a complement than a competing proposal. There is still a fair bit of work to do before the two can be combined. r? @jseyfried cc @alexcrichton, @cgswords, @eddyb, @aturon
This commit is an implementation of RFC 1681 which adds support to the
compiler for first-class user-define custom
#[derive]modes with a far morestable API than plugins have today.
The main features added by this commit are:
rustc-macrocrate-type. This crate type represents one which willprovide custom
deriveimplementations and perhaps eventually flower into theimplementation of macros 2.0 as well.
rustc_macrocrate in the standard distribution. This crate willprovide the runtime interface between macro crates and the compiler. The API
here is particularly conservative right now but has quite a bit of room to
expand into any manner of APIs required by macro authors.
#[macro_use]annotations onother crates.
All support added here is gated behind the
rustc_macrofeature gate, both forthe library support (the
rustc_macrocrate) as well as the language features.There are a few minor differences from the implementation outlined in the RFC,
such as the
rustc_macrocrate being available as a dylib and all symbols aredlsym'd directly instead of having a shim compiled. These should only affectthe implementation, however, not the public interface.
This commit also ended up touching a lot of code related to
#[derive], makinga few notable changes:
derive_Foo. Wasn'tsure how to keep this behavior and not expose it to custom derive.
have to opt in on a granular level.
derive(Copy,Clone)optimization is now done through another "obscureattribute" which is just intended to ferry along in the compiler that such an
optimization is possible. The
derive(PartialEq,Eq)optimization was alsoupdated to do something similar.
One part of this PR which needs to be improved before stabilizing are the errors
and exact interfaces here. The error messages are relatively poor quality and
there are surprising spects of this such as
#[derive(PartialEq, Eq, MyTrait)]not working by default. The custom attributes added by the compiler end up
becoming unstable again when going through a custom impl.
Hopefully though this is enough to start allowing experimentation on crates.io!