internal/lsp: fix variadic interface completion ranking

In cases like:

var foo []io.Writer
var buf *bytes.Buffer
foo = append(foo, <>)

we weren't giving "buf" a good score. When comparing the candidate
type *bytes.Buffer to the (variadic) expected type []io.Writer we were
turning the candidate type into []*bytes.Buffer. However, of course,
[]*bytes.Buffer is not assignable to []io.Writer, so the types didn't
match. Now we instead turn the expected type []io.Writer into
io.Writer and compare to *bytes.Buffer.

I fixed the @rank test note to check that the candidates' scores are
strictly decreasing. Previously it would allow candidates with the
same score if they happened to be in the right order. This made it
easier to right a test for this issue, but also uncovered an issue
with untyped completion logic. I fixed it to do the untyped constant
check if _either_ the expected or candidate type is
untyped (previously it required the candidate type to be untyped).

Fixes golang/go#35625.

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

internal/lsp/completion_test.go

@@ -49,7 +49,7 @@ func (r *runner) UnimportedCompletion(t *testing.T, src span.Span, test tests.Co
 		got = tests.FilterBuiltins(got)
 	}
 	want := expected(t, test, items)
-	if diff := tests.CheckCompletionOrder(want, got); diff != "" {
+	if diff := tests.CheckCompletionOrder(want, got, false); diff != "" {
 		t.Errorf("%s: %s", src, diff)
 	}
 }
@@ -101,7 +101,7 @@ func (r *runner) RankCompletion(t *testing.T, src span.Span, test tests.Completi
 		Deep:          true,
 	})
 	want := expected(t, test, items)
-	if msg := tests.CheckCompletionOrder(want, got); msg != "" {
+	if msg := tests.CheckCompletionOrder(want, got, true); msg != "" {
 		t.Errorf("%s: %s", src, msg)
 	}
 }

internal/lsp/source/completion.go

@@ -1462,8 +1462,8 @@ func (c *completer) matchingCandidate(cand *candidate) bool {
 		return false
 	}
 
-	objType := cand.obj.Type()
-	if objType == nil {
+	candType := cand.obj.Type()
+	if candType == nil {
 		return true
 	}
 
@@ -1472,17 +1472,20 @@ func (c *completer) matchingCandidate(cand *candidate) bool {
 	// are invoked by default.
 	cand.expandFuncCall = isFunc(cand.obj)
 
-	typeMatches := func(candType types.Type) bool {
+	typeMatches := func(expType, candType types.Type) bool {
+		if expType == nil {
+			return false
+		}
+
 		// Take into account any type modifiers on the expected type.
 		candType = c.expectedType.applyTypeModifiers(candType)
 
-		if c.expectedType.objType != nil {
-			wantType := types.Default(c.expectedType.objType)
-
-			// Handle untyped values specially since AssignableTo gives false negatives
-			// for them (see https://golang.org/issue/32146).
-			if candBasic, ok := candType.(*types.Basic); ok && candBasic.Info()&types.IsUntyped > 0 {
-				if wantBasic, ok := wantType.Underlying().(*types.Basic); ok {
+		// Handle untyped values specially since AssignableTo gives false negatives
+		// for them (see https://golang.org/issue/32146).
+		if candBasic, ok := candType.Underlying().(*types.Basic); ok {
+			if wantBasic, ok := expType.Underlying().(*types.Basic); ok {
+				// Make sure at least one of them is untyped.
+				if isUntyped(candType) || isUntyped(expType) {
 					// Check that their constant kind (bool|int|float|complex|string) matches.
 					// This doesn't take into account the constant value, so there will be some
 					// false positives due to integer sign and overflow.
@@ -1490,32 +1493,29 @@ func (c *completer) matchingCandidate(cand *candidate) bool {
 						// Lower candidate score if the types are not identical.
 						// This avoids ranking untyped integer constants above
 						// candidates with an exact type match.
-						if !types.Identical(candType, c.expectedType.objType) {
+						if !types.Identical(candType, expType) {
 							cand.score /= 2
 						}
 						return true
 					}
 				}
-				return false
 			}
-
-			// AssignableTo covers the case where the types are equal, but also handles
-			// cases like assigning a concrete type to an interface type.
-			return types.AssignableTo(candType, wantType)
 		}
 
-		return false
+		// AssignableTo covers the case where the types are equal, but also handles
+		// cases like assigning a concrete type to an interface type.
+		return types.AssignableTo(candType, expType)
 	}
 
-	if typeMatches(objType) {
+	if typeMatches(c.expectedType.objType, candType) {
 		// If obj's type matches, we don't want to expand to an invocation of obj.
 		cand.expandFuncCall = false
 		return true
 	}
 
 	// Try using a function's return type as its type.
-	if sig, ok := objType.Underlying().(*types.Signature); ok && sig.Results().Len() == 1 {
-		if typeMatches(sig.Results().At(0).Type()) {
+	if sig, ok := candType.Underlying().(*types.Signature); ok && sig.Results().Len() == 1 {
+		if typeMatches(c.expectedType.objType, sig.Results().At(0).Type()) {
 			// If obj's return value matches the expected type, we need to invoke obj
 			// in the completion.
 			cand.expandFuncCall = true
@@ -1523,15 +1523,16 @@ 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
+	// When completing the variadic parameter, if the expected type is
+	// []T then check candType against T.
+	if c.expectedType.variadic {
+		if slice, ok := c.expectedType.objType.(*types.Slice); ok && typeMatches(slice.Elem(), candType) {
+			return true
+		}
 	}
 
 	if c.expectedType.convertibleTo != nil {
-		return types.ConvertibleTo(objType, c.expectedType.convertibleTo)
+		return types.ConvertibleTo(candType, c.expectedType.convertibleTo)
 	}
 
 	return false

internal/lsp/source/source_test.go

@@ -138,7 +138,7 @@ func (r *runner) UnimportedCompletion(t *testing.T, src span.Span, test tests.Co
 	if !strings.Contains(string(src.URI()), "builtins") {
 		got = tests.FilterBuiltins(got)
 	}
-	if diff := tests.CheckCompletionOrder(want, got); diff != "" {
+	if diff := tests.CheckCompletionOrder(want, got, false); diff != "" {
 		t.Errorf("%s: %s", src, diff)
 	}
 }
@@ -229,7 +229,7 @@ func (r *runner) RankCompletion(t *testing.T, src span.Span, test tests.Completi
 		}
 		got = append(got, item)
 	}
-	if msg := tests.CheckCompletionOrder(want, got); msg != "" {
+	if msg := tests.CheckCompletionOrder(want, got, true); msg != "" {
 		t.Errorf("%s: %s", src, msg)
 	}
 }

internal/lsp/source/util.go

@@ -330,6 +330,13 @@ func isEmptyInterface(T types.Type) bool {
 	return intf != nil && intf.NumMethods() == 0
 }
 
+func isUntyped(T types.Type) bool {
+	if basic, ok := T.(*types.Basic); ok {
+		return basic.Info()&types.IsUntyped > 0
+	}
+	return false
+}
+
 // isSelector returns the enclosing *ast.SelectorExpr when pos is in the
 // selector.
 func enclosingSelector(path []ast.Node, pos token.Pos) *ast.SelectorExpr {

internal/lsp/testdata/deep/deep.go

@@ -29,7 +29,7 @@ func _() {
 	context.Background() //@item(ctxBackground, "context.Background", "func() context.Context", "func", "Background returns a non-nil, empty Context.")
 	context.TODO()       //@item(ctxTODO, "context.TODO", "func() context.Context", "func", "TODO returns a non-nil, empty Context.")
 
-	wantsContext(c) //@rank(")", ctxBackground, ctxTODO)
+	wantsContext(c) //@rank(")", ctxBackground),rank(")", ctxTODO)
 }
 
 func _() {

internal/lsp/testdata/rank/convert_rank.go.in

@@ -30,7 +30,7 @@ func _() {
 	mi = conv //@rank(" //", convertG, convertD, convertE)
 	mi + conv //@rank(" //", convertG, convertD, convertE)
 
-	1 + conv //@rank(" //", convertD, convertE, convertG)
+	1 + conv //@rank(" //", convertD, convertC),rank(" //", convertE, convertC),rank(" //", convertG, convertC)
 
 	type myString string
 	var ms myString

internal/lsp/testdata/summary.txt.golden

@@ -4,7 +4,7 @@ CompletionSnippetCount = 47
 UnimportedCompletionsCount = 3
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 7
-RankedCompletionsCount = 22
+RankedCompletionsCount = 26
 CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 22
 FoldingRangesCount = 2

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

@@ -13,10 +13,10 @@ func _() {
 		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)
+	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, s, "") //@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/testdata/variadic/variadic_intf.go

@@ -0,0 +1,21 @@
+package variadic
+
+type baz interface {
+	baz()
+}
+
+func wantsBaz(...baz) {}
+
+type bazImpl int
+
+func (bazImpl) baz() {}
+
+func _() {
+	var (
+		impls []bazImpl //@item(vImplSlice, "impls", "[]bazImpl", "var")
+		impl  bazImpl   //@item(vImpl, "impl", "bazImpl", "var")
+		bazes []baz     //@item(vIntfSlice, "bazes", "[]baz", "var")
+	)
+
+	wantsBaz() //@rank(")", vImpl, vImplSlice),rank(")", vIntfSlice, vImplSlice)
+}

internal/lsp/tests/completion.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"sort"
+	"strconv"
 	"strings"
 
 	"golang.org/x/tools/internal/lsp/protocol"
@@ -28,6 +29,8 @@ func ToProtocolCompletionItem(item source.CompletionItem) protocol.CompletionIte
 		TextEdit: &protocol.TextEdit{
 			NewText: item.Snippet(),
 		},
+		// Negate score so best score has lowest sort text like real API.
+		SortText: fmt.Sprint(-item.Score),
 	}
 	if pItem.InsertText == "" {
 		pItem.InsertText = pItem.Label
@@ -64,11 +67,13 @@ func isBuiltin(label, detail string, kind protocol.CompletionItemKind) bool {
 	return false
 }
 
-func CheckCompletionOrder(want, got []protocol.CompletionItem) string {
+func CheckCompletionOrder(want, got []protocol.CompletionItem, strictScores bool) string {
 	var (
 		matchedIdxs []int
 		lastGotIdx  int
+		lastGotSort float64
 		inOrder     = true
+		errorMsg    = "completions out of order"
 	)
 	for _, w := range want {
 		var found bool
@@ -76,10 +81,19 @@ func CheckCompletionOrder(want, got []protocol.CompletionItem) string {
 			if w.Label == g.Label && w.Detail == g.Detail && w.Kind == g.Kind {
 				matchedIdxs = append(matchedIdxs, i)
 				found = true
+
 				if i < lastGotIdx {
 					inOrder = false
 				}
 				lastGotIdx = i
+
+				sort, _ := strconv.ParseFloat(g.SortText, 64)
+				if strictScores && len(matchedIdxs) > 1 && sort <= lastGotSort {
+					inOrder = false
+					errorMsg = "candidate scores not strictly decreasing"
+				}
+				lastGotSort = sort
+
 				break
 			}
 		}
@@ -95,7 +109,7 @@ func CheckCompletionOrder(want, got []protocol.CompletionItem) string {
 	}
 
 	if !inOrder {
-		return summarizeCompletionItems(-1, want, matched, "completions out of order")
+		return summarizeCompletionItems(-1, want, matched, errorMsg)
 	}
 
 	return ""