make regular io errors pretty

[vitiral]

This aims to make errors prettier even when they are downcast to io::Error. It does this by slightly modifying the type -- I doubt the downcast io::Error type is dependend on by anyone, but I suppose this could theoretically be considered a breaking change.

I added a quick println! example that should be removed before merging. It prints:

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

[vitiral]

Note: I am trying to conglomerate several filesystem crates and want to make sure they all have pretty error messages, even when downcast to std::io::Error. Thanks!

[BurntSushi]

Hmm this seems a little strange to me and seems like a violation of what the intent of the conversion is. That is, instead of returning the underlying I/O error, we're returning a new I/O error that embeds the walkdir error which in turn still contains the underlying I/O error.

I suspect you'll need to find another way to make errors pretty.

[vitiral]

We could also do something like io::Error::new(kind, walk_err.to_string()).

The io::Error is next to useless. It never gives you any context (it is made for both filesystems and network sockets after all). I don't think having exactly the same display output is a feature per say, it is just what the stdlib does to save on performance (it is a really small gain).

[BurntSushi]

@vitiral I don't think you're hearing me. I'm not talking about the display output. I'm talking providing a way for callers to get the underlying I/O error exactly as it occurred.

Could you brainstorm other means of solving your problem? I can help you.

[vitiral]

I mean, the io::Error docs state:

Creates a new I/O error from a known kind of error as well as an arbitrary error payload.

This function is used to generically create I/O errors which do not originate from the OS itself. The error argument is an arbitrary payload which will be contained in this Error.

It isn't against the io::Error API to include an arbitrary payload, like a walkdir::Error. The error preserves the io::ErrorKind, which is the only relevant equivalency to io::Error.

[vitiral]

We could keep the From implementation as it is in this review, but also provide Into<io::Error> for walkdir::Error. This would return the original io::Error (if there even is one).

Edit: basically Into<io::Error> would have the current behavior of From, whereas From (which is an automatic conversion) would look like an io::Error but would basically stay a walkdir::Error.

[BurntSushi]

@vitiral I don't think that's possible because of the blanket impl on Into, and even if it were, implementing subtly difference semantics on those two impls doesn't sound like a good idea to me.

[BurntSushi]

Can you find precedent for this sort of error conversion elsewhere in the ecosystem?

Another consideration here is that if you're after more nicely formatted errors, then keeping to io::Error might be too low of a standard.

[vitiral]

I'm confused why you want the "original IO Error". What is the important information it contains other than a string?

We could add a method on walkdir::Error called to_io_error(self) -> Option<io::Error> -- i.e. only returns an io::Error in the case the original error actually was an io::Error. I think this would be more faithful to the desire to get "the original io::Error" if that was actually wanted.

[vitiral]

It makes sense to not do a blanket impl on Into, especially one that is different than From.

Sorry if I'm being combative. I got way too little sleep last night thinking about rust filesystem ergonomics >_>

[vitiral]

I mean, the only precedent is the version I just pushed to path_abs. See some of it's logic.

Basically when I write a function, I typically don't care about the particular IO error it returns, but as an application developer I do care about the context. I can't return walkdir::Error since that would mean that I couldn't use ? with File::read(), and it wouldn't interoperate with other io::Errror-like crates. For many cases, depending on something like Failure or error-chain is way too heavy handed as well.

When I do care I can preserve the type, when I don't the conversion happens automatically but I keep the context. That (in my mind) is good ergonomics. I can write my library pretending everything is the base io::Error (which ensure good interoperability with other crates) and my users still get to see which file caused their error (and what action as well!)

Edit: without this, the user of a library that wants to use io::Error has to call map_err everwhere and include the context themselves, which gets really annyoing considering the number of places the filesystem can fail.

[BurntSushi]

I'm confused why you want the "original IO Error". What is the important information it contains other than a string?

My goal is to provide as thin of a layer possible to the underlying errors. If it weren't for the fact that this crate produces loop errors, then it's unlikely this crate would have its own error type at all.

We could add a method on walkdir::Error called to_io_error(self) which returns a Some -- i.e. only returns an io::Error in the case the original error actually was an io::Error. I think this would be more faithful to the desire to get "the original io::Error" if that was actually wanted.

This is a possibility. Are there other crates that do this?

How are you handling io::Errors produced by std? Why can't you do the same with walkdir?

It makes sense to not do a blanket impl on Into, especially one that is different than From.

You can't "choose" to provide a blanket impl, it is already done: https://doc.rust-lang.org/std/convert/trait.Into.html#generic-implementations

I can't return walkdir::Error since that would mean that I couldn't use ? with File::read(), and it wouldn't interoperate with other io::Errror-like crates.

Sorry, but I don't understand this. The From impl in question is exactly what permits one to use walkdir in an io::Error context. That is, if you use ? on a walkdir error call where the enclosing function returns an io::Error, then the walkdir error will get converted into an io::Error using exactly the conversion you're modifying in this PR.

When I do care I can preserve the type, when I don't the conversion happens automatically but I keep the context. That (in my mind) is good ergonomics. I can write my library pretending everything is the base io::Error (which ensure good interoperability with other crates) and my users still get to see which file caused their error (and what action as well!)

I do find this somewhat compelling. Does std do it? If not, why not?

Sorry if I'm being combative. I got way too little sleep last night thinking about rust filesystem ergonomics >_>

It's fine. I'd encourage you to get some sleep. Please take a breath, back up, and talk about the specific problem you're trying to solve. I'd also like to encourage you to slow down. Getting an onslaught of rapid comments isn't a pleasant experience.

[vitiral]

std doesn't do it because it uses &static strings everywhere. Probably for (minor) performance reasons, as well as better compatibility (which is a worthy goal for the rust stdlib).

std doesn't do it wrong, it is just that it is too low level and IMO not worth emulating for a display output, especially when the library already has the context it needs for prettier errors.

I'll get to the other points after I get some sleep 😄

[vitiral]

How are you handling io::Errors produced by std? Why can't you do the same with walkdir?

I'll do just this one. In path_abs I do this kind of thing everywhere:

        self.file
            .set_permissions(perm)
            .map_err(|err| Error::new(err, "setting permisions for", self.path.clone().into()))

I copied my Error from yours, but mine does keep the formatting. Other than that our error types are very similar.

However, if someone had a function that (for example) used both walkdir and path_abs but they wanted to preserve the error formatting it would be very unergonomic right now. If they returned walkdir::Error then they couldn't use ? with path_abs stuff, but if they returned io::Error they would loose the pretty errors for walkdir.

This is actually the exact case I'm trying to solve in an example for ergo_fs.

[vitiral]

here is the example: https://docs.rs/ergo_fs/0.1.0/ergo_fs/#examples

[vitiral]

Okay, I've gotten some sleep. I'm going to close this and try again, this time with a more polite approach 😄