Don't include sources in their errors' display()

[ldesgoui]

When trying to use source() in a chain, each display() currently includes the succeeding errors, causing a lot of duplication.

When one wants to easily display an error chain intercalated by ": ", they should format using the alternate format: format!("{:#}", err)

Also fixed extraneous quoting around paths in errors

[nightkr]

👍 on adding #[source] annotations.

I'm not quite so sure about removing the source from the Display impls.. I don't see anything about {:#} for getting the error chain in the docs for std::error::Error, std::fmt, or std::fmt::Display. Is that a thiserrorism?

[nightkr]

Not seeing anything about the alternate display mode in thiserror's docs either, nor does it seem to work in practice (https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f8fa20aca31d5a4ab5dd725c62a92f54).

[ldesgoui]

Oh, you're totally right, the {:#} handling is a anyhow::Error feature, my bad.

[ldesgoui]

My point still stands w.r.t. display(), it's impossible to display each error in the chain without the remainder at each step, it's how color_eyre displays errors, so I assume that's how display() is meant to be implemented, I might be wrong.

[kazk]

Including the source in Display impl or not is still under debate (leaning towards not), but we shouldn't do both (Display and #[source]).

The short answer is that both design decisions (as in, whether to print the source error on the Display impl) have upsides and downsides; some error handling libraries impose an opinion on this (e.g. anyhow/eyre) whereas others currently do not (SNAFU); and although things appear to be turning towards letting the job of printing the source error to the reporter, this is still not a well established consensus, to the point that this has not been accepted to become part of the API guidelines (rust-lang/api-guidelines#210), mostly because it would still break some expectations.

rust-lang/project-error-handling#23 (comment)

To add some more to that, we're currently leaning towards the reporter based design, but it's too early to start pushing for it across the ecosystem because there are quite a few edge cases still where context for errors can be discarded because no reporter is ever used.
...
For now, the recommendation is to always do one or the other. If you're going to print your sources, you should not return them via the source function and vice versa.

rust-lang/project-error-handling#23 (comment)

For thiserror usage, dtolnay wrote:

Using thiserror's attribute syntax for convenience, the contents of the error type should appear either in the message:

#[derive(Error)]
enum Error {
    #[error("Failed to read settings: {0}")]
    ReadSettings(io::Error),
}

or as a source:

#[derive(Error)]
pub enum Error {
    #[error("Failed to read settings")]
    ReadSettings(#[source] io::Error),
}

but never both.

https://users.rust-lang.org/t/do-people-not-care-about-printable-error-chains-a-k-a-how-to-nicely-implement-display-for-an-error/35362

[nightkr]

Now that we're getting more into opinions, my vote would go towards putting sources into both Display and source. Why?

1. Not implementing source means that we hide semantically useful information (such as source downcasting)
2. I'm not really aware of a good error formatting library that I'd be comfortable recommending (that doesn't also try to be its own error handling universe, like color-eyre does)
3. If you're doing custom trace formatting then it's not much extra work to strip_suffix the source (see https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f3a8475d9934fa520fc8ecd4181e18e8, for example)
4. Even if you don't strip the duplicates, duplicate messages is still a better situation to be in than missing messages

[kazk]

Found rust-lang/project-error-handling#27 which is more relevant to us a library.

And more up to date comment:

We came to the consensus that the primary role of the error trait is providing a structured access to error context for the purpose of reporting, so our guidance is officially decided.

• Return source errors via Error::source unless the source error's message is included in your own error message in Display.

rust-lang/project-error-handling#27 (comment)

[kazk]

We should follow the official guidelines, but we don't have the necessary tools yet (they're planning to add Report in std). I think the "fix" to remove source from Display can wait until when the migration path is established.

I'm more concerned about not having focused errors (lightly discussed in #453, #453 (reply in thread)).

[LukeMathWalker]

I bumped into this very same issue recently - specifically when trying to publish Kubernetes' events in a controller.
I'd like a short to-the-point description of what went wrong for the operator using kubectl describe, while the more verbose cause chain should be displayed in the controller logs.
By including the source in both source and Display, it becomes impossible to achieve.

We should follow the official guidelines, but we don't have the necessary tools yet (they're planning to add Report in std). I think the "fix" to remove source from Display can wait until when the migration path is established.

By only returning the underlying error in source, you can still print the entire error chain without any special support by std or having to pull in external libraries:

fn error_chain_fmt(e: &impl std::error::Error) -> String {
    let mut buffer = String::new();
    writeln!(buffer, "{}\n", e).unwrap();
    let mut current = e.source();
    while let Some(cause) = current {
        writeln!(buffer, "Caused by:\n\t{}", cause).unwrap();
        current = cause.source();
    }
    buffer
}

Adding Report to std will likely make it more ergonomic, but it's possible today.

[nightkr]

@LukeMathWalker

By including the source in both source and Display, it becomes impossible to achieve.

It's not the prettiest, but you could do something along the lines of:

let mut msg = err.to_string();
if let Some(source) = err.source() {
    msg = msg.trim_end_matches(format!(": {}", source)).to_string();
}

By only returning the underlying error in source, you can still print the entire error chain without any special support by std or having to pull in external libraries:

You can, but the question is what people actually do. FWIW, tracing's console formatter only fixed this less than two weeks ago (and the JSON formatter still doesn't, over backwards compat concerns).

[LukeMathWalker]

It's not the prettiest, but you could do something along the lines of:

let mut msg = err.to_string();
if let Some(source) = err.source() {
    msg = msg.trim_end_matches(format!(": {}", source)).to_string();
}

That looks very brittle :/
I also doubt that kube-rs considers (or wants to consider!) the structure of the Display representation as part of its API interface guarantees, semver-wise.

You can, but the question is what people actually do. FWIW, tracing's console formatter only fixed this less than two weeks ago (and the JSON formatter still doesn't, over backwards compat concerns).

I am not arguing with the broader underlying assumption - the Rust community is still, collectively, iterating on what are considered the best patterns for error handling. Tools in the space have some catch-up to do.
But there is clear guidance from the relevant working group around the usage of source and its interplay with Display, so it's safe to assume that the ecosystem will converge to it.

[nightkr]

That looks very brittle :/
I also doubt that kube-rs considers (or wants to consider!) the structure of the Display representation as part of its API interface guarantees, semver-wise.

Absolutely, please don't try to parse the Display output!

That said, in this particular case, the only way that I can imagine we'd change this would be to remove the source entirely from the Display (once the error-rendering ecosystem has caught up with the error WG's guidance), which that example would cope with fine (trim_end_matches becomes a no-op if there are no matches). Plus, the worst-case scenario (changing {}: {} to something like {}, caused by {}) would still fail mostly graciously (the full source error would be included again, but it shouldn't cause anything to fail).

[kazk]

We'll do this as part of refining errors (#688).