From 6f5e27347a0cc033990d87f7a27f1fab67bad829 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 15 Oct 2019 17:11:27 -0400 Subject: [PATCH] go/packages: handle invalid files in overlays This change handles a specific case where `go list` returns an error on a file that lacks a package declaration. If only one package is returned as a response, we should add that file to that package, since it may have valid content in an overlay. This change uses the internal/span package to correctly parse the error message from `go list`. Change-Id: I5909c4fd765746df1003685aa915b7c5f9cdcee5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/201220 Run-TryBot: Rebecca Stambler Reviewed-by: Michael Matloob --- go/packages/golist.go | 113 ++++++++++++++++++++++++++++------- go/packages/packages_test.go | 49 +++++++++++++++ 2 files changed, 140 insertions(+), 22 deletions(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index ff0f55e2c4..873ec12943 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -26,6 +26,7 @@ import ( "golang.org/x/tools/go/internal/packagesdriver" "golang.org/x/tools/internal/gopathwalk" "golang.org/x/tools/internal/semver" + "golang.org/x/tools/internal/span" ) // debug controls verbose logging. @@ -281,31 +282,42 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err) } dirResponse, err := driver(cfg, pattern) - if err != nil || (len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1) { - // There was an error loading the package. Try to load the file as an ad-hoc package. - // Usually the error will appear in a returned package, but may not if we're in modules mode - // and the ad-hoc is located outside a module. + if err != nil { var queryErr error - dirResponse, queryErr = driver(cfg, query) - if queryErr != nil { - // Return the original error if the attempt to fall back failed. - return err + if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil { + return err // return the original error } - // Special case to handle issue #33482: - // If this is a file= query for ad-hoc packages where the file only exists on an overlay, - // and exists outside of a module, add the file in for the package. - if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) { - if len(dirResponse.Packages[0].GoFiles) == 0 { - filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath - // TODO(matloob): check if the file is outside of a root dir? - for path := range cfg.Overlay { - if path == filename { - dirResponse.Packages[0].Errors = nil - dirResponse.Packages[0].GoFiles = []string{path} - dirResponse.Packages[0].CompiledGoFiles = []string{path} - } - } + } + // `go list` can report errors for files that are not listed as part of a package's GoFiles. + // In the case of an invalid Go file, we should assume that it is part of package if only + // one package is in the response. The file may have valid contents in an overlay. + if len(dirResponse.Packages) == 1 { + pkg := dirResponse.Packages[0] + for i, err := range pkg.Errors { + s := errorSpan(err) + if !s.IsValid() { + break } + if len(pkg.CompiledGoFiles) == 0 { + break + } + dir := filepath.Dir(pkg.CompiledGoFiles[0]) + filename := filepath.Join(dir, filepath.Base(s.URI().Filename())) + if info, err := os.Stat(filename); err != nil || info.IsDir() { + break + } + if !contains(pkg.CompiledGoFiles, filename) { + pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, filename) + pkg.GoFiles = append(pkg.GoFiles, filename) + pkg.Errors = append(pkg.Errors[:i], pkg.Errors[i+1:]...) + } + } + } + // A final attempt to construct an ad-hoc package. + if len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1 { + var queryErr error + if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil { + return err // return the original error } } isRoot := make(map[string]bool, len(dirResponse.Roots)) @@ -333,6 +345,63 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q return nil } +// adHocPackage attempts to construct an ad-hoc package given a query that failed. +func adHocPackage(cfg *Config, driver driver, pattern, query string) (*driverResponse, error) { + // There was an error loading the package. Try to load the file as an ad-hoc package. + // Usually the error will appear in a returned package, but may not if we're in modules mode + // and the ad-hoc is located outside a module. + dirResponse, err := driver(cfg, query) + if err != nil { + return nil, err + } + // Special case to handle issue #33482: + // If this is a file= query for ad-hoc packages where the file only exists on an overlay, + // and exists outside of a module, add the file in for the package. + if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) { + if len(dirResponse.Packages[0].GoFiles) == 0 { + filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath + // TODO(matloob): check if the file is outside of a root dir? + for path := range cfg.Overlay { + if path == filename { + dirResponse.Packages[0].Errors = nil + dirResponse.Packages[0].GoFiles = []string{path} + dirResponse.Packages[0].CompiledGoFiles = []string{path} + } + } + } + } + return dirResponse, nil +} + +func contains(files []string, filename string) bool { + for _, f := range files { + if f == filename { + return true + } + } + return false +} + +// errorSpan attempts to parse a standard `go list` error message +// by stripping off the trailing error message. +// +// It works only on errors whose message is prefixed by colon, +// followed by a space (": "). For example: +// +// attributes.go:13:1: expected 'package', found 'type' +// +func errorSpan(err Error) span.Span { + if err.Pos == "" { + input := strings.TrimSpace(err.Msg) + msgIndex := strings.Index(input, ": ") + if msgIndex < 0 { + return span.Parse(input) + } + return span.Parse(input[:msgIndex]) + } + return span.Parse(err.Pos) +} + // modCacheRegexp splits a path in a module cache into module, module version, and package. var modCacheRegexp = regexp.MustCompile(`(.*)@([^/\\]*)(.*)`) diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 2a73156a38..e3c70080b8 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -1079,6 +1079,51 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) { } } +func TestContainsInOverlays(t *testing.T) { packagestest.TestAll(t, testcontainsInOverlays) } +func testcontainsInOverlays(t *testing.T, exporter packagestest.Exporter) { + // This test checks a specific case where a file is empty on disk. + // In this case, `go list` will return the package golang.org/fake/c + // with only c.go as a GoFile, with an error message for c2.go. + // Since there is only one possible package for c2.go to be a part of, + // go/packages will parse the filename out of error and add it to GoFiles and CompiledGoFiles. + exported := packagestest.Export(t, exporter, []packagestest.Module{{ + Name: "golang.org/fake", + Files: map[string]interface{}{ + "c/c.go": `package c; const C = "c"`, + "c/c2.go": ``, + }}}) + defer exported.Cleanup() + + dir := filepath.Dir(exported.File("golang.org/fake", "c/c.go")) + + for _, test := range []struct { + overlay map[string][]byte + want string + }{ + { + overlay: map[string][]byte{filepath.Join(dir, "c2.go"): []byte(`nonsense`)}, + want: "golang.org/fake/c", + }, + } { + exported.Config.Overlay = test.overlay + exported.Config.Mode = packages.LoadImports + exported.Config.Logf = t.Logf + for filename := range exported.Config.Overlay { + initial, err := packages.Load(exported.Config, "file="+filename) + if err != nil { + t.Fatal(err) + } + if len(initial) == 0 { + t.Fatalf("no packages for %s", filename) + } + pkg := initial[0] + if pkg.PkgPath != test.want { + t.Errorf("got %s, want %s", pkg.PkgPath, test.want) + } + } + } +} + func TestAdHocPackagesBadImport(t *testing.T) { // TODO: Enable this test when github.com/golang/go/issues/33374 is resolved. t.Skip() @@ -2201,10 +2246,14 @@ func testAdHocContains(t *testing.T, exporter packagestest.Exporter) { }() exported.Config.Mode = packages.NeedImports | packages.NeedFiles + exported.Config.Logf = t.Logf pkgs, err := packages.Load(exported.Config, "file="+filename) if err != nil { t.Fatal(err) } + if len(pkgs) == 0 { + t.Fatalf("no packages for %s", filename) + } if len(pkgs) != 1 && pkgs[0].PkgPath != "command-line-arguments" { t.Fatalf("packages.Load: want [command-line-arguments], got %v", pkgs) }