internal/lsp: improve variadic completion

Improve candidate ranking when completing the variadic parameter of
function calls.

Using the example:

func foo(strs ...string) {}

- When completing foo(<>), we prefer candidates of type []string or
  string (previously we only preferred []string).

- When completing foo("hi", <>), we prefer candidates of type
  string (previously we preferred []string).

- When completing foo(<>), we use a snippet to add on the "..."
  automatically to candidates of type []string.

I also fixed completion tests to work properly when you have multiple
notes referring to the same position. For example:

foo() //@rank(")", a, b),rank(")", a, c)

Previously the second "rank" was silently overwriting the first
because they both refer to the same ")".

Fixes golang/go#34334.

Change-Id: I4f64be44a4ccbb533fb7682738c759cbca3a93cd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205117
Reviewed-by: Rebecca Stambler <[email protected]>
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: muirdm

internal/lsp/source/completion.go

@@ -1101,16 +1101,41 @@ Nodes:
 
 				if tv, ok := c.pkg.GetTypesInfo().Types[node.Fun]; ok {
 					if sig, ok := tv.Type.(*types.Signature); ok {
-						if sig.Params().Len() == 0 {
+						numParams := sig.Params().Len()
+						if numParams == 0 {
 							return inf
 						}
-						i := indexExprAtPos(c.pos, node.Args)
+
+						var (
+							exprIdx         = indexExprAtPos(c.pos, node.Args)
+							isLastParam     = exprIdx == numParams-1
+							beyondLastParam = exprIdx >= numParams
+						)
+
+						if sig.Variadic() {
+							// If we are beyond the last param or we are the last
+							// param w/ further expressions, we expect a single
+							// variadic item.
+							if beyondLastParam || isLastParam && len(node.Args) > numParams {
+								inf.objType = sig.Params().At(numParams - 1).Type().(*types.Slice).Elem()
+								break Nodes
+							}
+
+							// Otherwise if we are at the last param then we are
+							// completing the variadic positition (i.e. we expect a
+							// slice type []T or an individual item T).
+							if isLastParam {
+								inf.variadic = true
+							}
+						}
+
 						// Make sure not to run past the end of expected parameters.
-						if i >= sig.Params().Len() {
-							i = sig.Params().Len() - 1
+						if beyondLastParam {
+							inf.objType = sig.Params().At(numParams - 1).Type()
+						} else {
+							inf.objType = sig.Params().At(exprIdx).Type()
 						}
-						inf.objType = sig.Params().At(i).Type()
-						inf.variadic = sig.Variadic() && i == sig.Params().Len()-1
+
 						break Nodes
 					}
 				}
@@ -1225,6 +1250,12 @@ func (ti typeInference) applyTypeNameModifiers(typ types.Type) types.Type {
 	return typ
 }
 
+// matchesVariadic returns true if we are completing a variadic
+// parameter and candType is a compatible slice type.
+func (ti typeInference) matchesVariadic(candType types.Type) bool {
+	return ti.variadic && types.AssignableTo(ti.objType, candType)
+}
+
 // findSwitchStmt returns an *ast.CaseClause's corresponding *ast.SwitchStmt or
 // *ast.TypeSwitchStmt. path should start from the case clause's first ancestor.
 func findSwitchStmt(path []ast.Node, pos token.Pos, c *ast.CaseClause) ast.Stmt {
@@ -1443,6 +1474,13 @@ func (c *completer) matchingCandidate(cand *candidate) bool {
 		}
 	}
 
+	// When completing the variadic parameter, say objType matches if
+	// []objType matches. This is because you can use []T or T for the
+	// variadic parameter.
+	if c.expectedType.variadic && typeMatches(types.NewSlice(objType)) {
+		return true
+	}
+
 	if c.expectedType.convertibleTo != nil {
 		return types.ConvertibleTo(objType, c.expectedType.convertibleTo)
 	}

internal/lsp/source/completion_format.go

@@ -49,6 +49,11 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 		snip = c.functionCallSnippet(label, params)
 		results, writeParens := formatResults(sig.Results(), c.qf)
 		detail = "func" + formatFunction(params, results, writeParens)
+
+		// Add variadic "..." if we are using a function result to fill in a variadic parameter.
+		if sig.Results().Len() == 1 && c.expectedType.matchesVariadic(sig.Results().At(0).Type()) {
+			snip.WriteText("...")
+		}
 	}
 
 	switch obj := obj.(type) {
@@ -73,6 +78,12 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 		if sig, ok := obj.Type().Underlying().(*types.Signature); ok && cand.expandFuncCall {
 			expandFuncCall(sig)
 		}
+
+		// Add variadic "..." if we are using a variable to fill in a variadic parameter.
+		if c.expectedType.matchesVariadic(obj.Type()) {
+			snip = &snippet.Builder{}
+			snip.WriteText(insert + "...")
+		}
 	case *types.Func:
 		sig, ok := obj.Type().Underlying().(*types.Signature)
 		if !ok {

internal/lsp/testdata/summary.txt.golden

@@ -1,10 +1,10 @@
 -- summary --
-CompletionsCount = 213
-CompletionSnippetCount = 40
+CompletionsCount = 214
+CompletionSnippetCount = 43
 UnimportedCompletionsCount = 3
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 7
-RankedCompletionsCount = 8
+RankedCompletionsCount = 16
 CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 22
 FoldingRangesCount = 2

internal/lsp/testdata/variadic/variadic.go.in

@@ -0,0 +1,23 @@
+package variadic
+
+func foo(i int, strs ...string) {}
+
+func bar() []string { //@item(vFunc, "bar", "func() []string", "func")
+	return nil
+}
+
+func _() {
+	var (
+		i  int      //@item(vInt, "i", "int", "var")
+		s  string   //@item(vStr, "s", "string", "var")
+		ss []string //@item(vStrSlice, "ss", "[]string", "var")
+	)
+
+	foo()          //@rank(")", vInt, vStr),rank(")", vInt, vStrSlice)
+	foo(123, )     //@rank(")", vStr, vInt),rank(")", vStrSlice, vInt)
+	foo(123, "", ) //@rank(")", vStr, vInt),rank(")", vStr, vStrSlice)
+	foo(123, , "") //@rank(" ,", vStr, vInt),rank(")", vStr, vStrSlice)
+
+  // snippet will add the "..." for you
+	foo(123, ) //@snippet(")", vStrSlice, "ss...", "ss..."),snippet(")", vFunc, "bar()...", "bar()..."),snippet(")", vStr, "s", "s")
+}

internal/lsp/tests/tests.go

@@ -15,6 +15,7 @@ import (
 	"io/ioutil"
 	"path/filepath"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -39,13 +40,13 @@ var UpdateGolden = flag.Bool("golden", false, "Update golden files")
 
 type Diagnostics map[span.URI][]source.Diagnostic
 type CompletionItems map[token.Pos]*source.CompletionItem
-type Completions map[span.Span]Completion
-type CompletionSnippets map[span.Span]CompletionSnippet
-type UnimportedCompletions map[span.Span]Completion
-type DeepCompletions map[span.Span]Completion
-type FuzzyCompletions map[span.Span]Completion
-type CaseSensitiveCompletions map[span.Span]Completion
-type RankCompletions map[span.Span]Completion
+type Completions map[span.Span][]Completion
+type CompletionSnippets map[span.Span][]CompletionSnippet
+type UnimportedCompletions map[span.Span][]Completion
+type DeepCompletions map[span.Span][]Completion
+type FuzzyCompletions map[span.Span][]Completion
+type CaseSensitiveCompletions map[span.Span][]Completion
+type RankCompletions map[span.Span][]Completion
 type FoldingRanges []span.Span
 type Formats []span.Span
 type Imports []span.Span
@@ -343,80 +344,67 @@ func Run(t *testing.T, tests Tests, data *Data) {
 	t.Helper()
 	checkData(t, data)
 
-	t.Run("Completion", func(t *testing.T) {
+	eachCompletion := func(t *testing.T, cases map[span.Span][]Completion, test func(*testing.T, span.Span, Completion, CompletionItems)) {
 		t.Helper()
-		for src, test := range data.Completions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.Completion(t, src, test, data.CompletionItems)
-			})
+
+		for src, exp := range cases {
+			for i, e := range exp {
+				t.Run(spanName(src)+"_"+strconv.Itoa(i), func(t *testing.T) {
+					t.Helper()
+					test(t, src, e, data.CompletionItems)
+				})
+			}
+
 		}
+	}
+
+	t.Run("Completion", func(t *testing.T) {
+		t.Helper()
+		eachCompletion(t, data.Completions, tests.Completion)
 	})
 
 	t.Run("CompletionSnippets", func(t *testing.T) {
 		t.Helper()
 		for _, placeholders := range []bool{true, false} {
-			for src, expected := range data.CompletionSnippets {
-				name := spanName(src)
-				if placeholders {
-					name += "_placeholders"
+			for src, expecteds := range data.CompletionSnippets {
+				for i, expected := range expecteds {
+					name := spanName(src) + "_" + strconv.Itoa(i+1)
+					if placeholders {
+						name += "_placeholders"
+					}
+
+					t.Run(name, func(t *testing.T) {
+						t.Helper()
+						tests.CompletionSnippet(t, src, expected, placeholders, data.CompletionItems)
+					})
 				}
-				t.Run(name, func(t *testing.T) {
-					t.Helper()
-					tests.CompletionSnippet(t, src, expected, placeholders, data.CompletionItems)
-				})
 			}
 		}
 	})
 
 	t.Run("UnimportedCompletion", func(t *testing.T) {
 		t.Helper()
-		for src, test := range data.UnimportedCompletions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.UnimportedCompletion(t, src, test, data.CompletionItems)
-			})
-		}
+		eachCompletion(t, data.UnimportedCompletions, tests.UnimportedCompletion)
 	})
 
 	t.Run("DeepCompletion", func(t *testing.T) {
 		t.Helper()
-		for src, test := range data.DeepCompletions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.DeepCompletion(t, src, test, data.CompletionItems)
-			})
-		}
+		eachCompletion(t, data.DeepCompletions, tests.DeepCompletion)
 	})
 
 	t.Run("FuzzyCompletion", func(t *testing.T) {
 		t.Helper()
-		for src, test := range data.FuzzyCompletions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.FuzzyCompletion(t, src, test, data.CompletionItems)
-			})
-		}
+		eachCompletion(t, data.FuzzyCompletions, tests.FuzzyCompletion)
 	})
 
 	t.Run("CaseSensitiveCompletion", func(t *testing.T) {
 		t.Helper()
-		for src, test := range data.CaseSensitiveCompletions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.CaseSensitiveCompletion(t, src, test, data.CompletionItems)
-			})
-		}
+		eachCompletion(t, data.CaseSensitiveCompletions, tests.CaseSensitiveCompletion)
 	})
 
 	t.Run("RankCompletions", func(t *testing.T) {
 		t.Helper()
-		for src, test := range data.RankCompletions {
-			t.Run(spanName(src), func(t *testing.T) {
-				t.Helper()
-				tests.RankCompletion(t, src, test, data.CompletionItems)
-			})
-		}
+		eachCompletion(t, data.RankCompletions, tests.RankCompletion)
 	})
 
 	t.Run("Diagnostics", func(t *testing.T) {
@@ -594,13 +582,25 @@ func checkData(t *testing.T, data *Data) {
 		}
 	}
 
-	fmt.Fprintf(buf, "CompletionsCount = %v\n", len(data.Completions))
-	fmt.Fprintf(buf, "CompletionSnippetCount = %v\n", len(data.CompletionSnippets))
-	fmt.Fprintf(buf, "UnimportedCompletionsCount = %v\n", len(data.UnimportedCompletions))
-	fmt.Fprintf(buf, "DeepCompletionsCount = %v\n", len(data.DeepCompletions))
-	fmt.Fprintf(buf, "FuzzyCompletionsCount = %v\n", len(data.FuzzyCompletions))
-	fmt.Fprintf(buf, "RankedCompletionsCount = %v\n", len(data.RankCompletions))
-	fmt.Fprintf(buf, "CaseSensitiveCompletionsCount = %v\n", len(data.CaseSensitiveCompletions))
+	snippetCount := 0
+	for _, want := range data.CompletionSnippets {
+		snippetCount += len(want)
+	}
+
+	countCompletions := func(c map[span.Span][]Completion) (count int) {
+		for _, want := range c {
+			count += len(want)
+		}
+		return count
+	}
+
+	fmt.Fprintf(buf, "CompletionsCount = %v\n", countCompletions(data.Completions))
+	fmt.Fprintf(buf, "CompletionSnippetCount = %v\n", snippetCount)
+	fmt.Fprintf(buf, "UnimportedCompletionsCount = %v\n", countCompletions(data.UnimportedCompletions))
+	fmt.Fprintf(buf, "DeepCompletionsCount = %v\n", countCompletions(data.DeepCompletions))
+	fmt.Fprintf(buf, "FuzzyCompletionsCount = %v\n", countCompletions(data.FuzzyCompletions))
+	fmt.Fprintf(buf, "RankedCompletionsCount = %v\n", countCompletions(data.RankCompletions))
+	fmt.Fprintf(buf, "CaseSensitiveCompletionsCount = %v\n", countCompletions(data.CaseSensitiveCompletions))
 	fmt.Fprintf(buf, "DiagnosticsCount = %v\n", diagnosticsCount)
 	fmt.Fprintf(buf, "FoldingRangesCount = %v\n", len(data.FoldingRanges))
 	fmt.Fprintf(buf, "FormatCount = %v\n", len(data.Formats))
@@ -723,10 +723,10 @@ func (data *Data) collectDiagnostics(spn span.Span, msgSource, msg string) {
 }
 
 func (data *Data) collectCompletions(typ CompletionTestType) func(span.Span, []token.Pos) {
-	result := func(m map[span.Span]Completion, src span.Span, expected []token.Pos) {
-		m[src] = Completion{
+	result := func(m map[span.Span][]Completion, src span.Span, expected []token.Pos) {
+		m[src] = append(m[src], Completion{
 			CompletionItems: expected,
-		}
+		})
 	}
 	switch typ {
 	case CompletionDeep:
@@ -896,11 +896,11 @@ func (data *Data) collectSignatures(spn span.Span, signature string, activeParam
 }
 
 func (data *Data) collectCompletionSnippets(spn span.Span, item token.Pos, plain, placeholder string) {
-	data.CompletionSnippets[spn] = CompletionSnippet{
+	data.CompletionSnippets[spn] = append(data.CompletionSnippets[spn], CompletionSnippet{
 		CompletionItem:     item,
 		PlainSnippet:       plain,
 		PlaceholderSnippet: placeholder,
-	}
+	})
 }
 
 func (data *Data) collectLinks(spn span.Span, link string, note *expect.Note, fset *token.FileSet) {