From b13fa046aa370b8ebff523482a63fa045b237daf Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 11 Sep 2019 13:13:44 -0400 Subject: [PATCH] internal/lsp: merge session and view options into one This fixes the issue of config options not being applied. Also, handle config errors and deprecation by showing a message to the user. Change-Id: I850d5303a7a1e301c97324060a87929710ee6700 Reviewed-on: https://go-review.googlesource.com/c/tools/+/194682 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/cache.go | 2 +- internal/lsp/cache/session.go | 8 +- internal/lsp/cache/view.go | 4 +- internal/lsp/cmd/cmd.go | 3 +- internal/lsp/completion.go | 2 +- internal/lsp/general.go | 32 +++++++- internal/lsp/lsp_test.go | 5 +- internal/lsp/source/options.go | 123 ++++++++++++++--------------- internal/lsp/source/source_test.go | 5 +- internal/lsp/source/view.go | 8 +- internal/lsp/workspace.go | 6 +- 11 files changed, 106 insertions(+), 92 deletions(-) diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go index 44dae995d8..b20712edab 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -76,7 +76,7 @@ func (c *cache) NewSession(ctx context.Context) source.Session { s := &session{ cache: c, id: strconv.FormatInt(index, 10), - options: source.DefaultSessionOptions, + options: source.DefaultOptions, overlays: make(map[span.URI]*overlay), filesWatchMap: NewWatchMap(), } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 605297693a..ac004c609d 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -28,7 +28,7 @@ type session struct { cache *cache id string - options source.SessionOptions + options source.Options viewMu sync.Mutex views []*view @@ -56,11 +56,11 @@ type overlay struct { unchanged bool } -func (s *session) Options() source.SessionOptions { +func (s *session) Options() source.Options { return s.options } -func (s *session) SetOptions(options source.SessionOptions) { +func (s *session) SetOptions(options source.Options) { s.options = options } @@ -79,7 +79,7 @@ func (s *session) Cache() source.Cache { return s.cache } -func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.ViewOptions) source.View { +func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) source.View { index := atomic.AddInt64(&viewIndex, 1) s.viewMu.Lock() defer s.viewMu.Unlock() diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 3a2f93f9e9..5168ebd53d 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -30,7 +30,7 @@ type view struct { session *session id string - options source.ViewOptions + options source.Options // mu protects all mutable state of the view. mu sync.Mutex @@ -113,7 +113,7 @@ func (v *view) Folder() span.URI { return v.folder } -func (v *view) Options() source.ViewOptions { +func (v *view) Options() source.Options { return v.options } diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index 3052840981..ce04ca17fc 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -298,8 +298,7 @@ func (c *cmdClient) Configuration(ctx context.Context, p *protocol.ParamConfig) env[l[0]] = l[1] } results[i] = map[string]interface{}{ - "env": env, - "noDocsOnHover": true, + "env": env, } } return results, nil diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 7b330633b2..df2659f505 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -51,7 +51,7 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara }, nil } -func (s *Server) toProtocolCompletionItems(candidates []source.CompletionItem, rng protocol.Range, options source.SessionOptions) []protocol.CompletionItem { +func (s *Server) toProtocolCompletionItems(candidates []source.CompletionItem, rng protocol.Range, options source.Options) []protocol.CompletionItem { var ( items = make([]protocol.CompletionItem, 0, len(candidates)) numDeepCompletionsSeen int diff --git a/internal/lsp/general.go b/internal/lsp/general.go index 69f973f40d..69f262a1b7 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -7,6 +7,7 @@ package lsp import ( "bytes" "context" + "fmt" "os" "path" @@ -33,7 +34,7 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitia) ( options := s.session.Options() defer func() { s.session.SetOptions(options) }() - //TODO: handle the options results + // TODO: Handle results here. source.SetOptions(&options, params.InitializationOptions) options.ForClientCapabilities(params.Capabilities) @@ -172,7 +173,7 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa return nil } -func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, vo *source.ViewOptions) error { +func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, o *source.Options) error { if !s.session.Options().ConfigurationSupported { return nil } @@ -193,8 +194,31 @@ func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, return err } for _, config := range configs { - //TODO: handle the options results - source.SetOptions(vo, config) + results := source.SetOptions(o, config) + for _, result := range results { + if result.Error != nil { + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: result.Error.Error(), + }) + } + switch result.State { + case source.OptionUnexpected: + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: fmt.Sprintf("unexpected config %s", result.Name), + }) + case source.OptionDeprecated: + msg := fmt.Sprintf("config %s is deprecated", result.Name) + if result.Replacement != "" { + msg = fmt.Sprintf("%s, use %s instead", msg, result.Replacement) + } + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Warning, + Message: msg, + }) + } + } } return nil } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 1b4b684d04..ce03f675ba 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -61,9 +61,8 @@ 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) + options.Env = data.Config.Env + session.NewView(ctx, viewName, span.FileURI(data.Config.Dir), options) for filename, content := range data.Config.Overlay { session.SetOverlay(span.FileURI(filename), content) } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 120e1703c0..2501a45618 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -14,7 +14,8 @@ import ( ) var ( - DefaultSessionOptions = SessionOptions{ + DefaultOptions = Options{ + Env: os.Environ(), TextDocumentSyncKind: protocol.Incremental, HoverKind: SynopsisDocumentation, InsertTextFormat: protocol.PlainTextTextFormat, @@ -32,15 +33,17 @@ var ( Deep: false, FuzzyMatching: false, }, - DefaultViewOptions: ViewOptions{ - Env: os.Environ(), - }, } ) -type SessionOptions struct { - Env []string - BuildFlags []string +type Options 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 + HoverKind HoverKind DisabledAnalyses map[string]struct{} @@ -58,16 +61,6 @@ 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 { @@ -81,10 +74,6 @@ type CompletionOptions struct { type HoverKind int -type Options interface { - set(name string, value interface{}) OptionResult -} - const ( SingleLine = HoverKind(iota) NoDocumentation @@ -104,8 +93,10 @@ type OptionResults []OptionResult type OptionResult struct { Name string Value interface{} - State OptionState Error error + + State OptionState + Replacement string } type OptionState int @@ -116,7 +107,7 @@ const ( OptionUnexpected ) -func SetOptions(options Options, opts interface{}) OptionResults { +func SetOptions(options *Options, opts interface{}) OptionResults { var results OptionResults switch opts := opts.(type) { case nil: @@ -133,7 +124,7 @@ func SetOptions(options Options, opts interface{}) OptionResults { return results } -func (o *SessionOptions) ForClientCapabilities(caps protocol.ClientCapabilities) { +func (o *Options) ForClientCapabilities(caps protocol.ClientCapabilities) { // Check if the client supports snippets in completion items. if caps.TextDocument.Completion.CompletionItem != nil && caps.TextDocument.Completion.CompletionItem.SnippetSupport { @@ -152,24 +143,46 @@ func (o *SessionOptions) ForClientCapabilities(caps protocol.ClientCapabilities) o.LineFoldingOnly = caps.TextDocument.FoldingRange.LineFoldingOnly } -func (o *SessionOptions) set(name string, value interface{}) OptionResult { +func (o *Options) set(name string, value interface{}) OptionResult { result := OptionResult{Name: name, Value: value} switch name { + case "env": + menv, ok := value.(map[string]interface{}) + if !ok { + result.errorf("invalid config gopls.env type %T", value) + break + } + for k, v := range menv { + o.Env = append(o.Env, fmt.Sprintf("%s=%s", k, v)) + } + + case "buildFlags": + iflags, ok := value.([]interface{}) + if !ok { + result.errorf("invalid config gopls.buildFlags type %T", value) + break + } + flags := make([]string, 0, len(iflags)) + for _, flag := range iflags { + flags = append(flags, fmt.Sprintf("%s", flag)) + } + o.BuildFlags = flags + case "noIncrementalSync": if v, ok := result.asBool(); ok && v { o.TextDocumentSyncKind = protocol.Full } case "watchFileChanges": result.setBool(&o.WatchFileChanges) - case "wantCompletionDocumentation": + case "completionDocumentation": result.setBool(&o.Completion.Documentation) case "usePlaceholders": result.setBool(&o.Completion.Placeholders) - case "disableDeepCompletion": - result.setNotBool(&o.Completion.Deep) - case "disableFuzzyMatching": - result.setNotBool(&o.Completion.FuzzyMatching) - case "wantUnimportedCompletions": + case "deepCompletion": + result.setBool(&o.Completion.Deep) + case "fuzzyMatching": + result.setBool(&o.Completion.FuzzyMatching) + case "completeUnimported": result.setBool(&o.Completion.Unimported) case "hoverKind": @@ -204,39 +217,25 @@ func (o *SessionOptions) set(name string, value interface{}) OptionResult { o.DisabledAnalyses[fmt.Sprint(a)] = struct{}{} } + // Deprecated settings. case "wantSuggestedFixes": result.State = OptionDeprecated - default: - return o.DefaultViewOptions.set(name, value) - } - return result -} + case "disableDeepCompletion": + result.State = OptionDeprecated + result.Replacement = "deepCompletion" -func (o *ViewOptions) set(name string, value interface{}) OptionResult { - result := OptionResult{Name: name, Value: value} - switch name { - case "env": - menv, ok := value.(map[string]interface{}) - if !ok { - result.errorf("invalid config gopls.env type %T", value) - break - } - for k, v := range menv { - o.Env = append(o.Env, fmt.Sprintf("%s=%s", k, v)) - } + case "disableFuzzyMatching": + result.State = OptionDeprecated + result.Replacement = "fuzzyMatching" - case "buildFlags": - iflags, ok := value.([]interface{}) - if !ok { - result.errorf("invalid config gopls.buildFlags type %T", value) - break - } - flags := make([]string, 0, len(iflags)) - for _, flag := range iflags { - flags = append(flags, fmt.Sprintf("%s", flag)) - } - o.BuildFlags = flags + case "wantCompletionDocumentation": + result.State = OptionDeprecated + result.Replacement = "completionDocumentation" + + case "wantUnimportedCompletions": + result.State = OptionDeprecated + result.Replacement = "completeUnimported" default: result.State = OptionUnexpected @@ -262,9 +261,3 @@ func (r *OptionResult) setBool(b *bool) { *b = v } } - -func (r *OptionResult) setNotBool(b *bool) { - if v, ok := r.asBool(); ok { - *b = !v - } -} diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 9734b79c38..36de19759d 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -49,10 +49,9 @@ 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 + options.Env = data.Config.Env r := &runner{ - view: session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), vo), + view: session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), options), data: data, ctx: ctx, } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 0480cc289b..a9c92b32d8 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -155,7 +155,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, options ViewOptions) View + NewView(ctx context.Context, name string, folder span.URI, options Options) View // Cache returns the cache that created this session. Cache() Cache @@ -196,10 +196,10 @@ type Session interface { DidChangeOutOfBand(ctx context.Context, f GoFile, change protocol.FileChangeType) // Options returns a copy of the SessionOptions for this session. - Options() SessionOptions + Options() Options // SetOptions sets the options of this session to new values. - SetOptions(SessionOptions) + SetOptions(Options) } // View represents a single workspace. @@ -246,7 +246,7 @@ type View interface { RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error, opts *imports.Options) error // Options returns a copy of the ViewOptions for this view. - Options() ViewOptions + Options() Options } // File represents a source file of any type. diff --git a/internal/lsp/workspace.go b/internal/lsp/workspace.go index 27bdc16340..42a252a9fa 100644 --- a/internal/lsp/workspace.go +++ b/internal/lsp/workspace.go @@ -38,8 +38,8 @@ func (s *Server) addView(ctx context.Context, name string, uri span.URI) error { return errors.Errorf("addView called before server initialized") } - viewOptions := s.session.Options().DefaultViewOptions - s.fetchConfig(ctx, name, uri, &viewOptions) - s.session.NewView(ctx, name, uri, viewOptions) + options := s.session.Options() + s.fetchConfig(ctx, name, uri, &options) + s.session.NewView(ctx, name, uri, options) return nil }