gopls: improve SignatureHelp triggering

- Allow manually triggering signature help inside strings.

- Prevent window from disappearing when typing or navigating inside strings.

- Add ')' as a retrigger character to support editors that
  don't send signature help requests on every keystroke.

Change-Id: I60da67cb9262e01c4acfa436815048f9f7ba7ec2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/684895
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: skewb1k

gopls/internal/golang/signature_help.go

@@ -23,7 +23,7 @@ import (
 
 // SignatureHelp returns information about the signature of the innermost
 // function call enclosing the position, or nil if there is none.
-func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, position protocol.Position) (*protocol.SignatureInformation, error) {
+func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, params *protocol.SignatureHelpParams) (*protocol.SignatureInformation, error) {
 	ctx, done := event.Start(ctx, "golang.SignatureHelp")
 	defer done()
 
@@ -33,7 +33,7 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle
 	if err != nil {
 		return nil, fmt.Errorf("getting file for SignatureHelp: %w", err)
 	}
-	pos, err := pgf.PositionPos(position)
+	pos, err := pgf.PositionPos(params.Position)
 	if err != nil {
 		return nil, err
 	}
@@ -78,12 +78,10 @@ loop:
 			// Don't show signature help in this case.
 			return nil, nil
 		case *ast.BasicLit:
-			if node.Kind == token.STRING {
-				// golang/go#43397: don't offer signature help when the user is typing
-				// in a string literal. Most LSP clients use ( or , as trigger
-				// characters, but within a string literal these should not trigger
-				// signature help (and it can be annoying when this happens after
-				// you've already dismissed the help!).
+			// golang/go#43397: don't offer signature help when the user is typing
+			// in a string literal unless it was manually invoked or help is already active.
+			if node.Kind == token.STRING &&
+				(params.Context == nil || (params.Context.TriggerKind != protocol.SigInvoked && !params.Context.IsRetrigger)) {
 				return nil, nil
 			}
 		}
@@ -119,7 +117,7 @@ loop:
 		// Special handling for error.Error, which is the only builtin method.
 		if obj.Name() == "Error" {
 			return &protocol.SignatureInformation{
-				Label: "Error()",
+				Label: "Error() string",
 				// TODO(skewb1k): move the docstring for error.Error to builtin.go and reuse it across all relevant LSP methods.
 				Documentation:   stringToSigInfoDocumentation("Error returns the error message.", snapshot.Options()),
 				Parameters:      nil,

gopls/internal/server/general.go

@@ -161,6 +161,9 @@ func (s *server) Initialize(ctx context.Context, params *protocol.ParamInitializ
 			},
 			SignatureHelpProvider: &protocol.SignatureHelpOptions{
 				TriggerCharacters: []string{"(", ","},
+				// Used to update or dismiss signature help when it's already active,
+				// typically after a call expression is closed.
+				RetriggerCharacters: []string{")"},
 			},
 			TextDocumentSync: &protocol.TextDocumentSyncOptions{
 				Change:    protocol.Incremental,

gopls/internal/server/signature_help.go

@@ -28,7 +28,7 @@ func (s *server) SignatureHelp(ctx context.Context, params *protocol.SignatureHe
 		return nil, nil // empty result
 	}
 
-	info, err := golang.SignatureHelp(ctx, snapshot, fh, params.Position)
+	info, err := golang.SignatureHelp(ctx, snapshot, fh, params)
 	if err != nil {
 		// TODO(rfindley): is this correct? Apparently, returning an error from
 		// signatureHelp is distracting in some editors, though I haven't confirmed

gopls/internal/test/marker/testdata/completion/testy.txt

@@ -57,5 +57,5 @@ func _() {
 }
 
 func issue63578(err error) {
-	err.Error() //@signature(")", "Error()", -1)
+	err.Error() //@signature(")", "Error() string", -1)
 }