Return the custom error instead of its cause in io::Error::{cause,source}

[thomcc]

Fixes #101817

One note about this that I realize now, is that this will result in an error that formats the same being hit twice in the cause chain. So I'm unsure this is actually desirable.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[thomcc]

This needs FCP, and I'm not sure it's actually a good idea.

[jonhoo]

I think this is a "damned if we do damned if we don't" kind of situation. Currently, io::Error has useful Debug output, but is occasionally impossible to downcast through. We could flip that, but the inverse situation is also not great. Or we can have the duplication, which is also not great, but at least it means we never remove useful information.

I'd also like to re-iterate the argument from #101817 that stabilizing #99262 would mean that io::Error::downcast has different semantics from <io::Error as Error>::source followed by a downcast.

[thomcc]

It's also Display, not just Debug, FWIW.

[thomcc]

Since the implementation is pretty simple, and this is mostly a decision about whether or not we should do this (and if we should it needs an FCP) I'm going to reassign to yaahc.

r? @yaahc

[yaahc]

Oh, I saw the original issue but I hadn't seen this PR. I agree with the discussion in the issue that we probably don't want to make this change, and that to access the inner error the io::Error is masquerading as we will need to either use get_ref or potentially some special helper APIs to attempt to transparently pierce through the io::Error while trying to find specific errors in a chain of errors.

I'd recommend experimenting with potential new APIs to make it easier to identify errors in source chains. For a while now I've wanted an equivalent of Error::is that actually iterates through the sources. I'd approach this problem with that as a starting point and think about ways I could modify the API to make it easy to special case io::Errors. I'm not really having any sudden inspirations or anything so the example I came up with isn't really something I'd want to put in std, but I also don't work in codebases that have this problem so I'm not really sure what shape the API needs to take to be most useful to you @jonhoo. Happy to talk more and brainstorm about it if you're interested.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b868bed4e90dbeff16fe7c525e75f251

#![feature(error_iter)]

use std::error::Error;
use std::io;

pub fn are_any<E: Error + 'static>(err: &(dyn Error + 'static)) -> bool {
    err.sources()
       .map(|e| {
            e.downcast_ref::<io::Error>()
                .and_then(|e| e.get_ref().map(|e| e as _))
                .unwrap_or(e)
       })
       .any(|e| e.is::<E>())
}

edit: looking at it more I think the middle map is the one that we could maybe abstract into its own function...

cleaned up version: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=058d3c0b6fb75b990cf05d48166245e0

#![feature(error_iter)]

use std::error::Error;
use std::io;

pub fn are_any<E: Error + 'static>(err: &(dyn Error + 'static)) -> bool {
    err.sources()
       .map(unwrap_if_io_error)
       .any(|e| e.is::<E>())
}

// hopefully obvious this would need a better name
fn unwrap_if_io_error<'a>(e: &'a (dyn Error + 'static)) -> &'a (dyn Error + 'static) {
    e.downcast_ref::<io::Error>()
        .and_then(|e| e.get_ref().map(|e| e as _))
        .unwrap_or(e)
    
}

The unwrap_if_io_error might be something that could go in std. Maybe as a free function associated with io::Error called try_get_dyn_ref. Then you'd just have to remember to write err.sources().map(io::Error::try_get_dyn_ref).any(|e| e.is::<E>()) in order to handle the special case. Then we could potentially add a restricted lint to clippy that people can enable if they have codebases that tend to react to erased errors inside of io::Errors that will help you find all the spots you need to remember to add this map.

[thomcc]

Yeah, I agree this doesn't seem like the right approach.

[jonhoo]

I think what makes it tricky to require the wrapper is that you want it to apply recursively. So if you have

SomeError(io::Error::Custom(SomeOtherError(io::Error::Custom(InnerError)))))))))

You can still get at InnerError from SomeError. That can be handled through a wrapper along the lines of what you propose, though a pub fn source_through_io_error in the standard library feels weird. Though I wonder if perhaps it could be made a part of the semantics for #58520 ?

[yaahc]

though a pub fn source_through_io_error in the standard library feels weird. Though I wonder if perhaps it could be made a part of the semantics for #58520 ?

Hundred percent agree that it feels weird, but I feel like having it be an implicit behavior as part of a more general API feels even more magical and spooky. Maybe in this case it's core enough to the language to justify it, but I can't help but think that io::Error can hardly be the only error type in the ecosystem with this same design pitfall. It's hard to balance whether it's better to be explicit so that you set a precedent for others to follow in similar situations vs implicitly handling common pitfalls to make the default outcomes better while still making it possible to handle similar situations manually.

I feel like if this were in cargo or one of the tools I'd happily choose the second option in the spirit of "make the happy path easy and the edge cases possible", but within the standard library the special casing still feels very spooky to me. That's just me though, it might be worth nominating for discussion in the next libs-api meeting to get more perspectives on how everyone else feels.

I think what makes it tricky to require the wrapper is that you want it to apply recursively. So if you have

I'm confused on this one. It seems like the example you gave would work with the snippet I posted. I think if it were an io::Error::Custom(io::Error::custom(InnerError)) it would break, is that what you meant or did I misunderstand the example?

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=1973bb9ee6347a767136e1eace3e3600

Which can be fixed with...

fn unwrap_if_io_error<'a>(mut e: &'a (dyn Error + 'static)) -> &'a (dyn Error + 'static) {
    while e.is::<io::Error>() {
        e = e.downcast_ref::<io::Error>()
            .and_then(|e| e.get_ref().map(|e| e as _))
            .unwrap();
    }
    e
}

That said, I can't imagine why someone would wrap an io::Error directly with another io::Error so I'm assuming I misunderstood you 😅. If this is a serious problem though I feel like the best place to fix it is probably in get_ref.

[jonhoo]

Heh, yeah, I'm very torn here on whether this is io::Error is weird for historical reasons and we should mask that weirdness to avoid confusion, or io::Error is an example of a common pattern that std should provide tools for working with. I think it's the former, but it's based on little more than a subjective feeling.

I think what makes it tricky to require the wrapper is that you want it to apply recursively. So if you have

I'm confused on this one. It seems like the example you gave would work with the snippet I posted.

Ah, I misremembered and extrapolated from io::Error::source always returning None, which isn't the case: it just skips the immediate error inside io::Error.

I think if it were an io::Error::Custom(io::Error::custom(InnerError)) it would break, is that what you meant or did I misunderstand the example?

Unfortunately, exactly that can pretty easily happen inadvertently through libraries that are generic over I/O. For example, this happens when using hyper on top of rustls: #101817 (comment). So I think the while loop in there is unfortunately necessary.

I also think such a helper should probably include both the io::Error and its inner error in the chain, rather than replace the io::Error with its inner. After all, there are two errors at play — the inner error and its representation as an I/O error. Which is, I suppose, why I feel like io::Error::source should return the immediate inner error — it's a distinct error from type I/O error itself.

[thomcc]

or io::Error is an example of a common pattern that std should provide tools for working with. I think it's the former, but it's based on little more than a subjective feeling.

I know of a few codebases that have replicated similar patterns to io::Error. That said, for those cases it's probably easier to justify changing source/cause.