Revert "Revert "internal/lsp/cache: don't pin a snapshot to view.importsState"

This reverts CL 416879, which itself was a revert of CL 416874.

Reason for revert: failure is understood now, as described in 
https://github.com/golang/go/issues/53796#issuecomment-1181157704 and fixed in CL 416881.

Change-Id: I1d6a4ae46fbb1bf78e2f23656de7885b439f43fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416882
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

2 files changed, 66 insertions, 18 deletions

diff --git a/internal/lsp/cache/imports.go b/internal/lsp/cache/imports.go
index 710a1f340..7877c4f07 100644
--- a/internal/lsp/cache/imports.go
+++ b/internal/lsp/cache/imports.go
@@ -7,6 +7,7 @@ package cache
 import (
 	"context"
 	"fmt"
+	"os"
 	"reflect"
 	"strings"
 	"sync"
@@ -141,21 +142,20 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho
 		pe.Logf = nil
 	}
 
-	// Take an extra reference to the snapshot so that its workspace directory
-	// (if any) isn't destroyed while we're using it.
-	release := snapshot.Acquire()
+	// Extract invocation details from the snapshot to use with goimports.
+	//
+	// TODO(rfindley): refactor to extract the necessary invocation logic into
+	// separate functions. Using goCommandInvocation is unnecessarily indirect,
+	// and has led to memory leaks in the past, when the snapshot was
+	// unintentionally held past its lifetime.
 	_, inv, cleanupInvocation, err := snapshot.goCommandInvocation(ctx, source.LoadWorkspace, &gocommand.Invocation{
 		WorkingDir: snapshot.view.rootURI.Filename(),
 	})
 	if err != nil {
-		release()
 		return nil, err
 	}
-	pe.WorkingDir = inv.WorkingDir
+
 	pe.BuildFlags = inv.BuildFlags
-	pe.WorkingDir = inv.WorkingDir
-	pe.ModFile = inv.ModFile
-	pe.ModFlag = inv.ModFlag
 	pe.Env = map[string]string{}
 	for _, kv := range inv.Env {
 		split := strings.SplitN(kv, "=", 2)
@@ -164,11 +164,31 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho
 		}
 		pe.Env[split[0]] = split[1]
 	}
+	// We don't actually use the invocation, so clean it up now.
+	cleanupInvocation()
+
+	// If the snapshot uses a synthetic workspace directory, create a copy for
+	// the lifecycle of the importsState.
+	//
+	// Notably, we cannot use the snapshot invocation working directory, as that
+	// is tied to the lifecycle of the snapshot.
+	//
+	// Otherwise return a no-op cleanup function.
+	cleanup = func() {}
+	if snapshot.usesWorkspaceDir() {
+		tmpDir, err := makeWorkspaceDir(ctx, snapshot.workspace, snapshot)
+		if err != nil {
+			return nil, err
+		}
+		pe.WorkingDir = tmpDir
+		cleanup = func() {
+			os.RemoveAll(tmpDir) // ignore error
+		}
+	} else {
+		pe.WorkingDir = snapshot.view.rootURI.Filename()
+	}
 
-	return func() {
-		cleanupInvocation()
-		release()
-	}, nil
+	return cleanup, nil
 }
 
 func (s *importsState) refreshProcessEnv() {
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index a5cb74355..c2438cdf2 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -398,6 +398,11 @@ func (s *snapshot) RunGoCommands(ctx context.Context, allowNetwork bool, wd stri
 	return true, modBytes, sumBytes, nil
 }
 
+// goCommandInvocation populates inv with configuration for running go commands on the snapshot.
+//
+// TODO(rfindley): refactor this function to compose the required configuration
+// explicitly, rather than implicitly deriving it from flags and inv.
+//
 // TODO(adonovan): simplify cleanup mechanism. It's hard to see, but
 // it used only after call to tempModFile. Clarify that it is only
 // non-nil on success.
@@ -429,7 +434,6 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat
 	//  - the working directory.
 	//  - the -mod flag
 	//  - the -modfile flag
-	//  - the -workfile flag
 	//
 	// These are dependent on a number of factors: whether we need to run in a
 	// synthetic workspace, whether flags are supported at the current go
@@ -480,6 +484,9 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat
 		}
 	}
 
+	// TODO(rfindley): in the case of go.work mode, modURI is empty and we fall
+	// back on the default behavior of vendorEnabled with an empty modURI. Figure
+	// out what is correct here and implement it explicitly.
 	vendorEnabled, err := s.vendorEnabled(ctx, modURI, modContent)
 	if err != nil {
 		return "", nil, cleanup, err
@@ -515,13 +522,15 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat
 		return "", nil, cleanup, source.ErrTmpModfileUnsupported
 	}
 
-	// We should use -workfile if:
-	//  1. We're not actively trying to mutate a modfile.
-	//  2. We have an active go.work file.
-	//  3. We're using at least Go 1.18.
+	// We should use -modfile if:
+	//  - the workspace mode supports it
+	//  - we're using a go.work file on go1.18+, or we need a temp mod file (for
+	//    example, if running go mod tidy in a go.work workspace)
+	//
+	// TODO(rfindley): this is very hard to follow. Refactor.
 	useWorkFile := !needTempMod && s.workspace.moduleSource == goWorkWorkspace && s.view.goversion >= 18
 	if useWorkFile {
-		// TODO(#51215): build a temp workfile and set GOWORK in the environment.
+		// Since we're running in the workspace root, the go command will resolve GOWORK automatically.
 	} else if useTempMod {
 		if modURI == "" {
 			return "", nil, cleanup, fmt.Errorf("no go.mod file found in %s", inv.WorkingDir)
@@ -542,6 +551,25 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat
 	return tmpURI, inv, cleanup, nil
 }
 
+// usesWorkspaceDir reports whether the snapshot should use a synthetic
+// workspace directory for running workspace go commands such as go list.
+//
+// TODO(rfindley): this logic is duplicated with goCommandInvocation. Clean up
+// the latter, and deduplicate.
+func (s *snapshot) usesWorkspaceDir() bool {
+	switch s.workspace.moduleSource {
+	case legacyWorkspace:
+		return false
+	case goWorkWorkspace:
+		if s.view.goversion >= 18 {
+			return false
+		}
+		// Before go 1.18, the Go command did not natively support go.work files,
+		// so we 'fake' them with a workspace module.
+	}
+	return true
+}
+
 func (s *snapshot) buildOverlay() map[string][]byte {
 	s.mu.Lock()
 	defer s.mu.Unlock()