From 6fbe43b70d4c56895a8c82c66820be1b62182760 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Fri, 7 Aug 2020 22:50:58 -0700 Subject: [PATCH] internal/lsp/source: improve completion in append() In the following example: var foo []someStruct foo = append(foo, <>) we now downrank "foo" as a candidate at "<>". You very rarely append a slice to itself, so having "foo" ranked highly was counterproductive. Fixes golang/go#40535. Change-Id: Ic01366aeded4ba2b6b64bfddd33415499b35a323 Reviewed-on: https://go-review.googlesource.com/c/tools/+/247519 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/completion_test.go | 1 + internal/lsp/source/completion.go | 51 ++++++++++--------- internal/lsp/source/completion_builtin.go | 17 +++++-- .../testdata/lsp/primarymod/append/append.go | 11 +++- internal/lsp/testdata/lsp/summary.txt.golden | 2 +- 5 files changed, 52 insertions(+), 30 deletions(-) diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go index 340dd1cc25..c8992f567b 100644 --- a/internal/lsp/completion_test.go +++ b/internal/lsp/completion_test.go @@ -96,6 +96,7 @@ func (r *runner) RankCompletion(t *testing.T, src span.Span, test tests.Completi opts.DeepCompletion = true opts.Matcher = source.Fuzzy opts.UnimportedCompletion = false + opts.LiteralCompletions = true }) want := expected(t, test, items) if msg := tests.CheckCompletionOrder(want, got, true); msg != "" { diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 64b63ec971..bcf6c28619 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -331,15 +331,8 @@ func (c *completer) found(ctx context.Context, cand candidate) { if c.matchingCandidate(&cand) { cand.score *= highScore - if c.seenInSwitchCase(&cand) { - // If this candidate matches an expression already used in a - // different case clause, downrank. We only downrank a little - // because the user could write something like: - // - // switch foo { - // case bar: - // case ba<>: // they may want "bar|baz" or "bar.baz()" - cand.score *= 0.9 + if p := c.penalty(&cand); p > 0 { + cand.score *= (1 - p) } } else if isTypeName(obj) { // If obj is a *types.TypeName that didn't otherwise match, check @@ -387,16 +380,17 @@ func (c *completer) found(ctx context.Context, cand candidate) { c.deepSearch(ctx, cand) } -// seenInSwitchCase reports whether cand has already been seen in -// another switch case statement. -func (c *completer) seenInSwitchCase(cand *candidate) bool { - for _, chain := range c.inference.seenSwitchCases { - if c.objChainMatches(cand.obj, chain) { - return true +// penalty reports a score penalty for cand in the range (0, 1). +// For example, a candidate is penalized if it has already been used +// in another switch case statement. +func (c *completer) penalty(cand *candidate) float64 { + for _, p := range c.inference.penalized { + if c.objChainMatches(cand.obj, p.objChain) { + return p.penalty } } - return false + return 0 } // candidate represents a completion candidate. @@ -1556,6 +1550,16 @@ const ( kindFunc ) +// penalizedObj represents an object that should be disfavored as a +// completion candidate. +type penalizedObj struct { + // objChain is the full "chain", e.g. "foo.bar().baz" becomes + // []types.Object{foo, bar, baz}. + objChain []types.Object + // penalty is score penalty in the range (0, 1). + penalty float64 +} + // candidateInference holds information we have inferred about a type that can be // used at the current position. type candidateInference struct { @@ -1602,13 +1606,12 @@ type candidateInference struct { // foo(bar, baz<>) // variadicAssignees=false variadicAssignees bool - // seenSwitchCases tracks the expressions already used in the - // containing switch statement's cases. Each expression is tracked - // as a slice of objects. For example, "case foo.bar().baz:" is - // tracked as []types.Object{foo, bar, baz}. Tracking the entire - // "chain" allows us to differentiate "a.foo" and "b.foo" when "a" - // and "b" are the same type. - seenSwitchCases [][]types.Object + // penalized holds expressions that should be disfavored as + // candidates. For example, it tracks expressions already used in a + // switch statement's other cases. Each expression is tracked using + // its entire object "chain" allowing differentiation between + // "a.foo" and "b.foo" when "a" and "b" are the same type. + penalized []penalizedObj // objChain contains the chain of objects representing the // surrounding *ast.SelectorExpr. For example, if we are completing @@ -1798,7 +1801,7 @@ Nodes: } if objs := objChain(c.pkg.GetTypesInfo(), caseExpr); len(objs) > 0 { - inf.seenSwitchCases = append(inf.seenSwitchCases, objs) + inf.penalized = append(inf.penalized, penalizedObj{objChain: objs, penalty: 0.1}) } } } diff --git a/internal/lsp/source/completion_builtin.go b/internal/lsp/source/completion_builtin.go index 781cec3bba..cfb004a953 100644 --- a/internal/lsp/source/completion_builtin.go +++ b/internal/lsp/source/completion_builtin.go @@ -72,10 +72,19 @@ func (c *completer) builtinArgType(obj types.Object, call *ast.CallExpr, parentI inf.objType = parentInf.objType - if exprIdx > 0 { - inf.objType = deslice(inf.objType) - // Check if we are completing the variadic append() param. - inf.variadic = exprIdx == 1 && len(call.Args) <= 2 + if exprIdx <= 0 { + break + } + + inf.objType = deslice(inf.objType) + + // Check if we are completing the variadic append() param. + inf.variadic = exprIdx == 1 && len(call.Args) <= 2 + + // Penalize the first append() argument as a candidate. You + // don't normally append a slice to itself. + if sliceChain := objChain(c.pkg.GetTypesInfo(), call.Args[0]); len(sliceChain) > 0 { + inf.penalized = append(inf.penalized, penalizedObj{objChain: sliceChain, penalty: 0.9}) } case "delete": if exprIdx > 0 && len(call.Args) > 0 { diff --git a/internal/lsp/testdata/lsp/primarymod/append/append.go b/internal/lsp/testdata/lsp/primarymod/append/append.go index 68cae673cb..f8e64c57d3 100644 --- a/internal/lsp/testdata/lsp/primarymod/append/append.go +++ b/internal/lsp/testdata/lsp/primarymod/append/append.go @@ -16,5 +16,14 @@ func _() { // Don't add "..." to append() argument. bar(append()) //@snippet("))", appendStrings, "aStrings", "aStrings") -} + + type baz struct{} + baz{} //@item(appendBazLiteral, "baz{}", "", "var") + var bazzes []baz //@item(appendBazzes, "bazzes", "[]baz", "var") + var bazzy baz //@item(appendBazzy, "bazzy", "baz", "var") + bazzes = append(bazzes, ba) //@rank(")", appendBazzy, appendBazLiteral, appendBazzes) + + var b struct{ b []baz } + b.b //@item(appendNestedBaz, "b.b", "[]baz", "field") + b.b = append(b.b, b) //@rank(")", appendBazzy, appendBazLiteral, appendNestedBaz) } diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 2ea94502d5..a76174de1f 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/internal/lsp/testdata/lsp/summary.txt.golden @@ -6,7 +6,7 @@ CompletionSnippetCount = 83 UnimportedCompletionsCount = 6 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 -RankedCompletionsCount = 137 +RankedCompletionsCount = 139 CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 44 FoldingRangesCount = 2