internal/lsp: store workspace package IDs with package paths

A test variant for a package can only be reloaded by running go/packages
on the non-test variants import path with the -test flag. We need to
cache this import path in order to be able to reload a test package
on-demand.

Also, always ignore test main packages by detecting them in the
metadata.

Fixes golang/go#36473

Change-Id: I6e5a61c8e73689212303ae09fa3aed4a2ee8762a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213660
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/check.go

@@ -136,7 +136,7 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
 	var depKeys [][]byte
 	for _, depID := range depList {
 		mode := source.ParseExported
-		if s.isWorkspacePackage(depID) {
+		if _, ok := s.isWorkspacePackage(depID); ok {
 			mode = source.ParseFull
 		}
 		depHandle, err := s.buildPackageHandle(ctx, depID, mode)
@@ -257,14 +257,15 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
 		},
 	}
 	var (
-		files       = make([]*ast.File, len(pkg.compiledGoFiles))
-		parseErrors = make([]error, len(pkg.compiledGoFiles))
-		wg          sync.WaitGroup
+		files        = make([]*ast.File, len(pkg.compiledGoFiles))
+		parseErrors  = make([]error, len(pkg.compiledGoFiles))
+		actualErrors = make([]error, len(pkg.compiledGoFiles))
+		wg           sync.WaitGroup
 	)
 	for i, ph := range pkg.compiledGoFiles {
 		wg.Add(1)
 		go func(i int, ph source.ParseGoHandle) {
-			files[i], _, parseErrors[i], _ = ph.Parse(ctx)
+			files[i], _, parseErrors[i], actualErrors[i] = ph.Parse(ctx)
 			wg.Done()
 		}(i, ph)
 	}
@@ -297,7 +298,7 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
 	if pkg.pkgPath == "unsafe" {
 		pkg.types = types.Unsafe
 	} else if len(files) == 0 { // not the unsafe package, no parsed files
-		return nil, errors.Errorf("no parsed files for package %s, expected: %s", pkg.pkgPath, pkg.compiledGoFiles)
+		return nil, errors.Errorf("no parsed files for package %s, expected: %s, errors: %v", pkg.pkgPath, pkg.compiledGoFiles, actualErrors)
 	} else {
 		pkg.types = types.NewPackage(string(m.pkgPath), m.name)
 	}

internal/lsp/cache/load.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"fmt"
 	"go/types"
+	"strings"
 
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/source"
@@ -37,6 +38,8 @@ type metadata struct {
 func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, error) {
 	var query string
 	switch scope := scope.(type) {
+	case packagePath:
+		query = string(scope)
 	case packageID:
 		query = string(scope)
 	case fileURI:
@@ -123,16 +126,19 @@ func (s *snapshot) updateMetadata(ctx context.Context, scope interface{}, pkgs [
 		if _, isDir := scope.(directoryURI); !isDir || s.view.Options().VerboseOutput {
 			log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
 		}
-		// Handle golang/go#36292 by ignoring packages with no sources and no errors.
+		// golang/go#36292: Ignore packages with no sources and no errors.
 		if len(pkg.GoFiles) == 0 && len(pkg.CompiledGoFiles) == 0 && len(pkg.Errors) == 0 {
 			continue
 		}
+		// Skip test main packages.
+		if s.view.isTestMain(ctx, pkg) {
+			continue
+		}
 		// Set the metadata for this package.
 		if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil {
 			return nil, err
 		}
-		m := s.getMetadata(packageID(pkg.ID))
-		if m != nil {
+		if m := s.getMetadata(packageID(pkg.ID)); m != nil {
 			results = append(results, m)
 		}
 	}
@@ -191,16 +197,36 @@ func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *
 			m.missingDeps[importPkgPath] = struct{}{}
 			continue
 		}
-		dep := s.getMetadata(importID)
-		if dep == nil {
+		if s.getMetadata(importID) == nil {
 			if err := s.updateImports(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
 				log.Error(ctx, "error in dependency", err)
 			}
 		}
 	}
-
 	// Add the metadata to the cache.
 	s.setMetadata(m)
-
 	return nil
 }
+
+func (v *view) isTestMain(ctx context.Context, pkg *packages.Package) bool {
+	// Test mains must have an import path that ends with ".test".
+	if !strings.HasSuffix(pkg.PkgPath, ".test") {
+		return false
+	}
+	// Test main packages are always named "main".
+	if pkg.Name != "main" {
+		return false
+	}
+	// Test mains always have exactly one GoFile that is in the build cache.
+	if len(pkg.GoFiles) > 1 {
+		return false
+	}
+	buildCachePath, err := v.getBuildCachePath(ctx)
+	if err != nil {
+		return false
+	}
+	if !strings.HasPrefix(pkg.GoFiles[0], buildCachePath) {
+		return false
+	}
+	return true
+}

internal/lsp/cache/session.go

@@ -102,7 +102,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 			files:             make(map[span.URI]source.FileHandle),
 			importedBy:        make(map[packageID][]packageID),
 			actions:           make(map[actionKey]*actionHandle),
-			workspacePackages: make(map[packageID]bool),
+			workspacePackages: make(map[packageID]packagePath),
 		},
 		ignoredURIs: make(map[span.URI]struct{}),
 		builtin:     &builtinPkg{},

internal/lsp/cache/snapshot.go

@@ -51,7 +51,7 @@ type snapshot struct {
 
 	// workspacePackages contains the workspace's packages, which are loaded
 	// when the view is created.
-	workspacePackages map[packageID]bool
+	workspacePackages map[packageID]packagePath
 }
 
 type packageKey struct {
@@ -127,7 +127,13 @@ func (s *snapshot) PackageHandle(ctx context.Context, pkgID string) (source.Pack
 	if m := s.getMetadata(id); m != nil {
 		meta = append(meta, m)
 	}
-	phs, err := s.packageHandles(ctx, id, meta)
+	// We might need to reload the package. If it is a workspace package,
+	// it may be a test variant and therefore have a different scope.
+	var scope interface{} = id
+	if path, ok := s.isWorkspacePackage(id); ok {
+		scope = path
+	}
+	phs, err := s.packageHandles(ctx, scope, meta)
 	if err != nil {
 		return nil, err
 	}
@@ -339,8 +345,7 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.PackageHandle, e
 	for pkgID := range wsPackages {
 		ph, err := s.PackageHandle(ctx, string(pkgID))
 		if err != nil {
-			log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID))
-			continue
+			return nil, err
 		}
 		results = append(results, ph)
 	}
@@ -491,19 +496,19 @@ func (s *snapshot) getIDs(uri span.URI) []packageID {
 	return s.ids[uri]
 }
 
-func (s *snapshot) isWorkspacePackage(id packageID) bool {
+func (s *snapshot) isWorkspacePackage(id packageID) (packagePath, bool) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	_, ok := s.workspacePackages[id]
-	return ok
+	scope, ok := s.workspacePackages[id]
+	return scope, ok
 }
 
-func (s *snapshot) setWorkspacePackage(id packageID) {
+func (s *snapshot) setWorkspacePackage(id packageID, pkgPath packagePath) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	s.workspacePackages[id] = true
+	s.workspacePackages[id] = pkgPath
 }
 
 func (s *snapshot) getFileURIs() []span.URI {
@@ -625,7 +630,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURI span.URI, withoutFileKi
 		packages:          make(map[packageKey]*packageHandle),
 		actions:           make(map[actionKey]*actionHandle),
 		files:             make(map[span.URI]source.FileHandle),
-		workspacePackages: make(map[packageID]bool),
+		workspacePackages: make(map[packageID]packagePath),
 	}
 
 	// Copy all of the FileHandles.

internal/lsp/cache/view.go

@@ -94,6 +94,9 @@ type view struct {
 	initializeOnce      sync.Once
 	initialized         chan struct{}
 	initializationError error
+
+	// buildCachePath is the value of `go env GOCACHE`.
+	buildCachePath string
 }
 
 // fileBase holds the common functionality for all files.
@@ -284,13 +287,11 @@ func (v *view) buildProcessEnv(ctx context.Context) (*imports.ProcessEnv, error)
 	}
 
 	if env.GOPATH == "" {
-		cmd := exec.CommandContext(ctx, "go", "env", "GOPATH")
-		cmd.Env = cfg.Env
-		if out, err := cmd.CombinedOutput(); err != nil {
+		gopath, err := getGoEnvVar(ctx, cfg, "GOPATH")
+		if err != nil {
 			return nil, err
-		} else {
-			env.GOPATH = strings.TrimSpace(string(out))
 		}
+		env.GOPATH = gopath
 	}
 	return env, nil
 }
@@ -481,8 +482,10 @@ func (v *view) WorkspacePackageIDs(ctx context.Context) ([]string, error) {
 		if err != nil {
 			v.initializationError = err
 		}
+		// A test variant of a package can only be loaded directly by loading
+		// the non-test variant with -test. Track the import path of the non-test variant.
 		for _, m := range meta {
-			s.setWorkspacePackage(m.id)
+			s.setWorkspacePackage(m.id, m.pkgPath)
 		}
 	})
 	if v.initializationError != nil {
@@ -615,3 +618,43 @@ func findFileInPackage(pkg source.Package, uri span.URI) (source.ParseGoHandle,
 	}
 	return nil, nil, errors.Errorf("no file for %s in package %s", uri, pkg.ID())
 }
+
+func (v *view) getBuildCachePath(ctx context.Context) (string, error) {
+	v.mu.Lock()
+	defer v.mu.Unlock()
+
+	if v.buildCachePath == "" {
+		path, err := getGoEnvVar(ctx, v.Config(ctx), "GOCACHE")
+		if err != nil {
+			return "", err
+		}
+		v.buildCachePath = path
+	}
+	return v.buildCachePath, nil
+}
+
+func getGoEnvVar(ctx context.Context, cfg *packages.Config, value string) (string, error) {
+	var result string
+	for _, kv := range cfg.Env {
+		split := strings.Split(kv, "=")
+		if len(split) < 2 {
+			continue
+		}
+		if split[0] == value {
+			result = split[1]
+		}
+	}
+	if result == "" {
+		cmd := exec.CommandContext(ctx, "go", "env", value)
+		cmd.Env = cfg.Env
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			return "", err
+		}
+		result = strings.TrimSpace(string(out))
+		if result == "" {
+			return "", errors.Errorf("no value for %s", value)
+		}
+	}
+	return result, nil
+}

internal/lsp/cmd/test/implementation.go

@@ -28,8 +28,10 @@ func (r *runner) Implementation(t *testing.T, spn span.Span, imps []span.Span) {
 	filename := uri.Filename()
 	target := filename + fmt.Sprintf(":%v:%v", spn.Start().Line(), spn.Start().Column())
 
-	got, _ := r.NormalizeGoplsCmd(t, "implementation", target)
-	if expect != got {
+	got, stderr := r.NormalizeGoplsCmd(t, "implementation", target)
+	if stderr != "" {
+		t.Errorf("implementation failed for %s: %s", target, stderr)
+	} else if expect != got {
 		t.Errorf("implementation failed for %s expected:\n%s\ngot:\n%s", target, expect, got)
 	}
 }

internal/lsp/cmd/test/references.go

@@ -27,8 +27,10 @@ func (r *runner) References(t *testing.T, spn span.Span, itemList []span.Span) {
 	uri := spn.URI()
 	filename := uri.Filename()
 	target := filename + fmt.Sprintf(":%v:%v", spn.Start().Line(), spn.Start().Column())
-	got, _ := r.NormalizeGoplsCmd(t, "references", "-d", target)
-	if expect != got {
+	got, stderr := r.NormalizeGoplsCmd(t, "references", "-d", target)
+	if stderr != "" {
+		t.Errorf("references failed for %s: %s", target, stderr)
+	} else if expect != got {
 		t.Errorf("references failed for %s expected:\n%s\ngot:\n%s", target, expect, got)
 	}
 }

internal/lsp/source/implementation.go

@@ -12,7 +12,6 @@ import (
 	"go/types"
 
 	"golang.org/x/tools/internal/lsp/protocol"
-	"golang.org/x/tools/internal/telemetry/log"
 	"golang.org/x/tools/internal/telemetry/trace"
 	errors "golang.org/x/xerrors"
 )
@@ -34,14 +33,12 @@ func Implementation(ctx context.Context, s Snapshot, f FileHandle, pp protocol.P
 
 		rng, err := objToMappedRange(s.View(), impl.pkg, impl.obj)
 		if err != nil {
-			log.Error(ctx, "Error getting range for object", err)
-			continue
+			return nil, err
 		}
 
 		pr, err := rng.Range()
 		if err != nil {
-			log.Error(ctx, "Error getting protocol range for object", err)
-			continue
+			return nil, err
 		}
 
 		locations = append(locations, protocol.Location{

internal/lsp/source/suggested_fix.go

@@ -1 +0,0 @@
-package source