go.tools: godoc/vfs -> buildfs

go.tools: godoc/vfs -> buildfs

Turn godoc's vfs into a general build system fs for go.tools.

Major API changes:
- Add a Context method buildfs.FileSystem that returns
  a *build.Context. This lets programs carry around a
  single buildfs.FileSystem containing all file context.
- gcimporter no longer automatically hooks itself up to
  the types package, instead it must be initialized. This
  is because all tools must add their own FileSystem to
  gcimporter, if those tools are to be easily extended with
  a custom file system.

Suggested reading order:
1. buildfs/vfs.go - new method on FileSystem
2. go/gcimporter/gcimporter.go - manual initialization
3. go/types/api.go - no major changes
4. cmd/vet/main.go - a simple worked example
5. everything else is just deck chair rearrangement

This CL can be divided into smaller components, but if it
is it becomes hard to see the purpose of the API changes.

[adonovan, 2013-12-17 22:26:38]

LGTM

This all seems pretty straightforward.  I'm sure gri will have some suggestions
about the getting the gcimporter API right.

Please add package documentation to buildfs explaining the theory of how to use
it.  For example, there are now effectively two kinds of filenames.   Ordinary
POSIX filenames which work with system calls, and "build" filenames that only
work relative to the instance of BuildFS in use.  Applications need to exercise
proper hygiene to make sure they don't use build filenames in system calls, and
they may need to call utility functions to convert build filenames into
something appropriate for printing out (and presumably such conversions can
fail).

In particular, the file name that a token.FileSet associates with an ast.Node is
an internal names, coming from build.Import, so it's not ok to print it using
fset.Position(pos).String() in the user interface.

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode20
buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err
error) { return fs.ReadDir(dir) }
Newlines wouldn't hurt.

https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go#newcode20
buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err
error) { return fs.ReadDir(dir) }
Newlines wouldn't hurt here and below.

https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go
File buildfs/httpfs/httpfs.go (right):

https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go#n...
buildfs/httpfs/httpfs.go:48: func (h *httpFS) Context() *build.Context     {
return h.fs.Context() }
Move closer to the other *httpFS method.

https://codereview.appspot.com/43490043/diff/60001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/43490043/diff/60001/go/types/api.go#newcode111
go/types/api.go:111: // The package
The quoted code is still a declaration.

[crawshaw1, 2014-01-08 22:20:32]

This still needs the path translation functions we have discussed (and some more
documentation), but the tests now pass and this hopefully captures the general
intention of this CL.

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode20
buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err
error) { return fs.ReadDir(dir) }
On 2013/12/17 22:26:39, adonovan wrote:
> Newlines wouldn't hurt.

Done.

https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go#newcode20
buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err
error) { return fs.ReadDir(dir) }
On 2013/12/17 22:26:39, adonovan wrote:
> Newlines wouldn't hurt here and below.

Done.

https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go
File buildfs/httpfs/httpfs.go (right):

https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go#n...
buildfs/httpfs/httpfs.go:48: func (h *httpFS) Context() *build.Context     {
return h.fs.Context() }
On 2013/12/17 22:26:39, adonovan wrote:
> Move closer to the other *httpFS method.

Done.

https://codereview.appspot.com/43490043/diff/60001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/43490043/diff/60001/go/types/api.go#newcode111
go/types/api.go:111: // The package
On 2013/12/17 22:26:39, adonovan wrote:
> The quoted code is still a declaration.

Done.

[adonovan, 2014-01-09 17:04:33]

> This still needs the path translation functions we have discussed (and
some more documentation)

For a change like this which comes with a "theory", it's not a bad idea to start
with the documentation and at least a brief explanation of how the code yet to
be written will work: e.g. the path conversion functions, and challenging parts
such as guessBuildPackage.  As you point out, the mechanical renamings are
distracting during the review, so it's fine to omit them from the CL initially. 
We can't commit the change until we're confident the rest of it can in fact be
implemented.

The code looks like a reasonable implementation of the buildfs concept, but I'm
worried that we're committing ourselves to a future of "incomplete
virtualization" problems.  We need to:
- audit all of these applications for all direct access to the file system (via
os, syscall, os/exec, etc) and for all places where an implicit conversion
occurs between an "openable" file path and a file path that is only meaningful
w.r.t. some buildfs.FileSystem instance.  
- make sure that for each string variable containing a path, that there is not
doubt which of these two kinds of path it is.
- avoid calling parse.ParseFile directly.  It will need to be wrapped so that it
is provided both the logical (not necessarily openable) filename to associate
with the file as well as a Reader for the actual bits (which will need to be
closed).
and we need to keep do all of these things as the code evolves.

What I'd really like to see is an end-to-end demo of one of the tools (e.g.
oracle) using a file system implementation that does the path remapping
necessary to read from (e.g.) Google's proprietary build system.  Though this
wouldn't be exercised by the existing oracle tests, it would help convince us
that the path name problems are manageable.

https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode13
buildfs/build.go:13: // BuildFS modifies a Context to use the given FileSystem,
s/BuildFS/Build/

It doesn't "modif[y] a Context" (it's passed by value), it "returns a new
FileSystem using a Context based on ctx, modified to use the given FileSystem".

https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode30
buildfs/build.go:30: fs  FileSystem
If you make this field anonymous, you can lose some of the method declarations
below, I think.

https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode47
buildfs/build.go:47: func (defaultFS) String() string                          
{ return "OS FS" }
"defaultFS"?

https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go
File buildfs/gatefs/gatefs.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n...
buildfs/gatefs/gatefs.go:20: func New(fs buildfs.FileSystem, gateCh chan bool)
buildfs.FileSystem {
Do callers send/recv on gateCh too?  If not, all you're getting here is a
number, cap(gateCh).

https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n...
buildfs/gatefs/gatefs.go:27: type gate chan bool
chan struct{} ?

https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n...
buildfs/gatefs/gatefs.go:29: func (g gate) enter() { g <- true }
Is this the most efficient implementation of a counting semaphore in Go?

https://codereview.appspot.com/43490043/diff/80001/buildfs/httpfs/httpfs.go
File buildfs/httpfs/httpfs.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/httpfs/httpfs.go#n...
buildfs/httpfs/httpfs.go:5: // Package httpfs implements http.FileSystem using a
godoc buildfs.FileSystem.
s/godoc //

https://codereview.appspot.com/43490043/diff/80001/buildfs/mapfs/mapfs.go
File buildfs/mapfs/mapfs.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/mapfs/mapfs.go#new...
buildfs/mapfs/mapfs.go:13: pathpkg "path"
I assume you're intentionally avoiding "path/filepath" since you don't want its
file system notions to creep into purely syntactical operations.

https://codereview.appspot.com/43490043/diff/80001/buildfs/namespace.go
File buildfs/namespace.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/namespace.go#newco...
buildfs/namespace.go:126: return fs.fs.Context()
This scares me.  You are assuming that all contexts of all filesystems are
identical and you're choosing one nondeterministically on each call.  If you
believe the assumption is sound, assert it.  If not---and I think it is
not---this will need more thought.

https://codereview.appspot.com/43490043/diff/80001/buildfs/os.go
File buildfs/os.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/os.go#newcode27
buildfs/os.go:27: func (root osFS) Context() *build.Context { return
&build.Default }
How would a user specify a non-default build.Context?
(For a concrete use-case, note how importer/util.go disables cgo in its
context.)

https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go
File buildfs/vfs.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode21
buildfs/vfs.go:21: // The FileSystem interface specifies the methods godoc is
using
// A FileSystem defines a read-only view of a file tree.
// Implementations may be based on the OS's file system,
// or alternatives such as archive files, in-memory data.

https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode23
buildfs/vfs.go:23: type FileSystem interface {
Can you comment on the "rootedness" of this abstraction?
i.e. are all paths relative to some implicit root, or can they be absolute too? 
What about uplevel references (../x)?

https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode28
buildfs/vfs.go:28: String() string
No need for String() here.

https://codereview.appspot.com/43490043/diff/80001/buildfs/zipfs/zipfs.go
File buildfs/zipfs/zipfs.go (right):

https://codereview.appspot.com/43490043/diff/80001/buildfs/zipfs/zipfs.go#new...
buildfs/zipfs/zipfs.go:81: func (fs *zipFS) Context() *build.Context { return
&build.Default }
What does it mean for a zipFS to have a build.Context?  Are there any meaningful
interactions between the two at all?

I'm concerned that an application supplying a non-default build.Context will
find its preference ignored because several other FSs use &build.Default and the
Namespace.Context() function returns an arbitrary one.

https://codereview.appspot.com/43490043/diff/80001/cmd/gotype/gotype.go
File cmd/gotype/gotype.go (right):

https://codereview.appspot.com/43490043/diff/80001/cmd/gotype/gotype.go#newco...
cmd/gotype/gotype.go:27: fs = buildfs.Default
I'm having flashbacks to Blaze, in which we built a similar layered virtual
filesystem.  It was a constant battle to stop programmers writing library
routines that called on the global buildfs.Default instance (or the standard
file I/O library) to interpret some filename string, without awareness of the
risk involved.  Effectively we're asking them to forego a bunch of basic
services and instead always plumb the correct FS all the way through their
program.

https://codereview.appspot.com/43490043/diff/80001/importer/importer.go
File importer/importer.go (right):

https://codereview.appspot.com/43490043/diff/80001/importer/importer.go#newco...
importer/importer.go:66: fs            buildfs.FileSystem     // client's file
system
FS?  I think you'll need to expose this since it's the effective file system in
use.  Config.FS may be nil.

https://codereview.appspot.com/43490043/diff/80001/importer/importer.go#newco...
importer/importer.go:113: FS buildfs.FileSystem
You'll need to merge this with CL 48770043 when it goes in.

https://codereview.appspot.com/43490043/diff/80001/oracle/oracle.go
File oracle/oracle.go (right):

https://codereview.appspot.com/43490043/diff/80001/oracle/oracle.go#newcode280
oracle/oracle.go:280: _, importPath, err :=
guessImportPath(fqpos.fset.File(fqpos.start).Name(), impcfg.FS.Context())
guessImportPath makes direct access to the file system.  It will need rethinking
to use the buildfs.  There may be other such functions too, but this one is
interesting because it's a candidate utility for moving closer to go/build: it
answers the query: given an openable filename, which (GOPATH entry, package,
package-relative path) does it belong to?

[gri, 2014-01-09 19:32:45]

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go
File buildfs/build.go (right):

https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode26
buildfs/build.go:26: fs  FileSystem
embed FileSystem and then you don't need (some of) the manual forwarders below ?

https://codereview.appspot.com/43490043/diff/40001/buildfs/httpfs/httpfs.go
File buildfs/httpfs/httpfs.go (right):

https://codereview.appspot.com/43490043/diff/40001/buildfs/httpfs/httpfs.go#n...
buildfs/httpfs/httpfs.go:5: // Package httpfs implements http.FileSystem using a
godoc buildfs.FileSystem.
remove reference to godoc?

https://codereview.appspot.com/43490043/diff/40001/buildfs/vfs.go
File buildfs/vfs.go (right):

https://codereview.appspot.com/43490043/diff/40001/buildfs/vfs.go#newcode16
buildfs/vfs.go:16: // The FileSystem interface specifies the methods godoc is
using
this is not godoc-specific, eventually, I assume?

https://codereview.appspot.com/43490043/diff/40001/cmd/gotype/gotype.go
File cmd/gotype/gotype.go (right):

https://codereview.appspot.com/43490043/diff/40001/cmd/gotype/gotype.go#newco...
cmd/gotype/gotype.go:241: types.DefaultImport = gcimporter.Importer(fs, "")
The global DefaultImport is a really bad hack to avoid cycles in common cases.
Since there's an explicit Config provided (line 191), instead you should set the
Config.Import field.

https://codereview.appspot.com/43490043/diff/40001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/43490043/diff/40001/cmd/vet/main.go#newcode103
cmd/vet/main.go:103: types.DefaultImport = gcimporter.Importer(fs, "")
Should probably also use a Config when calling types.Check.

https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.go
File go/gcimporter/gcimporter.go (left):

https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.g...
go/gcimporter/gcimporter.go:31: types.DefaultImport = Import
This might break some code...

https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.go
File go/gcimporter/gcimporter.go (right):

https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.g...
go/gcimporter/gcimporter.go:111: // If no FileSystem is specified,
buildfs.Default is used.
s/FileSystem/file system/ since there's no such parameter name (it's "fs"). And
later you refer to srcDir and not source directory.