From bc4e384f856e909b9ea0fcea24d662e254af38a3 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 14 Oct 2022 18:49:47 -0400 Subject: [PATCH] gopls/internal/lsp/cache: fix crash in analysis Since we removed the 'recover' in analysis, which swept crash bugs under the rug, we've seen a large number of crashes on the builders in which an analyzer crashes while trying to use the result of another analyzer (e.g. the inspector) on the same package. Logging shows that the "same" package is in fact a stale version of the same package. The root cause is that the key for the analysis cache is the analyzer name paired with the "recipe" for the package. However, analysis is not content with an equivalent package: identity matters for ast.Nodes, types.Objects, etc. This change attemps to fix the problem by, in effect, using the identity (not the recipe) of the package as part of the key. It is materialized by adding a store of promises to the pkg, and then using the analysis name as the key within this store. Change-Id: I057807d2781492a685f27c815250c3445e7475f9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/443100 gopls-CI: kokoro Reviewed-by: Robert Findley Run-TryBot: Alan Donovan TryBot-Result: Gopher Robot --- gopls/internal/lsp/cache/analysis.go | 17 ++++++++++------- gopls/internal/lsp/cache/pkg.go | 3 +++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index f54d0ed872a..c75b8de36b4 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/gopls/internal/lsp/cache/analysis.go @@ -66,8 +66,6 @@ type actionKey struct { analyzer *analysis.Analyzer } -type actionHandleKey source.Hash - // An action represents one unit of analysis work: the application of // one analysis to one package. Actions form a DAG, both within a // package (as different analyzers are applied, either in sequence or @@ -162,7 +160,16 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A } } - promise, release := s.store.Promise(buildActionKey(a, ph), func(ctx context.Context, arg interface{}) interface{} { + // The promises are kept in a store on the package, + // so the key need only include the analyzer name. + // + // (Since type-checking and analysis depend on the identity + // of packages--distinct packages produced by the same + // recipe are not fungible--we must in effect use the package + // itself as part of the key. Rather than actually use a pointer + // in the key, we get a simpler object graph if we shard the + // store by packages.) + promise, release := pkg.analyses.Promise(a.Name, func(ctx context.Context, arg interface{}) interface{} { res, err := actionImpl(ctx, arg.(*snapshot), deps, a, pkg) return actionResult{res, err} }) @@ -186,10 +193,6 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A return ah, nil } -func buildActionKey(a *analysis.Analyzer, ph *packageHandle) actionHandleKey { - return actionHandleKey(source.Hashf("%p%s", a, ph.key[:])) -} - func (key actionKey) String() string { return fmt.Sprintf("%s@%s", key.analyzer, key.pkgid) } diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go index 44fe855c0d3..0c9914d66fd 100644 --- a/gopls/internal/lsp/cache/pkg.go +++ b/gopls/internal/lsp/cache/pkg.go @@ -13,6 +13,7 @@ import ( "golang.org/x/mod/module" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/internal/memoize" ) // pkg contains the type information needed by the source package. @@ -30,6 +31,8 @@ type pkg struct { typesInfo *types.Info typesSizes types.Sizes hasFixedFiles bool // if true, AST was sufficiently mangled that we should hide type errors + + analyses memoize.Store // maps analyzer.Name to Promise[actionResult] } // A loadScope defines a package loading scope for use with go/packages.