From 49729134c3c7a07607732d00fb9aa879ed7ce5df Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Mon, 26 Oct 2020 17:57:45 -0400 Subject: [PATCH] internal/lsp: unify go command invocation logic We have two parallel code paths for Load and other go command invocations. Unify them by introducing a mode argument to the various functions that run the go command, indicating the purpose of the invocation. That purpose can be used to infer what features should be enabled. In the future, I hope we can use the mode to decide whether mod file updates and network access should be allowed. Change-Id: I49c67fcefc9141287b78c56e9812ee6a8ac8378a Reviewed-on: https://go-review.googlesource.com/c/tools/+/265238 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/load.go | 65 +++------------------------ internal/lsp/cache/mod.go | 4 +- internal/lsp/cache/mod_tidy.go | 5 +-- internal/lsp/cache/snapshot.go | 65 ++++++++++++++++++++------- internal/lsp/command.go | 8 ++-- internal/lsp/source/gc_annotations.go | 2 +- internal/lsp/source/view.go | 22 ++++++++- 7 files changed, 82 insertions(+), 89 deletions(-) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 12a09b6bc1..0d07070df0 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -16,6 +16,7 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/packagesinternal" @@ -91,70 +92,14 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { defer done() cleanup := func() {} - wdir := s.view.rootURI.Filename() - var modFile string - var modURI span.URI - var modContent []byte - switch { - case s.workspaceMode()&usesWorkspaceModule != 0: - var ( - tmpDir span.URI - err error - ) - tmpDir, cleanup, err = s.tempWorkspaceModule(ctx) - if err != nil { - return err - } - wdir = tmpDir.Filename() - modURI = span.URIFromPath(filepath.Join(wdir, "go.mod")) - modContent, err = ioutil.ReadFile(modURI.Filename()) - if err != nil { - return err - } - case s.workspaceMode()&tempModfile != 0: - // -modfile is unsupported when there are > 1 modules in the workspace. - if len(s.modules) != 1 { - panic(fmt.Sprintf("unsupported use of -modfile, expected 1 module, got %v", len(s.modules))) - } - var mod *moduleRoot - for _, m := range s.modules { // range to access the only element - mod = m - } - modURI = mod.modURI - modFH, err := s.GetFile(ctx, mod.modURI) - if err != nil { - return err - } - modContent, err = modFH.Read() - if err != nil { - return err - } - var sumFH source.FileHandle - if mod.sumURI != "" { - sumFH, err = s.GetFile(ctx, mod.sumURI) - if err != nil { - return err - } - } - var tmpURI span.URI - tmpURI, cleanup, err = tempModFile(modFH, sumFH) - if err != nil { - return err - } - modFile = tmpURI.Filename() - } - - cfg := s.config(ctx, wdir) - packagesinternal.SetModFile(cfg, modFile) - modMod, err := s.needsModEqualsMod(ctx, modURI, modContent) + _, inv, cleanup, err := s.goCommandInvocation(ctx, source.ForTypeChecking, &gocommand.Invocation{ + WorkingDir: s.view.rootURI.Filename(), + }) if err != nil { return err } - if modMod { - packagesinternal.SetModFlag(cfg, "mod") - } - + cfg := s.config(ctx, inv) pkgs, err := packages.Load(cfg, query...) cleanup() diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 736b56bbab..f95590f6cd 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -238,7 +238,7 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string for _, req := range pm.File.Require { inv.Args = append(inv.Args, req.Mod.Path) } - stdout, err := snapshot.RunGoCommandDirect(ctx, inv) + stdout, err := snapshot.RunGoCommandDirect(ctx, source.Normal, inv) if err != nil { return &modWhyData{err: err} } @@ -336,7 +336,7 @@ func (s *snapshot) ModUpgrade(ctx context.Context, fh source.FileHandle) (map[st // (see golang/go#38711). inv.ModFlag = "readonly" } - stdout, err := snapshot.RunGoCommandDirect(ctx, inv) + stdout, err := snapshot.RunGoCommandDirect(ctx, source.Normal, inv) if err != nil { return &modUpgradeData{err: err} } diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 375fddc6f5..9e92287852 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -57,9 +57,6 @@ func (s *snapshot) ModTidy(ctx context.Context, fh source.FileHandle) (*source.T if fh.Kind() != source.Mod { return nil, fmt.Errorf("%s is not a go.mod file", fh.URI()) } - if s.workspaceMode()&tempModfile == 0 { - return nil, source.ErrTmpModfileUnsupported - } if handle := s.getModTidyHandle(fh.URI()); handle != nil { return handle.tidy(ctx, s) } @@ -118,7 +115,7 @@ func (s *snapshot) ModTidy(ctx context.Context, fh source.FileHandle) (*source.T Args: []string{"tidy"}, WorkingDir: filepath.Dir(fh.URI().Filename()), } - tmpURI, inv, cleanup, err := snapshot.goCommandInvocation(ctx, true, inv) + tmpURI, inv, cleanup, err := snapshot.goCommandInvocation(ctx, source.WriteTemporaryModFile, inv) if err != nil { return &modTidyData{err: err} } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 34d78a17bc..27b8ba1909 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -182,18 +182,16 @@ func (s *snapshot) workspaceMode() workspaceMode { // TODO(rstambler): go/packages requires that we do not provide overlays for // multiple modules in on config, so buildOverlay needs to filter overlays by // module. -func (s *snapshot) config(ctx context.Context, dir string) *packages.Config { +func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packages.Config { s.view.optionsMu.Lock() - env := s.view.options.EnvSlice() - buildFlags := append([]string{}, s.view.options.BuildFlags...) verboseOutput := s.view.options.VerboseOutput s.view.optionsMu.Unlock() cfg := &packages.Config{ Context: ctx, - Dir: dir, - Env: append(append([]string{}, env...), "GO111MODULE="+s.view.go111module), - BuildFlags: buildFlags, + Dir: inv.WorkingDir, + Env: inv.Env, + BuildFlags: inv.BuildFlags, Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles | @@ -213,6 +211,8 @@ func (s *snapshot) config(ctx context.Context, dir string) *packages.Config { }, Tests: true, } + packagesinternal.SetModFile(cfg, inv.ModFile) + packagesinternal.SetModFlag(cfg, inv.ModFlag) // We want to type check cgo code if go/types supports it. if typesinternal.SetUsesCgo(&types.Config{}) { cfg.Mode |= packages.LoadMode(packagesinternal.TypecheckCgo) @@ -221,8 +221,8 @@ func (s *snapshot) config(ctx context.Context, dir string) *packages.Config { return cfg } -func (s *snapshot) RunGoCommandDirect(ctx context.Context, inv *gocommand.Invocation) (*bytes.Buffer, error) { - _, inv, cleanup, err := s.goCommandInvocation(ctx, false, inv) +func (s *snapshot) RunGoCommandDirect(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation) (*bytes.Buffer, error) { + _, inv, cleanup, err := s.goCommandInvocation(ctx, mode, inv) if err != nil { return nil, err } @@ -231,8 +231,8 @@ func (s *snapshot) RunGoCommandDirect(ctx context.Context, inv *gocommand.Invoca return s.view.session.gocmdRunner.Run(ctx, *inv) } -func (s *snapshot) RunGoCommandPiped(ctx context.Context, inv *gocommand.Invocation, stdout, stderr io.Writer) error { - _, inv, cleanup, err := s.goCommandInvocation(ctx, true, inv) +func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation, stdout, stderr io.Writer) error { + _, inv, cleanup, err := s.goCommandInvocation(ctx, mode, inv) if err != nil { return err } @@ -240,16 +240,49 @@ func (s *snapshot) RunGoCommandPiped(ctx context.Context, inv *gocommand.Invocat return s.view.session.gocmdRunner.RunPiped(ctx, *inv, stdout, stderr) } -func (s *snapshot) goCommandInvocation(ctx context.Context, allowTempModfile bool, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { +func (s *snapshot) goCommandInvocation(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { s.view.optionsMu.Lock() - env := s.view.options.EnvSlice() + inv.Env = append(append(append([]string{}, s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.go111module) + inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...) s.view.optionsMu.Unlock() - cleanup = func() {} // fallback - inv.Env = append(append(append([]string{}, env...), inv.Env...), "GO111MODULE="+s.view.go111module) - modURI := s.GoModForFile(ctx, span.URIFromPath(inv.WorkingDir)) - if allowTempModfile && s.workspaceMode()&tempModfile != 0 { + var modURI span.URI + if s.workspaceMode()&moduleMode != 0 { + // 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.ForTypeChecking { + if s.workspaceMode()&usesWorkspaceModule == 0 { + var mod *moduleRoot + for _, m := range s.modules { // range to access the only element + mod = m + } + modURI = mod.modURI + } else { + var tmpDir span.URI + var err error + tmpDir, cleanup, err = s.tempWorkspaceModule(ctx) + if err != nil { + return "", nil, cleanup, err + } + inv.WorkingDir = tmpDir.Filename() + modURI = span.URIFromPath(filepath.Join(tmpDir.Filename(), "go.mod")) + } + } else { + modURI = s.GoModForFile(ctx, span.URIFromPath(inv.WorkingDir)) + } + } + + wantTempMod := mode != source.UpdateUserModFile + needTempMod := mode == source.WriteTemporaryModFile + tempMod := wantTempMod && s.workspaceMode()&tempModfile != 0 + if needTempMod && !tempMod { + return "", nil, cleanup, source.ErrTmpModfileUnsupported + } + + if tempMod { if modURI == "" { return "", nil, cleanup, fmt.Errorf("no go.mod file found in %s", inv.WorkingDir) } diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 80e762c544..134d3fd8d8 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -265,7 +265,7 @@ func (s *Server) directGoModCommand(ctx context.Context, uri protocol.DocumentUR } snapshot, release := view.Snapshot(ctx) defer release() - _, err = snapshot.RunGoCommandDirect(ctx, &gocommand.Invocation{ + _, err = snapshot.RunGoCommandDirect(ctx, source.UpdateUserModFile, &gocommand.Invocation{ Verb: verb, Args: args, WorkingDir: filepath.Dir(uri.SpanURI().Filename()), @@ -296,7 +296,7 @@ func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri pro Args: []string{pkgPath, "-v", "-count=1", "-run", fmt.Sprintf("^%s$", funcName)}, WorkingDir: filepath.Dir(uri.SpanURI().Filename()), } - if err := snapshot.RunGoCommandPiped(ctx, inv, out, out); err != nil { + if err := snapshot.RunGoCommandPiped(ctx, source.Normal, inv, out, out); err != nil { if errors.Is(err, context.Canceled) { return err } @@ -312,7 +312,7 @@ func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri pro Args: []string{pkgPath, "-v", "-run=^$", "-bench", fmt.Sprintf("^%s$", funcName)}, WorkingDir: filepath.Dir(uri.SpanURI().Filename()), } - if err := snapshot.RunGoCommandPiped(ctx, inv, out, out); err != nil { + if err := snapshot.RunGoCommandPiped(ctx, source.Normal, inv, out, out); err != nil { if errors.Is(err, context.Canceled) { return err } @@ -367,7 +367,7 @@ func (s *Server) runGoGenerate(ctx context.Context, snapshot source.Snapshot, di WorkingDir: dir.Filename(), } stderr := io.MultiWriter(er, workDoneWriter{work}) - if err := snapshot.RunGoCommandPiped(ctx, inv, er, stderr); err != nil { + if err := snapshot.RunGoCommandPiped(ctx, source.Normal, inv, er, stderr); err != nil { return err } return nil diff --git a/internal/lsp/source/gc_annotations.go b/internal/lsp/source/gc_annotations.go index 0a0d2b50cb..8bf8ad73ca 100644 --- a/internal/lsp/source/gc_annotations.go +++ b/internal/lsp/source/gc_annotations.go @@ -46,7 +46,7 @@ func GCOptimizationDetails(ctx context.Context, snapshot Snapshot, pkgDir span.U }, WorkingDir: pkgDir.Filename(), } - _, err = snapshot.RunGoCommandDirect(ctx, inv) + _, err = snapshot.RunGoCommandDirect(ctx, Normal, inv) if err != nil { return nil, err } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 2f3a212af1..8a12e0c832 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -84,11 +84,11 @@ type Snapshot interface { // RunGoCommandPiped runs the given `go` command, writing its output // to stdout and stderr. Verb, Args, and WorkingDir must be specified. - RunGoCommandPiped(ctx context.Context, inv *gocommand.Invocation, stdout, stderr io.Writer) error + RunGoCommandPiped(ctx context.Context, mode InvocationMode, inv *gocommand.Invocation, stdout, stderr io.Writer) error // RunGoCommandDirect runs the given `go` command. Verb, Args, and // WorkingDir must be specified. - RunGoCommandDirect(ctx context.Context, inv *gocommand.Invocation) (*bytes.Buffer, error) + RunGoCommandDirect(ctx context.Context, mode InvocationMode, inv *gocommand.Invocation) (*bytes.Buffer, error) // RunProcessEnvFunc runs fn with the process env for this snapshot's view. // Note: the process env contains cached module and filesystem state. @@ -169,6 +169,24 @@ const ( WidestPackage ) +// InvocationMode represents the goal of a particular go command invocation. +type InvocationMode int + +const ( + // Normal is appropriate for commands that might be run by a user and don't + // deliberately modify go.mod files, e.g. `go test`. + Normal = iota + // UpdateUserModFile is for commands that intend to update the user's real + // go.mod file, e.g. `go mod tidy` in response to a user's request to tidy. + UpdateUserModFile + // WriteTemporaryModFile is for commands that need information from a + // modified version of the user's go.mod file, e.g. `go mod tidy` used to + // generate diagnostics. + WriteTemporaryModFile + // ForTypeChecking is for packages.Load. + ForTypeChecking +) + // View represents a single workspace. // This is the level at which we maintain configuration like working directory // and build tags.