go/packages: handle multiple modules in gopackagestest

Adding a second module to the gopackagestest configuration exposed a
flaky failure. go/packages was attempting to construct an ID relative to
a module. Depending on which module it saw first, it could construct a
relative path based on the incorrect module. Add an additional module to
the overlay tests to make sure we don't regress here.

Also, fix up a few staticcheck errors that were spotted along the way.

Change-Id: I53c2c0eee62ff88eadcb96a3770452dd7799312e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/199645
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/packages/golist.go

@@ -294,15 +294,16 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
 			// 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 && len(dirResponse.Packages) == 1 &&
+			if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) {
-				dirResponse.Packages[0].ID == "command-line-arguments" && len(dirResponse.Packages[0].GoFiles) == 0 {
+				if len(dirResponse.Packages[0].GoFiles) == 0 {
-				filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
+					filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
-				// TODO(matloob): check if the file is outside of a root dir?
+					// TODO(matloob): check if the file is outside of a root dir?
-				for path := range cfg.Overlay {
+					for path := range cfg.Overlay {
-					if path == filename {
+						if path == filename {
-						dirResponse.Packages[0].Errors = nil
+							dirResponse.Packages[0].Errors = nil
-						dirResponse.Packages[0].GoFiles = []string{path}
+							dirResponse.Packages[0].GoFiles = []string{path}
-						dirResponse.Packages[0].CompiledGoFiles = []string{path}
+							dirResponse.Packages[0].CompiledGoFiles = []string{path}
+						}
 					}
 				}
 			}
@@ -682,7 +683,7 @@ func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driv
 		// contained in a known module or GOPATH entry. This will allow the package to be
 		// properly "reclaimed" when overlays are processed.
 		if filepath.IsAbs(p.ImportPath) && p.Error != nil {
-			pkgPath, ok := getPkgPath(p.ImportPath, rootsDirs)
+			pkgPath, ok := getPkgPath(cfg, p.ImportPath, rootsDirs)
 			if ok {
 				p.ImportPath = pkgPath
 			}
@@ -792,15 +793,31 @@ func golistDriver(cfg *Config, rootsDirs func() *goInfo, words ...string) (*driv
 }
 
 // getPkgPath finds the package path of a directory if it's relative to a root directory.
-func getPkgPath(dir string, goInfo func() *goInfo) (string, bool) {
+func getPkgPath(cfg *Config, dir string, goInfo func() *goInfo) (string, bool) {
+	absDir, err := filepath.Abs(dir)
+	if err != nil {
+		cfg.Logf("error getting absolute path of %s: %v", dir, err)
+		return "", false
+	}
 	for rdir, rpath := range goInfo().rootDirs {
+		absRdir, err := filepath.Abs(rdir)
+		if err != nil {
+			cfg.Logf("error getting absolute path of %s: %v", rdir, err)
+			continue
+		}
+		// Make sure that the directory is in the module,
+		// to avoid creating a path relative to another module.
+		if !strings.HasPrefix(absDir, absRdir) {
+			cfg.Logf("%s does not have prefix %s", absDir, absRdir)
+			continue
+		}
 		// TODO(matloob): This doesn't properly handle symlinks.
 		r, err := filepath.Rel(rdir, dir)
 		if err != nil {
 			continue
 		}
 		if rpath != "" {
-			// We choose only ore root even though the directory even it can belong in multiple modules
+			// We choose only one root even though the directory even it can belong in multiple modules
 			// or GOPATH entries. This is okay because we only need to work with absolute dirs when a
 			// file is missing from disk, for instance when gopls calls go/packages in an overlay.
 			// Once the file is saved, gopls, or the next invocation of the tool will get the correct
@@ -808,6 +825,7 @@ func getPkgPath(dir string, goInfo func() *goInfo) (string, bool) {
 			// TODO(matloob): Implement module tiebreaking?
 			return path.Join(rpath, filepath.ToSlash(r)), true
 		}
+		return filepath.ToSlash(r), true
 	}
 	return "", false
 }
@@ -859,7 +877,7 @@ func invokeGo(cfg *Config, args ...string) (*bytes.Buffer, error) {
 	cmd.Stdout = stdout
 	cmd.Stderr = stderr
 	defer func(start time.Time) {
-		cfg.Logf("%s for %v, stderr: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, args...), stderr)
+		cfg.Logf("%s for %v, stderr: <<%s>> stdout: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, args...), stderr, stdout)
 	}(time.Now())
 
 	if err := cmd.Run(); err != nil {
@@ -987,12 +1005,6 @@ func invokeGo(cfg *Config, args ...string) (*bytes.Buffer, error) {
 	if len(stderr.Bytes()) != 0 && os.Getenv("GOPACKAGESPRINTGOLISTERRORS") != "" {
 		fmt.Fprintf(os.Stderr, "%s stderr: <<%s>>\n", cmdDebugStr(cmd, args...), stderr)
 	}
-
-	// debugging
-	if false {
-		fmt.Fprintf(os.Stderr, "%s stdout: <<%s>>\n", cmdDebugStr(cmd, args...), stdout)
-	}
-
 	return stdout, nil
 }
 

go/packages/golist_overlay.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"go/parser"
 	"go/token"
-	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -87,26 +86,10 @@ func processGolistOverlay(cfg *Config, response *responseDeduper, rootDirs func(
 		if pkg == nil {
 			// Try to find the module or gopath dir the file is contained in.
 			// Then for modules, add the module opath to the beginning.
-			var pkgPath string
+			pkgPath, ok := getPkgPath(cfg, dir, rootDirs)
-			for rdir, rpath := range rootDirs().rootDirs {
+			if !ok {
-				// TODO(matloob): This doesn't properly handle symlinks.
-				r, err := filepath.Rel(rdir, dir)
-				if err != nil {
-					continue
-				}
-				pkgPath = filepath.ToSlash(r)
-				if rpath != "" {
-					pkgPath = path.Join(rpath, pkgPath)
-				}
-				// We only create one new package even it can belong in multiple modules or GOPATH entries.
-				// This is okay because tools (such as the LSP) that use overlays will recompute the overlay
-				// once the file is saved, and golist will do the right thing.
-				// TODO(matloob): Implement module tiebreaking?
 				break
 			}
-			if pkgPath == "" {
-				continue
-			}
 			isXTest := strings.HasSuffix(pkgName, "_test")
 			if isXTest {
 				pkgPath += "_test"

go/packages/packages_test.go

@@ -983,14 +983,23 @@ func testOverlayDeps(t *testing.T, exporter packagestest.Exporter) {
 
 func TestNewPackagesInOverlay(t *testing.T) { packagestest.TestAll(t, testNewPackagesInOverlay) }
 func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
-	exported := packagestest.Export(t, exporter, []packagestest.Module{{
+	exported := packagestest.Export(t, exporter, []packagestest.Module{
-		Name: "golang.org/fake",
+		{
-		Files: map[string]interface{}{
+			Name: "golang.org/fake",
-			"a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`,
+			Files: map[string]interface{}{
-			"b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`,
+				"a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`,
-			"c/c.go": `package c; const C = "c"`,
+				"b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`,
-			"d/d.go": `package d; const D = "d"`,
+				"c/c.go": `package c; const C = "c"`,
-		}}})
+				"d/d.go": `package d; const D = "d"`,
+			},
+		},
+		{
+			Name: "example.com/extramodule",
+			Files: map[string]interface{}{
+				"pkg/x.go": "package pkg\n",
+			},
+		},
+	})
 	defer exported.Cleanup()
 
 	dir := filepath.Dir(filepath.Dir(exported.File("golang.org/fake", "a/a.go")))
@@ -1120,7 +1129,7 @@ func TestAdHocOverlays(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.
-	tmp, err := ioutil.TempDir("", "a")
+	tmp, err := ioutil.TempDir("", "testAdHocOverlays")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1137,10 +1146,14 @@ const A = 1
 		Overlay: map[string][]byte{
 			filename: content,
 		},
+		Logf: t.Logf,
 	}
 	initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
+	}
+	if len(initial) == 0 {
+		t.Fatalf("no packages for %s", filename)
 	}
 	// Check value of a.A.
 	a := initial[0]