internal/imports: remove the unused ProcessEnv.ModFile field

Remove the ProcessEnv.ModFile field, which was never set. Also add
additional comments and superficial cleanup.

Change-Id: I45d63e5039043d01d4280d2d916df6794e437d8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559506
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

gopls/internal/cache/session.go

@@ -201,9 +201,11 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, *
 			}
 		}
 		pe = &imports.ProcessEnv{
-			GocmdRunner:    s.gocmdRunner,
-			BuildFlags:     slices.Clone(def.folder.Options.BuildFlags),
-			ModFlag:        "readonly", // processEnv operations should not mutate the modfile
+			GocmdRunner: s.gocmdRunner,
+			BuildFlags:  slices.Clone(def.folder.Options.BuildFlags),
+			// TODO(rfindley): an old comment said "processEnv operations should not mutate the modfile"
+			// But shouldn't we honor the default behavior of mod vendoring?
+			ModFlag:        "readonly",
 			SkipPathInScan: skipPath,
 			Env:            env,
 			WorkingDir:     def.root.Path(),

internal/imports/fix.go

@@ -852,7 +852,6 @@ type ProcessEnv struct {
 
 	BuildFlags []string
 	ModFlag    string
-	ModFile    string
 
 	// SkipPathInScan returns true if the path should be skipped from scans of
 	// the RootCurrentModule root type. The function argument is a clean,
@@ -960,6 +959,12 @@ func (e *ProcessEnv) GetResolver() (Resolver, error) {
 	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
@@ -968,6 +973,10 @@ func (e *ProcessEnv) GetResolver() (Resolver, error) {
 	return e.resolver, nil
 }
 
+// buildContext returns the build.Context to use for matching files.
+//
+// TODO(rfindley): support dynamic GOOS, GOARCH here, when doing cross-platform
+// development.
 func (e *ProcessEnv) buildContext() (*build.Context, error) {
 	ctx := build.Default
 	goenv, err := e.goEnv()

internal/imports/mod.go

@@ -34,7 +34,7 @@ import (
 //  - Split scanning the module cache from other ModuleResolver functionality,
 //    as it is the source of performance woes (and inconsistency).
 //  - Allow sharing module cache state across multiple ModuleResolvers.
-//  - Optimize the scan itself, as there is a lot of redundancy statting and
+//  - Optimize the scan itself, as there is some redundancy statting and
 //    reading go.mod files.
 //  - Make it possible to reuse the current state while running a refresh in
 //    the background.
@@ -109,7 +109,6 @@ func (r *ModuleResolver) init() error {
 	inv := gocommand.Invocation{
 		BuildFlags: r.env.BuildFlags,
 		ModFlag:    r.env.ModFlag,
-		ModFile:    r.env.ModFile,
 		Env:        r.env.env(),
 		Logf:       r.env.Logf,
 		WorkingDir: r.env.WorkingDir,

internal/imports/mod_test.go

@@ -17,6 +17,7 @@ import (
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"golang.org/x/mod/module"
 	"golang.org/x/tools/internal/gocommand"
@@ -1291,14 +1292,17 @@ import (
 func BenchmarkScanModCache(b *testing.B) {
 	env := &ProcessEnv{
 		GocmdRunner: &gocommand.Runner{},
-		Logf:        b.Logf,
+		// Uncomment for verbose logging (too verbose to enable by default).
+		// Logf:        b.Logf,
 	}
 	exclude := []gopathwalk.RootType{gopathwalk.RootGOROOT}
 	resolver, err := env.GetResolver()
 	if err != nil {
 		b.Fatal(err)
 	}
+	start := time.Now()
 	scanToSlice(resolver, exclude)
+	b.Logf("warming the mod cache took %v", time.Since(start))
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		scanToSlice(resolver, exclude)