From 5a76f03bc7c327212912ed6b2a76a10d7f39b224 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Fri, 15 Nov 2019 16:14:15 -0800 Subject: [PATCH] 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 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot --- internal/lsp/completion_test.go | 4 +- internal/lsp/source/completion.go | 53 ++++++++++--------- internal/lsp/source/source_test.go | 4 +- internal/lsp/source/util.go | 7 +++ internal/lsp/testdata/deep/deep.go | 2 +- internal/lsp/testdata/rank/convert_rank.go.in | 2 +- internal/lsp/testdata/summary.txt.golden | 2 +- internal/lsp/testdata/variadic/variadic.go.in | 8 +-- .../lsp/testdata/variadic/variadic_intf.go | 21 ++++++++ internal/lsp/tests/completion.go | 18 ++++++- 10 files changed, 82 insertions(+), 39 deletions(-) create mode 100644 internal/lsp/testdata/variadic/variadic_intf.go diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go index 6fdffa1fb1..664e075901 100644 --- a/internal/lsp/completion_test.go +++ b/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) } } diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 82230c9a0f..de26587c67 100644 --- a/internal/lsp/source/completion.go +++ b/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 diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 3ed53a50d0..2e2b0fd81c 100644 --- a/internal/lsp/source/source_test.go +++ b/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) } } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 4e7138592b..68df3fb7fb 100644 --- a/internal/lsp/source/util.go +++ b/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 { diff --git a/internal/lsp/testdata/deep/deep.go b/internal/lsp/testdata/deep/deep.go index df70c14725..1bedc9416c 100644 --- a/internal/lsp/testdata/deep/deep.go +++ b/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 _() { diff --git a/internal/lsp/testdata/rank/convert_rank.go.in b/internal/lsp/testdata/rank/convert_rank.go.in index ac17416260..77e2d27d0a 100644 --- a/internal/lsp/testdata/rank/convert_rank.go.in +++ b/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 diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 19e9ad33cd..682605b004 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/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 diff --git a/internal/lsp/testdata/variadic/variadic.go.in b/internal/lsp/testdata/variadic/variadic.go.in index 76e3a56655..8a7fa241fe 100644 --- a/internal/lsp/testdata/variadic/variadic.go.in +++ b/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") diff --git a/internal/lsp/testdata/variadic/variadic_intf.go b/internal/lsp/testdata/variadic/variadic_intf.go new file mode 100644 index 0000000000..6e23fc9960 --- /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 8936e506b0..ba5ec70f24 100644 --- a/internal/lsp/tests/completion.go +++ b/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 ""