go/importer: default importer should not use out of date type information #11415
Comments
|
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. |
@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 |
This importer reads type information directly from up-to-date source. See golang/go#11415.
quentinmit
added
the
NeedsFix
|
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. |
As noted by griesemer in golang/go#18799, this doesn't address the issues raised in golang/go#11415 because when checking an xtest package the corresponding package is assumed to have been installed. This however is similar to the assumptions made by go vet (raised as an issue in golang/go#16086). So whilst not perfect, it will probably suffice until golang/go#11415 is resolved. Fixes golang/go#18799 Change-Id: I1ea005c402e5d6f5abddda68fee6386b0531dfba Reviewed-on: https://go-review.googlesource.com/36992 Reviewed-by: Robert Griesemer <gri@golang.org>
|
CL https://golang.org/cl/37393 mentions this issue. |
|
CL https://golang.org/cl/37405 mentions this issue. |
For #11415. Change-Id: I87a8f534ab9dfd5022422457ea637b342c057d77 Reviewed-on: https://go-review.googlesource.com/37393 Reviewed-by: Alan Donovan <adonovan@google.com>
For #11415. Change-Id: I5da39dad059113cfc4276152390aa4925bd18862 Reviewed-on: https://go-review.googlesource.com/37405 Run-TryBot: Robert Griesemer <gri@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
|
The 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 |
|
Yes. It's already using it. But it's in (std lib) go/types/gotype.go, to be
built explicitly (go build gotype.go). There's a new -c flag where you can
specify a different importer.
- gri
…On Fri, Mar 3, 2017 at 12:19 PM, Will Faught ***@***.***> wrote:
Is it planned for gotype to use this new importer?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#11415 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIIkT7w7RX8sj2Iwuitckc7k2p-TPQ7Kks5riHXNgaJpZM4FMbsc>
.
|
|
@mvdan Didn't know it had been removed. GTK. @griesemer Doesn't that break a plain |
|
@willfaught The file has a |
|
Missed that. Thanks. I was thinking it was still meant to be used
publically. Seems like a strange way to "archive" it. Why keep it at all?
…On Sat, Mar 4, 2017 at 8:02 PM Robert Griesemer ***@***.***> wrote:
@willfaught <https://github.com/willfaught> The file has a // +build
ignore line.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11415 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD5VuL5t7w6dNuxUx1Qx1Jdmox6kNclks5rijPvgaJpZM4FMbsc>
.
|
|
@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). |
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 text was updated successfully, but these errors were encountered: