From f217c98fae20b7e3cc3c407af13b132b46484a15 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Fri, 17 May 2019 17:45:59 +0000 Subject: [PATCH] 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 Reviewed-by: Ian Cottrell --- internal/lsp/completion.go | 24 ++++--- internal/lsp/source/completion.go | 75 ++++++++++++++-------- internal/lsp/source/completion_format.go | 3 - internal/lsp/source/source_test.go | 8 ++- internal/lsp/testdata/snippets/snippets.go | 9 ++- internal/lsp/tests/tests.go | 2 +- 6 files changed, 72 insertions(+), 49 deletions(-) diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 94b53bdb..03062ecb 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -30,29 +30,33 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara 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 8b193b74..a95001fe 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -13,6 +13,7 @@ import ( "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 @@ type completer struct { // 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 @@ type compLitInfo struct { 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 @@ func (c *completer) found(obj types.Object, weight float64) { // 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 @@ func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, 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 @@ func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, // 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 @@ func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, // 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 @@ func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, 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 @@ func Completion(ctx context.Context, f GoFile, pos token.Pos) ([]CompletionItem, 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 d2128687..e5d06986 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -58,9 +58,6 @@ func (c *completer) item(obj types.Object, score float64) CompletionItem { 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 9041160b..475d9ace 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -135,7 +135,7 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests } 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 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests 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 af10c885..0cf92328 100644 --- a/internal/lsp/testdata/snippets/snippets.go +++ b/internal/lsp/testdata/snippets/snippets.go @@ -15,7 +15,7 @@ func _() { 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 @@ func _() { 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 ae5cfaa1..301b847e 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -29,7 +29,7 @@ import ( // 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