internal/lsp: improve signatureHelp in various cases

- show signature for function calls whose function expression is not
  an object (e.g. the second call in foo()()). since the function name
  is not available, we use the generic "func"
- only provide signature help when the position is on or within the
  call expression parens. this is consistent with the one other lsp
  server i tried (java). this improves the gopls experience in emacs
  where lsp-mode is constantly calling "hover" and
  "signatureHelp" ("hover" should be preferred unless you are inside
  the function params list)
- use the entire signature type string as the label since that includes
  the return values, which are useful to see
- don't qualify the function name with its package. it looks funny to
  see "bytes.Cap()" as the help when you are in a call
  to (*bytes.Buffer).Cap(). it could be useful to include invocant
  type info, but leave it out for now since signature help is meant to
  focus on the function parameters.
- don't turn variadic args "foo ...int" into "foo []int" for the
  parameter information (i.e. maintain it as "foo ...int")
- when determining active parameter, count the space before a
  parameter name as being part of that parameter (e.g. the space
  before "b" in "func(a int, b int)")
- handle variadic params when determining the active param (i.e.
  highlight "foo(a int, *b ...string*)" on signature help for final
  param in `foo(123, "a", "b", "c")`
- don't generate an extra space in formatParams() for unnamed
  arguments

I also tweaked the signatureHelp server log message to include the
error message itself, and populated the server's logger in lsp_test.go
to aid in development.

Fixes golang/go#31448

Change-Id: Iefe0e1e3c531d17197c0fa997b949174475a276c
GitHub-Last-Rev: 5c0b8ebd87a8c05d5d8f519ea096f94e89c77e2c
GitHub-Pull-Request: golang/tools#82
Reviewed-on: https://go-review.googlesource.com/c/tools/+/172439
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/lsp_test.go

@@ -45,6 +45,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	const expectedTypeDefinitionsCount = 2
 	const expectedHighlightsCount = 2
 	const expectedSymbolsCount = 1
+	const expectedSignaturesCount = 19
 
 	files := packagestest.MustCopyFileTree(dir)
 	for fragment, operation := range files {
@@ -77,6 +78,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	s := &Server{
 		views:       []*cache.View{cache.NewView(ctx, log, "lsp_test", span.FileURI(cfg.Dir), &cfg)},
 		undelivered: make(map[span.URI][]source.Diagnostic),
+		log:         log,
 	}
 	// Do a first pass to collect special markers for completion.
 	if err := exported.Expect(map[string]interface{}{
@@ -98,6 +100,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 		m:        make(map[span.URI][]protocol.DocumentSymbol),
 		children: make(map[string][]protocol.DocumentSymbol),
 	}
+	expectedSignatures := make(signatures)
 
 	// Collect any data that needs to be used by subsequent tests.
 	if err := exported.Expect(map[string]interface{}{
@@ -109,6 +112,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 		"typdef":    expectedTypeDefinitions.collect,
 		"highlight": expectedHighlights.collect,
 		"symbol":    expectedSymbols.collect,
+		"signature": expectedSignatures.collect,
 	}); err != nil {
 		t.Fatal(err)
 	}
@@ -176,6 +180,14 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 		}
 		expectedSymbols.test(t, s)
 	})
+
+	t.Run("Signatures", func(t *testing.T) {
+		t.Helper()
+		if len(expectedSignatures) != expectedSignaturesCount {
+			t.Errorf("got %v signatures expected %v", len(expectedSignatures), expectedSignaturesCount)
+		}
+		expectedSignatures.test(t, s)
+	})
 }
 
 func addUnsavedFiles(t *testing.T, cfg *packages.Config, exported *packagestest.Exported) {
@@ -207,6 +219,7 @@ type symbols struct {
 	m        map[span.URI][]protocol.DocumentSymbol
 	children map[string][]protocol.DocumentSymbol
 }
+type signatures map[token.Position]*protocol.SignatureHelp
 
 func (d diagnostics) test(t *testing.T, v source.View) int {
 	count := 0
@@ -610,6 +623,72 @@ func (s symbols) test(t *testing.T, server *Server) {
 	}
 }
 
+func (s signatures) collect(src token.Position, signature string, activeParam int64) {
+	s[src] = &protocol.SignatureHelp{
+		Signatures:      []protocol.SignatureInformation{{Label: signature}},
+		ActiveSignature: 0,
+		ActiveParameter: float64(activeParam),
+	}
+}
+
+func diffSignatures(src token.Position, want, got *protocol.SignatureHelp) string {
+	decorate := func(f string, args ...interface{}) string {
+		return fmt.Sprintf("Invalid signature at %s: %s", src, fmt.Sprintf(f, args...))
+	}
+
+	if lw, lg := len(want.Signatures), len(got.Signatures); lw != lg {
+		return decorate("wanted %d signatures, got %d", lw, lg)
+	}
+
+	if want.ActiveSignature != got.ActiveSignature {
+		return decorate("wanted active signature of %f, got %f", want.ActiveSignature, got.ActiveSignature)
+	}
+
+	if want.ActiveParameter != got.ActiveParameter {
+		return decorate("wanted active parameter of %f, got %f", want.ActiveParameter, got.ActiveParameter)
+	}
+
+	for i := range want.Signatures {
+		wantSig, gotSig := want.Signatures[i], got.Signatures[i]
+
+		if wantSig.Label != gotSig.Label {
+			return decorate("wanted label %q, got %q", wantSig.Label, gotSig.Label)
+		}
+
+		var paramParts []string
+		for _, p := range gotSig.Parameters {
+			paramParts = append(paramParts, p.Label)
+		}
+		paramsStr := strings.Join(paramParts, ", ")
+		if !strings.Contains(gotSig.Label, paramsStr) {
+			return decorate("expected signature %q to contain params %q", gotSig.Label, paramsStr)
+		}
+	}
+
+	return ""
+}
+
+func (s signatures) test(t *testing.T, server *Server) {
+	for src, expectedSignatures := range s {
+		gotSignatures, err := server.SignatureHelp(context.Background(), &protocol.TextDocumentPositionParams{
+			TextDocument: protocol.TextDocumentIdentifier{
+				URI: protocol.NewURI(span.FileURI(src.Filename)),
+			},
+			Position: protocol.Position{
+				Line:      float64(src.Line - 1),
+				Character: float64(src.Column - 1),
+			},
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := diffSignatures(src, expectedSignatures, gotSignatures); diff != "" {
+			t.Error(diff)
+		}
+	}
+}
+
 func diffSymbols(uri span.URI, want, got []protocol.DocumentSymbol) string {
 	sort.Slice(want, func(i, j int) bool { return want[i].Name < want[j].Name })
 	sort.Slice(got, func(i, j int) bool { return got[i].Name < got[j].Name })

internal/lsp/server.go

@@ -395,7 +395,7 @@ func (s *Server) SignatureHelp(ctx context.Context, params *protocol.TextDocumen
 	}
 	info, err := source.SignatureHelp(ctx, f, rng.Start)
 	if err != nil {
-		s.log.Infof(ctx, "no signature help for %s:%v:%v", uri, int(params.Position.Line), int(params.Position.Character))
+		s.log.Infof(ctx, "no signature help for %s:%v:%v : %s", uri, int(params.Position.Line), int(params.Position.Character), err)
 	}
 	return toProtocolSignatureHelp(info), nil
 }

internal/lsp/source/completion.go

@@ -508,7 +508,11 @@ func formatParams(t *types.Tuple, variadic bool, qualifier types.Qualifier) stri
 		if variadic && i == t.Len()-1 {
 			typ = strings.Replace(typ, "[]", "...", 1)
 		}
-		fmt.Fprintf(&b, "%v %v", el.Name(), typ)
+		if el.Name() == "" {
+			fmt.Fprintf(&b, "%v", typ)
+		} else {
+			fmt.Fprintf(&b, "%v %v", el.Name(), typ)
+		}
 	}
 	b.WriteByte(')')
 	return b.String()

internal/lsp/source/signature_help.go

@@ -10,6 +10,7 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
+	"strings"
 
 	"golang.org/x/tools/go/ast/astutil"
 )
@@ -38,7 +39,7 @@ func SignatureHelp(ctx context.Context, f File, pos token.Pos) (*SignatureInform
 		return nil, fmt.Errorf("cannot find node enclosing position")
 	}
 	for _, node := range path {
-		if c, ok := node.(*ast.CallExpr); ok {
+		if c, ok := node.(*ast.CallExpr); ok && pos >= c.Lparen && pos <= c.Rparen {
 			callExpr = c
 			break
 		}
@@ -47,37 +48,25 @@ func SignatureHelp(ctx context.Context, f File, pos token.Pos) (*SignatureInform
 		return nil, fmt.Errorf("cannot find an enclosing function")
 	}
 
-	// Get the type information for the function corresponding to the call expression.
-	var obj types.Object
-	switch t := callExpr.Fun.(type) {
-	case *ast.Ident:
-		obj = pkg.GetTypesInfo().ObjectOf(t)
-	case *ast.SelectorExpr:
-		obj = pkg.GetTypesInfo().ObjectOf(t.Sel)
-	default:
-		return nil, fmt.Errorf("the enclosing function is malformed")
-	}
-	if obj == nil {
-		return nil, fmt.Errorf("cannot resolve %s", callExpr.Fun)
-	}
-	// Find the signature corresponding to the object.
-	var sig *types.Signature
-	switch obj.(type) {
-	case *types.Var:
-		if underlying, ok := obj.Type().Underlying().(*types.Signature); ok {
-			sig = underlying
-		}
-	case *types.Func:
-		sig = obj.Type().(*types.Signature)
+	// Get the type information for the function being called.
+	sigType := pkg.GetTypesInfo().TypeOf(callExpr.Fun)
+	if sigType == nil {
+		return nil, fmt.Errorf("cannot get type for Fun %[1]T (%[1]v)", callExpr.Fun)
 	}
+
+	sig, _ := sigType.Underlying().(*types.Signature)
 	if sig == nil {
-		return nil, fmt.Errorf("no function signatures found for %s", obj.Name())
+		return nil, fmt.Errorf("cannot find signature for Fun %[1]T (%[1]v)", callExpr.Fun)
 	}
+
 	pkgStringer := qualifier(fAST, pkg.GetTypes(), pkg.GetTypesInfo())
 	var paramInfo []ParameterInformation
 	for i := 0; i < sig.Params().Len(); i++ {
 		param := sig.Params().At(i)
 		label := types.TypeString(param.Type(), pkgStringer)
+		if sig.Variadic() && i == sig.Params().Len()-1 {
+			label = strings.Replace(label, "[]", "...", 1)
+		}
 		if param.Name() != "" {
 			label = fmt.Sprintf("%s %s", param.Name(), label)
 		}
@@ -85,30 +74,56 @@ func SignatureHelp(ctx context.Context, f File, pos token.Pos) (*SignatureInform
 			Label: label,
 		})
 	}
+
 	// Determine the query position relative to the number of parameters in the function.
 	var activeParam int
 	var start, end token.Pos
-	for i, expr := range callExpr.Args {
+	for _, expr := range callExpr.Args {
 		if start == token.NoPos {
 			start = expr.Pos()
 		}
 		end = expr.End()
-		if i < len(callExpr.Args)-1 {
-			end = callExpr.Args[i+1].Pos() - 1 // comma
-		}
 		if start <= pos && pos <= end {
 			break
 		}
-		activeParam++
+
+		// Don't advance the active parameter for the last parameter of a variadic function.
+		if !sig.Variadic() || activeParam < sig.Params().Len()-1 {
+			activeParam++
+		}
 		start = expr.Pos() + 1 // to account for commas
 	}
-	// Label for function, qualified by package name.
-	label := obj.Name()
-	if pkg := pkgStringer(obj.Pkg()); pkg != "" {
-		label = pkg + "." + label
+
+	// Get the object representing the function, if available.
+	// There is no object in certain cases such as calling a function returned by
+	// a function (e.g. "foo()()").
+	var obj types.Object
+	switch t := callExpr.Fun.(type) {
+	case *ast.Ident:
+		obj = pkg.GetTypesInfo().ObjectOf(t)
+	case *ast.SelectorExpr:
+		obj = pkg.GetTypesInfo().ObjectOf(t.Sel)
 	}
+
+	var label string
+	if obj != nil {
+		label = obj.Name()
+	} else {
+		label = "func"
+	}
+
+	label += formatParams(sig.Params(), sig.Variadic(), pkgStringer)
+	if sig.Results().Len() > 0 {
+		results := types.TypeString(sig.Results(), pkgStringer)
+		if sig.Results().Len() == 1 && sig.Results().At(0).Name() == "" {
+			// Trim off leading/trailing parens to avoid results like "foo(a int) (int)".
+			results = strings.Trim(results, "()")
+		}
+		label += " " + results
+	}
+
 	return &SignatureInformation{
-		Label:           label + formatParams(sig.Params(), sig.Variadic(), pkgStringer),
+		Label:           label,
 		Parameters:      paramInfo,
 		ActiveParameter: activeParam,
 	}, nil

internal/lsp/testdata/signature/signature.go

@@ -0,0 +1,61 @@
+package signature
+
+import (
+	"bytes"
+	"encoding/json"
+	"math/big"
+)
+
+func Foo(a string, b int) (c bool) {
+	return
+}
+
+func Bar(float64, ...byte) {
+}
+
+type myStruct struct{}
+
+func (*myStruct) foo(e *json.Decoder) (*big.Int, error) {
+	return nil, nil
+}
+
+type MyFunc func(foo int) string
+
+func Qux() {
+	Foo("foo", 123) //@signature("(", "Foo(a string, b int) (c bool)", 2)
+	Foo("foo", 123) //@signature("123", "Foo(a string, b int) (c bool)", 1)
+	Foo("foo", 123) //@signature(",", "Foo(a string, b int) (c bool)", 0)
+	Foo("foo", 123) //@signature(" 1", "Foo(a string, b int) (c bool)", 1)
+	Foo("foo", 123) //@signature(")", "Foo(a string, b int) (c bool)", 1)
+
+	Bar(13.37, 0x1337)     //@signature("13.37", "Bar(float64, ...byte)", 0)
+	Bar(13.37, 0x1337)     //@signature("0x1337", "Bar(float64, ...byte)", 1)
+	Bar(13.37, 1, 2, 3, 4) //@signature("4", "Bar(float64, ...byte)", 1)
+
+	fn := func(hi, there string) func(i int) rune {
+		return 0
+	}
+
+	fn("hi", "there")    //@signature("hi", "fn(hi string, there string) func(i int) rune", 0)
+	fn("hi", "there")(1) //@signature("1", "func(i int) rune", 0)
+
+	fnPtr := &fn
+	(*fnPtr)("hi", "there") //@signature("hi", "func(hi string, there string) func(i int) rune", 0)
+
+	var fnIntf interface{} = Foo
+	fnIntf.(func(string, int) bool)("hi", 123) //@signature("123", "func(string, int) bool", 1)
+
+	(&bytes.Buffer{}).Next(2) //@signature("2", "Next(n int) []byte", 0)
+
+	myFunc := MyFunc(func(n int) string { return "" })
+	myFunc(123) //@signature("123", "myFunc(foo int) string", 0)
+
+	var ms myStruct
+	ms.foo(nil) //@signature("nil", "foo(e *json.Decoder) (*big.Int, error)", 0)
+
+	panic("oops!")    //@signature("oops", "panic(interface{})", 0)
+	make([]int, 1, 2) //@signature("2", "make([]int, int, int) []int", 2)
+
+	Foo(myFunc(123), 456) //@signature("myFunc", "Foo(a string, b int) (c bool)", 0)
+	Foo(myFunc(123), 456) //@signature("123", "myFunc(foo int) string", 0)
+}

internal/span/token.go

@@ -19,8 +19,8 @@ type Range struct {
 }
 
 // TokenConverter is a Converter backed by a token file set and file.
-// It uses the file set methods to work out determine the conversions which
-// make if fast and do not require the file contents.
+// It uses the file set methods to work out the conversions, which
+// makes it fast and does not require the file contents.
 type TokenConverter struct {
 	fset *token.FileSet
 	file *token.File