Add error::Report type

[seanchen1991]

This implements std::error::Report, a wrapper around an error that exposes the entire error chain, along with options to format the output of the report as either a single line or as multiple lines.

Tracking issue: #90172.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

I don't think we have any 'builder pattern' functions in std right now that take and return self by value. Instead, they take &mut self and return &mut Self. That way, you can also use them without chaining them, as they modify the original object, but also allow chaining.

[yaahc]

it's definitely preferable to have this one done by value, because otherwise you can end up with common usages breaking. Our original version used &mut Self but broke in the following trivial example:

let report = Report::new(error).pretty(); // error: reference to temporary
println!("{}", report); 

Report is designed so that you're always going to create it and then use it immediately, so being able to mutate a pre-defined report is never going to be useful.

[m-ou-se]

Report is designed so that you're always going to create it and then use it immediately, so being able to mutate a pre-defined report is never going to be useful.

Then why is it a problem that your example breaks, if you're not supposed to store it for later use?

Your example makes me think that it would be useful to be able to do things like:

let mut report = Report::new(error);

if opts.verbose_errors {
    report.pretty(true);
}

println!("{}", report); 

[yaahc]

I'm not sure I agree, I'd write that like this:

let report = Report::new(error).pretty(opts.verbose_errors);
println!("{}", report);

Then why is it a problem that your example breaks, if you're not supposed to store it for later use?

The issue to me is that I don't want to be forced to add a bunch of extra lines or inline the entire thing in a println in order to have it compile, if it it was returned by reference my example could only be written like this:

let mut report = Report::new(error);
report.pretty(opts.verbose_errors);
println!("{}", report);

or

println!("{}", report.pretty(opts.verbose_errors));

In my experience using Report so far I have found that it tends to be more annoying to use when it returned &mut Self and when I inlined the method calls they often ended up having to be spread over multiple lines and made the code look much harder to read for me.

[seanchen1991]

At the very least, it seems like show_backtrace and pretty should be reverted to accept a boolean (we had this in a previous iteration, but ended up removing it).

[m-ou-se]

(This is why in the other cases the builder type is different type than the type you end up building. Then the builder type doesn't need to contain any references or resources yet, such that (&mut self) -> &mut Self can be used, and only an .open(), .spawn(..), or w/e at the end will make the object you wanted to make, which might end up borrowing things into the newly created object.

But having two types in this case might get a bit annoying.)

[yaahc]

Yea, I think its particularly important that fallible_fn().map_err(Report::new).unwrap() be usable, so forcing a .build() call at the end feels a little awkward to me.

[m-ou-se]

Can you add some tests?

There's a doc example, but it doesn't check the output.

(And it'd be good to test what happens with multiple sources, multi-line errors, etc., as those are broken now.)

[m-ou-se]

Looks like there's a few more bugs:

Empty lines are dropped in sources:

line 1

line 3

Caused by:
   0: line 1
      line 3
   1: line 1
      line 3

And with a single source, multi-line indentation breaks:

line 1
line 2

Caused by:
    line 1
line 2

[yaahc]

Almost done resolving the recent round of comments but I have a couple of style questions that came up during the implementation that I could use some input on.

First, I want to make sure we have the right level of indentation setup for our multiline errors so they match up nicely with the output of Backtrace, and I'm wondering what you think would be best @m-ou-se.

Here's the current version with the changes I made and one source error:

Error: The source of the error

Caused by:
    Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
  11: clone

And with 2 or more sources

Error: Error with two sources

Caused by:
    0: The source of the error
    1: Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly_with_two_sources
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8

This is identical to the formatting rules for anyhow, as far as I can tell. Right now it seems to want to line up the error messages so that each message starts on the same column, but my instinct is to line it up so that the causes start on the same column as the symbols in the backtrace. The case where the errors aren't numbered already doesn't line up with the outermost source error message, so either way I'd like it to be changed for consistency, whether or not that ends up being based on the first error message or the backtrace.

My preferred version would look like this

Error: Error with two sources

Caused by:
   0: The source of the error
   1: Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly_with_two_sources
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8

and

Error: The source of the error

Caused by:
      Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
  11: clone

[bors]

☔ The latest upstream changes (presumably #92844) made this pull request unmergeable. Please resolve the merge conflicts.

[yaahc]

Merged as #91938, closing this PR now