internal/imports: merge init and newModuleResolver

Switch the ModuleResolver from a lazy initialization model to a
constructor, so that the resolver returned by ProcessEnv.GetResolver is
ready to use.

A constructor is easier to maintain as it involves less state, avoids
redundant calls to init, and avoids bugs where the call to init is
missing (such as was the case for the scoreImportPath method). It also
lets the caller differentiate between a failure to construct the
resolver and a resolver that only returns errors.

Pragmatically, I'd like to move toward a model where the caller re-scans
imports by asynchronously cloning, priming, and replacing the resolver,
rather than blocking. This is a step in that direction.

This change is not without risk, but it looks like all calls to
ModuleResolver methods are preceded by a call to GetResolver, and
errors from GetResolver are handled similarly errors from methods.

There is some messiness resulting from this change, particularly in
tests. These are noted inline, and can eventually be fixed by similarly
avoiding lazy initialization of ProcessEnv.

For golang/go#59216

Change-Id: I3b7206417f61a5697ed83e811c177f646afce3b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559635
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/cache/imports.go

@@ -51,12 +51,7 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *Snapshot
 	// update the processEnv. Clearing caches blocks on any background
 	// scans.
 	if modFileHash != s.cachedModFileHash {
-		if resolver, err := s.processEnv.GetResolver(); err == nil {
-			if modResolver, ok := resolver.(*imports.ModuleResolver); ok {
-				modResolver.ClearForNewMod()
-			}
-		}
-
+		s.processEnv.ClearModuleInfo()
 		s.cachedModFileHash = modFileHash
 	}
 

internal/imports/fix.go

@@ -847,6 +847,21 @@ var requiredGoEnvVars = []string{
 
 // ProcessEnv contains environment variables and settings that affect the use of
 // the go command, the go/build package, etc.
+//
+// ...a ProcessEnv *also* overwrites its Env along with derived state in the
+// form of the resolver. And because it is lazily initialized, an env may just
+// be broken and unusable, but there is no way for the caller to detect that:
+// all queries will just fail.
+//
+// TODO(rfindley): refactor this package so that this type (perhaps renamed to
+// just Env or Config) is an immutable configuration struct, to be exchanged
+// for an initialized object via a constructor that returns an error. Perhaps
+// the signature should be `func NewResolver(*Env) (*Resolver, error)`, where
+// resolver is a concrete type used for resolving imports. Via this
+// refactoring, we can avoid the need to call ProcessEnv.init and
+// ProcessEnv.GoEnv everywhere, and implicitly fix all the places where this
+// these are misused. Also, we'd delegate the caller the decision of how to
+// handle a broken environment.
 type ProcessEnv struct {
 	GocmdRunner *gocommand.Runner
 
@@ -869,11 +884,13 @@ type ProcessEnv struct {
 	// 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
+	initialized bool // see TODO above
 
-	resolver Resolver
+	// resolver and resolverErr are lazily evaluated (see GetResolver).
+	// This is unclean, but see the big TODO in the docstring for ProcessEnv
+	// above: for now, we can't be sure that the ProcessEnv is fully initialized.
+	resolver    Resolver
+	resolverErr error
 }
 
 func (e *ProcessEnv) goEnv() (map[string]string, error) {
@@ -953,24 +970,25 @@ func (e *ProcessEnv) env() []string {
 }
 
 func (e *ProcessEnv) GetResolver() (Resolver, error) {
-	if e.resolver != nil {
-		return e.resolver, nil
-	}
 	if err := e.init(); err != nil {
 		return nil, err
 	}
-	// TODO(rfindley): we should only use a gopathResolver here if the working
-	// directory is actually *in* GOPATH. (I seem to recall an open gopls issue
-	// for this behavior, but I can't find it).
-	//
-	// For gopls, we can optionally explicitly choose a resolver type, since we
-	// already know the view type.
-	if len(e.Env["GOMOD"]) == 0 && len(e.Env["GOWORK"]) == 0 {
-		e.resolver = newGopathResolver(e)
-		return e.resolver, nil
-	}
-	e.resolver = newModuleResolver(e)
-	return e.resolver, nil
+
+	if e.resolver == nil && e.resolverErr == nil {
+		// TODO(rfindley): we should only use a gopathResolver here if the working
+		// directory is actually *in* GOPATH. (I seem to recall an open gopls issue
+		// for this behavior, but I can't find it).
+		//
+		// For gopls, we can optionally explicitly choose a resolver type, since we
+		// already know the view type.
+		if len(e.Env["GOMOD"]) == 0 && len(e.Env["GOWORK"]) == 0 {
+			e.resolver = newGopathResolver(e)
+		} else {
+			e.resolver, e.resolverErr = newModuleResolver(e)
+		}
+	}
+
+	return e.resolver, e.resolverErr
 }
 
 // buildContext returns the build.Context to use for matching files.

internal/imports/mod.go

@@ -62,8 +62,7 @@ import (
 type ModuleResolver struct {
 	env *ProcessEnv
 
-	// Module state, populated by init
-	initialized    bool
+	// Module state, populated during construction
 	dummyVendorMod *gocommand.ModuleJSON            // if vendoring is enabled, a pseudo-module to represent the /vendor directory
 	moduleCacheDir string                           // GOMODCACHE, inferred from GOPATH if unset
 	roots          []gopathwalk.Root                // roots to scan, in approximate order of importance
@@ -87,24 +86,18 @@ type ModuleResolver struct {
 	otherCache       *dirInfoCache
 }
 
-func newModuleResolver(e *ProcessEnv) *ModuleResolver {
+func newModuleResolver(e *ProcessEnv) (*ModuleResolver, error) {
 	r := &ModuleResolver{
 		env:      e,
 		scanSema: make(chan struct{}, 1),
 	}
 	r.scanSema <- struct{}{}
-	return r
-}
-
-func (r *ModuleResolver) init() error {
-	if r.initialized {
-		return nil
-	}
 
 	goenv, err := r.env.goEnv()
 	if err != nil {
-		return err
+		return nil, err
 	}
+
 	// TODO(rfindley): can we refactor to share logic with r.env.invokeGo?
 	inv := gocommand.Invocation{
 		BuildFlags: r.env.BuildFlags,
@@ -125,7 +118,7 @@ func (r *ModuleResolver) init() error {
 		// invocation?
 		vendorEnabled, mainModVendor, err = gocommand.VendorEnabled(context.TODO(), inv, r.env.GocmdRunner)
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
 
@@ -146,19 +139,14 @@ func (r *ModuleResolver) init() error {
 		// GO111MODULE=on. Other errors are fatal.
 		if err != nil {
 			if errMsg := err.Error(); !strings.Contains(errMsg, "working directory is not part of a module") && !strings.Contains(errMsg, "go.mod file not found") {
-				return err
+				return nil, err
 			}
 		}
 	}
 
-	if gmc := r.env.Env["GOMODCACHE"]; gmc != "" {
-		r.moduleCacheDir = gmc
-	} else {
-		gopaths := filepath.SplitList(goenv["GOPATH"])
-		if len(gopaths) == 0 {
-			return fmt.Errorf("empty GOPATH")
-		}
-		r.moduleCacheDir = filepath.Join(gopaths[0], "/pkg/mod")
+	r.moduleCacheDir = gomodcacheForEnv(goenv)
+	if r.moduleCacheDir == "" {
+		return nil, fmt.Errorf("cannot resolve GOMODCACHE")
 	}
 
 	sort.Slice(r.modsByModPath, func(i, j int) bool {
@@ -212,20 +200,32 @@ func (r *ModuleResolver) init() error {
 	}
 
 	r.scannedRoots = map[gopathwalk.Root]bool{}
-	if r.moduleCacheCache == nil {
-		r.moduleCacheCache = &dirInfoCache{
-			dirs:      map[string]*directoryPackageInfo{},
-			listeners: map[*int]cacheListener{},
-		}
+	r.moduleCacheCache = &dirInfoCache{
+		dirs:      map[string]*directoryPackageInfo{},
+		listeners: map[*int]cacheListener{},
 	}
-	if r.otherCache == nil {
-		r.otherCache = &dirInfoCache{
-			dirs:      map[string]*directoryPackageInfo{},
-			listeners: map[*int]cacheListener{},
-		}
+	r.otherCache = &dirInfoCache{
+		dirs:      map[string]*directoryPackageInfo{},
+		listeners: map[*int]cacheListener{},
 	}
-	r.initialized = true
-	return nil
+	return r, nil
+}
+
+// gomodcacheForEnv returns the GOMODCACHE value to use based on the given env
+// map, which must have GOMODCACHE and GOPATH populated.
+//
+// TODO(rfindley): this is defensive refactoring.
+//  1. Is this even relevant anymore? Can't we just read GOMODCACHE.
+//  2. Use this to separate module cache scanning from other scanning.
+func gomodcacheForEnv(goenv map[string]string) string {
+	if gmc := goenv["GOMODCACHE"]; gmc != "" {
+		return gmc
+	}
+	gopaths := filepath.SplitList(goenv["GOPATH"])
+	if len(gopaths) == 0 {
+		return ""
+	}
+	return filepath.Join(gopaths[0], "/pkg/mod")
 }
 
 func (r *ModuleResolver) initAllMods() error {
@@ -271,21 +271,27 @@ func (r *ModuleResolver) ClearForNewScan() {
 	r.scanSema <- struct{}{}
 }
 
-// ClearForNewMod invalidates resolver state that depends on the go.mod file
-// (essentially, the output of go list -m -json ...).
+// ClearModuleInfo invalidates resolver state that depends on go.mod file
+// contents (essentially, the output of go list -m -json ...).
 //
 // Notably, it does not forget directory contents, which are reset
 // asynchronously via ClearForNewScan.
-func (r *ModuleResolver) ClearForNewMod() {
-	<-r.scanSema
-	*r = ModuleResolver{
-		env:              r.env,
-		moduleCacheCache: r.moduleCacheCache,
-		otherCache:       r.otherCache,
-		scanSema:         r.scanSema,
+//
+// If the ProcessEnv is a GOPATH environment, ClearModuleInfo is a no op.
+//
+// TODO(rfindley): move this to a new env.go, consolidating ProcessEnv methods.
+func (e *ProcessEnv) ClearModuleInfo() {
+	if r, ok := e.resolver.(*ModuleResolver); ok {
+		resolver, resolverErr := newModuleResolver(e)
+		if resolverErr == nil {
+			<-r.scanSema // guards caches
+			resolver.moduleCacheCache = r.moduleCacheCache
+			resolver.otherCache = r.otherCache
+			r.scanSema <- struct{}{}
+		}
+		e.resolver = resolver
+		e.resolverErr = resolverErr
 	}
-	r.init()
-	r.scanSema <- struct{}{}
 }
 
 // findPackage returns the module and directory that contains the package at
@@ -355,10 +361,6 @@ func (r *ModuleResolver) cacheStore(info directoryPackageInfo) {
 	}
 }
 
-func (r *ModuleResolver) cacheKeys() []string {
-	return append(r.moduleCacheCache.Keys(), r.otherCache.Keys()...)
-}
-
 // cachePackageName caches the package name for a dir already in the cache.
 func (r *ModuleResolver) cachePackageName(info directoryPackageInfo) (string, error) {
 	if info.rootType == gopathwalk.RootModuleCache {
@@ -469,9 +471,6 @@ func (r *ModuleResolver) dirInModuleCache(dir string) bool {
 }
 
 func (r *ModuleResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) {
-	if err := r.init(); err != nil {
-		return nil, err
-	}
 	names := map[string]string{}
 	for _, path := range importPaths {
 		_, packageDir := r.findPackage(path)
@@ -491,10 +490,6 @@ func (r *ModuleResolver) scan(ctx context.Context, callback *scanCallback) error
 	ctx, done := event.Start(ctx, "imports.ModuleResolver.scan")
 	defer done()
 
-	if err := r.init(); err != nil {
-		return err
-	}
-
 	processDir := func(info directoryPackageInfo) {
 		// Skip this directory if we were not able to get the package information successfully.
 		if scanned, err := info.reachedStatus(directoryScanned); !scanned || err != nil {
@@ -672,9 +667,6 @@ func (r *ModuleResolver) canonicalize(info directoryPackageInfo) (*pkg, error) {
 }
 
 func (r *ModuleResolver) loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []string, error) {
-	if err := r.init(); err != nil {
-		return "", nil, err
-	}
 	if info, ok := r.cacheLoad(pkg.dir); ok && !includeTest {
 		return r.cacheExports(ctx, r.env, info)
 	}