Document the need to handle the underlying error source explicitly

[edmorley]

Hi! Thank you for this crate.

I recently tried migrating one of my projects from fs to fs-err.

Given the README says:

fs-err is a drop-in replacement for std::fs that provides more helpful messages on errors. Extra information includes which operations was attempted and any involved paths.

...and then gives examples like:

failed to open file `does not exist.txt`
    caused by: The system cannot find the file specified. (os error 2)

...I was expecting the error messages to include additional information out of the box, and for their display representation to look like the above.

However, I found that it actually made our project's user-facing error messages worse in many cases (for an example, see heroku/buildpacks-python#270), since the underlying cause is no longer printed, and now only the operation and filename was. Printing the debug representation showed the underlying error existed, but under source.

Searching for the caused by line shown in the README examples, I found it didn't originate in the display representation in fs-err, but instead is a feature of e.g. anyhow:
https://github.com/dtolnay/anyhow/blob/afe93e7b167d069ac79f2c7f363b919d3793e6ce/src/fmt.rs#L30

This was surprising, since anyhow isn't mentioned in the README.

I was going to file a bug/feature request about including source in the top-level error's display representation, however, found #51 which suggests not including it is by design.

I get the reasoning in that issue (so I'm not necessarily suggesting the fs-err behaviour should be changed), however, this means fs-err isn't quite the drop-in replacement it says it is, and at the very least, I think the README should mention that consumers need to adjust their error logging to print the error source too, if they are not using a library that does it for them (like anyhow).

[schneems]

I was bit by this behavior recently as well. I had it on my backlog to file an issue about it, but I see Ed already has. Thanks Ed. I'll add my experiences:

I've been advocating for migrating more projects over to fs_err and didn't actually realize it was eating the original error message. I.e. this code will panic:

let temp = tempfile::tempdir().unwrap();
let path = temp.path().join("doesnotexist.txt");

let result = std::fs::read(&path);
let error_string = match result {
    Ok(_) => panic!("Expected an error"),
    Err(e) => format!("{e}"),
};

let result = fs_err::read(&path);
let fs_error_string = match result {
    Ok(_) => panic!("Expected an error"),
    Err(e) => format!("{e}"),
};

assert!(
    fs_error_string.contains(&error_string),
    "Expected FS error '{fs_error_string}' to contain '{error_string}' but it did not"
);

with

Expected FS error 'failed to open file `/var/folders/yr/yytf3z3n3q336f1tj2b2j0gw0000gn/T/.tmpkzhKG9/doesnotexist.txt`' to contain 'No such file or directory (os error 2)' but it did not

The main motivation for switching to fs_err (for me) is to reduce time spent debugging so we don't have to hunt for which file failed to do what operation. However without the original error text we now have to do additional debugging to determine if it's because that file doesn't exist or if permissions don't allow it etc.

Moving forwards

Adding docs to spell out the behavior and explicit recommendations could be good. We could also introduce some behavior changes behind feature flags.

If we wanted to preserve the existing default the feature could be include_source and adding that flag would add back in the original error message. If we're open to a change in defaults then I would suggest printing it by default and then letting anyhow users discover there's a hide_source feature they could toggle on to not get duplicate information.

I'm biased in that this is my main use case (not using anyhow), I also think if you consider the two failure modes it's better to have duplicate information that someone can look up docs for how to disable than it is to have insufficient information and not realize they're missing anything. I am therefore advocating for:

• Changing the default behavior to show the source
• Adding a hide_source feature flag
• Document the new flag and behavior
• Rev a major version (if we want to be really safe, anyhow users would be more likely to look at the changelog and use the new flag if they desire)

What do you think @andrewhickman? Would you be willing to review a PR for something like that?