Added support for rendering errors with causes

[mitsuhiko]

When .display() is called on an error or fail this renders out the
fail and its causes into a human readable string:

error: this is the first error
  caused by: this is the second error
  caused by: this is the third error

[matklad]

Nice! One question is should we add a knob to control backtrace printing? I’d say no: if you want to customize the rendering, just iterate causes yourself.

[mitsuhiko]

@matklad i do want to auto trim the backtraces in the default rendering not to show useless internals but that's about it.

[BurntSushi]

@mitsuhiko What is your plan for allowing others to control the formatting of error messages? For example, I'd be unlikely to use something like this, since I'd much rather use the (IMO) more standard msg1: msg2: ... msgN format.

[mitsuhiko]

@BurntSushi: just formatting or also what’s in the text? For formatting a builder pattern probably would be useful but also the default should be good.

[BurntSushi]

I agree the default should be good. I don't think the formatting/text in this PR is not good per se, for what it's worth. It just seems somewhat non-standard to me, at least for command line applications. I'm used to seeing things like this:

$ find /var/lib/postgres/data
/var/lib/postgres/data
find: ‘/var/lib/postgres/data’: Permission denied

That is, the general format here might be cause: cause: cause: message. When I've used failure in command line applications, I've used this to format my error messages:

fn pretty_error(err: &failure::Error) -> String {
    let mut pretty = err.to_string();
    let mut prev = err.cause();
    while let Some(next) = prev.cause() {
        pretty.push_str(": ");
        pretty.push_str(&next.to_string());
        prev = next;
    }
    pretty
}

(The omission of the backtrace here isn't meant to be important. I like the idea of how you include it in this PR.)

Maybe there's room for "display pretty" and "display condensed." Not sure.

[mitsuhiko]

@BurntSushi one thing I noticed is that it's pretty unclear how contexts are used and how errors are wrapped. For instance in one of our sentry projects a common chained error looks like this rendered right now:

error: could not parse yaml config file (file /Users/mitsuhiko/Development/sentry-semaphore/.semaphore/config.yml)
  caused by: mapping values are not allowed in this context at line 2 column 6

As an example for a broken config. These errors are so long and involved that I don't like rendering them in a single lie. But I suppose it depends on how these errors are actually structured.

[BurntSushi]

@mitsuhiko Yeah I hear ya. I agree that multiple line formatting in cases like that is better.

[mitsuhiko]

@BurntSushi i think it would be interesting to start collecting uses of failure in the wild to see how errors are actually used. For instance for the error above our own code also runs into #189 and we have to do quite a lot of extra handling to get extra info (the path) into the error. There is no pattern for this in failure right now and different libraries come up with different solutions.

If we could collect real world uses of failure in small examples it could be really helpful for driving both default formatting decisions as well as helping evolving the API and guide users.

[BurntSushi]

@mitsuhiko Yeah that's a good idea. I've also struggled with getting file paths into my errors. The only way I've succeeded has been by being very diligent.

[matklad]

One tiny drawback of the proposed API is that it's unclear, without referring to the docs, what is the difference between format!("{}", an_fail) and format!("{}", an_fail.display()). That is, display name does say that it's multiline. I don't know the better name though, and "display" does seem good enough.

[dekellum]

How about into_display_ml ?

[dekellum]

sigh, how about waves_hands_without_offering_solution() @matklad.

[mitsuhiko]

@matklad I cannot change how Display works because every single fail can implement it independently.

[dekellum]

(Steps up to bat a second time, this time with helmet on): How about naming it displayln()? You know like println is to print? Short but still a good clue.

[dekellum]

Oh, also... :) If the displayln() output style has internal NEWLINEs, it should (I think) also have a terminal NEWLINE, but I don't think (I'm not sure) that the current implementation does that.

[dekellum]

(Ball 3): Or

a_fail.as_display(FailDisplayOpts::LineBreaks)

…for self documenting, and to give it some future expansion room. (Oh dear, let this not regress into another exhaustive matching debate.)

[mitsuhiko]

Makes no sense to call it displayln because it does not end with a newline. Also there is absolutely no precedent for that.

[dekellum]

@mitsuhiko Um, but shouldn't it end with a newline if its going to contain new lines? Like for predictable console/log output? (Apologies, I should have made one comment instead of 3 with edits. )

[mitsuhiko]

There are no APIs that I know about that work that way.

[dekellum]

Probably not the best example, but the first that comes to my mind is the Ruby language, puts on an array of strings:

puts(["a", "b", "c"]) # =>
a
b
c

Note all lines, including the final "c" are newline terminated in the output.

Its also how I immediately interpreted your PR would behave based on the block formatting of your first example. If there is also great examples of not doing it this way, then I stand corrected. See also (Ball 3) alternative naming. :)

[dekellum]

Here is much better example:

#[macro_use] extern crate serde_json;

fn main() {
    let obj = json!({"foo":1,"bar":2});
    println!("{}", serde_json::to_string_pretty(&obj).unwrap());
}

Output includes a terminal NEWLINE:

{
  "bar": 2,
  "foo": 1
}

[Celti]

Erm... your output is using println!(), so of course it's going to have a terminal newline. Notably, serde_json::to_string_pretty() does not emit a terminal newline, which is the exact opposite of the point you were trying to make, as far as I can tell?

[dekellum]

Good point! But this was the first example usage I found of to_string_pretty, suggesting that users expect a terminal NEWLINE for something that is "pretty printed" or block formatted. But if no terminal NEWLINE is common in some family of languages I've forgotten about, and rust has inherited that, then I stand corrected. Which is least surprising here?

[Celti]

Least surprising to me is to not have a terminal newline. The various "pretty-printers" I'm familiar with in Rust do not emit them; nor does, say, Python's pprint module — pprint.format does not have a terminal newline, while pprint.print does only because Python's print is equivalent to Rust's println!.

[dekellum]

OK, I stand corrected. displayln is a poor name and output should not be NEWLINE terminated. How about as_display(FailDisplayOpts::LineBreaks) :)

[mitsuhiko]

I'm going to lock this conversation for now because it went into IMO irrelevant bikeshedding territory and I keep getting notifications for this.

[dekellum]

This is the first time I've seen an issue locked on github. Apologies for having made a nuisance of myself here. At the start I was assuming you (@mitsuhiko) were attempting to include this in 0.1.2, and while I think @matklad was correct that the display name is less than ideal, I ironically also didn't want semantics to get in the way of this being included. I want to use this feature! The segue into rust print-output conventions was my education at your expense, but my thanks and apologies regardless.

As is, In a custom FailExt trait, I or anyone could wrap a_fail.display() as a_fail.as_display(Lines) or (builder-ish) a_fail.with_lines()—for clarity in a local code base.

[mitsuhiko]

display() is the pattern that is also used on Path.

[matklad]

Here's an alternative proposal, which I am not sure is better. We specify that Display impl for T: Fail must use # flag to print full traceback and error chain.

That is, format!("{}", an_fail) prints a one-line human-readable summary, and format!("{:#}", an_fail) prints the whole error-chain, together with all the tracebacks, and uses format("{}") internally.

I feel like this would be the actually intended use-case for # in non-Debug contexts. OTOH, I don't think this API would be easy to discover, because in practice # is only ever used for Debug I think?

[dekellum]

Use of "{:#}" (alternate) for Display is documented, incl. an example, even if its rarely used:

https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.alternate

Access to the feature via "{:#}", or an optional (not compiler required) public method of any name, are equally in need of a doctest example to be discovered, so I don't see that as much of a downside in itself.

Perhaps the bicycle storage(?) issue is more about giving the impression of a default vs a secondary/alternative Display form with internal new-lines?

[mitsuhiko]

We cannot change the rendering of # because the user implements Display themselves.

[dekellum]

OK, but we could do two things to help the situation, I believe:

1. Implement the line formatted form via a public default method of the Fail trait, like:

pub fn fmt_with_lines(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
    //...
}

2. Implement Display with this check for alternative via derive(Fail) (as suggested in doc and impl) and for Error.

Then the user still has the option of overriding Display in a custom Fail, but that seems more like a feature than an issue, right? I won't claim more on (2) without prototyping, but with (1) in place, at least a user-custom impl Display for a custom Fail type, would be able to conveniently reuse that method, no? And you still have the option to also use and expose (1) via a custom adapter as you currently return from display in this PR?

[mitsuhiko]

I don't think that's either user friendly or a good idea. What exactly is the issue with a separate display method on the trait?

[dekellum]

What exactly is the issue with a separate display method on the trait?

Because this:

display() is the pattern that is also used on Path.

In the Path case, if you attempt to format an instance directly with {} you get a compiler error because the type doesn't implement Display. In your proposed API, user won't get a compiler error without the call, and the display name doesn't indicate why its necessary or useful. Said another way: its not the same situation as with Path::display, so it shoudln't use the same name display. For me the name with_lines solves the immediate problem with your adapter design approach, but I also think {:#} may be a better pattern for general adoption of the formatter facility.

Disclaimer: This is a reply to your direct question and not in any way because I have a bicycle storage problem! I don't intend to comment further.

[mitsuhiko]

Your comment suggests the only issue with display is the name.

[dekellum]

@mitsuhiko Given your tireless encouragement of my participation, I've prototyped my suggestions, learned some of the limitations, and refined the naming and features in #244. In summary this uses chain_display for naming, adds a top level convenience method which makes your original example/simple usage nicer (see diff below from this PR), and incorporates the above suggestions to make "{:#}" the means of selection of multi-line (e.g. pretty) formatting with backtraces.

--- a/examples/simple.rs
+++ b/examples/simple.rs
 fn main() {
-    println!("{}", Fail::display(&bad_function().unwrap_err()));
+    println!("### default fail(display = \"...\") ###");
+    if let Err(ref e) = bad_function() {
+        println!("{}", e);
+        println!("{:#} (with {{:#}})", e);
+    }
+    println!();
+
+    println!("### line ({{}}) chain_display ###");
+    if let Err(ref e) = bad_function() {
+        println!("{}", chain_display(e));
+    }
+    println!();
+
+    println!("### block ({{:#}}) chain_display ###");
+    if let Err(ref e) = bad_function() {
+        println!("{:#}", chain_display(e));
+    }
+    println!();

The examples/simple changes are a bit pedantic and might be better moved to unit or doctests (also lacking for this PR).

[dekellum]

It also made sense to impl Debug for the ChainDisplay new type—done in #244 with the following output from cargo run --example simple. Please consider closing this in preference to #244, or otherwise commenting on that PR. Thanks!

——— default fail(display = "...") ———
my wrapping error
my wrapping error (with {:#})

——— default fmt::Debug ———
WrappingError(MyError) (with {:?}) 
WrappingError(
    MyError
) (with {:#?})

——— line ({}) chain_display ———
error: my wrapping error; caused by: my error

——— block ({:#}) chain_display ———
error: my wrapping error
  caused by: my error

——— line ({:?}) fmt::Debug for chain_display ———
error: WrappingError(MyError); caused by: MyError

——— block ({:#?}) fmt::Debug for chain_display ———
error: WrappingError(
    MyError
)
  caused by: MyError