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.

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

proposal: io: add ReadSome #48182

Closed
bradfitz opened this issue Sep 3, 2021 · 7 comments
Closed

proposal: io: add ReadSome #48182

bradfitz opened this issue Sep 3, 2021 · 7 comments

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Sep 3, 2021

The io.Reader.Read method is notoriously tricky, with lots of docs: https://pkg.go.dev/io#Reader

In particular, it's common for people new to Go to write:

var buf [N]byte
_, err := r.Read(buf[:])

... and think they're reading N bytes. Especially when it almost always works. The testing/iotest package is easy to miss (and easier to just not use, even when you're aware of it).

I regularly correct that pattern in code reviews when I see others make that mistake, and despite writing Go for over 11 years, I also made that mistake the other day, causing problems today.

The io package has these helpers that, in addition to doing as documented, also declare the author's intent:

func ReadAll(r Reader) ([]byte, error)
func ReadAtLeast(r Reader, buf []byte, min int) (n int, err error)
func ReadFull(r Reader, buf []byte) (n int, err error)

When the author writes the snippet at top, though, it's hard to know intent. Sometimes it's actually legit to read fewer:

var buf [1500]byte
if n, err := conn.Read(buf[:]); err != nil {
  panic(err)
}
processIncomingUDPPacket(buf[:n])

It'd almost be nice to have a new "helper" in the io package like:

package io

// ReadSome returns r.Read(buf).
//
// It does nothing extra besides declare your intent that you're
// cool with n being < len(buf).
func ReadSome(r io.Reader, buf []byte) (n int, err error) { return r.Read(buf) }

Then all raw io.Reader.Read calls could be replaced with io.ReadFull, io.ReadSome etc, and a raw calls would then stand out as shady, warranting further inspection or annotation.

People who wanted to be really strict could make their static analysis tool(s) of choice even forbid raw Read calls.

/cc @danderson @dsnet

@gopherbot gopherbot added this to the Proposal milestone Sep 3, 2021
@bradfitz
Copy link
Contributor Author

bradfitz commented Sep 3, 2021

Of course, that's just ReadAtLeast(r, buf, 1), really. Except an io.Reader can sometimes return (0, nil), which ReadAtLeast doesn't let you handle. (not that you generally want to)

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

Why can't this be in a third-party library?

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@nightlyone
Copy link
Contributor

What about ReadPartial to emphasize the fact that you can handle partial results in this part of the code?

@martin-sucha
Copy link
Contributor

It seems that a linter that checks whether you use the n return value from Read would catch these cases.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Oct 27, 2021
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@golang golang locked and limited conversation to collaborators Oct 27, 2022
@rsc rsc removed this from Proposals Nov 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants