embedded_io::ReadExactError

[MabezDev]

Following up from https://github.com/rust-embedded/embedded-hal/pull/505, why don't we remove the ReadExactError type as well, in favour of panicking if read returns 0? Right now we have an asymmetric API, which isn't great. I haven't been able to attend the weekly meetings recently, so sorry if this has already been discussed.

[Dirbaio]

We fixed #505 by documenting write() implementations must never return Ok(0) if buf.len() > 0. This justifies panicking in write_all: it only panics if the impl violates the "must not return Ok(0)" requirement, which is something that truly should never happen.

Reading is a bit different. read() is allowed to return Ok(0) on EOF. It's not an error to EOF, and it's certainly not something that "should never happen", so it doesn't justify panicking.

We could similarly say EOF needs to be signaled by an error instead of Ok(0), then make read_exact panic. I'm not convinced by this though:

• It's a bigger deviation from std::io: Almost no write impl returns Ok(0) and almost no user handles it. It's almost a theoretical thing. However, a lot of read impls can EOF returning Ok(0), and a lot of users handle it. If we were to deviate from std::io in this, many more people would hit it.
• It's debatable that EOF is an error. It's a normal condition for a reader to hit EOF.

[MabezDev]

I agree that we should try to be in line with std as much as possible, and I agree with your points about EoF in general. However, it would still be nice to remove the enum, because this is extra headache for users used to std. Ideally I'd like a situation where a user can match on kind() and determine whether its an Eof/WriteZero error, without actually caring about the type itself.

What about pushing this awkwardness to the implementors instead? I'm thinking about adding a new method to the read trait called:

fn unexpected_eof_error() -> Self::Error;

which would allow the read_exact method to create such an error and return it in the case of read returning Ok(0).

What do you think?

[Dirbaio]

it doesn't allow setting Error=Infallible. Also it'd mean read() could return "unexpected EOF", while onlyread_exact` actually does.

[MabezDev]

it doesn't allow setting Error=Infallible

Is this not the case already since the WriteZero change? For anything that implements Write will never be able to have Error=Infallible (see here because The error type is shared via the ErrorType trait. I suppose this doesn't effect things that implement just Read though.

Also it'd mean read() could return "unexpected EOF", while only read_exact` actually does

True, but that would be a bug in the implementation, and we can document read to say that it should never return an error with the ErrorKind as UnexpectedEof.

[MabezDev]

I'm not pushing super hard for my solution, I would just like to see a symmetrical API. I didn't really see what was so wrong with the original WriteAll enum. Unless I'm missing something I found the original arguments not particularly convincing:

This makes it awkward to write middleware which needs to call write_all(), as each middleware layer then needs to implement their own wrapping error type rather than passing up the error type used by the underlying writer.

Because in the example they gave, they didn't use read_exact, if they did they would also require the bounds there for T::Error: From<ReadExactError<T::Error>>,.

[Dirbaio]

Is this not the case already since the WriteZero change? For anything that implements Write will never be able to have Error=Infallible (see here because The error type is shared via the ErrorType trait. I suppose this doesn't effect things that implement just Read though.

writing to a slice can fail because it can get full. Stuff like embassy_symc::PIpe can't get full, that one can still set Error = Infallible.

Because in the example they gave, they didn't use read_exact, if they did they would also require the bounds there for T::Error: From<ReadExactError>,.

if a read() impl calls read_exact() on the underlying stream, and that fails with UnexpectedEOF, it can propagate it up as Ok(0), indicating EOF. (Or it can propagate an error if that's considered a "protocol error", of course).

In the original write case, which needed calling write_all() from flush(), there was no way to propagate up that "writer full" condition.

[MabezDev]

if a read() impl calls read_exact() on the underlying stream, and that fails with UnexpectedEOF, it can propagate it up as Ok(0), indicating EOF. (Or it can propagate an error if that's considered a "protocol error", of course).

Ah! This was the piece I was missing. That makes complete sense, thanks for bearing with me!