-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document the proc_macro
feature in the Unstable Book
#41476
Conversation
} | ||
``` | ||
|
||
`my_macro_user/main.rs`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to show adding the dependency on my_macro_crate
to my_macro_user/Cargo.toml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe at the top where I show adding the [lib] proc-macro = true
section, so it doesn't get repetitive? Or do we want to make the dependency relationship clearer in this specific example (and presumably in its sibling below as well)?
I realized I used "which" a lot and now it's bothering me. |
102a955
to
aae1e64
Compare
I would like to have doctesting work for the code examples but I don't think doctests can test code snippets as separate crates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great start 👍
|
||
#### Error Reporting | ||
|
||
Currently the only support for error reporting from procedural macros is through panics. Any panics in a procedural |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, it's better to write things in a "timeless" way, if that makes any sense. Basically, if we miss updating these docs when different error reporting stuff is added, does this still make sense? With this phrasing, it doesn't, but if you remove the first sentence, it does. We might be able to find a way to re-phrase the first sentence too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the convention you want to enforce, I won't argue it.
But in my mind, saying "currently" tends to encourage the reader to be a little more forgiving with outdated information (context helps, being the "unstable" book and all). If that first bit was dropped so that it read "The only support for error reporting [...]" and then that fact changed later without the book updating, it would seem... disingenuous? Something like that.
I would also note that using "currently" tells the reader to expect this aspect to change/improve in the future, though I guess they've probably already acknowledged that by this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, after some consideration, dropping the whole first sentence seems okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(context helps, being the "unstable" book and all)
I think this might be it, that is, it's true that for now, this stuff is not too stable, but we also don't want to do too much massaging once things actually need to go in stable places. It's possible I'm thinking too much of the future here.
I would also note that using "currently" tells the reader to expect this aspect to change/improve in the future,
Right, so the problem with this has been that in the future, changes get made, and then docs don't change, and then they're left with the incorrect impression that changes never happened. It's just tough.
Regardless, all of this is fairly minor and I'm happy to land it either way, to be honest. As you said, it is the unstable book 😄
Currently the only support for error reporting from procedural macros is through panics. Any panics in a procedural | ||
macro implementation will be caught by the compiler and turned into an error message pointing to the problematic | ||
invocation. Thus, it is important to make your panic messages as informative as possible: use `Option::expect()` | ||
instead of `Option::unwrap()` and `Result::expect()` instead of `Result::unwrap()`, and inform the user of the error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no ()
s on function names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, I like ()
s because it distinguishes functions from public fields in this kind of context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer it, unfortunately, the RFC process yielded different results 😄
* A `Display` impl which writes out the contained tokens. Of course, the blanket `ToString` impl applies | ||
to this type as well. | ||
|
||
### Function-like / Bang-style Procedural Macros |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do need to sort out these names at some point 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally leaning towards "function-like" just because it sounds more formal. "bang" or "bang-style" just does not sound like it belongs in a technical document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, is mentioning both all that bad? It might help the reader if they later come across discussions where the two terms are used interchangeably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nudging people toward proper use is a good idea, IMHO. there's no reason to have two names.
} | ||
``` | ||
|
||
`my_macro_user/main.rs`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be nice
**Note**: invocations of this kind of macro require a wrapping `[]`, `{}` or `()` like regular macros, but these do not | ||
appear in the input, only the tokens between them. The tokens between the braces do not need to be valid Rust syntax. | ||
|
||
`my_macro_crate/lib.rs`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should steal the formatting from the book:
Check out the *src/main.rs* file:
<span class="filename">Filename: src/main.rs</span>
```rust
fn main() {
println!("Hello, world!");
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what point does it stop being "stealing" and becomes "following convention"? 😄
@steveklabnik Requested revisions made. Now I'm mainly just waiting for an executive decision from either @nrc or @jseyfried on the preferred terminology as mentioned above. |
@steveklabnik Weird error in the test failure, it's trying to test a slice of an ignored code block...? |
I suspect that the placing of the filename directly above the code block is causing the parser (hoedown? pulldown? I don't recall what we're at right now) to misinterpret the code block somehow. Perhaps a blank line would be a good idea. |
Worth a shot but the other code blocks work fine so it's weird. It's also starting to interpret the code block halfway through it as far as I can tell. |
/cc @GuillaumeGomez for the weird errors |
We're back to hoedown so it should be the old (and long running) doc test system. Ping me if you can't figure it out once tests finished. |
Huh, looks like these latest test failures are truly unrelated. |
@steveklabnik Can you get bors to retry the build? |
@bors: retry |
@steveklabnik it looks like some updates have happened since you last took a look I think, mind taking another look? |
Still waiting on answers to the questions in the OP as well. @jseyfried @nrc |
This looks great to me; we still have the naming question to resolve though. Unsure exactly how best to do that... |
@jseyfried @nrc Is there a chance you could take a look and provide some thoughts on the naming? Specifically, what do we call function-like / bang-macros? note: Also pinged @nrc on IRC; jseyfried wasn't present. |
Hey, sorry I missed the earlier pings - I was on parental leave and then had to rather forcefully clear my inbox. I definitely prefer 'function-like' to 'bang' macros - I think it is highly likely that readers will not associate |
|
||
* Attribute procedural macros which can be applied to any item which built-in attributes can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call these 'attribute-like' since they only look like attributes and are implemented differently - also for symmetry with 'function-like'
the syntax is valid but not in the context in which the macro is invoked, an error is thrown outside the macro. | ||
|
||
* A `Display` impl which writes out the contained tokens. Of course, the blanket `ToString` impl applies | ||
to this type as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't describe these as the main APIs - they are kind of a stop-gap measure for custom derives. Macro authors should prefer using quote!
to create tokens. (And of course more APIs will be arriving in the future).
|
||
### Function-like / Bang-style Procedural Macros | ||
|
||
These are procedural macros that are invoked like `macro_rules!` macros. They are declared as public functions in crates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminology nit - I prefer 'declarative macros' to 'macro_rules! macros' since the former is a descriptive name and the latter is a detail of how they are (currently) declared.
@nrc nits fixed |
r? @nrc |
@bors: r+ |
proc_macro
feature in the Unstable Bookproc_macro
feature in the Unstable Book
5cc69d7
to
e4208d5
Compare
@nrc squashed and PR post cleaned up. |
@bors r=nrc |
📌 Commit e4208d5 has been approved by |
⌛ Testing commit e4208d5 with merge 88d44aa... |
💔 Test failed - status-travis |
e4208d5
to
0089e22
Compare
Can I get an @bors retry |
Ugh, it's hitting a wall with the broken-link-checker, on a relative link to the chapter for |
Hm, I am not sure why it fails in that case, looks right to me. @frewsxcv any ideas? |
It looks like CI is still failing?
|
@abonander can you try rebasing this off master? there's a chance this could be fixed with #41992, not entirely sure though what's going on with the linkchecker |
0089e22
to
82488df
Compare
Rebased, let's see. |
82488df
to
e616d12
Compare
I think the link is supposed to be to |
So it turns out that for subdirectories (like Anyways, tests seem to be passing now so: @bors r=nrc |
📌 Commit e616d12 has been approved by |
Document the `proc_macro` feature in the Unstable Book Discusses the `proc_macro` feature flag and the features it enables: * Implicit enable of `extern_use_macros` feature and how to import proc macros * Error handling in proc macros (using panic messages) * Function-like proc macros using `#[proc_macro]` and a usage example for creating and invoking * Attribute-like proc macros using `#[proc_macro_attribute]` and a usage example for creating and invoking [Rendered](https://github.com/abonander/rust/blob/book_proc_macro/src/doc/unstable-book/src/language-features/proc-macro.md)
☀️ Test successful - status-appveyor, status-travis |
Discusses the
proc_macro
feature flag and the features it enables:extern_use_macros
feature and how to import proc macros#[proc_macro]
and a usage example for creating and invoking#[proc_macro_attribute]
and a usage example for creating and invokingRendered