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 <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>

Author: stamblerre

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(`(.*)@([^/\\]*)(.*)`)
 

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)
 	}