Better panic location reporting for `unwrap()` and friends

[abonander]

One thing that makes debugging kind of a pain is that the panics in Result::unwrap() and Option::unwrap() (and *::expect()) report the file and line inside the unwrap() function when this is hardly ever relevant. The primary interest is almost always the call site of the unwrap() instead, but this requires having backtraces and debug info; backtraces from panics in optimized code are rarely helpful because of aggressive inlining. This can be remedied just hand-inlining unwrap() (or using a macro for convenience) but this is far from an optimal solution.

The idea itself is really basic: during inlining, replace the result of this block (and the one below it) as expanded in the panicking function with the values for the function’s call site.

I’m thinking it would add two attributes, one for stabilization and one internal to rustc. The first would be to annotate a function which wants to replace these values with those of its call-site, and then the second attribute annotates the aforementioned block so that the inlining pass knows that it should be replaced.

However, this would probably require some sort of inlining pass at the HIR level (when we still have access to this information) and sounds nontrivial. I’m not sure the slight gain to ergonomics compared to some high-level solution, like a version of unwrap() that takes this file/line tuple, is worth the work, but maybe I’m underestimating the frustration that comes from misleading backtraces in release mode.

[kennytm]

There was Line info for `unwrap`/`expect` but went nowhere

[kornel]

That would be fantastic, even if it was a private hack just for std. I’m really disappointed whenever I see a panic reported in std’s option.rs, and not in my code.

[abonander]

As a true hack, I’m conceptualizing a procedural macro that rewrites all unwrap() invocations to a method from an extension trait that takes the file and line. It has the potential to be rather slow (the impending improvements to TokenStream should help) and would require an invocation at the top of every file, but it wouldn’t require changes to the compiler and it would be mostly transparent to the user.

[sfackler]

IIRC @eddyb has talked about a similar concept of semantic inlining before.

[eeeeeta]

If we get macros in method position, we could just start writing unwrap!() and be done, no?

[abonander]

Yeah, but that’s a pretty big if. I’m not sure there’s a whole lot of desire for those, though Macros 2.0 is sort-of paving the way for them by making macros less special overall.

[Gankra]

See also https://github.com/rust-lang/rfcs/issues/1744

[whitequark]

On my embedded target there are no native backtraces unless I go out of my way to link libunwind (which is nontrivial due to PT_GNU_EH_FRAME etc) and no gdb. This would be fantastic to have.

[abonander]

I’ve got a working-ish prototype using attribute procedural macros to rewrite .unwrap() invocations to ones from an extension trait which take the call site’s file and line.

However, due to the current implementation of TokenStream, the expansions of any generated line!() invocations point to the attribute itself, so it’s basically useless. I believe https://github.com/rust-lang/rust/pull/40939 should improve this as it can reuse the original spans for the generated tokens. (Reusing the token stream will also be more efficient than the current stringly typed API for folding over whole files).

There’s also currently an ICE when invoking attribute procedural macros with a top-level attribute, which limits the utility somewhat: https://github.com/rust-lang/rust/issues/41211

Prototype published here: https://github.com/abonander/better_unwraps

[kennytm]

I’ve submitted RFC 2091 for this. Using issue 36479 as demo:

$ rustc 1.rs
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.21.0-nightly (aac223f4f 2017-07-30) running on x86_64-apple-darwin

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:335:20
note: Run with `RUST_BACKTRACE=1` for a backtrace.


$ ./build/x86_64-apple-darwin/stage2/bin/rustc 1.rs
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.21.0-dev (ec6655b91 2017-07-31) running on x86_64-apple-darwin

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/librustc_trans/mir/block.rs:433:46
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[abonander]

As an update to my approach described above, the API improvements in https://github.com/rust-lang/rust/pull/40939 got me painfully close to a working prototype but it’s not quite there.

This is because the implementation of the built-in line!() and file!() macros deliberately finds the span of the outermost macro invocation instead of giving the actual source of the invocation, which is, in general, more useful, but causes issues here because they always point to the #[rewrite_unwraps] attribute.

Essentially, I’d like to be able to extract the true file, starting line/column and ending line/column of a given Span, maybe something like this:

impl Span {

    /// Get the file into which this span points.
    fn file(&self) -> &str {...} // or an interned string type, could be `Term` but 
    // that's described as a valid identifier which paths usually are not

    /// Get the starting line and column of this span in the source file.
    fn start(&self) -> (u32, u32) {...}

    /// Get the ending line and column of this span in the source file.
    fn end(&self) -> (u32, u32) {...}

    /// If this span and `other` are adjacent [or overlapping?], return a new span representing the two of them combined.
    fn glue(&self, other: Span) -> Option<Span> {...} // This operation is already implemented in libsyntax
}

glue() would also be immensely useful to create a wider, but still valid, span of some non-delimited tokens (like the entire span of a function/method call, which includes two or more separate TokenTree elements). N.B. The libsyntax implementation doesn’t require spans to be adjacent, and I’m not sure whether or not we want it to be this powerful; ostensibly, the only spans you could have accessible are for the current invocation, but I think it would be possible to cheat this with a thread-local. Would this cause any problems?

It might also be useful to have something like this:

impl Span {
    /// If `self` and `others` are all adjacent [or overlapping?], return a new span encompassing them all.
    fn glue_many(&self, others: &[Span]) -> Option<Span> {...}
}

Though its implementation would be nontrivial (but also possible in client code… I think).

These APIs combined would suffice to make #[rewrite_unwraps] work as intended, as well as provide the building blocks for truly useful error reporting in procedural macros. This could be expanded on in the future, perhaps with a struct that can be used as an argument to panic! to pass a span as well as a message back to the compiler to be reported:

panic!(ProcMacroError { span: span, message: "Could not parse tokens as an expression" });

[abonander]

^ cc @jseyfried @nrc