Add section about not including sources in Display impl

[faern]

This PR tries to codify what's being brought up in this forum thread:
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

These guidelines are still lacking a lot of information about how to design error types well overall. How to make composite errors vs leaf errors etc. This includes when and how to implement Error::source. But a good start is at least to try to stop people from making their errors contain a lot of redundant information.

[faern]

We can word it differently and maybe skip the example. But I feel it would be good if the guidelines touched on this topic in general. If we agree it's a good guideline that is. Because I see people regularly break this pattern in the wild, and it makes errors look bad when printed.

[wookietreiber]

i.e. if you want context, use any of the error-chain libraries, but keep error messages clean? would you still recommend to add some context, e.g. if opening a file failed, include the path?

[faern]

By "context" to you mean to have the error return the source error from its "Error::source" implementation? I think errors should return the error that caused them where relevant, yes. But I don't think the official guidelines should recommend any third party library for doing so. It's perfectly fine to store the source error and return it from Error::source manually. The only thing the third party crates help with is to reduce the boilerplate needed to create types that adhere to the best practices anyway.

Yes, an error failing to open a path can indeed include the path in the Display impl if it wants to. That is orthogonal and unrelated to including the source error in the Display impl. My main point is that an error should describe itself well. It should not describe its source or any other error in the Display impl. See this example:

pub struct OpenSettingsError {
    path: PathBuf,
    source: io::Error,
}

impl fmt::Display for OpenSettingsError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "failed to open settings at {}", self.path.display()),
    }
}

impl Error for OpenSettingsError {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        Some(&self.source)
    }
}

I think one of the confusing parts here is how some third party error libraries use the word "context". My understanding is that most of them when "attaching context" to an error means they wrap the original error as a source error of whatever "context" you provide? As an example, something like an_error.context("Failed to garble") returns a new error that prints as "Failed to garble" and then returns an_error if you call .source() on it? If so, I find that very confusing and strange.

I would prefer to not talk about third party helper crates in the guidelines. The guidelines are about how the public API of a crate should look and behave. Helper crates are just boilerplate reducers for reaching said look and feel. If the guidelines say "just use error-chain" then suddenly whatever error-chain does becomes the guideline, and that's not good. And it does not provide any community consensus as to which direction error-chain itself should go, it just says that whatever it does right now is the best thing to do.

[BurntSushi]

I'm not sure I agree with promoting this to an API guideline at this point in time. Certainly, if we had started with the current definition of the std::error::Error trait, then I would agree with it. The problem is that the Error trait has only recently evolved to a point where this sort of API guideline is actually reasonable. Which means a lot of crates aren't actually doing this correctly.

I'm not quite sure how to handle this. If an implementation is including source errors in its Display impl, then "fixing" that would appear to be a breaking change no matter how you slice it. Namely:

1. Removing source errors from the Display impl means that all existing uses of println!("{}", some_error) will result in much worse error messages.
2. Removing source errors from the source method that are used in Display is a breaking change in the sense that the underlying errors can no longer be accessed.

I think that the advice in this PR is probably correct moving forward, so perhaps the wording could be changed to acknowledge the current situation with appropriate context.

This is still quite gnarly though, because if you're doing println!("{}", some_error) and you're not using some error helper crate, then your error messages might end up getting clipped from some crates but not others.

[faern]

The problem is that the Error trait has only recently evolved to a point where this sort of API guideline is actually reasonable.

I don't think I understand. Error::cause has been around a long time and it has been used by error-chain and manually since basically forever to print errors together with their causes/sources. The only thing that recently changed is the name of the method to get the cause/source and the fact that it now supports downcasting.

If an implementation is including source errors in its Display impl, then "fixing" that would appear to be a breaking change no matter how you slice it.

Depending on the definition of breaking. In my book it's not. The API does not change. Only runtime results. But yes, I agree with your two points about how existing programs could potentially get worse error messages if they start using libraries designed for the current Error trait without updating their error handling code. Regarding point 2 about removing sources breaks programs: Yeah, that's why it sucks that people rely on opaque errors and downcasting them. You can't tell at compile time if you do your error handling correctly or not.

I think that the advice in this PR is probably correct moving forward, so perhaps the wording could be changed to acknowledge the current situation with appropriate context.

We can indeed word it in some way so people understand the difference between new and old Error. I think the guideline should encourage users to write errors according to the latest best practice. And it could in the same place tell them how to best use said errors in applications.

I'm not sure I agree with promoting this to an API guideline at this point in time. Certainly, if we had started with the current definition of the std::error::Error trait, then I would agree with it.

In my opinion the guidelines have to reflect the best practices of the latest version of Rust. I strongly believe this. If we don't encourage new users to the language to use the new stuff, why do we even invent new better stuff? To say "not at this point in time" if the preferred point in time already passed is a strange argument for me. Because it by definition means it should never be done. And then we'll sit with bad errors five years from now as well. If we encourage people to write software according to the latest best practices all the time, then the amount of "old" software will be a shrinking percentage of all Rust software. But if we encourage people to support the old standard then the amount of "old" software will continue grow forever. And the more it grows the argument for supporting it even further grows. Which is why I never opt for backwards compatibility.

If people have existing code bases they also already have experience in Rust and they probably have already established best practices/guidelines for their own code base. For me the guidelines have always been "This is the best possible thing to do if you are on a clean slate and/or are not limited by other stuff". If a new library is published on crates.io I expect it to have the best possible API for the latest Rust at that point in time. And if I write an application and I pull in a new dependency it's my responsibility to check how I should handle the errors from said library.

Regarding existing applications using existing libraries that suddenly change how their errors look/behave: This is why I like checked exceptions and structured errors, and not downcasting. If the users of your library are expected to want to destructure your error into exact types and source errors, then the exact error types are part of your public API and changing anything about them is practically breaking. But it's not technically breaking if the error is opaque and people use downcasting.

[faern]

Alternative solution: Deprecate source and make the guideline be to fully explain an error (including its source) in the Display impl. IMO that's even better. I have never understood the usage of source (except for logging the Display of it) anyway. If I want to destructure an error I create a structured error that I can pick apart with compile time checks.

[faern]

My takeaway is that we first need to figure out what the preferred way of using errors is, before we know the best way to design them.

• How should applications print/log/display errors
• How should applications/libraries destructure/handle errors

:)

[KodrAus]

cc @rust-lang/project-error-handling

This guideline deals with implementations of Display for Errors with respect to their implementation of Error::source.

[yaahc]

relevant thread rust-lang/project-error-handling#23

I tend to agree with @BurntSushi here and stand by a similar comment I made towards the end of the above thread.

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.

Tho backwards compatibility can trump this if youre worried people have been specifically relying on being able to downcast to sources that are also included in the outermost display impl

[mkantor]

I sense a possible vicious cycle:

1. Producers of errors are incentivized to include as much context as possible in every error message since they can't rely on end users seeing messages from sources.
2. Applications that report errors by concatenating source messages find their error reports to be redundant (because of 1). Application developers prefer not to write bespoke message-munging code for each error, so default to only printing the message from the topmost error.
3. The incentive in point 1 is strengthened.

If this is true it makes it unlikely that the ecosystem will ever naturally evolve towards a world where the source chain is useful and structured error reporting is the norm, even if a majority of rustaceans would prefer to live in such a world.

(I'm not sure how accurately this reflects reality—someone who's in tune with a large slice of the crate ecosystem might be able to say.)

[demurgos]

I agree with the message of @mkantor above: the lack of consistency makes it harder to predict what will be present in the message or what will be reported.

It's been about 4 years since the issue was opened, and I feel that most reporters converged on relying on the source. I feel like it's getting urgent to stabilize the guideline to avoid displaying the source message. Related to this unclear situation, snafu is resorting to manually clean-up the error message to remove the underlying source message.

Lack of guidelines makes it also harder when discussion error implementations with other people.

[shepmaster]

snafu is resorting to manually clean-up the error message to remove the underlying source message

In addition, I'm working on releasing SNAFU 0.8 (sometime this month, 🤞) which will no longer include the source message in the default Display implementation and I've removed the usage of source inside the Display implementation from the documentation / examples.

_{I'm doing my part!}