guidance on handling the display of io::Error with file paths

[BurntSushi]

Sorry for the length of this, but I think there are some aspects of this that I don't understand, so it's difficult for me to cut to the chase.

It is very well possible that I'm attacking this from the wrong angle, but the high level problem I'd like to solve is this: when I show errors whose underlying cause is an io::Error, it is very typically paired with a file path. For end users, both pieces of information are crucial: I need to show both the file path and the I/O error itself (which might be "not found" or "no permission" or whatever). Moreover, I'd like show it in the following format, which I feel is fairly standard:

path/to/foo: No such file or directory (os error 2)

As an application writer, the most natural-seeming path to take here was to use contexts. This is even highlighted as a prototypical example from the failure book:

let mut file = File::open(cargo_toml_path).context("Missing Cargo.toml")?;
file.read_to_end(&buffer).context("Could not read Cargo.toml")?;

But I can't actually use this approach, because it turns out that the the error messages emitted here are 1) fixed and 2) therefore might be wrong. That is, this might say that Cargo.toml is missing when the actual problem is that there was a permission error or something else entirely. An easy way to solve this problem is to surface the error message from the underlying I/O error. Being an inexperienced user of failure, I tried this approach:

let file = File::open(path).context(path.display().to_string())?;

If I had read the docs more carefully, I would have known that this indeed only prints the file path itself, and leaves out the underlying error. #182 seems to propose a fix to this, but I wonder whether the result of that would be this (the inverted format of my ideal error message):

No such file or directory (os error 2): path/to/foo

My next approach here was to just embed the io::Error's display string into my context:

let file = File::open(path).with_context(|e| format!("{}: {}", path.display(), e)?;

This gives me the output I want. As an application writer, I made this a bit easier by adding my own ResultExt trait with a blanket impl:

pub trait ResultExt<T, E>: failure::ResultExt<T, E> where E: fmt::Display {
    fn with_path<P: AsRef<Path>>(
        self,
        path: P,
    ) -> result::Result<T, failure::Context<String>>
    where Self: Sized
    {
        self.with_context(|e| format!("{}: {}", path.as_ref().display(), e))
    }
}

impl<T, E: fmt::Display>
    ResultExt<T, E> for result::Result<T, E>
where result::Result<T, E>: failure::ResultExt<T, E> {}

So that now I can just write

let file = File::open(path).with_path(path)?;

This was a somewhat annoying process, and being nudged to define my own trait here is slightly unfortunate. Regardless, as an application writer, I'm not too unhappy.

---

Let's switch gears. Instead of playing the role of an application writer, now let's talk about what I'm supposed to do as a library writer. To give away the ending here: I had to abandon derive(Fail)'s support for deriving Display and ignore some pieces of advice from the failure book. The end result is still better than the status quo with std::error::Error because I get causes/backtraces, but it took a quite a bit of will power/thinking to get here because I basically had to discard everything nice about failure and go out of my way to ignore your advice. :-)

OK, so, as a library writer, I really want to provide structured errors to my consumers. I do not want to use failure::Error because I want my errors to have types and be documented. I might be able to be convinced otherwise on this point, but I'm 99% sure this is non-negotiable. (For applications, I <3 failure::Error.)

Since I knew I wanted a structured error, I jumped right to the section on "An Error and ErrorKind pair." (I did look over the page on "A Custom Fail type," but I felt like the distinction between that page and "An Error and ErrorKind pair" wasn't especially clear to me. They almost look one in the same to me, at least, from the perspective of my particular problem.)

After reading "An Error and ErrorKind pair," I came up with this, which I figured would be wrong based on my experience above with contexts:

#[derive(Debug)]
pub struct Error {
    inner: Context<ErrorKind>,
}

#[derive(Debug, Fail)]
pub enum ErrorKind {
    #[fail(display = "{:?}", _0)]
    Path(PathBuf),
}

I also added the various Fail, Display, and From impls as suggested in your docs. Given my stumbling above with errors in my application, the problem here should be clear: the underlying io::Error won't get surfaced to the end user because the context swallows it up. (And if fixing #182 does permit it to be surfaced, then it seems likely it will be in the "wrong" format anyway.) At this point, it became clear to be that I needed to embed an io::Error into my ErrorKind enum. This was a difficult leap to make, because it seems strongly recommended against doing this in the section, "What should your ErrorKind contain?" In particular, it seems to specifically imply that the io::Error values should get swallowed into the context, and the ErrorKind should be responsible for specifying what went wrong from the perspective of the library.

Another problem with this approach is that I can't really use derive(Fail)'s Display auto-derive here because I can't use the Display impl for paths, which requires calling the display method. I believe an issue for that is filed already for this (#183, https://github.com/withoutboats/display_derive/issues/3).

So given that I 1) seemingly need to embed my own io::Error and 2) need to write my own Display impl, I finally landed with this:

#[derive(Debug)]
pub struct Error {
    inner: Context<ErrorKind>,
}

#[derive(Debug, Fail)]
pub enum ErrorKind {
    Io {
        #[cause]
        err: io::Error,
        path: PathBuf,
    },
}

Which is basically what I'd do today using std::error::Error, with of course the added benefit of implementing Fail, which gets me proper cause support, and of course, backtraces. Those are nice benefits!

My question is: is the above the ideal way to go about solving problem with failure? Is there a more idiomatic approach? It feels somewhat like I'm eschewing the happy path of failure here by embedding errors coming from dependencies (std in this case) into my own error types, instead of letting them become "causes" more naturally (at least, this is the philosophical impression I got from your book). In particular, one obvious downside from this approach is that this does seem to break my natural expectation of what causes would return here. Namely, if I iterate over all of the causes, then I get exactly one: my Error value. But since I've marked the io::Error as cause, I'd expect to get that too. In particular, that permits me to write something like this in my main function:

fn main() {
    if let Err(err) = try_main() {
        if let Some(ioerr) = err.root_cause().downcast_ref::<io::Error>() {
            if ioerr.kind() == io::ErrorKind::BrokenPipe {
                // broken pipe means our consume hung up, quit gracefully
                process:exit(0);
            }
        }
        eprintln!("{}", err);
        process::exit(1);
    }
}

It's clear to me from looking at the implementation of failure why this doesn't work. In particular, Error's cause method is implemented by calling Context's cause method, which in turn defers to its internal failure's cause method. Since I'm using Context::new, failure is a Either::This(Backtrace::new()), which has no cause. What isn't clear to me is why this is the implementation. My suspicion is that I've committed a category error, and that I probably shouldn't be using Context at all, and perhaps I should just create my own Backtrace.

Alternatively, I can eschew your recommended implementation of impl Fail for Error and write cause by skipping over the context completely:

impl Fail for Error {
    fn cause(&self) -> Option<&Fail> {
        self.inner.get_context().cause()
    }
}

This does indeed work as expected, but it is now abundantly clear that I should jettison Context altogether and just embed a backtrace directly into my error. The downside of this is that I (think) I now miss out on failure's conditional handling of backtraces with respect to no_std?

The final code I wind up with is this:

#[derive(Debug)]
pub struct Error {
    kind: ErrorKind,
    backtrace: Backtrace,
}

#[derive(Debug, Fail)]
pub enum ErrorKind {
    Io {
        #[cause]
        err: io::Error,
        path: PathBuf,
    },
}

impl Fail for Error {
    fn cause(&self) -> Option<&Fail> {
        self.kind.cause()
    }

    fn backtrace(&self) -> Option<&Backtrace> {
        Some(&self.backtrace)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        self.kind.fmt(f)
    }
}

And now I've finally wound up where I want to be, but I am completely and utterly uncertain about whether this is a good place to be. In particular, this particular type of error reporting is something I do very frequently, and I expect others will as well. That's why I've taken the time to write this issue, because I see this as an important problem to solve.

---

Let me pop up a level and add some meta discussion just to make sure my opinions/intent on this is clear:

• I don't necessarily believe my final destination is bad. For the most part, I've been of the opinion that error handling using just std::error::Error is Pretty Much Great Except for Causes and Backtraces, so needing to write out what is in my mind write-once but somewhat boiler-platey code is basically a non-issue for me personally. (I recognize that this is absolutely not true in the wider ecosystem, and that many people will go to great lengths to eliminate every ounce of boiler plate as possible. :-))
• My intent in filing this issue is to both give feedback on my own personal learning process, and to receive feedback on whether my proposed solution to my problem is the expected solution. If it is, I think the only real action item here would be documentation (from my perspective). If it isn't, then I'd very much appreciate being pointed in the right direction, because I definitely do not want to repeat this pattern over and over if it is not something that is intended to be supported well by failure.
• It seems plausible that this experience has soured me on the use of Context inside a library, but perhaps it is unique to this specific problem. In particular, it seems like the issue I'm running into here is that the user facing error message requires the composition of two bits of information, but it seems like failure's design and documentation push toward a more hierarchical approach where errors don't really know about each other, and thus, can't be formatted holistically. And this is perhaps in turn a consequence of the fact that a bare io::Error is rarely a sufficient user displayable error on its own when dealing with file I/O, but does tend to be necessary in my experience.

[BurntSushi]

After sleeping on this, I realized that a simpler solution might exist. Namely, I could go back to my types looking like this:

#[derive(Debug)]
pub struct Error {
    ctx: Context<ErrorKind>,
}

#[derive(Debug, Fail)]
pub enum ErrorKind {
    Path(PathBuf),
}

But in my application, I can display the error message by iterating over the causes and assembling the message. I got the idea from the #165 PR, which basically does exactly that.

I'm still a tad uneasy with this, and I would still appreciate advice on this entire affair. In particular: relying on causes to format the error message feels a little funny, almost too implicit, but I can't quite convince myself that it's much different than what I'd do more explicitly/manually. I think my brain just needs to adapt to it.

[BurntSushi]

One other short-coming I can see is that this approach (I think) is that it makes it impossible or hard to pull out the original io::Error. (It does seem possible if you're determined, by taking advantage of causes/downcast_mut/mem::replace.) The use cases for that are hard to articulate though (I've certainly never needed it. I only ever need to inspect the io::Error.)

[IslandUsurper]

After sleeping on this, I realized that a simpler solution might exist. Namely, I could go back to my types looking like this:

I had just gotten to this point in my library, but for some reason it took me the longest time to do

But in my application, I can display the error message by iterating over the causes and assembling the message.

even after reading this whole issue a few times. For some reason I expected failure to do that automatically. Now that I've actually implemented it myself, though, that's probably an unreasonable expectation. There are many different ways it could be done (and probably specific to one's own Fail types), and I can imagine someone might not want such a thing to happen in their code.

I think the documentation ought to nudge users in the direction of putting causes in their Display impls, but then that makes deriving Display for Fail types much less useful.

[BurntSushi]

For some reason I expected failure to do that automatically. Now that I've actually implemented it myself, though, that's probably an unreasonable expectation.

I suspect so as well, but on the flip side, I will literally need to write this code for every application that I write. I'll even want it for small one-offs. I suspect there is some middleground between "that's what failure does automatically" and "failure can be told to do it." :-)

[mitsuhiko]

I keep running into this. In my case I actually want to use the error kind pattern and use the kind to say what type of error I have and that wraps via ResultExt a inner io::Error. Sadly this now means I cannot attach more information into the outer error.

What I wanted is something like this:

#[derive(Debug)]
pub struct ConfigError {
    filename: PathBuf,
    inner: Context<ConfigErrorKind>,
}

impl Fail for ConfigError {
    fn cause(&self) -> Option<&Fail> {
        self.inner.cause()
    }

    fn backtrace(&self) -> Option<&Backtrace> {
        self.inner.backtrace()
    }
}

impl fmt::Display for ConfigError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{} (file {})", self.inner, self.filename().display())
    }
}

impl ConfigError {
    /// Creates a new config error
    pub fn new<P: AsRef<Path>>(p: P, kind: ConfigErrorKind) -> ConfigError {
        ConfigError {
            filename: p.as_ref().to_path_buf(),
            inner: Context::new(kind),
        }
    }

    /// Returns the filename that failed.
    pub fn filename(&self) -> &Path {
        &self.filename
    }

    /// Returns the error kind of the error.
    pub fn kind(&self) -> ConfigErrorKind {
        *self.inner.get_context()
    }
}

/// Indicates config related errors.
#[derive(Fail, Debug)]
pub enum ConfigErrorKind {
    /// Failed to open the file.
    #[fail(display = "could not open config file")]
    CouldNotOpenFile,
    /// Failed to save a file.
    #[fail(display = "could not write config file")]
    CouldNotWriteFile,
    /// Parsing a YAML error failed.
    #[fail(display = "could not parse yaml config file")]
    BadYaml,
}

The issue here now however is that I do not know how to actually construct the ConfigError with context()/with_context(). As far as I can tell there is no way other than to first construct the error with context and then to provide a second method that attaches the location info.

[mitsuhiko]

To clarify this is what I do at the moment:

macro_rules! ctry {
    ($expr:expr, $kind:expr, $path:expr) => {
        match $expr {
            Ok(val) => val,
            Err(err) => return Err(ConfigError::new($path, err.context($kind)))
        }
    }
}

And then accept the Context directly in the constructor of the config error.

[richard-uk1]

A related problem I have is when I have

enum ErrorKind {
    Variant(PathBuf)
}

and I want to display the path, I really want to do #[fail(display = "text {}", _0.display())], but for the moment I have to use the debug formatter.

[0xpr03]

Same problem here, trying to wrap my head around getting all of these together and having some context. The examples here at least give me a starting point.

[casey]

I'm trying to figure this out, and the thing that worked the best for me wound up being a custom type implementing Fail that contains a generic error and a generic location, along with a convenience method on Result<T, E> that iff is_err annotates with a location.

The crate is here, feedback is most welcome.

Leaving out some of the details, it looks like this:

/// Location that an `Error` can occur
pub trait Location: Debug + Send + Sync + 'static {
  /// Format the location and error for display
  fn fmt_error(&self, f: &mut Formatter, error: &dyn Fail) -> fmt::Result;
}

// `Fail` is implemented for `Error` elsewhere
#[derive(Debug)]
pub struct Error<E: Fail, L: Location> {
  /// Error
  pub error: E,
  /// Location at which `error` occurred
  pub location: L,
}

impl<E: Fail, L: Location> Display for Error<E, L> {
  fn fmt(&self, f: &mut Formatter) -> fmt::Result {
    // defer to location so users can implement custom formatting
    self.location.fmt_error(f, &self.error)
  }
}

pub trait ErrAt<T, E: Fail> {
  /// Iff self.is_err() annotate with location
  fn err_at<L: Location, I: Into<L>>(self, location: I) -> Result<T, Error<E, L>>;
}

I wonder if this is a reasonable approach, or if I'm missing a more idiomatic way to do this.