From 3d22a3cfff5f51e8f1f3dd41df13b43fef4e79ad Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Mon, 9 Sep 2019 13:04:12 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/session.go | 7 ++--- internal/lsp/cache/view.go | 30 ++----------------- internal/lsp/general.go | 48 ++++++++++++++---------------- internal/lsp/lsp_test.go | 11 +++---- internal/lsp/server.go | 4 +++ internal/lsp/source/options.go | 17 +++++++++-- internal/lsp/source/source_test.go | 6 ++-- internal/lsp/source/view.go | 11 +------ internal/lsp/workspace.go | 14 +++++---- 9 files changed, 66 insertions(+), 82 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 517538d7ad..5a469e0860 100644 --- a/internal/lsp/cache/session.go +++ b/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), diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index f43bcb16b8..767bdbd268 100644 --- a/internal/lsp/cache/view.go +++ b/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) } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index f48507090f..1e7ac99fa9 100644 --- a/internal/lsp/general.go +++ b/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. diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index b756ebf745..4ca0b7dfc6 100644 --- a/internal/lsp/lsp_test.go +++ b/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{ diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 5622522c3e..423f734c22 100644 --- a/internal/lsp/server.go +++ b/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 diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 81596f8284..a2b58d4e83 100644 --- a/internal/lsp/source/options.go +++ b/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 { diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index f0ec5bdda6..1b88f88648 100644 --- a/internal/lsp/source/source_test.go +++ b/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) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 252c4c9284..e3d8d03c4a 100644 --- a/internal/lsp/source/view.go +++ b/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) diff --git a/internal/lsp/workspace.go b/internal/lsp/workspace.go index e357fe35f4..bfbf0652dc 100644 --- a/internal/lsp/workspace.go +++ b/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 }