Review proc macro API 1.2 #50473

petrochenkov · 2018-05-06T00:46:20Z

cc #38356

Summary of applied changes:

Documentation for proc macro API 1.2 is expanded.
Renamed APIs: Term -> Ident, TokenTree::Term -> TokenTree::Ident, Op -> Punct, TokenTree::Op -> TokenTree::Punct, Op::op -> Punct::as_char.
Removed APIs: Ident::as_str, use Display impl for Ident instead.
New APIs (not stabilized in 1.2): Ident::new_raw for creating a raw identifier (I'm not sure new_x it's a very idiomatic name though).
Runtime changes:
- Punct::new now ensures that the input char is a valid punctuation character in Rust.
- Ident::new ensures that the input str is a valid identifier in Rust.
- Lifetimes in proc macros are now represented as two joint tokens - Punct('\'', Spacing::Joint) and Ident("lifetime_name_without_quote") similarly to multi-character operators.
Stabilized APIs: None yet.

A bit of motivation for renaming (although it was already stated in the review comments):

With my compiler frontend glasses on Ident is the single most appropriate name for this thing, especially if we are doing input validation on construction. TokenTree::Ident effectively wraps token::Ident or ast::Ident + is_raw, its meaning is "identifier" and it's already named ident in declarative macros.
Regarding Punct, the motivation is that Op is actively misleading. The thing doesn't mean an operator, it's neither a subset of operators (there is non-operator punctuation in the language), nor superset (operators can be multicharacter while this thing is always a single character). So I named it Punct (first proposed in the original RFC, then by @SimonSapin) , together with input validation it's now a subset of ASCII punctuation character category (u8::is_ascii_punctuation).

rust-highfive · 2018-05-06T00:46:30Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2018-05-06T00:50:02Z

@dtolnay @alexcrichton @eddyb @nrc
Could you go through the comments marked with "REVIEW" and tell what you think?

bors · 2018-05-06T06:41:41Z

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

eddyb · 2018-05-06T09:41:51Z

src/libproc_macro/lib.rs

@@ -207,7 +236,7 @@ pub mod token_stream {

 /// `quote!(..)` accepts arbitrary tokens and expands into a `TokenStream` describing the input.
 /// For example, `quote!(a + b)` will produce a expression, that, when evaluated, constructs
-/// the `TokenStream` `[Word("a"), Op('+', Alone), Word("b")]`.
+/// the `TokenStream` `[Word("a"), Punct('+', Alone), Word("b")]`.


Also, Word is now Term, right?

eddyb · 2018-05-06T09:43:11Z

src/libproc_macro/lib.rs

+    /// An identifier or lifetime identifier.
+    Ident(Ident),
+    /// A single punctuation character (`+`, `,`, `$`, etc.).
+    Punct(Punct),


Oh I didn't expect these changes. I should leave most of this up to @alexcrichton (I do not have a strong preference either way).

eddyb · 2018-05-06T09:45:35Z

src/libproc_macro/lib.rs

+                                       '^', '&', '|', '@', '.', ',', ';', ':', '#', '$', '?'];
+        if !LEGAL_CHARS.contains(&ch) {
+            panic!("unsupported character `{:?}`", ch)
+        }


Validation seems really nice, I think previously it's been deferred until conversion to the compiler-internal forms is needed.

eddyb · 2018-05-06T09:45:54Z

src/libproc_macro/lib.rs

 #[derive(Copy, Clone, Debug)]
 #[unstable(feature = "proc_macro", issue = "38356")]
-pub struct Term {
+pub struct Ident {
+    // REVIEW(INTERNAL) Symbol + Span is actually `ast::Ident`! We can use it here.


I can confirm that this would not cause issues with #49219.

eddyb · 2018-05-06T09:47:32Z

src/libproc_macro/lib.rs

@@ -1199,7 +1300,7 @@ impl TokenTree {
            '#' => Pound,
            '$' => Dollar,
            '?' => Question,
-            _ => panic!("unsupported character {}", op),
+            _ => panic!("unsupported character {}", ch),


Can this become unreachable!() now? As only a finite set of characters can be used.

eddyb · 2018-05-06T09:57:51Z

src/libproc_macro/lib.rs

+/// An identifier (`ident`) or lifetime identifier (`'ident`).
+///
+/// REVIEW ATTENTION: `Copy` impl on a struct with private fields.
+/// REVIEW Do we want to guarantee `Ident` to be `Copy`?


If we do keep it Copy, that means we can never soundly offer an &self -> &str API. See #38356 (comment).

I personally don't mind if we remove the Copy here.

alexcrichton · 2018-05-07T19:00:28Z

All the changes here look great to me, thanks @petrochenkov! While we've already FCP'd for stability in #38356 I think the Ident name is probably better than Term and I'm personally ambivalent on Op or Punct, but am happy to go with Punct.

I'd prefer to land this all at once if possible to reduce churn in the ecosystem, so I'd be mostly in favor of going ahead and stabilizing everything in this PR (if you're ok with that). I don't mind doing the work myself for the APIs here and pushing those up (annotation-wise). Along those lines, want to remove as_str as well from this?

alexcrichton · 2018-05-07T19:01:21Z

src/libproc_macro/lib.rs

+/// May fail for a number of reasons, for example, if the string contains unbalanced delimiters
+/// or characters not existing in the language.
+///
+/// REVIEW The function actually panics on any error and never returns `LexError`.


I think this doesn't match the original implementation, when we first started we were sure to return an error I thought...

In any case I'd think it's a bug that this panics, it should definitely be returning an error (but we can fix that later)

alexcrichton · 2018-05-07T19:02:08Z

src/libproc_macro/lib.rs

+/// Creates a token stream containing a single token tree.
+///
+/// REVIEW We don't generally have impls `From<T> for Collection<T>`, but I see why this exists
+/// REVIEW from practical point of view.


Yeah this is currently the main way to go from a TokenTree to a TokenStream, and I think we also accidentally stabilized it already so we're required to keep it.

alexcrichton · 2018-05-07T19:02:44Z

src/libproc_macro/lib.rs

@@ -227,6 +256,9 @@ pub fn quote_span(span: Span) -> TokenStream {
 }

 /// A region of source code, along with macro expansion information.
+///
+/// REVIEW ATTENTION: `Copy` impl on a struct with private fields.
+/// REVIEW Do we want to guarantee `Span` to be `Copy`? Yes.


Yeah I think we're likely to always want Span to be Copy

alexcrichton · 2018-05-07T19:03:30Z

src/libproc_macro/lib.rs

            TokenTree::Literal(ref tt) => tt.fmt(f),
        }
    }
 }

+/// REVIEW the impls below are kind of `From<T> for Option<T>`, not strictly necessary,
+/// REVIEW but convenient. No harm, I guess. I'd actually like to see impls
+/// REVIEW `From<Group/Ident/Punct/Literal> for TokenStream` to avoid stuttering like


I suspect impls like these will grow and come to exist over time yeah, that's definitely the direction that libstd has taken.

alexcrichton · 2018-05-07T19:04:18Z

src/libproc_macro/lib.rs

 /// forms of `Spacing` returned.
+///
+/// REVIEW ATTENTION: `Copy` impl on a struct with private fields.
+/// REVIEW Do we want to guarantee `Punct` to be `Copy`?


I think this is less critical than Span to be copy, and wouldn't mind going either way here. Making this not-Copy is probably the most conservative thing to do

I agree, let's go with no Copy.

alexcrichton · 2018-05-07T19:04:42Z

src/libproc_macro/lib.rs

    /// which can be further configured with the `set_span` method below.
+    ///
+    /// REVIEW Why we even use `char` here? There's no reason to use unicode here.
+    /// REVIEW I guess because it's more convenient to write `new('+')` than `new(b'+')`, that's ok.


Yes let's stick to char for maximal future flexibility.

alexcrichton · 2018-05-07T19:05:12Z

src/libproc_macro/lib.rs

+    ///
+    /// REVIEW Again, there's no need for unicode here,
+    /// REVIEW except for maybe future compatibility in case Rust turns into APL,
+    /// REVIEW but if it's more convenient to use `char` then that's okay.


Yes for future flexibility I think char's the way to go here

dtolnay

The reason we decided against TokenTree::Ident was because the matcher $i:ident does not accept lifetimes. I would be opposed to calling it Ident here if it can hold lifetimes and $i:ident cannot.

petrochenkov · 2018-05-07T21:00:33Z

@dtolnay
I'd be totally okay with accepting lifetimes in $i: ident (with or without dropping $l: lifetime) if we are ready to cement lifetimes as a "lexical class inside identifiers" as opposed to "' plus an identifier" (which proc macro API already does).
There's internals thread about this https://internals.rust-lang.org/t/pre-rfc-splitting-lifetime-into-two-tokens/6716/10. I don't think the original suggestion is a good idea anymore, but see https://internals.rust-lang.org/t/pre-rfc-splitting-lifetime-into-two-tokens/6716/11 in particular about the "lexical class inside identifiers" alternative.

The key questions here are probably:

Is it important to discern between identifiers and lifetimes in procedural macros? Why?
- We want to minimize API surface. It's always possible to detect lifetimes using the first character.
Is it important to discern between identifiers and lifetimes in declarative macros? Why?
- I'd personally want to minimize the language surface (and simplify compiler as well if we drop $l: lifetime). It's possible to detect lifetimes in macros left sides if we keep lifetime as a subset of ident (kinda like path is a subset of ty/expr/pat) or allow lifetime idents in macro "variables" ($'var).

petrochenkov · 2018-05-07T21:01:45Z

@alexcrichton

I'd prefer to land this all at once if possible to reduce churn in the ecosystem, so I'd be mostly in favor of going ahead and stabilizing everything in this PR (if you're ok with that). I don't mind doing the work myself for the APIs here and pushing those up (annotation-wise). Along those lines, want to remove as_str as well from this?

Sure, I'll add stable annotations and remove as_str.

eddyb · 2018-05-07T21:03:07Z

Is it important to discern between identifiers and lifetimes in declarative macros? Why?

Probably to parse anything looking like a list of generics, if you want to tell apart 'a and T.

dtolnay · 2018-05-07T21:13:48Z

It's possible to detect lifetimes in macros left sides if we keep lifetime as a subset of ident

I would need to play with this in nightly before I would feel comfortable committing to it. What I wouldn't want is:

We stabilize TokenTree::Ident under the expectation that we will make $:ident accept lifetimes.
We change $:ident in decl macros to accept lifetimes, while keeping $:lifetime.
After some use we discover this is worse this way than having $:ident and $:lifetime disjoint.
We revert $:ident to reject lifetimes.
Now proc macro and decl macro are inconsistent in an obnoxious way.

dtolnay · 2018-05-07T21:23:18Z

Probably to parse anything looking like a list of generics, if you want to tell apart 'a and T.

Hmm, can we imagine any macros that would need to handle lifetime parameters and type parameters differently? There is some appeal in being able to say the distinction never matters until much later in the compile process.

dtolnay · 2018-05-07T21:27:24Z

One place I can imagine running into trouble is if a macro somehow needs to handle optional lifetimes on a reference.

macro_rules! m {
    (& $($p:lifetime)? $i:ident) => {}
}

m!(&foo);
m!(&'a foo);

error: local ambiguity: multiple parsing options: built-in NTs lifetime ('p') or ident ('i').
 --> src/main.rs:4:5
  |
4 | m!(&foo);
  |     ^^^

petrochenkov · 2018-05-07T22:11:06Z

@dtolnay

local ambiguity: multiple parsing options: built-in NTs lifetime ('p') or ident ('i').

This is a weird error. I never investigated how our macro matching works in detail, but I expected it to be modeled after regular expressions (greedy by default) and work in both cases, whether lifetime is a subset of ident or not.

extern crate regex;
use regex::Regex;

fn main() {
    let not_subset = Regex::new(r"a?b").unwrap();
    assert!(not_subset.is_match("ab"));
    assert!(not_subset.is_match("b"));
    let subset = Regex::new(r"a?\w").unwrap();
    assert!(subset.is_match("ab"));
    assert!(subset.is_match("aa"));
    assert!(subset.is_match("b"));
    assert!(subset.is_match("a"));
}

http://play.rust-lang.org/?gist=372d5a5f25219670dabc89a1dfb536ba&version=nightly&mode=debug

petrochenkov · 2018-05-08T01:22:55Z

We have 6 weeks if we want to stabilize the API in 1.28.

@eddyb
Can the lifetime question get a slot in the next lang team meeting?

Q1. Treating lifetimes as a lexical subset of identifiers (identifier = '? XID_Start XID_Continue*), closing https://internals.rust-lang.org/t/pre-rfc-splitting-lifetime-into-two-tokens/6716/11. Procedural macros currently treat lifetimes like this.
Q2. Accepting lifetimes in $i: ident matcher in declarative macros, delaying stabilization of $l: lifetime (Tracking issue for adding a lifetime specifier to macro_rules! #34303) until a good use case not covered by ident is found.
Q3. How to refer to both identifiers and lifetime identifiers if they are kept separate. Is "term" previously used by proc macro API a good name?

Remove unnecessary proc-macro-related `feature`s

petrochenkov · 2018-05-15T22:23:32Z

@bors r=alexcrichton

Stabilization commit for later cherry-picking: petrochenkov@3e71e49

bors · 2018-05-15T22:23:33Z

📌 Commit dab8c0a has been approved by alexcrichton

kennytm · 2018-05-16T11:15:35Z

@bors p=1

Unblocks #49219

bors · 2018-05-16T11:18:13Z

⌛ Testing commit dab8c0a with merge 2a3f536...

@SimonSapin

Review proc macro API 1.2 cc #38356 Summary of applied changes: - Documentation for proc macro API 1.2 is expanded. - Renamed APIs: `Term` -> `Ident`, `TokenTree::Term` -> `TokenTree::Ident`, `Op` -> `Punct`, `TokenTree::Op` -> `TokenTree::Punct`, `Op::op` -> `Punct::as_char`. - Removed APIs: `Ident::as_str`, use `Display` impl for `Ident` instead. - New APIs (not stabilized in 1.2): `Ident::new_raw` for creating a raw identifier (I'm not sure `new_x` it's a very idiomatic name though). - Runtime changes: - `Punct::new` now ensures that the input `char` is a valid punctuation character in Rust. - `Ident::new` ensures that the input `str` is a valid identifier in Rust. - Lifetimes in proc macros are now represented as two joint tokens - `Punct('\'', Spacing::Joint)` and `Ident("lifetime_name_without_quote")` similarly to multi-character operators. - Stabilized APIs: None yet. A bit of motivation for renaming (although it was already stated in the review comments): - With my compiler frontend glasses on `Ident` is the single most appropriate name for this thing, *especially* if we are doing input validation on construction. `TokenTree::Ident` effectively wraps `token::Ident` or `ast::Ident + is_raw`, its meaning is "identifier" and it's already named `ident` in declarative macros. - Regarding `Punct`, the motivation is that `Op` is actively misleading. The thing doesn't mean an operator, it's neither a subset of operators (there is non-operator punctuation in the language), nor superset (operators can be multicharacter while this thing is always a single character). So I named it `Punct` (first proposed in [the original RFC](rust-lang/rfcs#1566), then [by @SimonSapin](#38356 (comment))) , together with input validation it's now a subset of ASCII punctuation character category (`u8::is_ascii_punctuation`).

bors · 2018-05-16T13:33:09Z

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

This commit fixes an accidental regression from rust-lang#50473 where lifetime tokens produced by procedural macros ended up getting lost in translation in the compiler and not actually producing parseable code. The issue lies in the fact that a lifetime's `Ident` is prefixed with `'`. The `glue` implementation for gluing joint tokens together forgot to take this into account so the lifetime inside of `Ident` was missing the leading tick! The `glue` implementation here is updated to create a new `Symbol` in these situations to manufacture a new `Ident` with a leading tick to ensure it parses correctly. Closes rust-lang#50942

…petrochenkov rustc: Fix procedural macros generating lifetime tokens This commit fixes an accidental regression from rust-lang#50473 where lifetime tokens produced by procedural macros ended up getting lost in translation in the compiler and not actually producing parseable code. The issue lies in the fact that a lifetime's `Ident` is prefixed with `'`. The `glue` implementation for gluing joint tokens together forgot to take this into account so the lifetime inside of `Ident` was missing the leading tick! The `glue` implementation here is updated to create a new `Symbol` in these situations to manufacture a new `Ident` with a leading tick to ensure it parses correctly. Closes rust-lang#50942

rust-highfive assigned alexcrichton May 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2018

petrochenkov mentioned this pull request May 6, 2018

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks

eddyb reviewed May 6, 2018

View reviewed changes

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2018

petrochenkov changed the title ~~[WIP] Review proc macro API 1.2~~ Review proc macro API 1.2 May 7, 2018

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2018

alexcrichton reviewed May 7, 2018

View reviewed changes

dtolnay previously requested changes May 7, 2018

View reviewed changes

petrochenkov mentioned this pull request May 8, 2018

stabilize macro_lifetime_matcher #50385

Merged

eddyb added the I-nominated label May 8, 2018

petrochenkov added 4 commits May 15, 2018 23:24

proc_macro: Validate inputs to Punct::new and Ident::new

780616e

Address feedback, remove remaining review comments, add some more docs

5b820a6

Represent lifetimes as two joint tokens in proc macros

c106125

Fix stability annotations for already stable bits of proc macro API 1.1

dab8c0a

Remove unnecessary proc-macro-related `feature`s

petrochenkov force-pushed the pmapi branch from a2e7f6c to dab8c0a Compare May 15, 2018 22:18

petrochenkov changed the title ~~Review and stabilize proc macro API 1.2~~ Review proc macro API 1.2 May 15, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2018

bors merged commit dab8c0a into rust-lang:master May 16, 2018

alexcrichton mentioned this pull request May 16, 2018

Track upstream proc_macro changes dtolnay/proc-macro2#90

Merged

hcpl mentioned this pull request May 21, 2018

Convenience trait implementations on Ident dtolnay/proc-macro2#93

Closed

This was referenced May 21, 2018

Build failures in Nightly due to Proc macro API 1.2 rust-lang/futures-rs#1018

Closed

Build failures in Nightly due to Proc macro API 1.2 changes rwf2/Rocket#646

Closed

Build failures in Nightly due to Proc macro API 1.2 rustwasm/wasm-bindgen#203

Closed

petrochenkov mentioned this pull request May 21, 2018

Lifetimes in procedural macros aren't being parsed correctly #50942

Closed

alexcrichton mentioned this pull request May 21, 2018

rustc: Fix procedural macros generating lifetime tokens #50946

Merged

tirkarthi mentioned this pull request May 22, 2018

Bump proc macros 2 module to latest version rust-num/num-derive#13

Closed

lambda-fairy mentioned this pull request May 27, 2018

Crate failed to build rust-lang/docs.rs#23

Closed

petrochenkov mentioned this pull request Jul 5, 2018

rustc: Stabilize the proc_macro feature #52081

Merged

petrochenkov deleted the pmapi branch June 5, 2019 16:05

dtolnay mentioned this pull request Oct 1, 2020

Replace ancient op naming dtolnay/proc-macro2#263

Merged

petrochenkov mentioned this pull request Jan 22, 2022

Store NtLiteral without generalizing to Expr #92392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review proc macro API 1.2 #50473

Review proc macro API 1.2 #50473

petrochenkov commented May 6, 2018 •

edited

Loading

rust-highfive commented May 6, 2018

petrochenkov commented May 6, 2018

bors commented May 6, 2018

eddyb May 6, 2018

eddyb May 6, 2018

eddyb May 6, 2018

eddyb May 6, 2018

eddyb May 6, 2018

eddyb May 6, 2018

dtolnay May 7, 2018

alexcrichton commented May 7, 2018

alexcrichton May 7, 2018

alexcrichton May 7, 2018

alexcrichton May 7, 2018

alexcrichton May 7, 2018

alexcrichton May 7, 2018

dtolnay May 7, 2018

alexcrichton May 7, 2018

alexcrichton May 7, 2018

dtolnay left a comment

petrochenkov commented May 7, 2018 •

edited

Loading

petrochenkov commented May 7, 2018

eddyb commented May 7, 2018

dtolnay commented May 7, 2018

dtolnay commented May 7, 2018

dtolnay commented May 7, 2018

petrochenkov commented May 7, 2018

petrochenkov commented May 8, 2018 •

edited

Loading

petrochenkov commented May 15, 2018

bors commented May 15, 2018

kennytm commented May 16, 2018

bors commented May 16, 2018

bors commented May 16, 2018

Review proc macro API 1.2 #50473

Review proc macro API 1.2 #50473

Conversation

petrochenkov commented May 6, 2018 • edited Loading

rust-highfive commented May 6, 2018

petrochenkov commented May 6, 2018

bors commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

petrochenkov commented May 7, 2018 • edited Loading

petrochenkov commented May 7, 2018

eddyb commented May 7, 2018

dtolnay commented May 7, 2018

dtolnay commented May 7, 2018

dtolnay commented May 7, 2018

petrochenkov commented May 7, 2018

petrochenkov commented May 8, 2018 • edited Loading

petrochenkov commented May 15, 2018

bors commented May 15, 2018

kennytm commented May 16, 2018

bors commented May 16, 2018

bors commented May 16, 2018

petrochenkov commented May 6, 2018 •

edited

Loading

petrochenkov commented May 7, 2018 •

edited

Loading

petrochenkov commented May 8, 2018 •

edited

Loading