Regression in error message when a custom derive panics

[dtolnay]

Between nightly-2018-01-26 and nightly-2018-01-27 the error message for a custom derive panic got worse. This is the range 9fd7da9...bacb5c5. Mentioning @Zoxc because #47634 sounds relevant.

src/main.rs

#[macro_use]
extern crate repro;

#[derive(Panic)]
struct S;

fn main() {}

src/lib.rs

extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Panic)]
pub fn f(_input: TokenStream) -> TokenStream {
    panic!("no can do");
}

Before:

error: proc-macro derive panicked
 --> src/main.rs:4:10
  |
4 | #[derive(Panic)]
  |          ^^^^^
  |
  = help: message: no can do

After:

thread 'rustc' panicked at 'no can do', src/lib.rs:57:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: proc-macro derive panicked
 --> src/main.rs:4:10
  |
4 | #[derive(Panic)]
  |          ^^^^^
  |
  = help: message: no can do

[Zoxc]

It is indeed caused by #47634 where rustc no longer swallows stderr. This does seem desirable to me however and helps track down the bug in the plugin. How would you get panic backtraces on stable?

[dtolnay]

It is not a bug in the plugin -- panicking is the intended way for custom derives to report errors to the user.

#[macro_use]
extern crate serde_derive;

#[derive(Serialize)]
struct S {
    #[serde(broken)]
    f: u8,
}

  |
4 | #[derive(Serialize)]
  |          ^^^^^^^^^
  |
  = help: message: unknown serde field attribute `broken`

Showing the line number within the custom derive at which the error was detected is unnecessary and unhelpful.

[Zoxc]

I see that the RFC uses panics as a temporary way to report errors. We do still want to report the backtrace if desired though.

cc @rust-lang/compiler

[nikomatsakis]

panicking is the intended way for custom derives to report errors to the user.

Longer term, this does seem like a suboptimal way to report errors, since it doesn't allow for more than one error at a time very gracefully, right? Still, if that's the expected way, we have to support it.

We do still want to report the backtrace if desired though.

At the limit, we can do a -Z option... but in any case can't we catch the panic or something to prevent it from being printed to stderr?

cc @rust-lang/libs -- how configurable is the "print to stderr on panic" code here?

[Zoxc]

We can install a panic handler in rustc which does nothing when we are in compiler plugins.

[nikomatsakis]

@Zoxc

We can install a panic handler in rustc which does nothing when we are in compiler plugins.

Seems like a decent idea. Would this also suppress RUST_BACKTRACE?

[Zoxc]

Would this also suppress RUST_BACKTRACE?

It would suppress the note: Run with `RUST_BACKTRACE=1` for a backtrace. line, if that is what you meant.

[alexcrichton]

@nikomatsakis yes set_hook can be used to customize what happens when a panic happens.

[nikomatsakis]

@Zoxc I meant, if one does set RUST_BACKTRACE=1, would you still get a backtrace from inside the plugin. I thought that was what you wanted to enable?

[Zoxc]

@nikomatsakis We could do that, although it is likely users will have RUST_BACKTRACE set when invoking the compiler because they want backtraces of the executables it builds and aren't interested in a backtrace for a compiler plugin error.

[nikomatsakis]

@Zoxc I suppose that could happen with cargo run -- but I am a bit confused. Didn't you write this (emphasis mine):

It is indeed caused by #47634 where rustc no longer swallows stderr. This does seem desirable to me however and helps track down the bug in the plugin. How would you get panic backtraces on stable?

Isn't this arguing that we should show some sort of backtrace from inside the plugin?

[Zoxc]

@nikomatsakis Indeed. I suggest we create a PROC_MACRO_BACKTRACE environment variable and add a note to the error: proc-macro derive panicked error which suggests setting this if you want a backtrace for the compiler plugin.

[nikomatsakis]

Huh. I propose we first fix the regression, then worry about PROC_MACRO_BACKTRACE. I feel like there is a similar conflict in env vars around RUST_LOG, and we've never gotten around to doing anything about it.

[nikomatsakis]

I'd sort of rather we debate the whole "reuse RUST_ env vars" separately.

[nikomatsakis]

triage: P-high

It's important that proc-macro derive output not regress in the normal case. The rest can be debated.

@oli-obk may take a shot at this over next few days, but I'm sure they won't object if somebody else does!

I'd be happy with any of the above:

• Reverting the PR that caused the problem until we can come back with a revised approach.
• Capture the panic and not printing anything, then printing to use RUST_BACKTRACE in the log if you want the backtrace.

I'm a bit reluctant about introducing our own env vars, would want to at least get wider agreement on that, and decide what name to use carefully.

[Zoxc]

@nrc Do any tools which depend on rustc also use a custom panic handler?

[Zoxc]

I've opened a PR to fix this.

Interestingly we have a test for the previous behavior already, ui-fulldeps\proc-macro\load-panic.rs, which did not fail...

[nrc]

panicking is only meant to be a temporary way to report errors for proc macros. There should be a better way in the long run, but we might also need to panic as a 'fatal error' mechanism. It is also the stabilised way to error out of a custom derive.

I'm not aware of tools which use a custom panic handler, but it is certainly something they might want to do. I'd be very wary of letting any kind of panic reach the compiler driver API points or forcing a backtrace (or any error info) to be printed out on a panic which a tool can't catch.

[nikomatsakis]

Huh. So I went digging into why the test did not fail and discovered that I just can't reproduce this problem at all...

@dtolnay, I tried your reproduction, except that I had to add #![crate_type = "proc-macro"] into the library.

Are there some funny options or something that would be needed? Basically, I only see this output:

lunch-box. rustc issue-47812.rs -L .
error: proc-macro derive panicked
 --> issue-47812.rs:4:10
  |
4 | #[derive(Panic)]
  |          ^^^^^
  |
  = help: message: no can do

[nikomatsakis]

I am testing commit 56733bc

[dtolnay]

This script should reproduce the error message I showed.

#!/bin/bash

rustup update nightly-2018-02-08

cargo new repro --bin

echo >>repro/Cargo.toml '
[lib]
proc-macro = true
'

echo >repro/src/lib.rs '
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Panic)]
pub fn f(_input: TokenStream) -> TokenStream {
    panic!("no can do");
}
'

echo >repro/src/main.rs '
#[macro_use]
extern crate repro;

#[derive(Panic)]
struct S;

fn main() {}
'

cargo +nightly-2018-02-08 build --manifest-path repro/Cargo.toml

[nikomatsakis]

So -- strangely -- I see the message with nightly, but not with my local rustc. Perhaps it's one of the options I have enabled?