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 <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: adonovan

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

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.