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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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

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()
 

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
-}

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 {

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 mod := v.session.cache.GetFile(v.realMod); mod.Identity() != v.cachedModFileVersion {
-		if err == nil && 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, buildFlags := v.env()
-	env := &imports.ProcessEnv{
+	processEnv := &imports.ProcessEnv{
-		WorkingDir: cfg.Dir,
+		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 len(buildFlags) > 0 {
-		if env.GOFLAGS != "" {
+		if processEnv.GOFLAGS != "" {
-			env.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 {

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
 		}
 	}

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

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.
+	// RunProcessEnvFunc runs fn with the process env for this snapshot's view.
-	Config(ctx context.Context) *packages.Config
-
-	// RunProcessEnvFunc runs fn with the process env for this view.
 	// Note: the process env contains cached module and filesystem state.
 	RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error) error