internal/lsp: stop trimming prefix from InsertText
After some discussion about how to handle insert and filter text
(https://github.com/microsoft/vscode-languageserver-node/issues/488), it
seems that it is better practice to overwrite the prefix in completion
items, rather than trimming the prefix from the insert text.
Change-Id: I7c794b4b1d4518af31e7318a283aa3681a0cf66a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/176958
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go
index 7028101..2f54c68 100644
--- a/internal/lsp/completion.go
+++ b/internal/lsp/completion.go
@@ -33,11 +33,23 @@
items, prefix, err := source.Completion(ctx, f, rng.Start)
if err != nil {
s.log.Infof(ctx, "no completions found for %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
- items = []source.CompletionItem{}
+ }
+ // We might need to adjust the position to account for the prefix.
+ pos := params.Position
+ if prefix.Pos().IsValid() {
+ spn, err := span.NewRange(view.FileSet(), prefix.Pos(), 0).Span()
+ if err != nil {
+ s.log.Infof(ctx, "failed to get span for prefix position: %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
+ }
+ if prefixPos, err := m.Position(spn.Start()); err == nil {
+ pos = prefixPos
+ } else {
+ s.log.Infof(ctx, "failed to convert prefix position: %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
+ }
}
return &protocol.CompletionList{
IsIncomplete: false,
- Items: toProtocolCompletionItems(items, prefix, params.Position, s.insertTextFormat, s.usePlaceholders),
+ Items: toProtocolCompletionItems(items, prefix.Content(), pos, s.insertTextFormat, s.usePlaceholders),
}, nil
}
@@ -45,7 +57,7 @@
sort.SliceStable(candidates, func(i, j int) bool {
return candidates[i].Score > candidates[j].Score
})
- items := []protocol.CompletionItem{}
+ items := make([]protocol.CompletionItem, 0, len(candidates))
for i, candidate := range candidates {
// Match against the label.
if !strings.HasPrefix(candidate.Label, prefix) {
@@ -59,16 +71,6 @@
insertText = candidate.Snippet.String()
}
}
- // If the user has already typed some part of the completion candidate,
- // don't insert that portion of the text.
- if strings.HasPrefix(insertText, prefix) {
- insertText = insertText[len(prefix):]
- }
- // Don't filter on text that might have snippets in it.
- filterText := candidate.InsertText
- if strings.HasPrefix(filterText, prefix) {
- filterText = filterText[len(prefix):]
- }
item := protocol.CompletionItem{
Label: candidate.Label,
Detail: candidate.Detail,
@@ -85,7 +87,7 @@
// according to their score. This can be removed upon the resolution of
// https://github.com/Microsoft/language-server-protocol/issues/348.
SortText: fmt.Sprintf("%05d", i),
- FilterText: filterText,
+ FilterText: candidate.InsertText,
Preselect: i == 0,
}
// Trigger signature help for any function or method completion.
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index ac37738..1c309c5 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -113,7 +113,7 @@
items []CompletionItem
// prefix is the already-typed portion of the completion candidates.
- prefix string
+ prefix Prefix
// expectedType is the type we expect the completion candidate to be.
// It may not be set.
@@ -152,6 +152,14 @@
maybeInFieldName bool
}
+type Prefix struct {
+ content string
+ pos token.Pos
+}
+
+func (p Prefix) Content() string { return p.content }
+func (p Prefix) Pos() token.Pos { return p.pos }
+
// found adds a candidate completion.
//
// Only the first candidate of a given name is considered.
@@ -178,28 +186,28 @@
// The prefix is computed based on the preceding identifier and can be used by
// the client to score the quality of the completion. For instance, some clients
// may tolerate imperfect matches as valid completion results, since users may make typos.
-func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, string, error) {
+func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, Prefix, error) {
file := f.GetAST(ctx)
pkg := f.GetPackage(ctx)
if pkg == nil || pkg.IsIllTyped() {
- return nil, "", fmt.Errorf("package for %s is ill typed", f.URI())
+ return nil, Prefix{}, fmt.Errorf("package for %s is ill typed", f.URI())
}
// Completion is based on what precedes the cursor.
// Find the path to the position before pos.
path, _ := astutil.PathEnclosingInterval(file, pos-1, pos-1)
if path == nil {
- return nil, "", fmt.Errorf("cannot find node enclosing position")
+ return nil, Prefix{}, fmt.Errorf("cannot find node enclosing position")
}
// Skip completion inside comments.
for _, g := range file.Comments {
if g.Pos() <= pos && pos <= g.End() {
- return nil, "", nil
+ return nil, Prefix{}, nil
}
}
// Skip completion inside any kind of literal.
if _, ok := path[0].(*ast.BasicLit); ok {
- return nil, "", nil
+ return nil, Prefix{}, nil
}
clInfo := enclosingCompositeLiteral(path, pos, pkg.GetTypesInfo())
@@ -217,9 +225,12 @@
enclosingCompositeLiteral: clInfo,
}
+ // Set the filter prefix.
if ident, ok := path[0].(*ast.Ident); ok {
- // Set the filter prefix.
- c.prefix = ident.Name[:pos-ident.Pos()]
+ c.prefix = Prefix{
+ content: ident.Name[:pos-ident.Pos()],
+ pos: ident.Pos(),
+ }
}
c.expectedType = expectedType(c)
@@ -227,7 +238,7 @@
// Struct literals are handled entirely separately.
if c.wantStructFieldCompletions() {
if err := c.structLiteralFieldName(); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
return c.items, c.prefix, nil
}
@@ -237,7 +248,7 @@
// Is this the Sel part of a selector?
if sel, ok := path[1].(*ast.SelectorExpr); ok && sel.Sel == n {
if err := c.selector(sel); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
return c.items, c.prefix, nil
}
@@ -251,11 +262,11 @@
qual := types.RelativeTo(pkg.GetTypes())
of += ", of " + types.ObjectString(obj, qual)
}
- return nil, "", fmt.Errorf("this is a definition%s", of)
+ return nil, Prefix{}, fmt.Errorf("this is a definition%s", of)
}
}
if err := c.lexical(); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
// The function name hasn't been typed yet, but the parens are there:
@@ -263,18 +274,18 @@
case *ast.TypeAssertExpr:
// Create a fake selector expression.
if err := c.selector(&ast.SelectorExpr{X: n.X}); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
case *ast.SelectorExpr:
if err := c.selector(n); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
default:
// fallback to lexical completions
if err := c.lexical(); err != nil {
- return nil, "", err
+ return nil, Prefix{}, err
}
}
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 11ee960..f840e41 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -145,8 +145,10 @@
if !wantBuiltins && isBuiltin(item) {
continue
}
- if !strings.HasPrefix(item.Label, prefix) {
- continue //TODO: why is this needed?
+ // We let the client do fuzzy matching, so we return all possible candidates.
+ // To simplify testing, filter results with prefixes that don't match exactly.
+ if !strings.HasPrefix(item.Label, prefix.Content()) {
+ continue
}
got = append(got, item)
}
@@ -174,7 +176,7 @@
}
tok := f.GetToken(ctx)
pos := tok.Pos(src.Start().Offset())
- list, prefix, err := source.Completion(ctx, f, pos)
+ list, _, err := source.Completion(ctx, f, pos)
if err != nil {
t.Fatalf("failed for %v: %v", src, err)
}
@@ -194,9 +196,9 @@
var expected string
if usePlaceholders {
- expected = prefix + want.PlaceholderSnippet
+ expected = want.PlaceholderSnippet
} else {
- expected = prefix + want.PlainSnippet
+ expected = want.PlainSnippet
}
insertText := gotItem.InsertText
if usePlaceholders && gotItem.PlaceholderSnippet != nil {
diff --git a/internal/lsp/testdata/snippets/snippets.go b/internal/lsp/testdata/snippets/snippets.go
index 9df7b63..9871e15 100644
--- a/internal/lsp/testdata/snippets/snippets.go
+++ b/internal/lsp/testdata/snippets/snippets.go
@@ -10,21 +10,21 @@
func (Foo) Baz() func() {} //@item(snipMethodBaz, "Baz()", "func()", "field")
func _() {
- f //@snippet(" //", snipFoo, "oo(${1})", "oo(${1:i int}, ${2:b bool})")
+ f //@snippet(" //", snipFoo, "foo(${1})", "foo(${1:i int}, ${2:b bool})")
- bar //@snippet(" //", snipBar, "(${1})", "(${1:fn func()})")
+ bar //@snippet(" //", snipBar, "bar(${1})", "bar(${1:fn func()})")
- bar(nil) //@snippet("(", snipBar, "", "")
- bar(ba) //@snippet(")", snipBar, "r(${1})", "r(${1:fn func()})")
+ bar(nil) //@snippet("(", snipBar, "bar", "bar")
+ bar(ba) //@snippet(")", snipBar, "bar(${1})", "bar(${1:fn func()})")
var f Foo
- bar(f.Ba) //@snippet(")", snipMethodBaz, "z()", "z()")
+ bar(f.Ba) //@snippet(")", snipMethodBaz, "Baz()", "Baz()")
Foo{
- B //@snippet(" //", snipFieldBar, "ar: ${1},", "ar: ${1:int},")
+ B //@snippet(" //", snipFieldBar, "Bar: ${1},", "Bar: ${1:int},")
}
- Foo{B} //@snippet("}", snipFieldBar, "ar: ${1}", "ar: ${1:int}")
+ Foo{B} //@snippet("}", snipFieldBar, "Bar: ${1}", "Bar: ${1:int}")
Foo{} //@snippet("}", snipFieldBar, "Bar: ${1}", "Bar: ${1:int}")
- Foo{Foo{}.B} //@snippet("} ", snipFieldBar, "ar", "ar")
+ Foo{Foo{}.B} //@snippet("} ", snipFieldBar, "Bar", "Bar")
}
