Skip to content

Commit

Permalink
internal/lsp: use dependencies in cache keys
Browse files Browse the repository at this point in the history 
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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
  • Loading branch information
@stamblerre
stamblerre committed Oct 3, 2019
1 parent 9ade4c7 commit c56b4b1
Show file tree
Hide file tree
Showing 9 changed files with 217 additions and 177 deletions.
197 changes: 104 additions & 93 deletions internal/lsp/cache/check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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()
Expand All @@ -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 {
Expand All @@ -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),
Expand All @@ -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))
Expand Down Expand Up @@ -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)

Expand All @@ -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 {
Expand All @@ -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,
}
}

Expand Down
Loading

0 comments on commit c56b4b1

Please sign in to comment.