go/packages: revert "handle invalid files in overlays"
This reverts commit 6f5e27347a0cc033990d87f7a27f1fab67bad829, golang.org/cl/201220.
Reason for revert: Produces unusable Package results. See golang/go#35949 and golang/go#35973.
Fixes golang/go#35949.
Change-Id: Ic4632fe7f00366b1edf59840c83160d66499ba12
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209978
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
(cherry picked from commit 427c522ce2c75efa74f81f86ebd00b2b19c402e2)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210206
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index c581bce..b327a4c 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -26,7 +26,6 @@
"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.
@@ -287,42 +286,43 @@
return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err)
}
dirResponse, err := driver(cfg, pattern)
- if err != nil {
+ 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.
var queryErr error
- if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil {
- return err // return the original error
+ dirResponse, queryErr = driver(cfg, query)
+ if queryErr != nil {
+ // Return the original error if the attempt to fall back failed.
+ return err
}
- }
- // `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:]...)
- }
+ // If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
+ if len(dirResponse.Packages) == 0 && queryErr == nil {
+ dirResponse.Packages = append(dirResponse.Packages, &Package{
+ ID: "command-line-arguments",
+ PkgPath: query,
+ GoFiles: []string{query},
+ CompiledGoFiles: []string{query},
+ Imports: make(map[string]*Package),
+ })
+ dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
}
- }
- // 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
+ // 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" ||
+ filepath.ToSlash(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}
+ }
+ }
+ }
}
}
isRoot := make(map[string]bool, len(dirResponse.Roots))
@@ -350,74 +350,6 @@
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
- }
- // If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
- if len(dirResponse.Packages) == 0 && err == nil {
- dirResponse.Packages = append(dirResponse.Packages, &Package{
- ID: "command-line-arguments",
- PkgPath: query,
- GoFiles: []string{query},
- CompiledGoFiles: []string{query},
- Imports: make(map[string]*Package),
- })
- dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
- }
- // 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 ff8bcc6..b2b2240 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -1099,51 +1099,6 @@
}
}
-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) {
// This test doesn't use packagestest because we are testing ad-hoc packages,
// which are outside of $GOPATH and outside of a module.
@@ -1210,36 +1165,38 @@
// Make sure that the user's value of GO111MODULE does not affect test results.
for _, go111module := range []string{"off", "auto", "on"} {
- config := &packages.Config{
- Dir: tmp,
- Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
- Mode: packages.LoadAllSyntax,
- Overlay: map[string][]byte{
- filename: content,
- },
- Logf: t.Logf,
- }
- initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
- if err != nil {
- t.Fatal(err)
- }
- if len(initial) == 0 {
- t.Fatalf("no packages for %s with GO111MODULE=%s", filename, go111module)
- }
- // Check value of a.A.
- a := initial[0]
- if a.Errors != nil {
- t.Fatalf("a: got errors %+v, want no error", err)
- }
- aA := constant(a, "A")
- if aA == nil {
- t.Errorf("a.A: got nil")
- return
- }
- got := aA.Val().String()
- if want := "1"; got != want {
- t.Errorf("a.A: got %s, want %s", got, want)
- }
+ t.Run("GO111MODULE="+go111module, func(t *testing.T) {
+ config := &packages.Config{
+ Dir: tmp,
+ Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
+ Mode: packages.LoadAllSyntax,
+ Overlay: map[string][]byte{
+ filename: content,
+ },
+ Logf: t.Logf,
+ }
+ initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(initial) == 0 {
+ t.Fatalf("no packages for %s", filename)
+ }
+ // Check value of a.A.
+ a := initial[0]
+ if a.Errors != nil {
+ t.Fatalf("a: got errors %+v, want no error", err)
+ }
+ aA := constant(a, "A")
+ if aA == nil {
+ t.Errorf("a.A: got nil")
+ return
+ }
+ got := aA.Val().String()
+ if want := "1"; got != want {
+ t.Errorf("a.A: got %s, want %s", got, want)
+ }
+ })
}
}
@@ -2273,14 +2230,10 @@
}()
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)
}
