Skip to content

Commit

Permalink
internal/imports: merge init and newModuleResolver
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@findleyr
findleyr committed Feb 6, 2024
1 parent 4f6024e commit f6dc1e9
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 101 deletions.
7 changes: 1 addition & 6 deletions gopls/internal/cache/imports.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
56 changes: 37 additions & 19 deletions internal/imports/fix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
108 changes: 50 additions & 58 deletions internal/imports/mod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
}
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down
Loading

0 comments on commit f6dc1e9

Please sign in to comment.