From b6e495100ec74eb3127d0b0af75ec441e2979077 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 11 Jul 2022 21:58:26 +0000 Subject: [PATCH] Revert "internal/lsp/cache: don't pin a snapshot to view.importsState" This reverts commit 42457a544a678826371e9ee4f874257a54314320. Reason for revert: test flakes (golang/go#53796) Change-Id: I9d7061220b43f9de88060a0bba5c5635d92fe3d9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/416879 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Gopher Robot --- internal/lsp/cache/imports.go | 44 ++++++++++------------------------ internal/lsp/cache/snapshot.go | 40 +++++-------------------------- 2 files changed, 18 insertions(+), 66 deletions(-) diff --git a/internal/lsp/cache/imports.go b/internal/lsp/cache/imports.go index 7877c4f074..710a1f3407 100644 --- a/internal/lsp/cache/imports.go +++ b/internal/lsp/cache/imports.go @@ -7,7 +7,6 @@ package cache import ( "context" "fmt" - "os" "reflect" "strings" "sync" @@ -142,20 +141,21 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho pe.Logf = nil } - // 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. + // 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() _, 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,31 +164,11 @@ 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 cleanup, nil + return func() { + cleanupInvocation() + release() + }, nil } func (s *importsState) refreshProcessEnv() { diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index c2438cdf20..a5cb74355e 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -398,11 +398,6 @@ 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. @@ -434,6 +429,7 @@ 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 @@ -484,9 +480,6 @@ 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 @@ -522,15 +515,13 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat return "", nil, cleanup, source.ErrTmpModfileUnsupported } - // 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. + // 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. useWorkFile := !needTempMod && s.workspace.moduleSource == goWorkWorkspace && s.view.goversion >= 18 if useWorkFile { - // Since we're running in the workspace root, the go command will resolve GOWORK automatically. + // TODO(#51215): build a temp workfile and set GOWORK in the environment. } else if useTempMod { if modURI == "" { return "", nil, cleanup, fmt.Errorf("no go.mod file found in %s", inv.WorkingDir) @@ -551,25 +542,6 @@ 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()