Change default output of panic!

[jooert]

This splits the output of panics into two lines as proposed in #15239 and adds a
note about how to get a backtrace. Because the default panic message consists of
multiple lines now, this changes the test runner's failure output to not indent
the first line anymore.

Fixes #15239 and fixes #11704.

[rust-highfive]

r? @alexcrichton

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

[alexcrichton]

I'm a little hesitant to change the format here, there's a few downsides to changing this format I think:

• By splitting the output on multiple lines, this means that concurrent panic messages will be interleaved and difficult to read by default
• This is a pretty core part of the standard library, so it's likely someone's relying on the output format
• Panic messages should never be a user visible error, so it's unclear how much effort we want to put forward into making them look nice
• It's not clear that this sort of information is immediately actionable to those receiving the error. As a consumer of a Rust program, seeing a panic message indicates a bug in the source, at which point the extra spacing and info may or may not help much.

I think that I would personally prefer to leave the message as-is, but I'm curious what others think.

cc @rust-lang/libs

[aturon]

@alexcrichton Let's start with the underlying concern this PR is trying to address: to make panic messages more helpful by e.g. pointing to details about debugging or backtraces. I think this is quite legitimate: many newcomers to Rust will encounter panics first through unwrap, and be quite frustrated by a pointer into the source code of libcore, without knowing what to do next.

In other words, I disagree with:

Panic messages should never be a user visible error, so it's unclear how much effort we want to put forward into making them look nice

I think the perspective here is really about developers, especially newbies, though it's worth thinking about the end-user experience as well. However, custom handlers will likely help the end-user story for robust apps. (This also addresses your final point.)

By splitting the output on multiple lines, this means that concurrent panic messages will be interleaved and difficult to read by default

Fair point -- let's see if we can find a way to solve the problem while keeping to just one line.

This is a pretty core part of the standard library, so it's likely someone's relying on the output format

Indeed. And this isn't something we can easily assess via crater, so we'd need to advertise this widely in some way.

Could we shrink this change by just adding a short (see <SOME_URL> for help) where the link explains panics, backtraces, and how to break on panic in a debugger?

[alexcrichton]

Yes I agree with the thrust here, but I also think there's always a price to pay when adding all these "easy to debug" features. An example of this is how we have backtrace support via RUST_BACKTRACE, but this is also easily obtained through debuggers like gdb/lldb. We receive complaints from time to time about the portability of the standard library or the size of binaries which pulling libbacktrace along doesn't help with.

What I'm getting at is that features we add to help newbies need to be weighed against their technical debt and maintenance cost. I don't think it's the case that "this is more user friendly" is an automatic "oh sure, let's land it!"

For example, even if we were to include some sort of URL in the text that's now a link we need to maintain for a very long time to make sure it always works. Not only that, but that text will be embedded in all future rust executables (which may or may not be desired). My own personal feeling as well is that after you see the link once it's basically always extra text you need to scan over.

One possible case study for leaving as-is is the assert macro in C, which has a similar message as our panic messages. It's certainly opaque if you run into it accidentally, but I don't think there's much thrust to change it to have fancy formatting or anything.

[brson]

I do think the user experience matters here, and the current isn't great. Of @alexcrichton's concerns, the interleaving of panics bothers me most, though with the line number and task number remaining on the same line, at least you can figure out where in the source code the panic was, even if you can't figure out what the message was.

Regarding the backtrace suggestion, I'm not sure that 'note: ' is the proper UI here - that's the compiler's UI, and there's no similar precedent in the standard library. I do tend to think that the first time a newbie sees a panic is the best time to inform them about what to do about it, but am sympathetic that this adds a lot of noise - a multithreaded program that panics a bunch is going to be too helpful with all the backtrace suggestions.

[aturon]

@alexcrichton

What I'm getting at is that features we add to help newbies need to be weighed against their technical debt and maintenance cost. I don't think it's the case that "this is more user friendly" is an automatic "oh sure, let's land it!"

Of course, but (1) discoverability of backtraces is hugely important for the newbie experience, given the likelihood of unwraps and (2) there's no reason that making this better entails a big maintenance burden.

I agree that a URL has downsides, but perhaps we can instead use something like the diagnostic codes you can pass to the compiler to get more information. I'm just trying to get a very short thing we can add that acts as a pointer to more information. That helps with the multiline concern as well as the noisy output concern. Surely there's a way to do that with ~zero maintenance burden?

[ranma42]

Would it be sufficient to provide the additional information just once, when the main thread of the application panics (i.e. basically here

rust/src/libstd/rt.rs

Line 63 in 176ee34

let res = panic::recover(mem::transmute::<_, fn()>(main));

)?

This would avoid troubles with interleaving of concurrent panic messages and allow for multiple lines of in-line explanation, which might avoid the burden of maintaining coherence elsewhere.
For example, if in the future we decided to deprecate RUST_BACKTRACE and instead to use the RUST_DEBUG env var to say that applications should drop into a debugger upon panic (maybe to remove the burden of libbacktrace from Rust), it would be possible to do that and each libstd would contain instructions compatible with its version.

[alexcrichton]

@aturon yeah something like diagnostic codes sounds plausible, and @ranma42's idea of only printing this out for the main thread also seems plausible (or perhaps just printing it out on the first panic, not all of them).

[alexcrichton]

We discussed this in the libs triage yesterday and the conclusions we reached were:

• The actual output of each panic message shouldn't be changed for now, we can perhaps consider reformatting and reorganizing it but that should happen in a later PR.
• The "helpful message" about RUST_BACKTRACE=1 should only happen once (probably guarded via an AtomicBool) on the first panic. The message would also be ideally buffered with the rest of the panic message before emission (but this may not always be possible).

@jooert would you be ok implementing these changes?

[jooert]

@alexcrichton I tried to implement this as specified and added a test for it. Should I update the examples' outputs in the book as well?

[alexcrichton]

This can actually just be a plain old static rather than a static mut (and all access will be safe!)

[jooert]

Oh, right! I would have thought that compare_and_swap takes a mutable reference...

[alexcrichton]

Nah I think that's fine to leave off until later if necessary (but cc @steveklabnik if you have an opinion on it)

[jooert]

Updated. Anything else that could be improved?

[ranma42]

Maybe the one-time message could also mention rust_panic. See #11704

[jooert]

Hm, the bug report guide was changed to only mention RUST_BACKTRACE instead of rust_panic because it doesn't require to start gdb (see #19717 and #19738), so that seems to be the preferred way.

[ranma42]

I agree that RUST_BACKTRACE is more convenient to report bugs against Rust programs (in the case you mentioned, rustc) as it does not require going through the debugger, but in order to actually debug a panic, a breakpoint is more convenient. In any case, your PR is definitely a step in the right direction (it provides an easy way to add this kind of information if wanted). The question about rust_panic should probably move to #11704

[alexcrichton]

@bors: r+ c07413c

Thanks @jooert!

@ranma42 yeah this is a good start and we don't want to inundate with information because this message will likely come up quite a bit, but we can perhaps wedge it in in the future.

[bors]

⌛ Testing commit c07413c with merge 13b5eda...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt