Panic iterating over proc_macro::TokenStream

[dtolnay]

In the following proc macro invocation the compiler passes in the input $crate as a TokenStream but panics when iterating over that TokenStream. Specifically the panic happens in next().

rustc 1.31.0-nightly (de9666f 2018-10-31)

repro/src/lib.rs

extern crate proc_macro;
use self::proc_macro::TokenStream;

#[proc_macro]
pub fn repro(input: TokenStream) -> TokenStream {
    input.into_iter().next();
    unimplemented!()
}

testing/src/lib.rs

macro_rules! m {
    () => {
        repro::repro!($crate);
    };
}

m!();

error: proc macro panicked                                                                                                     
 --> src/main.rs:3:9                                                                                                           
  |                                                                                                                            
3 |         repro::repro!($crate);                                                                                             
  |         ^^^^^^^^^^^^^^^^^^^^^^                                                                                             
...                                                                                                                            
7 | m!();                                                                                                                      
  | ----- in this macro invocation                                                                                             
  |                                                                                                                            
  = help: message: `"$crate"` is not a valid identifier

Originally reported in dtolnay/paste#3. Mentioning @softprops.

[eddyb]

cc @petrochenkov Looks like $crate is a single ident so it need splitting.

[dtolnay]

Don't split it, allow $crate as a single ident. Macro_rules does this correctly.

macro_rules! is_ident {
    ($i:ident) => {};
}

macro_rules! check {
    () => {
        is_ident!($crate);
    };
}

check!();

Otherwise all proc macro parsing code will need separate cases for many places that an ident is parsed or stored to also accept $ crate.

[petrochenkov]

This is a tricky case, yes.
$crate produced by declarative macros is accepted by the ident matcher (including in path segments), so it should somehow act as a single identifier at least for compatibility.

[petrochenkov]

Derive macro infra currently does a horrible thing called eliminate_crate_var replacing the $crate segment either with a crate name or with nothing (depending on the context) before passing tokens to the macro.
Fn-like and attribute macros don't do that (thankfully).

@dtolnay
How derive macros treat the is_ident!($crate) case when the $crate is turned into nothing?
Does it happen in practice?

[dtolnay]

I believe we don't currently have any code in any proc macro library specifically for accommodating $crate.

[petrochenkov]

There's one thing I'm sure in - $crate after processing by the decl-macro it belongs to is no longer two tokens, it's one token - reserved/keyword identifier - in all respects.
It's very similar to crate, but slightly different in terms of resolution (Note: I don't think crate with tweaked span/hygiene can emulate $crate, unfortunately).

If we introduced a new keyword, say dollar_crate, with the same meaning, we could easily replace the "processed $crate" with it.
The obvious solution is to extend the lexical grammar of identifiers, relax validation in Ident::new, and treat "$crate" itself as such keyword.
It just doesn't feel right though, e.g. one issue with it is that such "keyword" can't survive stringification and reparsing.

[dtolnay]

I would be fine with accepting "$crate" in Ident::new. Lots of things don't survive stringification and reparsing so that shouldn't be a requirement.

[petrochenkov]

Some status update: the ICE no longer happens because #49219 removed all the validation for tokens coming to proc macros from the compiler, but the underlying issue is still there.

[eddyb]

@petrochenkov Wait there was validation? Where exactly, I might've missed it?

[petrochenkov]

Ident::new(...)/Punct::new(...) instead of unchecked Ident { /* fields */ }/Punct { /* fields */ } like it's done now in proc_macro_server.rs / fn from_internal.

[petrochenkov]

Input from the compiler is kind of controlled, so validation there is not so important as for the user-facing proc_macro::Ident::new, but it still was there exactly to catch unexpected cases / bugs like this issue.

[eddyb]

Before:

            Ident(ident, false) => tt!(self::Ident::new(&ident.as_str(), Span(span))),
            Ident(ident, true) => tt!(self::Ident::new_raw(&ident.as_str(), Span(span))),

After:

            Ident(ident, is_raw) => tt!(Ident {
                sym: ident.name,
                is_raw
            }),

Just looking at this, without remembering exactly what happened, I suspect a rebase killed this accidentally. I don't remember noticing reinterning was happening.

I guess my version also preserves gensyms when it shouldn't?

[petrochenkov]

I guess my version also preserves gensyms ...

Yes.

... when it shouldn't?

I'm not sure what should happen here.
(I'm only sure that we need to get rid of gensyms in general.)

[petrochenkov]

Fixed in #56647