From 3e6e7c4239da85388d17ae22a34eb67c86525d72 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 10 Sep 2019 17:33:35 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 6 ++++++ internal/lsp/cache/load.go | 3 +-- internal/lsp/cache/session.go | 1 - internal/lsp/cache/view.go | 2 +- internal/lsp/source/diagnostics.go | 14 +++++++++++++- internal/lsp/source/view.go | 3 +++ 6 files changed, 24 insertions(+), 5 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 558836d3db..b9bae20cfe 100644 --- a/internal/lsp/cache/check.go +++ b/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 { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 99a2078d93..a5ee4196ba 100644 --- a/internal/lsp/cache/load.go +++ b/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) } } } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 4d4281da79..605297693a 100644 --- a/internal/lsp/cache/session.go +++ b/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{}), } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index f235aa3ca0..f8d7a36ea9 100644 --- a/internal/lsp/cache/view.go +++ b/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()), }) diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 70108f3901..5cb5559b8b 100644 --- a/internal/lsp/source/diagnostics.go +++ b/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) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 4d7ba3e1c4..dbd8f2b133 100644 --- a/internal/lsp/source/view.go +++ b/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