gopls/internal/cache/imports: simplify importsState invalidation

Now that views are immutable, we can simplify the invalidation of imports state. The imports.ProcessEnv can also be immutable, and we need only invalidate module state (i.e. dependencies) if a go.mod file changes. While at it, add documentation and perform some superficial cleanup in the internal/imports package. This is a first step toward larger state optimizations needed for the issues below. TODOs are added for remaining work. For golang/go#44863 For golang/go#59216 Change-Id: I23c1ea96f241334efdbfb4c09f6265637b38f497 Reviewed-on: https://go-review.googlesource.com/c/tools/+/559496 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
golang · Jan 31, 2024 · da7ed64 · da7ed64
1 parent 7ec8ac0
commit da7ed64
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 96 deletions.
diff --git a/gopls/internal/cache/imports.go b/gopls/internal/cache/imports.go
@@ -7,9 +7,6 @@ package cache
 import (
 	"context"
 	"fmt"
-	"os"
-	"reflect"
-	"strings"
 	"sync"
 	"time"
 
@@ -22,13 +19,11 @@ import (
 type importsState struct {
 	ctx context.Context
 
-	mu                     sync.Mutex
-	processEnv             *imports.ProcessEnv
-	cacheRefreshDuration   time.Duration
-	cacheRefreshTimer      *time.Timer
-	cachedModFileHash      file.Hash
-	cachedBuildFlags       []string
-	cachedDirectoryFilters []string
+	mu                   sync.Mutex
+	processEnv           *imports.ProcessEnv
+	cacheRefreshDuration time.Duration
+	cacheRefreshTimer    *time.Timer
+	cachedModFileHash    file.Hash
 }
 
 func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *Snapshot, fn func(context.Context, *imports.Options) error) error {
@@ -52,33 +47,17 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *Snapshot
 		modFileHash.XORWith(fh.Identity().Hash)
 	}
 
-	// view.goEnv is immutable -- changes make a new view. Options can change.
-	// We can't compare build flags directly because we may add -modfile.
-	localPrefix := snapshot.Options().Local
-	currentBuildFlags := snapshot.Options().BuildFlags
-	currentDirectoryFilters := snapshot.Options().DirectoryFilters
-	changed := !reflect.DeepEqual(currentBuildFlags, s.cachedBuildFlags) ||
-		snapshot.Options().VerboseOutput != (s.processEnv.Logf != nil) ||
-		modFileHash != s.cachedModFileHash ||
-		!reflect.DeepEqual(snapshot.Options().DirectoryFilters, s.cachedDirectoryFilters)
-
 	// If anything relevant to imports has changed, clear caches and
 	// update the processEnv. Clearing caches blocks on any background
 	// scans.
-	if changed {
-		if err := populateProcessEnvFromSnapshot(ctx, s.processEnv, snapshot); err != nil {
-			return err
-		}
-
+	if modFileHash != s.cachedModFileHash {
 		if resolver, err := s.processEnv.GetResolver(); err == nil {
 			if modResolver, ok := resolver.(*imports.ModuleResolver); ok {
 				modResolver.ClearForNewMod()
 			}
 		}
 
 		s.cachedModFileHash = modFileHash
-		s.cachedBuildFlags = currentBuildFlags
-		s.cachedDirectoryFilters = currentDirectoryFilters
 	}
 
 	// Run the user function.
@@ -91,7 +70,7 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *Snapshot
 		TabIndent:   true,
 		TabWidth:    8,
 		Env:         s.processEnv,
-		LocalPrefix: localPrefix,
+		LocalPrefix: snapshot.Options().Local,
 	}
 
 	if err := fn(ctx, opts); err != nil {
@@ -111,36 +90,6 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *Snapshot
 	return nil
 }
 
-// populateProcessEnvFromSnapshot sets the dynamically configurable fields for
-// the view's process environment. Assumes that the caller is holding the
-// importsState mutex.
-func populateProcessEnvFromSnapshot(ctx context.Context, pe *imports.ProcessEnv, snapshot *Snapshot) error {
-	ctx, done := event.Start(ctx, "cache.populateProcessEnvFromSnapshot")
-	defer done()
-
-	if snapshot.Options().VerboseOutput {
-		pe.Logf = func(format string, args ...interface{}) {
-			event.Log(ctx, fmt.Sprintf(format, args...))
-		}
-	} else {
-		pe.Logf = nil
-	}
-
-	pe.WorkingDir = snapshot.view.root.Path()
-	pe.ModFlag = "readonly" // processEnv operations should not mutate the modfile
-	pe.Env = map[string]string{}
-	pe.BuildFlags = append([]string{}, snapshot.Options().BuildFlags...)
-	env := append(append(os.Environ(), snapshot.Options().EnvSlice()...), "GO111MODULE="+snapshot.view.adjustedGO111MODULE())
-	for _, kv := range env {
-		split := strings.SplitN(kv, "=", 2)
-		if len(split) != 2 {
-			continue
-		}
-		pe.Env[split[0]] = split[1]
-	}
-	return nil
-}
-
 func (s *importsState) refreshProcessEnv() {
 	ctx, done := event.Start(s.ctx, "cache.importsState.refreshProcessEnv")
 	defer done()

diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go
@@ -17,12 +17,13 @@ import (
 	"sync/atomic"
 	"time"
 
-	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/cache/typerefs"
+	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/persistent"
+	"golang.org/x/tools/gopls/internal/util/slices"
 	"golang.org/x/tools/gopls/internal/vulncheck"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/gocommand"
@@ -190,6 +191,30 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, *
 		ignoreFilter = newIgnoreFilter(dirs)
 	}
 
+	var pe *imports.ProcessEnv
+	{
+		env := make(map[string]string)
+		envSlice := slices.Concat(os.Environ(), def.folder.Options.EnvSlice(), []string{"GO111MODULE=" + def.adjustedGO111MODULE()})
+		for _, kv := range envSlice {
+			if k, v, ok := strings.Cut(kv, "="); ok {
+				env[k] = v
+			}
+		}
+		pe = &imports.ProcessEnv{
+			GocmdRunner:    s.gocmdRunner,
+			BuildFlags:     slices.Clone(def.folder.Options.BuildFlags),
+			ModFlag:        "readonly", // processEnv operations should not mutate the modfile
+			SkipPathInScan: skipPath,
+			Env:            env,
+			WorkingDir:     def.root.Path(),
+		}
+		if def.folder.Options.VerboseOutput {
+			pe.Logf = func(format string, args ...interface{}) {
+				event.Log(ctx, fmt.Sprintf(format, args...))
+			}
+		}
+	}
+
 	v := &View{
 		id:                   strconv.FormatInt(index, 10),
 		gocmdRunner:          s.gocmdRunner,
@@ -201,11 +226,8 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, *
 		fs:                   s.overlayFS,
 		viewDefinition:       def,
 		importsState: &importsState{
-			ctx: backgroundCtx,
-			processEnv: &imports.ProcessEnv{
-				GocmdRunner:    s.gocmdRunner,
-				SkipPathInScan: skipPath,
-			},
+			ctx:        backgroundCtx,
+			processEnv: pe,
 		},
 	}
 

diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go
@@ -1759,8 +1759,6 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru
 		count++
 	}
 
-	ctx, cancel := context.WithCancel(ctx)
-
 	var mu sync.Mutex
 	add := func(pkg imports.ImportFix) {
 		if ignoreUnimportedCompletion(&pkg) {
@@ -1776,7 +1774,6 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru
 		}
 
 		if count >= maxUnimportedPackageNames {
-			cancel()
 			return
 		}
 
@@ -1794,10 +1791,11 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru
 		})
 		count++
 	}
+
 	c.completionCallbacks = append(c.completionCallbacks, func(ctx context.Context, opts *imports.Options) error {
-		defer cancel()
 		return imports.GetAllCandidates(ctx, add, prefix, c.filename, c.pkg.GetTypes().Name(), opts.Env)
 	})
+
 	return nil
 }
 

diff --git a/gopls/internal/util/slices/slices.go b/gopls/internal/util/slices/slices.go
@@ -4,6 +4,14 @@
 
 package slices
 
+// Clone returns a copy of the slice.
+// The elements are copied using assignment, so this is a shallow clone.
+// TODO(rfindley): use go1.19 slices.Clone.
+func Clone[S ~[]E, E any](s S) S {
+	// The s[:0:0] preserves nil in case it matters.
+	return append(s[:0:0], s...)
+}
+
 // Contains reports whether x is present in slice.
 // TODO(adonovan): use go1.19 slices.Contains.
 func Contains[S ~[]E, E comparable](slice S, x E) bool {
@@ -35,7 +43,7 @@ func ContainsFunc[S ~[]E, E any](s S, f func(E) bool) bool {
 }
 
 // Concat returns a new slice concatenating the passed in slices.
-// TODO(rfindley): use go1.22 slices.Contains.
+// TODO(rfindley): use go1.22 slices.Concat.
 func Concat[S ~[]E, E any](slices ...S) S {
 	size := 0
 	for _, s := range slices {

diff --git a/internal/imports/fix.go b/internal/imports/fix.go
@@ -828,7 +828,22 @@ func GetPackageExports(ctx context.Context, wrapped func(PackageExport), searchP
 	return getCandidatePkgs(ctx, callback, filename, filePkg, env)
 }
 
-var requiredGoEnvVars = []string{"GO111MODULE", "GOFLAGS", "GOINSECURE", "GOMOD", "GOMODCACHE", "GONOPROXY", "GONOSUMDB", "GOPATH", "GOPROXY", "GOROOT", "GOSUMDB", "GOWORK"}
+// TODO(rfindley): we should depend on GOOS and GOARCH, to provide accurate
+// imports when doing cross-platform development.
+var requiredGoEnvVars = []string{
+	"GO111MODULE",
+	"GOFLAGS",
+	"GOINSECURE",
+	"GOMOD",
+	"GOMODCACHE",
+	"GONOPROXY",
+	"GONOSUMDB",
+	"GOPATH",
+	"GOPROXY",
+	"GOROOT",
+	"GOSUMDB",
+	"GOWORK",
+}
 
 // ProcessEnv contains environment variables and settings that affect the use of
 // the go command, the go/build package, etc.
@@ -847,14 +862,16 @@ type ProcessEnv struct {
 	// Env overrides the OS environment, and can be used to specify
 	// GOPROXY, GO111MODULE, etc. PATH cannot be set here, because
 	// exec.Command will not honor it.
-	// Specifying all of RequiredGoEnvVars avoids a call to `go env`.
+	// Specifying all of requiredGoEnvVars avoids a call to `go env`.
 	Env map[string]string
 
 	WorkingDir string
 
 	// If Logf is non-nil, debug logging is enabled through this function.
 	Logf func(format string, args ...interface{})
 
+	// TODO(rfindley): for simplicity, use a constructor rather than
+	// initialization pattern for ProcessEnv.
 	initialized bool
 
 	resolver Resolver
@@ -1030,11 +1047,14 @@ func addStdlibCandidates(pass *pass, refs references) error {
 type Resolver interface {
 	// loadPackageNames loads the package names in importPaths.
 	loadPackageNames(importPaths []string, srcDir string) (map[string]string, error)
+
 	// scan works with callback to search for packages. See scanCallback for details.
 	scan(ctx context.Context, callback *scanCallback) error
+
 	// loadExports returns the set of exported symbols in the package at dir.
 	// loadExports may be called concurrently.
 	loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []string, error)
+
 	// scoreImportPath returns the relevance for an import path.
 	scoreImportPath(ctx context.Context, path string) float64
 
@@ -1121,7 +1141,7 @@ func addExternalCandidates(ctx context.Context, pass *pass, refs references, fil
 		go func(pkgName string, symbols map[string]bool) {
 			defer wg.Done()
 
-			found, err := findImport(ctx, pass, found[pkgName], pkgName, symbols, filename)
+			found, err := findImport(ctx, pass, found[pkgName], pkgName, symbols)
 
 			if err != nil {
 				firstErrOnce.Do(func() {
@@ -1550,7 +1570,7 @@ func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, incl
 
 // findImport searches for a package with the given symbols.
 // If no package is found, findImport returns ("", false, nil)
-func findImport(ctx context.Context, pass *pass, candidates []pkgDistance, pkgName string, symbols map[string]bool, filename string) (*pkg, error) {
+func findImport(ctx context.Context, pass *pass, candidates []pkgDistance, pkgName string, symbols map[string]bool) (*pkg, error) {
 	// Sort the candidates by their import package length,
 	// assuming that shorter package names are better than long
 	// ones.  Note that this sorts by the de-vendored name, so

diff --git a/internal/imports/imports.go b/internal/imports/imports.go
@@ -236,7 +236,7 @@ func parse(fset *token.FileSet, filename string, src []byte, opt *Options) (*ast
 			src = src[:len(src)-len("}\n")]
 			// Gofmt has also indented the function body one level.
 			// Remove that indent.
-			src = bytes.Replace(src, []byte("\n\t"), []byte("\n"), -1)
+			src = bytes.ReplaceAll(src, []byte("\n\t"), []byte("\n"))
 			return matchSpace(orig, src)
 		}
 		return file, adjust, nil