Macros 2.0: Negative numbers as a single token

[alexcrichton]

@ngg ran into some weird bugs and made an awesome reduction which boils down to:

#[proc_macro]
pub fn buggy_macro(_: TokenStream) -> TokenStream {
    TokenNode::Literal(Literal::i32(-1)).into()
}

It turns out that when expanding this rustc will print out:

error: int literal is too large

Originating from here I think that this is actually a misdiagnosed error in the sense that the parse is failing because the string is "-1" which isn't parseable as a u128, but the current code assumes that it can only fail due to overflow. Generally this is correct because the parser never feeds minus signs to this function!

So the broader issue here I believe is that Literal::i32 can't actually exist. Our parser, as-is today, (and tokenizer?) dictates that the source string -1 is lexed as two tokens, the minus and then the 1. This means that Literal::i32 pretending it can only be one token isn't actually right!

Should we remove Literal::i32 and all other signed functions? If so should we also panic on negative floats? If not, should we refactor the AST to support literal tokens with negative integers?

cc @jseyfried, @dtolnay, @nrc

[alexcrichton]

I opened dtolnay/quote#65 as a workaround for the quote crate for now, but it also made me realize that removing functions like Literal::i32 isn't actually possible but we may still want to change them! Specifically i32::min_value() makes this pretty tricky. So today we have Literal::i32(arg: i32), but it turns out we've actually got no method to do represent i32::min_value() as a literal via the proc_macro API!

Given that information I think I'd propose the following change:

• Change Literal::iNN from taking iNN to taking uNN. For example Literal::i32 would take a u32 argument. This argument would not be validate to fit within the range of valid i32 integer values. It's stored internally as a stringified u128 effectively.
• Change Literal::{f32, f64} to panic if the input is negative (like today it panics if it's NaN or infinite).

I think with that change we should be able to put all integers into literals (with suffixes) and we'd also be able to be clear that the negative tokens don't go into the literal but rather need to get emitted as a separate token.

[nrc]

iirc, we do quite a few 'tricks' with tokens, that is the way we lex source code is not necessarily how the parser treats the tokens. The interpolated tokens from macros are an obvious example which can't be written, only produced from code. I think << is handled in some tricky way too, and negative numbers might be included in this category (that is, maybe they should be if they are not currently).

This is a long-way of saying that perhaps the TokenStream as seen by a proc macro does not need to match exactly what the lexer outputs and that the Rust parser should be able to cope with Tokens created by proc macros which are not able to be created elsewhere.

[dtolnay]

👍 for what I think Nick is saying which is Literal::i32(-1) should be a perfectly legitimate single token.

[ngg]

+1 Even if quote! can apply some workarounds, it would be easier for macros that want to manipulate token streams in some other ways.