Keep context when converting to io::Error

[vitiral]

This change will keep "pretty" error messages even when the error has been converted to io::Error.

Example:

Pretty error: IO error for operation on /path/does-not-exist: No such file or directory (os error 2)

Justification

The standard library returns std::io::Error for failed filesystem operations. io::Error is an extremely lightweight type which typically uses &'static str for its "caused-by" error argument to keep it even more lightweight. Many filesystem operations could include path information, but they don't -- probably for performance and compatibility reasons.

However, the type itself makes no requirements on what the "caused-by" error type is. When the type is returned unmodified from std::fs or std::path methods it doesn't include context (for the above reasons). This makes working with files extremely difficult especially while debugging or trying to present a simple error to a user. In order to include context information it is necessary to do something like:

file.set_permissions(perm)
    .map_err(|err| io::Error::new(err.kind(), format!("{} while setting permisions for {}", err, path.display())))?;

As you can imagine this can lead to a lot of code duplication.

Walkdir solves this problem, as detailed in Solution below. Other libraries also solve the same problem. However, if using them both from within the same function (and not wanting to use Failure or similar type) you are forced to use io::Error as the intermediate error type (for ergonomic reasons).

For instance, this example which is a non-contrived example that will happen quite often in the new ergo_fs library:

use ergo_fs::{WalkDir, PathType};

fn some_fn(...) -> io::Result<_> {
    for entry in WalkDir::new("foo").min_depth(1) {
        match PathType::new(entry?.path())? {
            PathType::File(file) => println!("got file {}", file.display()),
            PathType::Dir(dir) => println!("got dir {}", dir.display()),
        }
    }
}

It would not be possible to use either path_abs::Error or walkdir::Error in that context, and in most cases io::Error is all the user wants or needs. However, using io::Error in this case will remove the context of the error for all walkdir operations.

Solution

Walkdir already includes contextual information in its walkdir::Error type. However, when converting From<Error> -> io::Error walkdir returns the original unaltered io error (if it exists). This strips the contextual information that it had access to.

If instead it returned io::Error::new(kind, walkdir_err) there would be zero performance penalties (except when formatting the io::Error) but the error messages would be ergonomic regardless of whether it was converted or not.

This solution also adds the to_io_error method, which will return the plain unaltered io::Error if the user needs it.

Disadvantages

The major downsides are:

• The error message as well as the get_ref result will change. Primarily get_ref will actually return something after converting to io::Error. This could be considered a semver breaking change, although that would be a very hard reading of an API regression (it is unlikely anyone depends on the current behavior).
• Extremely minor additional memory usage by the io::Error after conversion.

I cannot think of additional disdvantages. This has zero cpu cost, does not contribute significantly to the size of the io::Error and will be a uniform benefit to the end user of this library.

Thanks for considering this change, and I'm sorry about the rought start in the previous PR

Edit: additional disadvantage

So there does seem to be one additional downside: raw_os_error appears to be able to return the last raw os error. I think it is doing so by digging into the inner error.

With this change the user would have to use downcast to retrieve the original walkdir::Error, which they would then call into_io_error on to get the original io::Error, from which they could (finally) retrieve the os error number if it existed.

I seriously doubt this is a common need, and for the cases where it is they would definitely be using walkdir::Error directly. However, the fact that the specific OS error is less retrievable is a minor disadvantage and minor breaking change.

[vitiral]

Also heads up, as I look over the walkdir docs more I'm convinced there might actually be a benefit to wrap the walkdir types in ergo_fs so that they have a unified API with the other types. I think I'll also just make the method PathDir::walk, which is super nice.

I would still appreciate this getting merged, as it would reduce a little bit of work on my end in making the error messages prettier -- but I don't think it is a big deal anymore.

So if the breaking change above makes you squeamish I understand and I think I can wrap walkdir reasonably well, which will have other benefits as well.

Thanks for the discussion/consideration.

Edit: I take back everything I said... as I'm writing the "wrapper" I basically see myself creating newtypes for all of your types... no fun :( It would be great to just have clean io::Error messages. Pleeeeease :)

[vitiral]

For an example of another major rust crate that keeps the original error when converting, see tar::Error.

I'm working right now on improving the context for errors returned from that library, but it already preserved the original error when converting.

[vitiral]

So another option which comes to mind (not necessarily related to this PR, could be in addition to it) is to have a crate which only defines a standardized FileSystemError type. This would be roughly:

pub struct FsError {
    /// The kind of this error
    kind: FsErrorKind,
    /// Caused By Error
    error: Option<impl Error>,
}

pub enum FsErrorKind {
    /// The core error is related to IO on some path
    IoError(PathBuf, io::Error),
    /// A loop detected in the filesystem, includes one of the nodes in the loop
    LinkLoop(PathBuf),
    /// Library specific error
    Other(impl Error),
    /// ... I can't think of others right now off the top of my head
    #[non_exahustive]
}

Then multiple libraries which interface with the filesystem could auto-convert to that type, which would always have useful error messages + context.

[vitiral]

Thanks for taking the time to think about this issue! You are certainly right that you are the core maintainer and writer of a fair number of foundational crates. As someone who is conglomerating foundational crates I find your work everywhere and I am always impressed.

From your comments I took the effort to do a complete doc review for the Error type. I think I've caught any bugs or misstatements. Hopefully this reduces your workload in the future of fixing mistakes left by my changes.

[BurntSushi]

This PR is included in walkdir 2.1.0 on crates.io.