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>
diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go
index 6fdffa1..664e075 100644
--- a/internal/lsp/completion_test.go
+++ b/internal/lsp/completion_test.go
@@ -49,7 +49,7 @@
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 @@
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)
}
}
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index 82230c9..de26587 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -1462,8 +1462,8 @@
return false
}
- objType := cand.obj.Type()
- if objType == nil {
+ candType := cand.obj.Type()
+ if candType == nil {
return true
}
@@ -1472,17 +1472,20 @@
// 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 @@
// 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 @@
}
}
- // 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
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 3ed53a5..2e2b0fd 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -138,7 +138,7 @@
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 @@
}
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)
}
}
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index 4e71385..68df3fb 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -330,6 +330,13 @@
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 {
diff --git a/internal/lsp/testdata/deep/deep.go b/internal/lsp/testdata/deep/deep.go
index df70c14..1bedc94 100644
--- a/internal/lsp/testdata/deep/deep.go
+++ b/internal/lsp/testdata/deep/deep.go
@@ -29,7 +29,7 @@
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 _() {
diff --git a/internal/lsp/testdata/rank/convert_rank.go.in b/internal/lsp/testdata/rank/convert_rank.go.in
index ac17416..77e2d27 100644
--- a/internal/lsp/testdata/rank/convert_rank.go.in
+++ b/internal/lsp/testdata/rank/convert_rank.go.in
@@ -30,7 +30,7 @@
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
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index 19e9ad3..682605b 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -4,7 +4,7 @@
UnimportedCompletionsCount = 3
DeepCompletionsCount = 5
FuzzyCompletionsCount = 7
-RankedCompletionsCount = 22
+RankedCompletionsCount = 26
CaseSensitiveCompletionsCount = 4
DiagnosticsCount = 22
FoldingRangesCount = 2
diff --git a/internal/lsp/testdata/variadic/variadic.go.in b/internal/lsp/testdata/variadic/variadic.go.in
index 76e3a56..8a7fa24 100644
--- a/internal/lsp/testdata/variadic/variadic.go.in
+++ b/internal/lsp/testdata/variadic/variadic.go.in
@@ -13,10 +13,10 @@
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")
diff --git a/internal/lsp/testdata/variadic/variadic_intf.go b/internal/lsp/testdata/variadic/variadic_intf.go
new file mode 100644
index 0000000..6e23fc9
--- /dev/null
+++ b/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)
+}
diff --git a/internal/lsp/tests/completion.go b/internal/lsp/tests/completion.go
index 8936e50..ba5ec70 100644
--- a/internal/lsp/tests/completion.go
+++ b/internal/lsp/tests/completion.go
@@ -4,6 +4,7 @@
"bytes"
"fmt"
"sort"
+ "strconv"
"strings"
"golang.org/x/tools/internal/lsp/protocol"
@@ -28,6 +29,8 @@
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 @@
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 @@
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 @@
}
if !inOrder {
- return summarizeCompletionItems(-1, want, matched, "completions out of order")
+ return summarizeCompletionItems(-1, want, matched, errorMsg)
}
return ""
