Tracking issue for "Macros 1.1" (RFC #1681) #35900

Closed
nikomatsakis opened this Issue Aug 22, 2016 · 262 comments

Projects

None yet
@nikomatsakis
Contributor
nikomatsakis commented Aug 22, 2016 edited

Tracking issue for rust-lang/rfcs#1681.

cc @alexcrichton

Stabilization TODO

Litmus tests:

Features:

  • Crate name, currently is proc_macro
  • Crate type, currently proc-macro
  • The #[proc_macro_derive(Foo)] attribute
  • Loading proc-macro crates with -L and #[macro_use] to load them
  • shadowing is an error
  • no hygiene
  • passing a token stream for the entire struct and receiving it all back
  • Cargo manifest attribute, currently proc-macro = true

Known bugs:

Implementation TODO

  • - Create a rustc_macro crate
    • - Have librustc_macro link to libsyntax. Depend on librustc_macro in librustc_driver
    • - Tag rustc_macro as unstable with our standard header.
    • - Only tag rustc_macro with #![crate_type = "rlib"], do not produce a dylib.
    • - Implement the API of rustc_macro using libsyntax's TokenStream internally
    • - tag rustc_macro with a TokenStream lang item so the compiler knows about it.
  • - Add rustc_macro_derive attribute
    • - validate it's of the exact form foo(bar), no other arguments/formats
    • - verify it's only applied to functions
    • - verify it's only applied to functions in the root module
    • - verify the signature with the TokenStream lang item added above
    • - encode all function symbols with rustc_macro_derive into metadata along with the derive mode they're used for.
  • - Add a rustc-macro crate type for other crates
    • - wire it up to produce a dylib
    • - ensure the dylib gets metadata
    • - ensure rustc-macro crates cannot be linked as dylibs
    • - ensure there are no reachable items other than those tagged with #[rustc_macro_derive]
    • - Add cfg(rustc_macro) as an unstable cfg directive, set it for the rustc-macro crate type
    • - ensure that rustc-macro crates link dynamically to libsytnax
  • - Fill in #[macro_use] support for rustc-macro crates
    • - extend library loader to find rustc-macro crates separately from dylib/rlib tracked today when loding crates
    • - Parse metadata for rustc-macro crates to learn about symbol/derive mode pairings
    • - dlopen the dylib
    • - generate an error if any derive mode would shadow any other.
  • - Add cargo integeration
    • - recognize rustc-macro similar to plugin = true
    • - pass --crate-type=rustc-macro when depending on it
    • - plumb the same host/target logic for rustc-macro crates as is present for plugin crates (e.g. always compile rustc-macro crates for hosts)
  • - Tests
    • - smoke test loading a rustc-macro, dummy #[derive] trait
    • - name conflicts are an error
    • - compiling for the wrong architecture is an error
    • - cannot emit crate type rustc-macro and anything else (e.g. rustc-macro+dylib) is an error
    • - span information not being horrendous
    • - removing attributes from a struct, as well as fields
    • - adding impls next to a struct
    • - cross compilation looks for the host rustc-macro crate type
    • - don't load vanilla dylibs as rustc-macro crate types
    • - can't have public exports beyond macro derive functions in a rustc-macro crate
    • - derive macros must have required signature
    • - load two macro crates in one compilation
@nikomatsakis nikomatsakis referenced this issue in rust-lang/rfcs Aug 22, 2016
Merged

RFC: Procedural macros 1.1 #1681

@alexcrichton
Member

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
Member

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

@alexcrichton alexcrichton self-assigned this Aug 22, 2016
@alexcrichton
Member

cc #35957, an implementation

@nrc
Contributor
nrc commented Aug 29, 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.

@eternaleye
Contributor
eternaleye commented Aug 29, 2016 edited

@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.
@aturon
Contributor
aturon commented Aug 30, 2016

@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.

@nrc
Contributor
nrc commented Aug 30, 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.

@aturon
Contributor
aturon commented Aug 30, 2016

@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?

@nrc
Contributor
nrc commented Aug 30, 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".

@eternaleye
Contributor

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.

@mystor
Contributor
mystor commented Aug 30, 2016

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

@jimmycuadra

"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.

@nikomatsakis
Contributor

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
Contributor

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.)

@eternaleye
Contributor
eternaleye commented Aug 31, 2016 edited

@nikomatsakis: A related but subtly different question is a use case I brought up in rust-lang/rfcs#1561 (comment) - 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).

@dtolnay
Contributor
dtolnay commented Sep 1, 2016

I have a cargo PR rust-lang/cargo#3064 which should check all the "cargo integration" boxes in the checklist.

@Ericson2314
Contributor
Ericson2314 commented Sep 1, 2016 edited

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.

@dtolnay
Contributor
dtolnay commented Sep 2, 2016

Does serde work?

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

(except for container attributes in some cases #36211)

@alexcrichton
Member

Awesome, thanks for the updates @dtolnay!

@White-Oak
Contributor

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

@dtolnay dtolnay referenced this issue in serde-rs/serde Sep 5, 2016
Closed

Remove dependency on Syntex #540

@Ericson2314
Contributor

OK cargo stuff has landed. That's fine, but it would be nice to revisit #35900 (comment) sometime before stabilization. [For what its worth, I meant to bring this up in the original RFC but forgot.]

@jessstrap

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?

@SimonSapin
Contributor

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.

@sgrif
Contributor
sgrif commented Sep 9, 2016

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.

@alexcrichton
Member

@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.

@colin-kiegel

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?

@alexcrichton
Member

@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!

@colin-kiegel

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?

@alexcrichton
Member

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.

@mystor
Contributor
mystor commented Sep 9, 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?

@alexcrichton
Member

@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.

@colin-kiegel
colin-kiegel commented Sep 10, 2016 edited

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?

@sgrif
Contributor
sgrif commented Sep 10, 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.

@Mark-Simulacrum Mark-Simulacrum added a commit to Mark-Simulacrum/rustc-perf that referenced this issue Sep 10, 2016
@Mark-Simulacrum Mark-Simulacrum Migrate to Macros 1.1 serde_derive, replacing serde_macros.
structural_match and rustc_attrs being included as features are a result of:
  - rust-lang/rust#36211
  - rust-lang/rust#35900 (comment)
46bba63
@Mark-Simulacrum Mark-Simulacrum added a commit to Mark-Simulacrum/rustc-perf that referenced this issue Sep 10, 2016
@Mark-Simulacrum Mark-Simulacrum Migrate to Macros 1.1 serde_derive, replacing serde_macros.
structural_match and rustc_attrs being included as features are a result of:
  - rust-lang/rust#36211
  - rust-lang/rust#35900 (comment)
060523f
@alexcrichton
Member

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 alexcrichton removed their assignment Sep 12, 2016
@nikomatsakis
Contributor

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?

@nrc
Contributor
nrc commented Sep 13, 2016

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).

@nikomatsakis
Contributor

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

@colin-kiegel

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?

@nikomatsakis
Contributor

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

@tomaka
Contributor
tomaka commented Sep 13, 2016 edited

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.

@nikomatsakis
Contributor
nikomatsakis commented Sep 13, 2016 edited

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)

@jimmycuadra

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.

@nikomatsakis
Contributor

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
Contributor
nikomatsakis commented Sep 13, 2016 edited

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. ;)

@mystor
Contributor
mystor commented Sep 13, 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.

@nrc
Contributor
nrc commented Sep 13, 2016

@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
Contributor
nrc commented Sep 13, 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.

@alexcrichton
Member

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.

@jimmycuadra

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.

@mystor
Contributor
mystor commented Sep 13, 2016 edited

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.

@alexcrichton
Member

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.

@mystor
Contributor
mystor commented Sep 13, 2016

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).

@colin-kiegel

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.

@jimmycuadra

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.

@mystor
Contributor
mystor commented Sep 13, 2016

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.

@Mark-Simulacrum Mark-Simulacrum added a commit to Mark-Simulacrum/rustc-perf that referenced this issue Sep 14, 2016
@Mark-Simulacrum Mark-Simulacrum Migrate to Macros 1.1 serde_derive, replacing serde_macros.
structural_match and rustc_attrs being included as features are a result of:
  - rust-lang/rust#36211
  - rust-lang/rust#35900 (comment)
792d028
@eddyb
Member
eddyb commented Sep 14, 2016 edited

@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.

@nikomatsakis
Contributor

@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. ;)

@mystor
Contributor
mystor commented Sep 16, 2016 edited

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)].

@dragostis

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?

@colin-kiegel

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.

@sgrif
Contributor
sgrif commented Sep 17, 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.

@colin-kiegel

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

@colin-kiegel

@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.
@hauleth hauleth added a commit to rust-num/num that referenced this issue Sep 18, 2016
@hauleth hauleth Fix `num-macros` `FromPrimitive` implementation
Current solution follow syntax proposed in rust-lang/rfcs#1681.

Tracking issue rust-lang/rust#35900

Close #227
decfedb
@hauleth hauleth referenced this issue in rust-num/num Sep 18, 2016
Merged

Fix `num-macros` `FromPrimitive` implementation #228

0 of 7 tasks complete
@hauleth hauleth added a commit to rust-num/num that referenced this issue Sep 18, 2016
@hauleth hauleth Fix `num-macros` `FromPrimitive` implementation
Current solution follow syntax proposed in rust-lang/rfcs#1681.

Tracking issue rust-lang/rust#35900

Close #227
11f8289
@nikomatsakis
Contributor
nikomatsakis commented Sep 19, 2016 edited

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.

@shepmaster
Contributor
shepmaster commented Sep 19, 2016 edited

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.

@mystor
Contributor
mystor commented Sep 19, 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.

@colin-kiegel

@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. :-)

@eternaleye
Contributor

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.

@Ericson2314
Contributor
Ericson2314 commented Sep 22, 2016 edited

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.

@eddyb
Member
eddyb commented Sep 22, 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.

@Ericson2314
Contributor

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.

@alexcrichton alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 27, 2016
@alexcrichton alexcrichton rustc: Tweak expansion order of custom derive
This commit alters the expansion order of custom macros-1.1 style `#[derive]`
modes. Instead of left-to-right the expansion now happens in three categories,
each of which is internally left-to-right:

* Old-style custom derive (`#[derive_Foo]`) is expanded
* New-style custom derive (macros 1.1) is expanded
* Built in derive modes are expanded

This gives built in derive modes maximal knowledge about the struct that's being
expanded and also avoids pesky issues like exposing `#[structural_match]` or
`#[rustc_copy_clone_marker]`.

cc #35900
e5e7021
@aturon aturon added the I-nominated label Sep 27, 2016
@aturon
Contributor
aturon commented Sep 27, 2016

Nominated for lang team discussion re: a stabilization plan.

@dtolnay
Contributor
dtolnay commented Sep 27, 2016 edited

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: serde-rs/serde#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 serde-rs/serde#548.

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

@nikomatsakis
Contributor

@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
Contributor

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
Contributor

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:

  1. 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. =)
  2. 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?
@alexcrichton
Member

@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?

@withoutboats
Contributor
withoutboats commented Nov 4, 2016 edited

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 😅), 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?

@keeperofdakeys
Contributor

I've created PR #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.

@dtolnay
Contributor
dtolnay commented Nov 7, 2016

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

@TheNeikos
Contributor

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).

@keeperofdakeys
Contributor
keeperofdakeys commented Nov 8, 2016 edited

@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.

@sgrif
Contributor
sgrif commented Nov 8, 2016

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.

@nrc
Contributor
nrc commented Nov 8, 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.

@keeperofdakeys
Contributor

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.

@withoutboats
Contributor

@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."

@nikomatsakis
Contributor

@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.

@keeperofdakeys
Contributor

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)].

@alexcrichton
Member

cc @dtolnay, @sgrif ^

that'll cause breakage soon unfortunately

@dtolnay
Contributor
dtolnay commented Nov 10, 2016

Yep, I filed serde-rs/serde#614 to track on our end.

@sgrif
Contributor
sgrif commented Nov 12, 2016

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

@Arnavion Arnavion referenced this issue in Arnavion/derive-error-chain Nov 14, 2016
Closed

Switch to new proc_macro_derive #2

@Arnavion

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.
@keeperofdakeys
Contributor

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

@est31
Contributor
est31 commented Nov 15, 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?

@eddyb
Member
eddyb commented Nov 15, 2016

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

@sgrif
Contributor
sgrif commented Nov 16, 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 sgrif referenced this issue in diesel-rs/diesel Nov 16, 2016
Merged

Fix breakage in latest nightlies #493

@est31
Contributor
est31 commented Nov 16, 2016

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

@eddyb
Member
eddyb commented Nov 16, 2016

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

@Arnavion
Arnavion commented Nov 17, 2016 edited

#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!

@tomaka tomaka referenced this issue in tomaka/rouille Nov 25, 2016
Closed

Use a custom derive for POST input structs #50

@emk
Contributor
emk commented Nov 26, 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.

@nikomatsakis
Contributor
nikomatsakis commented Nov 30, 2016 edited

@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

@rfcbot
rfcbot commented Nov 30, 2016 edited

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

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.

@sgrif
Contributor
sgrif commented Nov 30, 2016

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

@withoutboats
Contributor

@rfcbot reviewed

@keeperofdakeys
Contributor

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.

@Arnavion
Arnavion commented Dec 2, 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.

@nrc
Contributor
nrc commented Dec 3, 2016

Over on #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.

@azriel91 azriel91 added a commit to azriel91/autexousious that referenced this issue Dec 4, 2016
@azriel91 azriel91 Modified View to take in Channel Sender
This also modifies the logging test to use serde with the unstable
proc_macro feature. Stability is pending
rust-lang/rust#35900

Issue #17
78ad8bb
@azriel91 azriel91 added a commit to azriel91/autexousious that referenced this issue Dec 5, 2016
@azriel91 azriel91 Modified View to take in Channel Sender
This also modifies the logging test to use serde with the unstable
proc_macro feature. Stability is pending
rust-lang/rust#35900

Issue #17
8045643
@scottlamb scottlamb added a commit to scottlamb/moonfire-nvr that referenced this issue Dec 8, 2016
@scottlamb scottlamb stop using a couple unstable features
It would be nice to build on stable Rust. In particular, I'm hitting
compiler bugs in Rust nightly, such at this one:
rust-lang/rust#38177
I imagine beta/stable compilers would be less problematic.

These two features were easy to get rid of:

* alloc was used to get a Box<[u8]> to uninitialized memory.
  Looks like that's possible with Vec.

* box_syntax wasn't actually used at all. (Maybe a leftover from something.)

The remaining features are:

* plugin, for clippy.
  rust-lang/rust#29597
  I could easily gate it with a "nightly" cargo feature.

* proc_macro, for serde_derive.
  rust-lang/rust#35900
  serde does support stable rust, although it's annoying.
  https://serde.rs/codegen-stable.html
  I might just wait a bit; this feature looks like it's getting close to
  stabilization.
678500b
@norcalli
Contributor
norcalli commented Dec 8, 2016

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

@sfackler
Member
sfackler commented Dec 8, 2016

Fixed

@aturon
Contributor
aturon commented Dec 14, 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!

@rfcbot
rfcbot commented Dec 14, 2016

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

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

@jimmycuadra

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?

@withoutboats
Contributor

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
Contributor

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

@steveklabnik
Contributor

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:

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

Possibly combining two and three.

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

@alexcrichton
Member

@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!

@dtolnay
Contributor
dtolnay commented Dec 30, 2016

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 #38706

@dtolnay
Contributor
dtolnay commented Dec 31, 2016

#38737 fixes the type macros ICE ❤️. 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.

@abonander
Contributor
abonander commented Jan 1, 2017 edited

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

@steveklabnik steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 2, 2017
@steveklabnik steveklabnik Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
64a96ce
@SimonSapin
Contributor

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 4ecc85b 2016-12-28. Is the plan to backport the stabilization change?

@nikomatsakis
Contributor

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

@steveklabnik
Contributor

It still is the plan :p

@alexcrichton alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 2, 2017
@alexcrichton alexcrichton rustc: Stabilize the `proc_macro` feature
This commit stabilizes the `proc_macro` and `proc_macro_lib` features in the
compiler to stabilize the "Macros 1.1" feature of the language. Many more
details can be found on the tracking issue, #35900.

Closes #35900
045f8f6
@steveklabnik steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 3, 2017
@steveklabnik steveklabnik Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
16f76c4
@steveklabnik steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 3, 2017
@steveklabnik steveklabnik Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
c0efdbf
@GuillaumeGomez GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 4, 2017
@GuillaumeGomez GuillaumeGomez Rollup merge of #38770 - steveklabnik:doc-custom-derive, r=nikomatsakis
Document custom derive.

These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.

So far, this PR includes only docs; @alexcrichton said in rust-lang#35900 that he'd be okay with landing them before stabilization; I don't mind either way.
e9926c0
@bors bors added a commit that referenced this issue Jan 4, 2017
@bors bors Auto merge of #38783 - alexcrichton:stabilize-proc-macro, r=nikomatsakis
rustc: Stabilize the `proc_macro` feature

This commit stabilizes the `proc_macro` and `proc_macro_lib` features in the
compiler to stabilize the "Macros 1.1" feature of the language. Many more
details can be found on the tracking issue, #35900.

Closes #35900
95b14a3
@bors bors closed this in #38783 Jan 4, 2017
@steveklabnik steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 4, 2017
@steveklabnik steveklabnik Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
3075c1f
@bors bors added a commit that referenced this issue Jan 4, 2017
@bors bors Auto merge of #38770 - steveklabnik:doc-custom-derive, r=alexcrichton
Document custom derive.

These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.

So far, this PR includes only docs; @alexcrichton said in #35900 that he'd be okay with landing them before stabilization; I don't mind either way.
8b9bc29
@bors bors added a commit that referenced this issue Jan 5, 2017
@bors bors Auto merge of #38770 - steveklabnik:doc-custom-derive, r=alexcrichton
Document custom derive.

These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.

So far, this PR includes only docs; @alexcrichton said in #35900 that he'd be okay with landing them before stabilization; I don't mind either way.
5d994d8
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@alexcrichton @nikomatsakis alexcrichton + nikomatsakis rustc: Stabilize the `proc_macro` feature
This commit stabilizes the `proc_macro` and `proc_macro_lib` features in the
compiler to stabilize the "Macros 1.1" feature of the language. Many more
details can be found on the tracking issue, #35900.

Closes #35900
12b607c
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@steveklabnik @nikomatsakis steveklabnik + nikomatsakis Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
bc46fef
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@alexcrichton @nikomatsakis alexcrichton + nikomatsakis rustc: Stabilize the `proc_macro` feature
This commit stabilizes the `proc_macro` and `proc_macro_lib` features in the
compiler to stabilize the "Macros 1.1" feature of the language. Many more
details can be found on the tracking issue, #35900.

Closes #35900
3b55150
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@steveklabnik @nikomatsakis steveklabnik + nikomatsakis Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
432ab2e
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@alexcrichton @nikomatsakis alexcrichton + nikomatsakis rustc: Stabilize the `proc_macro` feature
This commit stabilizes the `proc_macro` and `proc_macro_lib` features in the
compiler to stabilize the "Macros 1.1" feature of the language. Many more
details can be found on the tracking issue, #35900.

Closes #35900
ce1a38c
@nikomatsakis nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
@steveklabnik @nikomatsakis steveklabnik + nikomatsakis Document custom derive.
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
rust-lang#35900

The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/

Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
2cb7a60
@tomaka
Contributor
tomaka commented Jan 13, 2017 edited

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.

@eddyb
Member
eddyb commented Jan 13, 2017

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

@tomaka
Contributor
tomaka commented Jan 13, 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.

@dtolnay
Contributor
dtolnay commented Jan 13, 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 dtolnay/syn#70 (comment).

@est31
Contributor
est31 commented Jan 13, 2017

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

@tomaka
Contributor
tomaka commented Jan 13, 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
Contributor
tomaka commented Jan 13, 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.

@SimonSapin
Contributor

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

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

@eddyb
Member
eddyb commented Jan 14, 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).

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