go/packages: handle an overlay edge case with test variants
As usual, in debugging the creation of a new file with gopls, I've
encountered a go/packages overlay bug. The issue is:
A file b/b.go with package name "b" exists on disk. A package
b/b_test.go with no content exists on disk. There is an overlay for
b/b_test.go that contains the package name "b". Running packages.Load
for file=b/b_test.go will result in a failure to load package b
[b.test]. This change adds this test to the go/packages tests.
This case is fixed by restricting the fallback logic in
runContainsQueries. We only attempt to construct an ad-hoc package if
the original package was returned with no GoFiles.
Also, a minor change to the gopls error parsing code that fixes a case
in which diagnostics were being sent without corresponding files.
Updates golang/go#36635.
Change-Id: I38680a2cc65ae9c3252294db6b942d031189faf5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215743
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index 31d6697..f098905 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -273,43 +273,16 @@
return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err)
}
dirResponse, err := state.createDriverResponse(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 there was an error loading the package, or the package is returned
+ // with errors, 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 module mode and the ad-hoc is located outside a module.
+ if err != nil || len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].GoFiles) == 0 &&
+ len(dirResponse.Packages[0].Errors) == 1 {
var queryErr error
- dirResponse, queryErr = state.createDriverResponse(query)
- if queryErr != nil {
- // Return the original error if the attempt to fall back failed.
- return 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 && 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")
- }
- // 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 state.cfg.Overlay {
- if path == filename {
- dirResponse.Packages[0].Errors = nil
- dirResponse.Packages[0].GoFiles = []string{path}
- dirResponse.Packages[0].CompiledGoFiles = []string{path}
- }
- }
- }
+ if dirResponse, queryErr = state.adhocPackage(pattern, query); queryErr != nil {
+ return err // return the original error
}
}
isRoot := make(map[string]bool, len(dirResponse.Roots))
@@ -337,6 +310,49 @@
return nil
}
+// adhocPackage attempts to load or construct an ad-hoc package for a given
+// query, if the original call to the driver produced inadequate results.
+func (state *golistState) adhocPackage(pattern, query string) (*driverResponse, error) {
+ response, err := state.createDriverResponse(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.
+ // TODO(rstambler): Should this check against the original response?
+ if len(response.Packages) == 0 {
+ response.Packages = append(response.Packages, &Package{
+ ID: "command-line-arguments",
+ PkgPath: query,
+ GoFiles: []string{query},
+ CompiledGoFiles: []string{query},
+ Imports: make(map[string]*Package),
+ })
+ response.Roots = append(response.Roots, "command-line-arguments")
+ }
+ // Handle special cases.
+ if len(response.Packages) == 1 {
+ // golang/go#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 to the package and remove the errors.
+ if response.Packages[0].ID == "command-line-arguments" ||
+ filepath.ToSlash(response.Packages[0].PkgPath) == filepath.ToSlash(query) {
+ if len(response.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 state.cfg.Overlay {
+ if path == filename {
+ response.Packages[0].Errors = nil
+ response.Packages[0].GoFiles = []string{path}
+ response.Packages[0].CompiledGoFiles = []string{path}
+ }
+ }
+ }
+ }
+ }
+ return response, nil
+}
+
// Fields must match go list;
// see $GOROOT/src/cmd/go/internal/load/pkg.go.
type jsonPackage struct {
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index bc73fd6..fe77a11 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -116,6 +116,11 @@
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}
+ }
}
}
}
diff --git a/go/packages/packages114_test.go b/go/packages/packages114_test.go
new file mode 100644
index 0000000..1b0a824
--- /dev/null
+++ b/go/packages/packages114_test.go
@@ -0,0 +1,77 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build go1.14
+
+package packages_test
+
+import (
+ "fmt"
+ "path/filepath"
+ "testing"
+
+ "golang.org/x/tools/go/packages"
+ "golang.org/x/tools/go/packages/packagestest"
+)
+
+func TestInvalidFilesInOverlay(t *testing.T) { packagestest.TestAll(t, testInvalidFilesInOverlay) }
+func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
+ 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/util.go": ``,
+ "d/d_test.go": ``,
+ },
+ },
+ })
+ defer exported.Cleanup()
+
+ dir := filepath.Dir(filepath.Dir(exported.File("golang.org/fake", "d/d.go")))
+
+ // Additional tests for test variants.
+ for i, tt := range []struct {
+ name string
+ overlay map[string][]byte
+ want string // expected value of d.D
+
+ }{
+ // 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) {};`)},
+ `"GET_test"`},
+ // Overlay in package.
+ {"second_file",
+ map[string][]byte{
+ filepath.Join(dir, "d", "util.go"): []byte(`package d; const D = d + "_util";`)},
+ `"GET_util"`},
+ } {
+ 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.Tests = true
+
+ for f := range tt.overlay {
+ initial, err := packages.Load(exported.Config, fmt.Sprintf("file=%s", f))
+ if err != nil {
+ t.Fatal(err)
+ }
+ d := initial[0]
+ // Check value of d.D.
+ dD := constant(d, "D")
+ if dD == nil {
+ t.Fatalf("%d. d.D: got nil", i)
+ }
+ got := dD.Val().String()
+ if got != tt.want {
+ t.Fatalf("%d. d.D: got %s, want %s", i, got, tt.want)
+ }
+ }
+ })
+
+ }
+}
diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go
index 56462e1..30ad8c7 100644
--- a/internal/lsp/cache/errors.go
+++ b/internal/lsp/cache/errors.go
@@ -44,16 +44,18 @@
if e.Pos == "" {
spn = parseGoListError(e.Msg)
+
+ // We may not have been able to parse a valid span.
+ if _, err := spanToRange(ctx, pkg, spn); err != nil {
+ return &source.Error{
+ URI: spn.URI(),
+ Message: msg,
+ Kind: kind,
+ }, nil
+ }
} else {
spn = span.Parse(e.Pos)
}
- // If the range can't be derived from the parseGoListError function, then we do not have a valid position.
- if _, err := spanToRange(ctx, pkg, spn); err != nil && e.Pos == "" {
- return &source.Error{
- Message: msg,
- Kind: kind,
- }, nil
- }
case *scanner.Error:
msg = e.Msg
kind = source.ParseError
