Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bytes: (*Buffer).UnreadRune at EOF unexpectedly returns an error #19522

Closed
bcmills opened this issue Mar 12, 2017 · 4 comments
Closed

bytes: (*Buffer).UnreadRune at EOF unexpectedly returns an error #19522

bcmills opened this issue Mar 12, 2017 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.9

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 12, 2017

The documentation for (*bytes.Buffer).UnreadRune says:

UnreadRune unreads the last rune returned by ReadRune. If the most recent read or write operation on the buffer was not a ReadRune, UnreadRune returns an error.

From this description alone, one might reasonably expect that after ReadRune returns io.EOF, UnreadRune is a no-op and returns nil.

However, UnreadRune at EOF currently returns an error (https://play.golang.org/p/DyRzp8-W8q):
bytes.Buffer: UnreadRune: previous operation was not ReadRune

The error message is misleading: the previous operation was, in fact, ReadRune — it just happened to return io.EOF.

It is not obvious to me whether this is a bug in UnreadRune, an unclear error message, or unclear documentation.
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 13, 2017
@bradfitz bradfitz added this to the Go1.9 milestone Mar 13, 2017
@bradfitz
Copy link
Contributor

/cc @robpike

@robpike
Copy link
Contributor

robpike commented Mar 13, 2017

Seems like it should be fine to unread a rune at EOF, so I would say "bug". But I am not the one who's going to fix it. @griesemer maybe?

@griesemer griesemer self-assigned this Mar 13, 2017
@ALTree ALTree added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Apr 20, 2017
@griesemer
Copy link
Contributor

I am inclined to say that current behavior is the documented behavior. Note that the documentation says:

UnreadRune unreads the last rune returned by ReadRune.

If ReadRune was called at EOF, it didn't return a rune, so it cannot be unread. The issue is the error message which should be clearer.

UnreadByte behaves the same: Unless the previous Read returned at least one byte, UnreadByte won't work and return an error.

For UnreadRune it would be pretty easy to "fix" so that it can be called at EOF. A solution could be to introduce a new internal readOp with value 0, say opReadRune0, (and use -1, -2 for opInvalid and opRead) and then use opReadRune0 for a ReadRune call that returned no rune. But we also need to do the same for any byte reading operation and then it gets pretty complicated.

It doesn't seem worthwhile adding complexity for a corner case especially given that the current semantics is quite reasonable. It's also reasonable to expect that a well-written program doesn't call UnreadByte or UnreadRune after an unsuccessful read operation.

Fixing this by clarifying the documentation and error message, plus extra tests.

@gopherbot
Copy link

CL https://golang.org/cl/42020 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
6 participants
@bradfitz @ALTree @robpike @bcmills @griesemer @gopherbot