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/ioutil: move Discard, NopCloser, ReadAll to io #40025

Open
rsc opened this issue Jul 3, 2020 · 12 comments
Open

proposal: io/ioutil: move Discard, NopCloser, ReadAll to io #40025

rsc opened this issue Jul 3, 2020 · 12 comments
Assignees
Projects
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jul 3, 2020

io/ioutil exists mainly to avoid import cycles: it can make use of packages that themselves depend on io.

The practical effect of this is that io/ioutil is mainly about convenient OS file access: ReadDir, ReadFile, TempDir, TempFile, and WriteFile. These are here because os was too low level, and io cannot import os. (For more about why io should not depend on os, see “Codebase Refactoring (with help from Go)”, especially section 3.)

The three functions that don't quite fit this bill are Discard, NopCloser, and ReadAll. These are general, useful I/O helpers without dependencies, at least conceptually. They don't need to be in io/ioutil.

It is confusing that nearly all reader and writer adapters—like LimitReader, MultiReader, MultiWriter, TeeReader, SectionReader—are in io, while NopCloser and Discard hide in io/ioutil. They should join the others. I think it was mostly accidental that they ended up in io/ioutil.

It is similarly confusing that the reader and writer helpers—Copy, CopyBuffer, CopyN, ReadAtLeast, ReadFull, WriteString—are all in io, while ReadAll alone hides in io/ioutil. It too should join the others.

In the case of ReadAll, there is a clearer reason why it was relegated to io/ioutil: it imports bytes for access to bytes.Buffer, and bytes imports io. But ReadAll need not use bytes.Buffer, especially now that we have append built in.

Obviously we cannot delete these three from io/ioutil. But we can move the implementations to io and leave wrappers behind. That will be easier for new users, and it lets more packages do general I/O without a dependency on os.

Edit: To be clear, no existing code will break. The old symbols will remain behind, as wrappers of the ones in io.

@rsc rsc added the Proposal label Jul 3, 2020
@rsc rsc added this to the Proposal milestone Jul 3, 2020
@josharian
Copy link
Contributor

@josharian josharian commented Jul 3, 2020

And if we moved the remaining functions to a new package, io/fileio, we could deprecate io/ioutil entirely.

But should this happen as part of a stdlib v2? It is backwards compatible, but at the cost of a fair amount of duplication.

@rsc
Copy link
Contributor Author

@rsc rsc commented Jul 5, 2020

The fileio suggestion is #19660 for what it's worth.

@rsc
Copy link
Contributor Author

@rsc rsc commented Jul 5, 2020

For what it's worth, I think we can correct these three symbols without waiting for a more comprehensive stdlib v2.
They clearly belong in io, by almost any criteria. (And we're not likely to start chopping up io or anything like that.)

The remaining symbols I think we can deal with at another time. It's possible they just belong in os, instead of isolated in a side package all by themselves, except that there's already an os.TempDir.

@rsc rsc added this to Incoming in Proposals Jul 8, 2020
@rsc rsc moved this from Incoming to Active in Proposals Jul 8, 2020
@rsc
Copy link
Contributor Author

@rsc rsc commented Jul 15, 2020

The reactions I see above (one comment, bunch of emoji) are all positive. Does anyone object to accepting this proposal?

@magical
Copy link
Contributor

@magical magical commented Jul 17, 2020

No objection from me. I can never remember what's in ioutil vs io.

@rsc
Copy link
Contributor Author

@rsc rsc commented Jul 22, 2020

Based on the discussion above, this seems like a likely accept.

(Part of the reason to do this is that then you never have to use io/ioutil when working with a non-operating-system fs.FS. io/ioutil becomes OS-only.)

@rsc rsc moved this from Active to Likely Accept in Proposals Jul 22, 2020
@beoran
Copy link

@beoran beoran commented Jul 22, 2020

Sorry to be the naysayer here, but how should we deal with the break in backwards compatibility this move causes? The go statement in the go.mod file and go fix or go fmt or such? It seems a lot of work for what is in essence a cosmetic change.

Once we have generics, we will have to rethink the whole standard library anyway. I prefer all breaking changes like this one to be made at once for the V2 standard library, then I only will have to upgrade once and not a dozen times.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jul 22, 2020

My assumption was that we would move the base symbols to io, then type alias them into io/ioutil. We can deprecated the aliases too if that is not excessive.

package io

var Discard io.Writer = devNull(0) // maybe we could make this a const?
func NopCloser(r io.Reader) io.ReadCloser  { /*...*/ }
func ReadAll(r io.Reader) ([]byte, error) { /*...*/ }
package ioutil

var Discard io.Writer = io.Discard
func NopCloser(r io.Reader) io.ReadCloser  { return io.NopCloser(r) }
func ReadAll(r io.Reader) ([]byte, error) { return io.ReadAll(r) }
@magical
Copy link
Contributor

@magical magical commented Jul 22, 2020

From the proposal:

Obviously we cannot delete these three from io/ioutil. But we can move the implementations to io and leave wrappers behind.

@rsc
Copy link
Contributor Author

@rsc rsc commented Jul 29, 2020

I've clarified in the text above that nothing will break.

Other than that comment, seems like there is no change in consensus. Accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jul 29, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 29, 2020

Change https://golang.org/cl/245657 mentions this issue: io: move Discard, NopCloser from ioutil

@rsc rsc modified the milestones: Proposal, Backlog Jul 30, 2020
@rsc rsc self-assigned this Jul 30, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 30, 2020

Change https://golang.org/cl/245658 mentions this issue: io: reimplement and move ReadAll from ioutil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.