From fa8ad2176f9e2b598cd7994219beda262b564dda Mon Sep 17 00:00:00 2001 From: xizi Date: Sat, 17 Aug 2024 11:52:07 +0800 Subject: [PATCH 01/14] gopls/internal/golang: get sign over func name Currently, signatures can only be returned within the parentheses when use lsp."textDocument/signatureHelp"; they cannot be returned on the function name. Because we prefere to use "." to trigger a code complete menu, we also need the ability to retrieve function signatures Fixes golang/go#68922 --- gopls/internal/golang/signature_help.go | 52 ++++++++++++++----- .../integration/misc/signature_help_test.go | 14 +++-- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index a91be296cbd..5416454d2b9 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -39,8 +39,8 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle if err != nil { return nil, 0, err } - // Find a call expression surrounding the query position. - var callExpr *ast.CallExpr + // Find a call/SelectorExpr expression surrounding the query position. + var callNode ast.Node path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos) if path == nil { return nil, 0, fmt.Errorf("cannot find node enclosing position") @@ -48,9 +48,14 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle FindCall: for _, node := range path { switch node := node.(type) { + case *ast.SelectorExpr: + if pos >= node.Pos() && pos <= node.End() { + callNode = node + break FindCall + } case *ast.CallExpr: if pos >= node.Lparen && pos <= node.Rparen { - callExpr = node + callNode = node break FindCall } case *ast.FuncLit, *ast.FuncType: @@ -67,23 +72,36 @@ FindCall: // you've already dismissed the help!). return nil, 0, nil } + default: } } - if callExpr == nil || callExpr.Fun == nil { + + if callNode == nil { return nil, 0, nil } info := pkg.TypesInfo() + var expr ast.Expr + switch node := callNode.(type) { + case *ast.SelectorExpr: + expr = node + case *ast.CallExpr: + if node.Fun == nil { + return nil, 0, nil + } + expr = node.Fun + } + // Get the type information for the function being called. var sig *types.Signature - if tv, ok := info.Types[callExpr.Fun]; !ok { - return nil, 0, fmt.Errorf("cannot get type for Fun %[1]T (%[1]v)", callExpr.Fun) + if tv, ok := info.Types[expr]; !ok { + return nil, 0, fmt.Errorf("cannot get type for Fun %[1]T (%[1]v)", expr) } else if tv.IsType() { return nil, 0, nil // a conversion, not a call } else if sig, ok = tv.Type.Underlying().(*types.Signature); !ok { - return nil, 0, fmt.Errorf("call operand is not a func or type: %[1]T (%[1]v)", callExpr.Fun) + return nil, 0, fmt.Errorf("call operand is not a func or type: %[1]T (%[1]v)", expr) } // Inv: sig != nil @@ -93,16 +111,26 @@ FindCall: // 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) { + switch t := expr.(type) { case *ast.Ident: obj = info.ObjectOf(t) case *ast.SelectorExpr: obj = info.ObjectOf(t.Sel) } + + activeParam := 0 + switch node := callNode.(type) { + case *ast.SelectorExpr: + case *ast.CallExpr: + // only return activeParam when CallExpr + // because we don't modify arguments when get function signature only + activeParam = activeParameter(node, sig.Params().Len(), sig.Variadic(), pos) + } + if obj != nil && isBuiltin(obj) { // function? if obj, ok := obj.(*types.Builtin); ok { - return builtinSignature(ctx, snapshot, callExpr, obj.Name(), pos) + return builtinSignature(ctx, snapshot, activeParam, obj.Name(), pos) } // method (only error.Error)? @@ -116,8 +144,6 @@ FindCall: return nil, 0, bug.Errorf("call to unexpected built-in %v (%T)", obj, obj) } - activeParam := activeParameter(callExpr, sig.Params().Len(), sig.Variadic(), pos) - var ( name string comment *ast.CommentGroup @@ -148,7 +174,7 @@ FindCall: }, activeParam, nil } -func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, callExpr *ast.CallExpr, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { +func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, activeParam int, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { sig, err := NewBuiltinSignature(ctx, snapshot, name) if err != nil { return nil, 0, err @@ -157,7 +183,7 @@ func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, callExpr *a for _, p := range sig.params { paramInfo = append(paramInfo, protocol.ParameterInformation{Label: p}) } - activeParam := activeParameter(callExpr, len(sig.params), sig.variadic, pos) + return &protocol.SignatureInformation{ Label: sig.name + sig.Format(), Documentation: stringToSigInfoDocumentation(sig.doc, snapshot.Options()), diff --git a/gopls/internal/test/integration/misc/signature_help_test.go b/gopls/internal/test/integration/misc/signature_help_test.go index 8dffedf48e0..d2cf1401ad9 100644 --- a/gopls/internal/test/integration/misc/signature_help_test.go +++ b/gopls/internal/test/integration/misc/signature_help_test.go @@ -21,6 +21,7 @@ go 1.18 -- a/a/a.go -- package a +func GoSomething(int) {} func DoSomething(int) {} func _() { @@ -40,6 +41,7 @@ import "a.com/a" func _() { a.DoSomething() + a.DoSomething. } ` @@ -48,8 +50,8 @@ func _() { ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("a/a/a.go") env.OpenFile("b/b/b.go") - signatureHelp := func(filename string) *protocol.SignatureHelp { - loc := env.RegexpSearch(filename, `DoSomething\(()\)`) + signatureHelp := func(filename, posRegex string) *protocol.SignatureHelp { + loc := env.RegexpSearch(filename, posRegex) var params protocol.SignatureHelpParams params.TextDocument.URI = loc.URI params.Position = loc.Range.Start @@ -59,11 +61,15 @@ func _() { } return help } - ahelp := signatureHelp("a/a/a.go") - bhelp := signatureHelp("b/b/b.go") + ahelp := signatureHelp("a/a/a.go", `DoSomething\(()\)`) + bhelp := signatureHelp("b/b/b.go", `DoSomething\(()\)`) if diff := cmp.Diff(ahelp, bhelp); diff != "" { t.Fatal(diff) } + chelp := signatureHelp("b/b/b.go", `DoSomethin()g\.`) + if diff := cmp.Diff(ahelp, chelp); diff != "" { + t.Fatal(diff) + } }) } From dfe09e08c428cf75b66cee07b8fcdc34a00bb742 Mon Sep 17 00:00:00 2001 From: xizi Date: Thu, 22 Aug 2024 16:15:31 +0800 Subject: [PATCH 02/14] try to get sign from ident --- gopls/internal/golang/signature_help.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 5416454d2b9..4c48b989103 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -48,10 +48,9 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle FindCall: for _, node := range path { switch node := node.(type) { - case *ast.SelectorExpr: + case *ast.SelectorExpr, *ast.Ident: if pos >= node.Pos() && pos <= node.End() { callNode = node - break FindCall } case *ast.CallExpr: if pos >= node.Lparen && pos <= node.Rparen { @@ -72,7 +71,6 @@ FindCall: // you've already dismissed the help!). return nil, 0, nil } - default: } } @@ -85,6 +83,8 @@ FindCall: var expr ast.Expr switch node := callNode.(type) { + case *ast.Ident: + expr = node case *ast.SelectorExpr: expr = node case *ast.CallExpr: @@ -129,8 +129,9 @@ FindCall: if obj != nil && isBuiltin(obj) { // function? - if obj, ok := obj.(*types.Builtin); ok { - return builtinSignature(ctx, snapshot, activeParam, obj.Name(), pos) + callExpr, isCall := callNode.(*ast.CallExpr) + if obj, ok := obj.(*types.Builtin); ok && isCall { + return builtinSignature(ctx, snapshot, callExpr, obj.Name(), pos) } // method (only error.Error)? @@ -174,7 +175,7 @@ FindCall: }, activeParam, nil } -func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, activeParam int, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { +func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, callExpr *ast.CallExpr, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { sig, err := NewBuiltinSignature(ctx, snapshot, name) if err != nil { return nil, 0, err @@ -183,7 +184,7 @@ func builtinSignature(ctx context.Context, snapshot *cache.Snapshot, activeParam for _, p := range sig.params { paramInfo = append(paramInfo, protocol.ParameterInformation{Label: p}) } - + activeParam := activeParameter(callExpr, len(sig.params), sig.variadic, pos) return &protocol.SignatureInformation{ Label: sig.name + sig.Format(), Documentation: stringToSigInfoDocumentation(sig.doc, snapshot.Options()), From c3f381acc21829a230257b81218d5d3f2af7d4dc Mon Sep 17 00:00:00 2001 From: xizi Date: Thu, 22 Aug 2024 16:15:43 +0800 Subject: [PATCH 03/14] add test cases --- gopls/internal/test/marker/testdata/signature/signature.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gopls/internal/test/marker/testdata/signature/signature.txt b/gopls/internal/test/marker/testdata/signature/signature.txt index 7bdd6341818..37001a1c689 100644 --- a/gopls/internal/test/marker/testdata/signature/signature.txt +++ b/gopls/internal/test/marker/testdata/signature/signature.txt @@ -49,6 +49,9 @@ func Qux() { 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) + Foo("foo", 123) //@signature("o", "Foo(a string, b int) (c bool)", 0) + _ = Foo //@signature("o", "Foo(a string, b int) (c bool)", 0) + Foo //@signature("o", "Foo(a string, b int) (c bool)", 0) Bar(13.37, 0x13) //@signature("13.37", "Bar(float64, ...byte)", 0) Bar(13.37, 0x37) //@signature("0x37", "Bar(float64, ...byte)", 1) @@ -100,6 +103,7 @@ package signature func _() { Foo(//@signature("//", "Foo(a string, b int) (c bool)", 0) + Foo.//@signature("//", "Foo(a string, b int) (c bool)", 0) } -- signature/signature3.go -- From 169fcf04c0e2d873e96fa2ebf8819ccc93dcc3c5 Mon Sep 17 00:00:00 2001 From: xizi Date: Thu, 22 Aug 2024 16:22:42 +0800 Subject: [PATCH 04/14] reset gopls/internal/test/integration/misc/signature_help_test.go --- .../test/integration/misc/signature_help_test.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/gopls/internal/test/integration/misc/signature_help_test.go b/gopls/internal/test/integration/misc/signature_help_test.go index d2cf1401ad9..8dffedf48e0 100644 --- a/gopls/internal/test/integration/misc/signature_help_test.go +++ b/gopls/internal/test/integration/misc/signature_help_test.go @@ -21,7 +21,6 @@ go 1.18 -- a/a/a.go -- package a -func GoSomething(int) {} func DoSomething(int) {} func _() { @@ -41,7 +40,6 @@ import "a.com/a" func _() { a.DoSomething() - a.DoSomething. } ` @@ -50,8 +48,8 @@ func _() { ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("a/a/a.go") env.OpenFile("b/b/b.go") - signatureHelp := func(filename, posRegex string) *protocol.SignatureHelp { - loc := env.RegexpSearch(filename, posRegex) + signatureHelp := func(filename string) *protocol.SignatureHelp { + loc := env.RegexpSearch(filename, `DoSomething\(()\)`) var params protocol.SignatureHelpParams params.TextDocument.URI = loc.URI params.Position = loc.Range.Start @@ -61,15 +59,11 @@ func _() { } return help } - ahelp := signatureHelp("a/a/a.go", `DoSomething\(()\)`) - bhelp := signatureHelp("b/b/b.go", `DoSomething\(()\)`) + ahelp := signatureHelp("a/a/a.go") + bhelp := signatureHelp("b/b/b.go") if diff := cmp.Diff(ahelp, bhelp); diff != "" { t.Fatal(diff) } - chelp := signatureHelp("b/b/b.go", `DoSomethin()g\.`) - if diff := cmp.Diff(ahelp, chelp); diff != "" { - t.Fatal(diff) - } }) } From 22f52bb658a694ae5c7110a40b7ab6a03e144f1d Mon Sep 17 00:00:00 2001 From: xizi Date: Thu, 22 Aug 2024 22:52:08 +0800 Subject: [PATCH 05/14] fix: Work on both the function name and the identifier --- gopls/internal/golang/signature_help.go | 11 +++++++++-- gopls/internal/test/marker/marker_test.go | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 4c48b989103..ee373d46653 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -39,7 +39,7 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle if err != nil { return nil, 0, err } - // Find a call/SelectorExpr expression surrounding the query position. + // Find a Ident/Call/SelectorExpr expression surrounding the query position. var callNode ast.Node path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos) if path == nil { @@ -49,6 +49,8 @@ FindCall: for _, node := range path { switch node := node.(type) { case *ast.SelectorExpr, *ast.Ident: + // golang/go#68922: offer signature help when the + // cursor over ident or function name. if pos >= node.Pos() && pos <= node.End() { callNode = node } @@ -86,7 +88,12 @@ FindCall: case *ast.Ident: expr = node case *ast.SelectorExpr: - expr = node + // If the cursor is not on sel, then it should be on name. + if pos >= node.Sel.Pos() && pos <= node.Sel.End() { + expr = node + } else { + expr = node.X + } case *ast.CallExpr: if node.Fun == nil { return nil, 0, nil diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 87aecfaf6ed..35176be74ca 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -114,6 +114,9 @@ func Test(t *testing.T) { for _, test := range tests { test := test + if !strings.Contains(test.name, "signature.txt") { + continue + } t.Run(test.name, func(t *testing.T) { t.Parallel() if test.skipReason != "" { From e8c77b0b70d584272714a398e6b2aeab85a0ef01 Mon Sep 17 00:00:00 2001 From: xizi Date: Fri, 23 Aug 2024 11:25:26 +0800 Subject: [PATCH 06/14] remove test code --- gopls/internal/test/marker/marker_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 35176be74ca..87aecfaf6ed 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -114,9 +114,6 @@ func Test(t *testing.T) { for _, test := range tests { test := test - if !strings.Contains(test.name, "signature.txt") { - continue - } t.Run(test.name, func(t *testing.T) { t.Parallel() if test.skipReason != "" { From 2cbb0e8d996aa245a911c217f225460877659057 Mon Sep 17 00:00:00 2001 From: xizi Date: Sat, 24 Aug 2024 09:36:11 +0800 Subject: [PATCH 07/14] refactor: remove unnecessary code --- gopls/internal/golang/signature_help.go | 77 ++++++++----------- .../marker/testdata/signature/signature.txt | 7 +- 2 files changed, 36 insertions(+), 48 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index ee373d46653..5c90d339e78 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -39,8 +39,8 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle if err != nil { return nil, 0, err } - // Find a Ident/Call/SelectorExpr expression surrounding the query position. - var callNode ast.Node + // Find a call expression surrounding the query position. + var callExpr *ast.CallExpr path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos) if path == nil { return nil, 0, fmt.Errorf("cannot find node enclosing position") @@ -48,15 +48,9 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle FindCall: for _, node := range path { switch node := node.(type) { - case *ast.SelectorExpr, *ast.Ident: - // golang/go#68922: offer signature help when the - // cursor over ident or function name. - if pos >= node.Pos() && pos <= node.End() { - callNode = node - } case *ast.CallExpr: - if pos >= node.Lparen && pos <= node.Rparen { - callNode = node + if pos >= node.Pos() && pos <= node.Rparen { + callExpr = node break FindCall } case *ast.FuncLit, *ast.FuncType: @@ -77,38 +71,31 @@ FindCall: } - if callNode == nil { - return nil, 0, nil - } - + var fnval ast.Expr info := pkg.TypesInfo() - - var expr ast.Expr - switch node := callNode.(type) { - case *ast.Ident: - expr = node - case *ast.SelectorExpr: - // If the cursor is not on sel, then it should be on name. - if pos >= node.Sel.Pos() && pos <= node.Sel.End() { - expr = node - } else { - expr = node.X - } - case *ast.CallExpr: - if node.Fun == nil { - return nil, 0, nil - } - expr = node.Fun + if callExpr != nil && callExpr.Fun != nil { + // Found enclosing call. + fnval = callExpr.Fun + } else if id, ok := path[0].(*ast.Ident); ok && + info.Defs[id] == nil && // must be a use, not a definition + is[*types.Signature](info.TypeOf(id).Underlying()) { + // golang/go#68922: offer signature help when the + // cursor over ident or function name. + // No enclosing call found. + // If the selection is an Ident of func type, use that instead. + fnval = id + } else { + return nil, 0, nil } // Get the type information for the function being called. var sig *types.Signature - if tv, ok := info.Types[expr]; !ok { - return nil, 0, fmt.Errorf("cannot get type for Fun %[1]T (%[1]v)", expr) + if tv, ok := info.Types[fnval]; !ok { + return nil, 0, fmt.Errorf("cannot get type for Fun %[1]T (%[1]v)", fnval) } else if tv.IsType() { return nil, 0, nil // a conversion, not a call } else if sig, ok = tv.Type.Underlying().(*types.Signature); !ok { - return nil, 0, fmt.Errorf("call operand is not a func or type: %[1]T (%[1]v)", expr) + return nil, 0, fmt.Errorf("call operand is not a func or type: %[1]T (%[1]v)", fnval) } // Inv: sig != nil @@ -118,26 +105,15 @@ FindCall: // 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 := expr.(type) { + switch t := fnval.(type) { case *ast.Ident: obj = info.ObjectOf(t) case *ast.SelectorExpr: obj = info.ObjectOf(t.Sel) } - - activeParam := 0 - switch node := callNode.(type) { - case *ast.SelectorExpr: - case *ast.CallExpr: - // only return activeParam when CallExpr - // because we don't modify arguments when get function signature only - activeParam = activeParameter(node, sig.Params().Len(), sig.Variadic(), pos) - } - if obj != nil && isBuiltin(obj) { // function? - callExpr, isCall := callNode.(*ast.CallExpr) - if obj, ok := obj.(*types.Builtin); ok && isCall { + if obj, ok := obj.(*types.Builtin); ok { return builtinSignature(ctx, snapshot, callExpr, obj.Name(), pos) } @@ -152,6 +128,13 @@ FindCall: return nil, 0, bug.Errorf("call to unexpected built-in %v (%T)", obj, obj) } + activeParam := 0 + if callExpr != nil { + // only return activeParam when CallExpr + // because we don't modify arguments when get function signature only + activeParam = activeParameter(callExpr, sig.Params().Len(), sig.Variadic(), pos) + } + var ( name string comment *ast.CommentGroup diff --git a/gopls/internal/test/marker/testdata/signature/signature.txt b/gopls/internal/test/marker/testdata/signature/signature.txt index 37001a1c689..ccab432b379 100644 --- a/gopls/internal/test/marker/testdata/signature/signature.txt +++ b/gopls/internal/test/marker/testdata/signature/signature.txt @@ -81,7 +81,12 @@ func Qux() { _ = make([]int, 1, 2) //@signature("2", "make(t Type, size ...int) Type", 1) - Foo(myFunc(123), 456) //@signature("myFunc", "Foo(a string, b int) (c bool)", 0) + Foo(myFunc(123), 456) //@signature("o(", "Foo(a string, b int) (c bool)", 0) + Foo(myFunc(123), 456) //@signature("(m", "Foo(a string, b int) (c bool)", 0) + Foo( myFunc(123), 456) //@signature(" m", "Foo(a string, b int) (c bool)", 0) + Foo(myFunc(123), 456) //@signature(", ", "Foo(a string, b int) (c bool)", 0) + Foo(myFunc(123), 456) //@signature("456", "Foo(a string, b int) (c bool)", 1) + Foo(myFunc(123), 456) //@signature("(1", "myFunc(foo int) string", 0) Foo(myFunc(123), 456) //@signature("123", "myFunc(foo int) string", 0) panic("oops!") //@signature(")", "panic(v any)", 0) From 4250037644d0e6a00fccf7b5589bb2075977b90b Mon Sep 17 00:00:00 2001 From: xizi Date: Sun, 25 Aug 2024 22:42:20 +0800 Subject: [PATCH 08/14] fix: ident signature help && add test --- gopls/internal/golang/signature_help.go | 3 +++ gopls/internal/test/marker/testdata/signature/signature.txt | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 5c90d339e78..fb6ee9930e3 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -84,6 +84,9 @@ FindCall: // No enclosing call found. // If the selection is an Ident of func type, use that instead. fnval = id + if s, ok := path[1].(*ast.SelectorExpr); ok { + fnval = s + } } else { return nil, 0, nil } diff --git a/gopls/internal/test/marker/testdata/signature/signature.txt b/gopls/internal/test/marker/testdata/signature/signature.txt index ccab432b379..2161ca4bcbc 100644 --- a/gopls/internal/test/marker/testdata/signature/signature.txt +++ b/gopls/internal/test/marker/testdata/signature/signature.txt @@ -16,6 +16,7 @@ import ( "bytes" "encoding/json" "math/big" + "fmt" ) func Foo(a string, b int) (c bool) { @@ -86,9 +87,12 @@ func Qux() { Foo( myFunc(123), 456) //@signature(" m", "Foo(a string, b int) (c bool)", 0) Foo(myFunc(123), 456) //@signature(", ", "Foo(a string, b int) (c bool)", 0) Foo(myFunc(123), 456) //@signature("456", "Foo(a string, b int) (c bool)", 1) + Foo(myFunc) //@signature(")", "Foo(a string, b int) (c bool)", 0) Foo(myFunc(123), 456) //@signature("(1", "myFunc(foo int) string", 0) Foo(myFunc(123), 456) //@signature("123", "myFunc(foo int) string", 0) + fmt.Println //@signature("ln", "Println(a ...any) (n int, err error)", 0) + panic("oops!") //@signature(")", "panic(v any)", 0) println("hello", "world") //@signature(",", "println(args ...Type)", 0) From 10b63ab5acfcb7c27c2fc8b0484e28243098dea4 Mon Sep 17 00:00:00 2001 From: xizi Date: Sun, 25 Aug 2024 23:09:27 +0800 Subject: [PATCH 09/14] add release note placeholders --- gopls/doc/features/passive.md | 1 + gopls/doc/release/v0.17.0.md | 1 + 2 files changed, 2 insertions(+) diff --git a/gopls/doc/features/passive.md b/gopls/doc/features/passive.md index 92ae929ad5e..d27975a075a 100644 --- a/gopls/doc/features/passive.md +++ b/gopls/doc/features/passive.md @@ -96,6 +96,7 @@ Client support: ## Signature Help +## FIXME: signature help on ast.Ident... The LSP [`textDocument/signatureHelp`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp) query returns information about the innermost function call enclosing the cursor or selection, including the signature of the function and diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index dba85fef46c..17126a48fdc 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -35,3 +35,4 @@ constructor of the type of each symbol: `interface`, `struct`, `signature`, `pointer`, `array`, `map`, `slice`, `chan`, `string`, `number`, `bool`, and `invalid`. Editors may use this for syntax coloring. +## FIXME: signature help release note here From df6634fe552c015a8753467c9ad20f28de7e0751 Mon Sep 17 00:00:00 2001 From: xizi Date: Mon, 26 Aug 2024 14:49:05 +0800 Subject: [PATCH 10/14] fix: func value inside the parentheses of another function call --- gopls/internal/golang/signature_help.go | 20 +++++++++++++++++-- .../marker/testdata/signature/signature.txt | 11 ++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index fb6ee9930e3..7811f2a4343 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -45,11 +45,28 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle if path == nil { return nil, 0, fmt.Errorf("cannot find node enclosing position") } + info := pkg.TypesInfo() FindCall: for _, node := range path { switch node := node.(type) { + case *ast.Ident: + // When a function value is inside the + // parentheses of another function call, + // querying only the CallExpr will return + // the signature of the nearest function + // to the current identifier. + // Consider the following case: + // The '⁁' is a cursor. + // fmt.Println(fmt.Sp⁁rint, fmt.Sprint("hello %s", world)) + // We expect fmt.Sprint(ast.Ident) to be returned, + // not fmt.Println(ast.CallExpr). + if info.Defs[node] == nil && // must be a use, not a definition + info.TypeOf(node) != nil && + is[*types.Signature](info.TypeOf(node).Underlying()) { + break FindCall + } case *ast.CallExpr: - if pos >= node.Pos() && pos <= node.Rparen { + if pos >= node.Lparen && pos <= node.Rparen { callExpr = node break FindCall } @@ -72,7 +89,6 @@ FindCall: } var fnval ast.Expr - info := pkg.TypesInfo() if callExpr != nil && callExpr.Fun != nil { // Found enclosing call. fnval = callExpr.Fun diff --git a/gopls/internal/test/marker/testdata/signature/signature.txt b/gopls/internal/test/marker/testdata/signature/signature.txt index 2161ca4bcbc..9b872a2fb81 100644 --- a/gopls/internal/test/marker/testdata/signature/signature.txt +++ b/gopls/internal/test/marker/testdata/signature/signature.txt @@ -92,6 +92,17 @@ func Qux() { Foo(myFunc(123), 456) //@signature("123", "myFunc(foo int) string", 0) fmt.Println //@signature("ln", "Println(a ...any) (n int, err error)", 0) + fmt.Println(myFunc) //@signature("ln", "Println(a ...any) (n int, err error)", 0) + fmt.Println(myFunc) //@signature("Func", "myFunc(foo int) string", 0) + + var hi string = "hello" + var wl string = " world: %s" + fmt.Println(fmt.Sprintf(wl, myFunc)) //@signature("Func", "myFunc(foo int) string", 0) + fmt.Println(fmt.Sprintf(wl, myFunc)) //@signature("wl", "Sprintf(format string, a ...any) string", 0) + fmt.Println(fmt.Sprintf(wl, myFunc)) //@signature(" m", "Sprintf(format string, a ...any) string", 1) + fmt.Println(hi, fmt.Sprintf(wl, myFunc)) //@signature("Sprint", "Sprintf(format string, a ...any) string", 0) + fmt.Println(hi, fmt.Sprintf(wl, myFunc)) //@signature(" fmt", "Println(a ...any) (n int, err error)", 0) + fmt.Println(hi, fmt.Sprintf(wl, myFunc)) //@signature("hi", "Println(a ...any) (n int, err error)", 0) panic("oops!") //@signature(")", "panic(v any)", 0) println("hello", "world") //@signature(",", "println(args ...Type)", 0) From e2347c9a3c7dc13b508ed82e2e13627a2f2dfb4e Mon Sep 17 00:00:00 2001 From: xizi Date: Fri, 30 Aug 2024 18:14:15 +0800 Subject: [PATCH 11/14] add release note --- gopls/doc/features/passive.md | 10 +++++++++- gopls/doc/release/v0.17.0.md | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/gopls/doc/features/passive.md b/gopls/doc/features/passive.md index d27975a075a..4f0d5b53a3f 100644 --- a/gopls/doc/features/passive.md +++ b/gopls/doc/features/passive.md @@ -96,7 +96,6 @@ Client support: ## Signature Help -## FIXME: signature help on ast.Ident... The LSP [`textDocument/signatureHelp`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp) query returns information about the innermost function call enclosing the cursor or selection, including the signature of the function and @@ -108,6 +107,15 @@ function call. +The query is not limited to functions that are currently being called; +it also includes function values, identifiers, etc. If the trigger is +not inside the function’s parentheses or if there are no parentheses, +the active parameter position will not be returned. Since there is an +identifier with a function signature (callable), we should return it. + +This is not only helpful for code reading but also beneficial for +some completions, as it allows us to manipulate the function’s signature. + Client support: - **VS Code**: enabled by default. Also known as "[parameter hints](https://code.visualstudio.com/api/references/vscode-api#SignatureHelpProvider)" in the [IntelliSense settings](https://code.visualstudio.com/docs/editor/intellisense#_settings). diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index 17126a48fdc..12f04dadf2d 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -35,4 +35,7 @@ constructor of the type of each symbol: `interface`, `struct`, `signature`, `pointer`, `array`, `map`, `slice`, `chan`, `string`, `number`, `bool`, and `invalid`. Editors may use this for syntax coloring. -## FIXME: signature help release note here +## SignatureHelp for ident and values. + +Now, function signature help can be used on any identifier with a function +signature, not just within the parentheses of a function being called. From 981feab7dd5f06430db0efc73266ae5b704dfd08 Mon Sep 17 00:00:00 2001 From: xizi Date: Wed, 4 Sep 2024 10:14:39 +0800 Subject: [PATCH 12/14] make doc precise --- gopls/doc/features/passive.md | 11 +++-------- gopls/internal/golang/signature_help.go | 17 ++++++----------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/gopls/doc/features/passive.md b/gopls/doc/features/passive.md index 4f0d5b53a3f..23a1938cbeb 100644 --- a/gopls/doc/features/passive.md +++ b/gopls/doc/features/passive.md @@ -107,14 +107,9 @@ function call. -The query is not limited to functions that are currently being called; -it also includes function values, identifiers, etc. If the trigger is -not inside the function’s parentheses or if there are no parentheses, -the active parameter position will not be returned. Since there is an -identifier with a function signature (callable), we should return it. - -This is not only helpful for code reading but also beneficial for -some completions, as it allows us to manipulate the function’s signature. +Call parens are not necessary if the cursor is within an identifier +that denotes a function or method. For example, Signature Help at +`once.Do(initialize‸)` will describe `initialize`, not `once.Do`. Client support: - **VS Code**: enabled by default. diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 7811f2a4343..2591931f3ab 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -50,17 +50,12 @@ FindCall: for _, node := range path { switch node := node.(type) { case *ast.Ident: - // When a function value is inside the - // parentheses of another function call, - // querying only the CallExpr will return - // the signature of the nearest function - // to the current identifier. - // Consider the following case: - // The '⁁' is a cursor. - // fmt.Println(fmt.Sp⁁rint, fmt.Sprint("hello %s", world)) - // We expect fmt.Sprint(ast.Ident) to be returned, - // not fmt.Println(ast.CallExpr). - if info.Defs[node] == nil && // must be a use, not a definition + // If the selection is a function/method identifier, + // even one not in function call position, + // show help for its signature. Example: + // once.Do(initialize⁁) + // should show help for initialize, not once.Do. + if info.Defs[node] == nil && info.TypeOf(node) != nil && is[*types.Signature](info.TypeOf(node).Underlying()) { break FindCall From 657749fccf1eb364bbbbca39b4697d90d79d95d6 Mon Sep 17 00:00:00 2001 From: xizi Date: Wed, 4 Sep 2024 11:37:05 +0800 Subject: [PATCH 13/14] refactor code & add more test --- gopls/internal/golang/signature_help.go | 37 ++++++++++--------- .../marker/testdata/signature/signature.txt | 1 + 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 2591931f3ab..882e86f5c54 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -46,8 +46,9 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle return nil, 0, fmt.Errorf("cannot find node enclosing position") } info := pkg.TypesInfo() + var fnval ast.Expr FindCall: - for _, node := range path { + for i, node := range path { switch node := node.(type) { case *ast.Ident: // If the selection is a function/method identifier, @@ -58,11 +59,28 @@ FindCall: if info.Defs[node] == nil && info.TypeOf(node) != nil && is[*types.Signature](info.TypeOf(node).Underlying()) { + // golang/go#68922: offer signature help when the + // cursor over ident or function name. + // No enclosing call found. + // If the selection is an Ident of func type, use that instead. + fnval = node + if _, ok := info.Types[fnval]; ok { + break FindCall + } + + if len(path) <= i+1 { + continue + } + if s, ok := path[i+1].(*ast.SelectorExpr); ok { + fnval = s + } + break FindCall } case *ast.CallExpr: if pos >= node.Lparen && pos <= node.Rparen { callExpr = node + fnval = callExpr.Fun break FindCall } case *ast.FuncLit, *ast.FuncType: @@ -83,22 +101,7 @@ FindCall: } - var fnval ast.Expr - if callExpr != nil && callExpr.Fun != nil { - // Found enclosing call. - fnval = callExpr.Fun - } else if id, ok := path[0].(*ast.Ident); ok && - info.Defs[id] == nil && // must be a use, not a definition - is[*types.Signature](info.TypeOf(id).Underlying()) { - // golang/go#68922: offer signature help when the - // cursor over ident or function name. - // No enclosing call found. - // If the selection is an Ident of func type, use that instead. - fnval = id - if s, ok := path[1].(*ast.SelectorExpr); ok { - fnval = s - } - } else { + if fnval == nil { return nil, 0, nil } diff --git a/gopls/internal/test/marker/testdata/signature/signature.txt b/gopls/internal/test/marker/testdata/signature/signature.txt index 9b872a2fb81..1da4eb5843e 100644 --- a/gopls/internal/test/marker/testdata/signature/signature.txt +++ b/gopls/internal/test/marker/testdata/signature/signature.txt @@ -124,6 +124,7 @@ package signature func _() { Foo(//@signature("//", "Foo(a string, b int) (c bool)", 0) Foo.//@signature("//", "Foo(a string, b int) (c bool)", 0) + Foo.//@signature("oo", "Foo(a string, b int) (c bool)", 0) } -- signature/signature3.go -- From 5e7965edd342a450fd486ccd1fb7149ad20a2733 Mon Sep 17 00:00:00 2001 From: xizi Date: Thu, 12 Sep 2024 00:23:41 +0800 Subject: [PATCH 14/14] make code more clear --- gopls/internal/golang/signature_help.go | 33 +++++++++---------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 882e86f5c54..26cb92c643b 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -47,41 +47,30 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle } info := pkg.TypesInfo() var fnval ast.Expr -FindCall: +loop: for i, node := range path { switch node := node.(type) { case *ast.Ident: - // If the selection is a function/method identifier, + // If the selected text is a function/method Ident orSelectorExpr, // even one not in function call position, // show help for its signature. Example: // once.Do(initialize⁁) // should show help for initialize, not once.Do. - if info.Defs[node] == nil && - info.TypeOf(node) != nil && - is[*types.Signature](info.TypeOf(node).Underlying()) { - // golang/go#68922: offer signature help when the - // cursor over ident or function name. - // No enclosing call found. - // If the selection is an Ident of func type, use that instead. - fnval = node - if _, ok := info.Types[fnval]; ok { - break FindCall + if t := info.TypeOf(node); t != nil && + info.Defs[node] == nil && + is[*types.Signature](t.Underlying()) { + if sel, ok := path[i+1].(*ast.SelectorExpr); ok && sel.Sel == node { + fnval = sel // e.g. fmt.Println⁁ + } else { + fnval = node } - - if len(path) <= i+1 { - continue - } - if s, ok := path[i+1].(*ast.SelectorExpr); ok { - fnval = s - } - - break FindCall + break loop } case *ast.CallExpr: if pos >= node.Lparen && pos <= node.Rparen { callExpr = node fnval = callExpr.Fun - break FindCall + break loop } case *ast.FuncLit, *ast.FuncType: // The user is within an anonymous function,