From 7e72c71c505fa4ee8da98520d2b0673ba2eefe56 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 28 Jun 2019 21:27:41 -0400 Subject: [PATCH] internal/lsp: hide signature help in function literals Often anonymous functions can be passed as arguments to a function. In these cases, it can be annoying for a user to see signature help for the entire duration of their writing this function. This change detects if the user is typing in a function literal and disables signature help in that case. Fixes golang/go#31633 Change-Id: I7166910739b6e1ec0da2ec852336136b81d13be0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/184260 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Suzy Mueller --- internal/lsp/lsp_test.go | 15 ++++++++++--- internal/lsp/source/signature_help.go | 15 ++++++++++--- internal/lsp/source/source_test.go | 23 ++++++++++++-------- internal/lsp/testdata/signature/signature.go | 7 ++++++ internal/lsp/tests/tests.go | 10 ++++++--- 5 files changed, 52 insertions(+), 18 deletions(-) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 117a301c..7edb5967 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -681,16 +681,25 @@ func (r *runner) SignatureHelp(t *testing.T, data tests.Signatures) { Position: loc.Range.Start, }) if err != nil { - t.Fatal(err) + // Only fail if we got an error we did not expect. + if expectedSignatures != nil { + t.Fatal(err) + } + continue + } + if expectedSignatures == nil { + if gotSignatures != nil { + t.Errorf("expected no signature, got %v", gotSignatures) + } + continue } - if diff := diffSignatures(spn, expectedSignatures, gotSignatures); diff != "" { t.Error(diff) } } } -func diffSignatures(spn span.Span, want source.SignatureInformation, got *protocol.SignatureHelp) string { +func diffSignatures(spn span.Span, want *source.SignatureInformation, got *protocol.SignatureHelp) string { decorate := func(f string, args ...interface{}) string { return fmt.Sprintf("Invalid signature at %s: %s", spn, fmt.Sprintf(f, args...)) } diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index c953e74a..784ba1e5 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -40,10 +40,19 @@ func SignatureHelp(ctx context.Context, f GoFile, pos token.Pos) (*SignatureInfo if path == nil { return nil, fmt.Errorf("cannot find node enclosing position") } +FindCall: for _, node := range path { - if c, ok := node.(*ast.CallExpr); ok && pos >= c.Lparen && pos <= c.Rparen { - callExpr = c - break + switch node := node.(type) { + case *ast.CallExpr: + if pos >= node.Lparen && pos <= node.Rparen { + callExpr = node + break FindCall + } + case *ast.FuncLit, *ast.FuncType: + // The user is within an anonymous function, + // which may be the parameter to the *ast.CallExpr. + // Don't show signature help in this case. + return nil, fmt.Errorf("no signature help within a function declaration") } } if callExpr == nil || callExpr.Fun == nil { diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index ff843489..5ee3eecf 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -615,7 +615,7 @@ func summarizeSymbols(i int, want []source.Symbol, got []source.Symbol, reason s func (r *runner) SignatureHelp(t *testing.T, data tests.Signatures) { ctx := context.Background() - for spn, expectedSignatures := range data { + for spn, expectedSignature := range data { f, err := r.view.GetFile(ctx, spn.URI()) if err != nil { t.Fatalf("failed for %v: %v", spn, err) @@ -624,27 +624,33 @@ func (r *runner) SignatureHelp(t *testing.T, data tests.Signatures) { pos := tok.Pos(spn.Start().Offset()) gotSignature, err := source.SignatureHelp(ctx, f.(source.GoFile), pos) if err != nil { - t.Fatalf("failed for %v: %v", spn, err) + // Only fail if we got an error we did not expect. + if expectedSignature != nil { + t.Fatalf("failed for %v: %v", spn, err) + } } - if diff := diffSignatures(spn, expectedSignatures, *gotSignature); diff != "" { + if expectedSignature == nil { + if gotSignature != nil { + t.Errorf("expected no signature, got %v", gotSignature) + } + continue + } + if diff := diffSignatures(spn, expectedSignature, gotSignature); diff != "" { t.Error(diff) } } } -func diffSignatures(spn span.Span, want source.SignatureInformation, got source.SignatureInformation) string { +func diffSignatures(spn span.Span, want *source.SignatureInformation, got *source.SignatureInformation) string { decorate := func(f string, args ...interface{}) string { return fmt.Sprintf("Invalid signature at %s: %s", spn, fmt.Sprintf(f, args...)) } - if want.ActiveParameter != got.ActiveParameter { return decorate("wanted active parameter of %d, got %f", want.ActiveParameter, got.ActiveParameter) } - if want.Label != got.Label { return decorate("wanted label %q, got %q", want.Label, got.Label) } - var paramParts []string for _, p := range got.Parameters { paramParts = append(paramParts, p.Label) @@ -653,10 +659,9 @@ func diffSignatures(spn span.Span, want source.SignatureInformation, got source. if !strings.Contains(got.Label, paramsStr) { return decorate("expected signature %q to contain params %q", got.Label, paramsStr) } - return "" } func (r *runner) Link(t *testing.T, data tests.Links) { - //This is a pure LSP feature, no source level functionality to be tested + // This is a pure LSP feature, no source level functionality to be tested. } diff --git a/internal/lsp/testdata/signature/signature.go b/internal/lsp/testdata/signature/signature.go index dd500350..9c8acbb8 100644 --- a/internal/lsp/testdata/signature/signature.go +++ b/internal/lsp/testdata/signature/signature.go @@ -60,4 +60,11 @@ func Qux() { panic("oops!") //@signature("oops", "panic(v interface{})", 0) println("hello", "world") //@signature("world", "println(args ...Type)", 0) + + Hello(func() { + //@signature("//", "", 0) + }) + } + +func Hello(func()) {} diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 86bd9228..6b6c39f2 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -36,7 +36,7 @@ const ( ExpectedReferencesCount = 4 ExpectedRenamesCount = 11 ExpectedSymbolsCount = 1 - ExpectedSignaturesCount = 20 + ExpectedSignaturesCount = 21 ExpectedLinksCount = 2 ) @@ -61,7 +61,7 @@ type References map[span.Span][]span.Span type Renames map[span.Span]string type Symbols map[span.URI][]source.Symbol type SymbolsChildren map[string][]source.Symbol -type Signatures map[span.Span]source.SignatureInformation +type Signatures map[span.Span]*source.SignatureInformation type Links map[span.URI][]Link type Data struct { @@ -505,10 +505,14 @@ func (data *Data) collectSymbols(name string, spn span.Span, kind string, parent } func (data *Data) collectSignatures(spn span.Span, signature string, activeParam int64) { - data.Signatures[spn] = source.SignatureInformation{ + data.Signatures[spn] = &source.SignatureInformation{ Label: signature, ActiveParameter: int(activeParam), } + // Hardcode special case to test the lack of a signature. + if signature == "" && activeParam == 0 { + data.Signatures[spn] = nil + } } func (data *Data) collectCompletionSnippets(spn span.Span, item token.Pos, plain, placeholder string) {