Tracking Issue for io_error_downcast

[NobodyXu]

Feature gate: #![feature(io_error_downcast)]

This is a tracking issue for new API std::io::Error::downcast.

Existing APIs requires two separate calls to obtain the raw os error or the inner error and they both return Option.

Thus, users would have to first call Error::get_ref to check that we indeed has an inner error and checked that it is the type we expected.

Users cannot use std::io::Error::into_inner becuase takes the error by value and returns Option<Box<dyn Error + Send + Sync>> instead of Result<Box<dyn Error + Send + Sync>, Self>.

They also cannot workaround this issue by calling Error::raw_os_error, since the io::Error can also be constructed using impl From<ErrorKind> for Error or using other internal methods only accessible by the std library.

This new feature io_error_downcast solved this issue by providing new API std::io::Error::downcast to downcast the inner error easily without unwrap or expect.

Public API

// std::io::Error

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

Steps / History

• API Change Proposal: Add new API try_downcast_inner to std::io::Error libs-team#57
• Implementation: Add new unstable API downcast to std::io::Error #98387
• Update std::io::Error::downcast return type and doc: Update std::io::Error::downcast return type #120117
• Final comment period (FCP)¹
• Stabilization PR Stablise io_error_downcast #124076

Unresolved Questions

• None yet.

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[NobodyXu]

@rust-lang/libs-api I'd like to push for stabilisation of this feature.

The only thing I'm not sure about is the return valueResult<Box<E>, Self>
Returning a Box here might expose too much of an internal detail, in case the internals of io::Error change in future to use something like a thin box.

[NobodyXu]

Stablisation Report

Implementation History

• std::io::Error::downcast implemented in Add new unstable API downcast to std::io::Error #98387

API Summary

A summary of the API is included in the description of this issue.

Experience Report

n/a

[ChrisDenton]

@rustbot label +S-waiting-on-team

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp merge

https://doc.rust-lang.org/nightly/std/io/struct.Error.html#method.downcast

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

Signature resembles these existing stable ones:

impl Box<dyn Any> {
    pub fn downcast<T>(self) -> Result<Box<T>, Self>
    where
        T: Any
}

impl dyn Error {
    pub fn downcast<T>(self: Box<Self>) -> Result<Box<T>, Box<Self>>
    where
        T: Error + 'static
}

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• box resolved by Tracking Issue for io_error_downcast #99262 (comment)
• docs resolved by Tracking Issue for io_error_downcast #99262 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

@rfcbot concern docs

The docs here I think should be beefed up a bit. I see a few things worth improving:

1. The prose in the docs use the word "downgrade" but the method is called "downcast." I expect we want these to be consistent?
2. I think it'd be worth mentioning that this is a convenience routine for some other sequence of methods.
3. The example looks pretty circuitous and incomplete. It shows downcast but doesn't actually show something that can be executed and whose result can be asserted. Can we come up with something better please?
4. The API guarantee stated here seems a little iffy. Is it really true that downcast will always return an error if the io::Error was constructed via any means other than io::Error::new? I'd really prefer if we could state the contract without referring to "you had to have called this particular method for this to work." Instead, can we talk about the data inside of it?

[BurntSushi]

@rfcbot concern box

@NobodyXu mentioned a question above about returning a Box<E> here potentially limiting our choices down the line. Is this true? It looks like there are other APIs on io::Error that return a Box, so I wonder if that ship has already sailed.

Also, AIUI, an std::error::Error can be ?Sized, but the API being proposed here asks for a sized E. Am I understanding that correctly? If so, and if the implicit Sized constraint is intentional, then presumably we don't need to return a Box<E> at all and could just return an E. But I imagine we probably want to support E: ?Sized?

[NobodyXu]

@NobodyXu mentioned a question above about returning a Box here potentially limiting our choices down the line. Is this true?

I was worried about that io::Error could change their implementation later.
It currently uses something like Box<dyn Error + Send + Sync> to store the error, but it could be changed to something else later.

For example, it could change Custom to store ErrorKind and the error in one allocation, though that will require it to manually deal with vtable and unsized allocation, but it is indeed possible.

In that case, returning Box<E> could incur additional overhead, though it could be alleviated by simply resizing the allocation.

It looks like there are other APIs on io::Error that return a Box, so I wonder if that ship has already sailed.

Yes, io::Error::into_inner, the API where io::Error::downcast is modeled after, return a Box<E>.

But TBF, io::Error::into_inner returns Option<Box<dyn Error + Send + Sync>> so it has to use Box.

io::Error::downcast returns a concrete type so it does not have to return a boxed value.

Also, AIUI, an std::error::Error can be ?Sized, but the API being proposed here asks for a sized E. Am I understanding that correctly? If so, and if the implicit Sized constraint is intentional, then presumably we don't need to return a Box at all and could just return an E.

Yes, that's right.

But I imagine we probably want to support E: ?Sized?

Well, I don't think we need support for E: ?Sized.

io::Error::new can take any type that Into<Box<dyn Error + Send + Sync>>, but AFAIK we can only create Box<dyn Error + Send + Sync> from Sized value right now.

In the future where it might be supported, but in practice I rarely seen Error types that are Unsized.

I think it makes more sense to remove the Box than adding E: ?Sized support.

4. The API guarantee stated here seems a little iffy. Is it really true that downcast will always return an error if the io::Error was constructed via any means other than io::Error::new?

Yes.

I'd really prefer if we could state the contract without referring to "you had to have called this particular method for this to work." Instead, can we talk about the data inside of it?

Won't that leak the internal into the API?

There are actually 4 ways of constructing io::Error:

• impl From<io::ErrorKind> for io::Error: Only store io::ErrorKind in io::Error
• io::Error::{from_raw_os_error, last_os_error}: Only store a RawOsError in io::Error
• io::Error::{new, other}: Store io::ErrorKind plus a Box<dyn Error + Send + Sync + 'static>.
• pub(crate) const fn from_static_message(msg: &'static SimpleMessage) -> Error: Stores &'static SimpleMessage:

  #[repr(align(4))]
  #[derive(Debug)]
  pub(crate) struct SimpleMessage {
      kind: ErrorKind,
      message: &'static str,
  }
  

The first 3 are public and can be done by everybody, the last one is pub(crate) and can be only done by std itself.
It's possible for io::Error to add more variants that are internal to std, so not sure this is a great idea.

Also, it's hard to explain that there is a pub(crate) only io::Error::from_static_message, that will only make the reader more confusing.

1. The prose in the docs use the word "downgrade" but the method is called "downcast." I expect we want these to be consistent?
2. The example looks pretty circuitous and incomplete. It shows downcast but doesn't actually show something that can be executed and whose result can be asserted. Can we come up with something better please?

Yes, I can submit a PR to improve its doc once we settle on whether to remove the Box or makes E: ?Sized.

2. I think it'd be worth mentioning that this is a convenience routine for some other sequence of methods.

I don't think this is necessary though.

Putting convenience aside, using io::Error::downcast also removes one unwrap, one heap deallocation and heap allocation if the E isn't the error stores internally:

Thus, users would have to first call Error::get_ref to check that we indeed has an inner error and checked that it is the type we expected.

Users cannot use std::io::Error::into_inner becuase takes the error by value and returns Option<Box<dyn Error + Send + Sync>> instead of Result<Box<dyn Error + Send + Sync>, Self>.

I actually have an example of this from cargo-binstall:

impl From<io::Error> for BinstallError {
    fn from(err: io::Error) -> Self {
        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| BinstallError::Io(io::Error::new(kind, err)))
        } else {
            BinstallError::Io(err)
        }
    }
}

As you can see, it has to call expect (which could be optimized out, but still a hazard if the user is pedantic) and it has to call io::Error::into_inner, which deallocates the Box<Custom> stored internally in io::Error and it calls io::Error::new again if unmatch, which requires Box<Custom> to be allocated again.

I haven't checked the generated assembly, the optimizer might be smart enough to optimize these heap allocation out.

But that isn't a guarantee and crates like cargo-binstall is built using opt-level = "z", so that might not actually happen.

[Amanieu]

@NobodyXu We discussed this in the libs-api meeting. We think that we can move forward with the FCP once the changes proposed by @BurntSushi are addressed:

• downcast should return plain Result<E, Self> instead of boxing the error. While it's true that the error is boxed internally, this may not always be true in the future, and if you're downcasting to a specific type then you usually don't need the box.
• Regarding the docs, I think your response addresses the concerns, so this should be resolved.

[NobodyXu]

Stablisation Report

Implementation History

• std::io::Error::downcast implemented in Add new unstable API downcast to std::io::Error #98387
• Update std::io::Error::downcast return type and doc according to concern from @BurntSushi in Update std::io::Error::downcast return type #120117

API Summary

https://doc.rust-lang.org/nightly/std/io/struct.Error.html#method.downcast

impl std::io::Error {
    pub fn downcast<E>(self) -> Result<E, Self>
    where
        E: Error + Send + Sync + 'static,
}

signature resembles existing stable ones:

impl Box<dyn Any> {
    pub fn downcast<T>(self) -> Result<Box<T>, Self>
    where
        T: Any
}

impl dyn Error {
    pub fn downcast<T>(self: Box<Self>) -> Result<Box<T>, Box<Self>>
    where
        T: Error + 'static
}

Experience Report

n/a

[Amanieu]

@BurntSushi I believe your concerns have now been addressed

[BurntSushi]

@rfcbot resolve box

[BurntSushi]

I think my docs concerns have not been fully addressed here. The docs for this routine seem both too sparse and too odd to me. I'll try to respond to the bits of @NobodyXu's comment above that I think are relevant here.

I'd really prefer if we could state the contract without referring to "you had to have called this particular method for this to work." Instead, can we talk about the data inside of it?

Won't that leak the internal into the API?

No. I'm certainly not asking that the documentation of a type discuss its internal representation. What I'm asking for is a contract that doesn't involve wording like, "you have to have called this method to get this guarantee." Like, I grant there are some situations in which it can make sense to use wording like that, but it's IMO bad juju to start phrasing API contracts in terms of sequences of function calls. It's rather inflexible. And while you enumerated the list of possible constructors today, that isn't necessarily an exhaustive list for all time. Indeed, it's already wrong! (Where "it" is the docs as written today. I see that you mention other in your comment.) Because it looks to me like io::Error::downcast will also attempt downcasting when io::Error::other was used to build the error.

Instead, I'd rather see the phrasing be something like this: "when this contains a custom boxed error, downcasting will be attempted on the boxed error."

I think it'd be worth mentioning that this is a convenience routine for some other sequence of methods.

I don't think this is necessary though.

I'm not really sure how to interpret this response, but the point of saying this is to orient the user's understanding of a routine in terms of another one. That is, it's useful to be able to say, "this doesn't introduce any new expressive power, but is instead a convenience routine for some other thing." Please add this to the docs.

I also still see a couple of other problems with the docs:

1. It says "downgrade" instead of "downcast."
2. The docs should minimally state what "success" means. That is, that E is the same underlying type as the boxed error that io::Error contains.

[NobodyXu]

@BurntSushi I have opened #123851, would like you to have a look

No. I'm certainly not asking that the documentation of a type discuss its internal representation. What I'm asking for is a contract that doesn't involve wording like, "you have to have called this method to get this guarantee."

NOTE that this is actually copied from std::io::Error::into_inner, so you would probably also want to update doc for that method.

[BurntSushi]

Now that #123851 is merged,

@rfcbot resolve docs

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.