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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

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
 	}

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
 	}

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,
+				Label:         info.Label,
-				Parameters: toProtocolParameterInformation(info.Parameters),
+				Documentation: info.Documentation,
+				Parameters:    toProtocolParameterInformation(info.Parameters),
 			},
 		},
 	}

internal/lsp/source/hover.go

@@ -15,52 +15,55 @@ import (
 	"strings"
 )
 
-// formatter returns the a hover value formatted with its documentation.
+type documentation struct {
-type formatter func(interface{}, *ast.CommentGroup) (string, error)
+	source  interface{}
+	comment *ast.CommentGroup
+}
 
-func (i *IdentifierInfo) Hover(ctx context.Context, qf types.Qualifier, markdownSupported, wantComments bool) (string, error) {
+func (i *IdentifierInfo) Hover(ctx context.Context, markdownSupported, wantComments bool) (string, error) {
-	file := i.File.GetAST(ctx)
+	h, err := i.decl.hover(ctx)
-	if qf == nil {
+	if err != nil {
-		pkg := i.File.GetPackage(ctx)
+		return "", err
-		qf = qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo())
+	}
+	c := h.comment
+	if !wantComments {
+		c = nil
 	}
 	var b strings.Builder
-	f := func(x interface{}, c *ast.CommentGroup) (string, error) {
+	return writeHover(h.source, i.File.FileSet(), &b, c, markdownSupported, i.qf)
-		if !wantComments {
+}
-			c = nil
+
-		}
+func (d declaration) hover(ctx context.Context) (*documentation, error) {
-		return writeHover(x, i.File.FileSet(), &b, c, markdownSupported, qf)
+	obj := d.obj
-	}
+	switch node := d.node.(type) {
-	obj := i.Declaration.Object
-	switch node := i.Declaration.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())
+}

internal/lsp/source/identifier.go

@@ -25,14 +25,21 @@ type IdentifierInfo struct {
 		Range  span.Range
 		Object types.Object
 	}
-	Declaration struct {
+	decl declaration
-		Range  span.Range
-		Node   ast.Node
-		Object types.Object
-	}
 
 	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)
+	result.decl.obj = pkg.GetTypesInfo().ObjectOf(result.ident)
-	if result.Declaration.Object == nil {
+	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
+		result.decl.node = decl
-		if result.Declaration.Range, err = posToRange(ctx, f.FileSet(), result.Name, decl.Pos()); err != nil {
+		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

internal/lsp/source/signature_help.go

@@ -15,9 +15,9 @@ import (
 )
 
 type SignatureInformation struct {
-	Label           string
+	Label, Documentation string
-	Parameters      []ParameterInformation
+	Parameters           []ParameterInformation
-	ActiveParameter int
+	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,
 	}

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