internal/lsp: don't overwrite suffix when inserting completion

In cases like "fmt.Pr<>int()" we previously would replace "Print" with
the new completion, yielding for example "fmt.Println()". Now we no
longer overwrite, yielding "fmt.Println()int()". There are some cases
where overwriting the suffix is what the user wants, but it is hard to
tell, so for now stick with the more expected behavior of not
overwriting.

Fixes golang/go#34011.

Change-Id: I8c3ccd8948245c27b52408ad508d8e01dc163ef4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196119
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/completion.go

@@ -46,8 +46,7 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara
 	return &protocol.CompletionList{
 		// When using deep completions/fuzzy matching, report results as incomplete so
 		// client fetches updated completions after every key stroke.
-		IsIncomplete: options.Completion.Deep,
-		Items:        toProtocolCompletionItems(candidates, rng, options),
+		Items: toProtocolCompletionItems(candidates, rng, options),
 	}, nil
 }
 

internal/lsp/source/completion.go

@@ -226,6 +226,10 @@ func (p Selection) Prefix() string {
 	return p.content[:p.cursor-p.spanRange.Start]
 }
 
+func (p Selection) Suffix() string {
+	return p.content[p.cursor-p.spanRange.Start:]
+}
+
 func (c *completer) setSurrounding(ident *ast.Ident) {
 	if c.surrounding != nil {
 		return
@@ -233,11 +237,13 @@ func (c *completer) setSurrounding(ident *ast.Ident) {
 	if !(ident.Pos() <= c.pos && c.pos <= ident.End()) {
 		return
 	}
+
 	c.surrounding = &Selection{
 		content: ident.Name,
 		cursor:  c.pos,
 		mappedRange: mappedRange{
-			spanRange: span.NewRange(c.view.Session().Cache().FileSet(), ident.Pos(), ident.End()),
+			// Overwrite the prefix only.
+			spanRange: span.NewRange(c.view.Session().Cache().FileSet(), ident.Pos(), c.pos),
 			m:         c.mapper,
 		},
 	}

internal/lsp/source/completion_snippet.go

@@ -41,9 +41,11 @@ func (c *completer) structFieldSnippet(label, detail string) *snippet.Builder {
 		}
 	})
 
+	fset := c.view.Session().Cache().FileSet()
+
 	// 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 {
+	if fset.Position(c.pos).Line != fset.Position(clInfo.cl.Lbrace).Line {
 		snip.WriteText(",")
 	}
 
@@ -52,9 +54,12 @@ func (c *completer) structFieldSnippet(label, detail string) *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 {
+	// If there is no suffix then we need to reuse existing call parens
+	// "()" if present. If there is an identifer suffix then we always
+	// need to include "()" since we don't overwrite the suffix.
+	if c.surrounding != nil && c.surrounding.Suffix() == "" && len(c.path) > 1 {
+		// 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.
 		switch n := c.path[1].(type) {
 		case *ast.CallExpr:
 			// The Lparen != Rparen check detects fudged CallExprs we

internal/lsp/testdata/snippets/snippets.go.in

@@ -32,8 +32,8 @@ func _() {
 	Foo{Foo{}.B} //@snippet("} ", snipFieldBar, "Bar", "Bar")
 
 	var err error
-	err.Error() //@snippet("E", Error, "Error", "Error")
-	f.Baz()     //@snippet("B", snipMethodBaz, "Baz", "Baz")
+	err.Error() //@snippet("E", Error, "Error()", "Error()")
+	f.Baz()     //@snippet("B", snipMethodBaz, "Baz()", "Baz()")
 
 	f.Baz()     //@snippet("(", snipMethodBazBar, "BazBar", "BazBar")
 }