internal/lsp: use dependencies in cache keys

This change includes dependencies in the cache keys for
CheckPackageHandles. This should fix the issue with propagating results
to reverse dependencies.

Updates golang/go#34410

Change-Id: I025b7f0d6b0dcaa89c3461ebd94eadd35de4955f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198317
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>

Author: stamblerre

internal/lsp/cache/check.go

@@ -7,25 +7,25 @@ package cache
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"go/ast"
 	"go/scanner"
 	"go/types"
+	"sort"
 	"sync"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/memoize"
-	"golang.org/x/tools/internal/telemetry/log"
 	"golang.org/x/tools/internal/telemetry/trace"
 	errors "golang.org/x/xerrors"
 )
 
 type importer struct {
 	snapshot *snapshot
 	ctx      context.Context
-	config   *packages.Config
 
 	// seen maintains the set of previously imported packages.
 	// If we have seen a package that is already in this map, we have a circular import.
@@ -41,38 +41,31 @@ type importer struct {
 	parentCheckPackageHandle *checkPackageHandle
 }
 
-// checkPackageKey uniquely identifies a package and its config.
-type checkPackageKey struct {
-	id     string
-	files  string
-	config string
-
-	// TODO: For now, we don't include dependencies in the key.
-	// This will be necessary when we change the cache invalidation logic.
-}
-
 // checkPackageHandle implements source.CheckPackageHandle.
 type checkPackageHandle struct {
 	handle *memoize.Handle
 
-	files   []source.ParseGoHandle
-	imports map[packagePath]*checkPackageHandle
+	// files are the ParseGoHandles that compose the package.
+	files []source.ParseGoHandle
+
+	// mode is the mode the the files were parsed in.
+	mode source.ParseMode
 
-	m      *metadata
-	config *packages.Config
+	// imports is the map of the package's imports.
+	imports map[packagePath]packageID
+
+	// m is the metadata associated with the package.
+	m *metadata
+
+	// key is the hashed key for the package.
+	key []byte
 }
 
-func (cph *checkPackageHandle) Mode() source.ParseMode {
-	if len(cph.files) == 0 {
-		return -1
-	}
-	mode := cph.files[0].Mode()
-	for _, ph := range cph.files[1:] {
-		if ph.Mode() != mode {
-			return -1
-		}
+func (cph *checkPackageHandle) packageKey() packageKey {
+	return packageKey{
+		id:   cph.m.id,
+		mode: cph.mode,
 	}
-	return mode
 }
 
 // checkPackageData contains the data produced by type-checking a package.
@@ -84,38 +77,82 @@ type checkPackageData struct {
 }
 
 // checkPackageHandle returns a source.CheckPackageHandle for a given package and config.
-func (imp *importer) checkPackageHandle(ctx context.Context, id packageID, s *snapshot) (*checkPackageHandle, error) {
-	m := s.getMetadata(id)
+func (imp *importer) checkPackageHandle(ctx context.Context, id packageID) (*checkPackageHandle, error) {
+	// Determine the mode that the files should be parsed in.
+	mode := imp.mode(id)
+
+	// Check if we already have this CheckPackageHandle cached.
+	if cph := imp.snapshot.getPackage(id, mode); cph != nil {
+		return cph, nil
+	}
+
+	// Build the CheckPackageHandle for this ID and its dependencies.
+	cph, err := imp.buildKey(ctx, id, mode)
+	if err != nil {
+		return nil, err
+	}
+
+	h := imp.snapshot.view.session.cache.store.Bind(string(cph.key), func(ctx context.Context) interface{} {
+		data := &checkPackageData{}
+		data.pkg, data.err = imp.typeCheck(ctx, cph)
+		return data
+	})
+	cph.handle = h
+
+	return cph, nil
+}
+
+// buildKey computes the checkPackageKey for a given checkPackageHandle.
+func (imp *importer) buildKey(ctx context.Context, id packageID, mode source.ParseMode) (*checkPackageHandle, error) {
+	m := imp.snapshot.getMetadata(id)
 	if m == nil {
 		return nil, errors.Errorf("no metadata for %s", id)
 	}
-	phs, err := imp.parseGoHandles(ctx, m)
+
+	phs, err := imp.parseGoHandles(ctx, m, mode)
 	if err != nil {
-		log.Error(ctx, "no ParseGoHandles", err, telemetry.Package.Of(id))
 		return nil, err
 	}
 	cph := &checkPackageHandle{
 		m:       m,
 		files:   phs,
-		config:  imp.config,
-		imports: make(map[packagePath]*checkPackageHandle),
+		imports: make(map[packagePath]packageID),
+		mode:    mode,
 	}
-	h := imp.snapshot.view.session.cache.store.Bind(cph.key(), func(ctx context.Context) interface{} {
-		data := &checkPackageData{}
-		data.pkg, data.err = imp.typeCheck(ctx, cph, m)
-		return data
+
+	// Make sure all of the deps are sorted.
+	deps := append([]packageID{}, m.deps...)
+	sort.Slice(deps, func(i, j int) bool {
+		return deps[i] < deps[j]
 	})
 
-	cph.handle = h
+	// Create the dep importer for use on the dependency handles.
+	depImporter := &importer{
+		snapshot:          imp.snapshot,
+		topLevelPackageID: imp.topLevelPackageID,
+	}
+	// Begin computing the key by getting the depKeys for all dependencies.
+	var depKeys [][]byte
+	for _, dep := range deps {
+		depHandle, err := depImporter.checkPackageHandle(ctx, dep)
+		if err != nil {
+			return nil, errors.Errorf("no dep handle for %s: %+v", dep, err)
+		}
+		cph.imports[depHandle.m.pkgPath] = depHandle.m.id
+		depKeys = append(depKeys, depHandle.key)
+	}
+	cph.key = checkPackageKey(cph.m.id, cph.files, m.config, depKeys)
 
 	// Cache the CheckPackageHandle in the snapshot.
-	for _, ph := range cph.files {
-		uri := ph.File().Identity().URI
-		s.addPackage(uri, cph)
-	}
+	imp.snapshot.addPackage(cph)
+
 	return cph, nil
 }
 
+func checkPackageKey(id packageID, phs []source.ParseGoHandle, cfg *packages.Config, deps [][]byte) []byte {
+	return []byte(hashContents([]byte(fmt.Sprintf("%s%s%s%s", id, hashParseKeys(phs), hashConfig(cfg), hashContents(bytes.Join(deps, nil))))))
+}
+
 // hashConfig returns the hash for the *packages.Config.
 func hashConfig(config *packages.Config) string {
 	b := bytes.NewBuffer(nil)
@@ -143,16 +180,12 @@ func (cph *checkPackageHandle) check(ctx context.Context) (*pkg, error) {
 
 	v := cph.handle.Get(ctx)
 	if v == nil {
-		return nil, ctx.Err()
+		return nil, errors.Errorf("no package for %s", cph.m.id)
 	}
 	data := v.(*checkPackageData)
 	return data.pkg, data.err
 }
 
-func (cph *checkPackageHandle) Config() *packages.Config {
-	return cph.config
-}
-
 func (cph *checkPackageHandle) Files() []source.ParseGoHandle {
 	return cph.files
 }
@@ -182,31 +215,26 @@ func (cph *checkPackageHandle) cached(ctx context.Context) (*pkg, error) {
 	return data.pkg, data.err
 }
 
-func (cph *checkPackageHandle) key() checkPackageKey {
-	return checkPackageKey{
-		id:     string(cph.m.id),
-		files:  hashParseKeys(cph.files),
-		config: hashConfig(cph.config),
-	}
-}
-
-func (imp *importer) parseGoHandles(ctx context.Context, m *metadata) ([]source.ParseGoHandle, error) {
+func (imp *importer) parseGoHandles(ctx context.Context, m *metadata, mode source.ParseMode) ([]source.ParseGoHandle, error) {
 	phs := make([]source.ParseGoHandle, 0, len(m.files))
 	for _, uri := range m.files {
 		f, err := imp.snapshot.view.GetFile(ctx, uri)
 		if err != nil {
 			return nil, err
 		}
 		fh := imp.snapshot.Handle(ctx, f)
-		mode := source.ParseExported
-		if imp.topLevelPackageID == m.id {
-			mode = source.ParseFull
-		}
 		phs = append(phs, imp.snapshot.view.session.cache.ParseGoHandle(fh, mode))
 	}
 	return phs, nil
 }
 
+func (imp *importer) mode(id packageID) source.ParseMode {
+	if imp.topLevelPackageID == id {
+		return source.ParseFull
+	}
+	return source.ParseExported
+}
+
 func (imp *importer) Import(pkgPath string) (*types.Package, error) {
 	ctx, done := trace.StartSpan(imp.ctx, "cache.importer.Import", telemetry.PackagePath.Of(pkgPath))
 	defer done()
@@ -215,16 +243,14 @@ func (imp *importer) Import(pkgPath string) (*types.Package, error) {
 	if imp.parentPkg == nil {
 		return nil, errors.Errorf("no parent package for import %s", pkgPath)
 	}
-
 	// Get the CheckPackageHandle from the importing package.
-	cph, ok := imp.parentCheckPackageHandle.imports[packagePath(pkgPath)]
+	id, ok := imp.parentCheckPackageHandle.imports[packagePath(pkgPath)]
 	if !ok {
 		return nil, errors.Errorf("no package data for import path %s", pkgPath)
 	}
-	for _, ph := range cph.Files() {
-		if ph.Mode() != source.ParseExported {
-			panic("dependency parsed in full mode")
-		}
+	cph := imp.snapshot.getPackage(id, source.ParseExported)
+	if cph == nil {
+		return nil, errors.Errorf("no package for %s", id)
 	}
 	pkg, err := cph.check(ctx)
 	if err != nil {
@@ -234,17 +260,17 @@ func (imp *importer) Import(pkgPath string) (*types.Package, error) {
 	return pkg.GetTypes(), nil
 }
 
-func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle, m *metadata) (*pkg, error) {
-	ctx, done := trace.StartSpan(ctx, "cache.importer.typeCheck", telemetry.Package.Of(m.id))
+func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*pkg, error) {
+	ctx, done := trace.StartSpan(ctx, "cache.importer.typeCheck", telemetry.Package.Of(cph.m.id))
 	defer done()
 
 	pkg := &pkg{
 		view:       imp.snapshot.view,
-		id:         m.id,
-		pkgPath:    m.pkgPath,
+		id:         cph.m.id,
+		pkgPath:    cph.m.pkgPath,
 		files:      cph.Files(),
 		imports:    make(map[packagePath]*pkg),
-		typesSizes: m.typesSizes,
+		typesSizes: cph.m.typesSizes,
 		typesInfo: &types.Info{
 			Types:      make(map[ast.Expr]types.TypeAndValue),
 			Defs:       make(map[*ast.Ident]types.Object),
@@ -257,22 +283,8 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle, m *
 	}
 	// If the package comes back with errors from `go list`,
 	// don't bother type-checking it.
-	for _, err := range m.errors {
-		pkg.errors = append(m.errors, err)
-	}
-	// Set imports of package to correspond to cached packages.
-	cimp := imp.child(ctx, pkg, cph)
-	for _, depID := range m.deps {
-		dep := imp.snapshot.getMetadata(depID)
-		if dep == nil {
-			continue
-		}
-		depHandle, err := cimp.checkPackageHandle(ctx, depID, imp.snapshot)
-		if err != nil {
-			log.Error(ctx, "no check package handle", err, telemetry.Package.Of(depID))
-			continue
-		}
-		cph.imports[dep.pkgPath] = depHandle
+	for _, err := range cph.m.errors {
+		pkg.errors = append(cph.m.errors, err)
 	}
 	var (
 		files       = make([]*ast.File, len(pkg.files))
@@ -305,19 +317,19 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle, m *
 	files = files[:i]
 
 	// Use the default type information for the unsafe package.
-	if m.pkgPath == "unsafe" {
+	if cph.m.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", pkg.pkgPath)
 	} else {
-		pkg.types = types.NewPackage(string(m.pkgPath), m.name)
+		pkg.types = types.NewPackage(string(cph.m.pkgPath), cph.m.name)
 	}
 
 	cfg := &types.Config{
 		Error: func(err error) {
 			imp.snapshot.view.session.cache.appendPkgError(pkg, err)
 		},
-		Importer: cimp,
+		Importer: imp.depImporter(ctx, cph, pkg),
 	}
 	check := types.NewChecker(cfg, imp.snapshot.view.session.cache.FileSet(), pkg.types, pkg.typesInfo)
 
@@ -327,7 +339,7 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle, m *
 	return pkg, nil
 }
 
-func (imp *importer) child(ctx context.Context, pkg *pkg, cph *checkPackageHandle) *importer {
+func (imp *importer) depImporter(ctx context.Context, cph *checkPackageHandle, pkg *pkg) *importer {
 	// Handle circular imports by copying previously seen imports.
 	seen := make(map[packageID]struct{})
 	for k, v := range imp.seen {
@@ -336,12 +348,11 @@ func (imp *importer) child(ctx context.Context, pkg *pkg, cph *checkPackageHandl
 	seen[pkg.id] = struct{}{}
 	return &importer{
 		snapshot:                 imp.snapshot,
-		ctx:                      ctx,
-		config:                   imp.config,
-		seen:                     seen,
 		topLevelPackageID:        imp.topLevelPackageID,
 		parentPkg:                pkg,
 		parentCheckPackageHandle: cph,
+		seen:                     seen,
+		ctx:                      ctx,
 	}
 }