From 597b165f5f33b22d3bf1a6015a1b48aeb46077fb Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 1 Feb 2022 13:47:20 -0500 Subject: [PATCH] internal/lsp/cache: use -workfile on 1.18 When using go 1.18 with go.work files, don't fake workspaces by creating a workspace module: just run from the workspace root and pass -workfile. For golang/go#44696 Change-Id: Iaa1979d26b1ce67c6e18e0bf26546a504069da8c Reviewed-on: https://go-review.googlesource.com/c/tools/+/382241 Trust: Robert Findley Run-TryBot: Robert Findley Reviewed-by: Michael Matloob gopls-CI: kokoro TryBot-Result: Gopher Robot --- .../regtest/workspace/workspace_test.go | 25 ++++--- internal/gocommand/invoke.go | 30 ++++++-- internal/lsp/cache/imports.go | 7 +- internal/lsp/cache/snapshot.go | 69 ++++++++++++++----- internal/lsp/cache/view.go | 31 +++++++-- internal/lsp/cache/workspace.go | 9 +-- 6 files changed, 129 insertions(+), 42 deletions(-) diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index a32a9644b4..93c5cac750 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -727,15 +727,20 @@ use ( env.OpenFile("modb/go.mod") env.Await(env.DoneWithOpen()) - var d protocol.PublishDiagnosticsParams - env.Await( - OnceMet( - env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"), - ReadDiagnostics("modb/go.mod", &d), - ), - ) - env.ApplyQuickFixes("modb/go.mod", d.Diagnostics) - env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x")) + // TODO(golang/go#50862): the go command drops error messages when using + // go.work, so we need to build our go.mod diagnostics in a different way. + if testenv.Go1Point() < 18 { + var d protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"), + ReadDiagnostics("modb/go.mod", &d), + ), + ) + env.ApplyQuickFixes("modb/go.mod", d.Diagnostics) + env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x")) + } + // Jumping to definition should now go to b.com in the workspace. if err := checkHelloLocation("modb/b/b.go"); err != nil { t.Fatal(err) @@ -755,7 +760,7 @@ use ( // should clear outstanding diagnostics... env.Await(OnceMet( env.DoneWithChange(), - EmptyDiagnostics("modb/go.mod"), + EmptyOrNoDiagnostics("modb/go.mod"), )) // ...but does not yet cause a workspace reload, so we should still jump to modb. if err := checkHelloLocation("modb/b/b.go"); err != nil { diff --git a/internal/gocommand/invoke.go b/internal/gocommand/invoke.go index 8659a0c5da..64fbad3765 100644 --- a/internal/gocommand/invoke.go +++ b/internal/gocommand/invoke.go @@ -9,7 +9,6 @@ import ( "bytes" "context" "fmt" - exec "golang.org/x/sys/execabs" "io" "os" "regexp" @@ -18,6 +17,8 @@ import ( "sync" "time" + exec "golang.org/x/sys/execabs" + "golang.org/x/tools/internal/event" ) @@ -131,9 +132,19 @@ type Invocation struct { Verb string Args []string BuildFlags []string - ModFlag string - ModFile string - Overlay string + + // If ModFlag is set, the go command is invoked with -mod=ModFlag. + ModFlag string + + // If ModFile is set, the go command is invoked with -modfile=ModFile. + ModFile string + + // If WorkFile is set, the go command is invoked with -workfile=WorkFile. + WorkFile string + + // If Overlay is set, the go command is invoked with -overlay=Overlay. + Overlay string + // If CleanEnv is set, the invocation will run only with the environment // in Env, not starting with os.Environ. CleanEnv bool @@ -159,6 +170,9 @@ func (i *Invocation) runWithFriendlyError(ctx context.Context, stdout, stderr io } func (i *Invocation) run(ctx context.Context, stdout, stderr io.Writer) error { + if i.ModFile != "" && i.WorkFile != "" { + return fmt.Errorf("bug: go command invoked with both -modfile and -workfile") + } log := i.Logf if log == nil { log = func(string, ...interface{}) {} @@ -171,6 +185,11 @@ func (i *Invocation) run(ctx context.Context, stdout, stderr io.Writer) error { goArgs = append(goArgs, "-modfile="+i.ModFile) } } + appendWorkFile := func() { + if i.WorkFile != "" { + goArgs = append(goArgs, "-workfile="+i.WorkFile) + } + } appendModFlag := func() { if i.ModFlag != "" { goArgs = append(goArgs, "-mod="+i.ModFlag) @@ -189,16 +208,19 @@ func (i *Invocation) run(ctx context.Context, stdout, stderr io.Writer) error { // mod needs the sub-verb before flags. goArgs = append(goArgs, i.Args[0]) appendModFile() + appendWorkFile() goArgs = append(goArgs, i.Args[1:]...) case "get": goArgs = append(goArgs, i.BuildFlags...) appendModFile() + appendWorkFile() goArgs = append(goArgs, i.Args...) default: // notably list and build. goArgs = append(goArgs, i.BuildFlags...) appendModFile() appendModFlag() + appendWorkFile() appendOverlayFlag() goArgs = append(goArgs, i.Args...) } diff --git a/internal/lsp/cache/imports.go b/internal/lsp/cache/imports.go index b8e0146782..01a2468ef3 100644 --- a/internal/lsp/cache/imports.go +++ b/internal/lsp/cache/imports.go @@ -39,7 +39,12 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot // is slightly wasteful, since we'll drop caches a little too often, but // the mod file shouldn't be changing while people are autocompleting. var modFileHash string - if snapshot.workspaceMode()&usesWorkspaceModule == 0 { + // If we are using 'legacyWorkspace' mode, we can just read the modfile from + // the snapshot. Otherwise, we need to get the synthetic workspace mod file. + // + // TODO(rfindley): we should be able to just always use the synthetic + // workspace module, or alternatively use the go.work file. + if snapshot.workspace.moduleSource == legacyWorkspace { for m := range snapshot.workspace.getActiveModFiles() { // range to access the only element modFH, err := snapshot.GetFile(ctx, m) if err != nil { diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index e913c3e16f..133843b399 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -198,18 +198,6 @@ func (s *snapshot) workspaceMode() workspaceMode { if options.TempModfile && s.view.workspaceInformation.goversion >= 14 { mode |= tempModfile } - // If the user is intentionally limiting their workspace scope, don't - // enable multi-module workspace mode. - // TODO(rstambler): This should only change the calculation of the root, - // not the mode. - if !options.ExpandWorkspaceToModule { - return mode - } - // The workspace module has been disabled by the user. - if s.workspace.moduleSource != goWorkWorkspace && s.workspace.moduleSource != goplsModWorkspace && !options.ExperimentalWorkspaceModule { - return mode - } - mode |= usesWorkspaceModule return mode } @@ -334,17 +322,41 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat inv.Env = append(inv.Env, "GOPROXY=off") } + // What follows is rather complicated logic for how to actually run the go + // command. A word of warning: this is the result of various incremental + // features added to gopls, and varying behavior of the Go command across Go + // versions. It can surely be cleaned up significantly, but tread carefully. + // + // Roughly speaking we need to resolve four things: + // - 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 + // version, and what we're actually trying to achieve (the + // source.InvocationFlags). + var modURI span.URI // Select the module context to use. // If we're type checking, we need to use the workspace context, meaning // the main (workspace) module. Otherwise, we should use the module for // the passed-in working dir. if mode == source.LoadWorkspace { - if s.workspaceMode()&usesWorkspaceModule == 0 { + switch s.workspace.moduleSource { + case legacyWorkspace: for m := range s.workspace.getActiveModFiles() { // range to access the only element modURI = m } - } else { + case goWorkWorkspace: + if s.view.goversion >= 18 { + break + } + // Before go 1.18, the Go command did not natively support go.work files, + // so we 'fake' them with a workspace module. + fallthrough + case fileSystemWorkspace, goplsModWorkspace: var tmpDir span.URI var err error tmpDir, err = s.getWorkspaceDir(ctx) @@ -375,9 +387,9 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat return "", nil, cleanup, err } + mutableModFlag := "" // If the mod flag isn't set, populate it based on the mode and workspace. if inv.ModFlag == "" { - mutableModFlag := "" if s.view.goversion >= 16 { mutableModFlag = "mod" } @@ -396,13 +408,32 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat } } - needTempMod := mode == source.WriteTemporaryModFile - tempMod := s.workspaceMode()&tempModfile != 0 - if needTempMod && !tempMod { + // Only use a temp mod file if the modfile can actually be mutated. + needTempMod := inv.ModFlag == mutableModFlag + useTempMod := s.workspaceMode()&tempModfile != 0 + if needTempMod && !useTempMod { return "", nil, cleanup, source.ErrTmpModfileUnsupported } - if tempMod { + // 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 { + workURI := uriForSource(s.workspace.root, goWorkWorkspace) + workFH, err := s.GetFile(ctx, workURI) + if err != nil { + return "", nil, cleanup, err + } + // TODO(rfindley): we should use the last workfile that actually parsed, as + // tracked by the workspace. + tmpURI, cleanup, err = tempWorkFile(workFH) + if err != nil { + return "", nil, cleanup, err + } + inv.WorkFile = tmpURI.Filename() + } else if useTempMod { if modURI == "" { return "", nil, cleanup, fmt.Errorf("no go.mod file found in %s", inv.WorkingDir) } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index b2648157cf..ed3d5eccbe 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -137,10 +137,6 @@ const ( // tempModfile indicates whether or not the -modfile flag should be used. tempModfile - - // usesWorkspaceModule indicates support for the experimental workspace module - // feature. - usesWorkspaceModule ) type builtinPackageHandle struct { @@ -171,6 +167,33 @@ func (f *fileBase) addURI(uri span.URI) int { func (v *View) ID() string { return v.id } +// TODO(rfindley): factor this out to use server.tempDir, and consolidate logic with tempModFile. +func tempWorkFile(workFH source.FileHandle) (tmpURI span.URI, cleanup func(), err error) { + filenameHash := hashContents([]byte(workFH.URI().Filename())) + tmpMod, err := ioutil.TempFile("", fmt.Sprintf("go.%s.*.work", filenameHash)) + if err != nil { + return "", nil, err + } + defer tmpMod.Close() + + tmpURI = span.URIFromPath(tmpMod.Name()) + + content, err := workFH.Read() + if err != nil { + return "", nil, err + } + + if _, err := tmpMod.Write(content); err != nil { + return "", nil, err + } + + cleanup = func() { + _ = os.Remove(tmpURI.Filename()) + } + + return tmpURI, cleanup, nil +} + // tempModFile creates a temporary go.mod file based on the contents of the // given go.mod file. It is the caller's responsibility to clean up the files // when they are done using them. diff --git a/internal/lsp/cache/workspace.go b/internal/lsp/cache/workspace.go index 853e37e05e..1f29cc9f27 100644 --- a/internal/lsp/cache/workspace.go +++ b/internal/lsp/cache/workspace.go @@ -21,13 +21,14 @@ import ( errors "golang.org/x/xerrors" ) +// workspaceSource reports how the set of active modules has been derived. type workspaceSource int const ( - legacyWorkspace = iota - goplsModWorkspace - goWorkWorkspace - fileSystemWorkspace + legacyWorkspace = iota // non-module or single module mode + goplsModWorkspace // modules provided by a gopls.mod file + goWorkWorkspace // modules provided by a go.work file + fileSystemWorkspace // modules scanned from the filesystem ) func (s workspaceSource) String() string {