Make adding paths to I/O errors even simpler

[sffc]

Currently the docs suggest doing something like

let content = fs::read(path)
    .wrap_err_with(|| format!("Failed to read instrs from {}", path.display()))?;

This is such a common case that the above code just becomes boilerplate. It would be nicer if it were just

let content = fs::read(path).wrap_err_with_path(path)?;

[yaahc]

It would be nicer if it were just

In my opinion, no. I don't think paths contain enough information on their own to construct a relevant error message. What would the error message in the second case be? We could follow the example of fs_err and print "failed to open file {path}", but this already assumes that the previous operation was an attempt at opening a file. I don't know of any way we could restrict wrap_err_with_path to only be callable on fs::read or equivalent APIs. The best I can think of would be to restrict it to Result<T, io:Error>, but even then that would allow this method to be callable in many places where the operation wouldn't be related to opening files. We could match the specific ErrorKind from each io::Error and assume something about the operation based on the error, but this feels fragile to me.

Imo, if you want a general solution for this where you don't have to think about adding the path, crates like fs_err are the correct approach to this problem. Instead of adding combinators to attach the extra information they wrap the entire API and are able to attach semantically relevant error messages to each API.

But even beyond that, I don't personally like using crates like fs_err because I think the error messages they add to the chain are still only vaguely relevant. "failed to open file {path}" is adding very little additional information about what error you encountered that prevented you from continuing, other than that you were attempting to open a file, and the path of the file. In the original example you posted it includes an error message "failed to read instrs from {}". To me this is ideal, you describe the higher level task that you were attempting to complete, reading instructions from a file, and you can include the relevant context about what file that was, then you have the source error which indicates "file doesnt exist" or something like that, which explains why you weren't able to read the file. Each error in the chain describes a reason why a specific unit of work couldn't be completed at a higher and higher level with minimal duplicated information.

I understand that there's tension here. This approach makes it easy to forget to introduce wrapping errors that contextualize what you were doing and the relevant state, making it easy to lose path information like this. So in a sense I'm not confident that my approach is the best one, but if I had to choose an alternative I'd prefer something like fs_err to attempting to solve the problem for fs in eyre.

[sffc]

Thank you for the detailed response!

What would the error message in the second case be?

Just a concatenation of the I/O error description with the path, with a joiner like ": ". The I/O error already tells us why the operation failed. For example, the full error message would be something like, "PermissionDenied: foo.txt". This would be at least as good as something you can already do with eyre:

fs::read(path).wrap_err_with(|| path.display().to_string())

The best I can think of would be to restrict it to Result<T, io:Error>

My experience suggests this is a common case that deserves a special function. Cases where files are processed without an io::Error can still use the plain wrap_err_with().

Imo, if you want a general solution for this where you don't have to think about adding the path, crates like fs_err are the correct approach to this problem. Instead of adding combinators to attach the extra information they wrap the entire API and are able to attach semantically relevant error messages to each API. ... But even beyond that, I don't personally like using crates like fs_err because I think the error messages they add to the chain are still only vaguely relevant. ... So in a sense I'm not confident that my approach is the best one, but if I had to choose an alternative I'd prefer something like fs_err to attempting to solve the problem for fs in eyre.

I didn't know about this alternative; thanks for the pointer.

To give an idea of my perspective (not that I expect you to share it): As a practitioner, I would like eyre to solve all my error needs. I/O errors are one of the main reasons I sought out something better than the standard library. From my perspective at least, I would like to use eyre alone, rather than making me find a second helper library.

[yaahc]

Just a concatenation of the I/O error description with the path, with a joiner like ": ". The I/O error already tells us why the operation failed. For example, the full error message would be something like, "PermissionDenied: foo.txt". This would be at least as good as something you can already do with eyre:

Aah, adding the info to the end of the existing error message without adding a new error to the chain of errors available via source seems like it could work well. The main issue I see there is that if you do this by wrapping the original error and masquerading as it you may make it substantially less convenient to react to specific error cases. With io::Error in particular I think there may be some clever tricks you could pull to mess with the error message while keeping the ErrorKind and other os error identifying information the same, but if you wanted this to work generally with all types of errors you'd run into significant issues if you ever need to downcast and match against the original error.

Regardless though, I think this is absolutely something that you could add as a helper trait locally, test out, and then report back how it went. It would look something like this:

use std::io;
use std::path::PathBuf;

// replace this with the type you actually care about such as `eyre::Report` or `io::Error`
// or some custom wrapper type.
struct WrappedIoError;

trait WithPath {
    type Return;
    
    fn wrap_err_with_path(self, path: PathBuf) -> Self::Return;
}

impl WithPath for io::Error {
    type Return = WrappedIoError;
    
    fn wrap_err_with_path(self, _path: PathBuf) -> Self::Return {
        todo!("append the pathbuf to the io::Error")
    }
}

impl<T> WithPath for Result<T, io::Error> {
    type Return = Result<T, <io::Error as WithPath>::Return>;
    
    fn wrap_err_with_path(self, path: PathBuf) -> Self::Return {
        self.map_err(|err| err.wrap_err_with_path(path))
    }
}

To give an idea of my perspective (not that I expect you to share it): As a practitioner, I would like eyre to solve all my error needs. I/O errors are one of the main reasons I sought out something better than the standard library. From my perspective at least, I would like to use eyre alone, rather than making me find a second helper library.

I don't know your specific use cases so this may be entirely reasonable, but I don't generally view eyre as a one size fits all error type. If you exclusively interact with errors by reporting them then it would be a perfect fit, but if you're ever going to react to error conditions programmatically I expect eyre is going to significantly degrade your experience, or if you ever need to assert that all error cases are handled at compile time.

My experience suggests this is a common case that deserves a special function. Cases where files are processed without an io::Error can still use the plain wrap_err_with().

I'm not following this bit, what do you mean by "Cases where files are processed without an io::Error"? The issue I was bringing up is that std uses the io::Error type in many places that aren't related to file io. Regardless though, I think your comment about just appending the path rather than adding a new error message to the chain makes this a moot point. I don't think its nearly as important to restrict when this method is callable if you're not going to be adding messages like "failed to open file" or something. Just appending the path seems like it would fit well in many more situations

[ten3roberts]

Chiming in to this.

I have throughout a couple of points in my Rust journey encountered this same issue of "I need a file path with this".

While providing a file path initially aids debugging, they do not as @yaahc put it contribute much more of what was attempted to be done.

Did we fail to read a config file? An image for displaying the UI, the project file etc. Each of these are different in their own right, and as such deserve their own context.

Adding the ability for Eyre to change the shape of the internal error message, rather than adding more causes in the chain. This not only creates a surprising side effect gotcha if the type matches, but also leads to Eyre being able to act in two different ways, without necessarily being clear which one will be used for a given case. I always tend to keep my apis predictable and strive towards making the api have one way, and one way only to achieve each distinct goal.

What I've done in more recent times is

read_config(path).await.wrap_err!(|| eyre!("Failed to read config file {path:?} specified by env"))?

This makes it clear what failed and why it failed, but also how it got there