internal/lsp: add ID to the package cache key

Our original caching plan was to use only the file ParseGoHandles as
cache keys to define a given package. However, because of package test
variants, we cannot rely on files alone. A package may have the exact
same set of files, but be a test variant. Add the ID to the key to avoid
clobbering entries in the cache.

Also, remove the unused metadata ID cache.

Change-Id: I4b33de1f83f6c769d23441e98a2a7324ff0fa1b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194571
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/cache/check.go

@@ -43,6 +43,7 @@ type importer struct {
 
 // checkPackageKey uniquely identifies a package and its config.
 type checkPackageKey struct {
+	id     string
 	files  string
 	config string
 
@@ -85,6 +86,7 @@ func (imp *importer) checkPackageHandle(ctx context.Context, m *metadata) (*chec
 		return nil, err
 	}
 	key := checkPackageKey{
+		id:     string(m.id),
 		files:  hashParseKeys(phs),
 		config: hashConfig(imp.config),
 	}
@@ -144,6 +146,10 @@ func (cph *checkPackageHandle) Files() []source.ParseGoHandle {
 	return cph.files
 }
 
+func (cph *checkPackageHandle) ID() string {
+	return string(cph.m.id)
+}
+
 func (cph *checkPackageHandle) Cached(ctx context.Context) (source.Package, error) {
 	v := cph.handle.Cached()
 	if v == nil {

internal/lsp/cache/load.go

@@ -278,7 +278,6 @@ func (v *view) link(ctx context.Context, g *importGraph) error {
 
 	// Add the metadata to the cache.
 	v.mcache.packages[m.id] = m
-	v.mcache.ids[g.pkgPath] = m.id
 
 	// Connect the import graph.
 	if g.parent != nil {
@@ -302,7 +301,7 @@ func (v *view) link(ctx context.Context, g *importGraph) error {
 				parent:         m,
 				missingImports: g.missingImports,
 			}); err != nil {
-				log.Error(ctx, "error in dependecny", err)
+				log.Error(ctx, "error in dependency", err)
 			}
 		}
 	}

internal/lsp/cache/session.go

@@ -100,7 +100,6 @@ func (s *session) NewView(ctx context.Context, name string, folder span.URI, opt
 		filesByBase:   make(map[string][]viewFile),
 		mcache: &metadataCache{
 			packages: make(map[packageID]*metadata),
-			ids:      make(map[packagePath]packageID),
 		},
 		ignoredURIs: make(map[span.URI]struct{}),
 	}

internal/lsp/cache/view.go

@@ -85,7 +85,6 @@ type view struct {
 type metadataCache struct {
 	mu       sync.Mutex // guards both maps
 	packages map[packageID]*metadata
-	ids      map[packagePath]packageID
 }
 
 type metadata struct {
@@ -423,6 +422,7 @@ func (v *view) remove(ctx context.Context, id packageID, seen map[packageID]stru
 		if cph, ok := gof.pkgs[id]; ok {
 			// Delete the package handle from the store.
 			v.session.cache.store.Delete(checkPackageKey{
+				id:     cph.ID(),
 				files:  hashParseKeys(cph.Files()),
 				config: hashConfig(cph.Config()),
 			})

internal/lsp/source/diagnostics.go

@@ -69,10 +69,22 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[
 	ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(f.URI()))
 	defer done()
 
-	cph, err := f.GetCheckPackageHandle(ctx)
+	cphs, err := f.GetCheckPackageHandles(ctx)
 	if err != nil {
 		return nil, err
 	}
+	// Use the "biggest" package we know about.
+	// If we know about a package and its in-package tests,
+	// we should send diagnostics for both.
+	var cph CheckPackageHandle
+	for _, h := range cphs {
+		if cph == nil || len(h.Files()) > len(cph.Files()) {
+			cph = h
+		}
+	}
+	if cph == nil {
+		return nil, errors.Errorf("no package for file %s", f.URI())
+	}
 	pkg, err := cph.Check(ctx)
 	if err != nil {
 		log.Error(ctx, "no package for file", err)

internal/lsp/source/view.go

@@ -109,6 +109,9 @@ const (
 // CheckPackageHandle represents a handle to a specific version of a package.
 // It is uniquely defined by the file handles that make up the package.
 type CheckPackageHandle interface {
+	// ID returns the ID of the package associated with the CheckPackageHandle.
+	ID() string
+
 	// ParseGoHandle returns a ParseGoHandle for which to get the package.
 	Files() []ParseGoHandle