internal/imports: cache GOPATH, exports

We intend to use the GOPATH resolver's results during LSP
autocompletion. That means we have to be able to cache its data, same as
we do for modules. Convert it to use a dirInfoCache.

Cache exports in the dirInfoCache. Along the way, store exports as slices
rather than maps. We don't need the extra structure the vast majority of
the time, and the memory overhead is nontrivial.

Change-Id: If267d6b00da2163a960b93b2cf2088ec2538f73d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205162
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/imports/fix.go

@@ -683,11 +683,11 @@ func getPackageExports(completePackage, filename string, env *ProcessEnv) ([]Pac
 			IdentName: pkg.packageName,
 			FixType:   AddImport,
 		}
-		var exportsMap map[string]bool
+		var exports []string
 		if e, ok := stdlib[pkg.importPathShort]; ok {
-			exportsMap = e
+			exports = e
 		} else {
-			exportsMap, err = env.GetResolver().loadExports(context.TODO(), completePackage, pkg)
+			exports, err = loadExportsForPackage(context.Background(), env, completePackage, pkg)
 			if err != nil {
 				if env.Debug {
 					env.Logf("while completing %q, error loading exports from %q: %v", completePackage, pkg.importPathShort, err)
@@ -695,10 +695,6 @@ func getPackageExports(completePackage, filename string, env *ProcessEnv) ([]Pac
 				continue
 			}
 		}
-		var exports []string
-		for export := range exportsMap {
-			exports = append(exports, export)
-		}
 		sort.Strings(exports)
 		results = append(results, PackageExport{
 			Fix:     fix,
@@ -848,9 +844,8 @@ type Resolver interface {
 	// could not be determined will be excluded.
 	scan(refs references, loadNames bool, exclude []gopathwalk.RootType) ([]*pkg, error)
 	// loadExports returns the set of exported symbols in the package at dir.
-	// It returns an error if the package name in dir does not match expectPackage.
 	// loadExports may be called concurrently.
-	loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[string]bool, error)
+	loadExports(ctx context.Context, pkg *pkg) (string, []string, error)
 }
 
 // gopackagesResolver implements resolver for GOPATH and module workspaces using go/packages.
@@ -906,24 +901,24 @@ func (r *goPackagesResolver) scan(refs references, _ bool, _ []gopathwalk.RootTy
 	return scan, nil
 }
 
-func (r *goPackagesResolver) loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[string]bool, error) {
+func (r *goPackagesResolver) loadExports(ctx context.Context, pkg *pkg) (string, []string, error) {
 	if pkg.goPackage == nil {
-		return nil, fmt.Errorf("goPackage not set")
+		return "", nil, fmt.Errorf("goPackage not set")
 	}
-	exports := map[string]bool{}
+	var exports []string
 	fset := token.NewFileSet()
 	for _, fname := range pkg.goPackage.CompiledGoFiles {
 		f, err := parser.ParseFile(fset, fname, nil, 0)
 		if err != nil {
-			return nil, fmt.Errorf("parsing %s: %v", fname, err)
+			return "", nil, fmt.Errorf("parsing %s: %v", fname, err)
 		}
 		for name := range f.Scope.Objects {
 			if ast.IsExported(name) {
-				exports[name] = true
+				exports = append(exports, name)
 			}
 		}
 	}
-	return exports, nil
+	return pkg.goPackage.Name, exports, nil
 }
 
 func addExternalCandidates(pass *pass, refs references, filename string) error {
@@ -1026,9 +1021,19 @@ func importPathToAssumedName(importPath string) string {
 // gopathResolver implements resolver for GOPATH workspaces.
 type gopathResolver struct {
 	env   *ProcessEnv
+	cache *dirInfoCache
+}
+
+func (r *gopathResolver) init() {
+	if r.cache == nil {
+		r.cache = &dirInfoCache{
+			dirs: map[string]*directoryPackageInfo{},
+		}
+	}
 }
 
 func (r *gopathResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) {
+	r.init()
 	names := map[string]string{}
 	for _, path := range importPaths {
 		names[path] = importPathToName(r.env, path, srcDir)
@@ -1157,39 +1162,50 @@ func distance(basepath, targetpath string) int {
 }
 
 func (r *gopathResolver) scan(_ references, loadNames bool, exclude []gopathwalk.RootType) ([]*pkg, error) {
-	dupCheck := make(map[string]bool)
-	var result []*pkg
-
-	var mu sync.Mutex
-
+	r.init()
 	add := func(root gopathwalk.Root, dir string) {
-		mu.Lock()
-		defer mu.Unlock()
-
-		if _, dup := dupCheck[dir]; dup {
+		// We assume cached directories have not changed. We can skip them and their
+		// children.
+		if _, ok := r.cache.Load(dir); ok {
 			return
 		}
-		dupCheck[dir] = true
+
 		importpath := filepath.ToSlash(dir[len(root.Path)+len("/"):])
-		p := &pkg{
-			importPathShort: VendorlessPath(importpath),
+		info := directoryPackageInfo{
+			status:                 directoryScanned,
 			dir:                    dir,
-			relevance:       1,
+			rootType:               root.Type,
+			nonCanonicalImportPath: VendorlessPath(importpath),
 		}
-		if root.Type == gopathwalk.RootGOROOT {
-			p.relevance = 0
-		}
-		if loadNames {
-			var err error
-			p.packageName, err = packageDirToName(dir)
-			if err != nil {
-				return // Typically an unimportable package like main.
-			}
-		}
-		result = append(result, p)
+		r.cache.Store(dir, info)
 	}
 	roots := filterRoots(gopathwalk.SrcDirsRoots(r.env.buildContext()), exclude)
 	gopathwalk.Walk(roots, add, gopathwalk.Options{Debug: r.env.Debug, ModulesEnabled: false})
+	var result []*pkg
+	for _, dir := range r.cache.Keys() {
+		info, ok := r.cache.Load(dir)
+		if !ok {
+			continue
+		}
+		if loadNames {
+			var err error
+			info, err = r.cache.CachePackageName(info)
+			if err != nil {
+				continue
+			}
+		}
+
+		p := &pkg{
+			importPathShort: info.nonCanonicalImportPath,
+			dir:             dir,
+			relevance:       1,
+			packageName:     info.packageName,
+		}
+		if info.rootType == gopathwalk.RootGOROOT {
+			p.relevance = 0
+		}
+		result = append(result, p)
+	}
 	return result, nil
 }
 
@@ -1207,8 +1223,12 @@ outer:
 	return result
 }
 
-func (r *gopathResolver) loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[string]bool, error) {
-	return loadExportsFromFiles(ctx, r.env, expectPackage, pkg.dir)
+func (r *gopathResolver) loadExports(ctx context.Context, pkg *pkg) (string, []string, error) {
+	r.init()
+	if info, ok := r.cache.Load(pkg.dir); ok {
+		return r.cache.CacheExports(ctx, r.env, info)
+	}
+	return loadExportsFromFiles(ctx, r.env, pkg.dir)
 }
 
 // VendorlessPath returns the devendorized version of the import path ipath.
@@ -1224,13 +1244,13 @@ func VendorlessPath(ipath string) string {
 	return ipath
 }
 
-func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, expectPackage string, dir string) (map[string]bool, error) {
-	exports := make(map[string]bool)
+func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string) (string, []string, error) {
+	var exports []string
 
 	// Look for non-test, buildable .go files which could provide exports.
 	all, err := ioutil.ReadDir(dir)
 	if err != nil {
-		return nil, err
+		return "", nil, err
 	}
 	var files []os.FileInfo
 	for _, fi := range all {
@@ -1246,47 +1266,42 @@ func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, expectPackage st
 	}
 
 	if len(files) == 0 {
-		return nil, fmt.Errorf("dir %v contains no buildable, non-test .go files", dir)
+		return "", nil, fmt.Errorf("dir %v contains no buildable, non-test .go files", dir)
 	}
 
+	var pkgName string
 	fset := token.NewFileSet()
 	for _, fi := range files {
 		select {
 		case <-ctx.Done():
-			return nil, ctx.Err()
+			return "", nil, ctx.Err()
 		default:
 		}
 
 		fullFile := filepath.Join(dir, fi.Name())
 		f, err := parser.ParseFile(fset, fullFile, nil, 0)
 		if err != nil {
-			return nil, fmt.Errorf("parsing %s: %v", fullFile, err)
+			return "", nil, fmt.Errorf("parsing %s: %v", fullFile, err)
 		}
-		pkgName := f.Name.Name
-		if pkgName == "documentation" {
+		if f.Name.Name == "documentation" {
 			// Special case from go/build.ImportDir, not
 			// handled by MatchFile above.
 			continue
 		}
-		if pkgName != expectPackage {
-			return nil, fmt.Errorf("scan of dir %v is not expected package %v (actually %v)", dir, expectPackage, pkgName)
-		}
+		pkgName = f.Name.Name
 		for name := range f.Scope.Objects {
 			if ast.IsExported(name) {
-				exports[name] = true
+				exports = append(exports, name)
 			}
 		}
 	}
 
 	if env.Debug {
-		exportList := make([]string, 0, len(exports))
-		for k := range exports {
-			exportList = append(exportList, k)
+		sortedExports := append([]string(nil), exports...)
+		sort.Strings(sortedExports)
+		env.Logf("loaded exports in dir %v (package %v): %v", dir, pkgName, strings.Join(sortedExports, ", "))
 	}
-		sort.Strings(exportList)
-		env.Logf("loaded exports in dir %v (package %v): %v", dir, expectPackage, strings.Join(exportList, ", "))
-	}
-	return exports, nil
+	return pkgName, exports, nil
 }
 
 // findImport searches for a package with the given symbols.
@@ -1361,7 +1376,7 @@ func findImport(ctx context.Context, pass *pass, dirScan []*pkg, pkgName string,
 				if pass.env.Debug {
 					pass.env.Logf("loading exports in dir %s (seeking package %s)", c.pkg.dir, pkgName)
 				}
-				exports, err := pass.env.GetResolver().loadExports(ctx, pkgName, c.pkg)
+				exports, err := loadExportsForPackage(ctx, pass.env, pkgName, c.pkg)
 				if err != nil {
 					if pass.env.Debug {
 						pass.env.Logf("loading exports in dir %s (seeking package %s): %v", c.pkg.dir, pkgName, err)
@@ -1370,10 +1385,15 @@ func findImport(ctx context.Context, pass *pass, dirScan []*pkg, pkgName string,
 					return
 				}
 
+				exportsMap := make(map[string]bool, len(exports))
+				for _, sym := range exports {
+					exportsMap[sym] = true
+				}
+
 				// If it doesn't have the right
 				// symbols, send nil to mean no match.
 				for symbol := range symbols {
-					if !exports[symbol] {
+					if !exportsMap[symbol] {
 						resc <- nil
 						return
 					}
@@ -1393,6 +1413,17 @@ func findImport(ctx context.Context, pass *pass, dirScan []*pkg, pkgName string,
 	return nil, nil
 }
 
+func loadExportsForPackage(ctx context.Context, env *ProcessEnv, expectPkg string, pkg *pkg) ([]string, error) {
+	pkgName, exports, err := env.GetResolver().loadExports(ctx, pkg)
+	if err != nil {
+		return nil, err
+	}
+	if expectPkg != pkgName {
+		return nil, fmt.Errorf("dir %v is package %v, wanted %v", pkg.dir, pkgName, expectPkg)
+	}
+	return exports, err
+}
+
 // pkgIsCandidate reports whether pkg is a candidate for satisfying the
 // finding which package pkgIdent in the file named by filename is trying
 // to refer to.
@@ -1524,10 +1555,10 @@ func (fn visitFn) Visit(node ast.Node) ast.Visitor {
 	return fn(node)
 }
 
-func copyExports(pkg map[string]bool) map[string]bool {
+func copyExports(pkg []string) map[string]bool {
 	m := make(map[string]bool, len(pkg))
-	for k, v := range pkg {
-		m[k] = v
+	for _, v := range pkg {
+		m[v] = true
 	}
 	return m
 }

internal/imports/fix_test.go

@@ -1556,7 +1556,7 @@ var _ = bytes.Buffer
 	// Force a scan of the stdlib.
 	savedStdlib := stdlib
 	defer func() { stdlib = savedStdlib }()
-	stdlib = map[string]map[string]bool{}
+	stdlib = map[string][]string{}
 
 	testConfig{
 		module: packagestest.Module{

internal/imports/mkstdlib.go

@@ -44,7 +44,7 @@ func main() {
 	}
 	outf("// Code generated by mkstdlib.go. DO NOT EDIT.\n\n")
 	outf("package imports\n")
-	outf("var stdlib = map[string]map[string]bool{\n")
+	outf("var stdlib = map[string][]string{\n")
 	f := io.MultiReader(
 		mustOpen(api("go1.txt")),
 		mustOpen(api("go1.1.txt")),
@@ -89,7 +89,7 @@ func main() {
 	}
 	sort.Strings(paths)
 	for _, path := range paths {
-		outf("\t%q: map[string]bool{\n", path)
+		outf("\t%q: []string{\n", path)
 		pkg := pkgs[path]
 		var syms []string
 		for sym := range pkg {
@@ -97,7 +97,7 @@ func main() {
 		}
 		sort.Strings(syms)
 		for _, sym := range syms {
-			outf("\t\t%q: true,\n", sym)
+			outf("\t\t%q,\n", sym)
 		}
 		outf("},\n")
 	}

internal/imports/mod.go

@@ -169,7 +169,7 @@ func (r *ModuleResolver) findPackage(importPath string) (*ModuleJSON, string) {
 			// resolution. package main or _test files should count but
 			// don't.
 			// TODO(heschi): fix this.
-			if _, err := r.cachePackageName(pkgDir); err == nil {
+			if _, err := r.cachePackageName(info); err == nil {
 				return m, pkgDir
 			}
 		}
@@ -211,20 +211,18 @@ func (r *ModuleResolver) cacheKeys() []string {
 }
 
 // cachePackageName caches the package name for a dir already in the cache.
-func (r *ModuleResolver) cachePackageName(dir string) (directoryPackageInfo, error) {
-	info, ok := r.cacheLoad(dir)
-	if !ok {
-		panic("cachePackageName on uncached dir " + dir)
+func (r *ModuleResolver) cachePackageName(info directoryPackageInfo) (directoryPackageInfo, error) {
+	if info.rootType == gopathwalk.RootModuleCache {
+		return r.moduleCacheCache.CachePackageName(info)
+	}
+	return r.otherCache.CachePackageName(info)
 }
 
-	loaded, err := info.reachedStatus(nameLoaded)
-	if loaded {
-		return info, err
+func (r *ModuleResolver) cacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []string, error) {
+	if info.rootType == gopathwalk.RootModuleCache {
+		return r.moduleCacheCache.CacheExports(ctx, env, info)
 	}
-	info.packageName, info.err = packageDirToName(info.dir)
-	info.status = nameLoaded
-	r.cacheStore(info)
-	return info, info.err
+	return r.otherCache.CacheExports(ctx, env, info)
 }
 
 // findModuleByDir returns the module that contains dir, or nil if no such
@@ -406,7 +404,7 @@ func (r *ModuleResolver) scan(_ references, loadNames bool, exclude []gopathwalk
 		// If we want package names, make sure the cache has them.
 		if loadNames {
 			var err error
-			if info, err = r.cachePackageName(info.dir); err != nil {
+			if info, err = r.cachePackageName(info); err != nil {
 				continue
 			}
 		}
@@ -465,11 +463,14 @@ func (r *ModuleResolver) canonicalize(info directoryPackageInfo) (*pkg, error) {
 	return res, nil
 }
 
-func (r *ModuleResolver) loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[string]bool, error) {
+func (r *ModuleResolver) loadExports(ctx context.Context, pkg *pkg) (string, []string, error) {
 	if err := r.init(); err != nil {
-		return nil, err
+		return "", nil, err
 	}
-	return loadExportsFromFiles(ctx, r.env, expectPackage, pkg.dir)
+	if info, ok := r.cacheLoad(pkg.dir); ok {
+		return r.cacheExports(ctx, r.env, info)
+	}
+	return loadExportsFromFiles(ctx, r.env, pkg.dir)
 }
 
 func (r *ModuleResolver) scanDirForPackage(root gopathwalk.Root, dir string) directoryPackageInfo {

internal/imports/mod_cache.go

@@ -1,6 +1,8 @@
 package imports
 
 import (
+	"context"
+	"fmt"
 	"sync"
 
 	"golang.org/x/tools/internal/gopathwalk"
@@ -30,6 +32,7 @@ const (
 	_ directoryPackageStatus = iota
 	directoryScanned
 	nameLoaded
+	exportsLoaded
 )
 
 type directoryPackageInfo struct {
@@ -58,6 +61,10 @@ type directoryPackageInfo struct {
 	// Set when status >= nameLoaded.
 
 	packageName string // the package name, as declared in the source.
+
+	// Set when status >= exportsLoaded.
+
+	exports []string
 }
 
 // reachedStatus returns true when info has a status at least target and any error associated with
@@ -121,3 +128,38 @@ func (d *dirInfoCache) Keys() (keys []string) {
 	}
 	return keys
 }
+
+func (d *dirInfoCache) CachePackageName(info directoryPackageInfo) (directoryPackageInfo, error) {
+	if loaded, err := info.reachedStatus(nameLoaded); loaded {
+		return info, err
+	}
+	if scanned, err := info.reachedStatus(directoryScanned); !scanned || err != nil {
+		return info, fmt.Errorf("cannot read package name, scan error: %v", err)
+	}
+	info.packageName, info.err = packageDirToName(info.dir)
+	info.status = nameLoaded
+	d.Store(info.dir, info)
+	return info, info.err
+}
+
+func (d *dirInfoCache) CacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []string, error) {
+	if reached, _ := info.reachedStatus(exportsLoaded); reached {
+		return info.packageName, info.exports, info.err
+	}
+	if reached, err := info.reachedStatus(nameLoaded); reached && err != nil {
+		return "", nil, err
+	}
+	info.packageName, info.exports, info.err = loadExportsFromFiles(ctx, env, info.dir)
+	if info.err == context.Canceled {
+		return info.packageName, info.exports, info.err
+	}
+	// The cache structure wants things to proceed linearly. We can skip a
+	// step here, but only if we succeed.
+	if info.status == nameLoaded || info.err == nil {
+		info.status = exportsLoaded
+	} else {
+		info.status = nameLoaded
+	}
+	d.Store(info.dir, info)
+	return info.packageName, info.exports, info.err
+}

internal/imports/mod_cache_test.go

@@ -2,6 +2,7 @@ package imports
 
 import (
 	"fmt"
+	"reflect"
 	"sort"
 	"testing"
 )
@@ -97,7 +98,7 @@ func TestModCacheInfo(t *testing.T) {
 			t.Errorf("directory not loaded: %s", d.dir)
 		}
 
-		if val != d.info {
+		if !reflect.DeepEqual(d.info, val) {
 			t.Errorf("expected: %v, got: %v", d.info, val)
 		}
 	}