Add new API try_downcast_inner to std::io::Error

[NobodyXu]

Proposal

Add new API try_downcast_inner to std::io::Error:

impl std::io::Error {
    fn try_downcast_inner<E: Error + Send + Sync + 'static>(self) -> Result<Box<E>, Self>;
}

Problem statement

Existing APIs requires two separate calls to obtain the raw os error or the inner error and they both return Option.
There's no way to avoid unwrap/expect if we want to cover every possible corner case here, e.g., both of them somehow returns None.

This is impossible without expect/unwrap because std::io::Error::into_inner takes the error by value and returns Option<Box<dyn Error + Send + Sync>> instead of Result<Box<dyn Error + Send + Sync>, Self>.

Currently, we would have to do this to downcast the inner error:

(Adapted from cargo-bins/cargo-binstall#180)

#[derive(Debug)]
#[derive(thiserror::Error)]
enum E {
    #[error(transparent)]
    Io(std::io::Error),

    #[error("...")]
    SomeOtherVariant,
}

impl From<std::io::Error> for E {
    fn from(err: std::io::Error) -> E {
        if err.get_ref().is_some() {
            let kind = err.kind();

            let inner = err
                .into_inner()
                .expect("err.get_ref() returns Some, so err.into_inner() should also return Some");

            inner
                .downcast()
                .map(|b| *b)
                .unwrap_or_else(|err| E::Io(std::io::Error::new(kind, err)))
        } else {
            E::Io(err)
        }
    }
}

This has only 1 expect/unwrap, but it uses std::io::Error::new, which uses Box::new internally.

Another way to implement it would be:

impl From<std::io::Error> for E {
    fn from(err: std::io::Error) -> E {
        let is_E = err
            .get_ref()
            .map(|e| e.is::<E>())
            .unwrap_or_default();

        if is_E {
            let inner = err
                .into_inner()
                .expect("err.get_ref() returns Some, so err.into_inner() should also return Some");

            inner
                .downcast()
                .map(|b| *b)
                .expect("e.is::<E>() returns true, so this must succeed")
        } else {
            E::Io(err)
        }
    }
}

This saves the call to std::io::Error::new, but adds another unwrap/expect.

Thus, I propose to add another API to simplify the downcasting while avoiding unwrap/expect, which is std::io::Error::try_downcast_inner.

It returns Result<Box<E>, Self>, and when the downcast fails, it simply return the original std::io::Error.

Motivation, use-cases

This makes downcasting much easier:

#[derive(Debug)]
#[derive(thiserror::Error)]
enum E {
    #[error(transparent)]
    Io(std::io::Error),

    #[error("...")]
    SomeOtherVariant,
}

impl From<std::io::Error> for E {
    fn from(err: std::io::Error) -> E {
        err.try_downcast_inner::<E>()
            .map(|b| *b)
            .unwrap_or_else(E::Io)
    }
}

Solution sketches

The first solution I came to mind with is:

impl std::io::Error {
    fn into_underlying_err(self) -> Cause;
    fn from_underlying_err(cause: Cause) -> Self;
}

#[non_exhaustive]
enum Cause {
    RawOsError(i32),
    InnerError(Box<dyn Error + Send + Sync>),
}

Then I realized that this is too complex and exposes too much details of std::io::Error.

The second solution:

impl std::io::Error {
    fn into_inner2(self) -> Result<Box<dyn Error + Send + Sync>, Self>
}

The problem with this solution is that it creates confusion by introducing a new function with name very similar to existing one.

[yaahc]

This is impossible without expect/unwrap because std::io::Error::into_inner takes the error by value and returns Option<Box<dyn Error + Send + Sync>> instead of Result<Box<dyn Error + Send + Sync>, Self>.

This is incredibly unfortunate and seems like quite a strong justification for this proposed API.

Seconded

@rustbot label +initial-comment-period

My only concern is to make sure that whatever name we pick is as consistent and intuitive as possible.

I'm looking at https://doc.rust-lang.org/stable/std/error/trait.Error.html#method.downcast and can't help but think that this is effectively the same API on io::Error instead of Box<dyn Error>. downcast already implies fallibility, so the try_ feels redundant to me. The _inner seems relevant if we're going to differentiate between cases where it matters if it fails because the inner type is different vs there being no inner type, but if that is the case I feel like a signature of Result<Result<Box<E>, Self>, Self> would be more appropriate, where returning Err on the outer Result indicates there was no inner error and returning Err on the inner Result indicates that there is an inner error but it has a different type.

We can still disambiguate between these two cases with an extra call to get_ref, so I doubt the double Result signature has any value, which all together makes me think that downcast by itself is probably the best name for this method, though I'm curious how other people feel about this.

[NobodyXu]

How about downcast_inner?

but if that is the case I feel like a signature of Result<Result<Box<E>, Self>, Self> would be more appropriate

I think it would be too hard to use and it doesn't really yield any benefit.

[yaahc]

I think it would be too hard to use and it doesn't really yield any benefit.

I fully agree, but I brought that up because I'm questioning the value of the _inner suffix. Why do you think it's important to keep? The only justification I can think of is to be consistent with into_inner but I don't think this represents a middle-ground between downcast and into_inner, this method is just identical to downcast but specific to io::Error.

[NobodyXu]

I fully agree, but I brought that up because I'm questioning the value of the _inner suffix. Why do you think it's important to keep? The only justification I can think of is to be consistent with into_inner but I don't think this represents a middle-ground between downcast and into_inner, this method is just identical to downcast but specific to io::Error.

Because it can only downcast if there is an inner error.
If the io::Error is created using From<ErrorKind>, from_os_error or the SimpleMessage, then it cannot be downcasted.

Although I am ok with downcast as long as people agree with it.

[yaahc]

Because it can only downcast if there is an inner error.
If the io::Error is created using From<ErrorKind>, from_os_error or the SimpleMessage, then it cannot be downcasted.

Yea, but we're not differentiating between failures that are due to mismatched type vs failures due to a lack of an inner error, that was the point of my original comment on the _inner only making sense if we differentiated between those somehow such as with the double Result return type. You indicated that there wasn't any value to that which in my mind means the _inner isn't actually adding anything and just makes this API have a different name from functionally identical APIs on other types.

[NobodyXu]

@yaahc Yeah I think you are right here.

I am OK to change the proposal and update the PR.

BTW, do w need more feedbacks from people before changing the name?

[yaahc]

@yaahc Yeah I think you are right here.

I am OK to change the proposal and update the PR.

BTW, do w need more feedbacks from people before changing the name?

Nope, since this is a nightly only ACP only one libs-api team member is needed to approve or give feedback on the API. Other people can of course give feedback and we will wait the 10 days since I seconded the proposal before approving the PR to give people a chance to participate if they want, but you should be good to update the PR whenever.

[NobodyXu]

Thanks for the explanation, I will update the PR later today.

[NobodyXu]

@yaahc I've updated the PR.