internal/lsp: cache the imports ProcessEnv across imports calls

The imports ProcessEnv contains cached module and filesystem state. This change
allows gopls to use the same ProcessEnv and resolver across multiple calls to the
internal/imports library.

A ProcessEnv belongs to a view, because the cached module state depends
on the module that is open in the workspace.

Since we do not yet track whether the 'go.mod' file has changed, we
conservatively reset the cached state in the module resolver before
every call to imports.Process.

Change-Id: I27c8e212cb0b477ff425d5ed98a544b27b7d92ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184921
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/imports/fix.go

@@ -272,7 +272,7 @@ func (p *pass) loadPackageNames(imports []*importInfo) error {
 		unknown = append(unknown, imp.importPath)
 	}
 
-	names, err := p.env.getResolver().loadPackageNames(unknown, p.srcDir)
+	names, err := p.env.GetResolver().loadPackageNames(unknown, p.srcDir)
 	if err != nil {
 		return err
 	}
@@ -595,7 +595,7 @@ type ProcessEnv struct {
 	// Logf is the default logger for the ProcessEnv.
 	Logf func(format string, args ...interface{})
 
-	resolver resolver
+	resolver Resolver
 }
 
 func (e *ProcessEnv) env() []string {
@@ -617,7 +617,7 @@ func (e *ProcessEnv) env() []string {
 	return env
 }
 
-func (e *ProcessEnv) getResolver() resolver {
+func (e *ProcessEnv) GetResolver() Resolver {
 	if e.resolver != nil {
 		return e.resolver
 	}
@@ -631,7 +631,7 @@ func (e *ProcessEnv) getResolver() resolver {
 		e.resolver = &gopathResolver{env: e}
 		return e.resolver
 	}
-	e.resolver = &moduleResolver{env: e}
+	e.resolver = &ModuleResolver{env: e}
 	return e.resolver
 }
 
@@ -700,15 +700,15 @@ func addStdlibCandidates(pass *pass, refs references) {
 	}
 }
 
-// A resolver does the build-system-specific parts of goimports.
-type resolver interface {
+// A Resolver does the build-system-specific parts of goimports.
+type Resolver interface {
 	// loadPackageNames loads the package names in importPaths.
 	loadPackageNames(importPaths []string, srcDir string) (map[string]string, error)
 	// scan finds (at least) the packages satisfying refs. The returned slice is unordered.
 	scan(refs references) ([]*pkg, error)
 }
 
-// gopathResolver implements resolver for GOPATH and module workspaces using go/packages.
+// gopackagesResolver implements resolver for GOPATH and module workspaces using go/packages.
 type goPackagesResolver struct {
 	env *ProcessEnv
 }
@@ -758,7 +758,7 @@ func (r *goPackagesResolver) scan(refs references) ([]*pkg, error) {
 }
 
 func addExternalCandidates(pass *pass, refs references, filename string) error {
-	dirScan, err := pass.env.getResolver().scan(refs)
+	dirScan, err := pass.env.GetResolver().scan(refs)
 	if err != nil {
 		return err
 	}

internal/imports/fix_test.go

@@ -1855,7 +1855,7 @@ func TestImportPathToNameGoPathParse(t *testing.T) {
 		if strings.Contains(t.Name(), "GoPackages") {
 			t.Skip("go/packages does not ignore package main")
 		}
-		r := t.env.getResolver()
+		r := t.env.GetResolver()
 		srcDir := filepath.Dir(t.exported.File("example.net/pkg", "z.go"))
 		names, err := r.loadPackageNames([]string{"example.net/pkg"}, srcDir)
 		if err != nil {

internal/imports/mod.go

@@ -18,37 +18,37 @@ import (
 	"golang.org/x/tools/internal/module"
 )
 
-// moduleResolver implements resolver for modules using the go command as little
+// ModuleResolver implements resolver for modules using the go command as little
 // as feasible.
-type moduleResolver struct {
+type ModuleResolver struct {
 	env *ProcessEnv
 
-	initialized   bool
-	main          *moduleJSON
-	modsByModPath []*moduleJSON // All modules, ordered by # of path components in module Path...
-	modsByDir     []*moduleJSON // ...or Dir.
+	Initialized   bool
+	Main          *ModuleJSON
+	ModsByModPath []*ModuleJSON // All modules, ordered by # of path components in module Path...
+	ModsByDir     []*ModuleJSON // ...or Dir.
 }
 
-type moduleJSON struct {
+type ModuleJSON struct {
 	Path     string           // module path
 	Version  string           // module version
 	Versions []string         // available module versions (with -versions)
-	Replace  *moduleJSON      // replaced by this module
+	Replace  *ModuleJSON      // replaced by this module
 	Time     *time.Time       // time version was created
-	Update   *moduleJSON      // available update, if any (with -u)
+	Update   *ModuleJSON      // available update, if any (with -u)
 	Main     bool             // is this the main module?
 	Indirect bool             // is this module only an indirect dependency of main module?
 	Dir      string           // directory holding files for this module, if any
 	GoMod    string           // path to go.mod file for this module, if any
-	Error    *moduleErrorJSON // error loading module
+	Error    *ModuleErrorJSON // error loading module
 }
 
-type moduleErrorJSON struct {
+type ModuleErrorJSON struct {
 	Err string // the error itself
 }
 
-func (r *moduleResolver) init() error {
-	if r.initialized {
+func (r *ModuleResolver) init() error {
+	if r.Initialized {
 		return nil
 	}
 	stdout, err := r.env.invokeGo("list", "-m", "-json", "...")
@@ -56,7 +56,7 @@ func (r *moduleResolver) init() error {
 		return err
 	}
 	for dec := json.NewDecoder(stdout); dec.More(); {
-		mod := &moduleJSON{}
+		mod := &ModuleJSON{}
 		if err := dec.Decode(mod); err != nil {
 			return err
 		}
@@ -67,34 +67,34 @@ func (r *moduleResolver) init() error {
 			// Can't do anything with a module that's not downloaded.
 			continue
 		}
-		r.modsByModPath = append(r.modsByModPath, mod)
-		r.modsByDir = append(r.modsByDir, mod)
+		r.ModsByModPath = append(r.ModsByModPath, mod)
+		r.ModsByDir = append(r.ModsByDir, mod)
 		if mod.Main {
-			r.main = mod
+			r.Main = mod
 		}
 	}
 
-	sort.Slice(r.modsByModPath, func(i, j int) bool {
+	sort.Slice(r.ModsByModPath, func(i, j int) bool {
 		count := func(x int) int {
-			return strings.Count(r.modsByModPath[x].Path, "/")
+			return strings.Count(r.ModsByModPath[x].Path, "/")
 		}
 		return count(j) < count(i) // descending order
 	})
-	sort.Slice(r.modsByDir, func(i, j int) bool {
+	sort.Slice(r.ModsByDir, func(i, j int) bool {
 		count := func(x int) int {
-			return strings.Count(r.modsByDir[x].Dir, "/")
+			return strings.Count(r.ModsByDir[x].Dir, "/")
 		}
 		return count(j) < count(i) // descending order
 	})
 
-	r.initialized = true
+	r.Initialized = true
 	return nil
 }
 
 // findPackage returns the module and directory that contains the package at
 // the given import path, or returns nil, "" if no module is in scope.
-func (r *moduleResolver) findPackage(importPath string) (*moduleJSON, string) {
-	for _, m := range r.modsByModPath {
+func (r *ModuleResolver) findPackage(importPath string) (*ModuleJSON, string) {
+	for _, m := range r.ModsByModPath {
 		if !strings.HasPrefix(importPath, m.Path) {
 			continue
 		}
@@ -123,7 +123,7 @@ func (r *moduleResolver) findPackage(importPath string) (*moduleJSON, string) {
 
 // findModuleByDir returns the module that contains dir, or nil if no such
 // module is in scope.
-func (r *moduleResolver) findModuleByDir(dir string) *moduleJSON {
+func (r *ModuleResolver) findModuleByDir(dir string) *ModuleJSON {
 	// This is quite tricky and may not be correct. dir could be:
 	// - a package in the main module.
 	// - a replace target underneath the main module's directory.
@@ -134,7 +134,7 @@ func (r *moduleResolver) findModuleByDir(dir string) *moduleJSON {
 	// - in /vendor/ in -mod=vendor mode.
 	//    - nested module? Dunno.
 	// Rumor has it that replace targets cannot contain other replace targets.
-	for _, m := range r.modsByDir {
+	for _, m := range r.ModsByDir {
 		if !strings.HasPrefix(dir, m.Dir) {
 			continue
 		}
@@ -150,7 +150,7 @@ func (r *moduleResolver) findModuleByDir(dir string) *moduleJSON {
 
 // dirIsNestedModule reports if dir is contained in a nested module underneath
 // mod, not actually in mod.
-func dirIsNestedModule(dir string, mod *moduleJSON) bool {
+func dirIsNestedModule(dir string, mod *ModuleJSON) bool {
 	if !strings.HasPrefix(dir, mod.Dir) {
 		return false
 	}
@@ -176,7 +176,7 @@ func findModFile(dir string) string {
 	}
 }
 
-func (r *moduleResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) {
+func (r *ModuleResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) {
 	if err := r.init(); err != nil {
 		return nil, err
 	}
@@ -195,7 +195,7 @@ func (r *moduleResolver) loadPackageNames(importPaths []string, srcDir string) (
 	return names, nil
 }
 
-func (r *moduleResolver) scan(_ references) ([]*pkg, error) {
+func (r *ModuleResolver) scan(_ references) ([]*pkg, error) {
 	if err := r.init(); err != nil {
 		return nil, err
 	}
@@ -204,15 +204,15 @@ func (r *moduleResolver) scan(_ references) ([]*pkg, error) {
 	roots := []gopathwalk.Root{
 		{filepath.Join(r.env.GOROOT, "/src"), gopathwalk.RootGOROOT},
 	}
-	if r.main != nil {
-		roots = append(roots, gopathwalk.Root{r.main.Dir, gopathwalk.RootCurrentModule})
+	if r.Main != nil {
+		roots = append(roots, gopathwalk.Root{r.Main.Dir, gopathwalk.RootCurrentModule})
 	}
 	for _, p := range filepath.SplitList(r.env.GOPATH) {
 		roots = append(roots, gopathwalk.Root{filepath.Join(p, "/pkg/mod"), gopathwalk.RootModuleCache})
 	}
 
 	// Walk replace targets, just in case they're not in any of the above.
-	for _, mod := range r.modsByModPath {
+	for _, mod := range r.ModsByModPath {
 		if mod.Replace != nil {
 			roots = append(roots, gopathwalk.Root{mod.Dir, gopathwalk.RootOther})
 		}
@@ -247,7 +247,7 @@ func (r *moduleResolver) scan(_ references) ([]*pkg, error) {
 		}
 		switch root.Type {
 		case gopathwalk.RootCurrentModule:
-			importPath = path.Join(r.main.Path, filepath.ToSlash(subdir))
+			importPath = path.Join(r.Main.Path, filepath.ToSlash(subdir))
 		case gopathwalk.RootModuleCache:
 			matches := modCacheRegexp.FindStringSubmatch(subdir)
 			modPath, err := module.DecodePath(filepath.ToSlash(matches[1]))

internal/imports/mod_test.go

@@ -140,7 +140,7 @@ import _ "rsc.io/quote"
 
 	mt.env.GOFLAGS = ""
 	// Clear out the resolver's cache, since we've changed the environment.
-	mt.resolver = &moduleResolver{env: mt.env}
+	mt.resolver = &ModuleResolver{env: mt.env}
 	mt.assertModuleFoundInDir("rsc.io/quote", "quote", `pkg.*mod.*/quote@.*$`)
 }
 
@@ -486,7 +486,7 @@ var proxyDir string
 type modTest struct {
 	*testing.T
 	env      *ProcessEnv
-	resolver *moduleResolver
+	resolver *ModuleResolver
 	cleanup  func()
 }
 
@@ -538,7 +538,7 @@ func setup(t *testing.T, main, wd string) *modTest {
 	return &modTest{
 		T:        t,
 		env:      env,
-		resolver: &moduleResolver{env: env},
+		resolver: &ModuleResolver{env: env},
 		cleanup: func() {
 			_ = filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
 				if err != nil {

internal/lsp/cache/view.go

@@ -12,9 +12,11 @@ import (
 	"go/types"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/internal/imports"
 	"golang.org/x/tools/internal/lsp/debug"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/xlog"
@@ -49,6 +51,10 @@ type view struct {
 	// env is the environment to use when invoking underlying tools.
 	env []string
 
+	// process is the process env for this view.
+	// Note: this contains cached module and filesystem state.
+	processEnv *imports.ProcessEnv
+
 	// buildFlags is the build flags to use when invoking underlying tools.
 	buildFlags []string
 
@@ -133,6 +139,47 @@ func (v *view) Config() *packages.Config {
 	}
 }
 
+func (v *view) ProcessEnv() *imports.ProcessEnv {
+	v.mu.Lock()
+	defer v.mu.Unlock()
+
+	if v.processEnv == nil {
+		v.processEnv = v.buildProcessEnv()
+	}
+	return v.processEnv
+}
+
+func (v *view) buildProcessEnv() *imports.ProcessEnv {
+	cfg := v.Config()
+	env := &imports.ProcessEnv{
+		WorkingDir: cfg.Dir,
+		Logf: func(format string, u ...interface{}) {
+			xlog.Infof(v.backgroundCtx, format, u...)
+		},
+	}
+	for _, kv := range cfg.Env {
+		split := strings.Split(kv, "=")
+		if len(split) < 2 {
+			continue
+		}
+		switch split[0] {
+		case "GOPATH":
+			env.GOPATH = split[1]
+		case "GOROOT":
+			env.GOROOT = split[1]
+		case "GO111MODULE":
+			env.GO111MODULE = split[1]
+		case "GOPROXY":
+			env.GOROOT = split[1]
+		case "GOFLAGS":
+			env.GOFLAGS = split[1]
+		case "GOSUMDB":
+			env.GOSUMDB = split[1]
+		}
+	}
+	return env
+}
+
 func (v *view) Env() []string {
 	v.mu.Lock()
 	defer v.mu.Unlock()
@@ -144,6 +191,7 @@ func (v *view) SetEnv(env []string) {
 	defer v.mu.Unlock()
 	//TODO: this should invalidate the entire view
 	v.env = env
+	v.processEnv = nil // recompute process env
 }
 
 func (v *view) SetBuildFlags(buildFlags []string) {

internal/lsp/source/format.go

@@ -10,7 +10,6 @@ import (
 	"context"
 	"fmt"
 	"go/format"
-	"strings"
 
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/go/packages"
@@ -68,8 +67,17 @@ func Imports(ctx context.Context, view View, f GoFile, rng span.Range) ([]TextEd
 	if hasListErrors(pkg.GetErrors()) {
 		return nil, fmt.Errorf("%s has list errors, not running goimports", f.URI())
 	}
+
+	if resolver, ok := view.ProcessEnv().GetResolver().(*imports.ModuleResolver); ok && resolver.Initialized {
+		// TODO(suzmue): only reset this state when necessary (eg when the go.mod files of this
+		// module or modules with replace directive changes).
+		resolver.Initialized = false
+		resolver.Main = nil
+		resolver.ModsByModPath = nil
+		resolver.ModsByDir = nil
+	}
 	options := &imports.Options{
-		Env: buildProcessEnv(ctx, view),
+		Env: view.ProcessEnv(),
 		// Defaults.
 		AllErrors:  true,
 		Comments:   true,
@@ -103,37 +111,6 @@ func hasListErrors(errors []packages.Error) bool {
 	return false
 }
 
-func buildProcessEnv(ctx context.Context, view View) *imports.ProcessEnv {
-	cfg := view.Config()
-	env := &imports.ProcessEnv{
-		WorkingDir: cfg.Dir,
-		Logf: func(format string, v ...interface{}) {
-			xlog.Infof(ctx, format, v...)
-		},
-	}
-	for _, kv := range cfg.Env {
-		split := strings.Split(kv, "=")
-		if len(split) < 2 {
-			continue
-		}
-		switch split[0] {
-		case "GOPATH":
-			env.GOPATH = split[1]
-		case "GOROOT":
-			env.GOROOT = split[1]
-		case "GO111MODULE":
-			env.GO111MODULE = split[1]
-		case "GOPROXY":
-			env.GOROOT = split[1]
-		case "GOFLAGS":
-			env.GOFLAGS = split[1]
-		case "GOSUMDB":
-			env.GOSUMDB = split[1]
-		}
-	}
-	return env
-}
-
 func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) {
 	ctx, done := trace.StartSpan(ctx, "source.computeTextEdits")
 	defer done()

internal/lsp/source/view.go

@@ -14,6 +14,7 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/internal/imports"
 	"golang.org/x/tools/internal/lsp/diff"
 	"golang.org/x/tools/internal/span"
 )
@@ -208,6 +209,15 @@ type View interface {
 	Ignore(span.URI) bool
 
 	Config() *packages.Config
+
+	// Process returns the process for this view.
+	// Note: this contains cached module and filesystem state, which must
+	// be invalidated after a 'go.mod' change.
+	//
+	// TODO(suzmue): the state cached in the process env is specific to each view,
+	// however, there is state that can be shared between views that is not currently
+	// cached, like the module cache.
+	ProcessEnv() *imports.ProcessEnv
 }
 
 // File represents a source file of any type.