Tracking issue for error messages from proc-macro implementations

[federicomenaquintero]

Currently, presenting good error messages from a proc-macro implementation — the user calls the macro and has a syntax or semantic error in the contents of the macro invocation — is very tricky. I.e.

some_proc_macro_with_its_own_syntax! {
    ... user's code here with invalid syntax/semantics ...
}

There are some things that make it hard to present good error messages. Assuming that one uses syn for the parsing stage:

• The syn code flow bubbles PResult upstream, but syn's parse_error() does not include an error message nor a span - it's just an empty struct.
• To actually present a nice error message, one cannot bubble up a Result to rustc; one must use something like proc_macro::Diagnostic:

Diagnostic::spanned(
    problematic_span_here,
    Level::Error,
    "your code has an error blah blah"
).emit();

• Also, one must emit something in the resulting TokenStream; maybe (quote! {()}).to_tokens(tokens);, or the compiler will think that the proc-macro emitted nothing.
• Documentation-wise, it would be nice to say how to use Diagnostic to emulate the various suggestions and annotations that rustc presesnts with its normal messages.
• (And how to use the compiletest_rs crate or whatever to test those messages...)
• Upon encountering a parse error at the toplevel, Relm consumes all the remaining tokens in the TokenStream so that rustc won't present the user with an extra, meaningless error after the first one. We should either document that trick, or provide a helper to do it, or some other way of avoiding errors past the first "real" one.

After the parsing stage, the proc-macro may encounter semantic errors. In that case, the implementation needs to return a TokenStream with spans with errors. Again, the disparity in carrying errors as a Result, versus emitting spans with errors, makes this tricky. It's also not clear whether it's up to the implementor to aggregate errors in a Vec or something, and then later emit a bunch of error spans for them somehow (instead of just stopping after the first one, for example).

[alexcrichton]

cc @SergioBenitez

[federicomenaquintero]

Note to self: the master tracking issue for proc-macros is #38356.

[alercah]

While experimenting with this, I discovered another major usability issue: testing. There's no way to write unit tests against Diagnostic::emit() as is, because of the close integration with the compiler.

I think that moving error emission to the return value would be ideal. I think I would design the API as follows:

1. Create a MacroResult type which can contain an optional TokenStream and a Vec<Diagnostic>.
2. Implement Try<Ok = TokenStream, Error = Vec<Diagnostic>>, so that ? can work easily, and PError can simply provide an appropriate conversion function.
3. Make it possible to inspect Diagnostic, for testing.
4. For advanced uses, let users specify the MacroResult directly, and in particular to return both a TokenStream and a Vec<Diagnostic>.
5. If a macro returns no TokenStream, but also errors, then this is an error.

[SergioBenitez]

Sorry, I just came across this issue. I think this is largely a documentation issue. Let me try to address your points one by one:

The syn code flow bubbles PResult upstream, but syn's parse_error() does not include an error message nor a span - it's just an empty struct.

Indeed. In Rocket, we have a Parser that integrates with Diagnostic: https://github.com/SergioBenitez/Rocket/blob/master/core/codegen_next/src/parser.rs. Feel free to use this code. I expect that syn or something similar will ship with something like this once proc_macro diagnostics stabilize.

To actually present a nice error message, one cannot bubble up a Result to rustc; one must use something like proc_macro::Diagnostic. [...] Also, one must emit something in the resulting TokenStream; maybe (quote! {()}).to_tokens(tokens);, or the compiler will think that the proc-macro emitted nothing.

This is true, but you don't need to emit() it directly. In Rocket, for instance, we follow this pattern:

type PResult<T> = ::std::result::Result<T, Diagnostic>;

fn real_derive(input: TokenStream) -> PResult<TokenStream> {
    // return early if an error occurs
    return Err(span.error("foo"));
}

#[proc_macro_derive(Trait)]
pub fn derive(input: TokenStream) -> TokenStream {
    real_derive(input).unwrap_or_else(|diag| {
        diag.emit();
        TokenStream::new()
    })
}

There's nothing wrong with emitting an empty TokenStream.

One thought that @alexcrichton and I have kicked around is to allow the following signature for derives directly:

#[proc_macro_derive(Trait)]
pub fn derive(input: TokenStream) -> Result<TokenStream, Diagnostic> { }

But this is just sugar for what I wrote before.

The primary concern is that this might incentivize users to bail on the first error as opposed to trying to make as much progress as possible to emit as many errors as possible at once. It could be that this doesn't actually matter in practice, however.

Documentation-wise, it would be nice to say how to use Diagnostic to emulate the various suggestions and annotations that rustc presesnts with its normal messages.

Indeed. But all of this is very unstable without RFC or clear stabilization paths. This is something I will look into personally somewhat soon.

Make it possible to inspect Diagnostic, for testing.

It should be inspectable, but I don't think this will be of much value for testing. You should really use compiletest-rs with UI tests instead. If you want an example, take a look at Rocket's use: https://github.com/SergioBenitez/Rocket/tree/master/core/codegen/tests/ui.

If a macro returns no TokenStream, but also errors, then this is an error.

This is akin to returning an empty TokenStream today.

[dtolnay]

The syn side of this has been addressed as of 0.15.0: https://github.com/dtolnay/syn/releases/tag/0.15.0

[federicomenaquintero]

A kind soul recently updated gnome-class to use syn 0.15.3, and I am incredibly, nicely impressed. The new parser code is much more legible! I tried adding a few deliberate syntax errors inside macro invocation, and the results are pretty spectacular. Translating a chain of lookahead.peek() into

error: expected one of: `class`, `impl`, `interface`
  --> tests/override.rs:10:5
   |
10 |     plonk class Foo {}
   |     ^^^^^

I'm so happy to see this work! Excellent, excellent job!

👍 👍 👍 👍 👍 👍 💯

[alexcrichton]

I think with syn's recent update I'm gonna close this in favor of #54140 which tracks a real diagnostic API being added to proc_macro