From b5b87423c93b800cdce5437496596722281c3cb8 Mon Sep 17 00:00:00 2001 From: Danish Dua Date: Thu, 1 Oct 2020 15:03:59 -0400 Subject: [PATCH] internal/lsp/source: refactor c.item to support deepSearch in all cases This change eliminates any special scenarios where we need to call c.item instead of going through deepSearch by adding support for all the cases in deepSearch and c.addItem (previously c.item). Change-Id: Ifb250be54da2f8c7b656475fcafaa38a4e306244 Reviewed-on: https://go-review.googlesource.com/c/tools/+/258858 Run-TryBot: Danish Dua gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Heschi Kreinick Trust: Danish Dua --- internal/lsp/completion.go | 2 +- internal/lsp/source/completion/completion.go | 135 ++++++++---------- .../source/completion/completion_format.go | 36 +++-- .../source/completion/completion_package.go | 8 +- .../lsp/source/completion/deep_completion.go | 20 +-- internal/lsp/source/signature_help.go | 5 +- internal/lsp/source/source_test.go | 2 +- internal/lsp/source/types_format.go | 4 +- .../comment_completion.go.in | 6 +- 9 files changed, 100 insertions(+), 118 deletions(-) diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 9ce8dca0f4..9b55f2ff12 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -28,7 +28,7 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara var surrounding *completion.Selection switch fh.Kind() { case source.Go: - candidates, surrounding, err = completion.Completion(ctx, snapshot, fh, params.Position, params.Context.TriggerCharacter) + candidates, surrounding, err = completion.Completion(ctx, snapshot, fh, params.Position, params.Context) case source.Mod: candidates, surrounding = nil, nil } diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go index 71403748b9..7e932eb4f7 100644 --- a/internal/lsp/source/completion/completion.go +++ b/internal/lsp/source/completion/completion.go @@ -155,8 +155,9 @@ type completer struct { qf types.Qualifier opts *completionOptions - // triggerCharacter is the character that triggered this request, if any. - triggerCharacter string + // completionContext contains information about the trigger for this + // completion request. + completionContext completionContext // fh is a handle to the file associated with this completion request. fh source.FileHandle @@ -259,6 +260,21 @@ type methodSetKey struct { addressable bool } +type completionContext struct { + // triggerCharacter is the character used to trigger completion at current + // position, if any. + triggerCharacter string + + // triggerKind is information about how a completion was triggered. + triggerKind protocol.CompletionTriggerKind + + // commentCompletion is true if we are completing a comment. + commentCompletion bool + + // packageCompletion is true if we are completing a package name. + packageCompletion bool +} + // A Selection represents the cursor position and surrounding identifier. type Selection struct { content string @@ -337,6 +353,10 @@ type candidate struct { // name is the deep object name path, e.g. "foo.bar" name string + // detail is additional information about this item. If not specified, + // defaults to type string for the object. + detail string + // path holds the path from the search root (excluding the candidate // itself) for a deep candidate. path []types.Object @@ -394,7 +414,7 @@ func (e ErrIsDefinition) Error() string { // The selection is computed based on the preceding identifier and can be used by // the client to score the quality of the completion. For instance, some clients // may tolerate imperfect matches as valid completion results, since users may make typos. -func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, protoPos protocol.Position, triggerCharacter string) ([]CompletionItem, *Selection, error) { +func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, protoPos protocol.Position, protoContext protocol.CompletionContext) ([]CompletionItem, *Selection, error) { ctx, done := event.Start(ctx, "completion.Completion") defer done() @@ -469,10 +489,13 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan opts := snapshot.View().Options() c := &completer{ - pkg: pkg, - snapshot: snapshot, - qf: source.Qualifier(pgf.File, pkg.GetTypes(), pkg.GetTypesInfo()), - triggerCharacter: triggerCharacter, + pkg: pkg, + snapshot: snapshot, + qf: source.Qualifier(pgf.File, pkg.GetTypes(), pkg.GetTypesInfo()), + completionContext: completionContext{ + triggerCharacter: protoContext.TriggerCharacter, + triggerKind: protoContext.TriggerKind, + }, fh: fh, filename: fh.URI().Filename(), file: pgf.File, @@ -556,9 +579,6 @@ func (c *completer) collectCompletions(ctx context.Context) error { // Inside comments, offer completions for the name of the relevant symbol. for _, comment := range c.file.Comments { if comment.Pos() < c.pos && c.pos <= comment.End() { - // Deep completion doesn't work properly in comments since we don't - // have a type object to complete further. - c.deepState.enabled = false c.populateCommentCompletions(ctx, comment) return nil } @@ -703,6 +723,9 @@ func (c *completer) emptySwitchStmt() bool { // (i.e. "golang.org/x/"). The user is meant to accept completion suggestions // until they reach a complete import path. func (c *completer) populateImportCompletions(ctx context.Context, searchImport *ast.ImportSpec) error { + // deepSearch is not valuable for import completions. + c.deepState.enabled = false + importPath := searchImport.Path.Value // Extract the text between the quotes (if any) in an import spec. @@ -804,15 +827,13 @@ func (c *completer) populateImportCompletions(ctx context.Context, searchImport mu.Lock() defer mu.Unlock() - obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkgToConsider, pkg.IdentName)) - cand := candidate{obj: obj, name: namePrefix + name + nameSuffix, score: score} - // We use c.item here to be able to manually update the detail for a - // candidate. deepSearch doesn't give us access to the completion item, - // so we don't enqueue the item here. - if item, err := c.item(ctx, cand); err == nil { - item.Detail = fmt.Sprintf("%q", pkgToConsider) - c.items = append(c.items, item) - } + name = namePrefix + name + nameSuffix + obj := types.NewPkgName(0, nil, name, types.NewPackage(pkgToConsider, name)) + c.deepState.enqueue(candidate{ + obj: obj, + detail: fmt.Sprintf("%q", pkgToConsider), + score: score, + }) } c.completionCallbacks = append(c.completionCallbacks, func(opts *imports.Options) error { @@ -825,7 +846,7 @@ func (c *completer) populateImportCompletions(ctx context.Context, searchImport func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast.CommentGroup) { // If the completion was triggered by a period, ignore it. These types of // completions will not be useful in comments. - if c.triggerCharacter == "." { + if c.completionContext.triggerCharacter == "." { return } @@ -835,6 +856,15 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast return } + // Deep completion doesn't work properly in comments since we don't + // have a type object to complete further. + c.deepState.enabled = false + c.completionContext.commentCompletion = true + + // Documentation isn't useful in comments, since it might end up being the + // comment itself. + c.opts.documentation = false + commentLine := file.Line(comment.End()) // comment is valid, set surrounding as word boundaries around cursor @@ -859,9 +889,6 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast continue } obj := c.pkg.GetTypesInfo().ObjectOf(name) - if matchScore := c.matcher.Score(name.String()); matchScore <= 0 { - continue - } c.deepState.enqueue(candidate{obj: obj, score: stdScore}) } case *ast.TypeSpec: @@ -881,9 +908,6 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast } obj := c.pkg.GetTypesInfo().ObjectOf(spec.Name) - if matchScore := c.matcher.Score(obj.Name()); matchScore <= 0 { - continue - } // Type name should get a higher score than fields but not highScore by default // since field near a comment cursor gets a highScore score := stdScore * 1.1 @@ -892,13 +916,7 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast score = highScore } - // we use c.item in addFieldItems so we have to use c.item - // here to ensure scoring order is maintained. deepSearch - // manipulates the score so we can't enqueue the item - // directly. - if item, err := c.item(ctx, candidate{obj: obj, name: obj.Name(), score: score}); err == nil { - c.items = append(c.items, item) - } + c.deepState.enqueue(candidate{obj: obj, score: score}) } } // handle functions @@ -926,18 +944,7 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast } for i := 0; i < recvStruct.NumFields(); i++ { field := recvStruct.Field(i) - // we use c.item in addFieldItems so we have to - // use c.item here to ensure scoring order is - // maintained. deepSearch manipulates the score so - // we can't enqueue the items directly. - if matchScore := c.matcher.Score(field.Name()); matchScore <= 0 { - continue - } - item, err := c.item(ctx, candidate{obj: field, name: field.Name(), score: lowScore}) - if err != nil { - continue - } - c.items = append(c.items, item) + c.deepState.enqueue(candidate{obj: field, score: lowScore}) } } } @@ -952,21 +959,7 @@ func (c *completer) populateCommentCompletions(ctx context.Context, comment *ast continue } - if matchScore := c.matcher.Score(obj.Name()); matchScore <= 0 { - continue - } - // We don't want to expandFuncCall inside comments. deepSearch - // doesn't respect this setting so we don't enqueue the item here. - item, err := c.item(ctx, candidate{ - obj: obj, - name: obj.Name(), - expandFuncCall: false, - score: highScore, - }) - if err != nil { - continue - } - c.items = append(c.items, item) + c.deepState.enqueue(candidate{obj: obj, score: highScore}) } } } @@ -1028,10 +1021,6 @@ func (c *completer) addFieldItems(ctx context.Context, fields *ast.FieldList) { continue } - if matchScore := c.matcher.Score(obj.Name()); matchScore <= 0 { - continue - } - // if we're in a field comment/doc, score that field as more relevant score := stdScore if field.Comment != nil && field.Comment.Pos() <= cursor && cursor <= field.Comment.End() { @@ -1040,17 +1029,7 @@ func (c *completer) addFieldItems(ctx context.Context, fields *ast.FieldList) { score = highScore } - cand := candidate{ - obj: obj, - name: obj.Name(), - expandFuncCall: false, - score: score, - } - // We don't want to expandFuncCall inside comments. deepSearch - // doesn't respect this setting so we don't enqueue the item here. - if item, err := c.item(ctx, cand); err == nil { - c.items = append(c.items, item) - } + c.deepState.enqueue(candidate{obj: obj, score: score}) } } } @@ -1065,7 +1044,7 @@ func (c *completer) wantStructFieldCompletions() bool { } func (c *completer) wantTypeName() bool { - return c.inference.typeName.wantTypeName + return !c.completionContext.commentCompletion && c.inference.typeName.wantTypeName } // See https://golang.org/issue/36001. Unimported completions are expensive. @@ -2435,6 +2414,10 @@ func (c *candidate) anyCandType(f func(t types.Type, addressable bool) bool) boo // matchingCandidate reports whether cand matches our type inferences. // It mutates cand's score in certain cases. func (c *completer) matchingCandidate(cand *candidate) bool { + if c.completionContext.commentCompletion { + return false + } + if isTypeName(cand.obj) { return c.matchingTypeName(cand) } else if c.wantTypeName() { diff --git a/internal/lsp/source/completion/completion_format.go b/internal/lsp/source/completion/completion_format.go index 9549f87c17..878bebaeec 100644 --- a/internal/lsp/source/completion/completion_format.go +++ b/internal/lsp/source/completion/completion_format.go @@ -17,12 +17,25 @@ import ( "golang.org/x/tools/internal/lsp/snippet" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" + errors "golang.org/x/xerrors" ) -// formatCompletion creates a completion item for a given candidate. +// item formats a candidate to a CompletionItem. func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, error) { obj := cand.obj + // if the object isn't a valid match against the surrounding, return early. + matchScore := c.matcher.Score(cand.name) + if matchScore <= 0 { + return CompletionItem{}, errors.New("not a surrounding match") + } + cand.score *= float64(matchScore) + + // Ignore deep candidates that wont be in the MaxDeepCompletions anyway. + if len(cand.path) != 0 && !c.deepState.isHighScore(cand.score) { + return CompletionItem{}, errors.New("not a high scoring candidate") + } + // Handle builtin types separately. if obj.Parent() == types.Universe { return c.formatBuiltin(ctx, cand) @@ -42,14 +55,10 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e // expandFuncCall mutates the completion label, detail, and snippet // to that of an invocation of sig. - expandFuncCall := func(sig *types.Signature) error { - s, err := source.NewSignature(ctx, c.snapshot, c.pkg, c.file, "", sig, nil, c.qf) - if err != nil { - return err - } + expandFuncCall := func(sig *types.Signature) { + s := source.NewSignature(ctx, c.snapshot, c.pkg, c.file, "", sig, nil, c.qf) snip = c.functionCallSnippet(label, s.Params()) detail = "func" + s.Format() - return nil } switch obj := obj.(type) { @@ -74,9 +83,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e } if sig, ok := obj.Type().Underlying().(*types.Signature); ok && cand.expandFuncCall { - if err := expandFuncCall(sig); err != nil { - return CompletionItem{}, err - } + expandFuncCall(sig) } case *types.Func: sig, ok := obj.Type().Underlying().(*types.Signature) @@ -89,9 +96,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e } if cand.expandFuncCall { - if err := expandFuncCall(sig); err != nil { - return CompletionItem{}, err - } + expandFuncCall(sig) } case *types.PkgName: kind = protocol.ModuleCompletion @@ -153,6 +158,10 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e } detail = strings.TrimPrefix(detail, "untyped ") + // override computed detail with provided detail, if something is provided. + if cand.detail != "" { + detail = cand.detail + } item := CompletionItem{ Label: label, InsertText: insert, @@ -207,6 +216,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e if c.opts.fullDocumentation { item.Documentation = hover.FullDocumentation } + return item, nil } diff --git a/internal/lsp/source/completion/completion_package.go b/internal/lsp/source/completion/completion_package.go index f8d3431da2..a6d9febe41 100644 --- a/internal/lsp/source/completion/completion_package.go +++ b/internal/lsp/source/completion/completion_package.go @@ -189,6 +189,8 @@ func (c *completer) packageNameCompletions(ctx context.Context, fileURI span.URI return errors.New("cursor is not in package name identifier") } + c.completionContext.packageCompletion = true + prefix := name.Name[:cursor] packageSuggestions, err := packageSuggestions(ctx, c.snapshot, fileURI, prefix) if err != nil { @@ -196,9 +198,7 @@ func (c *completer) packageNameCompletions(ctx context.Context, fileURI span.URI } for _, pkg := range packageSuggestions { - if item, err := c.item(ctx, pkg); err == nil { - c.items = append(c.items, item) - } + c.deepState.enqueue(pkg) } return nil } @@ -220,7 +220,7 @@ func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI s toCandidate := func(name string, score float64) candidate { obj := types.NewPkgName(0, nil, name, types.NewPackage("", name)) - return candidate{obj: obj, name: name, score: score} + return candidate{obj: obj, name: name, detail: name, score: score} } matcher := fuzzy.NewMatcher(prefix) diff --git a/internal/lsp/source/completion/deep_completion.go b/internal/lsp/source/completion/deep_completion.go index da8c117e1d..61b29c3447 100644 --- a/internal/lsp/source/completion/deep_completion.go +++ b/internal/lsp/source/completion/deep_completion.go @@ -130,8 +130,10 @@ outer: } // If obj is not accessible because it lives in another package and is - // not exported, don't treat it as a completion candidate. - if obj.Pkg() != nil && obj.Pkg() != c.pkg.GetTypes() && !obj.Exported() { + // not exported, don't treat it as a completion candidate unless it's + // a package completion candidate. + if !c.completionContext.packageCompletion && + obj.Pkg() != nil && obj.Pkg() != c.pkg.GetTypes() && !obj.Exported() { continue } @@ -262,19 +264,9 @@ func (c *completer) addCandidate(ctx context.Context, cand *candidate) { } cand.name = strings.Join(append(cand.names, cand.obj.Name()), ".") - matchScore := c.matcher.Score(cand.name) - if matchScore > 0 { - cand.score *= float64(matchScore) - - // Avoid calling c.item() for deep candidates that wouldn't be in the top - // MaxDeepCompletions anyway. - if len(cand.path) == 0 || c.deepState.isHighScore(cand.score) { - if item, err := c.item(ctx, *cand); err == nil { - c.items = append(c.items, item) - } - } + if item, err := c.item(ctx, *cand); err == nil { + c.items = append(c.items, item) } - } // penalty reports a score penalty for cand in the range (0, 1). diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 890d8e0feb..ed0252cfaf 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -116,10 +116,7 @@ FindCall: } else { name = "func" } - s, err := NewSignature(ctx, snapshot, pkg, pgf.File, name, sig, comment, qf) - if err != nil { - return nil, 0, err - } + s := NewSignature(ctx, snapshot, pkg, pgf.File, name, sig, comment, qf) paramInfo := make([]protocol.ParameterInformation, 0, len(s.params)) for _, p := range s.params { paramInfo = append(paramInfo, protocol.ParameterInformation{Label: p}) diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 27c4a1d97c..38fdb3133e 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -301,7 +301,7 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*sourc list, surrounding, err := completion.Completion(r.ctx, r.snapshot, fh, protocol.Position{ Line: float64(src.Start().Line() - 1), Character: float64(src.Start().Column() - 1), - }, "") + }, protocol.CompletionContext{}) if err != nil && !errors.As(err, &completion.ErrIsDefinition{}) { t.Fatalf("failed for %v: %v", src, err) } diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go index 07784c1673..9da19c8189 100644 --- a/internal/lsp/source/types_format.go +++ b/internal/lsp/source/types_format.go @@ -160,7 +160,7 @@ func formatFieldList(ctx context.Context, snapshot Snapshot, list *ast.FieldList } // NewSignature returns formatted signature for a types.Signature struct. -func NewSignature(ctx context.Context, s Snapshot, pkg Package, file *ast.File, name string, sig *types.Signature, comment *ast.CommentGroup, qf types.Qualifier) (*signature, error) { +func NewSignature(ctx context.Context, s Snapshot, pkg Package, file *ast.File, name string, sig *types.Signature, comment *ast.CommentGroup, qf types.Qualifier) *signature { params := make([]string, 0, sig.Params().Len()) for i := 0; i < sig.Params().Len(); i++ { el := sig.Params().At(i) @@ -198,7 +198,7 @@ func NewSignature(ctx context.Context, s Snapshot, pkg Package, file *ast.File, results: results, variadic: sig.Variadic(), needResultParens: needResultParens, - }, nil + } } // FormatVarType formats a *types.Var, accounting for type aliases. diff --git a/internal/lsp/testdata/comment_completion/comment_completion.go.in b/internal/lsp/testdata/comment_completion/comment_completion.go.in index e48b581310..dbca0ff175 100644 --- a/internal/lsp/testdata/comment_completion/comment_completion.go.in +++ b/internal/lsp/testdata/comment_completion/comment_completion.go.in @@ -32,16 +32,16 @@ var C string //@item(variableC, "C", "string", "var") //@complete(" ", variableC // //@complete(" ", constant) const Constant = "example" //@item(constant, "Constant", "string", "const") //@complete(" ", constant) -// //@complete(" ", structType, fieldA, fieldB) +// //@complete(" ", structType, fieldB, fieldA) type StructType struct { //@item(structType, "StructType", "struct{...}", "struct") //@complete(" ", structType, fieldA, fieldB) // //@complete(" ", fieldA, structType, fieldB) A string //@item(fieldA, "A", "string", "field") //@complete(" ", fieldA, structType, fieldB) b int //@item(fieldB, "b", "int", "field") //@complete(" ", fieldB, structType, fieldA) } -// //@complete(" ", method, paramX, resultY, structRecv, fieldA, fieldB) +// //@complete(" ", method, structRecv, paramX, resultY, fieldB, fieldA) func (structType *StructType) Method(X int) (Y int) { //@item(structRecv, "structType", "*StructType", "var"),item(method, "Method", "func(X int) (Y int)", "method"),item(paramX, "X", "int", "var"),item(resultY, "Y", "int", "var") - // //@complete(" ", method, paramX, resultY, structRecv, fieldA, fieldB) + // //@complete(" ", method, structRecv, paramX, resultY, fieldB, fieldA) return }