Add filename information to `std::io::Error` to improve `std::io` error messages

[henryboisdequin]

🖼️ Rendered

[tesuji]

How do I opt out of the filename?

~~This also makes io::Error harder/impossible to move it to libcore.~~ Sorry, I mistaken it with error::Error trait.

Did the author open a conversation on internals forum yet? Because I think the RFC process prefer conversation on internals before opening the RFC in this repository.

[henryboisdequin]

How do I opt out of the filename? This also makes io::Error harder/impossible to move it to libcore.

Did the author open a conversation on internals forum yet? Because I think the RFC process prefer conversation on internals before opening the RFC in this repository.

@tesuji I felt like it wasn't necessary to open up another forum as there was a previous discussion about this topic here. Also, after the discussion, issue rust-lang/rust#44938 was opened. I'm addressing this comment from that issue. As there were pros and cons discussed on either side, I wanted to follow up with an RFC (as the comment states) and see where that takes this idea.

To address your first comment, I would prefer to open up an initial implementation PR (I'll try to tackle this weekend) and see how this affects std's performance as explained in the RFC. Based on that, we can move forward.

[liigo]

the rustc like compiler error message confused me. io::Error is run-time error, not compile-time.

[ogoffart]

I think having the filename in the error message would indeed makes it more user friendly.

In prior work, there is also the fs-err crate: https://crates.io/crates/fs-err

Note that this RFC use compilation-error kind of message with reference to the source. But referring to the source may be challenging. To get similar error to what's described, one would need to have #[track_caller] feature that tracks the span of the argument. Also, one does not so often call these functions with a string literal, often it is some kind of arbitrary expression, and then highlighting that may not be so useful. Since these error are most likely being seen by users of the application and not the developer, a link to the source code location doesn't actually make so much sense, so it might be better to leave that out.

[henryboisdequin]

I think having the filename in the error message would indeed makes it more user friendly.

In prior work, there is also the fs-err crate: https://crates.io/crates/fs-err

Note that this RFC use compilation-error kind of message with reference to the source. But referring to the source may be challenging. To get similar error to what's described, one would need to have #[track_caller] feature that tracks the span of the argument. Also, one does not so often call these functions with a string literal, often it is some kind of arbitrary expression, and then highlighting that may not be so useful. Since these error are most likely being seen by users of the application and not the developer, a link to the source code location doesn't actually make so much sense, so it might be better to leave that out.

@ogoffart Do you think it would be better if std::io::Error had a field of help_msgs that contained some useful messages on how to fix the problem?

[matklad]

There's an important note about performance. Because Path is not represented as CString / OsString, every operation which can fail with path-related error already has to allocate on the happy path. So the perf impact here is not "no allocs -> 2 allocs on a sad path", but rather "1 alloc -> 3 allocs on a sad path".

[programmerjake]

There's an important note about performance. Because Path is not represented as CString / OsString, every operation which can fail with path-related error already has to allocate on the happy path. So the perf impact here is not "no allocs -> 2 allocs on a sad path", but rather "1 alloc -> 3 allocs on a sad path".

That could be reduced by allocating all the strings' backing storage into a single allocation, which would be an allowed change afaict. It would make the internals more complicated, but I think that's an acceptable tradeoff.

[ssokolow]

I'm not a fan of the proposed rustc-style error messages, but I would like having the path as either part of the message or a new field in the struct. It would simplify how I handle errors to be displayed to the user in CLI tools I write.

[henryboisdequin]

@ssokolow, @ogoffart, @liigo, I've changed the RFC such that the error message style stays the same (no rustc like compile time error messages)

[ssokolow]

Definitely better... though I'm not sure the help key is a good idea. Might be better to give it a separate RFC of its own if this one gets the go-ahead.

These Debug outputs are more likely to be exposed to non-programmers than rustc help is and "Create a file in..." could lead them down the wrong route if it's a typo in a path provided via a CLI argument parser or what they actually need to do is move/copy an existing file into place.

At the very least, you'd want to change that to something like "Check the path for typos and then make sure the file /path/to/whatever.txt exists". (And even then, if it's a failure at an earlier stage in the pipeline, they may not be responsible for ensuring such a file gets created.)

[henryboisdequin]

@ssokolow Agreed, I think help is another idea on its own. If this gets accepted then I will look at help (going to make the change).

[oleid]

I was wondering : the examples in the RFC are all about files. What about socket connections which fail? How would those be rendered if this RFC passes?

[henryboisdequin]

I was wondering : the examples in the RFC are all about files. What about socket connections which fail? How would those be rendered if this RFC passes?

@oleid I will be posting a code and error example for each of the variants in std::io::ErrorKind soon.

[joshtriplett]

I wonder if build-std could one day help with this kind of thing: the top-level crate could use feature flags on std to make the trade-off between more helpful error messages and the overhead of failure cases. (There are occasional applications where the performance of the failure case is important, such as when the application expects to have primarily failures to open things, and failing to open is the common case.)

[axelkar]

What does the Rust community think about this pattern of giving context (like anyhow::Context::context inside functions compared to outside?

It's annoying to wrap functions like this (or use immediately-called closures), but it means that the caller doesn't have to write potentially repetitive boilerplate for it:

pub fn example(text: &str) -> Result<(), anyhow::Error> {
    example_inner(text).context_with(|| format!("Failed to run example with {text}"))
}
fn example_inner(text: &str) -> Result<(), anyhow::Error> {
    // do things
}

or

#[derive(thiserror::Error)]
enum Error {
    #[error("I/O error")]
    Io(#[from] ...),
    #[error("Database error")]
    Database(#[from] ...),
    #[error("Failed to run example with {text}")]
    Example {
        inner: #[source] Box<Error>,
        text: String
    }
}
pub fn example(text: &str) -> Result<(), Error> {
    example_inner(text).map_err(|err| Error::Example {
        inner: err,
        text: text.to_owned()
    })
}
fn example_inner(text: &str) -> Result<(), Error> {
    // do things
}

I believe the "expose_original_error pattern" is also something to discuss. Currently core::error::Error::source is ignored by the Termination trait.

[shepmaster]

What does the Rust community think about this pattern of giving context inside functions compared to outside?

My default practice (and generally what I suggest for SNAFU) is that there's no reason to add context that was passed in or is otherwise known by the caller of the function.

// Yes, add the path as context here because no one else knows the path
fn example1() -> Result<(), Error> {
    let path = generate_it_somehow();
    fs::read_to_string(&path).context(ReadSnafu { path })
}

// No, do not add the path as context here;
// the caller is capable of adding it *if they need it*
fn example2(path: &Path) -> Result<(), Error> {
    fs::read_to_string(&path).context(ReadSnafu)
}

---

This is a big reason why I'm generally against this RFC. By baking in the file name, you force that extra load on everyone, regardless if they need it or not.

[ssokolow]

From my perspective, most of the time, why the I/O failed is self-evident if you know what path it failed on, but the error message without the path is useless... so requiring the user to .context() to incorporate filenames into error messages turns Rust's elegant ? into Go-esque boilerplate as anyhow's .context() (or an even more manual equivalent) becomes mandatory in tons of places to have useful error messages.

Likewise, my perspective is also that a well-designed error message should not obscure information known at the site of its emission. The current state of things feels more like the programmer version of Windows's "I'm not going to tell you what went wrong. Go bother your administrator. Oh, you're not in a Fortune 500 with a co-worker who paid for our administrator training to learn how to get more info? Sucks to be you."-style error messages.

(But then I'm the guy who ported gtkexcepthook to PyQt for my GUIs so I can get an augmented stack trace which includes the contents of all locals in each stack frame.)

The only reason I don't use fs-err in every single project which calls std::fs functions is that I consider the auditing burden for a third-party crate higher than the boilerplate burden of saying .actually_say_something_useful(...) over and over again.

[axelkar]

This reminds me of #[tracing::instrument], which is actually supported by color-eyre. Edit: For the unknowing, it can add function parameters as context.

I do agree that with fs the user is supposed to see the path. Why bother printing an I/O error when it's not telling the user where it came from? Imagine an obscure "Access denied" popup on Windows.

This is partly remediated by error type boundaries and From+Display impls like from thiserror, which add some context automatically. This is a fairly popular pattern to my belief. Compare:

fn database() -> Result<(), Box<dyn ...>> {
    fs().context("I/O error")?;
}
fn my_code() -> Result<(), Box<dyn ...>> {
    database().context("Database error")?;
}

// vs getting the context automatically

enum DatabaseError {
    #[error("I/O error")]
    Io(#[from] std::io::Error)
}

enum MyCodeError {
    #[error("Database operation failed")]
    Database(#[from] DatabaseError)
}

fn database() -> Result<(), DatabaseError> {
    fs()?;
}
fn my_code() -> Result<(), MyCodeError> {
    database()?;
}

// vs error messages in the callee's error type. more compatible with e.g. DivisionByZeroError or std::io::Error which print what they are

#[error("Database error")]
enum DatabaseError {
    Io(#[from] std::io::Error)
}

// this message is debatable/superfluous in a binary crate
#[error("My code failed!")
enum MyCodeError {
    Database(#[from] DatabaseError)
}

fn database() -> Result<(), DatabaseError> {
    fs()?;
}
fn my_code() -> Result<(), MyCodeError> {
    database()?;
}