From 75c71030ab2411331d220ad5a050cdb033a4c12d Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Tue, 4 Aug 2020 16:58:55 -0400 Subject: [PATCH] go/packages: expand cases when filenames are parsed from errors Only add files in errors if either the error's import stack is empty, or the import stack's top package is the same as the package itself, so we have more confidence that the error applies to the package. This is bound to be flaky, but shouldn't be worse than the current state. (And it unblocks a cl from going into the RC...). Updates golang/go#40544 Change-Id: Ie21a8abec7150800d3d34b94a7ec90fd40d93fe9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/246758 Run-TryBot: Michael Matloob TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- go/packages/golist.go | 58 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index 220d409878..b4d232be70 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -89,6 +89,10 @@ type golistState struct { rootDirsError error rootDirs map[string]string + goVersionOnce sync.Once + goVersionError error + goVersion string // third field of 'go version' + // vendorDirs caches the (non)existence of vendor directories. vendorDirs map[string]bool } @@ -638,7 +642,7 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse // Temporary work-around for golang/go#39986. Parse filenames out of // error messages. This happens if there are unrecoverable syntax // errors in the source, so we can't match on a specific error message. - if err := p.Error; err != nil && len(err.ImportStack) == 0 && len(pkg.CompiledGoFiles) == 0 { + if err := p.Error; err != nil && state.shouldAddFilenameFromError(p) { addFilenameFromPos := func(pos string) bool { split := strings.Split(pos, ":") if len(split) < 1 { @@ -697,6 +701,58 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse return &response, nil } +func (state *golistState) shouldAddFilenameFromError(p *jsonPackage) bool { + if len(p.GoFiles) > 0 || len(p.CompiledGoFiles) > 0 { + return false + } + + goV, err := state.getGoVersion() + if err != nil { + return false + } + + // On Go 1.14 and earlier, only add filenames from errors if the import stack is empty. + // The import stack behaves differently for these versions than newer Go versions. + if strings.HasPrefix(goV, "go1.13") || strings.HasPrefix(goV, "go1.14") { + return len(p.Error.ImportStack) == 0 + } + + // On Go 1.15 and later, only parse filenames out of error if there's no import stack, + // or the current package is at the top of the import stack. This is not guaranteed + // to work perfectly, but should avoid some cases where files in errors don't belong to this + // package. + return len(p.Error.ImportStack) == 0 || p.Error.ImportStack[len(p.Error.ImportStack)-1] == p.ImportPath +} + +func (state *golistState) getGoVersion() (string, error) { + state.goVersionOnce.Do(func() { + var b *bytes.Buffer + // Invoke go version. Don't use invokeGo because it will supply build flags, and + // go version doesn't expect build flags. + inv := gocommand.Invocation{ + Verb: "version", + Env: state.cfg.Env, + Logf: state.cfg.Logf, + } + gocmdRunner := state.cfg.gocmdRunner + if gocmdRunner == nil { + gocmdRunner = &gocommand.Runner{} + } + b, _, _, state.goVersionError = gocmdRunner.RunRaw(state.cfg.Context, inv) + if state.goVersionError != nil { + return + } + + sp := strings.Split(b.String(), " ") + if len(sp) < 3 { + state.goVersionError = fmt.Errorf("go version output: expected 'go version ', got '%s'", b.String()) + return + } + state.goVersion = sp[2] + }) + return state.goVersion, state.goVersionError +} + // getPkgPath finds the package path of a directory if it's relative to a root directory. func (state *golistState) getPkgPath(dir string) (string, bool, error) { absDir, err := filepath.Abs(dir)