internal/lsp: add a "usePlaceholders" setting to gopls configuration

This change allows us to determine if we should complete with
placeholders in the function signature or not.

We currently complete all functions with parameters with a set of
parentheses with the cursor between them. Now, we support the option to
tab through the parameters.

Change-Id: I01d9d7ffdc76eb2f8e009ab9c8aa46d71c24c358
Reviewed-on: https://go-review.googlesource.com/c/tools/+/170877
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/completion.go

@@ -6,15 +6,42 @@ package lsp
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"sort"
 	"strings"
 
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/span"
 )
 
-func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string, pos protocol.Position, snippetsSupported, signatureHelpEnabled bool) []protocol.CompletionItem {
+func (s *Server) completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) {
+	uri := span.NewURI(params.TextDocument.URI)
+	view := s.findView(ctx, uri)
+	f, m, err := newColumnMap(ctx, view, uri)
+	if err != nil {
+		return nil, err
+	}
+	spn, err := m.PointSpan(params.Position)
+	if err != nil {
+		return nil, err
+	}
+	rng, err := spn.Range(m.Converter)
+	if err != nil {
+		return nil, err
+	}
+	items, prefix, err := source.Completion(ctx, f, rng.Start)
+	if err != nil {
+		return nil, err
+	}
+	return &protocol.CompletionList{
+		IsIncomplete: false,
+		Items:        toProtocolCompletionItems(items, prefix, params.Position, s.snippetsSupported, s.usePlaceholders),
+	}, nil
+}
+
+func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string, pos protocol.Position, snippetsSupported, usePlaceholders bool) []protocol.CompletionItem {
 	insertTextFormat := protocol.PlainTextTextFormat
 	if snippetsSupported {
 		insertTextFormat = protocol.SnippetTextFormat
@@ -24,14 +51,11 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 	})
 	items := []protocol.CompletionItem{}
 	for i, candidate := range candidates {
-		// Matching against the label.
+		// Match against the label.
 		if !strings.HasPrefix(candidate.Label, prefix) {
 			continue
 		}
-		// InsertText is deprecated in favor of TextEdits.
+		insertText := labelToInsertText(candidate.Label, candidate.Kind, insertTextFormat, usePlaceholders)
-		// TODO(rstambler): Remove this logic when we are confident that we no
-		// longer need to support it.
-		insertText, triggerSignatureHelp := labelToProtocolSnippets(candidate.Label, candidate.Kind, insertTextFormat, signatureHelpEnabled)
 		if strings.HasPrefix(insertText, prefix) {
 			insertText = insertText[len(prefix):]
 		}
@@ -54,12 +78,6 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 			FilterText: insertText,
 			Preselect:  i == 0,
 		}
-		// If we are completing a function, we should trigger signature help if possible.
-		if triggerSignatureHelp && signatureHelpEnabled {
-			item.Command = &protocol.Command{
-				Command: "editor.action.triggerParameterHints",
-			}
-		}
 		items = append(items, item)
 	}
 	return items
@@ -90,13 +108,13 @@ func toProtocolCompletionItemKind(kind source.CompletionItemKind) protocol.Compl
 	}
 }
 
-func labelToProtocolSnippets(label string, kind source.CompletionItemKind, insertTextFormat protocol.InsertTextFormat, signatureHelpEnabled bool) (string, bool) {
+func labelToInsertText(label string, kind source.CompletionItemKind, insertTextFormat protocol.InsertTextFormat, usePlaceholders bool) string {
 	switch kind {
 	case source.ConstantCompletionItem:
 		// The label for constants is of the format "<identifier> = <value>".
 		// We should not insert the " = <value>" part of the label.
 		if i := strings.Index(label, " ="); i >= 0 {
-			return label[:i], false
+			return label[:i]
 		}
 	case source.FunctionCompletionItem, source.MethodCompletionItem:
 		var trimmed, params string
@@ -105,16 +123,16 @@ func labelToProtocolSnippets(label string, kind source.CompletionItemKind, inser
 			params = strings.Trim(label[i:], "()")
 		}
 		if params == "" || trimmed == "" {
-			return label, true
+			return label
 		}
 		// Don't add parameters or parens for the plaintext insert format.
 		if insertTextFormat == protocol.PlainTextTextFormat {
-			return trimmed, true
+			return trimmed
 		}
-		// If we do have signature help enabled, the user can see parameters as
+		// If we don't want to use placeholders, just add 2 parentheses with
-		// they type in the function, so we just return empty parentheses.
+		// the cursor in the middle.
-		if signatureHelpEnabled {
+		if !usePlaceholders {
-			return trimmed + "($1)", true
+			return trimmed + "($1)"
 		}
 		// If signature help is not enabled, we should give the user parameters
 		// that they can tab through. The insert text format follows the
@@ -131,11 +149,12 @@ func labelToProtocolSnippets(label string, kind source.CompletionItemKind, inser
 			if i != 0 {
 				b.WriteString(", ")
 			}
-			fmt.Fprintf(b, "${%v:%v}", i+1, r.Replace(strings.Trim(p, " ")))
+			p = strings.Split(strings.Trim(p, " "), " ")[0]
+			fmt.Fprintf(b, "${%v:%v}", i+1, r.Replace(p))
 		}
 		b.WriteByte(')')
-		return b.String(), false
+		return b.String()
 
 	}
-	return label, false
+	return label
 }

internal/lsp/server.go

@@ -75,7 +75,9 @@ type Server struct {
 	initializedMu sync.Mutex
 	initialized   bool // set once the server has received "initialize" request
 
-	signatureHelpEnabled          bool
+	// Configurations.
+	// TODO(rstambler): Separate these into their own struct?
+	usePlaceholders               bool
 	snippetsSupported             bool
 	configurationSupported        bool
 	dynamicConfigurationSupported bool
@@ -102,36 +104,14 @@ func (s *Server) Initialize(ctx context.Context, params *protocol.InitializePara
 	}
 	s.initialized = true // mark server as initialized now
 
-	// Check if the client supports snippets in completion items.
-	if x, ok := params.Capabilities["textDocument"].(map[string]interface{}); ok {
-		if x, ok := x["completion"].(map[string]interface{}); ok {
-			if x, ok := x["completionItem"].(map[string]interface{}); ok {
-				if x, ok := x["snippetSupport"].(bool); ok {
-					s.snippetsSupported = x
-				}
-			}
-		}
-	}
-	// Check if the client supports configuration messages.
-	if x, ok := params.Capabilities["workspace"].(map[string]interface{}); ok {
-		if x, ok := x["configuration"].(bool); ok {
-			s.configurationSupported = x
-		}
-		if x, ok := x["didChangeConfiguration"].(map[string]interface{}); ok {
-			if x, ok := x["dynamicRegistration"].(bool); ok {
-				s.dynamicConfigurationSupported = x
-			}
-		}
-	}
-
-	s.signatureHelpEnabled = true
-
 	// TODO(rstambler): Change this default to protocol.Incremental (or add a
 	// flag). Disabled for now to simplify debugging.
 	s.textDocumentSyncKind = protocol.Full
 
-	//We need a "detached" context so it does not get timeout cancelled.
+	s.setClientCapabilities(params.Capabilities)
-	//TODO(iancottrell): Do we need to copy any values across?
+
+	// We need a "detached" context so it does not get timeout cancelled.
+	// TODO(iancottrell): Do we need to copy any values across?
 	viewContext := context.Background()
 	folders := params.WorkspaceFolders
 	if len(folders) == 0 {
@@ -195,6 +175,30 @@ func (s *Server) Initialize(ctx context.Context, params *protocol.InitializePara
 	}, nil
 }
 
+func (s *Server) setClientCapabilities(caps protocol.ClientCapabilities) {
+	// Check if the client supports snippets in completion items.
+	if x, ok := caps["textDocument"].(map[string]interface{}); ok {
+		if x, ok := x["completion"].(map[string]interface{}); ok {
+			if x, ok := x["completionItem"].(map[string]interface{}); ok {
+				if x, ok := x["snippetSupport"].(bool); ok {
+					s.snippetsSupported = x
+				}
+			}
+		}
+	}
+	// Check if the client supports configuration messages.
+	if x, ok := caps["workspace"].(map[string]interface{}); ok {
+		if x, ok := x["configuration"].(bool); ok {
+			s.configurationSupported = x
+		}
+		if x, ok := x["didChangeConfiguration"].(map[string]interface{}); ok {
+			if x, ok := x["dynamicRegistration"].(bool); ok {
+				s.dynamicConfigurationSupported = x
+			}
+		}
+	}
+}
+
 func (s *Server) Initialized(ctx context.Context, params *protocol.InitializedParams) error {
 	if s.configurationSupported {
 		if s.dynamicConfigurationSupported {
@@ -346,28 +350,7 @@ func (s *Server) DidClose(ctx context.Context, params *protocol.DidCloseTextDocu
 }
 
 func (s *Server) Completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) {
-	uri := span.NewURI(params.TextDocument.URI)
+	return s.completion(ctx, params)
-	view := s.findView(ctx, uri)
-	f, m, err := newColumnMap(ctx, view, uri)
-	if err != nil {
-		return nil, err
-	}
-	spn, err := m.PointSpan(params.Position)
-	if err != nil {
-		return nil, err
-	}
-	rng, err := spn.Range(m.Converter)
-	if err != nil {
-		return nil, err
-	}
-	items, prefix, err := source.Completion(ctx, f, rng.Start)
-	if err != nil {
-		return nil, err
-	}
-	return &protocol.CompletionList{
-		IsIncomplete: false,
-		Items:        toProtocolCompletionItems(items, prefix, params.Position, s.snippetsSupported, s.signatureHelpEnabled),
-	}, nil
 }
 
 func (s *Server) CompletionResolve(context.Context, *protocol.CompletionItem) (*protocol.CompletionItem, error) {
@@ -640,10 +623,8 @@ func (s *Server) processConfig(view *cache.View, config interface{}) error {
 	if !ok {
 		return fmt.Errorf("invalid config gopls type %T", config)
 	}
-	env := c["env"]
+	// Get the environment for the go/packages config.
-	if env == nil {
+	if env := c["env"]; env != nil {
-		return nil
-	}
 		menv, ok := env.(map[string]interface{})
 		if !ok {
 			return fmt.Errorf("invalid config gopls.env type %T", env)
@@ -651,6 +632,11 @@ func (s *Server) processConfig(view *cache.View, config interface{}) error {
 		for k, v := range menv {
 			view.Config.Env = applyEnv(view.Config.Env, k, v)
 		}
+	}
+	// Check if placeholders are enabled.
+	if usePlaceholders, ok := c["usePlaceholders"].(bool); ok {
+		s.usePlaceholders = usePlaceholders
+	}
 	return nil
 }