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: af5ab4d
GitHub-Pull-Request: #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>

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
 }
 

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,32 +259,29 @@ 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)
 
 	// 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) {
 	case *ast.Ident:
 		// 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,34 +293,36 @@ 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:
 	//   recv.‸(arg)
 	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 {

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

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)

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")
 }

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