go/importer: default importer should not use out of date type information
Closed this issue · 15 comments
It is common to use go/types is as part of a go generate command
or other command line tool to introspect a Go package.
The go tool goes to some lengths to ensure that whenever
you operate on a package, it builds the source for that
package into object form before using it, so you don't
end up using symbol information that's out of date with respect
to the source.
When using go/types from a command line tool, you
don't have that luxury, and it's not necessarily appropriate
to call "go install" automatically.
I believe that a better way would be for the default
importer to use the object file data only if it's newer
than the source files, otherwise fall back to using the
source directly. Or at least it would be nice to have
that option, so that it's easy to make go generate
utilities operate on the up to date code.
I hope this could be considered before Go 1.5 as
we won't be able to change this behaviour after then.
The go/types package takes no opinion on issues like this, by design. It's simply a function from ASTs to type information, and it makes upcalls into the client application if it needs to import type information for dependencies.
Many applications use the gcimporter package to read type information from compiled object files, though as you point out, there's no guarantee that those files are even remotely recent. This seems like a bad default to me.
Many other applications use the golang.org/x/tools/go/loader package, which loads the entire program from source code, and parses and type-checks it, so the correct result is guaranteed. It's slower of course, but still quite fast: to load golang.org/x/tools/cmd/oracle (130KLoC) takes about 400ms.
If you care about staleness, and you should then, then use go/loader. A hybrid mechanism that uses gcimporter if fresh and source if not seems tricky to get right. I recently dropped support for hybrid loading from go/loader for reasons of complexity, and it didn't even use a staleness check.
This is one of many things that we need (and plan) to document in the go/types API for Go 1.5.
This is one of many things that we need (and plan) to document in the go/types API for Go 1.5.
@adonovan couldn't agree more. I spent two days tracking an import bug (mentioned just above) that was just this. Could have saved me quite some time if either go/types or go/importer mentioned this common assumption/mistake.
Will the fix for this make go/importer compatible with how the go tool treats vendored types as discussed in #14496?
@willfaught #14496 was closed as a duplicate of this issue - so yes.
CL https://golang.org/cl/36992 mentions this issue.
CL https://golang.org/cl/37393 mentions this issue.
CL https://golang.org/cl/37405 mentions this issue.
The go/importer package now supports importing from "source" (use importer.For("source", nil) rather than importer.Default()). See the above-mentioned CLs for details.
The original issue description asks for a more automatic approach that selects between installed packages and/or source depending on whichever is newer. Using source always will most often be the correct (and least surprising) approach.
See issue #19337 for problems related to making a mixed-mode automatic approach feasible.
Considering this issue resolved at this time.
Is it planned for gotype to use this new importer?
@willfaught if you mean x/tools/cmd/gotype, it's gone: golang/tools@f5a6ee1
@mvdan Didn't know it had been removed. GTK.
@griesemer Doesn't that break a plain go build command in src/go/types, since its package is main? How does that work?
@willfaught The file has a // +build ignore line.
@willfaught It's not "archived". I for one (and others) use it frequently. It's just not yet become a "regular" command (in the std library).