Fix read bug in readByteBuf

[MarcoPolo]

readByteBuf was incorrectly returning the error (EOF) before returning the first byte read. It is a valid for an io.Reader to return both n == 1 and err == EOF (see https://pkg.go.dev/io#Reader). But readByteBuf dropped the read byte and instead returned EOF.

This uses io.ReadFull to handle this logic, where io.ReadFull follows these semantics:

ReadFull reads exactly len(buf) bytes from r into buf. It returns the number of bytes copied and an error if fewer bytes were read. The error is EOF only if no bytes were read. If an EOF happens after reading some but not all the bytes, ReadFull returns ErrUnexpectedEOF. On return, n == len(buf) if and only if err == nil. If r returns an error having read at least len(buf) bytes, the error is dropped.

This also makes sure that the discard helper also follows those semantics. Returning EOF if no bytes were discarded and ErrUnexpectedEOF if only some of the bytes were discarded.

---

Note this started as a continuation of #44, but we realized along the way that most of this code was correct by the semantics above.

This change fixes two things:

1. readyByteBuf bug
2. discard not consistently returning io.EOF or io.ErrUnexpectedEOF

[Stebalien]

LGTM!

[Stebalien]

Note: there is a tiny problem here. Ideally we would return EOF if we read no bytes at all. But that's probably not worth it.

[MarcoPolo]

Okay. Agreed. I’ll make that change later today.

[MarcoPolo]

Updated and added a comment here https://github.com/whyrusleeping/cbor-gen/pull/60/files#diff-189b40dd496fee4378c7839f550f27e38db377a64fbe0881ce48651f0ac809faR64-R68

That explains the general logic of when things return EOF vs. ErrUnexpectedEOF.

[MarcoPolo]

Ideally we would return EOF if we read no bytes at all.

I think this is correct, but then what's the point of ReadExact? io.ReadFull does this:

The error is EOF only if no bytes were read. If an EOF happens after reading some but not all the bytes, ReadFull returns ErrUnexpectedEOF.

[MarcoPolo]

Continuing the thought above, I think most of the old code is correct. EOF happens when we read no bytes and ErrUnexpectedEOF when we read some but not all and ran into EOF.

So I'm going to discard most of the commits and fix only the readByte and readByteBuf, and check that discard follows the same semantics.

[Stebalien]

I think this is correct, but then what's the point of ReadExact? io.ReadFull does this:

Because we were using ReadExact internally. Basically:

1. If I try to decode a CBOR object and read nothing, I should return an EOF.
2. If I try to decode a CBOR object and read some bytes, I should return an UnexpectedEOF.

[MarcoPolo]

yep! I think we're on the same page. My point was that with that change ReadExact becomes ReadFull, or in other words the implementation of ReadExact becomes:

func ReadExact(r io.Reader, buf []byte) error {
	_, err := io.ReadFull(r, buf)
	return err
}

So then a lot of the changes in #44 aren't needed.

[MarcoPolo]

Updated this PR and the description.

I took out most of the changes from #44 because they aren't needed since io.ReadFull already does this.

I fixed the bug in readByteBuf, and I made discard return the correct eof error kind.

For general reference, the comment on io.EOF also summarizes this distinction pretty well:

EOF is the error returned by Read when no more input is available. (Read must return EOF itself, not an error wrapping EOF, because callers will test for EOF using ==.) Functions should return EOF only to signal a graceful end of input. If the EOF occurs unexpectedly in a structured data stream, the appropriate error is either ErrUnexpectedEOF or some other error giving more detail.

[Stebalien]

Strictly speaking, this code is correct. But it doesn't fix the original bug. We need to ensure that UnmarshalCBOR returns:

1. io.EOF if it reads nothing.
2. io.UnexpectedEOF if it reads some data but hits an EOF before it finishes the object.

As far as I can tell, the current code can still return an EOF if, e.g., stop reading a CBOR object on some field boundary.

[MarcoPolo]

Yeah I think that's true. This does fix the other bug in readByteBuf.

@Stebalien / @whyrusleeping could either of you merge this? I don't have that ability

[Stebalien]

Done.