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

Whenever a view.importsState value is created, we use
snapshot.goCommandInvocation to extract information used to run the go
command in the imports.ProcessEnv. Before this CL, that process acquired
the current snapshot (whatever it was), and held onto it for the purpose
of sharing a workspace directory. As a consequence all the memory in
that snapshot was pinned for the lifecycle of the importsState, which
can be the entire editing session. This results in a memory leak as
information in the session is invalidated.

Fix this by creating a copy of the workspace directory to be owned by
the importsState.

Also:
 - Add some TODOs
 - Clean up some stale comments

Fixes golang/go#53780

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

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()
 
-	return func() {
-		cleanupInvocation()
-		release()
-	}, nil
+	// 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 cleanup, nil
 }
 
 func (s *importsState) refreshProcessEnv() {

internal/lsp/cache/snapshot.go

@@ -383,6 +383,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): remove unused cleanup mechanism.
 func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.InvocationFlags, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) {
 	s.view.optionsMu.Lock()
@@ -412,7 +417,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
@@ -463,6 +467,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
@@ -498,13 +505,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)
@@ -525,6 +534,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()