From dd2b5c81c5787692d57849f9ab43d07977f3e76c Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Wed, 4 Sep 2019 10:23:14 -0700 Subject: [PATCH] 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 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot --- internal/lsp/completion.go | 3 +- internal/lsp/general.go | 2 +- internal/lsp/lsp_test.go | 3 +- internal/lsp/snippet/snippet_builder.go | 3 +- internal/lsp/snippet/snippet_builder_test.go | 4 +- internal/lsp/source/completion.go | 38 ++++----- internal/lsp/source/completion_format.go | 24 +++--- internal/lsp/source/completion_snippet.go | 78 +++++++++---------- internal/lsp/source/options.go | 2 +- internal/lsp/source/source_test.go | 15 +++- .../snippets/{snippets.go => snippets.go.in} | 14 ++-- 11 files changed, 90 insertions(+), 96 deletions(-) rename internal/lsp/testdata/snippets/{snippets.go => snippets.go.in} (64%) diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 77850e2609..7b330633b2 100644 --- a/internal/lsp/completion.go +++ b/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, diff --git a/internal/lsp/general.go b/internal/lsp/general.go index 0f49faa1b7..f48507090f 100644 --- a/internal/lsp/general.go +++ b/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 } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index fd9fbe7ec6..b756ebf745 100644 --- a/internal/lsp/lsp_test.go +++ b/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) diff --git a/internal/lsp/snippet/snippet_builder.go b/internal/lsp/snippet/snippet_builder.go index 14c5d712b3..a17091cab3 100644 --- a/internal/lsp/snippet/snippet_builder.go +++ b/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('}') diff --git a/internal/lsp/snippet/snippet_builder_test.go b/internal/lsp/snippet/snippet_builder_test.go index 810a7fe195..bb47f52e47 100644 --- a/internal/lsp/snippet/snippet_builder_test.go +++ b/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) }) diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index faa2aaaff8..497ecbffec 100644 --- a/internal/lsp/source/completion.go +++ b/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 } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 587c5fc6a0..e86ab2911d 100644 --- a/internal/lsp/source/completion_format.go +++ b/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 diff --git a/internal/lsp/source/completion_snippet.go b/internal/lsp/source/completion_snippet.go index 90cf02a33a..02c56f50a8 100644 --- a/internal/lsp/source/completion_snippet.go +++ b/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 } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 6770902031..81596f8284 100644 --- a/internal/lsp/source/options.go +++ b/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 diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 51b220e2aa..f0ec5bdda6 100644 --- a/internal/lsp/source/source_test.go +++ b/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) + } } } } diff --git a/internal/lsp/testdata/snippets/snippets.go b/internal/lsp/testdata/snippets/snippets.go.in similarity index 64% rename from internal/lsp/testdata/snippets/snippets.go rename to internal/lsp/testdata/snippets/snippets.go.in index ee56eb3c86..872753b7b7 100644 --- a/internal/lsp/testdata/snippets/snippets.go +++ b/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")