Include the errored file path (when available) in std::io::Error instances

[mqudsi]

Given that probably the most common model for handling errors in rust programs is just to bubble them up, it is far too common to see errors like this:

mqudsi@ZBOOK /m/c/U/m/g/mytube> env RUSTC_WRAPPER=sccache cargo build
error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `sccache rustc - --crate-name ___ --print=file-names -C target-cpu=native -C link-arg=-fuse-ld=lld --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=cfg` (exit code: 2)
--- stderr
error: No such file or directory (os error 2)

Note the last line: sccache panicked because it couldn't find some file. Which file? Who knows. You'll need to debug or grep the code to find out.

This is actually because std::io::Error is guilty of the same thing sccache did: it just bubbled up the OS error without adding any context.

This particular sccache error is just the error I was dealing with last before filing this issue, but I run into this about every other day dealing with 3rd party crates in the ecosystem. In my own code, all of my crates have to wrap std::io::Error in a new error type that includes the file path and at the site of invocation where std::io::Result is first returned, every std::io::Error gets mapped to a custom error that includes the path in question, if any.

I would like to propose that - at least in debug mode, although preferably always - any IO operations that involve a named path should include that path in their error text, and perhaps even the error type should be extended to include an Option<PathBuf> field (or select APIs returning a different error type).

I know this comes with some caveats. It's hard to use a reference to the path to generate the error text since that would tie the lifetime of the error to (likely) the scope it was called from, making bubbling it up harder, so it would necessarily have to copy the value. This increases the size of std::io::Error for cases where errors are not being bubbled up, but I've been mulling this in the back of my head for some time now, and I think the ergonomics outweigh the cost... and I'm also hopeful someone can figure some clever trick around that.

[burdges]

io::File consists of a unix file descriptor, which lack any filename information. io::File cannot be expanded because doing so violates rust's zero cost abstraction goal.

anyhow::Error allows adding context if you like.

Also, io::{Read,Write,Cursor} need io::Error, but should really be made usable no_std and even without alloc, which limits our options for adding anyhow::Error functionality.

[Mark-Simulacrum]

We cannot include this information in a zero cost way, so it would be a price that everyone would need to pay (even if they don't want to). As such, it would not fit well into std; a library providing a wrapper around std which adds relevant context could be viable though. Thanks for the suggestion, though!

If you want to discuss this further, I recommend going to internals.rust-lang.org.

[mqudsi]

Would it be inappropriate even in debug mode only?

[Mark-Simulacrum]

We would need an RFC at minimum to design such a change and I personally am not sure that it's possible given that we only ship one std (which is always compiled in release mode).

(Regardless, this is a sufficiently large request to need an RFC if you want to pursue it further).

[burdges]

Yes, I'd wager most code that benefits from this in debug would benefit in release too, making the debug effort wasted. In C/C++ you must attach such contextual information manually. anyhow::Error improves manual contexts with clean error propagation.

In Linux, you could read file paths from open using /proc/self and fs::read_link
https://stackoverflow.com/a/1189582/667457 It'll returns an incorrect name if they file gets moved, but roughly this should work

fn file_to_path(file: &File) -> Result<PathBuf> {
    use std::os::unix::io::AsRawFd;
    let fd: i32 = file.as_raw_fd().into();
    use fmt::Write;
    let mut path = String:: with_capacity(24);
    path.write_fmt(format_args!("/proc/self/fd/{}", fd)) ?;
    ::std::fs::read_link(path)
}

In principle, you could write an anyhow::io crate that duplicates the std::io traits with such functionality. It's plausible one could define some new architecture linux-with-filename-error-context in https://github.com/rust-lang/rust/tree/master/src/libstd/sys but probably easier doing this through new traits.

[Kixunil]

I object to the idea that adding this is not zero cost. io::Error already contains an enum with several variants that needs to be copied around. File::open and other functions using Path already have to allocate CString for conversions. Adding this allocated string to io::Error is literally two MOV instructions around syscall. Syscalls are much more expensive than two MOV instructions, so if anyone wants to optimize anything, they should optimize number of syscalls, not number of move instructions associated with each syscall.

The benefit of adding Path to io::Error is enormous and should not be dismissed just because it requires two more MOV instructions.

The only issue I see is that paths will not be present in operations after opening the file. However in my experience errors after opening the file are extremely rare and they are pretty much guaranteed to be problems with whole filesystem (full, device unplugged...) in which case the specific file doesn't matter that much anyway.

I'm willing to write the RFC if my arguments convinced you that it's worth pursuing.

[Mark-Simulacrum]

I think I've personally been slowly more convinced over time that this is probably the right thing to do if we can pull it off on benchmarks and such (seems not implausible, given existing allocations etc) -- I think the right next step is to kick off some discussion on internals.rust-lang.org and get a read on how libs team feels about this (#t-libs on Zulip is probably best for that). See also #2979, which tries to codify some of these processes :)

[paulzzy]

Hello! I'm here from r/rust. Has there been more recent discussion/progress on this issue? I'd love to have/help with this feature!

[Kixunil]

@paulzzy not that I'm aware of. I had it in the back of the mind but didn't find the time to write the RFC. If it starts moving, I will try to schedule some time to help. For now, here are my thoughts:

• io::Error contains an enum, add another variant carrying extra data: operation and path
• probably do crater run for changing display impl to print out extra information
• if changing Display is unacceptable, provide some other interface to display it (e.g. using # alternate fmt option)
• do not expose the information directly, or at least not as a reference
• We can reuse the allocation performed by open but on windows we have to convert on-the-fly when displaying
• May need more variants for copy/move; command or other operations, or just box it all - needs benchmarks

My desired error messages:

• File::open(): "failed to open regular file {path} for reading: {reason}"
• File::create(): "failed to create or open regular file {path} for writing: {reason}"
• analogous for append or other open options
• copy: "failed to copy file from {source} to {destination}: failed to open {source|destination}: {reason}"
• for command failing the arguments probably shouldn't be shown by default but opt-in

[eddyb]

There's a relevant PR from @m-ou-se (opened just over a year ago):

• [draft] Store the path in io::Error without extra allocations. rust#86826

[thomcc]

At some point in the past I did https://github.com/thomcc/rust/tree/io-error-path which was the start of an updated version #86826. It was mostly to convince myself this was still possible with the new error repr (it is, but it does require an allocation now), but I ended up coming to the conclusion that it wasn't really worth doing, mostly because of what @m-ou-se said in that issue:

This only shows the path in the Debug implementation of io::Error. It's not clear what else we should do with it. Putting it in Display will make many programs show the path twice, and have less control over the output format. Adding a .path() accessor to io::Error is also not great, because it'll have to be a Option, and won't be accessible through the Error trait.

One thought is that the path could be provided even via the error trait via the provider API (CC @yaahc). Beyond that, I do think the fact that it will end up making many programs show the path twice is unfortunate. I'm not sure that this outweighs the utility of having the path available (certainly that would be nice, and it's tedious to have to truck path around just to be able to display it with the error), but yeah. That and the fact that I had convinced myself it was still possible is why I stopped working on that branch.

FWIW, it is also clear that it's not zero-cost anymore, at least not on 64 bit systems, as it requires an additional allocation (considerably more expensive than the two mov instructions cited above), but it is probably true that this is outweighted by the cost of a syscall. (It actually may require two allocations now, as since writing the code in that branch, we don't always allocate paths that are passed into the methods)

[Kixunil]

@thomcc interesting, I don't have enough time to look into it deeply but in my experience path being displayed twice is less bad than path not being displayed at all. There could be fn no_display_context(&mut self) that'd disable displaying, so if people want to handle it themselves they can. (Maybe there's even a better way.)

[mqudsi]

Displaying the path for {:?} and not {} is probably better than never displaying it, but it might not be enough. (Edit: especially with std::error::Error being so prevalent and implementing Display)

I agree that duplicating the path is problematic. Since first realizing that rust io errors are this useless on their own, I now always map IO errors at the site of creation to a custom error (or just a String in quick apps, i.e. not libraries) with format!("{}: {}", path.display(), e), and I know that I'm not the only one that does that.

Still, it's not the end of the world: if paths were included from day 0, no one would have formatted IO errors that way. If paths get included in Display today, there will be an ugly transition period but people will learn not to include them (easier if this behavior is gated on "edition 2023" or something, which should be legal).

However, there's also the matter of whether or not it's out of the question to use some internal magic to detect whether we are in a top-level fmt() call or in a recursive fmt() call. It should be possible to distinguish (probably in a zero-cost fashion) whether eprintln!("{}", e) (where e is an std::io::Error) was invoked or eprintln!("{}", e) (where e is a different std::error::Error wrapping the std::io::Error) that ends up internally formatting the underlying std::io::Error as part of its fmt() call.

It would be harder (and probably not zero-cost) to determine whether eprintln!("{}: {}", path.display(), e) (with e being std::io::Error) was called, but it's also certainly doable if there's a consensus that this is something that needs to be done one way or the other. Keep in mind that none of needs to be stable or exposed via any public APIs - the only real issue would be that <std::io::Error as std::fmt::Display> would behave differently in different contexts, which might end up being too bitter a pill to swallow.

[thomcc]

I'm going to reopen this since it's getting active discussion and seems plausible we could include it, and @Mark-Simulacrum (the person who closed it in the first place) indicated in an above comment that they've changed their mind somewhat.

I'll note that given the current io::Error implementation, this wouldn't be fully zero cost. That said it probably would be negligible compared to the cost of a system call, and the cost would be entirely in the error case -- the success path would be unpunished. It would be doable by conceptually storing (something similar to) ThinBox<(i32, Path)> as one of the variants, and expanding the pointer tagging to 3 bits (rather the current 2bit tags). This is effectively the approach used in the unfinished patchset linked earlier.

Doing so would incur a single extra allocation, but only in the case that the file operation failed. I'm unconvinced this matters or would show up in benchmarks, but perhaps it's not strictly zero cost.

CC @yaahc since this is related to errors.