From 7e40e1726ee0fcf776fcf00f033ef7ed114ca1c4 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Wed, 17 Apr 2019 04:54:13 +0000 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/lsp_test.go | 79 +++++++++++++++++++ internal/lsp/server.go | 2 +- internal/lsp/source/completion.go | 6 +- internal/lsp/source/signature_help.go | 83 ++++++++++++-------- internal/lsp/testdata/signature/signature.go | 61 ++++++++++++++ internal/span/token.go | 4 +- 6 files changed, 197 insertions(+), 38 deletions(-) create mode 100644 internal/lsp/testdata/signature/signature.go diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index fc55aae532..4c27c4f284 100644 --- a/internal/lsp/lsp_test.go +++ b/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 }) diff --git a/internal/lsp/server.go b/internal/lsp/server.go index d41aa602a8..092df5601c 100644 --- a/internal/lsp/server.go +++ b/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 } diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 81702ea092..38f1df760c 100644 --- a/internal/lsp/source/completion.go +++ b/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() diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index f64088f779..bcb4091fd3 100644 --- a/internal/lsp/source/signature_help.go +++ b/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 diff --git a/internal/lsp/testdata/signature/signature.go b/internal/lsp/testdata/signature/signature.go new file mode 100644 index 0000000000..72351c486c --- /dev/null +++ b/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) +} diff --git a/internal/span/token.go b/internal/span/token.go index 0fc0169dba..406da4af3c 100644 --- a/internal/span/token.go +++ b/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