From 59534d075a876675ef41a20fd596536cfa103662 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 11 Jun 2019 18:06:27 -0400 Subject: [PATCH] internal/lsp: use ids instead of package paths as map keys This adds an IDs map to the metadata cache, which maps package paths to IDs. This is only ever used by the Import function in the type checker. Change-Id: I8677d9439895bc6cbca5072e3fa9fddad4e165d5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/181683 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 36 +++++++++++++----------- internal/lsp/cache/gofile.go | 16 +++++------ internal/lsp/cache/load.go | 52 ++++++++++++++++++----------------- internal/lsp/cache/pkg.go | 4 +++ internal/lsp/cache/session.go | 5 ++-- internal/lsp/cache/view.go | 30 ++++++++++---------- internal/lsp/source/view.go | 1 + 7 files changed, 79 insertions(+), 65 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 99a4f67d..e9ae93b2 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -22,7 +22,7 @@ type importer struct { // 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. - seen map[packagePath]struct{} + seen map[packageID]struct{} // topLevelPkgID is the ID of the package from which type-checking began. topLevelPkgID packageID @@ -32,19 +32,23 @@ type importer struct { } func (imp *importer) Import(pkgPath string) (*types.Package, error) { - pkg, err := imp.getPkg(packagePath(pkgPath)) + id, ok := imp.view.mcache.ids[packagePath(pkgPath)] + if !ok { + return nil, fmt.Errorf("no known ID for %s", pkgPath) + } + pkg, err := imp.getPkg(id) if err != nil { return nil, err } return pkg.types, nil } -func (imp *importer) getPkg(pkgPath packagePath) (*pkg, error) { - if _, ok := imp.seen[pkgPath]; ok { +func (imp *importer) getPkg(id packageID) (*pkg, error) { + if _, ok := imp.seen[id]; ok { return nil, fmt.Errorf("circular import detected") } imp.view.pcache.mu.Lock() - e, ok := imp.view.pcache.packages[pkgPath] + e, ok := imp.view.pcache.packages[id] if ok { // cache hit @@ -54,12 +58,12 @@ func (imp *importer) getPkg(pkgPath packagePath) (*pkg, error) { } else { // cache miss e = &entry{ready: make(chan struct{})} - imp.view.pcache.packages[pkgPath] = e + imp.view.pcache.packages[id] = e imp.view.pcache.mu.Unlock() // This goroutine becomes responsible for populating // the entry and broadcasting its readiness. - e.pkg, e.err = imp.typeCheck(pkgPath) + e.pkg, e.err = imp.typeCheck(id) close(e.ready) } @@ -68,11 +72,11 @@ func (imp *importer) getPkg(pkgPath packagePath) (*pkg, error) { if e.err == context.Canceled && imp.ctx.Err() == nil { imp.view.pcache.mu.Lock() // Clear out canceled cache entry if it is still there. - if imp.view.pcache.packages[pkgPath] == e { - delete(imp.view.pcache.packages, pkgPath) + if imp.view.pcache.packages[id] == e { + delete(imp.view.pcache.packages, id) } imp.view.pcache.mu.Unlock() - return imp.getPkg(pkgPath) + return imp.getPkg(id) } return nil, e.err } @@ -80,10 +84,10 @@ func (imp *importer) getPkg(pkgPath packagePath) (*pkg, error) { return e.pkg, nil } -func (imp *importer) typeCheck(pkgPath packagePath) (*pkg, error) { - meta, ok := imp.view.mcache.packages[pkgPath] +func (imp *importer) typeCheck(id packageID) (*pkg, error) { + meta, ok := imp.view.mcache.packages[id] if !ok { - return nil, fmt.Errorf("no metadata for %v", pkgPath) + return nil, fmt.Errorf("no metadata for %v", id) } pkg := &pkg{ id: meta.id, @@ -123,11 +127,11 @@ func (imp *importer) typeCheck(pkgPath packagePath) (*pkg, error) { pkg.syntax = files // Handle circular imports by copying previously seen imports. - seen := make(map[packagePath]struct{}) + seen := make(map[packageID]struct{}) for k, v := range imp.seen { seen[k] = v } - seen[pkgPath] = struct{}{} + seen[id] = struct{}{} cfg := &types.Config{ Error: func(err error) { @@ -198,7 +202,7 @@ func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata) if err != nil { continue } - pkg.imports[importPath] = importPkg + pkg.imports[importPkg.pkgPath] = importPkg } } diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index a9dd25f4..1be2ffdf 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -131,9 +131,9 @@ func (f *goFile) GetActiveReverseDeps(ctx context.Context) []source.GoFile { f.view.mcache.mu.Lock() defer f.view.mcache.mu.Unlock() - seen := make(map[packagePath]struct{}) // visited packages + seen := make(map[packageID]struct{}) // visited packages results := make(map[*goFile]struct{}) - f.view.reverseDeps(ctx, seen, results, packagePath(pkg.PkgPath())) + f.view.reverseDeps(ctx, seen, results, packageID(pkg.ID())) var files []source.GoFile for rd := range results { @@ -149,12 +149,12 @@ func (f *goFile) GetActiveReverseDeps(ctx context.Context) []source.GoFile { return files } -func (v *view) reverseDeps(ctx context.Context, seen map[packagePath]struct{}, results map[*goFile]struct{}, pkgPath packagePath) { - if _, ok := seen[pkgPath]; ok { +func (v *view) reverseDeps(ctx context.Context, seen map[packageID]struct{}, results map[*goFile]struct{}, id packageID) { + if _, ok := seen[id]; ok { return } - seen[pkgPath] = struct{}{} - m, ok := v.mcache.packages[pkgPath] + seen[id] = struct{}{} + m, ok := v.mcache.packages[id] if !ok { return } @@ -164,7 +164,7 @@ func (v *view) reverseDeps(ctx context.Context, seen map[packagePath]struct{}, r results[f.(*goFile)] = struct{}{} } } - for parentPkgPath := range m.parents { - v.reverseDeps(ctx, seen, results, parentPkgPath) + for parentID := range m.parents { + v.reverseDeps(ctx, seen, results, parentID) } } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 71da94c9..c8963fd8 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -18,7 +18,6 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er if f.astIsTrimmed() { f.invalidateAST() } - // Save the metadata's current missing imports, if any. var originalMissingImports map[packagePath]struct{} if f.meta != nil { @@ -37,21 +36,19 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er if sameSet(originalMissingImports, f.meta.missingImports) && f.pkg != nil { return nil, nil } - imp := &importer{ view: v, - seen: make(map[packagePath]struct{}), + seen: make(map[packageID]struct{}), ctx: ctx, fset: f.FileSet(), topLevelPkgID: f.meta.id, } - // Start prefetching direct imports. - for importPath := range f.meta.children { - go imp.Import(string(importPath)) + for importID := range f.meta.children { + go imp.getPkg(importID) } // Type-check package. - pkg, err := imp.getPkg(f.meta.pkgPath) + pkg, err := imp.getPkg(f.meta.id) if err != nil { return nil, err } @@ -137,24 +134,26 @@ func (v *view) parseImports(ctx context.Context, f *goFile) bool { } func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) *metadata { - m, ok := v.mcache.packages[pkgPath] + id := packageID(pkg.ID) + m, ok := v.mcache.packages[id] // If a file was added or deleted we need to invalidate the package cache // so relevant packages get parsed and type checked again. if ok && !filenamesIdentical(m.files, pkg.CompiledGoFiles) { - v.invalidatePackage(pkgPath) + v.invalidatePackage(id) } - + // If we haven't seen this package before. if !ok { m = &metadata{ pkgPath: pkgPath, - id: packageID(pkg.ID), + id: id, typesSizes: pkg.TypesSizes, - parents: make(map[packagePath]bool), - children: make(map[packagePath]bool), + parents: make(map[packageID]bool), + children: make(map[packageID]bool), missingImports: make(map[packagePath]struct{}), } - v.mcache.packages[pkgPath] = m + v.mcache.packages[id] = m + v.mcache.ids[pkgPath] = id } // Reset any field that could have changed across calls to packages.Load. m.name = pkg.Name @@ -170,26 +169,29 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack } // Connect the import graph. if parent != nil { - m.parents[parent.pkgPath] = true - parent.children[pkgPath] = true + m.parents[parent.id] = true + parent.children[id] = true } for importPath, importPkg := range pkg.Imports { if len(importPkg.Errors) > 0 { m.missingImports[pkgPath] = struct{}{} } - importPkgPath := packagePath(importPath) - if _, ok := m.children[importPkgPath]; !ok { - v.link(ctx, importPkgPath, importPkg, m) + if _, ok := m.children[packageID(importPkg.ID)]; !ok { + v.link(ctx, packagePath(importPath), importPkg, m) } } // Clear out any imports that have been removed. - for importPath := range m.children { - if _, ok := pkg.Imports[string(importPath)]; !ok { - delete(m.children, importPath) - if child, ok := v.mcache.packages[importPath]; ok { - delete(child.parents, pkgPath) - } + for importID := range m.children { + child, ok := v.mcache.packages[importID] + if !ok { + continue } + importPath := string(child.pkgPath) + if _, ok := pkg.Imports[importPath]; ok { + continue + } + delete(m.children, importID) + delete(child.parents, id) } return m } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 7a847857..b50011bf 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -137,6 +137,10 @@ func (pkg *pkg) GetActionGraph(ctx context.Context, a *analysis.Analyzer) (*sour return e.Action, nil } +func (pkg *pkg) ID() string { + return string(pkg.id) +} + func (pkg *pkg) PkgPath() string { return string(pkg.pkgPath) } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 5fc40319..8d6819ab 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -81,10 +81,11 @@ func (s *session) NewView(name string, folder span.URI) source.View { filesByURI: make(map[span.URI]viewFile), filesByBase: make(map[string][]viewFile), mcache: &metadataCache{ - packages: make(map[packagePath]*metadata), + packages: make(map[packageID]*metadata), + ids: make(map[packagePath]packageID), }, pcache: &packageCache{ - packages: make(map[packagePath]*entry), + packages: make(map[packageID]*entry), }, ignoredURIs: make(map[span.URI]struct{}), } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index c7ef8282..53a81e7e 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -70,8 +70,9 @@ type view struct { } type metadataCache struct { - mu sync.Mutex - packages map[packagePath]*metadata + mu sync.Mutex // guards both maps + packages map[packageID]*metadata + ids map[packagePath]packageID } type metadata struct { @@ -80,7 +81,7 @@ type metadata struct { name string files []string typesSizes types.Sizes - parents, children map[packagePath]bool + parents, children map[packageID]bool // missingImports is the set of unresolved imports for this package. // It contains any packages with `go list` errors. @@ -89,7 +90,7 @@ type metadata struct { type packageCache struct { mu sync.Mutex - packages map[packagePath]*entry + packages map[packageID]*entry } type entry struct { @@ -247,32 +248,33 @@ func (f *goFile) invalidateAST() { // Remove the package and all of its reverse dependencies from the cache. if f.pkg != nil { - f.view.remove(f.pkg.pkgPath, map[packagePath]struct{}{}) + f.view.remove(f.pkg.id, map[packageID]struct{}{}) } } // invalidatePackage removes the specified package and dependents from the // package cache. -func (v *view) invalidatePackage(pkgPath packagePath) { +func (v *view) invalidatePackage(id packageID) { v.pcache.mu.Lock() defer v.pcache.mu.Unlock() - v.remove(pkgPath, make(map[packagePath]struct{})) + + v.remove(id, make(map[packageID]struct{})) } // remove invalidates a package and its reverse dependencies in the view's // package cache. It is assumed that the caller has locked both the mutexes // of both the mcache and the pcache. -func (v *view) remove(pkgPath packagePath, seen map[packagePath]struct{}) { - if _, ok := seen[pkgPath]; ok { +func (v *view) remove(id packageID, seen map[packageID]struct{}) { + if _, ok := seen[id]; ok { return } - m, ok := v.mcache.packages[pkgPath] + m, ok := v.mcache.packages[id] if !ok { return } - seen[pkgPath] = struct{}{} - for parentPkgPath := range m.parents { - v.remove(parentPkgPath, seen) + seen[id] = struct{}{} + for parentID := range m.parents { + v.remove(parentID, seen) } // All of the files in the package may also be holding a pointer to the // invalidated package. @@ -283,7 +285,7 @@ func (v *view) remove(pkgPath packagePath, seen map[packagePath]struct{}) { } } } - delete(v.pcache.packages, pkgPath) + delete(v.pcache.packages, id) } // FindFile returns the file if the given URI is already a part of the view. diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index a88bc471..7447cba4 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -220,6 +220,7 @@ type SumFile interface { // Package represents a Go package that has been type-checked. It maintains // only the relevant fields of a *go/packages.Package. type Package interface { + ID() string PkgPath() string GetFilenames() []string GetSyntax() []*ast.File