Skip to content

Conversation

steveklabnik
Copy link
Contributor

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.

@rust-highfive
Copy link
Contributor

@steveklabnik: no appropriate reviewer found, use r? to override

@steveklabnik
Copy link
Contributor Author

/cc @rust-lang/docs @rust-lang/lang

@est31
Copy link
Member

est31 commented Jan 2, 2017

Would it make sense to document it in the reference as well? Derive is documented there too.

@steveklabnik
Copy link
Contributor Author

Would it make sense to document it in the reference as well? Derive is documented there too.

Ah ha! I knew I forgot something. Yes.

Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Added a few inline comments with suggestions :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be src/main.rs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a sentence about naming convention? I've seen a bunch of crates that call these <name>_derive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link "syn" and "quote" to their repositories or crates.io pages?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People who don't know nom will be confused by this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowercase "rust", same a few times below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double space after sentence here and a few times below; I don't think the rest of the book does this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put "String" in back ticks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe "serializing rust code" → "generating Rust code"?

Copy link
Contributor

@killercup killercup Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling probably deserves a short paragraph as well, even if it's just "panicking the proc macro code will show it as a compiler error pointing to the derive".

@withoutboats
Copy link
Contributor

How do you feel about making HelloWorld into a trait? Implementing traits is intended as the normal use case for derive, and it seems odd to document it with a non-trait impl (though of course that does work).

@steveklabnik
Copy link
Contributor Author

Okay! I think I've fixed up everything, @withoutboats @killercup . Thanks for the review 😄

Still have to update the reference, should push that in a minute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this path uses backslashes, the rest of the book probably not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little weird to me that ## Macros is a subheading within # Macros, but I see why you did it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i would like to re-do this and other parts of the reference but i'm trying to be minimally invasive here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier in this section, "derive" is styled as

`derive`

with backticks, so might be good to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/t will/it will/

It looks like crate types are styled with backticks, so also:

s/proc-macro/`proc-macro`/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/rustc-macro/`rustc-macro`/

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed like a decent summary. The one thing I would say is that I would have expected a bit more up-front material discussing the 'basic model' for derive (string in, string out, you do some parsing and serialization in the model), but that's because I tend towards more of a "top-down" style, I think, whereas this is kind of "bottom-up". (i.e., here is the code to write, let's explain why you wrote it) In any case, all the important stuff seems to be there, and it reads pretty smoothly.

I left some nits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: procedural

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/repition/repetition/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/Procedrual/Procedural/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/Procedrual/Procedural/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am... really bad at this spelling apparently, haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stream of consciousness bit felt a little over the top to me, but it's fine. =)

@steveklabnik
Copy link
Contributor Author

The one thing I would say is that I would have expected a bit more up-front material discussing the 'basic model' for derive

I'm not totally opposed to this; I was thinking about making it future proof. That is, right now it's string in string out because it's a hack. It's really TokenStream -> String -> process -> String -> TokenStream, and ideally those string bits are going away. But I guess it will be a long time...

@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 2, 2017
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we’re continuing the tradition of inconsistent capitalisation—nay, taking it to a new level, with mixed title and sentence case all in one line!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah "Derive" felt like something that deserved a capital here, maybe not, idk

@F001
Copy link
Contributor

F001 commented Jan 3, 2017

Thanks for writing the doc. I can get the basic concept of proc-macro now, but still haunted by some questions.

  1. Is it possible to write this macro without any third party dependency? What if we create it from scratch? What kind of benefit we can get from syn&quote crate?
  2. Why does TokenStream type exist, if the only purpose of it is to be converted to a String? It is weird to convert a structured type to a String, and then parse. I would like to see more explanation about what kind of API does the proc-macro feature provide for us, not the API of a third party lib.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably continue to document compiler plugins in the reference for the time being; they still exist & will for some time, and of course some project are heavily reliant on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't removing documentation; it's re-ordering it. I thought putting macros by example first was a better way of doing things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it went from Compiler plugins, Macros to Macros, Procedural Macros, shouldn't it be Macros, Procedural Macros, Compiler plugins?

Copy link
Contributor Author

@steveklabnik steveklabnik Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that procedural macros are compiler plugins. That is, today "compiler plugins" are "using libsyntax to extend the compiler" but that's never going to be stable, only the new interface we're calling "procedural macros"

The names of all of this stuff has been very hard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, but we've implemented (and now stabilized) the new procedural macros interface only for controlling derive macros. Users who want to define custom attributes or function style procedural macros (without using a hack) are still using the never-to-be-stabilized compiler plugin interface.

Until we've fully implemented procedural macros, compiler plugins will still be used by these users and so we shouldn't drop whatever docs we have on it (even if we don't focus any attention on improving it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't actually remove any of those docs generally, just the small description here. Let me fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/crated/created/

@steveklabnik
Copy link
Contributor Author

@F001

Is it possible to write this macro without any third party dependency? What if we create it from scratch? What kind of benefit we can get from syn&quote crate?

Sure, but it's much, much, much harder. The benefits are that syn and quote do parsing and generation for you.

Why does TokenStream type exist, if the only purpose of it is to be converted to a String? It is weird to convert a structured type to a String, and then parse.

This is the difference between macros 1.1 and macros 2.0, in a nutshell.

I would like to see more explanation about what kind of API does the proc-macro feature provide for us, not the API of a third party lib.

This is the entire API. That's it. There's nothing else to explain. It is extremely bare-bones.

@steveklabnik
Copy link
Contributor Author

Okay, I believe that I've addressed everyone's comments now, both on GitHub and in the text. I've squashed, since this is going to need to be backported, and want to make that easy.

@nikomatsakis
Copy link
Contributor

Let's do it.

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 3, 2017

📌 Commit eb2c83f has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors rollup

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 3, 2017
@steveklabnik
Copy link
Contributor Author

@bors: r=nikomatsakis rollup (#38770 (comment))

@bors
Copy link
Collaborator

bors commented Jan 3, 2017

📌 Commit c0efdbf has been approved by nikomatsakis

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 4, 2017
…ikomatsakis

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.
@alexcrichton
Copy link
Member

@bors: rollup- p=1

Let's land this quickly so we can backport

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

🔒 Merge conflict

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

☔ The latest upstream changes (presumably #38783) made this pull request unmergeable. Please resolve the merge conflicts.

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.
@steveklabnik
Copy link
Contributor Author

@bors: r=alexcrichton p=1

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

📌 Commit 3075c1f has been approved by alexcrichton

moment, procedural macros need to be in their own crate. Eventually, this
restriction may be lifted, but for now, it's required. As such, there's a
convention; for a crate named `foo`, a custom derive procedural macro is called
`foo-derive`. Let's start a new crate called `hello-world-derive` inside our
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the convention is foo_derive not foo-derive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package names are supposed to prefer - over _.

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

⌛ Testing commit 3075c1f with merge 8b9bc29...

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

💔 Test failed - status-travis

@steveklabnik
Copy link
Contributor Author

@bors: retry

(travis is spurious)

@steveklabnik
Copy link
Contributor Author

@bors: force

@bors
Copy link
Collaborator

bors commented Jan 5, 2017

⌛ Testing commit 3075c1f with merge 5d994d8...

bors added a commit that referenced this pull request Jan 5, 2017
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.
@bors
Copy link
Collaborator

bors commented Jan 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 5d994d8 to master...

@bors bors merged commit 3075c1f into rust-lang:master Jan 5, 2017
@nikomatsakis nikomatsakis mentioned this pull request Jan 6, 2017
17 tasks
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.