From b753a1ba74fa2c944df1826a2f467fb9e51f801e Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 6 Feb 2020 17:49:19 -0500 Subject: [PATCH] internal/lsp: build overlays through the snapshot This is the first in a series of changes to move overlay handling to the snapshot instead of the session. We may not be able to fully get away from managing overlays on the session, but we should be able to only use overlays when they are known to the snapshot. Change-Id: I88b125117cd2cfbd0ff9ef16a944a34297c81b10 Reviewed-on: https://go-review.googlesource.com/c/tools/+/218324 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/load.go | 2 +- internal/lsp/cache/mod_tidy.go | 2 +- internal/lsp/cache/overlay.go | 15 ------ internal/lsp/cache/snapshot.go | 50 ++++++++++++++++++++ internal/lsp/cache/view.go | 84 ++++++++++++---------------------- internal/lsp/command.go | 5 +- internal/lsp/lsp_test.go | 2 +- internal/lsp/source/view.go | 8 ++-- 8 files changed, 88 insertions(+), 80 deletions(-) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index fc4e68e762..af08519176 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -80,7 +80,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.Query.Of(query)) defer done() - cfg := s.view.Config(ctx) + cfg := s.Config(ctx) pkgs, err := s.view.loadPackages(cfg, query...) // If the context was canceled, return early. Otherwise, we might be diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index d6bb7c9d68..ed86290d58 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -93,7 +93,7 @@ func (mth *modTidyHandle) Tidy(ctx context.Context) (*modfile.File, *protocol.Co func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) (source.ModTidyHandle, error) { realURI, tempURI := s.view.ModFiles() - cfg := s.View().Config(ctx) + cfg := s.Config(ctx) options := s.View().Options() folder := s.View().Folder().Filename() diff --git a/internal/lsp/cache/overlay.go b/internal/lsp/cache/overlay.go index dfe868720c..6d3dbd146c 100644 --- a/internal/lsp/cache/overlay.go +++ b/internal/lsp/cache/overlay.go @@ -116,18 +116,3 @@ func (s *session) readOverlay(uri span.URI) *overlay { } return nil } - -func (s *session) buildOverlay() map[string][]byte { - s.overlayMu.Lock() - defer s.overlayMu.Unlock() - - overlays := make(map[string][]byte) - for uri, overlay := range s.overlays { - // TODO(rstambler): Make sure not to send overlays outside of the current view. - if overlay.saved { - continue - } - overlays[uri.Filename()] = overlay.text - } - return overlays -} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 61d7b403a5..7488a98bb1 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -7,14 +7,18 @@ package cache import ( "context" "fmt" + "go/ast" + "go/token" "os" "path/filepath" "sync" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/telemetry" "golang.org/x/tools/internal/span" + "golang.org/x/tools/internal/telemetry/log" errors "golang.org/x/xerrors" ) @@ -73,6 +77,52 @@ func (s *snapshot) View() source.View { return s.view } +// Config returns the configuration used for the snapshot's interaction with the +// go/packages API. +func (s *snapshot) Config(ctx context.Context) *packages.Config { + env, buildFlags := s.view.env() + cfg := &packages.Config{ + Env: env, + Dir: s.view.folder.Filename(), + Context: ctx, + BuildFlags: buildFlags, + Mode: packages.NeedName | + packages.NeedFiles | + packages.NeedCompiledGoFiles | + packages.NeedImports | + packages.NeedDeps | + packages.NeedTypesSizes, + Fset: s.view.session.cache.fset, + Overlay: s.buildOverlay(), + 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.view.options.VerboseOutput { + log.Print(ctx, fmt.Sprintf(format, args...)) + } + }, + Tests: true, + } + return cfg +} + +func (s *snapshot) buildOverlay() map[string][]byte { + overlays := make(map[string][]byte) + for uri, fh := range s.files { + overlay, ok := fh.(*overlay) + if !ok { + continue + } + if overlay.saved { + continue + } + // TODO(rstambler): Make sure not to send overlays outside of the current view. + overlays[uri.Filename()] = overlay.text + } + return overlays +} + func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]source.PackageHandle, error) { // If the file is a go.mod file, go.Packages.Load will always return 0 packages. if fh.Identity().Kind == source.Mod { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index d923cbd2a0..fc9066bb4c 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -10,7 +10,6 @@ import ( "encoding/json" "fmt" "go/ast" - "go/token" "io" "io/ioutil" "os" @@ -262,44 +261,6 @@ func (v *view) buildBuiltinPackage(ctx context.Context, goFiles []string) error return nil } -// Config returns the configuration used for the view's interaction with the -// go/packages API. It is shared across all views. -func (v *view) Config(ctx context.Context) *packages.Config { - // TODO: Should we cache the config and/or overlay somewhere? - - // We want to run the go commands with the -modfile flag if the version of go - // that we are using supports it. - buildFlags := v.options.BuildFlags - if v.tempMod != "" { - buildFlags = append(buildFlags, fmt.Sprintf("-modfile=%s", v.tempMod.Filename())) - } - cfg := &packages.Config{ - Dir: v.folder.Filename(), - Context: ctx, - BuildFlags: buildFlags, - Mode: packages.NeedName | - packages.NeedFiles | - packages.NeedCompiledGoFiles | - packages.NeedImports | - packages.NeedDeps | - packages.NeedTypesSizes, - Fset: v.session.cache.fset, - Overlay: v.session.buildOverlay(), - 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 v.options.VerboseOutput { - log.Print(ctx, fmt.Sprintf(format, args...)) - } - }, - Tests: true, - } - cfg.Env = append(cfg.Env, fmt.Sprintf("GOPATH=%s", v.gopath)) - cfg.Env = append(cfg.Env, v.options.Env...) - return cfg -} - func (v *view) RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error) error { v.importsMu.Lock() defer v.importsMu.Unlock() @@ -313,8 +274,7 @@ func (v *view) RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) // In module mode, check if the mod file has changed. if v.realMod != "" { - mod, err := v.Snapshot().GetFile(v.realMod) - if err == nil && mod.Identity() != v.cachedModFileVersion { + if mod := v.session.cache.GetFile(v.realMod); mod.Identity() != v.cachedModFileVersion { v.processEnv.GetResolver().(*imports.ModuleResolver).ClearForNewMod() v.cachedModFileVersion = mod.Identity() } @@ -369,42 +329,54 @@ func (v *view) refreshProcessEnv() { } func (v *view) buildProcessEnv(ctx context.Context) (*imports.ProcessEnv, error) { - cfg := v.Config(ctx) - env := &imports.ProcessEnv{ - WorkingDir: cfg.Dir, + env, buildFlags := v.env() + processEnv := &imports.ProcessEnv{ + WorkingDir: v.folder.Filename(), Logf: func(format string, args ...interface{}) { log.Print(ctx, fmt.Sprintf(format, args...)) }, LocalPrefix: v.options.LocalPrefix, Debug: v.options.VerboseOutput, } - for _, kv := range cfg.Env { + for _, kv := range env { split := strings.Split(kv, "=") if len(split) < 2 { continue } switch split[0] { case "GOPATH": - env.GOPATH = split[1] + processEnv.GOPATH = split[1] case "GOROOT": - env.GOROOT = split[1] + processEnv.GOROOT = split[1] case "GO111MODULE": - env.GO111MODULE = split[1] + processEnv.GO111MODULE = split[1] case "GOPROXY": - env.GOPROXY = split[1] + processEnv.GOPROXY = split[1] case "GOFLAGS": - env.GOFLAGS = split[1] + processEnv.GOFLAGS = split[1] case "GOSUMDB": - env.GOSUMDB = split[1] + processEnv.GOSUMDB = split[1] } } - if len(cfg.BuildFlags) > 0 { - if env.GOFLAGS != "" { - env.GOFLAGS += " " + if len(buildFlags) > 0 { + if processEnv.GOFLAGS != "" { + processEnv.GOFLAGS += " " } - env.GOFLAGS += strings.Join(cfg.BuildFlags, " ") + processEnv.GOFLAGS += strings.Join(buildFlags, " ") } - return env, nil + return processEnv, nil +} + +func (v *view) env() ([]string, []string) { + // We want to run the go commands with the -modfile flag if the version of go + // that we are using supports it. + buildFlags := v.options.BuildFlags + if v.tempMod != "" { + buildFlags = append(buildFlags, fmt.Sprintf("-modfile=%s", v.tempMod.Filename())) + } + env := []string{fmt.Sprintf("GOPATH=%s", v.gopath)} + env = append(env, v.options.Env...) + return env, buildFlags } func (v *view) contains(uri span.URI) bool { diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 6e94a58a74..0f2399dbe9 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -21,7 +21,8 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom if err != nil { return nil, err } - fh, err := view.Snapshot().GetFile(uri) + snapshot := view.Snapshot() + fh, err := snapshot.GetFile(uri) if err != nil { return nil, err } @@ -31,7 +32,7 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom // Run go.mod tidy on the view. // TODO: This should go through the ModTidyHandle on the view. // That will also allow us to move source.InvokeGo into internal/lsp/cache. - if _, err := source.InvokeGo(ctx, view.Folder().Filename(), view.Config(ctx).Env, "mod", "tidy"); err != nil { + if _, err := source.InvokeGo(ctx, view.Folder().Filename(), snapshot.Config(ctx).Env, "mod", "tidy"); err != nil { return nil, err } } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 1f4f528ae1..debdae08ba 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -61,7 +61,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { // Check to see if the -modfile flag is available, this is basically a check // to see if the go version >= 1.14. Otherwise, the modfile specific tests // will always fail if this flag is not available. - for _, flag := range v.Config(ctx).BuildFlags { + for _, flag := range v.Snapshot().Config(ctx).BuildFlags { if strings.Contains(flag, "-modfile=") { datum.ModfileFlagAvailable = true break diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 551ff0d96e..3f0e30d02e 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -26,6 +26,9 @@ type Snapshot interface { // View returns the View associated with this snapshot. View() View + // Config returns the configuration for the view. + Config(ctx context.Context) *packages.Config + // GetFile returns the file object for a given URI, initializing it // if it is not already part of the view. GetFile(uri span.URI) (FileHandle, error) @@ -109,10 +112,7 @@ type View interface { // Ignore returns true if this file should be ignored by this view. Ignore(span.URI) bool - // Config returns the configuration for the view. - Config(ctx context.Context) *packages.Config - - // RunProcessEnvFunc runs fn with the process env for this view. + // RunProcessEnvFunc runs fn with the process env for this snapshot's view. // Note: the process env contains cached module and filesystem state. RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error) error