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: os: File should be an interface #14106

Open
robpike opened this issue Jan 26, 2016 · 18 comments
Open

proposal: os: File should be an interface #14106

robpike opened this issue Jan 26, 2016 · 18 comments
Labels
Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. Proposal v2 A language change or incompatible library change
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented Jan 26, 2016

By analogy with #13473, os.File should also be an interface. That would permit a program to use files in the abstract sense, including implementations not provided by the os package.

@robpike robpike added the v2 A language change or incompatible library change label Jan 26, 2016
@bradfitz
Copy link
Contributor

Related: #5636 (VFS layer), in which we were never able to find a design that fit everybody's needs.

@mattn
Copy link
Member

mattn commented Jan 27, 2016

https://godoc.org/golang.org/x/net/webdav#FileSystem

This may be one of implementations.

@rsc rsc added this to the Unplanned milestone Jan 27, 2016
@rsc
Copy link
Contributor

rsc commented Jan 27, 2016

I think I'd argue that io.File should be the interface but it's all moot now.

@bronze1man
Copy link
Contributor

rsc How about follow interface?
I think following interface should work in go1.x

type FileV2 interface{
   io.ReadWriteCloser
   //....
}
func NewFileFromV2(v2 FileV2) *File{
  //...
}

@minux
Copy link
Member

minux commented Feb 17, 2016 via email

@rsc
Copy link
Contributor

rsc commented Feb 17, 2016

This is a long-term issue that we are not actively working on.

@rsc rsc changed the title os: File should be an interface proposal: os: File should be an interface Jun 17, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Jan 9, 2018
@ianlancetaylor ianlancetaylor added the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Jan 9, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 10, 2018

After several years of maintaining various Google filesystem implementations, I disagree strenuously with this proposal.

Go interfaces act as bounds on types. On function inputs, they are lower bounds: every implementation must provide at least the methods described in the interface. On function outputs, they are upper bounds: even if the implementation provides methods beyond those in the interface, they are not visible in the compile-time type system, and they are therefore also less visible in the documentation.

Because they act as bounds, interfaces work best on the consumer side of an API: for example, io.Reader belongs in the package that defines io.Copy, because Copy consumes the output of a passed‐in Reader. On the other hand, interfaces do not belong on the implementation side of the API: bytes.NewReader returns a *bytes.Reader, not an io.Reader, because a *bytes.Reader provides methods that are not required of every io.Reader.

Turning that observation back toward the os package, we see that the os package is clearly on the implementation side of os.File: there are several functions in the package that return a *File, but zero that accept one as an argument other than a method receiver (#13473 notwithstanding).

If we were to make os.File an interface, that would make every further addition to the os.File API either a breaking change (because existing implementations would no longer match the new method) or a second-class, under-documented feature (because the new method would not be in the method set of the exported interface). That is a high cost.

In exchange for that cost, we would get very little benefit. It is already possible today for any package to define and use an interface with an arbitrary subset of the methods provided by *os.File, and so it is already possible for programs to use “files in the abstract sense”, independent of the os package but including *os.File.

@aviau
Copy link

aviau commented May 3, 2018

interfaces work best on the consumer side of an API

Would you agree with adding symbols to ioutil so that we can pass it filesystem implementations?

Whoever writes a file system implementation is now forced to duplicate ioutil functions.

@DeedleFake
Copy link

DeedleFake commented May 4, 2018

An alternative would be to split the implementation. Leave os.File in place, but also introduce an io.File interface that covers the methods generally associated with files. For example:

type File interface {
  Reader
  Writer
  Seeker
  ReaderAt
  WriterAt
  Syncer // Doesn't exist, but you never know. Maybe `Flusher` instead?
  Close
}

This is analogous to the difference between net.Conn and the various specific connection types. Go 2 could then decide what direct usages of *os.Fileto replace with io.File.

In this case, an extra io.Directory interface covering directory-specific os.File operations, such as Readdir(), might make sense as well, although that might be complicated by the reliance on os.FileInfo.

It is already possible today for any package to define and use an interface with an arbitrary subset of the methods provided by *os.File

This is very true, but I think there's some merit to having a standard of some kind. The io package provides a number of types that are not used by itself, but they provide a standard set of methods and types that everyone recognizes and uses. Personally, I think that the existence and ubiquitousness of io.Reader and io.Writer are one of the best things about Go.

Although, there is technically http.File as well.

@bcmills
Copy link
Contributor

bcmills commented May 8, 2018

@aviau

Would you agree with adding symbols to ioutil so that we can pass it filesystem implementations?

Yes to adding symbols, no to ioutil. (I think ioutil should be specific to OS files; see #19660. For specific alternatives, see #13473 (comment).)

@bradfitz
Copy link
Contributor

bradfitz commented Nov 13, 2018

*os.File currently has 20 methods. I don't think we want to introduce an interface with 20 or even 15 or 10 methods.

And we don't want to go overkill on optional interfaces because experience with http.ResponseWriter and its optional interfaces show that wrappers too often hide methods that the wrapped value does implement. Or you have to have wrappers implement the world and define each optional method to have a "Oh, nevermind, carry on as if I didn't implement this" return value.

Also, changing *os.File to be an interface would be pretty infectious, for better or worse.

Counterproposal

If one of the primary motivations for this is to implement VFS packages that return files, the real problem is there's no way to instantiate arbitrary *os.File values that aren't backed by the filesystem.

We could instead add constructor func(s) to return *os.File values given an interface value. For example,

// NewReaderAtFile returns an *os.File given the provided stat information and ReaderAt.
// The file is assumed to be a regular file with size fi.Size(), backed by the data in contents.
func NewReaderAtFile(fi FileInfo, contents io.ReaderAt) *os.File

You could imagine a few of those for various types (regular files, symlinks, directories).

For misc methods like File.Chdir or File.Truncate, you could return an error by default, or document that the content interface value is checked for matching names & signatures. That might avoid the composition problem with ResponseWriters because it wouldn't be interfaces all the way down and people would pass around an *os.File with a concrete set of methods that could grow over time.

@bradfitz
Copy link
Contributor

bradfitz commented Nov 13, 2018

I addition, this counterproposal would have a nice solution to #13473 too. Instead of that bug's:

os.Stdout = bufio.NewWriter(os.Stdout)
os.Stderr = bufio.NewWriter(os.Stderr)

You'd instead write something like:

os.Stdout = os.NewWritableFile(bufio.NewWriter(os.Stdout))
os.Stderr = os.NewWritableFile(bufio.NewWriter(os.Stderr))

So the type of os.Stdout and os.Stderr would remain *os.File, but it'd be easier to create custom implementations of *os.File.

@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2018

@bradfitz, what would the *os.File returned by NewReaderAtFile do for things like the Fd method, that really do rely on having something that the OS recognizes as a file?

@bradfitz
Copy link
Contributor

Either return -1 by default (in lieu of an error return value there) or delegate to the wrapped interface as described above.

@bradfitz
Copy link
Contributor

I note that we went this route a bit with the *net.Resolver type. It's a concrete type instead of an interface so it can have a dozen methods, but if you want to customize its behavior you can shove a DNS-acting Dial func into it and do whatever behavior you want. (see #12503 and dup #17554)

@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2018

I agree that that approach works around many of the maintainability problems of interfaces, but I still don't understand why it belongs in the os package (rather than, say, a subpackage in io).

@gertcuykens
Copy link
Contributor

Can we maybe start with making a overview of what is currently a interface and what is not?

Example

https://golang.org/src/os/types.go?s=369:411#L6
https://github.com/golang/go/blob/go1.11.2/src/net/http/fs.go#L93

Sure there is a perfect reason for why one is a struct and the other a interface. We only have one shot to make it so that everything get to be in harmony again that developers can predict with almost 100% accuracy if something going to be a struct or a interface without looking at the code.

I suggest we must first create a world map where we define interface or struct countries, but for me your proposal is already like a street name in a unknown country where you need to explain to the mail man to use the godoc stars to navigate his airplane

I think we are skipping a step here and need to zoom way out just to make sure we have this logic harmony now that we have the chance to finally fix the world map after 10 years.

So basically what I am trying to convince is that if A and B is a interface C must be too. Not saying your solution is bad I simply want a complete interface list that make sense first. Thanks

@robpike
Copy link
Contributor Author

robpike commented Nov 14, 2018

I just want to to be able to make them not be files at all. VFS is a different - important, but different - case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

13 participants