internal/lsp: fix completion insertion
The insertion range for completion items was not right. The range's
end was 1 before the start. Fix by taking into account the length of
the prefix when generating the range start and end.
Now instead of a "prefix", we track the completion's
"surrounding". This is basically the start and end of the abutting
identifier along with the cursor position. When we insert the
completion text, we overwrite the entire identifier, not just the
prefix. This fixes postfix completion like completing "foo.<>Bar" to
"foo.BarBaz".
Fixes golang/go#32078
Fixes golang/go#32057
Change-Id: I9d065a413ff9a6e20ae662ff93ad0092c2007c1d
GitHub-Last-Rev: af5ab4d60566bf0589d9a712c80d75280178cba9
GitHub-Pull-Request: golang/tools#103
Reviewed-on: https://go-review.googlesource.com/c/tools/+/177757
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go
index 94b53bd..03062ec 100644
--- a/internal/lsp/completion.go
+++ b/internal/lsp/completion.go
@@ -30,29 +30,33 @@
if err != nil {
return nil, err
}
- items, prefix, err := source.Completion(ctx, f, rng.Start)
+ items, surrounding, err := source.Completion(ctx, f, rng.Start)
if err != nil {
s.session.Logger().Infof(ctx, "no completions found for %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
}
// We might need to adjust the position to account for the prefix.
- prefixRng := protocol.Range{
+ insertionRng := protocol.Range{
Start: params.Position,
End: params.Position,
}
- if prefix.Pos().IsValid() {
- spn, err := span.NewRange(view.FileSet(), prefix.Pos(), 0).Span()
+ var prefix string
+ if surrounding != nil {
+ prefix = surrounding.Prefix()
+ spn, err := surrounding.Range.Span()
if err != nil {
- s.session.Logger().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 {
- prefixRng.End = prefixPos
+ s.session.Logger().Infof(ctx, "failed to get span for surrounding position: %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
} else {
- s.session.Logger().Infof(ctx, "failed to convert prefix position: %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
+ rng, err := m.Range(spn)
+ if err != nil {
+ s.session.Logger().Infof(ctx, "failed to convert surrounding position: %s:%v:%v: %v", uri, int(params.Position.Line), int(params.Position.Character), err)
+ } else {
+ insertionRng = rng
+ }
}
}
return &protocol.CompletionList{
IsIncomplete: false,
- Items: toProtocolCompletionItems(items, prefix.Content(), prefixRng, s.insertTextFormat, s.usePlaceholders),
+ Items: toProtocolCompletionItems(items, prefix, insertionRng, s.insertTextFormat, s.usePlaceholders),
}, nil
}
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index 8b193b7..a95001f 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -13,6 +13,7 @@
"golang.org/x/tools/go/ast/astutil"
"golang.org/x/tools/internal/lsp/snippet"
+ "golang.org/x/tools/internal/span"
)
type CompletionItem struct {
@@ -126,8 +127,8 @@
// items is the list of completion items returned.
items []CompletionItem
- // prefix is the already-typed portion of the completion candidates.
- prefix Prefix
+ // surrounding describes the identifier surrounding the position.
+ surrounding *Selection
// expectedType is the type we expect the completion candidate to be.
// It may not be set.
@@ -166,13 +167,32 @@
maybeInFieldName bool
}
-type Prefix struct {
- content string
- pos token.Pos
+// A Selection represents the cursor position and surrounding identifier.
+type Selection struct {
+ Content string
+ Range span.Range
+ Cursor token.Pos
}
-func (p Prefix) Content() string { return p.content }
-func (p Prefix) Pos() token.Pos { return p.pos }
+func (p Selection) Prefix() string {
+ return p.Content[:p.Cursor-p.Range.Start]
+}
+
+func (c *completer) setSurrounding(ident *ast.Ident) {
+ if c.surrounding != nil {
+ return
+ }
+
+ if !(ident.Pos() <= c.pos && c.pos <= ident.End()) {
+ return
+ }
+
+ c.surrounding = &Selection{
+ Content: ident.Name,
+ Range: span.NewRange(c.view.FileSet(), ident.Pos(), ident.End()),
+ Cursor: c.pos,
+ }
+}
// found adds a candidate completion.
//
@@ -197,31 +217,31 @@
// Completion returns a list of possible candidates for completion, given a
// a file and a position.
//
-// The prefix is computed based on the preceding identifier and can be used by
+// The selection 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 GoFile, pos token.Pos) ([]CompletionItem, Prefix, error) {
+func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, *Selection, error) {
file := f.GetAST(ctx)
pkg := f.GetPackage(ctx)
if pkg == nil || pkg.IsIllTyped() {
- return nil, Prefix{}, fmt.Errorf("package for %s is ill typed", f.URI())
+ return nil, nil, 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, Prefix{}, fmt.Errorf("cannot find node enclosing position")
+ return nil, nil, 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, Prefix{}, nil
+ return nil, nil, nil
}
}
// Skip completion inside any kind of literal.
if _, ok := path[0].(*ast.BasicLit); ok {
- return nil, Prefix{}, nil
+ return nil, nil, nil
}
clInfo := enclosingCompositeLiteral(path, pos, pkg.GetTypesInfo())
@@ -239,12 +259,9 @@
enclosingCompositeLiteral: clInfo,
}
- // Set the filter prefix.
+ // Set the filter surrounding.
if ident, ok := path[0].(*ast.Ident); ok {
- c.prefix = Prefix{
- content: ident.Name[:pos-ident.Pos()],
- pos: ident.Pos(),
- }
+ c.setSurrounding(ident)
}
c.expectedType = expectedType(c)
@@ -252,9 +269,9 @@
// Struct literals are handled entirely separately.
if c.wantStructFieldCompletions() {
if err := c.structLiteralFieldName(); err != nil {
- return nil, Prefix{}, err
+ return nil, nil, err
}
- return c.items, c.prefix, nil
+ return c.items, c.surrounding, nil
}
switch n := path[0].(type) {
@@ -262,9 +279,9 @@
// 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, Prefix{}, err
+ return nil, nil, err
}
- return c.items, c.prefix, nil
+ return c.items, c.surrounding, nil
}
// reject defining identifiers
if obj, ok := pkg.GetTypesInfo().Defs[n]; ok {
@@ -276,11 +293,11 @@
qual := types.RelativeTo(pkg.GetTypes())
of += ", of " + types.ObjectString(obj, qual)
}
- return nil, Prefix{}, fmt.Errorf("this is a definition%s", of)
+ return nil, nil, fmt.Errorf("this is a definition%s", of)
}
}
if err := c.lexical(); err != nil {
- return nil, Prefix{}, err
+ return nil, nil, err
}
// The function name hasn't been typed yet, but the parens are there:
@@ -288,22 +305,24 @@
case *ast.TypeAssertExpr:
// Create a fake selector expression.
if err := c.selector(&ast.SelectorExpr{X: n.X}); err != nil {
- return nil, Prefix{}, err
+ return nil, nil, err
}
case *ast.SelectorExpr:
+ c.setSurrounding(n.Sel)
+
if err := c.selector(n); err != nil {
- return nil, Prefix{}, err
+ return nil, nil, err
}
default:
// fallback to lexical completions
if err := c.lexical(); err != nil {
- return nil, Prefix{}, err
+ return nil, nil, err
}
}
- return c.items, c.prefix, nil
+ return c.items, c.surrounding, nil
}
func (c *completer) wantStructFieldCompletions() bool {
diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go
index d212868..e5d0698 100644
--- a/internal/lsp/source/completion_format.go
+++ b/internal/lsp/source/completion_format.go
@@ -58,9 +58,6 @@
results, writeParens := formatResults(sig.Results(), c.qf)
label, detail = formatFunction(obj.Name(), params, results, writeParens)
plainSnippet, placeholderSnippet = c.functionCallSnippets(obj.Name(), params)
- if plainSnippet == nil && placeholderSnippet == nil {
- insert = ""
- }
kind = FunctionCompletionItem
if sig.Recv() != nil {
kind = MethodCompletionItem
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 9041160..475d9ac 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -135,7 +135,7 @@
}
tok := f.(source.GoFile).GetToken(ctx)
pos := tok.Pos(src.Start().Offset())
- list, prefix, err := source.Completion(ctx, f.(source.GoFile), pos)
+ list, surrounding, err := source.Completion(ctx, f.(source.GoFile), pos)
if err != nil {
t.Fatalf("failed for %v: %v", src, err)
}
@@ -145,9 +145,13 @@
if !wantBuiltins && isBuiltin(item) {
continue
}
+ var prefix string
+ if surrounding != nil {
+ prefix = surrounding.Prefix()
+ }
// 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()) {
+ if !strings.HasPrefix(item.Label, prefix) {
continue
}
got = append(got, item)
diff --git a/internal/lsp/testdata/snippets/snippets.go b/internal/lsp/testdata/snippets/snippets.go
index af10c88..0cf9232 100644
--- a/internal/lsp/testdata/snippets/snippets.go
+++ b/internal/lsp/testdata/snippets/snippets.go
@@ -15,7 +15,7 @@
bar //@snippet(" //", snipBar, "bar(${1})", "bar(${1:fn func()})")
- bar(nil) //@snippet("(", snipBar, "", "")
+ bar(nil) //@snippet("(", snipBar, "bar", "bar")
bar(ba) //@snippet(")", snipBar, "bar(${1})", "bar(${1:fn func()})")
var f Foo
bar(f.Ba) //@snippet(")", snipMethodBaz, "Baz()", "Baz()")
@@ -32,9 +32,8 @@
Foo{Foo{}.B} //@snippet("} ", snipFieldBar, "Bar", "Bar")
var err error
- err.Error() //@snippet("E", Error, "", "")
- f.Baz() //@snippet("B", snipMethodBaz, "", "")
+ err.Error() //@snippet("E", Error, "Error", "Error")
+ f.Baz() //@snippet("B", snipMethodBaz, "Baz", "Baz")
- // TODO(rstambler): Handle this case correctly.
- f.Baz() //@fails("(", snipMethodBazBar, "Bar", "Bar")
+ f.Baz() //@snippet("(", snipMethodBazBar, "BazBar", "BazBar")
}
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index ae5cfaa..301b847 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -29,7 +29,7 @@
// are being executed. If a test is added, this number must be changed.
const (
ExpectedCompletionsCount = 121
- ExpectedCompletionSnippetCount = 13
+ ExpectedCompletionSnippetCount = 14
ExpectedDiagnosticsCount = 17
ExpectedFormatCount = 5
ExpectedDefinitionsCount = 35
