WIP: use trait object for io::Error representation

[Plecra]

This is a little experiment in the hopes that io::Error could box some user types without allocating.

I want to do a perf run on these changes, but there are a couple things to fix up first:

• I'm using i32::MIN as a sentinel value for os errors at the moment, which is a bit of a shame. This shouldn't ever be returned from libc or windows APIs, but there was nothing stopping users from passing it to the API before.
  • On 64 bit targets, we can simply store a flag in the higher order bytes
  • For 32 bit, we could technically store the data as a (ptr::NonNull<VTable>, *mut ()) to allow zero.
    • Checking this condition manually may have performance problems
• Will those as_error_* calls be much of a performance hit?
• Can we repr(u8) ErrorKind without a stability problem?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

I doubt that a perf run would be particularly revealing - it both is unlikely to see many errors, but also doesn't really use I/O all that much.

I'm not sure how to best evaluate these changes, and unfortunately don't have a lot of time myself to investigate here - let me try switching to r? @KodrAus, maybe Ashley has some more time :)

[KodrAus]

Thanks for the PR @Plecra! So the thing you're trying to avoid here is the double box needed to stash a custom error type? Have you looked at how anyhow does this? It uses a static vtable that's boxed alongside the concrete value itself to avoid the double boxing. The vtable is responsible for accessing and dropping the inner value itself.

I think in a scheme like that we'd break the Repr enum variants out into their own structs and provide vtables for them.

[Plecra]

I'm actually more interested in avoiding allocations altogether (right now, Custom is itself boxed) - If the error type always holds a pointer to the VTable, we could create an API that constructed user errors with effectively zero runtime cost:

fn main() -> io::Result<()> {
    if bad {
        // creates a single use type that implements `IoError` with these values
        // As a ZST, no allocations have to be made to return it.
        return io::err!(Other; "*crickets*");
        // Naturally, there are many variations to this - any `repr(int)` enum could be packed
        // into the data half of the pointer. One could declare a `NotFound` type that
        // held a `&'static ThinStr` and always returned `ErrorKind::NotFound`, etc...
    }
    Ok(())
}

We'd need a new API to do this, something similar to fn(impl error::Error + Clone + Into<ErrorKind>) -> io::Error (without Clone), and I just wanted to check the representation wouldn't have too many drawbacks.

anyhow's design wouldn't help much there, since it always allocates (to make the stack value as small as posisble). anyhow actually allocates the VTable statically and holds a reference to it, the same way trait objects work, so I don't think we'd gain much from manually constructing the VTable in the same way. What would this approach give us?

[JohnCSimon]

@Plecra ping from triage: can you please fix the merge conflict?
@rustbot label: -S-waiting-on-review +S-waiting-on-author

[Plecra]

Can I create a ZST Box in a const function? Fixing this merge is a bit troublesome if I can't...

[Plecra]

Oh... not sure how I managed to do that. Can anybody reopen this?

[crlf0710]

@Plecra Ping from triage, what's the current status of this?

[Plecra]

An actual implementation of this is waiting on a few questions around the state of io::Error and error::Error. If they are moving to core, we'll need to use a slightly different representation.

I'm tempted to simply close this and revisit it when the position of io::Error is clearer. The current changes are only a means to an end: they'd enable io::Error::new_no_alloc() and moving io::Error into core. Maybe it'd be worth jotting a reminder down somewhere visible?

[bors]

☔ The latest upstream changes (presumably #84310) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

triage:

I'm tempted to simply close this and revisit it when the position of io::Error is clearer

@Plecra I'm closing this for as inactive, please feel free to reopen when things get cleared up.
Thank you!