From 0139d5756a7dfc6302f04af275a50cbc37ea0153 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 5 Jun 2019 01:16:23 -0400 Subject: [PATCH] internal/lsp: attach documentation to signature help This change refactors hover to generate documentation for just the declaration portion of an identifier. Updates golang/go#29151 Change-Id: I16d48a99b56c36132e49cc87e2736f85c88ed14a Reviewed-on: https://go-review.googlesource.com/c/tools/+/180657 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/definition.go | 2 +- internal/lsp/hover.go | 2 +- internal/lsp/signature_help.go | 5 +- internal/lsp/source/hover.go | 80 +++++++++++++++------------ internal/lsp/source/identifier.go | 42 ++++++++------ internal/lsp/source/signature_help.go | 36 +++++++++--- internal/lsp/source/source_test.go | 4 +- 7 files changed, 107 insertions(+), 64 deletions(-) diff --git a/internal/lsp/definition.go b/internal/lsp/definition.go index 1453aa80..d90385fd 100644 --- a/internal/lsp/definition.go +++ b/internal/lsp/definition.go @@ -31,7 +31,7 @@ func (s *Server) definition(ctx context.Context, params *protocol.TextDocumentPo if err != nil { return nil, err } - decSpan, err := ident.Declaration.Range.Span() + decSpan, err := ident.DeclarationRange().Span() if err != nil { return nil, err } diff --git a/internal/lsp/hover.go b/internal/lsp/hover.go index bdc295b1..44d24eef 100644 --- a/internal/lsp/hover.go +++ b/internal/lsp/hover.go @@ -32,7 +32,7 @@ func (s *Server) hover(ctx context.Context, params *protocol.TextDocumentPositio if err != nil { return nil, err } - hover, err := ident.Hover(ctx, nil, s.preferredContentFormat == protocol.Markdown, !s.noDocsOnHover) + hover, err := ident.Hover(ctx, s.preferredContentFormat == protocol.Markdown, !s.noDocsOnHover) if err != nil { return nil, err } diff --git a/internal/lsp/signature_help.go b/internal/lsp/signature_help.go index 20fe7b4a..7a31d819 100644 --- a/internal/lsp/signature_help.go +++ b/internal/lsp/signature_help.go @@ -43,8 +43,9 @@ func toProtocolSignatureHelp(info *source.SignatureInformation) *protocol.Signat ActiveSignature: 0, // there is only ever one possible signature Signatures: []protocol.SignatureInformation{ { - Label: info.Label, - Parameters: toProtocolParameterInformation(info.Parameters), + Label: info.Label, + Documentation: info.Documentation, + Parameters: toProtocolParameterInformation(info.Parameters), }, }, } diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index 4d00527a..5f098126 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -15,52 +15,55 @@ import ( "strings" ) -// formatter returns the a hover value formatted with its documentation. -type formatter func(interface{}, *ast.CommentGroup) (string, error) +type documentation struct { + source interface{} + comment *ast.CommentGroup +} -func (i *IdentifierInfo) Hover(ctx context.Context, qf types.Qualifier, markdownSupported, wantComments bool) (string, error) { - file := i.File.GetAST(ctx) - if qf == nil { - pkg := i.File.GetPackage(ctx) - qf = qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()) +func (i *IdentifierInfo) Hover(ctx context.Context, markdownSupported, wantComments bool) (string, error) { + h, err := i.decl.hover(ctx) + if err != nil { + return "", err + } + c := h.comment + if !wantComments { + c = nil } var b strings.Builder - f := func(x interface{}, c *ast.CommentGroup) (string, error) { - if !wantComments { - c = nil - } - return writeHover(x, i.File.FileSet(), &b, c, markdownSupported, qf) - } - obj := i.Declaration.Object - switch node := i.Declaration.Node.(type) { + return writeHover(h.source, i.File.FileSet(), &b, c, markdownSupported, i.qf) +} + +func (d declaration) hover(ctx context.Context) (*documentation, error) { + obj := d.obj + switch node := d.node.(type) { case *ast.GenDecl: switch obj := obj.(type) { case *types.TypeName, *types.Var, *types.Const, *types.Func: - return formatGenDecl(node, obj, obj.Type(), f) + return formatGenDecl(node, obj, obj.Type()) } case *ast.TypeSpec: if obj.Parent() == types.Universe { if obj.Name() == "error" { - return f(node, nil) + return &documentation{node, nil}, nil } - return f(node.Name, nil) // comments not needed for builtins + return &documentation{node.Name, nil}, nil // comments not needed for builtins } case *ast.FuncDecl: switch obj.(type) { case *types.Func: - return f(obj, node.Doc) + return &documentation{obj, node.Doc}, nil case *types.Builtin: - return f(node.Type, node.Doc) + return &documentation{node.Type, node.Doc}, nil } } - return f(obj, nil) + return &documentation{obj, nil}, nil } -func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type, f formatter) (string, error) { +func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type) (*documentation, error) { if _, ok := typ.(*types.Named); ok { switch typ.Underlying().(type) { case *types.Interface, *types.Struct: - return formatGenDecl(node, obj, typ.Underlying(), f) + return formatGenDecl(node, obj, typ.Underlying()) } } var spec ast.Spec @@ -71,31 +74,31 @@ func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type, f format } } if spec == nil { - return "", fmt.Errorf("no spec for node %v at position %v", node, obj.Pos()) + return nil, fmt.Errorf("no spec for node %v at position %v", node, obj.Pos()) } // If we have a field or method. switch obj.(type) { case *types.Var, *types.Const, *types.Func: - return formatVar(spec, obj, f) + return formatVar(spec, obj) } // Handle types. switch spec := spec.(type) { case *ast.TypeSpec: if len(node.Specs) > 1 { // If multiple types are declared in the same block. - return f(spec.Type, spec.Doc) + return &documentation{spec.Type, spec.Doc}, nil } else { - return f(spec, node.Doc) + return &documentation{spec, node.Doc}, nil } case *ast.ValueSpec: - return f(spec, spec.Doc) + return &documentation{spec, spec.Doc}, nil case *ast.ImportSpec: - return f(spec, spec.Doc) + return &documentation{spec, spec.Doc}, nil } - return "", fmt.Errorf("unable to format spec %v (%T)", spec, spec) + return nil, fmt.Errorf("unable to format spec %v (%T)", spec, spec) } -func formatVar(node ast.Spec, obj types.Object, f formatter) (string, error) { +func formatVar(node ast.Spec, obj types.Object) (*documentation, error) { var fieldList *ast.FieldList if spec, ok := node.(*ast.TypeSpec); ok { switch t := spec.Type.(type) { @@ -112,22 +115,22 @@ func formatVar(node ast.Spec, obj types.Object, f formatter) (string, error) { field := fieldList.List[i] if field.Pos() <= obj.Pos() && obj.Pos() <= field.End() { if field.Doc.Text() != "" { - return f(obj, field.Doc) + return &documentation{obj, field.Doc}, nil } else if field.Comment.Text() != "" { - return f(obj, field.Comment) + return &documentation{obj, field.Comment}, nil } } } } // If we weren't able to find documentation for the object. - return f(obj, nil) + return &documentation{obj, nil}, nil } // writeHover writes the hover for a given node and its documentation. func writeHover(x interface{}, fset *token.FileSet, b *strings.Builder, c *ast.CommentGroup, markdownSupported bool, qf types.Qualifier) (string, error) { if c != nil { // TODO(rstambler): Improve conversion from Go docs to markdown. - b.WriteString(doc.Synopsis(c.Text())) + b.WriteString(formatDocumentation(c)) b.WriteRune('\n') } if markdownSupported { @@ -146,3 +149,10 @@ func writeHover(x interface{}, fset *token.FileSet, b *strings.Builder, c *ast.C } return b.String(), nil } + +func formatDocumentation(c *ast.CommentGroup) string { + if c == nil { + return "" + } + return doc.Synopsis(c.Text()) +} diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 02df6b03..06c7e0df 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -25,14 +25,21 @@ type IdentifierInfo struct { Range span.Range Object types.Object } - Declaration struct { - Range span.Range - Node ast.Node - Object types.Object - } + decl declaration ident *ast.Ident wasEmbeddedField bool + qf types.Qualifier +} + +type declaration struct { + rng span.Range + node ast.Node + obj types.Object +} + +func (i *IdentifierInfo) DeclarationRange() span.Range { + return i.decl.rng } // Identifier returns identifier information for a position @@ -72,7 +79,10 @@ func identifier(ctx context.Context, v View, f GoFile, pos token.Pos) (*Identifi return result, err } - result := &IdentifierInfo{File: f} + result := &IdentifierInfo{ + File: f, + qf: qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()), + } switch node := path[0].(type) { case *ast.Ident: @@ -91,21 +101,21 @@ func identifier(ctx context.Context, v View, f GoFile, pos token.Pos) (*Identifi } result.Name = result.ident.Name result.Range = span.NewRange(f.FileSet(), result.ident.Pos(), result.ident.End()) - result.Declaration.Object = pkg.GetTypesInfo().ObjectOf(result.ident) - if result.Declaration.Object == nil { + result.decl.obj = pkg.GetTypesInfo().ObjectOf(result.ident) + if result.decl.obj == nil { return nil, fmt.Errorf("no object for ident %v", result.Name) } var err error // Handle builtins separately. - if result.Declaration.Object.Parent() == types.Universe { + if result.decl.obj.Parent() == types.Universe { decl, ok := lookupBuiltinDecl(f.View(), result.Name).(ast.Node) if !ok { return nil, fmt.Errorf("no declaration for %s", result.Name) } - result.Declaration.Node = decl - if result.Declaration.Range, err = posToRange(ctx, f.FileSet(), result.Name, decl.Pos()); err != nil { + result.decl.node = decl + if result.decl.rng, err = posToRange(ctx, f.FileSet(), result.Name, decl.Pos()); err != nil { return nil, err } return result, nil @@ -114,17 +124,17 @@ func identifier(ctx context.Context, v View, f GoFile, pos token.Pos) (*Identifi if result.wasEmbeddedField { // The original position was on the embedded field declaration, so we // try to dig out the type and jump to that instead. - if v, ok := result.Declaration.Object.(*types.Var); ok { + if v, ok := result.decl.obj.(*types.Var); ok { if typObj := typeToObject(v.Type()); typObj != nil { - result.Declaration.Object = typObj + result.decl.obj = typObj } } } - if result.Declaration.Range, err = objToRange(ctx, f.FileSet(), result.Declaration.Object); err != nil { + if result.decl.rng, err = objToRange(ctx, f.FileSet(), result.decl.obj); err != nil { return nil, err } - if result.Declaration.Node, err = objToNode(ctx, v, result.Declaration.Object, result.Declaration.Range); err != nil { + if result.decl.node, err = objToNode(ctx, v, result.decl.obj, result.decl.rng); err != nil { return nil, err } typ := pkg.GetTypesInfo().TypeOf(result.ident) @@ -245,7 +255,7 @@ func importSpec(f GoFile, fAST *ast.File, pkg Package, pos token.Pos) (*Identifi if dest == nil { return nil, fmt.Errorf("package %q has no files", importPath) } - result.Declaration.Range = span.NewRange(f.FileSet(), dest.Name.Pos(), dest.Name.End()) + result.decl.rng = span.NewRange(f.FileSet(), dest.Name.Pos(), dest.Name.End()) return result, nil } return nil, nil diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index e5214ec2..26d304ca 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -15,9 +15,9 @@ import ( ) type SignatureInformation struct { - Label string - Parameters []ParameterInformation - ActiveParameter int + Label, Documentation string + Parameters []ParameterInformation + ActiveParameter int } type ParameterInformation struct { @@ -82,13 +82,34 @@ func SignatureHelp(ctx context.Context, f GoFile, pos token.Pos) (*SignatureInfo results, writeResultParens := formatResults(sig.Results(), qf) activeParam := activeParameter(callExpr, sig.Params().Len(), sig.Variadic(), pos) - var name string + var ( + name string + comment *ast.CommentGroup + ) if obj != nil { + rng, err := objToRange(ctx, f.FileSet(), obj) + if err != nil { + return nil, err + } + node, err := objToNode(ctx, f.View(), obj, rng) + if err != nil { + return nil, err + } + decl := &declaration{ + obj: obj, + rng: rng, + node: node, + } + d, err := decl.hover(ctx) + if err != nil { + return nil, err + } name = obj.Name() + comment = d.comment } else { name = "func" } - return signatureInformation(name, params, results, writeResultParens, activeParam), nil + return signatureInformation(name, comment, params, results, writeResultParens, activeParam), nil } func builtinSignature(ctx context.Context, v View, callExpr *ast.CallExpr, name string, pos token.Pos) (*SignatureInformation, error) { @@ -111,10 +132,10 @@ func builtinSignature(ctx context.Context, v View, callExpr *ast.CallExpr, name } } activeParam := activeParameter(callExpr, numParams, variadic, pos) - return signatureInformation(name, params, results, writeResultParens, activeParam), nil + return signatureInformation(name, nil, params, results, writeResultParens, activeParam), nil } -func signatureInformation(name string, params, results []string, writeResultParens bool, activeParam int) *SignatureInformation { +func signatureInformation(name string, comment *ast.CommentGroup, params, results []string, writeResultParens bool, activeParam int) *SignatureInformation { paramInfo := make([]ParameterInformation, 0, len(params)) for _, p := range params { paramInfo = append(paramInfo, ParameterInformation{Label: p}) @@ -126,6 +147,7 @@ func signatureInformation(name string, params, results []string, writeResultPare } return &SignatureInformation{ Label: label, + Documentation: formatDocumentation(comment), Parameters: paramInfo, ActiveParameter: activeParam, } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 1837783f..8135e1db 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -362,11 +362,11 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) { if err != nil { t.Fatalf("failed for %v: %v", d.Src, err) } - hover, err := ident.Hover(ctx, nil, false, true) + hover, err := ident.Hover(ctx, false, true) if err != nil { t.Fatalf("failed for %v: %v", d.Src, err) } - rng := ident.Declaration.Range + rng := ident.DeclarationRange() if d.IsType { rng = ident.Type.Range hover = ""