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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

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) {
+func (imp *importer) getPkg(id packageID) (*pkg, error) {
-	if _, ok := imp.seen[pkgPath]; ok {
+	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 {
+			if imp.view.pcache.packages[id] == e {
-				delete(imp.view.pcache.packages, pkgPath)
+				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) {
+func (imp *importer) typeCheck(id packageID) (*pkg, error) {
-	meta, ok := imp.view.mcache.packages[pkgPath]
+	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
 	}
 }
 

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) {
+func (v *view) reverseDeps(ctx context.Context, seen map[packageID]struct{}, results map[*goFile]struct{}, id packageID) {
-	if _, ok := seen[pkgPath]; ok {
+	if _, ok := seen[id]; ok {
 		return
 	}
-	seen[pkgPath] = struct{}{}
+	seen[id] = struct{}{}
-	m, ok := v.mcache.packages[pkgPath]
+	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 {
+	for parentID := range m.parents {
-		v.reverseDeps(ctx, seen, results, parentPkgPath)
+		v.reverseDeps(ctx, seen, results, parentID)
 	}
 }

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 {
+	for importID := range f.meta.children {
-		go imp.Import(string(importPath))
+		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),
+			parents:        make(map[packageID]bool),
-			children:       make(map[packagePath]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
+		m.parents[parent.id] = true
-		parent.children[pkgPath] = 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[packageID(importPkg.ID)]; !ok {
-		if _, ok := m.children[importPkgPath]; !ok {
+			v.link(ctx, packagePath(importPath), importPkg, m)
-			v.link(ctx, importPkgPath, importPkg, m)
 		}
 	}
 	// Clear out any imports that have been removed.
-	for importPath := range m.children {
+	for importID := range m.children {
-		if _, ok := pkg.Imports[string(importPath)]; !ok {
+		child, ok := v.mcache.packages[importID]
-			delete(m.children, importPath)
+		if !ok {
-			if child, ok := v.mcache.packages[importPath]; ok {
+			continue
-				delete(child.parents, pkgPath)
 		}
+		importPath := string(child.pkgPath)
+		if _, ok := pkg.Imports[importPath]; ok {
+			continue
 		}
+		delete(m.children, importID)
+		delete(child.parents, id)
 	}
 	return m
 }

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

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

internal/lsp/cache/view.go

@@ -70,8 +70,9 @@ type view struct {
 }
 
 type metadataCache struct {
-	mu       sync.Mutex
+	mu       sync.Mutex // guards both maps
-	packages map[packagePath]*metadata
+	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{}) {
+func (v *view) remove(id packageID, seen map[packageID]struct{}) {
-	if _, ok := seen[pkgPath]; ok {
+	if _, ok := seen[id]; ok {
 		return
 	}
-	m, ok := v.mcache.packages[pkgPath]
+	m, ok := v.mcache.packages[id]
 	if !ok {
 		return
 	}
-	seen[pkgPath] = struct{}{}
+	seen[id] = struct{}{}
-	for parentPkgPath := range m.parents {
+	for parentID := range m.parents {
-		v.remove(parentPkgPath, seen)
+		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.

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