Tidy std::io::Error Usage

[bushrat011899]

Objective

no_std will require independence from std::io::Error. Several usages could be trivially replaced by expanding DecodingError and EncodingError to include more specific error types. Additionally, LzwReader::decode_bytes currently fails if the output buffer is of type OutputBuffer::Vec, it'd be better for that code-path to be implemented.

Solution

• [Breaking] Marked DecodingError and EncodingError as non_exhaustive to accommodate possible future changes in a non-breaking way.
• [Breaking] Added extra variants to both enums to reduce usage of std::io::Error and provide more detailed error information to consumers.
• [Feature] Implemented OutputBuffer::Vec path of LzwReader::decode_bytes using Decoder::into_vec.

[kornelski]

What's your plan for handling io::Write? That's the root source of io::Error.

[bushrat011899]

What's your plan for handling io::Write? That's the root source of io::Error.

#200 includes the overall plan that this PR is an offshoot from. The rough overview is to either use Box<dyn Error> or allow the methods to be generic over the Io error type.

[kornelski]

The changes to use specific enum variants for crate's own problems, like image dimensions too large, are fine. They're currently io::Error only to keep backwards compatibility. This crate could be released as 1.0 in the near future, and adding non_exhaustive error type is a good idea.

However, I don't see how this path can lead to the crate actually being no-std compatible. You can move Vec to alloc, but if you can't get rid of io::Write, you can't use no-std. As far as I see, the other PR sill has std::io::Write.

If you're going to provide a custom re-implementation/polyfill of std::io::Write, why not also provide a custom reimplementation of std::io::Error? That would reduce code changes elsewhere.

I'm concerned about adding performance and code size overheads for std code. For example, for private helper functions that just call io::Write methods, wrapping the result in crate's error type is not beneficial. It only adds code bloat, and doesn't solve the compatibility problem of using io::Write.

Perhaps another option would be to replace io::Write with some other trait for all users, including std users.
It would actually be quite nice to have some trait optimized for appending to a Vec without the extra copying and io::Result that can't fail. But Rust lacks specialisation currently, so I don't know how to nicely replace io::Write while also remaining compatible with io::Write destinations, and would like to see a more specific proposal.

[bushrat011899]

As far as I see, the other PR sill has std::io::Write.

The other PR allows std::io::Write as the recommended user-facing API, but (as you suggest further down), it's just a convenience for a new more specific trait specific to image-gif.

If you're going to provide a custom re-implementation/polyfill of std::io::Write, why not also provide a custom reimplementation of std::io::Error? That would reduce code changes elsewhere.

std::io::Error is actually quite painful to re-implement, even only a subset (I've tried a few times now!). The bigger issue is how you could work with that without making features non-additive and allow conversion to/fro std::io::Error when std is enabled.

I'm concerned about adding performance and code size overheads for std code. For example, for private helper functions that just call io::Write methods, wrapping the result in crate's error type is not beneficial. It only adds code bloat, and doesn't solve the compatibility problem of using io::Write.

Perhaps another option would be to replace io::Write with some other trait for all users, including std users. It would actually be quite nice to have some trait optimized for appending to a Vec without the extra copying and io::Result that can't fail. But Rust lacks specialisation currently, so I don't know how to nicely replace io::Write while also remaining compatible with io::Write destinations, and would like to see a more specific proposal.

So that's actually what I've achieved here. Looking at just io::Write for a moment, the technique I'm using is to define our own Write (which I will refer to as gif::Write for brevity). In #200, only the methods from gif::Write are used, achieving no_std support. To then maintain compatibility with existing io::Write types, we use a type IoWrite<T: io::Write> which implements gif::Write by deferring to T's implementation of io::Write. Since the forwarded methods are marked #[inline], the compiler always has the required visibility to remove the indirection, making the wrapping type and intermediate trait zero-cost abstractions.

What this indirection buys us is the ability for a 3rd party to implement gif::Write for their no_std writer. And since gif::Write::Error is an associated type, they can choose to make it a Never type like Infallible or the future ! type.

struct VecWriter(Vec<u8>);

impl image_gif::Write for VecWriter {
    type Error = core::convert::Infallible;
    
    fn write_all(&mut self, buf: &[u8]) -> Result<(), Self::Error> {
        self.0.extend_from_slice(buf);
        Ok(())
    }
}

Now when working with Encoder<VecWriter>, the EncodingError::Io error could be made unreachable by just making EncodingError generic over the IO error type (not shown in the PR, a newer idea I had more recently). This gives benefits to the std users like you said because in recent versions of Rust, unreachable enum variants can be omitted from match statements.

All of the above equally applied to BufReader. I do think the trait names could be bike-shedded to be less confusing. An alternative I considered was EncoderSink and DecoderSource, and I do like them more the more I think about them.

[kornelski]

Associated error type for gif::Write is quite nice. With an Into<gif::Error> bound it would enable making Io(io::Error) variant optional in gif::Error enum.

Could you implement that? And keep Result<(), W::Error> where Result<(), io::Error> is used currently.

[kornelski]

Just an idea, but maybe there could be:

struct NoStdEncoder<W: gif::Write> {…}
struct StdIoAdapter<W: io::Write> {…}

type Encoder<W> = NoStdEncoder<StdIoAdapter<W>>

this way the Encoder<io::Write> would remain API-compatible, and no-std users would use NoStdEncoder instead.

[197g]

Agreed with @kornelski here, regardless of whether this leads to a no_std use or not the error cleanup itself is the right direction and non_exhaustive makes us more forward compatible. We can do a new major version bump without much of problem. (I'll have a look of whether the decoding loop could be modelled after png so that different readers can be integrated parallel to each other but that's beside this PR).