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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>

Author: stamblerre

go/packages/golist.go

@@ -273,43 +273,16 @@ func (state *golistState) runContainsQueries(response *responseDeduper, queries
 			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 @@ func (state *golistState) runContainsQueries(response *responseDeduper, queries
 	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 {

go/packages/golist_overlay.go

@@ -116,6 +116,11 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 				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}
+					}
 				}
 			}
 		}

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

internal/lsp/cache/errors.go

@@ -44,16 +44,18 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface
 
 		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