internal/lsp, go/packages: reproduce and fix golang/go#39646

This test exposes a lot of fundamental issues with `go list` overlays.
Now that we have the regression test framework, we can copy it over and
make any flakes completely reproducible by waiting for each change to
complete.

The issue here ended up being that initial workspace load returns
workspace packages with no associated files due to golang/go#39986.
Working around this allows invalidation to proceed as usual.

In the process of debugging this, I also noticed that packages can get
loaded as command-line-arguments even when we can get the correct
package path for them. It's pretty complicated to fix this, so
restructured the code a tiny bit and left a TODO. I'd like to come back
to this at some point, but it's not pressing since I was able to fix
this bug.

Fixes golang/go#39646.
Updates golang/go#39986.

Change-Id: Id6b08a5e92d28eddc731feb0ef3fd3b3fc69e64b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/240098
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

@@ -635,6 +635,23 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 			pkg.CompiledGoFiles = pkg.GoFiles
 		}
 
+		// 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 split := strings.Split(err.Pos, ":"); len(split) > 1 {
+				if filename := split[0]; filename != "" {
+					if !filepath.IsAbs(filename) {
+						filename = filepath.Join(state.cfg.Dir, filename)
+					}
+					if info, _ := os.Stat(filename); info != nil {
+						pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, filename)
+						pkg.GoFiles = append(pkg.GoFiles, filename)
+					}
+				}
+			}
+		}
+
 		if p.Error != nil {
 			msg := strings.TrimSpace(p.Error.Err) // Trim to work around golang.org/issue/32363.
 			// Address golang.org/issue/35964 by appending import stack to error message.

go/packages/golist_overlay.go

@@ -102,8 +102,11 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 				}
 			}
 		}
-		// The overlay could have included an entirely new package.
-		if pkg == nil {
+		// The overlay could have included an entirely new package or an
+		// ad-hoc package. An ad-hoc package is one that we have manually
+		// constructed from inadequate `go list` results for a file= query.
+		// It will have the ID command-line-arguments.
+		if pkg == nil || pkg.ID == "command-line-arguments" {
 			// Try to find the module or gopath dir the file is contained in.
 			// Then for modules, add the module opath to the beginning.
 			pkgPath, ok, err := state.getPkgPath(dir)
@@ -127,35 +130,40 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 					id = fmt.Sprintf("%s [%s.test]", pkgPath, pkgPath)
 				}
 			}
-			// Try to reclaim a package with the same ID, if it exists in the response.
-			for _, p := range response.dr.Packages {
-				if reclaimPackage(p, id, opath, contents) {
-					pkg = p
-					break
-				}
-			}
-			// Otherwise, create a new package.
-			if pkg == nil {
-				pkg = &Package{
-					PkgPath: pkgPath,
-					ID:      id,
-					Name:    pkgName,
-					Imports: make(map[string]*Package),
-				}
-				response.addPackage(pkg)
-				havePkgs[pkg.PkgPath] = id
-				// Add the production package's sources for a test variant.
-				if isTestFile && !isXTest && testVariantOf != nil {
-					pkg.GoFiles = append(pkg.GoFiles, testVariantOf.GoFiles...)
-					pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, testVariantOf.CompiledGoFiles...)
-					// Add the package under test and its imports to the test variant.
-					pkg.forTest = testVariantOf.PkgPath
-					for k, v := range testVariantOf.Imports {
-						pkg.Imports[k] = &Package{ID: v.ID}
+			if pkg != nil {
+				// TODO(rstambler): We should change the package's path and ID
+				// here. The only issue is that this messes with the roots.
+			} else {
+				// Try to reclaim a package with the same ID, if it exists in the response.
+				for _, p := range response.dr.Packages {
+					if reclaimPackage(p, id, opath, contents) {
+						pkg = p
+						break
 					}
 				}
-				if isXTest {
-					pkg.forTest = forTest
+				// Otherwise, create a new package.
+				if pkg == nil {
+					pkg = &Package{
+						PkgPath: pkgPath,
+						ID:      id,
+						Name:    pkgName,
+						Imports: make(map[string]*Package),
+					}
+					response.addPackage(pkg)
+					havePkgs[pkg.PkgPath] = id
+					// Add the production package's sources for a test variant.
+					if isTestFile && !isXTest && testVariantOf != nil {
+						pkg.GoFiles = append(pkg.GoFiles, testVariantOf.GoFiles...)
+						pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, testVariantOf.CompiledGoFiles...)
+						// Add the package under test and its imports to the test variant.
+						pkg.forTest = testVariantOf.PkgPath
+						for k, v := range testVariantOf.Imports {
+							pkg.Imports[k] = &Package{ID: v.ID}
+						}
+					}
+					if isXTest {
+						pkg.forTest = forTest
+					}
 				}
 			}
 		}

go/packages/overlay_test.go

@@ -15,12 +15,19 @@ import (
 	"golang.org/x/tools/internal/testenv"
 )
 
-const commonMode = packages.NeedName | packages.NeedFiles |
-	packages.NeedCompiledGoFiles | packages.NeedImports | packages.NeedSyntax
+const (
+	commonMode = packages.NeedName | packages.NeedFiles |
+		packages.NeedCompiledGoFiles | packages.NeedImports | packages.NeedSyntax
+	everythingMode = commonMode | packages.NeedDeps | packages.NeedTypes |
+		packages.NeedTypesSizes
+)
 
-func TestOverlayChangesPackage(t *testing.T) {
+func TestOverlayChangesPackageName(t *testing.T) {
+	packagestest.TestAll(t, testOverlayChangesPackageName)
+}
+func testOverlayChangesPackageName(t *testing.T, exporter packagestest.Exporter) {
 	log.SetFlags(log.Lshortfile)
-	exported := packagestest.Export(t, packagestest.GOPATH, []packagestest.Module{{
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
 		Name: "fake",
 		Files: map[string]interface{}{
 			"a.go": "package foo\nfunc f(){}\n",
@@ -45,9 +52,12 @@ func TestOverlayChangesPackage(t *testing.T) {
 	}
 	log.SetFlags(0)
 }
-func TestOverlayChangesBothPackages(t *testing.T) {
+func TestOverlayChangesBothPackageNames(t *testing.T) {
+	packagestest.TestAll(t, testOverlayChangesBothPackageNames)
+}
+func testOverlayChangesBothPackageNames(t *testing.T, exporter packagestest.Exporter) {
 	log.SetFlags(log.Lshortfile)
-	exported := packagestest.Export(t, packagestest.GOPATH, []packagestest.Module{{
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
 		Name: "fake",
 		Files: map[string]interface{}{
 			"a.go":      "package foo\nfunc g(){}\n",
@@ -88,10 +98,12 @@ func TestOverlayChangesBothPackages(t *testing.T) {
 	}
 	log.SetFlags(0)
 }
-
-func TestOverlayChangesTestPackage(t *testing.T) {
+func TestOverlayChangesTestPackageName(t *testing.T) {
+	packagestest.TestAll(t, testOverlayChangesTestPackageName)
+}
+func testOverlayChangesTestPackageName(t *testing.T, exporter packagestest.Exporter) {
 	log.SetFlags(log.Lshortfile)
-	exported := packagestest.Export(t, packagestest.GOPATH, []packagestest.Module{{
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
 		Name: "fake",
 		Files: map[string]interface{}{
 			"a_test.go": "package foo\nfunc f(){}\n",
@@ -131,6 +143,14 @@ func TestOverlayChangesTestPackage(t *testing.T) {
 	log.SetFlags(0)
 }
 
+func checkPkg(t *testing.T, p *packages.Package, id, name string, syntax int) bool {
+	t.Helper()
+	if p.ID == id && p.Name == name && len(p.Syntax) == syntax {
+		return true
+	}
+	return false
+}
+
 func TestOverlayXTests(t *testing.T) {
 	packagestest.TestAll(t, testOverlayXTests)
 }
@@ -514,8 +534,8 @@ const A = 1
 	}
 }
 
-// TestOverlayModFileChanges tests the behavior resulting from having files from
-// multiple modules in overlays.
+// TestOverlayModFileChanges tests the behavior resulting from having files
+// from multiple modules in overlays.
 func TestOverlayModFileChanges(t *testing.T) {
 	testenv.NeedsTool(t, "go")
 
@@ -674,18 +694,60 @@ func testContainsOverlayXTest(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+func TestInvalidFilesBeforeOverlay(t *testing.T) {
+	packagestest.TestAll(t, testInvalidFilesBeforeOverlay)
+}
+
+func testInvalidFilesBeforeOverlay(t *testing.T, exporter packagestest.Exporter) {
+	testenv.NeedsGo1Point(t, 15)
+
+	exported := packagestest.Export(t, exporter, []packagestest.Module{
+		{
+			Name: "golang.org/fake",
+			Files: map[string]interface{}{
+				"d/d.go":  ``,
+				"main.go": ``,
+			},
+		},
+	})
+	defer exported.Cleanup()
+
+	dir := filepath.Dir(filepath.Dir(exported.File("golang.org/fake", "d/d.go")))
+
+	exported.Config.Mode = everythingMode
+	exported.Config.Tests = true
+
+	// First, check that all packages returned have files associated with them.
+	// Tests the work-around for golang/go#39986.
+	t.Run("no overlay", func(t *testing.T) {
+		initial, err := packages.Load(exported.Config, fmt.Sprintf("%s/...", dir))
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, pkg := range initial {
+			if len(pkg.CompiledGoFiles) == 0 {
+				t.Fatalf("expected at least 1 CompiledGoFile for %s, got none", pkg.PkgPath)
+			}
+		}
+	})
+
+}
+
 // Tests golang/go#35973, fixed in Go 1.14.
-func TestInvalidFilesInOverlay(t *testing.T) { packagestest.TestAll(t, testInvalidFilesInOverlay) }
-func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
+func TestInvalidFilesBeforeOverlayContains(t *testing.T) {
+	packagestest.TestAll(t, testInvalidFilesBeforeOverlayContains)
+}
+func testInvalidFilesBeforeOverlayContains(t *testing.T, exporter packagestest.Exporter) {
 	testenv.NeedsGo1Point(t, 14)
 
 	exported := packagestest.Export(t, exporter, []packagestest.Module{
 		{
 			Name: "golang.org/fake",
 			Files: map[string]interface{}{
-				"d/d.go":      `package d; import "net/http"; const d = http.MethodGet;`,
+				"d/d.go":      `package d; import "net/http"; const Get = http.MethodGet; const Hello = "hello";`,
 				"d/util.go":   ``,
 				"d/d_test.go": ``,
+				"main.go":     ``,
 			},
 		},
 	})
@@ -703,18 +765,22 @@ func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 		// Overlay with a test variant.
 		{"test_variant",
 			map[string][]byte{
-				filepath.Join(dir, "d", "d_test.go"): []byte(`package d; import "testing"; const D = d + "_test"; func TestD(t *testing.T) {};`)},
+				filepath.Join(dir, "d", "d_test.go"): []byte(`package d; import "testing"; const D = Get + "_test"; func TestD(t *testing.T) {};`)},
 			`"GET_test"`},
 		// Overlay in package.
 		{"second_file",
 			map[string][]byte{
-				filepath.Join(dir, "d", "util.go"): []byte(`package d; const D = d + "_util";`)},
+				filepath.Join(dir, "d", "util.go"): []byte(`package d; const D = Get + "_util";`)},
 			`"GET_util"`},
+		// Overlay on the main file.
+		{"main",
+			map[string][]byte{
+				filepath.Join(dir, "main.go"): []byte(`package main; import "golang.org/fake/d"; const D = d.Get + "_main"; func main() {};`)},
+			`"GET_main"`},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			exported.Config.Overlay = tt.overlay
-			exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles |
-				packages.NeedDeps | packages.NeedTypes | packages.NeedTypesSizes
+			exported.Config.Mode = everythingMode
 			exported.Config.Tests = true
 
 			for f := range tt.overlay {
@@ -722,35 +788,27 @@ func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 				if err != nil {
 					t.Fatal(err)
 				}
-				d := initial[0]
+				pkg := initial[0]
 				var containsFile bool
-				for _, goFile := range d.CompiledGoFiles {
+				for _, goFile := range pkg.CompiledGoFiles {
 					if f == goFile {
 						containsFile = true
 						break
 					}
 				}
 				if !containsFile {
-					t.Fatalf("expected %s in CompiledGoFiles, got %v", f, d.CompiledGoFiles)
+					t.Fatalf("expected %s in CompiledGoFiles, got %v", f, pkg.CompiledGoFiles)
 				}
 				// Check value of d.D.
-				dD := constant(d, "D")
-				if dD == nil {
-					t.Fatalf("%d. d.D: got nil", i)
+				D := constant(pkg, "D")
+				if D == nil {
+					t.Fatalf("%d. D: got nil", i)
 				}
-				got := dD.Val().String()
+				got := D.Val().String()
 				if got != tt.want {
-					t.Fatalf("%d. d.D: got %s, want %s", i, got, tt.want)
+					t.Fatalf("%d. D: got %s, want %s", i, got, tt.want)
 				}
 			}
 		})
 	}
 }
-
-func checkPkg(t *testing.T, p *packages.Package, id, name string, syntax int) bool {
-	t.Helper()
-	if p.ID == id && p.Name == name && len(p.Syntax) == syntax {
-		return true
-	}
-	return false
-}

internal/lsp/regtest/diagnostics_test.go

@@ -865,42 +865,57 @@ const C = a.A
 // This is a copy of the scenario_default/quickfix_empty_files.txt test from
 // govim. Reproduces golang/go#39646.
 func TestQuickFixEmptyFiles(t *testing.T) {
+	testenv.NeedsGo1Point(t, 15)
+
 	const mod = `
 -- go.mod --
 module mod.com
 
 go 1.12
 `
-	runner.Run(t, mod, func(t *testing.T, env *Env) {
-		// To fully recreate the govim tests, we create files by inserting
-		// a newline, adding to the file, and then deleting the newline.
-		// Wait for each event to process to avoid cancellations.
-		writeGoVim := func(name, content string) {
-			env.OpenFileWithContent(name, "\n")
-			env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1))
+	// To fully recreate the govim tests, we create files by inserting
+	// a newline, adding to the file, and then deleting the newline.
+	// Wait for each event to process to avoid cancellations and force
+	// package loads.
+	writeGoVim := func(env *Env, name, content string) {
+		env.WriteWorkspaceFile(name, "")
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1))
 
-			env.EditBuffer(name, fake.NewEdit(1, 0, 1, 0, content))
-			env.Await(env.AnyDiagnosticAtCurrentVersion(name))
+		env.OpenFileWithContent(name, "\n")
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1))
 
-			env.EditBuffer(name, fake.NewEdit(0, 0, 1, 0, ""))
-			env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1))
-		}
-		writeGoVim("p/p.go", `package p
+		env.EditBuffer(name, fake.NewEdit(1, 0, 1, 0, content))
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1))
 
+		env.EditBuffer(name, fake.NewEdit(0, 0, 1, 0, ""))
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1))
+	}
 
-func DoIt(s string) {
-	var x int
-}
-`)
-		writeGoVim("main.go", `package main
+	const p = `package p; func DoIt(s string) {};`
+	const main = `package main
 
 import "mod.com/p"
 
 func main() {
 	p.DoIt(5)
 }
-`)
-		writeGoVim("p/p_test.go", `package p
+`
+	// A simple version of the test that reproduces most of the problems it
+	// exposes.
+	t.Run("short", func(t *testing.T) {
+		runner.Run(t, mod, func(t *testing.T, env *Env) {
+			writeGoVim(env, "p/p.go", p)
+			writeGoVim(env, "main.go", main)
+			env.Await(env.DiagnosticAtRegexp("main.go", "5"))
+		})
+	})
+
+	// A full version that replicates the whole flow of the test.
+	t.Run("full", func(t *testing.T) {
+		runner.Run(t, mod, func(t *testing.T, env *Env) {
+			writeGoVim(env, "p/p.go", p)
+			writeGoVim(env, "main.go", main)
+			writeGoVim(env, "p/p_test.go", `package p
 	
 import "testing"
 	
@@ -908,7 +923,7 @@ func TestDoIt(t *testing.T) {
 	DoIt(5)
 }
 `)
-		writeGoVim("p/x_test.go", `package p_test
+			writeGoVim(env, "p/x_test.go", `package p_test
 	
 import (
 	"testing"
@@ -920,17 +935,18 @@ func TestDoIt(t *testing.T) {
 	p.DoIt(5)
 }
 `)
-		env.Await(
-			env.DiagnosticAtRegexp("main.go", "5"),
-			env.DiagnosticAtRegexp("p/p_test.go", "5"),
-			env.DiagnosticAtRegexp("p/x_test.go", "5"),
-		)
-		env.RegexpReplace("p/p.go", "s string", "i int")
-		env.Await(
-			EmptyDiagnostics("main.go"),
-			EmptyDiagnostics("p/p_test.go"),
-			EmptyDiagnostics("p/x_test.go"),
-		)
+			env.Await(
+				env.DiagnosticAtRegexp("main.go", "5"),
+				env.DiagnosticAtRegexp("p/p_test.go", "5"),
+				env.DiagnosticAtRegexp("p/x_test.go", "5"),
+			)
+			env.RegexpReplace("p/p.go", "s string", "i int")
+			env.Await(
+				EmptyDiagnostics("main.go"),
+				EmptyDiagnostics("p/p_test.go"),
+				EmptyDiagnostics("p/x_test.go"),
+			)
+		})
 	})
 }