gopls/internal/lsp/cache: always init the resolver in runProcessEnvFunc

The current logic populates the process env *after* clearing, but
clearing uses fields in the process env. To work around this, clearing
is not performed on the first run.

Chesterton's fence may apply: I don't really understand this, but don't
see how it could be correct: if the clearing requires the environment,
then the environment should be populated before clearing.

For golang/go#59216

Change-Id: Ia0bc0de10284abf9853158e4f7e60de3d338083d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/492679
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/imports.go

@@ -29,13 +29,6 @@ type importsState struct {
 	cachedModFileHash      source.Hash
 	cachedBuildFlags       []string
 	cachedDirectoryFilters []string
-
-	// runOnce records whether runProcessEnvFunc has been called at least once.
-	// This is necessary to avoid resetting state before the process env is
-	// populated.
-	//
-	// TODO(rfindley): this shouldn't be necessary.
-	runOnce bool
 }
 
 func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot, fn func(*imports.Options) error) error {
@@ -72,24 +65,19 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 	// update the processEnv. Clearing caches blocks on any background
 	// scans.
 	if changed {
-		// As a special case, skip cleanup the first time -- we haven't fully
-		// initialized the environment yet and calling GetResolver will do
-		// unnecessary work and potentially mess up the go.mod file.
-		if s.runOnce {
-			if resolver, err := s.processEnv.GetResolver(); err == nil {
-				if modResolver, ok := resolver.(*imports.ModuleResolver); ok {
-					modResolver.ClearForNewMod()
-				}
+		if err := populateProcessEnvFromSnapshot(ctx, s.processEnv, snapshot); err != nil {
+			return err
+		}
+
+		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
-		if err := s.populateProcessEnv(ctx, snapshot); err != nil {
-			return err
-		}
-		s.runOnce = true
 	}
 
 	// Run the user function.
@@ -122,11 +110,10 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 	return nil
 }
 
-// populateProcessEnv sets the dynamically configurable fields for the view's
-// process environment. Assumes that the caller is holding the s.view.importsMu.
-func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapshot) error {
-	pe := s.processEnv
-
+// 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 {
 	if snapshot.view.Options().VerboseOutput {
 		pe.Logf = func(format string, args ...interface{}) {
 			event.Log(ctx, fmt.Sprintf(format, args...))