Error::source and Error::cause do not expose immediate error in custom io::Error

[jonhoo]

I tried this code (playground):

use std::error::Error;

#[derive(Debug, PartialEq, Eq)]
struct E;

impl std::fmt::Display for E {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "E")
    }
}

impl Error for E {}

#[test]
fn custom_io_has_source() {
    let e = E;
    let e = std::io::Error::new(std::io::ErrorKind::Other, e);
    assert_eq!(e.source().and_then(|s| s.downcast_ref::<E>()), Some(&E));
}

I expected to see this happen: the test passed

Instead, this happened: the test failed with

thread 'custom_io_has_source' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(E)`', src/lib.rs:18:5

I don't believe this is intentional, since the code standard library impl Error for io::Error specifically has:

rust/library/std/src/io/error.rs

Line 963 in a926696

ErrorData::Custom(c) => c.error.source(),

In fact, it was specifically implemented in #58963, but that PR doesn't appear to have included any tests. The test above fails even on 1.30.0 (which is the first Rust version with fn source).

What's even more interesting is that assert!(e.cause().is_some()) fails all the way back to Rust 1.0, so it sure seems like something is fishy here.

I wonder if @thomcc may know how the source might be disappearing given his work in #87869?

Another interesting tidbit here is that using io_error_downcast we do get the inner error, meaning there will (when that feature lands) be a discrepancy between io::Error's .source() and .downcast().

assert_eq!(e.downcast::<E>().unwrap(), Box::new(E));

Meta

rustc --version --verbose:

rustc 1.63.0 (4b91a6ea7 2022-08-08)
binary: rustc
commit-hash: 4b91a6ea7258a947e59c6522cd5898e7c0a6a88f
commit-date: 2022-08-08
host: aarch64-unknown-linux-gnu
release: 1.63.0
LLVM version: 14.0.5

[thomcc]

This happens because in https://github.com/rust-lang/rust/blob/master/library/std/src/io/error.rs#L954 we return e.error.cause(), the cause of the underlying io::Error.

I agree this is kind of confusing, and probably semantically wrong.

[thomcc]

I'm in favor of changing it in principal, but it has the chance to break a lot of people's code, and only in an error-handling path which may not have good test coverage (although, perhaps the code would be made to work, since this is the way they assumed it would behave...)

[jonhoo]

I thought that for a second too, but I don't think that's right either, since I also get None with:

use std::error::Error;

#[derive(Debug, PartialEq, Eq)]
struct E;

static E2: E = E;

impl std::fmt::Display for E {
    fn fmt<'a>(&self, f: &mut std::fmt::Formatter<'a>) -> std::fmt::Result {
        write!(f, "E")
    }
}

impl Error for E {
    fn description(&self) -> &'static str {
        "E"
    }

    fn cause(&self) -> Option<&Error> {
        Some(&E2)
    }
}

#[test]
fn custom_io_has_source() {
    let e = E;
    let e = std::io::Error::new(std::io::ErrorKind::Other, e);
    assert!(e.cause().is_some());
}

[jonhoo]

Ah, wait, no, never mind, I was still testing with Rust 1.0! Yes, on newer Rust versions I do get the return value from the inner error's cause/source.

[thomcc]

Fixing this has the issue that it means displaying every error in the cause chain will make it appear that this error is hit twice. That seems undesirable to me.

[jonhoo]

Because io::Error's Debug output for custom errors just forwards to the Debug of the inner error, right? Yeah, I think @yaahc (and others) have made the argument elsewhere that error types should only ever do one or the other (print or return via source), never both, and we'd violate that by making that change. And I suppose here the argument is that we want printing io::Error on custom errors to yield some useful information beyond just printing "Custom" because it's such a common case?

That said, I think it's quite important to retain the full error chain through io::Error — not doing so makes it impossible to check for certain error cases except by doing string matching on Debug output. As a concrete example, I'm building a credentials provider for the AWS SDK, and it turns out that on certain macOS hosts the OS-level certificate store is set up incorrectly, causing an UnknownIssuer error. When that happens, I'd like to print a message telling the user what the likely problem is and how they can fix it. However, the error propagates up from TLS, which means it surfaces via the I/O traits. This in turn means the error type is forced to go through io::Error, so by the time it makes it into hyper::Error all I'm left with is a hyper::Error. That error I can then only deconstruct to io::Error, but no further (since webpki::Error doesn't have source), and thus I have to do a stringly check on Debug instead.

[thomcc]

That error I can then only deconstruct to io::Error, but no further

You should be able to use io::Error::get_ref (or get_mut, or into_inner, depending on your needs) to access the inner error in a way unrelated to the cause chain, FWIW.

[jonhoo]

True, though that won't work with anything that expects to be able to walk the error chain solely through source, such as anyhow's Error::chain.

[jonhoo]

I just realized the rustls situation I ran into is even worse because multiple layers in the hyper TLS stack end up converting their errors through io::Error, so you actually end up with:

Some(hyper::Error(Connect, Custom { kind: Other, error: Custom { kind: InvalidData, error: WebPKIError(UnknownIssuer) } }))

Note the nested Custom here, so I'd actually have to walk the chain using source unless the error is io:Error in which case I have to use get_ref instead.

[thomcc]

Yeah, I agree it seems weird and isn't ideal. I'm not sure there's an ideal fix though -- it's quite common IME for people to print out the Displayed causes for error reporting, and this will make that have a notably worse report.

But yeah, the alternative is that it's very tricky and subtle to walk the error chain looking for a specific error.

[jonhoo]

I suppose it might be possible to change anyhow (and other crates like it) to "know" about this oddity about io::Error and implement some special logic in the chain walking around here:
https://github.com/dtolnay/anyhow/blob/94f6f4df66b48c0082a3ebd6cf6b28097a69558c/src/chain.rs#L42

But it feels unfortunate to have this foot-gun here for anything that wants to implement error walking to work around.

[yaahc]

vendoring this here for visibility, I replied to the PR that was opened associated with this issue: #101818 (comment)