internal/lsp: only build a view when we have its configuration

We now wait to build views until we have the options for that view,
and pass the options in to the view constructor.
The environment and build flags are now part of the view options.

Change-Id: I303c8ba1eefd01b66962ba9cadb4847d3d2e1d3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194278
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/session.go

@@ -6,7 +6,6 @@ package cache
 
 import (
 	"context"
-	"os"
 	"path/filepath"
 	"sort"
 	"strconv"
@@ -79,22 +78,22 @@ func (s *session) Cache() source.Cache {
 	return s.cache
 }
 
-func (s *session) NewView(ctx context.Context, name string, folder span.URI) source.View {
+func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.ViewOptions) source.View {
 	index := atomic.AddInt64(&viewIndex, 1)
 	s.viewMu.Lock()
 	defer s.viewMu.Unlock()
-	// We want a true background context and not a detatched context here
+	// We want a true background context and not a detached context here
 	// the spans need to be unrelated and no tag values should pollute it.
 	baseCtx := trace.Detach(xcontext.Detach(ctx))
 	backgroundCtx, cancel := context.WithCancel(baseCtx)
 	v := &view{
 		session:       s,
 		id:            strconv.FormatInt(index, 10),
+		options:       options,
 		baseCtx:       baseCtx,
 		backgroundCtx: backgroundCtx,
 		cancel:        cancel,
 		name:          name,
-		env:           os.Environ(),
 		folder:        folder,
 		filesByURI:    make(map[span.URI]viewFile),
 		filesByBase:   make(map[string][]viewFile),

internal/lsp/cache/view.go

@@ -53,9 +53,6 @@ type view struct {
 	// Folder is the root of this view.
 	folder span.URI
 
-	// env is the environment to use when invoking underlying tools.
-	env []string
-
 	// process is the process env for this view.
 	// Note: this contains cached module and filesystem state.
 	//
@@ -69,9 +66,6 @@ type view struct {
 	// TODO(suzmue): These versions may not actually be on disk.
 	modFileVersions map[string]string
 
-	// buildFlags is the build flags to use when invoking underlying tools.
-	buildFlags []string
-
 	// keep track of files by uri and by basename, a single file may be mapped
 	// to multiple uris, and the same basename may map to multiple files
 	filesByURI  map[span.URI]viewFile
@@ -130,8 +124,8 @@ func (v *view) Config(ctx context.Context) *packages.Config {
 	// TODO: Should we cache the config and/or overlay somewhere?
 	return &packages.Config{
 		Dir:        v.folder.Filename(),
-		Env:        v.env,
-		BuildFlags: v.buildFlags,
+		Env:        v.options.Env,
+		BuildFlags: v.options.BuildFlags,
 		Mode: packages.NeedName |
 			packages.NeedFiles |
 			packages.NeedCompiledGoFiles |
@@ -262,26 +256,6 @@ func (v *view) fileVersion(filename string) string {
 	return f.Identity().Version
 }
 
-func (v *view) Env() []string {
-	v.mu.Lock()
-	defer v.mu.Unlock()
-	return v.env
-}
-
-func (v *view) SetEnv(env []string) {
-	v.mu.Lock()
-	defer v.mu.Unlock()
-	//TODO: this should invalidate the entire view
-	v.env = env
-	v.processEnv = nil // recompute process env
-}
-
-func (v *view) SetBuildFlags(buildFlags []string) {
-	v.mu.Lock()
-	defer v.mu.Unlock()
-	v.buildFlags = buildFlags
-}
-
 func (v *view) Shutdown(ctx context.Context) {
 	v.session.removeView(ctx, v)
 }

internal/lsp/general.go

@@ -60,10 +60,10 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitia) (
 
 	s.setClientCapabilities(&options, params.Capabilities)
 
-	folders := params.WorkspaceFolders
-	if len(folders) == 0 {
+	s.pendingFolders = params.WorkspaceFolders
+	if len(s.pendingFolders) == 0 {
 		if params.RootURI != "" {
-			folders = []protocol.WorkspaceFolder{{
+			s.pendingFolders = []protocol.WorkspaceFolder{{
 				URI:  params.RootURI,
 				Name: path.Base(params.RootURI),
 			}}
@@ -75,12 +75,6 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitia) (
 		}
 	}
 
-	for _, folder := range folders {
-		if err := s.addView(ctx, folder.Name, span.NewURI(folder.URI)); err != nil {
-			return nil, err
-		}
-	}
-
 	var codeActionProvider interface{}
 	if params.Capabilities.TextDocument.CodeAction.CodeActionLiteralSupport != nil &&
 		len(params.Capabilities.TextDocument.CodeAction.CodeActionLiteralSupport.CodeActionKind.ValueSet) > 0 {
@@ -208,28 +202,32 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa
 		})
 	}
 
-	if options.ConfigurationSupported {
-		for _, view := range s.session.Views() {
-			if err := s.fetchConfig(ctx, view, &options); err != nil {
-				return err
-			}
-		}
-	}
 	buf := &bytes.Buffer{}
 	debug.PrintVersionInfo(buf, true, debug.PlainText)
 	log.Print(ctx, buf.String())
+
+	for _, folder := range s.pendingFolders {
+		if err := s.addView(ctx, folder.Name, span.NewURI(folder.URI)); err != nil {
+			return err
+		}
+	}
+	s.pendingFolders = nil
+
 	return nil
 }
 
-func (s *Server) fetchConfig(ctx context.Context, view source.View, options *source.SessionOptions) error {
+func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, options *source.SessionOptions, vo *source.ViewOptions) error {
+	if !options.ConfigurationSupported {
+		return nil
+	}
 	v := protocol.ParamConfig{
 		protocol.ConfigurationParams{
 			Items: []protocol.ConfigurationItem{{
-				ScopeURI: protocol.NewURI(view.Folder()),
+				ScopeURI: protocol.NewURI(folder),
 				Section:  "gopls",
 			}, {
-				ScopeURI: protocol.NewURI(view.Folder()),
-				Section:  view.Name(),
+				ScopeURI: protocol.NewURI(folder),
+				Section:  name,
 			},
 			},
 		}, protocol.PartialResultParams{},
@@ -239,14 +237,14 @@ func (s *Server) fetchConfig(ctx context.Context, view source.View, options *sou
 		return err
 	}
 	for _, config := range configs {
-		if err := s.processConfig(ctx, view, options, config); err != nil {
+		if err := s.processConfig(ctx, options, vo, config); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func (s *Server) processConfig(ctx context.Context, view source.View, options *source.SessionOptions, config interface{}) error {
+func (s *Server) processConfig(ctx context.Context, options *source.SessionOptions, vo *source.ViewOptions, config interface{}) error {
 	// TODO: We should probably store and process more of the config.
 	if config == nil {
 		return nil // ignore error if you don't have a config
@@ -263,11 +261,9 @@ func (s *Server) processConfig(ctx context.Context, view source.View, options *s
 		if !ok {
 			return errors.Errorf("invalid config gopls.env type %T", env)
 		}
-		env := view.Env()
 		for k, v := range menv {
-			env = append(env, fmt.Sprintf("%s=%s", k, v))
+			vo.Env = append(vo.Env, fmt.Sprintf("%s=%s", k, v))
 		}
-		view.SetEnv(env)
 	}
 
 	// Get the build flags for the go/packages config.
@@ -280,7 +276,7 @@ func (s *Server) processConfig(ctx context.Context, view source.View, options *s
 		for _, flag := range iflags {
 			flags = append(flags, fmt.Sprintf("%s", flag))
 		}
-		view.SetBuildFlags(flags)
+		vo.BuildFlags = flags
 	}
 
 	// Set the hover kind.

internal/lsp/lsp_test.go

@@ -50,11 +50,6 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 
 	cache := cache.New()
 	session := cache.NewSession(ctx)
-	view := session.NewView(ctx, viewName, span.FileURI(data.Config.Dir))
-	view.SetEnv(data.Config.Env)
-	for filename, content := range data.Config.Overlay {
-		session.SetOverlay(span.FileURI(filename), content)
-	}
 	options := session.Options()
 	options.SupportedCodeActions = map[source.FileKind]map[protocol.CodeActionKind]bool{
 		source.Go: {
@@ -66,6 +61,12 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	}
 	options.HoverKind = source.SynopsisDocumentation
 	session.SetOptions(options)
+	vo := options.DefaultViewOptions
+	vo.Env = data.Config.Env
+	session.NewView(ctx, viewName, span.FileURI(data.Config.Dir), vo)
+	for filename, content := range data.Config.Overlay {
+		session.SetOverlay(span.FileURI(filename), content)
+	}
 
 	r := &runner{
 		server: &Server{

internal/lsp/server.go

@@ -82,6 +82,10 @@ type Server struct {
 	// failed to deliver for some reason.
 	undeliveredMu sync.Mutex
 	undelivered   map[span.URI][]source.Diagnostic
+
+	// folders is only valid between initialize and initialized, and holds the
+	// set of folders to build views for when we are ready
+	pendingFolders []protocol.WorkspaceFolder
 }
 
 // General

internal/lsp/source/options.go

@@ -4,7 +4,11 @@
 
 package source
 
-import "golang.org/x/tools/internal/lsp/protocol"
+import (
+	"os"
+
+	"golang.org/x/tools/internal/lsp/protocol"
+)
 
 var (
 	DefaultSessionOptions = SessionOptions{
@@ -24,8 +28,10 @@ var (
 			Deep:          true,
 			FuzzyMatching: true,
 		},
+		DefaultViewOptions: ViewOptions{
+			Env: os.Environ(),
+		},
 	}
-	DefaultViewOptions = ViewOptions{}
 )
 
 type SessionOptions struct {
@@ -47,9 +53,16 @@ type SessionOptions struct {
 	TextDocumentSyncKind protocol.TextDocumentSyncKind
 
 	Completion CompletionOptions
+
+	DefaultViewOptions ViewOptions
 }
 
 type ViewOptions struct {
+	// Env is the current set of environment overrides on this view.
+	Env []string
+
+	// BuildFlags is used to adjust the build flags applied to the view.
+	BuildFlags []string
 }
 
 type CompletionOptions struct {

internal/lsp/source/source_test.go

@@ -48,12 +48,14 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
 
 	cache := cache.New()
 	session := cache.NewSession(ctx)
+	options := session.Options()
+	vo := options.DefaultViewOptions
+	vo.Env = data.Config.Env
 	r := &runner{
-		view: session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir)),
+		view: session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), vo),
 		data: data,
 		ctx:  ctx,
 	}
-	r.view.SetEnv(data.Config.Env)
 	for filename, content := range data.Config.Overlay {
 		session.SetOverlay(span.FileURI(filename), content)
 	}

internal/lsp/source/view.go

@@ -151,7 +151,7 @@ type Cache interface {
 // A session may have many active views at any given time.
 type Session interface {
 	// NewView creates a new View and returns it.
-	NewView(ctx context.Context, name string, folder span.URI) View
+	NewView(ctx context.Context, name string, folder span.URI, options ViewOptions) View
 
 	// Cache returns the cache that created this session.
 	Cache() Cache
@@ -229,15 +229,6 @@ type View interface {
 	// on behalf of this view.
 	BackgroundContext() context.Context
 
-	// Env returns the current set of environment overrides on this view.
-	Env() []string
-
-	// SetEnv is used to adjust the environment applied to the view.
-	SetEnv([]string)
-
-	// SetBuildFlags is used to adjust the build flags applied to the view.
-	SetBuildFlags([]string)
-
 	// Shutdown closes this view, and detaches it from it's session.
 	Shutdown(ctx context.Context)
 

internal/lsp/workspace.go

@@ -31,14 +31,18 @@ func (s *Server) changeFolders(ctx context.Context, event protocol.WorkspaceFold
 }
 
 func (s *Server) addView(ctx context.Context, name string, uri span.URI) error {
-	view := s.session.NewView(ctx, name, uri)
 	s.stateMu.Lock()
 	state := s.state
 	s.stateMu.Unlock()
-	options := s.session.Options()
-	defer func() { s.session.SetOptions(options) }()
-	if state >= serverInitialized {
-		s.fetchConfig(ctx, view, &options)
+	if state < serverInitialized {
+		return errors.Errorf("addView called before server initialized")
 	}
+
+	options := s.session.Options()
+	viewOptions := options.DefaultViewOptions
+	//TODO: take this out, we only allow new session options here
+	defer func() { s.session.SetOptions(options) }()
+	s.fetchConfig(ctx, name, uri, &options, &viewOptions)
+	s.session.NewView(ctx, name, uri, viewOptions)
 	return nil
 }