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>

internal/lsp/completion.go

@@ -33,11 +33,23 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara
 	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 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 	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 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 				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 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 			// 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.

internal/lsp/source/completion.go

@@ -113,7 +113,7 @@ type completer struct {
 	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 @@ type compLitInfo struct {
 	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 @@ func (c *completer) found(obj types.Object, weight float64) {
 // 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 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 		enclosingCompositeLiteral: clInfo,
 	}
 
-	if ident, ok := path[0].(*ast.Ident); ok {
 	// Set the filter prefix.
-		c.prefix = ident.Name[:pos-ident.Pos()]
+	if ident, ok := path[0].(*ast.Ident); ok {
+		c.prefix = Prefix{
+			content: ident.Name[:pos-ident.Pos()],
+			pos:     ident.Pos(),
+		}
 	}
 
 	c.expectedType = expectedType(c)
@@ -227,7 +238,7 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 	// 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 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 		// 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 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 					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 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 	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
 		}
 	}
 

internal/lsp/source/source_test.go

@@ -145,8 +145,10 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests
 			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 @@ func (r *runner) checkCompletionSnippets(ctx context.Context, t *testing.T, data
 			}
 			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 @@ func (r *runner) checkCompletionSnippets(ctx context.Context, t *testing.T, data
 
 			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 {

internal/lsp/testdata/snippets/snippets.go

@@ -10,21 +10,21 @@ type Foo struct {
 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")
 }