internal/lsp/source: offer smart "append()" completions

Assigning a slice to the appendage of itself is common and tedious
enough to warrant a special case completion candidate. We now offer
smarter "append()" candidates:

    var foo []int
    foo = app<> // offer "append(foo, <>)"
    fo<> // offer "foo = append(foo, <>)"

The latter is only offered if the best completion candidate is a
slice. It is inserted as the second-best candidate because it seems
impossible to avoid annoying false positives if it is ranked first.

I added a new debug option to disable literal completions. This was to
clean up some test logic that was disabling snippets for all tests
just to defeat literal completions. My tests were failing mysteriously
due to having snippets disabled, and it was hard to figure out why.

Change-Id: I3e8313e00a1409840cb99d5d71c593435a7aeb71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221777
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/completion_test.go

@@ -15,13 +15,10 @@ func (r *runner) Completion(t *testing.T, src span.Span, test tests.Completion,
 		opts.DeepCompletion = false
 		opts.Matcher = source.CaseInsensitive
 		opts.UnimportedCompletion = false
-		opts.InsertTextFormat = protocol.PlainTextTextFormat
-		// Only enable literal completions if in the completion literals tests.
-		// TODO(rstambler): Separate out literal completion tests.
-		if strings.Contains(string(src.URI()), "literal") {
 		opts.InsertTextFormat = protocol.SnippetTextFormat
+		if !strings.Contains(string(src.URI()), "literal") {
+			opts.LiteralCompletions = false
 		}
-
 	})
 	got = tests.FilterBuiltins(src, got)
 	want := expected(t, test, items)

internal/lsp/source/completion.go

@@ -78,6 +78,10 @@ type CompletionItem struct {
 
 	// Documentation is the documentation for the completion item.
 	Documentation string
+
+	// obj is the object from which this candidate was derived, if any.
+	// obj is for internal use only.
+	obj types.Object
 }
 
 // Snippet is a convenience returns the snippet if available, otherwise
@@ -501,7 +505,7 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, protoPos
 			documentation:     opts.CompletionDocumentation,
 			fullDocumentation: opts.HoverKind == FullDocumentation,
 			placeholders:      opts.Placeholders,
-			literal:           opts.InsertTextFormat == protocol.SnippetTextFormat,
+			literal:           opts.LiteralCompletions && opts.InsertTextFormat == protocol.SnippetTextFormat,
 			budget:            opts.CompletionBudget,
 		},
 		// default to a matcher that always matches
@@ -553,10 +557,6 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, protoPos
 		return c.items, c.getSurrounding(), nil
 	}
 
-	// Statement candidates offer an entire statement in certain
-	// contexts, as opposed to a single object.
-	c.addStatementCandidates()
-
 	if c.emptySwitchStmt() {
 		// Empty switch statements only admit "default" and "case" keywords.
 		c.addKeywordItems(map[string]bool{}, highScore, CASE, DEFAULT)
@@ -570,9 +570,20 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, protoPos
 			if err := c.selector(ctx, sel); err != nil {
 				return nil, nil, err
 			}
-			return c.items, c.getSurrounding(), nil
+		} else if obj, ok := pkg.GetTypesInfo().Defs[n]; ok {
+			// reject defining identifiers
+
+			if v, ok := obj.(*types.Var); ok && v.IsField() && v.Embedded() {
+				// An anonymous field is also a reference to a type.
+			} else {
+				objStr := ""
+				if obj != nil {
+					qual := types.RelativeTo(pkg.GetTypes())
+					objStr = types.ObjectString(obj, qual)
 				}
-		if err := c.lexical(ctx); err != nil {
+				return nil, nil, ErrIsDefinition{objStr: objStr}
+			}
+		} else if err := c.lexical(ctx); err != nil {
 			return nil, nil, err
 		}
 	// The function name hasn't been typed yet, but the parens are there:
@@ -599,6 +610,12 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, protoPos
 		}
 	}
 
+	// Statement candidates offer an entire statement in certain
+	// contexts, as opposed to a single object. Add statement candidates
+	// last because they depend on other candidates having already been
+	// collected.
+	c.addStatementCandidates()
+
 	return c.items, c.getSurrounding(), nil
 }
 

internal/lsp/source/completion_format.go

@@ -166,6 +166,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e
 		Score:               cand.score,
 		Depth:               len(c.deepState.chain),
 		snippet:             snip,
+		obj:                 obj,
 	}
 	// If the user doesn't want documentation for completion items.
 	if !c.opts.documentation {

internal/lsp/source/completion_statements.go

@@ -18,6 +18,147 @@ import (
 // appropriate for the current context.
 func (c *completer) addStatementCandidates() {
 	c.addErrCheckAndReturn()
+	c.addAssignAppend()
+}
+
+// addAssignAppend offers a completion candidate of the form:
+//
+//     someSlice = append(someSlice, )
+//
+// It will offer the "append" completion in two situations:
+//
+// 1. Position is in RHS of assign, prefix matches "append", and
+//    corresponding LHS object is a slice. For example,
+//    "foo = ap<>" completes to "foo = append(foo, )".
+//
+// Or
+//
+// 2. Prefix is an ident or selector in an *ast.ExprStmt (i.e.
+//    beginning of statement), and our best matching candidate is a
+//    slice. For example: "foo.ba" completes to "foo.bar = append(foo.bar, )".
+func (c *completer) addAssignAppend() {
+	if len(c.path) < 3 {
+		return
+	}
+
+	ident, _ := c.path[0].(*ast.Ident)
+	if ident == nil {
+		return
+	}
+
+	var (
+		// sliceText is the full name of our slice object, e.g. "s.abc" in
+		// "s.abc = app<>".
+		sliceText string
+		// needsLHS is true if we need to prepend the LHS slice name and
+		// "=" to our candidate.
+		needsLHS = false
+		fset     = c.snapshot.View().Session().Cache().FileSet()
+	)
+
+	switch n := c.path[1].(type) {
+	case *ast.AssignStmt:
+		// We are already in an assignment. Make sure our prefix matches "append".
+		if c.matcher.Score("append") <= 0 {
+			return
+		}
+
+		exprIdx := exprAtPos(c.pos, n.Rhs)
+		if exprIdx == len(n.Rhs) || exprIdx > len(n.Lhs)-1 {
+			return
+		}
+
+		lhsType := c.pkg.GetTypesInfo().TypeOf(n.Lhs[exprIdx])
+		if lhsType == nil {
+			return
+		}
+
+		// Make sure our corresponding LHS object is a slice.
+		if _, isSlice := lhsType.Underlying().(*types.Slice); !isSlice {
+			return
+		}
+
+		// The name or our slice is whatever's in the LHS expression.
+		sliceText = formatNode(fset, n.Lhs[exprIdx])
+	case *ast.SelectorExpr:
+		// Make sure we are a selector at the beginning of a statement.
+		if _, parentIsExprtStmt := c.path[2].(*ast.ExprStmt); !parentIsExprtStmt {
+			return
+		}
+
+		// So far we only know the first part of our slice name. For
+		// example in "s.a<>" we only know our slice begins with "s."
+		// since the user could still be typing.
+		sliceText = formatNode(fset, n.X) + "."
+		needsLHS = true
+	case *ast.ExprStmt:
+		needsLHS = true
+	default:
+		return
+	}
+
+	var (
+		label string
+		snip  snippet.Builder
+		score = highScore
+	)
+
+	if needsLHS {
+		// Offer the long form assign + append candidate if our best
+		// candidate is a slice.
+		bestItem := c.topCandidate()
+		if bestItem == nil || bestItem.obj == nil || bestItem.obj.Type() == nil {
+			return
+		}
+
+		if _, isSlice := bestItem.obj.Type().Underlying().(*types.Slice); !isSlice {
+			return
+		}
+
+		// Don't rank the full form assign + append candidate above the
+		// slice itself.
+		score = bestItem.Score - 0.01
+
+		// Fill in rest of sliceText now that we have the object name.
+		sliceText += bestItem.Label
+
+		// Fill in the candidate's LHS bits.
+		label = fmt.Sprintf("%s = ", bestItem.Label)
+		snip.WriteText(label)
+	}
+
+	snip.WriteText(fmt.Sprintf("append(%s, ", sliceText))
+	snip.WritePlaceholder(nil)
+	snip.WriteText(")")
+
+	c.items = append(c.items, CompletionItem{
+		Label:   label + fmt.Sprintf("append(%s, )", sliceText),
+		Kind:    protocol.FunctionCompletion,
+		Score:   score,
+		snippet: &snip,
+	})
+}
+
+// topCandidate returns the strictly highest scoring candidate
+// collected so far. If the top two candidates have the same score,
+// nil is returned.
+func (c *completer) topCandidate() *CompletionItem {
+	var bestItem, secondBestItem *CompletionItem
+	for i := range c.items {
+		if bestItem == nil || c.items[i].Score > bestItem.Score {
+			bestItem = &c.items[i]
+		} else if secondBestItem == nil || c.items[i].Score > secondBestItem.Score {
+			secondBestItem = &c.items[i]
+		}
+	}
+
+	// If secondBestItem has the same score, bestItem isn't
+	// the strict best.
+	if secondBestItem != nil && secondBestItem.Score == bestItem.Score {
+		return nil
+	}
+
+	return bestItem
 }
 
 // addErrCheckAndReturn offers a completion candidate of the form:

internal/lsp/source/options.go

@@ -120,6 +120,7 @@ func DefaultOptions() Options {
 		},
 		DebuggingOptions: DebuggingOptions{
 			CompletionBudget:   100 * time.Millisecond,
+			LiteralCompletions: true,
 		},
 		ExperimentalOptions: ExperimentalOptions{
 			TempModfile: true,
@@ -272,6 +273,11 @@ type DebuggingOptions struct {
 	// dynamically reduce the search scope to ensure we return timely
 	// results. Zero means unlimited.
 	CompletionBudget time.Duration
+
+	// LiteralCompletions controls whether literal candidates such as
+	// "&someStruct{}" are offered. Tests disable this flag to simplify
+	// their expected values.
+	LiteralCompletions bool
 }
 
 type Matcher int

internal/lsp/source/source_test.go

@@ -117,11 +117,9 @@ func (r *runner) Completion(t *testing.T, src span.Span, test tests.Completion,
 		opts.Matcher = source.CaseInsensitive
 		opts.DeepCompletion = false
 		opts.UnimportedCompletion = false
-		opts.InsertTextFormat = protocol.PlainTextTextFormat
-		// Only enable literal completions if in the completion literals tests.
-		// TODO(rstambler): Separate out literal completion tests.
-		if strings.Contains(string(src.URI()), "literal") {
 		opts.InsertTextFormat = protocol.SnippetTextFormat
+		if !strings.Contains(string(src.URI()), "literal") {
+			opts.LiteralCompletions = false
 		}
 	})
 	got = tests.FilterBuiltins(src, got)

internal/lsp/testdata/lsp/primarymod/statements/append.go

@@ -0,0 +1,42 @@
+package statements
+
+func _() {
+	type mySlice []int
+
+	var (
+		abc    []int   //@item(stmtABC, "abc", "[]int", "var")
+		abcdef mySlice //@item(stmtABCDEF, "abcdef", "mySlice", "var")
+	)
+
+	/* abcdef = append(abcdef, ) */ //@item(stmtABCDEFAssignAppend, "abcdef = append(abcdef, )", "", "func")
+
+	// don't offer "abc = append(abc, )" because "abc" isn't necessarily
+	// better than "abcdef".
+	abc //@complete(" //", stmtABC, stmtABCDEF)
+
+	abcdef //@complete(" //", stmtABCDEF, stmtABCDEFAssignAppend)
+
+	/* append(abc, ) */ //@item(stmtABCAppend, "append(abc, )", "", "func")
+
+	abc = app //@snippet(" //", stmtABCAppend, "append(abc, ${1:})", "append(abc, ${1:})")
+}
+
+func _() {
+	var s struct{ xyz []int }
+
+	/* xyz = append(s.xyz, ) */ //@item(stmtXYZAppend, "xyz = append(s.xyz, )", "", "func")
+
+	s.x //@snippet(" //", stmtXYZAppend, "xyz = append(s.xyz, ${1:})", "xyz = append(s.xyz, ${1:})")
+
+	/* s.xyz = append(s.xyz, ) */ //@item(stmtDeepXYZAppend, "s.xyz = append(s.xyz, )", "", "func")
+
+	sx //@snippet(" //", stmtDeepXYZAppend, "s.xyz = append(s.xyz, ${1:})", "s.xyz = append(s.xyz, ${1:})")
+}
+
+func _() {
+	var foo [][]int
+
+	/* append(foo[0], ) */ //@item(stmtFooAppend, "append(foo[0], )", "", "func")
+
+	foo[0] = app //@complete(" //"),snippet(" //", stmtFooAppend, "append(foo[0], ${1:})", "append(foo[0], ${1:})")
+}

internal/lsp/testdata/lsp/summary.txt.golden

@@ -1,7 +1,7 @@
 -- summary --
 CodeLensCount = 4
-CompletionsCount = 241
-CompletionSnippetCount = 76
+CompletionsCount = 244
+CompletionSnippetCount = 80
 UnimportedCompletionsCount = 6
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8