rust 🚀 - Tracking issue for "Macros 1.1" (RFC #1681)

I've updated the issue description with a checklist of what needs to be done. It's likely not exhaustive but it should get us 90% of the way there hopefully

alexcrichton on 22 Aug 2016

Ok, I'm going to look into this today and see how far I get.

alexcrichton on 22 Aug 2016

❤16 🎉7

cc https://github.com/rust-lang/rust/pull/35957, an implementation

alexcrichton on 24 Aug 2016

From #35957: we should bikeshed the name of the librustc_macro crate some more. In particular, this is intended to be a long-lived crate which will have essentials for all macro authors in it, so limiting to rustc_macro (which in my mind, at least) is just about the 1.1 idea seems bad. I'd previously wanted libmacro for this, but since macro is a reserved word (and we might want it as a keyword in the future) that is impossible. @cgswords and I have been using libproc_macro, and I think that is not a bad name, although I am not 100% happy with it.

nrc on 29 Aug 2016

@nrc: Okay, my immediate thoughts on naming:

extern crate macros; - short and sweet, but might be read as containing macros, rather than support code for writing them
extern crate macro_runtime; - exactly what it says on the tin
extern crate metarust; - write Rust, about Rust, for operating on Rust
extern crate bikeshed; - with procedural macros, you can have whatever color of Rust you want!
extern crate macrame; - sounds like "macro make[r]"; possibly better left for a future "nice" API over the raw library.

eternaleye on 30 Aug 2016

😄7 👍1

@nrc It seems like an important aspect of this question is the naming of our various macro styles over all -- in particular, if we go with libproc_macro, we're making a hard commitment to the "procedural macro" terminology. I don't have a strong opinion here, but I'm not sure if we've openly explored the space of terminology.

To be clear, are you thinking that today's macro_rules would simply be "macros", i.e. the default thing we mean by macros, while you have to qualify "procedural macros"? That seems like a pretty reasonable plan to me. And in that world, I'd argue that libproc_macro is better than libmacros.

aturon on 30 Aug 2016

My thinking here is that all macros are "macros", and when we need to make a distinction based on the implementation (the usage should be exactly the same for all kinds) we use "procedural macros" vs "macros by example". I would like to banish "syntax extension" and "compiler plugin" entirely (and one day re-use the latter for actual plugins).

But, yes, I strongly want to get behind the "procedural macro" terminology.

nrc on 30 Aug 2016

👍2

@nrc Makes good sense to me! While "macros by example" is a bit unwieldy, it's also a very evocative/intuitive term. My one worry about "procedural macro" is that "procedure" is not a thing in Rust. I wonder if there's a way of making the connection more direct. "Function macro" isn't quite right, but maybe gives you a sense of what I mean?

aturon on 30 Aug 2016

Yeah, it's not quite perfect, but given that it is a well-known/used term outside of Rust I think it is better than coining our own term. "Programmatic macro" is possible, but I prefer "procedural".

nrc on 31 Aug 2016

Perl's term for the closest equivalent it has to "procedural macros" is "source filters", which (especially with the shift from AST to tokens) is a pretty fitting description.

eternaleye on 31 Aug 2016

Perhaps 'Syntax transformers' or 'programmatic macros' would work well as names? I don't have a problem with procedural macros however.

mystor on 31 Aug 2016

"Procedural" is already what people call this, and I think it's clearly understood what it means, especially in contrast to "macros by example." I wouldn't worry about trying to find a different name.

jimmycuadra on 31 Aug 2016

👍3

I like the term "procedural macro" for regular use (or maybe "custom macro"). I particularly like using the word _macro_ so that it's clear that they can (eventually...) be used in the same way as "regular macros". For this reason, I don't like "source filters" (I also expect a filter to just drop data, not transform it, though I know the term is used for both).

I am fine with either libproc_macro or libmacros. I sort of prefer the latter just because I don't love having _ in crate names when it can be readily avoided. =)

nikomatsakis on 31 Aug 2016

👍4

One question: do we ever expect to have "support routines" that could be used from non-procedural macros? I know of no such plans, but if we did, and we wanted them in the same crate, then libmacros would be a better name. =)

(I am thinking a bit about e.g., @dherman's comment from the eager-expansion RFC.)

nikomatsakis on 31 Aug 2016

@nikomatsakis: A related but subtly different question is a use case I brought up in https://github.com/rust-lang/rfcs/pull/1561#discussion_r60459479 - will we want procedural macros' implementation functions to be able to call other procedural macros' implementation functions?

I can easily see wanting to allow one custom derive to call another, and that would essentially make procedural macro definitions themselves capable of being used as such "support routines"

But yes, I think @dherman's gensym example is pretty compelling. Of course, if the answer to my question above is "yes", gensym is both a macro (which could be used by macros-by-example) and a support function (which could be used by procedural macros).

eternaleye on 31 Aug 2016

I have a cargo PR https://github.com/rust-lang/cargo/pull/3064 which should check all the "cargo integration" boxes in the checklist.

dtolnay on 1 Sep 2016

I left a comment on the cargo PR, but I think we want a different type of _dependency_, not just a different type of _package_. Firstly, I think this just is just better aestheticly and ergnomicly, but that's just my opinion. But I have two concrete reasons too.

In a future with public and private deps, it's important to know that its to know that public deps of procedural deps and regular public deps need not coincide. E.g.
In a future with inline procedural macros/quasi-quoting, any library can be used at compile time.
> will we want procedural macros' implementation functions to be able to call other procedural macros' implementation functions?

As this shows the same crate could be a regular dep of another crate of procedural macros, or a macro dep of another crate.

Ericson2314 on 1 Sep 2016

👍3

Does serde work?

Yes https://github.com/serde-rs/serde/releases/tag/v0.8.6

(except for container attributes in some cases #36211)

dtolnay on 2 Sep 2016

Awesome, thanks for the updates @dtolnay!

alexcrichton on 2 Sep 2016

Are there docs on these macros? I suppose, the only example of using them is in serde, am I right?

White-Oak on 2 Sep 2016

OK cargo stuff has landed. That's fine, but it would be nice to revisit https://github.com/rust-lang/rust/issues/35900#issuecomment-243976887 sometime before stabilization. [For what its worth, I meant to bring this up in the original RFC but forgot.]

Ericson2314 on 6 Sep 2016

I think "macros by example" and "procedural macros" could be better catagorized as "declarative macros" and "imperative macros" respectively. This gives informative parallels to the more widely known declarative/imperative categorization of programming languages. As imperative is treated as a superset or synonym of procedural, it should be close enough for people used to "procedural macro" terminology to make the jump. It should also avoid any confusion with procedure/function/method concepts in rust itself.
This naming scheme gives us a macro_imp crate and module to parallel macro_rules. macro_rules could eventually become a module of a more general macro_dec crate.

@nrc, When you refer to "actual plugins" are you including things like metacollect and clippy, things like rustw, rustfmt, and the Rust Language Server, or some other category of program?

jessstrap on 9 Sep 2016

👍4

For what it’s worth, I mildly dislike the "by example" name (since $foo patterns aren’t "examples" in my mind). Declarative vs imperative sounds better to me.

SimonSapin on 9 Sep 2016

👍1

Playing around with this I've noticed one issue. It looks like Rust's provided derives sometimes add attributes which the compiler knows to ignore. This context gets lost when it goes through a custom derive. I'd expect the identify function given as a custom derive to be a no-op, but it will cause errors around the #[structural_match] attribute getting added.

Reproduction script

(In a crate named demo_plugin)

#![feature(rustc_macro, rustc_macro_lib)]

extern crate rustc_macro;

use rustc_macro::TokenStream;

#[rustc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {
    input
}

(in another crate)

#![feature(rustc_macro)]

#[macro_use] extern crate demo_plugin;

#[derive(PartialEq, Eq, Foo)]
struct Bar {
    a: i32,
    b: i32,
}

Removing the #[derive(Eq)] causes everything to work fine.

sgrif on 9 Sep 2016

@sgrif ah yes indeed thanks for reminding me!

So there's a few things going on here:

With #[derive(PartialEq, Eq)], the compiler is silently adding #[structural_match] because it statically understands that the PartialEq derivation does indeed fulfill that contract.
I believe a similar attribute arises with #[derive(Copy, Clone)] with #[rustc_copy_clone_marker] being added.
Currently this works because the span of these attributes says "allows internal unstable". We lose the span information, however, when parsing and reparsing.

So, some solutions we could do:

Conventionally say custom derive comes first, e.g. #[derive(Foo, Eq, PartialEq)]
Rely on custom derive to omit these attributes
Don't emit custom attributes if we detect custom derive
Use an entirely different mechanism in the compiler to communicate what these attributes are saying, but not through the AST itself.

I'd be in favor of either not emitting these attributes or using a different mechanism. This is really tricky, though, because #[derive(Copy, Foo, Clone)] also needs to work (where custom code runs in the middle that could change definitions).

My preferred course of action would be to just not emit these attributes if we detect custom derive. Even that, though, may not be trivial. For now a convention of "custom first and standard last" should suffice, but I do think we should fix this before stabilizing.

alexcrichton on 9 Sep 2016

Disclaimer: I only have an outside view of the compiler - so there is a lot I don't know. ^^

But from what I understand, the current approach of macros 1.1 custom derive is more like a temporary workaround. Temporary workaround might translate to "quite a while". But in the long run (=macros 2.0) we won't do the string-parsing-round-trip anymore, which currently leads to the loss of span-information. I wonder if these hidden attributes in the AST were such a bad thing in a macros 2.0 world. Maybe someone with more inside-knowledge about the compiler can tell. If these hidden attributes actually make sense in that future world, I would argue to go for the workaround by conventionally putting custom derives up front. The whole thing already is a workaround anyway, isn't it?

colin-kiegel on 9 Sep 2016

@colin-kiegel I believe you're right in the sense that what we're doing right now is _not_ future-proof. Let's say you have, for example:

#[derive(Eq, Foo, PartialEq)]

Today we'd add the Eq implementation, then run the custom code for Foo, and then add an implementation of PartialEq. The structure could _change_ between Eq and PartialEq, so the #[structural_match] added by Eq may not actually be correct after Foo runs.

In that sense I agree that they're not necessarily future-proof in any case!

alexcrichton on 9 Sep 2016

My feeling is, that custom derives which rely on structural changes generally won't compose very well, independently of those hidden attributes. Will a v2.0 custom derive be able to change the structure of the item, or will it be somehow limited to decorating only?

colin-kiegel on 9 Sep 2016

Yeah the ability will always be there I believe (inherent in the TokenStream -> TokenStream interface) although I suspect any reasonable implementation of #[derive] would retain the structure of the original structure.

alexcrichton on 10 Sep 2016

I don't suppose we could require that the output tokenstream not contain the struct itself, to make sure that the structure doesn't get changed? The input struct TokenStream would be an immutable prefix? The big problem would be making sure to ignore the unrecognized attributes which the plugins are using after the build is complete. Perhaps each #[derive()] can have a prefix (say #[derive(Foo)] has the prefix Foo_) which the attributes which they understand must start with, and after processing each custom derive, we strip off those attributes?

mystor on 10 Sep 2016

@mystor yeah the problem with that approach is unrecognized attributes, which is why we have the entire struct as input. That's generally more flexible than relying on a particular prefix/suffix/registration/etc.

alexcrichton on 10 Sep 2016

If a v2.0 custom derive could mark custom attributes as _used_, it could be limited to _read-only_ access to the rest of the items token stream. This way better composability of custom derives could be guaranteed IMO. If a v2.0 macro needs to change the structure of an item it would have to ue another api, but not custom derive. This way the problem with #[structural_mach] and ordering of (custom) derives would only be present in macros 1.1. Would that make sense?

colin-kiegel on 10 Sep 2016

Another issue. If a struct has two different custom derives on it and the second one panics, the error span will point to the first one, not the one which panicked.

Reproduction Script

In a crate called demo_plugin

#![feature(rustc_macro, rustc_macro_lib)]

extern crate rustc_macro;

use rustc_macro::TokenStream;

#[rustc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {
    input
}

#[rustc_macro_derive(Bar)]
pub fn derive_bar(input: TokenStream) -> TokenStream {
    panic!("lolnope");
}

In another crate

#![feature(rustc_macro)]

#[macro_use] extern crate demo_plugin;

#[derive(Foo, Bar)]
struct Baz {
    a: i32,
    b: i32,
}

The error will highlight Foo even though Bar panicked.

sgrif on 10 Sep 2016

Thanks for the report @sgrif! I've updated the description of this issue and I hope to keep track of all outstanding issues related to macros 1.1 there as well.

alexcrichton on 12 Sep 2016

Hmm, the interaction of custom derive with #[structural_eq] is something I hadn't thought of before. It rather bothers me!

It seems to me that having a way to "append" text to a token stream might be a better interface at the end of the day...it would preserve span information and avoid this problem, no?

nikomatsakis on 13 Sep 2016

👍2 😕1

One advantage of the more general interface is that it allows packages to have attributes on fields, which can be stripped when the macro runs. This is the only real use-case I know of for allowing custom derive macros to alter the original item.

I think that the interface question comes down to whether we want custom derive to be 'just another macro', in which case it seems important to have the same interface as other procedural macros (where we want to modify the original item). Or whether it should be its own thing with a special interface, in which case the more restrictive (append) interface makes sense.

I'll note that syntax extensions long had a distinction between modifiers and decorators, and I think that everyone involved really hated that distinction. I'm therefore a bit reluctant to go down the path of having custom derive be a bit special (an alternative that has been discussed is some kind of very special custom derive, possibly even a declarative format).

nrc on 13 Sep 2016

@nrc well, it can still be tokenstream -> tokenstream, where we just don't expose a new method that starts from scratch, right?

nikomatsakis on 13 Sep 2016

I agree that it must be possible to have custom attributes on fields for a custom derive to really make sense. But I believe there may be many ways this could be done with the "append"-style - this should of course be discussed. I'd definitely prefer the append style, because I prefer a world where derive macros don't change the item and are composable, plus it solves the #[structural_eq] issue. This is just right amount of freedom IMO.

If people didn't like that destinction I would like to ask why, because obviously there was enough reason to make this distinction before, wasn't it?

colin-kiegel on 13 Sep 2016

(I guess what I suggested is just a temporary hack, doesn't really address the longer term composability problem.)

nikomatsakis on 13 Sep 2016

My various libraries currently have macros that look like foo!(Bar, parameters...), which generate a struct Bar from the parameters.

During some discussion on IRC, we had an idea to write #[derive(Foo)] #[params] struct Bar; instead, and replace foo! with a #[derive(Foo)] that would generate the body of the struct.

It's obviously not a strong argument, but I really liked that idea, as it's clearer for the user that a struct is being built.

tomaka on 13 Sep 2016

~~I wonder if we could rework the #[structural_match] to be placed on the generated impl instead. Would probably be fairly easy, actually.~~

(Doesn't really solve the problem)

nikomatsakis on 13 Sep 2016

Worth noting that both Serde and Diesel make a lot of use of custom attributes on fields, so there is a definite need for custom derive to allow for replacement.

jimmycuadra on 13 Sep 2016

So actually maybe I am just not thinking straight about the #[structural_match] issue. After all, what can a custom derive do?

If the custom derive inserts the #[structural_match] attribute falsely, it should fail the stability check. If not, that seems like a bug itself! (Given the complex and wacky way that this code works, though, that wouldn't surprise me.)
If the custom derive removes it, no harm done.

nikomatsakis on 13 Sep 2016

Sorry for writing tons of small comments and thinking aloud, but there is one other concern. Although a custom derive may not be able to "falsify" a #[structural_match] annotation (because it would wind up without the "magic span"), it would probably wind up screwing up the span of any existing annotation, unless the derives are applied in the correct order, which is unfortunate. Basically an instance of the non-composability that @colin-kiegel has been talking about, but without any attempt to modify the struct in flight.

(In other words, since we rely on the span to judge whether stable stuff can be used, losing span information can cause some tricky problems there.)

EDIT: OK, reading back I see that I just rederived what @sgrif already reported. Sorry again. ;)

nikomatsakis on 13 Sep 2016

It's also kinda gross, because it means we're exposing unstable implementation details to stable code. Ideally stable code would never even know that the #[structural_match] annotation exists.

mystor on 13 Sep 2016

👍1

@nikomatsakis well, one way or another, we need to enforce different constraints depending on whether the macro is intended to be a custom derive or some other kind. That means some separate treatment (and different semantics), whatever the signature of the function.

@colin-kiegel

I'd definitely prefer the append style, because I prefer a world where derive macros don't change the item and are composable, plus it solves the #[structural_eq] issue. This is just right amount of freedom IMO.

I think that mutating macros can be composable, although of course the terms of composability are different. There must clearly be a set of pre- and post-conditions on the operation of derives, either enforced by the compiler or by convention. Non-mutation seems like one extreme on the spectrum of invariants we might choose here, and note that already we are discussing ways in which that can be softened (e.g., marking attributes used). I think in general, we would prefer the simplest conditions and to have them enforced by the compiler. However, this is subsumed somwhat by the question of how specially custom derive should be treated.

If people didn't like that destinction I would like to ask why, because obviously there was enough reason to make this distinction before, wasn't it?

I don't believe there was strong motivation at the time. I think it was easy to implement and 'seemed like a good idea'. It has been disliked since because it makes the implementation more complex, it adds a distinction for macro authors which is usually irrelevant, and it made macros less flexible by having to choose whether to modify or decorate.

nrc on 13 Sep 2016

I would very much like to consider the long-term design of custom derive, and ensure we are heading in the right direction. It seems to me that the constraints of the 1.1 solution and the desire to do as much as possible as soon as possible are muddying the waters here and we are losing sight of the larger vision.

nrc on 13 Sep 2016

👍2

I agree with @jimmycuadra in that it seems like supporting custom attributes in one way or another is a hard requirement. @nikomatsakis is also right though in that the current treatment of #[derive(PartialEq, Eq)] is subpar and we shouldn't stabilize it. Finally, @mystor has a very good point that custom derive modes shouldn't even know about this magical attribute. We're bound to want to add more in the future and I don't want macros 1.1 to prevent us from doing that.

Also echoing @nrc's sentiment about the long term design of custom derive, I think a lot of this boils down to how #[derive] actually works. If and when we support arbitrary attributes I think @nrc has a good point about only having modifiers and not having decorators, but #[derive] is pretty special where a custom derive is not defining a new attribute but rather just tacking on to an existing one.

Right now the implementation has a strict left-to-right expansion of #[derive] modes. All internal derive modes are expanded in a loop and whenever a custom derive mode is hit we reserialize, expand, then go back to stage 1. This in turn means that the compiler may run the #[derive] attribute _multiple times for one type definition_. That leads to a bunch of hairiness.

One proposal I might have is to tweak the expansion order of #[derive]:

First, all custom derive attributes from macros 1.1 are expanded one by one. That is, if you have #[derive(Clone, Foo)] we'd first derive Foo where the struct had a #[derive(Clone)] annotation. If that #[derive] persisted we'd then derive the custom built-in trait Clone.
Second, all derive attributes unknown by the compiler are expanded to #[derive_Bar] attributes. This is just a backwards compatibility hack we should remove at some point, and is how attributes used to get expanded.
Finally, the compiler expands all known and built-in #[derive] attributes

This has the surprising effect that you're not expanding left-to-right, but then again remember that this is only #[derive]. This gives the compiler maximal knowledge about the struct definition and when it's expanding builtin traits we know that the structure of the type will never change.

How does that sound? I believe it solves all the constraints here?

@nikomatsakis I'm not sure that the strategy of placing the attribute on the impl will work because other custom derive modes could in theory change the layout of the struct, even the types of the fields. This would violate the compiler's assumptions when it first expanded I think.

alexcrichton on 14 Sep 2016

Has the order in which derives are processed ever been officially declared as being left-to-right, via the Rust reference or anything? More generally, does the order of attributes ever matter? It sounds like it's just incidental that it was implemented that way, and macro authors shouldn't have been relying on a left-to-right order. Alex's proposal of processing custom derives first so that they never see magical attributes added by the compiler makes a lot of sense.

jimmycuadra on 14 Sep 2016

I'd just like to add that I don't like the idea that custom derives can change the layout of the struct. I would want to be able to use this for something which is safety-sensitive. As an example, consider the #[derive(Trace)] implementation used by rust-gc.

#[derive(Trace)]
struct Foo {
    a: Gc<i32>,
}

Expanding to:

struct Foo {
    a: Gc<i32>,
}

unsafe impl Trace { // NOTE: Strawman impl
    unsafe fn trace(&self) { Trace::trace(&self.a) }
}

However, if we allow changing the fields in the struct, we can define an Evil custom derive:

#[derive(Evil)]
struct Foo {
    a: Gc<i32>,
}

Expanding to:

struct Foo {
    a: Gc<i32>,
    b: Gc<i32>,
}

Which, if we combine them:

#[derive(Trace, Evil)]
struct Foo {
    a: Gc<i32>,
}

Expanding to:

struct Foo {
    a: Gc<i32>,
    b: Gc<i32>,
}

unsafe impl Trace {
    unsafe fn trace(&self) { Trace::trace(&self.a) }
}

Which is an unsound implementation of Trace. When used with rust-gc, this allows for b to be a dangling reference, which is horribly unsafe and unsound. This means that Trace isn't a safe thing to #[derive] anymore on a type, which is highly unfortunate.

mystor on 14 Sep 2016

👍3 👎1

I personally feel that any well-behaved #[derive] will not modify the layout/makeup of a structure, and if it does then you're just out of luck. The ability for a custom derive to drop attributes is critical, and giving that up is a non-starter. Additionally, other implementations that involve some form of whitelisting or whatnot diverge quite greatly from the simple interface we have today.

Put another way, I don't feel that the "purity" of having #[derive] never modify the struct is worth the cost.

alexcrichton on 14 Sep 2016

👍1

I just wonder if there will be a way for it to drop attributes without allowing it to add or remove fields (for example, verifying that the fields are the same in the re-parsed struct as the original struct, and erroring if they aren't, but not complaining if other things are changed).

mystor on 14 Sep 2016

I have a bad feeling about allowing derive to change the struct. The example of @mystor is what I had in mind when I was talking about composability .. on a high level (may be it was not the right term).

I think people will exploit this, if it is available. And this will force consumers to reason about details of custom derive implementations and their order of execution.

I would prefer if I can say 'hey, I don't know what this derive does, but I understand the other one' without interdependency. Otherwise it will be a pain in the toe and I believe it will happen.

colin-kiegel on 14 Sep 2016

Is a procedural macro doing something malicious really any different from any crate you use doing something malicious? Any crate can have unsafe code in it that does something it shouldn't. Seems like this case just falls in with how you determine the trustworthiness of any code you didn't write yourself, e.g. community reputation, inspecting the source yourself, etc.

jimmycuadra on 14 Sep 2016

👍1

I don't think crates are going to try to do something malicious, rather I expect them to be "clever" and do neat tricks to make their implementation more efficient or because they can, and for it to break other custom derive implementations. I wouldn't be super surprised if some custom derives start adding fields to structs which are only used in their implementations because they can, and those then break something like Trace.

mystor on 14 Sep 2016

@mystor That sounds relevant in theory but remember you actually need to provide all the fields of a structure in Rust, so it's much less likely to work silently like that, than in C++, for example.

eddyb on 14 Sep 2016

@alexcrichton

One proposal I might have is to tweak the expansion order of #[derive]

I like this idea. Perhaps the thing to state in terms of the documentation is simply that the order of expansion is "undefined". We happen to expand PartialEq/Eq last these days, but there is no strict reason to do so.

As for derives modifying the struct definition, I agree that sounds kind of subtle, but it doesn't bother me too much. I would also think that "auto-inserting" fields could be quite handy -- but I'd prefer for such modifiers to be distinct attributes rather than put into the derive listing, primarily because we don't want people to rely on the order of expansion just now.

PS. I would seriously consider using a (deterministic) RNG seeded by the crate hash or something like that to reorder the expansion of the user's custom derivations, so that people cannot rely on the expansion order. I've always wanted to do this in some context to prevent implicit dependencies but never gotten the chance. ;)

nikomatsakis on 16 Sep 2016

👍3

EDIT: I've changed my mind, and don't see any reason to disallow mutating the struct anymore, but here's my original comment for context

So, from my understanding, these are the arguments for allowing custom #[derive] to mutate the struct:

It could be useful in some cases (haven't seen any examples, but I believe that they exist)

We want to be able to remove attributes once they have been used

Giving more power to custom derive authors

While the arguments for adding limitations to custom #[derive] implementations (such as requiring the name of the struct and the fields/names of fields in the struct to stay the same) are:

It allows for the code generated by a custom #[derive] to depend on the structure of the type it is being derived on for soundness (e.g. #[derive(Trace)] from rust-gc which _must_ see the actual backing type, or it is unsound, potentially in a subtle use-after-free-y way)

It reduces the likelihood of implicit dependencies in macro expansions, as there is less information conveyed between them through the struct

In my opinion, the ability to write sound custom derive implementations which generate unsafe trait impls or code which depends on unsafe code is extremely important, and I believe that there are ways to achieve most of the abilities in the first section (with the exception of the ability to add struct fields) in a safe way. If we don't have some form of constraint, I don't think that crates like rust-gc will be possible to implement in a safe way. I have two ideas:

Idea 1

Before running a custom derive pass, read in the name of the struct, and the names of each of the fields. When the pass is complete, and the struct is re-parsed, check if the name of the struct is the same, and if the names of each of the fields (and count of the fields) are the same. if they are not, raise an error and kill the build.

This would ensure that the basic structural properties which we expect custom derive plugins to depend on are not broken, and means that we have more, sound, custom derive plugins. This also has the advantage of being backwards compatible with the current approach, so if we decide we like it better in the future, we can just switch over and break nobody's code. It also handles the unused attribute case, just like today.

Idea 2

Give every custom derive plugin the same input TokenStream (the original text written in the program). When the result is re-parsed, record which attributes are still present on the output struct. If an attribute is present in every output tokenstream, then complain about an unused attribute.

This means that it is impossible to have ordering dependencies (as conceptually, every custom derive plugin works off of the same original object), and it also makes it impossible to screw up the structure of the plugin. I like this idea as it ensures that every custom derive acts in a mostly-sane way, only generating new items based on the existing struct. This would also likely be easy to transform into whatever solution we could transform the current solution into.

TL;DR

In summary, I would like to understand what the particular advantage is of allowing mutating structs, and why it outweighs the safety concerns of making safe #[derive(Trace)] and always-correct #[derive(Serialize)] etc. possible. I'm sure that if we end up going down the mutating structs route, there will be a good reason, but I will be very sad to change the name of my Trace custom derive to #[derive(unsafe_Trace)].

mystor on 16 Sep 2016

👎3 👍2

I find @alexcrichton's solution to be a good tradeoff. I would definitely expect that any changes some custom derives do, the default ones are applied to it.

Even though @mystor has a good point that it can lead to unpleasant surprises, having the possibility to change the struct seems mandatory. On the other hand, crates combining the use cases of procedural macros, unsafe code _and_ security concerns seem rather uncommon.

Off topic: will this implementation provide a way for the macro to report errors gracefully?

dragostis on 17 Sep 2016

I like the idea of @nikomatsakis to randomize the expansion order of custom derives. That would definitely help to prevent "bad" custom derives from becoming too popular - and should not be too much effort to do.

@mystor a third option would be to do these safety-checks only once after all derives are applied. That would not be 100% sound (two derives could add and remove the same field), but in terms of a heuristic countermeasure it should be sufficient to prevent any attempts to change the struct definition in a custom derive in the first place.

colin-kiegel on 17 Sep 2016

I really don't see the concern around struct modification. A field cannot be added invisibly, something will have to care during initialization. If you're _really_ worried about that happening, you can write your derive to generate code which fails to compile if it doesn't see the whole struct pretty easily.

sgrif on 17 Sep 2016

👍3

@sgrif probably true in _most_ cases - but not so much if you also derive and use the default trait, or something equivalent.

colin-kiegel on 17 Sep 2016

@sgrif PS: It is true that most rust authors probably understand what's going on in their own code and may therefore not be surprised by struct alterations if they chose to use such a macro on purpose.

The general use case for applying macros on structs certainly is a _combination_ of struct-alterations + decorations. But I expect the general ratio to be "a lot of decorations" with "only a few alterations". It's good to have a clear separation here, because that improves readability and flexibility.

flexibility: Say you want to apply two custom derives which both do both, i.e. alterate and decorate. No matter how you order them you may not get the result you want. However if the alteration happens via a different mechanism and the decorations are all applied afterwards you have the flexibility to combine multiple alterations and decorations in a more controllable way.
readability: If you read someone elses code and there are 10 derives applied to a struct and one of them alterates the structure, you'll need more time to figure that out.

colin-kiegel on 17 Sep 2016

So, while I do find this part of @mystor's argument compelling:

It allows for the code generated by a custom #[derive] to depend on the structure of the type it is being derived on for soundness (e.g. #[derive(Trace)] from rust-gc which must see the actual backing type, or it is unsound, potentially in a subtle use-after-free-y way)

I think that trying to enforce this in derive may be the wrong way to go about things. Specifically, we probably do want the ability to modify structure definitions in the general case (yes, it won't be transparent, but so what). Which means that the idea of having a "sound" Trace that can be assured that the struct doesn't change afterwards might need to be solved in some _other_ way. Consider if decorators are applied bottom-to-top:

#[mangle] // <-- custom decorator that does bad things to struct definition
#[derive(Trace)]
struct Foo {
    x: T, y: U
}

One thought might be that the Trace impl can be written in such a way that _it_ fails to compile if the struct definition changes. For example:

unsafe impl Trace for Foo {
    fn trace(&self) {
        let &Foo { ref x, ref y } = self;
        <T as Trace>::trace(x);
        <U as Trace>::trace(y);
    }
}

Note though that #[mangle] can also screw with your impl if it is truly diabolical. =) There's only so much we can do here.

nikomatsakis on 19 Sep 2016

As an observer to these conversations, I'd be happy having the formal or informal rule that #[derive] is only allowed to add impl blocks and introduce a sibling annotation (#[mangle(Foo, Bar)] sounds good to me 😸) that is dedicated for _changing_ the structure of a type. #[mangle] could have a defined evaluation order.

There probably needs to be a defined evaluation order between annotations if there isn't already.

shepmaster on 19 Sep 2016

I think my opinions on this have relaxed. @nikomatsakis makes a good point that even if we got rid of mutating structs within #[derive] we wouldn't get away with being able to make assumptions about struct layouts in the code anyways. The let Foo{ ... } trick seems like it will work for making sure that the layout is correct in sane cases. We'll probably need to document it somewhere however so that everyone doesn't have to discover it independently.

mystor on 19 Sep 2016

@nikomatsakis

mh .. there could be the additional rule, that _custom decorators must be applied before derive_, plus _derives may not alter the item_. This would still allow to harden/purify the derive mechanics generally.

But I'm also glad to see there is another way to harden custom derives _individually_ against later changes. :-)

colin-kiegel on 19 Sep 2016

Regarding cases that need to modify the interior of the item the attribute is applied to, I just came across the "Pre-implementation feedback for Qt with Rust" thread on u.r-l.o, and made this post:

https://users.rust-lang.org/t/pre-implementation-feedback-for-qt-with-rust/7300/19

One notable facet is that here, I'm suggesting a #[derive] (or similar) be applied to a _trait_, rather than a struct - and the item added inside it would be a const trait method.

eternaleye on 21 Sep 2016

Similar to the cargo issues I raised above, I am unsure about

#[macro_use]
extern crate double;

importing procedural macros from a crate called double. Since we are using runtime code (as in real Ruest) at compile time, there should be a phase-incrementing import statement analogous to Racket's (require (for-syntax ...)). [Racket doc is https://docs.racket-lang.org/reference/require.html#%28form._%28%28lib._racket%2Fprivate%2Fbase..rkt%29._for-meta%29%29, unfortunately I can't figure out how to link the right section.]

The blog post http://blog.ezyang.com/2016/07/what-template-haskell-gets-wrong-and-racket-gets-right/ points out the phasing mistakes made in Template Haskell and may be of interest---I don't want to make the same mistakes in Rust.

Ericson2314 on 22 Sep 2016

@Ericson2314 The difference in Rust is that double is already compiled _for a specific phase_.
That is, the crate only exports the macro/modifier interface, as if they were defined with, e.g. macro_rules.
Being able to create crates that export both procedural macros and the underlying Rust items (that form the procedural macro) would be interesting, but so far it doesn't seem to be proposed in any capacity.

It may make sense to allow the crate being built to choose a lot about what and how to export, but just taking a system wholesale from a LISP with a different compilation model seems counter-productive.

eddyb on 22 Sep 2016

Yeah @eddyb I'm skeptical of this "create knows how it will be used downstream" methodology. If anything phases are more important to us than Racket due to our compilation model (I'm not even sure if Racket can cross-compile), so I don't get your last argument.

Ericson2314 on 23 Sep 2016

Nominated for lang team discussion re: a stabilization plan.

aturon on 28 Sep 2016

On the serde side, here is the short list of remaining issues before we can stop supporting the existing compiler plugin and officially recommend Macros 1.1 for all nightly users: https://github.com/serde-rs/serde/issues/545. ~~The only thing we need from Rust is for #36211 to be fixed.~~ Everything ~~else~~ we are making quick progress on.

I have a PR open that implements our rustc_macro without using syntex, so we can stop worrying about compile time https://github.com/serde-rs/serde/pull/548.

EDIT: never mind, #36211 only affected the old syntex implementation

dtolnay on 28 Sep 2016

I will try to finish the Diesel port by Friday so I can confirm this does everything we need on that end.

sgrif on 28 Sep 2016

@dtolnay given serde-rs/serde#548, are there any remaining blockers for serde?

@sgrif awesome, thanks! Once you've done that (or before) could you comment here with any remaining blockers you've encountered?

alexcrichton on 28 Sep 2016

Yes, I will.

On Tue, Sep 27, 2016, 7:29 PM Alex Crichton [email protected]
wrote:

@dtolnay https://github.com/dtolnay given serde-rs/serde#548
https://github.com/serde-rs/serde/pull/548, are there any remaining
blockers for serde?

@sgrif https://github.com/sgrif awesome, thanks! Once you've done that
(or before) could you comment here with any remaining blockers you've
encountered?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/35900#issuecomment-250028743,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdWK7MnA1rpn-WGji8gwAT2JCIqB4LFks5quaa-gaJpZM4JqEAX
.

sgrif on 28 Sep 2016

👍1

@alexcrichton

given serde-rs/serde#548, are there any remaining blockers for serde?

No, if @oli-obk or @erickt review that PR today or tomorrow then I can push everything out the next day and migrate a few prominent users like Rusoto.

dtolnay on 28 Sep 2016

👍1

@dtolnay I'm making heavy use of serde_macros in Ruma and would like to help you test serde_derive as well, if you need more eyes.

In fact, I use diesel_codegen, too, so Ruma is a good testing ground for the Macros 1.1 version of both of these libraries.

jimmycuadra on 28 Sep 2016

👍3

I released Serde 0.8.10 announcing that serde_macros is deprecated in favor of Macros 1.1.

dtolnay on 28 Sep 2016

👍7 🎉6

Let's add a checkbox for "cargo doc works" - https://github.com/rust-lang/cargo/issues/3132.

dtolnay on 29 Sep 2016

@dtolnay done!

alexcrichton on 29 Sep 2016

Not sure if these are serde_derive bugs or a rustc/rustdoc bugs, but I'm noticing two problems in generated docs:

A literal "///" appears at the beginning of generated docstrings of types that use custom derive.
Deserialize and Serialize don't show up in the "Trait Implementations" section for types that use custom derive.

jimmycuadra on 1 Oct 2016

A literal "///" appears at the beginning of generated docstrings of types that use custom derive.

This was a syn bug. I released 0.8.2 with a fix so cargo update to pick it up.

I don't know about the second one, will leave that to @alexcrichton.

dtolnay on 1 Oct 2016

@jimmycuadra

Deserialize and Serialize don't show up in the "Trait Implementations" section for types that use custom derive.

That sounds quite fishy! Looks like this is a bug in rustdoc though, but perhaps not a new one. For example rustdoc does not show this with the Copy implementation:

#[derive(Clone)]           
pub struct Point {         
    x: i32,                
    y: i32,                
}                          

const _FOO: () = {         
    impl Copy for Point {} 
    ()                     
};

That pattern is used by serde-derive to generate impls. @dtolnay did serde_macros have that same form of generation as well? Would it be easy to move away from for now to solve the documentation problem?

@jimmycuadra want to open up a separate bug to track the rustdoc issue about that?

alexcrichton on 3 Oct 2016

@dtolnay did serde_macros have that same form of generation as well?

Yes.

Would it be easy to move away from for now to solve the documentation problem?

No. As far as I know this is the only solution that meets both of these constraints:

Must support serde not in the crate root, i.e. not ::serde. This is common when people put serde impls behind a feature flag in a separate module.
Must be able to use private types from the surrounding module.

See https://github.com/serde-rs/serde/issues/159 for the details.

dtolnay on 3 Oct 2016

@dtolnay ah ok, so to clarify, @jimmycuadra this isn't a regression from before, right?

alexcrichton on 3 Oct 2016

Update: Not quite done with the port, but almost there. No issues other than things which I've reported already or minor limitations that we knew we'd encounter. Probably safe to check the "diesel works" box, we'll have a release out tomorrow on Macros 1.1 if it doesn't happen tonight.

sgrif on 4 Oct 2016

For those following along, I've opened #36945 to renamed rustc_macro to proc_macro virtually everywhere which appears to be the consensus on the name for this crate.

alexcrichton on 4 Oct 2016

It looks like the missing trait implementations in rustdoc (which is now being tracked in another issue) is not specific to Macros 1.1. Safe to ignore.

jimmycuadra on 4 Oct 2016

I tried to port my mockers library from compiler plugin to macros 1.1 and got “error: custom derive attributes may only be applied to struct/enum items”. With compiler plugin it is possible (though somewhat strange) to use "derive" on traits. Am I out of luck here?

kriomant on 5 Oct 2016

@kriomant interesting! I think that may actually be a bug in custom derive today, as I'm not sure it was ever intended to allow for #[derive] to apply to a trait...

I think for now to be conservative we're likely to not _stabilize_ derive-on-traits just yet, but we could perhaps add an unstable feature for it. Thoughts @rust-lang/lang?

alexcrichton on 5 Oct 2016

👍1

@alexcrichton What's a difference between traits and structs from an aspect of custom_derive?

KalitaAlexey on 5 Oct 2016

@KalitaAlexey none, it's an artificial limitation to match the real derive implementation

alexcrichton on 5 Oct 2016

@alexcrichton Could we extend a support to traits?

KalitaAlexey on 5 Oct 2016

As I mentioned above, it's likely a bug that custom derive was ever allowed on traits, and this was not spec'd in the RFC, so more discussion would need to happen before extending. In any case, support for derive-on-trait is unlikely to be stabilized in the first pass, but we could consider a separate feature gate.

I personally would not want to add derive-on-trait as that seems much more along the lines of custom attribute territory rather than derive itself. (e.g. goes against the spirit of #[derive] as originally created)

alexcrichton on 5 Oct 2016

I would prefer for custom derive to follow the exact same rules as regular derive. We _might_ want to change that later, but I think it should be RFC'ed (I'm also pretty cold on the idea, tbh, but I could have my mind changed by a compelling use case and a decent handling of various edge cases).

nrc on 5 Oct 2016

@alexcrichton

I think for now to be conservative we're likely to not stabilize derive-on-traits just yet, but we could perhaps add an unstable feature for it. Thoughts @rust-lang/lang?

👍 from me.

nikomatsakis on 5 Oct 2016

Ok, unstable feature is good, but it means that my library won't work on stable still (without code generation). And macro!(…) syntax is not covered by "macros 1.1" too, am I correct?

kriomant on 6 Oct 2016

I'm revisiting the proc macro RFC and the plan there was that macro crates should declare themselves #[cfg(macro)]. We don't require this for macros 1.1, but we do require a special crate type. The two mechanisms are somewhat orthogonal - the first describes the phase of compilation, the second the kind of crate. But they are also somewhat overlapping - it the latter implies the former. The former also scales to declaring proc macros and non-macro functions in the same crates, whereas the latter does not.

I'm not sure whether we need to change anything here, we could probably hack the proc macros proposal into backwards compatibility (it deliberately elides describing the mechanism of loading macros and thus the crate types). But something to ponder during the stabilisation period.

nrc on 6 Oct 2016

@kriomant correct, yeah

@nrc right now that's actually defined as cfg(rustc_macro) (although soon to change to proc_macro). We don't require it, no, but I thought it was going to be necessary for the concept of both linking to a crate a compile time and also at runtime? That is, we'd compile the proc-macro crate twice: once with the crate type proc-macro and once with the crate type rlib, and the latter wouldn't link to libsyntax or anything like that.

For now though it seems fine to not _require_ it, although I guess that would mean at a later date you have to opt-in to runtime support? (compiling the crate twice)

alexcrichton on 6 Oct 2016

@alexcrichton Could #[proc_macro_derive] imply it? The same way #[test] implies #[cfg(test)].
(Doesn't mean we have to add it now, just that the worst case if we add cfg is unused item warnings.)

eddyb on 6 Oct 2016

@eddyb What about extern crate proc_macro;? I feel like those would break too.

mystor on 6 Oct 2016

@mystor It's just a regular crate with a bunch of types and impls.

eddyb on 6 Oct 2016

Doesn't it also link to libsyntax though, which means that if a crate used a proc_macro crate and wanted to cross compile it would also have to cross compile syntax?

mystor on 6 Oct 2016

@eddyb yes #[proc_macro_derive] can get automatically ignored, but the problem is that we _also_ need to ignore everything that those functions transitively reach (like the extern crate proc_macro). Although it's "just a crate" it has severe runtime implications (dynamic linking, not available for cross-compiled targets, etc).

Note that tests have #[cfg(test)], so it seems reasonable to me that we still give #[cfg(proc_macro)] for basically the same purpose.

alexcrichton on 6 Oct 2016

@alexcrichton _Ideally_, the crate would have nothing more than what's exported and not require dynamic linking or anything like that, only libstd. However, I could see it causing problems in #![no_std] cases.

Sounds like we'd have to do the double-compilation from the start if we want to catch everything :disappointed:.

EDIT: Wait, what am I even thinking? It requires a custom crate type, double-compilation applies to _regular_ crates that _also_ export procedural macros/attributes/derives/etc. So not relevant for now.
But we could at least introduce #[cfg(proc_macro)] that's always set for the new crate type.

eddyb on 6 Oct 2016

🔔 This feature is entering its final comment period with intent to stabilize at the end of this release cycle! 🔔

The rationale for consider stabilization now is:

The API surface of this feature is, by design, _extremely_ conservative. At the same time it is forward-compatible with the de facto plan for procedural macros
The feature is quickly getting widespread use through Serde, and soon Diesel -- though, notably, the widespread use is largely as a _client_ of custom derive.
Going into FCP now gives us three months before the feature is actually shipped in stable, which should be ample time to catch any remaining issues.

Note that the name is shifting to proc_macro, based on earlier consensus on this thread. We can continue bikeshedding this and any other fine points for the rest of this release cycle.

aturon on 6 Oct 2016

Compiling the crate twice sounds very crude: scoping will not work correctly. Really something like extern! crate needed_for_my_inline_proc_macros; is a much nicer solution.

Ericson2314 on 6 Oct 2016

@aturon what, didn't people just start using this days ago?

Ericson2314 on 6 Oct 2016

@Ericson2314 It's been a bit longer than that, but as the FCP comment explains, the _minimum_ time before shipping to the stable channel is three months from now, which we feel is more than sufficient for this extremely narrow interface.

Note that the FCP itself is a 6 week long affair that doesn't necessarily mean we would even put it on the path to stabilization. But by at least starting the process now, we create the opportunity to ship this feature in 3 months, assuming no issues are discovered before then.

aturon on 6 Oct 2016

Ah ok. I remembered the 3 months part, but forgot the location of FCP within those 3 mouths---lest it be it was 3 mounths from Aug 22. Nevermind the timing then.

I do think the phasing issues I brought up do impact even this small part of the proc macros story, so I would like to see those addressed.

Ericson2314 on 6 Oct 2016

@alexcrichton the rustc_copy_clone_marker and other rustc_attrs are still biting us. See https://github.com/serde-rs/serde/issues/577.

#[derive(Copy, Clone)]
#[cfg_attr(feature = "serialize", derive(Serialize, Deserialize))]
pub struct MyStruct {
    value: i64,
}

The workaround is to swap the order but I figured I would check whether this is fixable.

#[cfg_attr(feature = "serialize", derive(Serialize, Deserialize))]
#[derive(Copy, Clone)]
pub struct MyStruct {
    value: i64,
}

dtolnay on 8 Oct 2016

I've finished porting https://github.com/sfackler/rust-postgres-derive over to macros 1.1 It was relatively painless, but handling of attributes that are read by multiple derives is a huge pain. Serde ran into the same thing, but it requires a bunch of weird logic to expand both derives at the same time: https://github.com/sfackler/rust-postgres-derive/blob/master/postgres-derive-internals/src/lib.rs#L26

It shouldn't block stabilization but I think it's a relatively large ergonomic issue on the authorship side that we should probably tackle soonish.

sfackler on 8 Oct 2016

👍2

I released quote 0.3.0 with support for fancy macro_rules!-style repetitions (thanks @nrc for the nudge). If you are developing proc macros without quasi/syntex you probably want this. Example usage from postgres-derive:

pub fn enum_body(name: &str, variants: &[Variant]) -> Tokens {
    let num_variants = variants.len();
    let variant_names = variants.iter().map(|v| &v.name);

    quote! {
        if type_.name() != #name {
            return false;
        }

        match *type_.kind() {
            ::postgres::types::Kind::Enum(ref variants) => {
                if variants.len() != #num_variants {
                    return false;
                }

                variants.iter().all(|v| {
                    match &**v {
                        #(                           // \
                            #variant_names => true,  //  |----- new feature
                        )*                           // /
                        _ => false,
                    }
                })
            }
            _ => false,
        }
    }
}

dtolnay on 9 Oct 2016

❤2

I'm curious, why # and not $?

EDIT: Always thought the quoting syntax would be ${...} but maybe I'm just too attached to ES6 templated literals even if it's been several years. \{...} also works although it'd be useful elsewhere.
No brackets seems hard to spot but I shouldn't worry.

eddyb on 9 Oct 2016

👍2

I'm curious, why # and not $?

Because it is a macro_rules macro and I can't do whatever I want :smile:. A $v gets matched as a single token and there is no way to go from $v to using the variable v. In contrast #v are two tokens so I can match them separately and do things with the ident.

macro_rules! demo {
    ($tt:tt) => {};
}

fn main() {
    demo!($v);
}

dtolnay on 9 Oct 2016

Wait all of that is done in macro_rules?! Forgot macros 1.1 was derive-only for a moment. That said, $x being one token is a design flaw IMO. cc @jseyfried

eddyb on 9 Oct 2016

👍1

@dtolnay

@alexcrichton the rustc_copy_clone_marker and other rustc_attrs are still biting us. See serde-rs/serde#577.

Oh dear, that seems bad! I'll update the list at the top. @nrc or @jseyfried thoughts about how we might tackle this? I'm not too familiar with the expansion order of everything, but maybe when one #[derive] attribute is being expanded it could try to slurp up all the future ones and do that first?

@sfackler

handling of attributes that are read by multiple derives is a huge pain

I'm not sure I quite followed the example you linked to, could you elaborate?

alexcrichton on 9 Oct 2016

@alexcrichton Take for example

#[derive(ToSql, FromSql)]
enum Foo {
    #[postgres(name = "bar")]
    Bar
}

The #[postgres] attribute is used to adjust how the ToSql and FromSql implementations are generated. It needs to be stripped before the final output since it's otherwise an unknown attribute, but the ToSql and FromSql implementations are run separately. If you do it the naive way by just generating the implementation and then stripping the attributes, the second deriving will be missing the customizations.

serde-derive and postgres-derive hack around this right now by having the implementation for both derived impls forward to the same function that generates both at once. We have to reattach the #[derive] for the one that's currently being invoked since the compiler strips it, and then send it along to be expanded: https://github.com/sfackler/rust-postgres-derive/blob/master/postgres-derive/src/lib.rs#L18

sfackler on 9 Oct 2016

👍1

@sfackler I _think_ you can also do it by having the extra attributes stripped only when there is no derive from the same implementer left around. Your way might be better _shrug_.

eddyb on 9 Oct 2016

@sfackler ah ok, makes perfect sense. Thanks!

I wonder if that logic may be brittle though. For example if you're expanding ToSql how is the future expansion of FromSql detected? It could be behind a #[cfg] like @dtolnay mentioned above maybe? So it may not be possible to detect in all cases?

alexcrichton on 9 Oct 2016

@alexcrichton Yes it is brittle, which is why a real solution seems important.

sfackler on 9 Oct 2016

👍1

Wouldn't #[cfg] and #[cfg_attr] be processed before derives?

eddyb on 9 Oct 2016

Since the macros 1.1 api works on strings, I can only imagine three solutions:

leave it as it is and wait for macros 2.0
allow unused attributes in custom derive
extend the macros 1.1 api by allowing a custom derive to push attribute names to a specific whitelist for the current item

Each option has there own pros/cons.
con 1: brittle workarounds for the meantime
con 2: some unused attributes will be undetected
con 3: more api surface for an interim solution

colin-kiegel on 9 Oct 2016

I have considered wrapping attributes in #[used(...)] to keep them and whitelist them at the same time, but it's pretty silly and too insta-stable.

eddyb on 9 Oct 2016

@alexcrichton

when one #[derive] attribute is being expanded it could try to slurp up all the future ones

I like this idea. Since #[cfg]s and and #[cfg_attr]s are processed before #[derive]s, #[cfg]-guarded #[derive]s are not a problem for this approach (or for @sfackler's analogous solution to the attribute stripping issue).

This approach would make @sfackler's solution slightly simpler, since other relevant derives could only be in the last attribute (I also think @eddyb's suggestion would simplify things).

jseyfried on 9 Oct 2016

One possibility for the control attributes issue is to add post-expansion callbacks, somewhat similar to syntex's: https://github.com/sfackler/rust-postgres-derive/blob/master/postgres-derive-codegen/src/lib.rs#L23-L50

Your derive impls can be independent and not strip control attributes, and you could register a pass that runs after all expansion is done to clean up.

sfackler on 9 Oct 2016

Diesel also is manually working around this. https://github.com/diesel-rs/diesel/blob/master/diesel_codegen/src/lib.rs#L101-L112

sgrif on 10 Oct 2016

@dtolnay your previous concern about multiple #[derive] attributes should be fixed shortly thanks to @jseyfried

alexcrichton on 10 Oct 2016

I had a play with this using the syn and quote packages, and had a really positive experience. This will be a great feature when it stabilises.

keeperofdakeys on 12 Oct 2016

👍1

The issue with attributes is currently biting my impl, being downstream of serdes derive logic.

Ignoring unused attributes in derived code, or allowing late expansions that run after regular derives to strip them seem like reasonable solutions to me.

It'd be ideal if macros didn't have the option to mutate the original token stream, it seems like a dangerous feature that could end up being relied on in the wild between now and macros 2.0 and therefore difficult to remove later.

KodrAus on 15 Oct 2016

👎1 👍1

So I had a missing semi-colon in a type path inside a function generic, and got the following error message from rustc. I'm wondering if more information could be provided, like which derive macro caused the lex error, which token caused it, etc.

error: custom derive attribute panicked
  --> src/simple.rs:69:1
   |
69 | struct C(u64);
   | ^^^^^^^^^^^^^^
   |
   = help: message: Failure parsing derived impl: LexError { _inner: () }

keeperofdakeys on 16 Oct 2016

👍2

I made a test case for the LexError, it appears to happen with the October 13th nightly (rustup had no updates) https://github.com/keeperofdakeys/proc_macro_derive_test.

keeperofdakeys on 17 Oct 2016

@rust-lang/lang I think we need to discuss the issue of whether derives can modify their annotated item. Although it is a simple solution to allow it, it seems to go against user expectations and seems to be fairly unpopular. I still prefer the current solution - the simplicity is nice, and none of the proposed alternatives strike me as wildly better.

nrc on 18 Oct 2016

We will need some solution to control attributes if the item can be modified. I don't think I feel particularly strongly about keeping the ability for derive to modify the item, as long as (in the future) non-derive attribute macros can.

sfackler on 18 Oct 2016

Somebody should define "modify". Are we talking about modifying the members of the struct, or are just we talking about stripping attributes in order to prevent the compiler from complaining about unknown attributes?

If we're talking about modifying the members of the struct, in my opinion the best solution is probably to allow only one struct-modifying macro on each struct that is expanded before all the others. That would have a well-defined and (in my opinion) expected behavior.

If we're only talking about stripping attributes, there should probably be a way to integrate attributes in the macros mechanism itself instead of leaving that to the discretion of the macro.

tomaka on 18 Oct 2016

👍1

My intuition about how derive behaves is that it adds impls. To the end user, derives should seem to add impls and do nothing else. In particular, they shouldn't modify the struct in a way that will matter to other derivers, and therefore they should be order independent. Do we agree that this is how a derive should behave, or do people think derives should also be able to perform transformations on the attached type?

If we agree on that, the question becomes - do we want the interface we expose to enforce this, or we do we want to leave it up to derive authors to enforce this? Here, there seem to be two countervailing problems:

Enforcing the behavioral contract of derive certainly seems more like the Rust solution to me.
Getting serde and deisel to work with that constraint presents a lot of challenges.

And of course how other attributes may modify the struct seems unrelated to how derive, specifically, should behave.

withoutboats on 18 Oct 2016

👍5 ❤1

Worth noting that the ability to remove the annotated item allows the current system to fill a lot of roles it otherwise wouldn't.

sgrif on 18 Oct 2016

👍1

@sgrif Could you elaborate on how deisel is using derive? I feel like I understand how serde uses its ability to modify the struct (to remove attributes that inform the macro to omit or rename fields in the serialization trait), but maybe deisel is using this to do something else. When you say "remove the annotated item," it seems like you're performing a pretty radical transformation on the item.

withoutboats on 18 Oct 2016

@withoutboats 90% of it is more or less what you'd expect. We don't touch items provided by the user. We remove annotated items to hack bang macros into the system though. https://github.com/diesel-rs/diesel/blob/master/diesel/src/macros/macros_from_codegen.rs#L12-L18. Beyond that, the only time we touch anything on the input token stream is to strip annotations. https://github.com/diesel-rs/diesel/blob/master/diesel_codegen/src/lib.rs#L109-L120

sgrif on 18 Oct 2016

@sgrif i have also abused custom derive to get procedural bang macros. I would personally much prefer there being a procedural bang macro system in macros 1.1 such that we weren't tempted to abuse this feature too much, because its gross. I feel like a good path to take would be to also get a near identical procedural bang macro story going (potentially with some heavy handed but simple hygiene, such as all identifiers not present in the TokenStream passed to the macro are hidden with hygiene? I don't know exactly what that would look like) and use that instead of abusing custom derive, and then make custom derive unable to modify the struct it is on. That way we enable even more crates, improve ux, and make derive sane.

I can understand the argument for keeping derive as a simple macro, however.

mystor on 18 Oct 2016

My perspective is that the goal of Macros 1.1 is to be as flexible as possible to fill as many needs as possible, with a low maintenance burden so it can be stabilized quickly and act as a stopgap until macros 2.0. The current design fits that role extremely well in my opinion.

If we were talking about something that was meant to permanently fill this role, I'd be much more opposed to it

sgrif on 18 Oct 2016

Maybe I'm wrong, but my reading of the RFC is that this is intended to be the basis for the behavior of derive permanently. That is, more methods will be added to TokenStream in the future, but derive macros will be using this API, which currently allows them to perform arbitrary mutations on the annotated item (and this ability is needed for deisel's use case).

I feel pretty negatively about allowing derives to do this permanently. If we accept that this is a system that is going to be deprecated, along with macro_rules macros, at some point in the future, and under macros 2.0 a different derivation API which is more restrained will be preferred, I am more comfortable with it.

withoutboats on 18 Oct 2016

It seems that the intention is to support decorators as transformers which can do anything.
And derive exposed as a simple decorator with no input in the attribute.

eddyb on 19 Oct 2016

@withoutboats: serde supports a number of attributes to modify how the impls are generated, and so we definitely need the ability to strip those out or otherwise ignore them after we've processed them. If it'd help, we could somehow provide a list of attributes to the compiler that ought to be stripped out, rather than us wanting to do that ourselves.

erickt on 19 Oct 2016

👍4

@eddyb I'm in favor of decorates being able to do anything, but not in favor of derives being unrestrained decorators (in the long term).

@erickt Right. I think in the long term the ideal solution would be for those attributes to be registered as no-op custom attributes, instead of the deriver being responsible for stripping them. But that's not feasible in the short term.

withoutboats on 19 Oct 2016

👍2

I think in the long term the ideal solution would be for those attributes to be registered as no-op custom attributes, instead of the deriver being responsible for stripping them. But that's not feasible in the short term.

I'm not familiar with the compiler internals that make this infeasible in the short term, but would it be possible for the custom derive plugin to present a list of custom attributes it intends to strip and then rejecting any other transformations on the attributes on the original item?

I also notice that Diesel does not follow Serde's approach of having all its custom attributes under one name (e.g. #[serde(rename = "name")] as opposed to Diesel's #[table_name = "name"].) Would it simplify implementation if only a single custom attribute name was registered instead of a list?

jimmycuadra on 19 Oct 2016

One possibility for the control attributes issue is to add post-expansion callbacks, somewhat similar to syntex's: https://github.com/sfackler/rust-postgres-derive/blob/master/postgres-derive-codegen/src/lib.rs#L23-L50

Your derive impls can be independent and not strip control attributes, and you could register a pass that runs after all expansion is done to clean up.

I implemented post-expansion callbacks for Macros 1.1 in post-expansion. Your derive impls can be independent and not strip control attributes, and you can register a pass that runs after all expansion is done to strip attributes.

dtolnay on 20 Oct 2016

👍4 🎉1

We discussed the modification/ordering issue today at the lang team meeting. There was a desire to meet user expectations, and for simplicity (in terms of design, not having too many hacks layered on tops, and not being too subject to hard to decipher/debug errors). It was noted that even though modification of the target data is surprising, it is not unsafe. It was also felt that we _might_ be tending towards purity for its own sake, rather than for well-motivated reasons.

In the end, we decided that derives which don't modify the source are probably better and we should change to that model. That might mean prolonging the FCP period. We did not think there should be a special mechanism for dealing with stripping attributes. Rather, that the way the compiler handles attributes should allow for ones used by a macro to remain in the program. RFC 1755 should take this into account.

This will delay stabilisation of some custom derive users. However, we believe that most uses of derives (especially those keeping users off a stable toolchain) do not use attributes, so for example, most users of Serde will be able to move to stable soon. Those that need attributes, will take a few cycles longer _but that won't affect the common case_.

cc @alexcrichton, @dtolnay, @sgrif, @erickt - thoughts?

nrc on 21 Oct 2016

👍3

Attributes are probably more commonly used with Serde and Diesel than you suggest. Just from my own experience, I'm pretty sure all of my programs that use Serde use attributes. With Diesel I definitely use attributes, and I think it's required in order to tell diesel_codegen which database table maps to the struct.

That said, not letting custom derive mutate the struct seems like the right choice to me. It just simplifies the whole thing, preventing lots of weird edge cases. It's more important to get it right than it is to get it done quickly, so if the feature has to stay unstable a little longer, that seems fine too.

jimmycuadra on 21 Oct 2016

👍1

It was noted that even though modification of the target data is surprising, it is not unsafe.

It is not unsafe, unless you're deriving an unsafe trait.

In my opinion one of the use cases of custom derives is to implement in a safe way traits marked as unsafe, like for example a trait that must describe exactly the layout of the members of the struct it is implemented on.

tomaka on 21 Oct 2016

👍1

My asn1 crate also requires attributes for anything but trivial usage, so I'd effectively need to wait till custom attributes land.

Would it be a good idea to split the custom attributes rfc into two?

An rfc to provide the notation and namespacing for custom attributes in general (allowing no-op attributes in stable).
An rfc for how to define, and the semantics around running custom attribute macros.

Custom attribute macros seem like something that will require a lot if fleshing out. So splitting the rfc into two may provide stable attributes for custom derive packages sooner.

This also means attributes are name-spaced, and intentional (ie. extern crate is required to use attributes for a crate). I can forsee this being a good thing to prevent two custom derive macros using the same attribute name. A good expectation here would be using the name of the crate the trait is in for the attributes.

keeperofdakeys on 21 Oct 2016

👍3

Is there any reason why this implementation does not encompass usual name! macros? Simple TokenStream in, TokenStream out for usual macros would prove extremely useful in pest where compile times for complex grammars exceed 30s.

dragostis on 21 Oct 2016

@dragostis To quote the rfc's summary:

Extract a very small sliver of today's procedural macro system in the compiler, just enough to get basic features like custom derive working, to have an eventually stable API. Ensure that these features will not pose a maintenance burden on the compiler but also don't try to provide enough features for the "perfect macro system" at the same time. Overall, this should be considered an incremental step towards an official "macros 2.0".

Or in more practical terms, it will take a lot of design and testing to get a procedural macro system that works well, just like the closure system we have now took to build. Crates like serde and diesel have serious usability issues without a proper custom derive feature, so let's make a stop-gap measure to fix it now - that also happens to help tooling and experience with a possible procedural macro system. The syn and quote crates are good examples of this.

keeperofdakeys on 21 Oct 2016

@keeperofdakeys Yep, got that. I'm not asking for a macros 2.0 implementation, I'm just wondering if there's any burden involved for adding another attribute, say proc_macro that is a minimal implementation which simply mirrors the derive design only for usual macros. The example in pest would simply derive a parser for a struct only it wouldn't derive it from the struct itself but from a simple grammar defined between {}. I hope I'm not digging up dead discussions, though!

dragostis on 21 Oct 2016

@dragostis I brought up this possibility on internals a while ago. You can read the responses there: https://internals.rust-lang.org/t/pre-rfc-extend-macros-1-1-to-support-foo-style-macros/3921

mystor on 21 Oct 2016

👍2

@nrc

It was also felt that we might be tending towards purity for its own sake, rather than for well-motivated reasons.

One note on this: in a lot of cases where we've had to make tough decisions, like parametricity and specialization, statically ensuring the relevant notion of "purity" would be difficult/impossible. But in this case, it's actually very straightforward to guarantee it by construction, and eliminating the significance of order for derives seems like a pretty strong simplification for users.

As far as stability goes, we could consider adding an unstable mechanism for ignoring attributes right now, which would be stable in practice (in that the API would not be prone to breakage) even if it still requires nightly to use. We could even consider stabilizing such a side-channel, with plans to deprecate it in favor of a more general mechanism once available. Or we could consider @keeperofdakeys's suggestion and push quickly on the piece of the general attributes solution that would address the immediate concern.

From my perspective, it's important that none of these concerns significantly block macros 1.1 from being widely usable for Serde and at least "de facto" stable (i.e., not breaking).

@sgrif

If we were talking about something that was meant to permanently fill this role, I'd be much more opposed to it

I wanted to echo @withoutboats and clarify that the current intent is for the macros 1.1 API surface to remain as-is even when we ship macros 2.0. That's something we could reconsider, though -- we could think of it more like today's macro_rules, with the intent to deprecate once the final system is in place.

aturon on 21 Oct 2016

👍1

My impression of the attribute usage on custom derive was similar to @jimmycuadra's impression, which is that they're pretty commonly used. I believe (but correct me if I'm wrong) that the custom attributes were crucial for a number of diesel's use cases.

In that sense I'm not 100% sure what the best way forward about those attributes would be. It would be a shame to stabilize macros 1.1 but still leave a large number of users on nightly (even if it's "defacto stable" nightly) because that somewhat defeats the purpose of quick stabilization of macros 1.1 in the first place. In other words, if we're not pulling a majority of macros 1.1 users _actually_ on to stable Rust, stabilizing macros 1.1 I think can wait.

One point that has been bugging me though is what we think custom attribute will look like in the long term and rationalizing that with the current macros 1.1 system. In the current RFC for custom attributes crates have to declare whitelisted namespaces of attributes at the top of a crate. This means that using a custom attribute in custom derive is radically different than using a custom attribute elsewhere. That disconnect worries me in terms of an ergonomic and "least surprise" perspective, although I also see that as a forcing function for tweaking the RFC (e.g. if I link to the serde crate perhaps that can automatically whitelist serde attributes).

Overall I would personally be fine moving forward with complete stabilization of the macros 1.1 system as-is today. Or in other words, stabilize the fact that custom derive expansion functions receive a struct and then must return the struct as well if they want it preserved.

alexcrichton on 21 Oct 2016

👍3

I tend to switch from rustc-serialize to serde _because_ serde's code generation supports control attributes.

We could extend macros 1.1 to support arguments to the derive attribute itself. That seems like a nice thing to have in general, and it could be abused to work around the lack of control attributes in the short term potentially.

sfackler on 21 Oct 2016

Ditto @jimmycuadra and @sfackler, attributes are more integral to Serde than @nrc's comment makes them sound. Almost all use cases without attributes would be served just as well by rustc-serialize. If we want to get people off nightly we can do it by deprecating Serde in favor of rustc-serialize.

I don't yet have an opinion on the right move here, but I know that if we stabilize without attributes I would strongly consider parsing doc comments to pull out attributes. I imagine many people would not want to deal with a build script just so they can write:

#[serde(skip_serializing)]

.. instead of:

/// <!-- serde(skip_serializing) -->

dtolnay on 21 Oct 2016

@tomaka

In my opinion one of the use cases of custom derives is to implement in a safe way traits marked as unsafe, like for example a trait that must describe exactly the layout of the members of the struct it is implemented on.

What do you think about my comment here in this respect? I found that a satisfactory pattern.

nikomatsakis on 21 Oct 2016

I want to clarify one thing:

Is removing custom attributes the major thing people do with custom derive?

I know there are some hacks to do foo! expansion in the context of a type (e.g., @sgrif mentioned such a use case in diesel, I think @tomaka mentioned one too) -- how central are those use cases?

The reason I ask is this: I see benefits to having the derive mechanism only give back a list of add'l impls. In particular, it means that the spans for the type declaration itself would be correct. Adding a quick-and-dirty API to the context that lets you supply a list of attributes names to whitelist in the context of that type seems like an easy enough fix for attributes, and we could always deprecate it.

If however we want to enable more use-cases (and frankly those use cases are a bit more...surprising, when you think about what one expects from derive), then this won't work. In that case, I'd probably be fine with stabilizing as is and planning to deprecate the "from raw bytes" API in the future in favor of a better way to write derive (after all, we don't really expect people to be using raw bytes anyway, right? but rather token streams?) We could maybe give the API a slightly clunkier name =)

nikomatsakis on 21 Oct 2016

👍2

@nikomatsakis The main need is not to remove but rather ignore the attributes. I am not aware of serious downsides to ignoring them. Providing a whitelist on the deriving function via an additional parameter to the main attribute, for example, should suffice for all practical needs that are true derives and not procedural macro hacks.

eddyb on 21 Oct 2016

Yeah, I'm torn between two approaches on this, both of which have clear downsides:

The "simple derives" approach - adjust the API to only produce additional items, making it still unstable for users to use custom attributes as part of deriving, and closing the hole that diesel and other crates are jumping through to get arbitrary procedural macros.
The "planned obsolescence" approach - stabilize the API as is, with small adjustments possibly like Niko mentioned about naming, with the intention to deprecate it someday. This will require all custom derive authors to someday rewrite their code, and allow for the possibility of surprising behavior in the interim period.

EDIT: but @eddyb's whitelisting also sounds promising.

withoutboats on 21 Oct 2016

I mean it's not like @nrc is proposing something much different (although IIRC he is talking about whitelist in the user crate), and it gets silly to talk about "marking attributes as used" when all you have is a token stream.

eddyb on 21 Oct 2016

The arbitrary procedural macros which I have been abusing the macros 1.1 system for in rust-cpp would be totally possible with an alternate approach which simply provides the ability to add impls and ignore attributes.

I do think that having the ability to ignore attributes is essential, but other than that I would be fine with being unable to modify the struct beyond that point.

I'm not sure what forms of hacks which @sgrif is using in deisel which wouldn't be _possible_ to do in a world where we cannot modify the struct itself, and instead may only add additional items.

mystor on 21 Oct 2016

Allowing custom derives to take arguments, as suggested by @sfackler, might be a way to get everyone the functionality they need while allowing any decision on custom attributes to be deferred. It won't be as nice looking or as readable, but it'd get the job done. e.g.:

#[derive(Debug, Clone, Serialize(field("bar", rename = "baz")))]
pub struct Foo {
  pub bar: String,
}

This form could later be deprecated in favor of custom attributes, once a decision about them is reached.

jimmycuadra on 22 Oct 2016

Both serde and my crate require field/variant attributes. Trying to emulate this with derive arguments doesn't make much sense, we need attributes.

Whatever decision we make to stabilise this, it would be nice if the user's of custom derive macros didn't need to modify their code when/if we switch to a macros 2.0 derive api (obviously the authors of custom derive macros will). It seems like the most sensible decision is to give the compiler a list of attributes to strip for your derive, and critically, those are only stripped after _all_ derive macros have run. Serde, diesel, and my crate all have the issue of requiring the same attribute over multiple derive macros.

With the stripping behaviour, I wouldn't need the post-expansion crate that @dtolnay made to add another derive macro, to strip attributes after the rest have run.

keeperofdakeys on 22 Oct 2016

FWIW the only reason to strip those attributes is to keep the HIR slimmer - as far as everything else is concerned you only need to mark them as used.

eddyb on 22 Oct 2016

Is removing custom attributes the major thing people do with custom derive?

I know there are some hacks to do foo! expansion in the context of a type (e.g., @sgrif mentioned such a use case in diesel, I think @tomaka mentioned one too) -- how central are those use cases?

I'm totally ok with the fact that custom derives can't modify the struct.

I often grumble about the lack of plugins in stable Rust, so when I saw custom derives I used them as an opportunity to have plugins. But I'm obviously not going to argue that custom derives must support something they were not designed for.

tomaka on 22 Oct 2016

It looks like there's a regression in the latest nightly. Getting the error

Queryable is a derive mode

when compiling our examples.

#[derive(Queryable)]
pub struct Post {
    pub id: i32,
    pub title: String,
    pub body: String,
    pub published: bool,
}

sgrif on 22 Oct 2016

@sgrif That was caused by #37198, which changed custom derives to use the same namespace as other macros (so that bang!() macros, #[attribute] macros, and #[derive(custom)] macros all share the same namespace).

In that example, #[macro_use] extern crate diesel; imports a bang macro named Queryable and #[macro_use] extern crate diesel_codegen; imports a custom derive macro also named Queryable, which silently overwrites the bang macro (aside -- #[macro_use] silently overwriting is not ideal, but won't be an issue once we can import macros from extern crates with use instead of #[macro_use], soon!).

I believe the error is caused by a bang macro invocation Queryable!(); in the custom derive's expansion, which resolves to the custom derive from diesel_codegen instead of the bang macro from diesel.

jseyfried on 22 Oct 2016

@jseyfried What is the reason to use a single namespace for all three "kinds" of macros? It doesn't seem to me to make sense to treat attributes and bang macros as occupying the same namespace, anymore than it would make sense to treat functions and types as occupying the same namespace (derive macros even less so).

withoutboats on 26 Oct 2016

@withoutboats I think that is the wrong analogy, I prefer to think that different kinds of macros share a namespace in the same way as different kinds of values or types do (e.g., functions and consts). There is a complexity cost in having namespaces and I believe the fewer we have the better, so the question should be why do we need different namespaces? Clearly for back compat we need macros to be in a different namespace from functions and types.

nrc on 26 Oct 2016

The real issue is that you can't use use to selectively import, or rename macros. So a crate user has no ability to get around macro name clashes. At the same time, I wouldn't expect simply importing a ProcMacro crate to clash with local macro names - I thought the derive macros started with derive_?

keeperofdakeys on 26 Oct 2016

There is a complexity cost in having namespaces and I believe the fewer we have the better, so the question should be why do we need different namespaces?

I think items should share a namespace only if they could be ambiguous with one another. consts and fns are in the same namespace because they're both used as idents in an expression context. Types and traits are in the same namespace because of the syntax for trait objects.

These three kinds of macros are all invoked in distinct ways. It doesn't make sense to me that they should share a namespace, because there can never be an ambiguity in invokation.

While there's an implementation complexity cost, I don't think there's a significant cost in _use_ complexity by namespacing these separately. When I think about the complexity cost of a language feature, I usually think about use complexity.

Would you say that _all_ items should be in the same namespace if it weren't a breaking change? Trying to clarify about your thought process.

withoutboats on 26 Oct 2016

👍4

Reflecting a bit more, I'd be fine with saying there should only be 1 namespace that all items are in - I even sort of prefer it; the whole "real constructors" pattern is kinda confusing IMO - but that isn't the decision Rust made for non-macro items, so to me it seems like it would be more consistent & expected for macro items to occupy different namespaces.

withoutboats on 26 Oct 2016

The real issue is that you can't use use to selectively import, or rename macros.

You will be able to with macros 2.0. The model here is basically a stop-gap hack.

I thought the derive macros started with derive_?

That was the old custom derive system, I don't think macros 1.1 does this.

While there's an implementation complexity cost, I don't think there's a significant cost in use complexity

There is a cost for some definition of use - it makes life harder for tools, in particular (though one might argue not much). I also think it is not so much implementation complexity that matters (although namespaces definitely complicate the compiler, I agree that this is not so important) as language complexity - users have to reason about this stuff to use Rust and it has a knock-on effect to things like hygiene and shadowing, further complicating things.

Would you say that all items should be in the same namespace if it weren't a breaking change? Trying to clarify about your thought process.

I wouldn't quite go that far - I think there are benefits to values and types being separate, but certainly a language with a single namespace would be much nicer to work with in many ways.

but that isn't the decision Rust made for non-macro items

Counterpoint: modules and fields are in the value namespace, although the places they can be used are (I think) distinct from the places other values can be used.

nrc on 26 Oct 2016

@keeperofdakeys

The real issue is that you can't use use to selectively import, or rename macros

We'll be able to use macros from extern crates behind a feature gate soon (~1 week), c.f. https://github.com/rust-lang/rfcs/pull/1561 and #35896. We might decide to stabilize useing macros from custom derive crates along with custom derives themselves.

I thought the derive macros started with derive_

This was true for old-style custom derives. With macros 1.1 custom derives, #[derive(Serialize)] (for example) expects Serialize to resolve to a custom derive macro.

@withoutboats

What is the reason to use a single namespace for all three "kinds" of macros?

Precedence: bang macros and attribute macros have shared a namespace for a long time, so until that changes I think custom derives should also share that namespace.

Backwards compatibility: If we start with a single macro namespace, we can split it up backwards compatibly. If we start with multiple macro namespaces, we're stuck with them forever.

Simplicity: If each macro "kind" had its own namespace, we'd have five total namespaces. Thus, after https://github.com/rust-lang/rfcs/pull/1561 lands, use foo::bar; could import _five separate items_ named bar (a value, a type, a bang macro, etc.), and there would be no simple way to, for example, re-export the bang macro but not the custom derive macro or to just import the custom derive macro as baz.

jseyfried on 26 Oct 2016

Backwards compatibility: If we start with a single macro namespace, we can split it up backwards compatibly. If we start with multiple macro namespaces, we're stuck with them forever.

This is compelling, especially since 1.1 is supposed to be a stopgap. :+1:

Does this break code for anyone who had a macro_rules macro called e.g. PartialEq!?

withoutboats on 26 Oct 2016

No, PartialEq isn't defined in the macro namespace today since it's not a custom derive.
#[derive(Foo)] first checks if Foo is a "builtin derive" (PartialEq, Copy, etc.) before looking for a custom derive Foo in the macro namespace. The builtins are hard-coded into the definition of derive.

That being said, we could break code as you described if we eventually decide to make PartialEq a custom derive in the prelude instead a builtin, so it might be a good idea to future proof for that now.

jseyfried on 26 Oct 2016

Proposal for the attributes problem (assuming we work on a model where derive macros can't modify the items they are declared on, only decorate):

behind the scenes, custom derive macros implement a trait, currently MultiItemModifier. It is the plan that macros 2.0 will continue to implement a trait and this trait be used for extensibility of the mechanism. The annotated function that is the macro implements this trait. Although we don't use the plugin registrar, this is essentially a desugaring.
we should split off a CustomDerive trait specifically for macros 1.1 custom derive. In the common case, macro authors never see it. But they have the option of implementing the trait directly, rather than using an attribute on a function (I expect we would re-use the attribute on the impl, maybe we need to discuss this registration mechanism).
we add a declare_attributes function to CustomDerivewhich returns Vec<String>. It has a default impl returning an empty vec.
If macro authors implement this method, then any attributes named exactly as one of the returned strings is considered to belong to the macro. Any such attribute is never looked up like a macro and does not trigger the custom attribute lint. Attributes are left in the code by derive expansion, but are marked as used. Any such attribute which is not touched by a derive expansion would still trigger the unused attribute lint (but not the custom attribute lint).
We might in the future introduce scoped attributes that macros including custom derives can use, these would be _in addition_ to declare_attributes and this mechanism would not be deprecated.
Alternative: the strings returned by declare_attributes are checked as a path prefix for attributes, e.g., if declare_attributes returned vec!["foo::bar"], then #[foo::bar::baz] and #[foo::bar::qux] would be allowed.

Thoughts? cc @alexcrichton @jseyfried @dtolnay @sgrif @erickt @rust-lang/lang

nrc on 26 Oct 2016

👍1

@nrc would that mechanism allow to disable _used_ attributes like #[cfg(..)] - either by accident or on purpose? And could that be changed?

colin-kiegel on 26 Oct 2016

@nrc unfortunately I can't quite see what the implementation would look like given those constraints, so I'm not entirely certain whether it'd work or not.

That being said I'm somewhat skeptical of whether traits and trait objects would work, because where are the instances of the trait being created?

alexcrichton on 26 Oct 2016

@nrc Why does this have to be imperative and can't just be a list in the attribute designating the derivw expander function? E.g. #[proc_macro_derive(Serialize, uses_attrs(serde_foo, serde_bar))] or some such.

eddyb on 26 Oct 2016

👍1

@colin-kiegel I imagine it could only apply in the scope of the derive application, in which case disabling other attributes is probably expected behaviour, although I think we might apply cfgs before macro expansion (this has changed, but I can't remember the before vs after).

@alexcrichton

That being said I'm somewhat skeptical of whether traits and trait objects would work, because where are the instances of the trait being created?

Hmm, good point, I suppose we'll need to address this for macros 2.0

@eddyb This is a good idea and probably easier to do short term. My reason for preferring the imperative approach is that it is a general extensibility mechanism and one we'll want to keep around, whereas adding things to the attribute doesn't scale too well and it may not be something we want to be stuck with forever. On the other hand it is certainly easier to do now and doesn't seem like a bad thing to have hanging around, so I think it might be a better bet.

nrc on 26 Oct 2016

Getting caught up -- I've been out of the country and then ill. I would like to revisit https://github.com/rust-lang/rust/pull/37198, as I don't think it makes sense for all kinds of macros to occupy the same namespace. I'd at the very least expect custom derive macros to be in that namespace as derive_Foo, or similar. There's use cases for multiple types of macros using the same name even in the standard library (e.g. #[cfg] and cfg!)

sgrif on 30 Oct 2016

👍1

I also find the shared namespace a bit awkward. From an end user perspective it might appear as if there was some kind of interchangeability, ambiguity or something interesting going on - which there isn't. I think 'random' limitations like this could make understanding Rust as a language a bit harder..

However I think it's probably not the end of the world. It appears that different namespaces could still be introduced in the future without breaking to much.

colin-kiegel on 30 Oct 2016

@sgrif @colin-kiegel I described my rationale for a single macro namespace in https://github.com/rust-lang/rust/issues/35900#issuecomment-256247659.

I don't think it makes sense for all kinds of macros to occupy the same namespace

To be clear, bang macros and attribute macros have always occupied the same namespace; #37198 just moved custom derives into this namespace.

There's use cases for multiple types of macros using the same name even in the standard library (e.g. #[cfg] and cfg!)

Alternatively, cfg could be viewed as a single macro that can be used via a bang invocation or an attribute invocation (while it's not possible today for users to define a macro that can be invoked via a bang or an attribute, we might decide to allow it in macros 2.0).

From an end user perspective it might appear as if there was some kind of interchangeability

I think this could be ameliorated with clear error messages when two macros conflict (I'll work on improving today's messages).

It appears that different namespaces could still be introduced in the future without breaking to much

I believe splitting up the macro namespace is fully backwards compatible (correct me if I'm wrong). This is my main motivation for keeping a single macro namespace today -- we want macros 1.1 to be as future compatible as possible.

Finally, I think bang/attribute macro vs custom derive macro conflicts will be rare in practice since bang/attribute macros are usually lower case and custom derives are usually upper case. In other words, custom derives already have their own namespace when those naming conventions are followed.

It looks like the breakage (https://github.com/diesel-rs/diesel/issues/485) is caused by supporting, e.g. Queryable! { struct S; } as well as #[derive(Queryable)] struct S;. Once custom derives are stable, there will be no need to support Queryable! { struct S; } so this will no longer be an issue, right?

In the meantime, I believe we could update diesel so that

Queryable! is still supported without #[macro_use] extern crate diesel_codegen;, and
#[derive(Queryable)], but not Queryable!, is supported with #[macro_use] extern crate diesel_codegen;.

Feel free to ping me on IRC to discuss -- I'd be happy to write a PR.

jseyfried on 31 Oct 2016

@nrc @alexcrichton

That being said I'm somewhat skeptical of whether traits and trait objects would work, because where are the instances of the trait being created?

I agree with @eddyb that if all we want is to handle attributes, in the interest of convenience and expediency, we should just extend the attributes.

But if we _did_ want a more flexible extension system, then I had imagined we would use traits, but those traits would be defined as a collection of methods that do not refer to Self:

trait CustomDerive {
     fn foo(); // note: no self
     fn bar(); // again, no self
}

You would then implement it on a dummy type like struct my_annotation_type; (sharing the same name as the attribute, I guess?), and we would use trait resolution to extract the relevant functions like <my_annotation as CustomDerive>::foo (probably when writing out the metadata, I guess). Point is, we'd never make (or need) an instance of my_annotation, it's just a grouping mechanism for lumping together a bunch of related functions.

Certainly not the most elegant thing ever, but I'm not sure of a better way? I think the inelegance is precisely why we wanted to start with attributed functions. =)

nikomatsakis on 1 Nov 2016

Regarding namespaces, @sgrif makes a good case with the #[cfg] and cfg! examples. I can certainly imagine one wanting to #[derive(SomeTrait)] and also have some macro like SomeTrait! { ... } that ... does something. =) But @jseyfried also makes a good case with backwards compatibility -- as long as we aren't hitting limitations _already_.

I tend to prefer fewer namespaces by default, mostly because of the pain that I think having a value/type namespace split brings us now. That said, I think most of the known pain points don't apply here:

the type/value split is a pain in name resolution because use foo::bar might or might not be importing a module named bar, and hence might (or might not) be relevant to a name resolution like bar::baz;
the type/value split is kind of a pain for generics over constants, though that pain is also because of the syntactic split between types and values.

But to a large extent, since we've crossed the bridge, I'm not sure that having "custom derive" live in its own namespace brings any particular challenge, does it?

nikomatsakis on 1 Nov 2016

We could add the derive_ prefix to the generated macros, to make them pseudo-namespaced. If we wanted to make that change, now would be the time.

keeperofdakeys on 1 Nov 2016

@keeperofdakeys Wouldn't this be equivalent to custom derive authors deciding to name their custom derives derive_*? Regardless, I think derive_* names are too ugly for end users.

@nikomatsakis

But to a large extent, since we've crossed the bridge, I'm not sure that having "custom derive" live in its own namespace brings any particular challenge, does it?

The import resolution algorithm can handle arbitrarily many namespaces, but it does a potentially nontrivial amount of work for each namespace. In particular, for each import I and each unused namespace S, it proves that the resolution I fails in S (c.f. https://github.com/rust-lang/rfcs/pull/1560#issuecomment-209119266). This proof often requires a DFS of the glob import graph (in which vertices are modules and edges are glob imports) to search for relevant indeterminate imports.

However, this extra per-namespace work might not make a difference in practice and can be avoided if needed (c.f. https://github.com/rust-lang/rfcs/pull/1560#issuecomment-209119266) at the cost of a minor restriction that would only apply to the macro namespaces.

I merged the namespaces in #37198 to mostly keep our options open and because I didn't think it would be limiting in practice. If people want it today and @rust-lang/lang is okay with having multiple macro namespaces forever, I have no objections.

jseyfried on 2 Nov 2016

@nikomatsakis

I think yeah your strategy would work, albeit wonkily. My personal feeling is that the oddness doesn't pull its weight (e.g. what we have today is fine by me), but it should be implementable whichever way.

alexcrichton on 2 Nov 2016

@alexcrichton as I think I wrote before, I'm happy to wait until we need more power (if ever) -- and then we can do something like the trait I described, or maybe something better. For now I think just extending the attributes applied to a fn would be sufficient.

nikomatsakis on 2 Nov 2016

I've gotten curious, and started a basic implementation of this idea (using an attribute on the proc_macro function to denote the names of attributes to be marked as used on the item).

keeperofdakeys on 3 Nov 2016

👍1

I've cleared the FCP tag because it seems clear we haven't _quite_ reached the point where we are ready to stabilize yet. I'm going to make an effort to summarize the state of the conversation and, in particular, to highlight the errors where we still need firm decisions and/or code contributions:

Question 1: Should custom-derive macros be able to mutate the item they are walking over?

Nobody thinks they ought to do that _in practice_ of course
It's another mode, which was initially rejected in the spirit of YAGNI
Derives that change the set of field names etc don't compose nicely, making order of application visible;
the danger of that is mitigated by two things:
- Change to the field names/types can't really be done transparently
- Safety-critical macros can use fully explicit struct patterns to check that nobody has changed the set of fields
On the other hand, if we say that custom derive only needs to return the new impls
- the span information is better
- custom derives are simpler to write
_But_ many custom derives use custom attributes to guide their expansion, and those will generate warnings/errors
- Current technique is to strip them out of the AST
_Also:_ we want to get this stuff out in the world ASAP, don't want to do lengthy experimentation or drastic changes

Proposals:

Keep everything as is, maybe deprecate later
- Expedient, somewhat unfortunate
Extend #[proc_macro] with a whitelist of attributes, tell rustc to consider them "used" and ignore them
- Simple, covers the main use case
- But somebody needs to implement it (maybe @keeperofdakeys!)

Question 2: Should custom derives share same namespace as other macros?

Argument for:

fewer namespaces is generally better, reduces running time of resolve
backwards compatibility, can't remove namespaces later
capitalization conventions (#[derive(Foo)] vs foo!) already serve the purpose
use foo::Bar can't really distinguish which namespace you mean anyway, so less room for confusion if they don't share

Argument against:

#[cfg()] vs cfg!() occurs today (but neither of these is custom derive, and this could be accommodated by letting macro know the context in which it is executing)
from _user_ perspective, unlikely to be confused about which one people are referring to

Proposals:

Split into "custom derive" namespace
Keep status quo

Other things?

Are there any other open questions?

nikomatsakis on 4 Nov 2016

👍6

Potential solution to the mutation problem:

Instead of custom derives having type TokenStream -> TokenStream, they would instead have type
Item -> TokenStream, where Item is an opaque type that would start with just two methods:

item.tokens(), which returns the TokenStream that would be passed today, and
item.use_attrs(name), which would mark all attributes with the given name as used.

The returned TokenStream would only include the derived impls.
We could eventually add to Item's API with convenience functions (e.g. iterating over the item's fields/variants) or a higher-level deriving API like the one in syntax_ext::deriving::generic.

jseyfried on 4 Nov 2016

👍1

I would be happy to implement allowing white-listed attributes in #[proc_macro_derive] (i.e. the second proposal for question 1 in https://github.com/rust-lang/rust/issues/35900#issuecomment-258315395) or my Item -> TokenStream proposal.

I think it would be a mistake to stabilize custom derives that can mutate the underlying item. The span information that we sacrifice by allowing mutation is already causing problems -- for example, we have to choose between fixing #37563 and fixing #36218.

jseyfried on 4 Nov 2016

👍1

I really don't see the appeal of imperative whitelisting, can anyone come up with an usecase?

eddyb on 4 Nov 2016

I'm not sure, is it intentional/desired that this code compiles:

#![feature(proc_macro)]

#[proc_macro_derive(Whatever)]
struct Foo {}

est31 on 4 Nov 2016

@eddyb I'm not sure if there is an inherent appeal of imperative whitelisting -- I think the benefit of
Item -> TokenStream is that it's easier to extend Item's API than to add more kinds of declarative attributes. That being said, thinking about this some more, there might not be many other use cases that requires an Item over a TokenStream, so TokenStream -> TokenStream with a declarative whitelist might be better.

jseyfried on 4 Nov 2016

Will macros 2.0 supersede this api? If so, extensibility is of no real concern, and the Item -> TokenStream api doesn't seem very compelling.

keeperofdakeys on 4 Nov 2016

@keeperofdakeys According to the RFC:

Overall, this should be considered an incremental step towards an official "macros 2.0".

The intention of macros 1.1 is to be as close as possible to macros 2.0 in spirit and implementation, just without stabilizing vast quantities of features. In that sense, it is the intention that given a stable macros 1.1, we can layer on features backwards-compatibly to get to macros 2.0.

https://github.com/rust-lang/rfcs/pull/1681#issuecomment-233449053 and https://github.com/rust-lang/rfcs/pull/1681#issuecomment-233708395 and https://github.com/rust-lang/rfcs/pull/1681#issuecomment-239558657 seem to indicate that only "limited" deprecations are expected when macros 2.0 arrives. In particular: "the pattern of string-ifying token trees would become non-preferred, if not actually deprecated".

Ixrec on 4 Nov 2016

@eddyb

I really don't see the appeal of imperative whitelisting, can anyone come up with an usecase?

Well, I could imagine a case where the names of the attributes are dynamically generated in some way (based on the name of the type or fields, perhaps?) -- and hence one cannot declare them ahead of time. Seems sort of artificial though, and not like a particularly good practice.

nikomatsakis on 4 Nov 2016

I find @jseyfried's alternative appealing -- not because of the imperative nature of marking attributes as used, but rather because the Item API might become richer over time. i.e., we might support walking the set of fields in a very structured way, provide access to more data (like name resolutions) that we may have at our disposal, etc.

Of course, some of this will come from (_eventually_) some form of standard AST API as well.

nikomatsakis on 4 Nov 2016

A note on timing: I really, really want to see Macros 1.1 get stabilized in the next cycle. The stuff we're blocked on feels, ultimately, fairly minor.

In that spirit, my take on the issues I described:

I am happy with either a declarative #[proc_macro] extension _or_ changing the type to Item. I also think that whichever we choose, we could potentially adopt the other in the future if it seemed like a good idea. I kind of want to go with whichever gets implemented first. =)
Regarding the multiple namespaces, I think we should keep them in the same namespace for now. For me, the combination of backwards compatibility (that is, keeping our options open) with the fact that capitalization already distinguishes "custom derive" macros from other things in practice is compelling. Distinguishing #[cfg] vs cfg! feels like something that we could handle in other ways, or if we want, we can introduce the split namespaces _then_. It doesn't seem like having a unified namespace is harming the custom derive use case in particular, which is the only thing we're talking about stabilizing anyway. Right?

nikomatsakis on 4 Nov 2016

❤4 👍4 🎉1

@nikomatsakis your summary sounds accurate, thanks for writing it up! I'm sad that we won't get macros 1.1 in Rust 1.14, but I understand that these are contentious issues.

My personal feeling remains to stabilize everything as-is where custom derive must strip attributes and there's only one namespace.

I don't quite follow the Item -> TokenStream API, does the returned token stream still encompass the original item or just the added impls? Does that mean it should take &mut Item?

@est31 your comment sounds like a bug, can you open a separate issue for that?

alexcrichton on 4 Nov 2016

👍1

I agree pretty much entirely with @nikomatsakis's comment. I think its important that derives not have free reign to mutate the item they're attached to, but all of the proposed implementations seem fine to me.

It doesn't seem like having a unified namespace is harming the custom derive use case in particular, which is the only thing we're talking about stabilizing anyway. Right?

The issue came up because it has broken diesel, which currently has macros called e.g. Queryable! which you can wrap a struct in to derive Queryable for it.

As someone who neither uses or maintains diesel right now (i.e. as someone who is not impacted by what I am about to propose :sweat_smile:), I think the best thing is to keep 1 namespace for now and for diesel to change the name of those macros to derive_Queryable! or something like that. They'll be deprecated when this is stable anyway, right?

withoutboats on 4 Nov 2016

👍2 ❤1

I've created PR https://github.com/rust-lang/rust/pull/37614 for the suggested feature. It uses a separate attribute #[proc_macro_attributes(..)], and you no longer need to return the item in the returned TokenStream.

keeperofdakeys on 6 Nov 2016

I filed #37637 to figure out how proc macros should treat $crate.

dtolnay on 7 Nov 2016

Just to clarify, is this use-case considered alright or a misuse of the system:

I generate new structs based on the existing struct and attributes here and I quite like the design as it allows me to consolidate things into one struct.

However if the system might change later on so as to disallow this kind of implementation I might just stop now instead of putting more effort into it (the current implementation is just a quick proof of concept).

TheNeikos on 8 Nov 2016

@TheNeikos

The main backwards incompatible change will be that you can no longer modify the item (you don't pass it back in the TokenStream). I'd say that deriving anything other than an impl is not intended, but there is nothing stopping you from doing this. You'll just need to be careful about name clashes.

The other main change is being able to provide a list of attribute names that shouldn't trigger custom attribute errors on the item.

keeperofdakeys on 8 Nov 2016

🎉1 👍1

RE: The namespace conflict in Diesel. I'm not sure if I'll be deprecating those macros or not once this feature is stable, it'll depend on if there's still a desire from some for compiler extension free stuff. It's doubtful. Still useful for testing internally though, but renaming it is fine for that.

I do think it's unfortunate to lose the ability to modify the input stream. Being able to remove the item being annotated lets us have bang macros with this system as well. I understand the concerns, but it's a shame to lose that capability over them.

sgrif on 8 Nov 2016

@TheNeikos @sgrif my point of view is that anything that seriously modifies the input is not a derive and is thus not meant to be addressed here. I think it is somewhat dangerous (lack of hygiene, etc.) to use custom derives for general purpose proc macros, so I'm not keen to encourage it.

nrc on 8 Nov 2016

👍6

Not being able to modify the item means that the item's span information is kept, which makes error messages on the item much clearer (it also means error messages in the derived output no longer point to the item's span, but the derive attribute's span). I can't see a good reason for letting people abuse this feature if we really just want proper procedural macros.

keeperofdakeys on 8 Nov 2016

@TheNeikos I don't think we're going to disallow generating new structs through a derive as long as you don't motify the struct you're deriving for.

In terms of what I think is idiomatic; I think its fine to generate new types, but its a lot better if those types are associated types for a trait you're deriving. For example, imagine if we were able to derive IntoIterator for a type - that would involve creating an Iterator struct. Conceptually, you should be deriving behavior for this type, but that behavior might not be "a trait impl."

withoutboats on 8 Nov 2016

@sgrif

I do think it's unfortunate to lose the ability to modify the input stream. Being able to remove the item being annotated lets us have bang macros with this system as well. I understand the concerns, but it's a shame to lose that capability over them.

Hmm, I am definitely sympathetic, though obviously (as @nrc noted) this is not what the system was designed for, and it will tend to expose the various rough edges. This probably doesn't matter in your use cases. I guess a key question is how promptly we can move on a "bang macros 1.1" sort of thing.

nikomatsakis on 9 Nov 2016

The PR has been merged, so you should be able to see the following changes in the next nightly. The proc_macro_derive function should no longer return the item (doing so will trigger an error about a type being defined twice), and you can now provide a list of attribute names to whitelist like this #[proc_macro_derive(Derive, attributes(Foo, Bar)].

keeperofdakeys on 10 Nov 2016

👍2

cc @dtolnay, @sgrif ^

that'll cause breakage soon unfortunately

alexcrichton on 10 Nov 2016

Yep, I filed https://github.com/serde-rs/serde/issues/614 to track on our end.

dtolnay on 10 Nov 2016

I think I've fixed the breakage in Diesel in https://github.com/diesel-rs/diesel/pull/493, will know for sure once nightlies are building again.

sgrif on 12 Nov 2016

So if I'm reading this thread correctly, since a proc macro can only emit extra items appended to the initial item, it also cannot add more annotations to the initial item? (I see a mention of allowing "sibling annotations" but nothing else about it.)

I have a #[derive(newtype)] proc macro in my (tiny, unpublished) crate that expands to a different set of other #[derive()]s based on the struct it's annotating. For example #[derive(newtype)] struct Foo(u64) expands to #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] struct Foo(u64); whereas #[derive(newtype)] struct Foo(::semver::VersionReq) expands to #[derive(Clone, Debug, PartialEq)] struct Foo(::semver::VersionReq);. So the struct members are not modified by this macro, but other derives are added to it (these don't modify the struct either).

There are a few dozen such structs and each has ten or so new derives after this macro is expanded. I like that if I realize I want all u64 newtypes to implement one more trait, I can just change the set of derivations at one place in the newtype macro code instead of on every single struct.

I used to have a macro_rules newtype! macro for this but switched to a proc macro because:

The presence or absence of doc comments, presence or absence of pub, etc get handled for free without needing a combinatorial number of macro match arms.
Even if I wrote the combinatorial macro match arms, finding an ordering where they didn't conflict with each other was hard.

Arnavion on 14 Nov 2016

Unfortunately no, you can no longer do this as you were doing.

keeperofdakeys on 14 Nov 2016

Regarding future compatibility of this feature being in stable: as the plugin function is not required to be "pure", will it be a breaking change if the order of objects given to the function processed changes in the future, or if rustc implements multi threaded compiling?

est31 on 15 Nov 2016

@est31 If we have time we should try to pull off the IPC isolation thing that has been mentioned around.

eddyb on 15 Nov 2016

I'm consistently seeing an ICE in Diesel after the most recent changes.

../src/librustc_metadata/decoder.rs:490: entry: id not found: DefIndex(1) in crate "diesel_codegen" with number 28

sgrif on 16 Nov 2016

@sgrif that would be issue #37788 which will be fixed by #37793 (let's hope it'll end up in tomorrow's nightly...).

est31 on 16 Nov 2016

@est31 It's too late at this hour to have it merged before the nightly build starts.

eddyb on 17 Nov 2016

https://github.com/rust-lang/rust/issues/37839 is an issue with using a lib crate that itself uses a proc_macro crate. AFAICT none of the tests are affected by this because they either compile only the proc macro module, or a proc macro module and a bin module that directly references the proc macro.

Edit: Now fixed!

Arnavion on 17 Nov 2016

@Arnavion The issue you saw with #37839 is fixed for regular complication, but it remains broken when using --target, as reported in #37958. I've provided a minimal test case using --target which still breaks.

emk on 26 Nov 2016

@rfcbot fcp merge

Now that the whitelisted attribute feature has been implemented, I think we should stabilize this! Serde, Diesel, and other users -- now is your change to object if the current design is not working for you. =)

cc @sgrif @erickt

nikomatsakis on 30 Nov 2016

🎉7

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

[x] @alexcrichton
[x] @aturon
[x] @brson
[x] @eddyb
[x] @japaric
[x] @michaelwoerister
[x] @nikomatsakis
[x] @nrc
[x] @pnkfelix
[x] @vadimcn
[x] @withoutboats
[x] @wycats

No concerns currently listed.

Once these reviewers reach consensus, 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.

rfcbot on 30 Nov 2016

I would love to see bang macros explored in the near future. No objections though.

sgrif on 30 Nov 2016

👍4

@rfcbot reviewed

withoutboats on 30 Nov 2016

Currently if a TokenStream fails to parse an empty LexError is returned. Would it be possible to return a better error message here, like which token failed to parse? Though users of your library probably don't want to see these kinds of error messages.

keeperofdakeys on 1 Dec 2016

Yes, that would be convenient for macro authors. I had to resort to writing the stream out to a file and viewing it in the playground to find errors.

I think users will benefit as well, if only to file better bug reports against the macro.

Arnavion on 2 Dec 2016

👍1

Over on https://github.com/rust-lang/rust/pull/38140, we're forcing custom derive declarations to be public. The theory being that we might one day want private derives and then we'll probably want to make that distinction based on the visibility of the defining function. In any case, this is the conservative choice. I thought I'd let it marinate for a day or two for people to see it before merging, since we're stabilising this feature.

nrc on 3 Dec 2016

👍3

#37480 is closed, which should be reflected in the checklist.

norcalli on 9 Dec 2016

Fixed

sfackler on 9 Dec 2016

@pnkfelix I took the liberty of checking your box for you, since you're on PTO and I believe are totally on board. Please let us know if that's not the case!

aturon on 14 Dec 2016

:bell: This is now entering its final comment period, as per the review above. :bell:

psst @nikomatsakis, I wasn't able to add the final-comment-period label, please do so.

rfcbot on 14 Dec 2016

Does the impending stabilization presume that the remaining known bugs in the checklist at the top will be fixed first? Or are those not blocking for stabilization?

jimmycuadra on 15 Dec 2016

There are two right now:

#36935 has a comment saying its resolved.
#36691 doesn't seem blocking to me, we can allow these to expand to mod foo; someday in the future if we want without breaking anything I believe.

withoutboats on 15 Dec 2016

Hey! This is an RFC 1636 documentation audit. This is the first major feature to stabilize since RFC 1636 was accepted, and we should do a good job of following it in this case.

The RFC states:

Prior to stabilizing a feature, the features will now be documented as follows:

Language features:

must be documented in the Rust Reference.

should be documented in The Rust Programming Language.

may be documented in Rust by Example.

Both language features and standard library changes must include:

a single line for the changelog

a longer summary for the long-form release announcement.

What is the right process for this? Should we add checklist items to the top comment of this issue, or create a new issue for tracking documentation? It seems to me like we should have documentation meeting these requirements in tree by the 1.15 release.

cc @rust-lang/docs

withoutboats on 26 Dec 2016

👍6

Thanks @withoutboats ! It's the first major one, yeah. I had had this on my list to look at this morning, and lo and behold, you beat me to it 😄

My imagining of this was always that the decision to stabilize and the actual stabilization are separate. That is, @rust-lang/lang can say "this can be made stable", but the commit to remove the gate also ensures the documentation exists. In a world where the unstable book exists, this would pull the docs from there into the stable docs; but until then, things are slightly awkward.

Given that we just had a release, my plan was to do something like this:

Wait for this to leave FCP
Land some docs. (I was planning on writing them in this case)
Make the stabilization PR.

Possibly combining two and three.

/cc @rust-lang/core , since this is a cross-team issue.

steveklabnik on 26 Dec 2016

❤3

@steveklabnik that sounds good to me, and I'd be ok landing docs whenever as well (even ahead of FCP finishing). The FCP here is sort of pseudo-finished anyway as we accidentally took a long time to enter.

It'd also be good to get these in quickly so we can ensure a safe backport to 1.15 beta branch!

alexcrichton on 27 Dec 2016

👍1

If I am the first one to hit this it can't be that bad but let's include it under known bugs: using a type macro inside a struct with a custom derive causes an ICE https://github.com/rust-lang/rust/issues/38706

dtolnay on 30 Dec 2016

https://github.com/rust-lang/rust/pull/38737 fixes the type macros ICE :heart:. Any chance of getting this backported to beta? Rationale: it seems bad that two prominent new features, one released in 1.13 and one released in 1.15 crash the compiler when you use them together.

dtolnay on 31 Dec 2016

👍4

I just created #38749 concerning documentation of the proc_macro crate.

abonander on 1 Jan 2017

I’ve read multiple times that Macros 1.1 will be stabilized in 1.15, but 1.15.0-beta.1 shipped two weeks ago and at least extern crate proc_macro; is still feature-gated in it as well as in nightly 4ecc85beb 2016-12-28. Is the plan to backport the stabilization change?

SimonSapin on 2 Jan 2017

👍2

@SimonSapin yes, that was the plan, but we need to make it happen!

nikomatsakis on 2 Jan 2017

It still is the plan :p

steveklabnik on 2 Jan 2017

❤2 😄2

If the user writes #[derive(Foo)] #[foo_def = "definition.json"] struct MyStruct; the macro handler has no way to know what "the current directory" is and thus can't find definition.json.

This is by-design and thus wouldn't be easy to fix, and I guess it's too late to fix this anyway.

tomaka on 13 Jan 2017

You can go Span -> FileMap -> filename -> directory. All that's missing is accessing the information through proc_macro.

eddyb on 13 Jan 2017

You would also need to tell the compiler to add a dependency to definition.json so that the build is dirty if it gets modified.

tomaka on 13 Jan 2017

The proc macro can use env::var("CARGO_MANIFEST_DIR") to get the directory containing Cargo.toml of the crate containing the macro invocation. Presumably foo_def is relative to that. See https://github.com/dtolnay/syn/issues/70#issuecomment-268895281.

dtolnay on 13 Jan 2017

@tomaka that can be done by mutating FileMap, e.g. this is how the include_str! macro does it.

est31 on 13 Jan 2017

Presumably foo_def is relative to that.

I think it's not very intuitive to have to put the path relative to the Cargo.toml.

tomaka on 13 Jan 2017

that can be done by mutating FileMap, e.g. this is how the include_str! macro does it.

Yeah I know it can be done, it just can't be done with the current procedural macros API.

The parameter is an Item. It would be acceptable to add a method to retrieve a span from that Item, but adding a method to Item to ask the compiler to add a dependency would be a hack IMO.

tomaka on 13 Jan 2017

You can go Span -> FileMap -> filename -> directory.

Are these APIs (in particular FileMap) on a path to be stabilized?

SimonSapin on 13 Jan 2017

They don't have to be, in fact I wouldn't want to stabilize any of the internals. We can, instead, stabilize APIs that extract information about a Span (e.g. line, column, filename).

eddyb on 14 Jan 2017

👍4

I just got this error in a crate of mine. What's going on?

``error: Cannot use#![feature(proc_macro)]and#![feature(custom_attribute)] at the same time
````

alexreg on 2 Feb 2017

@alexreg If you're using #[derive], it's stable now. You don't need #![feature(proc_macro)].

abonander on 2 Feb 2017

@alexreg
proc_macro_derives (macros 1.1) are now stable -- you can just remove #![feature(proc_macro)].

#[proc_macro_attribute] recently landed behind the #![feature(proc_macro)] feature gate; these are incompatible with #![feature(custom_attribute)]. #![feature(custom_attribute)] will be deprecated once the replacement lands (https://github.com/rust-lang/rfcs/pull/1755).

jseyfried on 2 Feb 2017

@jseyfried I think we should change the tracking issue on proc_macro since this is closed and doesn't contain relevant information.

abonander on 2 Feb 2017

Thanks guys. That makes sense.

alexreg on 2 Feb 2017

@abonander Yeah, #![feature(proc_macro)] should definitely point to #38356 now -- I should have verified that when reviewing #38842. Could you submit a PR?

jseyfried on 2 Feb 2017

👍1

Rust: Tracking issue for "Macros 1.1" (RFC #1681)

Stabilization TODO

Implementation TODO

Most helpful comment

All 268 comments

Idea 1

Idea 2

TL;DR

Question 1: Should custom-derive macros be able to mutate the item they are walking over?

Question 2: Should custom derives share same namespace as other macros?

Other things?

Related issues