diff --git a/go/packages/golist.go b/go/packages/golist.go index 6e91391ce2..1a5aba9f9f 100644 --- a/go/packages/golist.go +++ b/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. diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go index 338d6f6232..4eabfd98c6 100644 --- a/go/packages/golist_overlay.go +++ b/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 + } } } } diff --git a/go/packages/overlay_test.go b/go/packages/overlay_test.go index d48c47bb14..b7d2f4573a 100644 --- a/go/packages/overlay_test.go +++ b/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 -} diff --git a/internal/lsp/regtest/diagnostics_test.go b/internal/lsp/regtest/diagnostics_test.go index 451aed11fc..13499fb2b1 100644 --- a/internal/lsp/regtest/diagnostics_test.go +++ b/internal/lsp/regtest/diagnostics_test.go @@ -865,72 +865,88 @@ 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" - + 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" "mod.com/p" ) - + 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"), + ) + }) }) }