internal/lsp: simplify snippet config/generation

I moved the "usePlaceholders" config field on to CompletionOptions.
This way the completion code generates a single snippet with a little
conditional logic based on the "WantPlaceholders" option instead of
juggling the generation of two almost identical "plain" and
"placeholder" snippets at the same time. It also reduces the work done
generating completion candidates a little.

I also made a minor tweak to the snippet builder where empty
placeholders are now always represented as e.g "${1:}" instead of
"${1}" or "${1:}", depending on if you passed a callback to
WritePlaceholder() or not.

Change-Id: Ib84cc0cd729a11b9e13ad3ac4b6fd2d82460acd5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193697
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/lsp/completion.go

@@ -70,8 +70,9 @@ func (s *Server) toProtocolCompletionItems(candidates []source.CompletionItem, r
 		}
 		insertText := candidate.InsertText
 		if options.InsertTextFormat == protocol.SnippetTextFormat {
-			insertText = candidate.Snippet(options.UsePlaceholders)
+			insertText = candidate.Snippet()
 		}
+
 		item := protocol.CompletionItem{
 			Label:  candidate.Label,
 			Detail: candidate.Detail,

internal/lsp/general.go

@@ -322,7 +322,7 @@ func (s *Server) processConfig(ctx context.Context, view source.View, options *s
 	setBool(&options.Completion.Unimported, c, "wantUnimportedCompletions")
 
 	// If the user wants placeholders for autocompletion results.
-	setBool(&options.UsePlaceholders, c, "usePlaceholders")
+	setBool(&options.Completion.Placeholders, c, "usePlaceholders")
 
 	return nil
 }

internal/lsp/lsp_test.go

@@ -153,12 +153,11 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests
 
 	modified.InsertTextFormat = protocol.SnippetTextFormat
 	for _, usePlaceholders := range []bool{true, false} {
-		modified.UsePlaceholders = usePlaceholders
-
 		for src, want := range snippets {
 			modified.Completion.Deep = strings.Contains(string(src.URI()), "deepcomplete")
 			modified.Completion.FuzzyMatching = strings.Contains(string(src.URI()), "fuzzymatch")
 			modified.Completion.Unimported = strings.Contains(string(src.URI()), "unimported")
+			modified.Completion.Placeholders = usePlaceholders
 			r.server.session.SetOptions(modified)
 
 			list := r.runCompletion(t, src)

internal/lsp/snippet/snippet_builder.go

@@ -43,9 +43,8 @@ func (b *Builder) WriteText(s string) {
 // The callback style allows for creating nested placeholders. To write an
 // empty tab stop, provide a nil callback.
 func (b *Builder) WritePlaceholder(fn func(*Builder)) {
-	fmt.Fprintf(&b.sb, "${%d", b.nextTabStop())
+	fmt.Fprintf(&b.sb, "${%d:", b.nextTabStop())
 	if fn != nil {
-		b.sb.WriteByte(':')
 		fn(b)
 	}
 	b.sb.WriteByte('}')

internal/lsp/snippet/snippet_builder_test.go

@@ -10,6 +10,8 @@ import (
 
 func TestSnippetBuilder(t *testing.T) {
 	expect := func(expected string, fn func(*Builder)) {
+		t.Helper()
+
 		var b Builder
 		fn(&b)
 		if got := b.String(); got != expected {
@@ -23,7 +25,7 @@ func TestSnippetBuilder(t *testing.T) {
 		b.WriteText(`hi { } $ | " , / \`)
 	})
 
-	expect("${1}", func(b *Builder) {
+	expect("${1:}", func(b *Builder) {
 		b.WritePlaceholder(nil)
 	})
 

internal/lsp/source/completion.go

@@ -55,21 +55,9 @@ type CompletionItem struct {
 	// A higher score indicates that this completion item is more relevant.
 	Score float64
 
-	// Snippet is the LSP snippet for the completion item, without placeholders.
-	// The LSP specification contains details about LSP snippets.
-	// For example, a snippet for a function with the following signature:
-	//
-	//     func foo(a, b, c int)
-	//
-	// would be:
-	//
-	//     foo(${1:})
-	//
-	plainSnippet *snippet.Builder
-
-	// PlaceholderSnippet is the LSP snippet for the completion ite, containing
-	// placeholders. The LSP specification contains details about LSP snippets.
-	// For example, a placeholder snippet for a function with the following signature:
+	// snippet is the LSP snippet for the completion item. The LSP
+	// specification contains details about LSP snippets. For example, a
+	// snippet for a function with the following signature:
 	//
 	//     func foo(a, b, c int)
 	//
@@ -77,22 +65,22 @@ type CompletionItem struct {
 	//
 	//     foo(${1:a int}, ${2: b int}, ${3: c int})
 	//
-	placeholderSnippet *snippet.Builder
+	// If Placeholders is false in the CompletionOptions, the above
+	// snippet would instead be:
+	//
+	//     foo(${1:})
+	snippet *snippet.Builder
 
 	// Documentation is the documentation for the completion item.
 	Documentation string
 }
 
-// Snippet is a convenience function that determines the snippet that should be
+// Snippet is a convenience returns the snippet if available, otherwise
+// the InsertText.
 // used for an item, depending on if the callee wants placeholders or not.
-func (i *CompletionItem) Snippet(usePlaceholders bool) string {
-	if usePlaceholders {
-		if i.placeholderSnippet != nil {
-			return i.placeholderSnippet.String()
-		}
-	}
-	if i.plainSnippet != nil {
-		return i.plainSnippet.String()
+func (i *CompletionItem) Snippet() string {
+	if i.snippet != nil {
+		return i.snippet.String()
 	}
 	return i.InsertText
 }

internal/lsp/source/completion_format.go

@@ -30,20 +30,19 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 	}
 
 	var (
-		label              = cand.name
-		detail             = types.TypeString(obj.Type(), c.qf)
-		insert             = label
-		kind               CompletionItemKind
-		plainSnippet       *snippet.Builder
-		placeholderSnippet *snippet.Builder
-		protocolEdits      []protocol.TextEdit
+		label         = cand.name
+		detail        = types.TypeString(obj.Type(), c.qf)
+		insert        = label
+		kind          CompletionItemKind
+		snip          *snippet.Builder
+		protocolEdits []protocol.TextEdit
 	)
 
-	// expandFuncCall mutates the completion label, detail, and snippets
+	// expandFuncCall mutates the completion label, detail, and snippet
 	// to that of an invocation of sig.
 	expandFuncCall := func(sig *types.Signature) {
 		params := formatParams(sig.Params(), sig.Variadic(), c.qf)
-		plainSnippet, placeholderSnippet = c.functionCallSnippets(label, params)
+		snip = c.functionCallSnippet(label, params)
 		results, writeParens := formatResults(sig.Results(), c.qf)
 		detail = "func" + formatFunction(params, results, writeParens)
 	}
@@ -59,7 +58,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 		}
 		if obj.IsField() {
 			kind = FieldCompletionItem
-			plainSnippet, placeholderSnippet = c.structFieldSnippets(label, detail)
+			snip = c.structFieldSnippet(label, detail)
 		} else if c.isParameter(obj) {
 			kind = ParameterCompletionItem
 		} else {
@@ -110,8 +109,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 		Kind:                kind,
 		Score:               cand.score,
 		Depth:               len(c.deepState.chain),
-		plainSnippet:        plainSnippet,
-		placeholderSnippet:  placeholderSnippet,
+		snippet:             snip,
 	}
 	// If the user doesn't want documentation for completion items.
 	if !c.opts.Documentation {
@@ -200,7 +198,7 @@ func (c *completer) formatBuiltin(cand candidate) CompletionItem {
 		results, writeResultParens := formatFieldList(c.ctx, c.view, decl.Type.Results)
 		item.Label = obj.Name()
 		item.Detail = "func" + formatFunction(params, results, writeResultParens)
-		item.plainSnippet, item.placeholderSnippet = c.functionCallSnippets(obj.Name(), params)
+		item.snippet = c.functionCallSnippet(obj.Name(), params)
 	case *types.TypeName:
 		if types.IsInterface(obj.Type()) {
 			item.Kind = InterfaceCompletionItem

internal/lsp/source/completion_snippet.go

@@ -5,57 +5,53 @@
 package source
 
 import (
-	"fmt"
 	"go/ast"
 
 	"golang.org/x/tools/internal/lsp/snippet"
 )
 
-// structFieldSnippets calculates the plain and placeholder snippets for struct literal field names.
-func (c *completer) structFieldSnippets(label, detail string) (*snippet.Builder, *snippet.Builder) {
+// structFieldSnippets calculates the snippet for struct literal field names.
+func (c *completer) structFieldSnippet(label, detail string) *snippet.Builder {
 	if !c.wantStructFieldCompletions() {
-		return nil, nil
+		return nil
 	}
 
 	// If we are in a deep completion then we can't be completing a field
 	// name (e.g. "Foo{f<>}" completing to "Foo{f.Bar}" should not generate
 	// a snippet).
 	if c.inDeepCompletion() {
-		return nil, nil
+		return nil
 	}
 
 	clInfo := c.enclosingCompositeLiteral
 
 	// If we are already in a key-value expression, we don't want a snippet.
 	if clInfo.kv != nil {
-		return nil, nil
+		return nil
 	}
 
-	plain, placeholder := &snippet.Builder{}, &snippet.Builder{}
-	label = fmt.Sprintf("%s: ", label)
+	snip := &snippet.Builder{}
 
 	// A plain snippet turns "Foo{Ba<>" into "Foo{Bar: <>".
-	plain.WriteText(label)
-	plain.WritePlaceholder(nil)
-
-	// A placeholder snippet turns "Foo{Ba<>" into "Foo{Bar: <*int*>".
-	placeholder.WriteText(label)
-	placeholder.WritePlaceholder(func(b *snippet.Builder) {
-		b.WriteText(detail)
+	snip.WriteText(label + ": ")
+	snip.WritePlaceholder(func(b *snippet.Builder) {
+		// A placeholder snippet turns "Foo{Ba<>" into "Foo{Bar: <*int*>".
+		if c.opts.Placeholders {
+			b.WriteText(detail)
+		}
 	})
 
 	// If the cursor position is on a different line from the literal's opening brace,
 	// we are in a multiline literal.
 	if c.view.Session().Cache().FileSet().Position(c.pos).Line != c.view.Session().Cache().FileSet().Position(clInfo.cl.Lbrace).Line {
-		plain.WriteText(",")
-		placeholder.WriteText(",")
+		snip.WriteText(",")
 	}
 
-	return plain, placeholder
+	return snip
 }
 
-// functionCallSnippets calculates the plain and placeholder snippets for function calls.
-func (c *completer) functionCallSnippets(name string, params []string) (*snippet.Builder, *snippet.Builder) {
+// functionCallSnippets calculates the snippet for function calls.
+func (c *completer) functionCallSnippet(name string, params []string) *snippet.Builder {
 	// If we are the left side (i.e. "Fun") part of a call expression,
 	// we don't want a snippet since there are already parens present.
 	if len(c.path) > 1 {
@@ -65,37 +61,37 @@ func (c *completer) functionCallSnippets(name string, params []string) (*snippet
 			// inserted when fixing the AST. In this case, we do still need
 			// to insert the calling "()" parens.
 			if n.Fun == c.path[0] && n.Lparen != n.Rparen {
-				return nil, nil
+				return nil
 			}
 		case *ast.SelectorExpr:
 			if len(c.path) > 2 {
 				if call, ok := c.path[2].(*ast.CallExpr); ok && call.Fun == c.path[1] && call.Lparen != call.Rparen {
-					return nil, nil
+					return nil
 				}
 			}
 		}
 	}
-	plain, placeholder := &snippet.Builder{}, &snippet.Builder{}
-	label := fmt.Sprintf("%s(", name)
+	snip := &snippet.Builder{}
+	snip.WriteText(name + "(")
 
-	// A plain snippet turns "someFun<>" into "someFunc(<>)".
-	plain.WriteText(label)
-	if len(params) > 0 {
-		plain.WritePlaceholder(nil)
-	}
-	plain.WriteText(")")
-
-	// A placeholder snippet turns "someFun<>" into "someFunc(<*i int*>, *s string*)".
-	placeholder.WriteText(label)
-	for i, p := range params {
-		if i > 0 {
-			placeholder.WriteText(", ")
+	if c.opts.Placeholders {
+		// A placeholder snippet turns "someFun<>" into "someFunc(<*i int*>, *s string*)".
+		for i, p := range params {
+			if i > 0 {
+				snip.WriteText(", ")
+			}
+			snip.WritePlaceholder(func(b *snippet.Builder) {
+				b.WriteText(p)
+			})
+		}
+	} else {
+		// A plain snippet turns "someFun<>" into "someFunc(<>)".
+		if len(params) > 0 {
+			snip.WritePlaceholder(nil)
 		}
-		placeholder.WritePlaceholder(func(b *snippet.Builder) {
-			b.WriteText(p)
-		})
 	}
-	placeholder.WriteText(")")
 
-	return plain, placeholder
+	snip.WriteText(")")
+
+	return snip
 }

internal/lsp/source/options.go

@@ -31,7 +31,6 @@ var (
 type SessionOptions struct {
 	Env              []string
 	BuildFlags       []string
-	UsePlaceholders  bool
 	HoverKind        HoverKind
 	DisabledAnalyses map[string]struct{}
 
@@ -59,6 +58,7 @@ type CompletionOptions struct {
 	Unimported        bool
 	Documentation     bool
 	FullDocumentation bool
+	Placeholders      bool
 }
 
 type HoverKind int

internal/lsp/source/source_test.go

@@ -167,6 +167,7 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests
 				Documentation: true,
 				Deep:          strings.Contains(string(src.URI()), "deepcomplete"),
 				FuzzyMatching: strings.Contains(string(src.URI()), "fuzzymatch"),
+				Placeholders:  usePlaceholders,
 			})
 			if err != nil {
 				t.Fatalf("failed for %v: %v", src, err)
@@ -186,8 +187,18 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests
 			if usePlaceholders {
 				expected = want.PlaceholderSnippet
 			}
-			if actual := got.Snippet(usePlaceholders); expected != actual {
-				t.Errorf("%s: expected placeholder snippet %q, got %q", src, expected, actual)
+			if expected == "" {
+				if got != nil {
+					t.Fatalf("%s:%d: expected no matching snippet", src.URI(), src.Start().Line())
+				}
+			} else {
+				if got == nil {
+					t.Fatalf("%s:%d: couldn't find completion matching %q", src.URI(), src.Start().Line(), wantItem.Label)
+				}
+				actual := got.Snippet()
+				if expected != actual {
+					t.Errorf("%s: expected placeholder snippet %q, got %q", src, expected, actual)
+				}
 			}
 		}
 	}

internal/lsp/testdata/snippets/snippets.go.in

@@ -11,23 +11,23 @@ func (Foo) Baz() func() {} //@item(snipMethodBaz, "Baz", "func() func()", "metho
 func (Foo) BazBar() func() {} //@item(snipMethodBazBar, "BazBar", "func() func()", "method")
 
 func _() {
-	f //@snippet(" //", snipFoo, "foo(${1})", "foo(${1:i int}, ${2:b bool})")
+	f //@snippet(" //", snipFoo, "foo(${1:})", "foo(${1:i int}, ${2:b bool})")
 
-	bar //@snippet(" //", snipBar, "bar(${1})", "bar(${1:fn func()})")
+	bar //@snippet(" //", snipBar, "bar(${1:})", "bar(${1:fn func()})")
 
 	bar(nil) //@snippet("(", snipBar, "bar", "bar")
-	bar(ba) //@snippet(")", snipBar, "bar(${1})", "bar(${1:fn func()})")
+	bar(ba) //@snippet(")", snipBar, "bar(${1:})", "bar(${1:fn func()})")
 	var f Foo
 	bar(f.Ba) //@snippet(")", snipMethodBaz, "Baz()", "Baz()")
-	(bar)(nil) //@snippet(")", snipBar, "bar(${1})", "bar(${1:fn func()})")
+	(bar)(nil) //@snippet(")", snipBar, "bar(${1:})", "bar(${1:fn func()})")
 	(f.Ba)() //@snippet(")", snipMethodBaz, "Baz()", "Baz()")
 
 	Foo{
-		B //@snippet(" //", snipFieldBar, "Bar: ${1},", "Bar: ${1:int},")
+		B //@snippet(" //", snipFieldBar, "Bar: ${1:},", "Bar: ${1:int},")
 	}
 
-	Foo{B} //@snippet("}", snipFieldBar, "Bar: ${1}", "Bar: ${1:int}")
-	Foo{} //@snippet("}", snipFieldBar, "Bar: ${1}", "Bar: ${1:int}")
+	Foo{B} //@snippet("}", snipFieldBar, "Bar: ${1:}", "Bar: ${1:int}")
+	Foo{} //@snippet("}", snipFieldBar, "Bar: ${1:}", "Bar: ${1:int}")
 
 	Foo{Foo{}.B} //@snippet("} ", snipFieldBar, "Bar", "Bar")