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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

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 != "" {

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 p := c.penalty(&cand); p > 0 {
-			// If this candidate matches an expression already used in a
+			cand.score *= (1 - p)
-			// 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
 		}
 	} 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
+// penalty reports a score penalty for cand in the range (0, 1).
-// another switch case statement.
+// For example, a candidate is penalized if it has already been used
-func (c *completer) seenInSwitchCase(cand *candidate) bool {
+// in another switch case statement.
-	for _, chain := range c.inference.seenSwitchCases {
+func (c *completer) penalty(cand *candidate) float64 {
-		if c.objChainMatches(cand.obj, chain) {
+	for _, p := range c.inference.penalized {
-			return true
+		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
+	// penalized holds expressions that should be disfavored as
-	// containing switch statement's cases. Each expression is tracked
+	// candidates. For example, it tracks expressions already used in a
-	// as a slice of objects. For example, "case foo.bar().baz:" is
+	// switch statement's other cases. Each expression is tracked using
-	// tracked as []types.Object{foo, bar, baz}. Tracking the entire
+	// its entire object "chain" allowing differentiation between
-	// "chain" allows us to differentiate "a.foo" and "b.foo" when "a"
+	// "a.foo" and "b.foo" when "a" and "b" are the same type.
-	// and "b" are the same type.
+	penalized []penalizedObj
-	seenSwitchCases [][]types.Object
 
 	// 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})
 							}
 						}
 					}

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 {
+		if exprIdx <= 0 {
-			inf.objType = deslice(inf.objType)
+			break
-			// Check if we are completing the variadic append() param.
+		}
-			inf.variadic = exprIdx == 1 && len(call.Args) <= 2
+
+		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 {

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)
 }

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