Skip to content

Commit

Permalink
gopls/internal/cache: simplify usage of snapshot.GoCommandInvocation
Browse files Browse the repository at this point in the history
The Snapshot.GoCommandInvocation API is a source of confusion, because
it passes in a gocommand.Invocation, performs arbitrary mutation, and
then passes it back to the caller (which may perform mutation on top).
Aside from the readability problems of this indirectness, this API can
lead to real bugs, for example if one of the mutations is incompatible
with another.

Furthermore, the fact that the GoCommandInvocation is the golden source
of information about the Go command environment leads to awkwardness and
redundant work. For example, to get the packages.Config we called
Snapshot.GoCommandInvocation, just to read env, working dir, and build
flags (and the now irrelevant ModFile and ModFlag). But
GoCommandInvocation wrote overlays, so we'd end up writing overlays
twice: once within the gopls layer, and then again in the go/packages go
list driver.

Simplify as follows:
- Pass in dir, verb, args, and env to GoCommandInvocation, to avoid
  passing around and mutating an Invocation.
- Extract the View.Env, which is a useful concept, and apply invocation
  env on top. Surveying existing use cases that this was correct, as all
  call sites expected their env not to be overwritten.
- Move Snapshot.config to load.go, where it belongs, and simplify not to
  depend on GoCommandInvocation.

Also add an additional test case for BenchmarkReload.

Change-Id: I8ae7a13a033360e0e7b0b24ff718b5a22123e99c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/626715
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
  • Loading branch information
findleyr committed Nov 14, 2024
1 parent 3c20e3f commit 29f4edb
Show file tree
Hide file tree
Showing 9 changed files with 167 additions and 178 deletions.
62 changes: 49 additions & 13 deletions gopls/internal/cache/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"context"
"errors"
"fmt"
"go/types"
"path/filepath"
"slices"
"sort"
Expand All @@ -25,8 +26,8 @@ import (
"golang.org/x/tools/gopls/internal/util/immutable"
"golang.org/x/tools/gopls/internal/util/pathutil"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/packagesinternal"
"golang.org/x/tools/internal/typesinternal"
"golang.org/x/tools/internal/xcontext"
)

Expand Down Expand Up @@ -57,6 +58,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
// Keep track of module query -> module path so that we can later correlate query
// errors with errors.
moduleQueries := make(map[string]string)

for _, scope := range scopes {
switch scope := scope.(type) {
case packageLoadScope:
Expand Down Expand Up @@ -118,21 +120,13 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc

startTime := time.Now()

inv, cleanupInvocation, err := s.GoCommandInvocation(allowNetwork, &gocommand.Invocation{
WorkingDir: s.view.root.Path(),
})
if err != nil {
return err
}
defer cleanupInvocation()

// Set a last resort deadline on packages.Load since it calls the go
// command, which may hang indefinitely if it has a bug. golang/go#42132
// and golang/go#42255 have more context.
ctx, cancel := context.WithTimeout(ctx, 10*time.Minute)
defer cancel()

cfg := s.config(ctx, inv)
cfg := s.config(ctx, allowNetwork)
pkgs, err := packages.Load(cfg, query...)

// If the context was canceled, return early. Otherwise, we might be
Expand Down Expand Up @@ -278,7 +272,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
if allFilesExcluded(pkg.GoFiles, filterFunc) {
continue
}
buildMetadata(newMetadata, pkg, cfg.Dir, standalone, s.view.typ != GoPackagesDriverView)
buildMetadata(newMetadata, cfg.Dir, standalone, pkg)
}

s.mu.Lock()
Expand Down Expand Up @@ -362,14 +356,56 @@ func (m *moduleErrorMap) Error() string {
return buf.String()
}

// config returns the configuration used for the snapshot's interaction with
// the go/packages API. It uses the given working directory.
//
// TODO(rstambler): go/packages requires that we do not provide overlays for
// multiple modules in one config, so buildOverlay needs to filter overlays by
// module.
// TODO(rfindley): ^^ is this still true?
func (s *Snapshot) config(ctx context.Context, allowNetwork bool) *packages.Config {
cfg := &packages.Config{
Context: ctx,
Dir: s.view.root.Path(),
Env: s.view.Env(),
BuildFlags: slices.Clone(s.view.folder.Options.BuildFlags),
Mode: packages.NeedName |
packages.NeedFiles |
packages.NeedCompiledGoFiles |
packages.NeedImports |
packages.NeedDeps |
packages.NeedTypesSizes |
packages.NeedModule |
packages.NeedEmbedFiles |
packages.LoadMode(packagesinternal.DepsErrors) |
packages.NeedForTest,
Fset: nil, // we do our own parsing
Overlay: s.buildOverlays(),
Logf: func(format string, args ...interface{}) {
if s.view.folder.Options.VerboseOutput {
event.Log(ctx, fmt.Sprintf(format, args...))
}
},
Tests: true,
}
if !allowNetwork {
cfg.Env = append(cfg.Env, "GOPROXY=off")
}
// We want to type check cgo code if go/types supports it.
if typesinternal.SetUsesCgo(&types.Config{}) {
cfg.Mode |= packages.LoadMode(packagesinternal.TypecheckCgo)
}
return cfg
}

// buildMetadata populates the updates map with metadata updates to
// apply, based on the given pkg. It recurs through pkg.Imports to ensure that
// metadata exists for all dependencies.
//
// Returns the metadata.Package that was built (or which was already present in
// updates), or nil if the package could not be built. Notably, the resulting
// metadata.Package may have an ID that differs from pkg.ID.
func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone, goListView bool) *metadata.Package {
func buildMetadata(updates map[PackageID]*metadata.Package, loadDir string, standalone bool, pkg *packages.Package) *metadata.Package {
// Allow for multiple ad-hoc packages in the workspace (see #47584).
pkgPath := PackagePath(pkg.PkgPath)
id := PackageID(pkg.ID)
Expand Down Expand Up @@ -524,7 +560,7 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag
continue
}

dep := buildMetadata(updates, imported, loadDir, false, goListView) // only top level packages can be standalone
dep := buildMetadata(updates, loadDir, false, imported) // only top level packages can be standalone

// Don't record edges to packages with no name, as they cause trouble for
// the importer (golang/go#60952).
Expand Down
7 changes: 1 addition & 6 deletions gopls/internal/cache/mod.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/protocol/command"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
)

Expand Down Expand Up @@ -252,11 +251,7 @@ func modWhyImpl(ctx context.Context, snapshot *Snapshot, fh file.Handle) (map[st
for _, req := range pm.File.Require {
args = append(args, req.Mod.Path)
}
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
Verb: "mod",
Args: args,
WorkingDir: filepath.Dir(fh.URI().Path()),
})
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, filepath.Dir(fh.URI().Path()), "mod", args)
if err != nil {
return nil, err
}
Expand Down
14 changes: 7 additions & 7 deletions gopls/internal/cache/mod_tidy.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"golang.org/x/tools/gopls/internal/protocol/command"
"golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
)

Expand Down Expand Up @@ -108,12 +107,13 @@ func modTidyImpl(ctx context.Context, snapshot *Snapshot, pm *ParsedModule) (*Ti
}
defer cleanup()

inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
Verb: "mod",
Args: []string{"tidy", "-modfile=" + filepath.Join(tempDir, "go.mod")},
Env: []string{"GOWORK=off"},
WorkingDir: pm.URI.Dir().Path(),
})
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(
false,
pm.URI.Dir().Path(),
"mod",
[]string{"tidy", "-modfile=" + filepath.Join(tempDir, "go.mod")},
"GOWORK=off",
)
if err != nil {
return nil, err
}
Expand Down
84 changes: 14 additions & 70 deletions gopls/internal/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"go/build/constraint"
"go/parser"
"go/token"
"go/types"
"os"
"path"
"path/filepath"
Expand All @@ -26,7 +25,6 @@ import (
"sync"

"golang.org/x/sync/errgroup"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/go/types/objectpath"
"golang.org/x/tools/gopls/internal/cache/metadata"
"golang.org/x/tools/gopls/internal/cache/methodsets"
Expand All @@ -49,8 +47,6 @@ import (
"golang.org/x/tools/internal/event/label"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
"golang.org/x/tools/internal/packagesinternal"
"golang.org/x/tools/internal/typesinternal"
)

// A Snapshot represents the current state for a given view.
Expand Down Expand Up @@ -363,50 +359,6 @@ func (s *Snapshot) Templates() map[protocol.DocumentURI]file.Handle {
return tmpls
}

// config returns the configuration used for the snapshot's interaction with
// the go/packages API. It uses the given working directory.
//
// TODO(rstambler): go/packages requires that we do not provide overlays for
// multiple modules in one config, so buildOverlay needs to filter overlays by
// module.
func (s *Snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packages.Config {

cfg := &packages.Config{
Context: ctx,
Dir: inv.WorkingDir,
Env: inv.Env,
BuildFlags: inv.BuildFlags,
Mode: packages.NeedName |
packages.NeedFiles |
packages.NeedCompiledGoFiles |
packages.NeedImports |
packages.NeedDeps |
packages.NeedTypesSizes |
packages.NeedModule |
packages.NeedEmbedFiles |
packages.LoadMode(packagesinternal.DepsErrors) |
packages.NeedForTest,
Fset: nil, // we do our own parsing
Overlay: s.buildOverlays(),
ParseFile: func(*token.FileSet, string, []byte) (*ast.File, error) {
panic("go/packages must not be used to parse files")
},
Logf: func(format string, args ...interface{}) {
if s.Options().VerboseOutput {
event.Log(ctx, fmt.Sprintf(format, args...))
}
},
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)
}
return cfg
}

// RunGoModUpdateCommands runs a series of `go` commands that updates the go.mod
// and go.sum file for wd, and returns their updated contents.
//
Expand All @@ -423,16 +375,14 @@ func (s *Snapshot) RunGoModUpdateCommands(ctx context.Context, modURI protocol.D
// TODO(rfindley): we must use ModFlag and ModFile here (rather than simply
// setting Args), because without knowing the verb, we can't know whether
// ModFlag is appropriate. Refactor so that args can be set by the caller.
inv, cleanupInvocation, err := s.GoCommandInvocation(true, &gocommand.Invocation{
WorkingDir: modURI.Dir().Path(),
ModFlag: "mod",
ModFile: filepath.Join(tempDir, "go.mod"),
Env: []string{"GOWORK=off"},
})
inv, cleanupInvocation, err := s.GoCommandInvocation(true, modURI.Dir().Path(), "", nil, "GOWORK=off")
if err != nil {
return nil, nil, err
}
defer cleanupInvocation()

inv.ModFlag = "mod"
inv.ModFile = filepath.Join(tempDir, "go.mod")
invoke := func(args ...string) (*bytes.Buffer, error) {
inv.Verb = args[0]
inv.Args = args[1:]
Expand Down Expand Up @@ -510,22 +460,16 @@ func TempModDir(ctx context.Context, fs file.Source, modURI protocol.DocumentURI
// BuildFlags should be more clearly expressed in the API.
//
// If allowNetwork is set, do not set GOPROXY=off.
func (s *Snapshot) GoCommandInvocation(allowNetwork bool, inv *gocommand.Invocation) (_ *gocommand.Invocation, cleanup func(), _ error) {
// TODO(rfindley): it's not clear that this is doing the right thing.
// Should inv.Env really overwrite view.options? Should s.view.envOverlay
// overwrite inv.Env? (Do we ever invoke this with a non-empty inv.Env?)
//
// We should survey existing uses and write down rules for how env is
// applied.
inv.Env = slices.Concat(
os.Environ(),
s.Options().EnvSlice(),
inv.Env,
[]string{"GO111MODULE=" + s.view.adjustedGO111MODULE()},
s.view.EnvOverlay(),
)
inv.BuildFlags = slices.Clone(s.Options().BuildFlags)

//
// TODO(rfindley): use a named boolean for allowNetwork.
func (s *Snapshot) GoCommandInvocation(allowNetwork bool, dir, verb string, args []string, env ...string) (_ *gocommand.Invocation, cleanup func(), _ error) {
inv := &gocommand.Invocation{
Verb: verb,
Args: args,
WorkingDir: dir,
Env: append(s.view.Env(), env...),
BuildFlags: slices.Clone(s.Options().BuildFlags),
}
if !allowNetwork {
inv.Env = append(inv.Env, "GOPROXY=off")
}
Expand Down
10 changes: 10 additions & 0 deletions gopls/internal/cache/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,16 @@ func (v *View) Folder() *Folder {
return v.folder
}

// Env returns the environment to use for running go commands in this view.
func (v *View) Env() []string {
return slices.Concat(
os.Environ(),
v.folder.Options.EnvSlice(),
[]string{"GO111MODULE=" + v.adjustedGO111MODULE()},
v.EnvOverlay(),
)
}

// UpdateFolders updates the set of views for the new folders.
//
// Calling this causes each view to be reinitialized.
Expand Down
8 changes: 1 addition & 7 deletions gopls/internal/golang/assembly.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@ import (
"context"
"fmt"
"html"
"path/filepath"
"regexp"
"strconv"
"strings"

"golang.org/x/tools/gopls/internal/cache"
"golang.org/x/tools/internal/gocommand"
)

// AssemblyHTML returns an HTML document containing an assembly listing of the selected function.
Expand All @@ -32,11 +30,7 @@ import (
// - cross-link jumps and block labels, like github.com/aclements/objbrowse.
func AssemblyHTML(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, symbol string, web Web) ([]byte, error) {
// Compile the package with -S, and capture its stderr stream.
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
Verb: "build",
Args: []string{"-gcflags=-S", "."},
WorkingDir: filepath.Dir(pkg.Metadata().CompiledGoFiles[0].Path()),
})
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, pkg.Metadata().CompiledGoFiles[0].Dir().Path(), "build", []string{"-gcflags=-S", "."})
if err != nil {
return nil, err // e.g. failed to write overlays (rare)
}
Expand Down
13 changes: 4 additions & 9 deletions gopls/internal/golang/gc_annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/settings"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
)

// GCOptimizationDetails invokes the Go compiler on the specified
Expand Down Expand Up @@ -57,14 +56,10 @@ func GCOptimizationDetails(ctx context.Context, snapshot *cache.Snapshot, mp *me
if !strings.HasPrefix(outDir, "/") {
outDirURI = protocol.DocumentURI(strings.Replace(string(outDirURI), "file:///", "file://", 1))
}
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
Verb: "build",
Args: []string{
fmt.Sprintf("-gcflags=-json=0,%s", outDirURI),
fmt.Sprintf("-o=%s", tmpFile.Name()),
".",
},
WorkingDir: pkgDir,
inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, pkgDir, "build", []string{
fmt.Sprintf("-gcflags=-json=0,%s", outDirURI),
fmt.Sprintf("-o=%s", tmpFile.Name()),
".",
})
if err != nil {
return nil, err
Expand Down
Loading

0 comments on commit 29f4edb

Please sign in to comment.