From cf2d1e09c8456c17308b4e86852cabb97ec0b384 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Sun, 1 Mar 2020 14:33:21 -0800 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/completion_test.go | 9 +- internal/lsp/source/completion.go | 33 +++- internal/lsp/source/completion_format.go | 1 + internal/lsp/source/completion_statements.go | 141 ++++++++++++++++++ internal/lsp/source/options.go | 8 +- internal/lsp/source/source_test.go | 8 +- .../lsp/primarymod/statements/append.go | 42 ++++++ internal/lsp/testdata/lsp/summary.txt.golden | 4 +- 8 files changed, 224 insertions(+), 22 deletions(-) create mode 100644 internal/lsp/testdata/lsp/primarymod/statements/append.go diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go index 04dc35e841..340dd1cc25 100644 --- a/internal/lsp/completion_test.go +++ b/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 + opts.InsertTextFormat = protocol.SnippetTextFormat + if !strings.Contains(string(src.URI()), "literal") { + opts.LiteralCompletions = false } - }) got = tests.FilterBuiltins(src, got) want := expected(t, test, items) diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index fe3ba16e17..d9f11b5e12 100644 --- a/internal/lsp/source/completion.go +++ b/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 - } - if err := c.lexical(ctx); err != 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) + } + 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 } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 2535432ace..a36bebd098 100644 --- a/internal/lsp/source/completion_format.go +++ b/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 { diff --git a/internal/lsp/source/completion_statements.go b/internal/lsp/source/completion_statements.go index 780616850a..8f72c379ce 100644 --- a/internal/lsp/source/completion_statements.go +++ b/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: diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 655bbf994b..fd19ad0e78 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -119,7 +119,8 @@ func DefaultOptions() Options { }, }, DebuggingOptions: DebuggingOptions{ - CompletionBudget: 100 * time.Millisecond, + 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 diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 4a0e9dde07..b0e50d5a57 100644 --- a/internal/lsp/source/source_test.go +++ b/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 + opts.InsertTextFormat = protocol.SnippetTextFormat + if !strings.Contains(string(src.URI()), "literal") { + opts.LiteralCompletions = false } }) got = tests.FilterBuiltins(src, got) diff --git a/internal/lsp/testdata/lsp/primarymod/statements/append.go b/internal/lsp/testdata/lsp/primarymod/statements/append.go new file mode 100644 index 0000000000..0eea85a282 --- /dev/null +++ b/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:})") +} diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 56d30df3ac..47c1e7b5b9 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/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