Specialize Termination for Result<!, Box<dyn Error>> to report errors via the error trait

[yaahc]

Currently all errors reported via the Termination impl on core::result::Result go through Debug instead of Error, which tends to be the wrong choice for errors. We have plans for fixing this via specialization but these plans are blocked on workarounds for a soundness issue and wouldn't even apply to Box<dyn Error> because it doesn't even implement the Error trait. We cannot fix this as far as we can tell, but it turns out we probably don't need to for this specific case! Specialization already works for concrete types, so we could specialize the Termination impl specifically for Box<dyn Error> today, there's nothing blocking this. That way we can confidently point people towards fn main() -> Result<(), Box<dyn Error>> knowing that it will output the right thing.

[seanchen1991]

so we could specialize the Termination impl specifically for Box today

Any notes or thoughts on what this might look like?

[programmerjake]

don't forget all the similar types with auto traits: Box<dyn Error + Send>, Box<dyn Error + Sync>, ...

[yaahc]

so we could specialize the Termination impl specifically for Box today

Any notes or thoughts on what this might look like?

https://searchfox.org/rust/rev/703f2e1685a63c9718bcc3b09eb33a24334a7541/library/std/src/ffi/c_str.rs#374-400 might be a good place to start

[ghost]

This is really great to see. Will this new specialized Termination impl check err.backtrace() and backtrace.status() to display an error's backtrace info or will it be expected that each error is expected to include its own backtrace info inline in its Display output? I think the first option would be much nicer from a library author and user perspective but many userland errors and error crates put the backtrace info into the Display because of how things work right now so under the first option the backtraces would be doubled up in the output. But hopefully that would be a rather temporary state of affairs porting things over. Or do backtraces need to stabilize first? I can open a separate issue for this. It seems like if it's really difficult to change the default fn main() Error reporting, a cohesive end-to-end story about how to do errors using only std for libraries and applications would be very helpful to go along with a new Termination impl.

[yaahc]

I personally definitely don't want to encourage people to put backtraces in the output of their error messages via Display. To me backtrace information is not an error and has no business being in an error message, it's context about an error that is intended to be included in full error reports when appropriate. That said, how we format the report is going to be a whole bikeshed probably and I have no idea what we will decide on in the end. I'm on the fence personally between wanting to default to a single line output like Error: outer error's message: root error's message and multi line output like anyhow::Error. If we go with a single line output I imagine it will be hard to sneak in the backtrace too, if we go with multi line output then I think we should probably include the backtrace, but we need to be careful because we don't want to go dumping backtraces by default for applications whose output is intended for users who aren't software devs.

[ghost]

Ok great, this is also what I think regarding backtraces in the Display string. I would think that the RUST_BACKTRACE env var would decide when to show backtraces or not? Right now you don't get any backtrace info when this env var isn't set to 1 or full, the Display impl only shows the message "disabled backtrace" so this is a natural spot to decide between a concise format and a longer format with a backtrace attached.

[yaahc]

Ok great, this is also what I think regarding backtraces in the Display string. I would think that the RUST_BACKTRACE env var would decide when to show backtraces or not? Right now you don't get any backtrace info when this env var isn't set to 1 or full, the Display impl only shows the message "disabled backtrace" so this is a natural spot to decide between a concise format and a longer format with a backtrace attached.

Possibly, but you can already override that environment variable by using Backtrace::force_capture(), so we'd have to decide what to do in the case where an error contains a force captured backtrace while the environment variable says backtraces are disabled, do we still display it or do we respect the environment variable? Also I'm not convinced it makes sense to associate both of these behaviors with the same env variable, it may be that we want to control them separately, or even expose a way to customize the logic, either way I expect an RFC is needed.

[thomaseizinger]

I would think that the RUST_BACKTRACE env var would decide when to show backtraces or not?

I can very much imagine a situation where the developer of a CLI wants to control this instead of the enduser having to fiddle with env vars.

[Kixunil]

Instead relying on specialization we could have:

struct std::error::Report(Box<dyn Error>);

impl<T: Error> From<T> for Report {
    fn from(error: T) -> Self {
        Report(Box::new(error))
    }
}

impl fmt::Debug for Report {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "Error: {}", &*self.0)?;
        let mut source = self.0.source();
        while let Some(source) = source {
            write!(f, ": {}", source)?;
            source = source.source();
        }
        Ok(())
    }
}

And point people at fn main() -> Result<(), std::error::Report>