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>
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index fc55aae..4c27c4f 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -45,6 +45,7 @@
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 @@
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 @@
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 @@
"typdef": expectedTypeDefinitions.collect,
"highlight": expectedHighlights.collect,
"symbol": expectedSymbols.collect,
+ "signature": expectedSignatures.collect,
}); err != nil {
t.Fatal(err)
}
@@ -176,6 +180,14 @@
}
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 @@
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 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 d41aa60..092df56 100644
--- a/internal/lsp/server.go
+++ b/internal/lsp/server.go
@@ -395,7 +395,7 @@
}
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 81702ea..38f1df7 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -508,7 +508,11 @@
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 f64088f..bcb4091 100644
--- a/internal/lsp/source/signature_help.go
+++ b/internal/lsp/source/signature_help.go
@@ -10,6 +10,7 @@
"go/ast"
"go/token"
"go/types"
+ "strings"
"golang.org/x/tools/go/ast/astutil"
)
@@ -38,7 +39,7 @@
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 @@
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")
+ // 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)
}
- 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)
- }
+
+ 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 @@
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 0000000..72351c4
--- /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 0fc0169..406da4a 100644
--- a/internal/span/token.go
+++ b/internal/span/token.go
@@ -19,8 +19,8 @@
}
// 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
