internal/lsp/protocol: unmarshal to pointers when dispatching requests

With #34111, we are forwarding the LSP from one gopls instance to
another. This exposed an asymmetry in our LSP dispatching: for both
ClientDispatcher and ServerDispatcher, we unmarshal to non-nil response
structs. This means that when forwarding the LSP, we translate empty
JSON responses (corresponding to nil values) into the non-nil zero
value.

This causes problems for some editors, as reported in #37570. Fix it by
instead unmarshaling to a pointer.

This is, of course, a somewhat dangerous change. I fixed the one NPE
that occurred in tests, and have done some mild manual testing.  I
wouldn't be surprised if we discover more NPEs later on, but I still
think this is the right change to make.

Updates golang/go#34111
Fixes golang/go#37570

Change-Id: Ie69e92d2821c829cdfc4f4ab303679a725f1f859
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222058
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/lsp/cmd/prepare_rename.go

@@ -6,6 +6,7 @@ package cmd
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 
@@ -35,6 +36,10 @@ Example:
 	f.PrintDefaults()
 }
 
+// ErrInvalidRenamePosition is returned when prepareRename is run at a position that
+// is not a candidate for renaming.
+var ErrInvalidRenamePosition = errors.New("request is not valid at the given position")
+
 func (r *prepareRename) Run(ctx context.Context, args ...string) error {
 	if len(args) != 1 {
 		return tool.CommandLineErrorf("prepare_rename expects 1 argument (file)")
@@ -66,7 +71,7 @@ func (r *prepareRename) Run(ctx context.Context, args ...string) error {
 		return fmt.Errorf("prepare_rename failed: %v", err)
 	}
 	if result == nil {
-		return fmt.Errorf("request is not valid at the given position")
+		return ErrInvalidRenamePosition
 	}
 
 	l := protocol.Location{Range: *result}

internal/lsp/cmd/signature.go

@@ -72,7 +72,7 @@ func (r *signature) Run(ctx context.Context, args ...string) error {
 		return err
 	}
 
-	if len(s.Signatures) == 0 {
+	if s == nil || len(s.Signatures) == 0 {
 		return tool.CommandLineErrorf("%v: not a function", from)
 	}
 

internal/lsp/cmd/test/prepare_rename.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"testing"
 
+	"golang.org/x/tools/internal/lsp/cmd"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
@@ -27,7 +28,7 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare
 	)
 
 	if want.Text == "" {
-		if stdErr != "" {
+		if stdErr != "" && stdErr != cmd.ErrInvalidRenamePosition.Error() {
 			t.Errorf("prepare_rename failed for %s,\nexpected:\n`%v`\ngot:\n`%v`", target, expect, stdErr)
 		}
 		return

internal/lsp/protocol/tsclient.go

@@ -3,7 +3,7 @@ package protocol
 // Package protocol contains data types and code for LSP jsonrpcs
 // generated automatically from vscode-languageserver-node
 // commit: 7b90c29d0cb5cd7b9c41084f6cb3781a955adeba
-// last fetched Wed Feb 12 2020 17:16:47 GMT-0500 (Eastern Standard Time)
+// last fetched Wed Mar 04 2020 13:02:46 GMT-0500 (Eastern Standard Time)
 
 // Code generated (see typescript/README.md) DO NOT EDIT.
 
@@ -194,17 +194,17 @@ func (s *clientDispatcher) UnregisterCapability(ctx context.Context, params *Unr
 }
 
 func (s *clientDispatcher) ShowMessageRequest(ctx context.Context, params *ShowMessageRequestParams) (*MessageActionItem /*MessageActionItem | null*/, error) {
-	var result MessageActionItem /*MessageActionItem | null*/
+	var result *MessageActionItem /*MessageActionItem | null*/
 	if err := s.Conn.Call(ctx, "window/showMessageRequest", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *clientDispatcher) ApplyEdit(ctx context.Context, params *ApplyWorkspaceEditParams) (*ApplyWorkspaceEditResponse, error) {
-	var result ApplyWorkspaceEditResponse
+	var result *ApplyWorkspaceEditResponse
 	if err := s.Conn.Call(ctx, "workspace/applyEdit", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }

internal/lsp/protocol/tsprotocol.go

@@ -1,7 +1,7 @@
 // Package protocol contains data types and code for LSP jsonrpcs
 // generated automatically from vscode-languageserver-node
 // commit: 7b90c29d0cb5cd7b9c41084f6cb3781a955adeba
-// last fetched Wed Feb 12 2020 17:16:47 GMT-0500 (Eastern Standard Time)
+// last fetched Wed Mar 04 2020 13:02:46 GMT-0500 (Eastern Standard Time)
 package protocol
 
 // Code generated (see typescript/README.md) DO NOT EDIT.

internal/lsp/protocol/tsserver.go

@@ -3,7 +3,7 @@ package protocol
 // Package protocol contains data types and code for LSP jsonrpcs
 // generated automatically from vscode-languageserver-node
 // commit: 7b90c29d0cb5cd7b9c41084f6cb3781a955adeba
-// last fetched Wed Feb 12 2020 17:16:47 GMT-0500 (Eastern Standard Time)
+// last fetched Wed Mar 04 2020 13:02:46 GMT-0500 (Eastern Standard Time)
 
 // Code generated (see typescript/README.md) DO NOT EDIT.
 
@@ -757,11 +757,11 @@ func (s *serverDispatcher) WorkDoneProgressCreate(ctx context.Context, params *W
 }
 
 func (s *serverDispatcher) Initialize(ctx context.Context, params *ParamInitialize) (*InitializeResult, error) {
-	var result InitializeResult
+	var result *InitializeResult
 	if err := s.Conn.Call(ctx, "initialize", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) Shutdown(ctx context.Context) error {
@@ -777,35 +777,35 @@ func (s *serverDispatcher) WillSaveWaitUntil(ctx context.Context, params *WillSa
 }
 
 func (s *serverDispatcher) Completion(ctx context.Context, params *CompletionParams) (*CompletionList /*CompletionItem[] | CompletionList | null*/, error) {
-	var result CompletionList /*CompletionItem[] | CompletionList | null*/
+	var result *CompletionList /*CompletionItem[] | CompletionList | null*/
 	if err := s.Conn.Call(ctx, "textDocument/completion", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) Resolve(ctx context.Context, params *CompletionItem) (*CompletionItem, error) {
-	var result CompletionItem
+	var result *CompletionItem
 	if err := s.Conn.Call(ctx, "completionItem/resolve", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) Hover(ctx context.Context, params *HoverParams) (*Hover /*Hover | null*/, error) {
-	var result Hover /*Hover | null*/
+	var result *Hover /*Hover | null*/
 	if err := s.Conn.Call(ctx, "textDocument/hover", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) SignatureHelp(ctx context.Context, params *SignatureHelpParams) (*SignatureHelp /*SignatureHelp | null*/, error) {
-	var result SignatureHelp /*SignatureHelp | null*/
+	var result *SignatureHelp /*SignatureHelp | null*/
 	if err := s.Conn.Call(ctx, "textDocument/signatureHelp", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) Definition(ctx context.Context, params *DefinitionParams) (Definition /*Definition | DefinitionLink[] | null*/, error) {
@@ -865,11 +865,11 @@ func (s *serverDispatcher) CodeLens(ctx context.Context, params *CodeLensParams)
 }
 
 func (s *serverDispatcher) ResolveCodeLens(ctx context.Context, params *CodeLens) (*CodeLens, error) {
-	var result CodeLens
+	var result *CodeLens
 	if err := s.Conn.Call(ctx, "codeLens/resolve", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) DocumentLink(ctx context.Context, params *DocumentLinkParams) ([]DocumentLink /*DocumentLink[] | null*/, error) {
@@ -881,11 +881,11 @@ func (s *serverDispatcher) DocumentLink(ctx context.Context, params *DocumentLin
 }
 
 func (s *serverDispatcher) ResolveDocumentLink(ctx context.Context, params *DocumentLink) (*DocumentLink, error) {
-	var result DocumentLink
+	var result *DocumentLink
 	if err := s.Conn.Call(ctx, "documentLink/resolve", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) Formatting(ctx context.Context, params *DocumentFormattingParams) ([]TextEdit /*TextEdit[] | null*/, error) {
@@ -913,19 +913,19 @@ func (s *serverDispatcher) OnTypeFormatting(ctx context.Context, params *Documen
 }
 
 func (s *serverDispatcher) Rename(ctx context.Context, params *RenameParams) (*WorkspaceEdit /*WorkspaceEdit | null*/, error) {
-	var result WorkspaceEdit /*WorkspaceEdit | null*/
+	var result *WorkspaceEdit /*WorkspaceEdit | null*/
 	if err := s.Conn.Call(ctx, "textDocument/rename", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) PrepareRename(ctx context.Context, params *PrepareRenameParams) (*Range /*Range | { range: Range, placeholder: string } | null*/, error) {
-	var result Range /*Range | { range: Range, placeholder: string } | null*/
+	var result *Range /*Range | { range: Range, placeholder: string } | null*/
 	if err := s.Conn.Call(ctx, "textDocument/prepareRename", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) ExecuteCommand(ctx context.Context, params *ExecuteCommandParams) (interface{} /*any | null*/, error) {
@@ -961,11 +961,11 @@ func (s *serverDispatcher) OutgoingCalls(ctx context.Context, params *CallHierar
 }
 
 func (s *serverDispatcher) SemanticTokens(ctx context.Context, params *SemanticTokensParams) (*SemanticTokens /*SemanticTokens | null*/, error) {
-	var result SemanticTokens /*SemanticTokens | null*/
+	var result *SemanticTokens /*SemanticTokens | null*/
 	if err := s.Conn.Call(ctx, "textDocument/semanticTokens", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) SemanticTokensEdits(ctx context.Context, params *SemanticTokensEditsParams) (interface{} /* SemanticTokens | SemanticTokensEdits | nil*/, error) {
@@ -977,11 +977,11 @@ func (s *serverDispatcher) SemanticTokensEdits(ctx context.Context, params *Sema
 }
 
 func (s *serverDispatcher) SemanticTokensRange(ctx context.Context, params *SemanticTokensRangeParams) (*SemanticTokens /*SemanticTokens | null*/, error) {
-	var result SemanticTokens /*SemanticTokens | null*/
+	var result *SemanticTokens /*SemanticTokens | null*/
 	if err := s.Conn.Call(ctx, "textDocument/semanticTokens/range", params, &result); err != nil {
 		return nil, err
 	}
-	return &result, nil
+	return result, nil
 }
 
 func (s *serverDispatcher) NonstandardRequest(ctx context.Context, method string, params interface{}) (interface{}, error) {

internal/lsp/protocol/typescript/README.md

@@ -9,7 +9,7 @@ Get the typescript code for the jsonrpc protocol with
 
 `util.ts`` expects it to be in your HOME directory
 
-If you want to reproduce the existing files you need to be on a branch with the same git hash, for instance, `git checkout 635ab1f`
+If you want to reproduce the existing files you need to be on a branch with the same git hash, for instance, `git checkout 7b90c29`
 
 ## Usage
 

internal/lsp/protocol/typescript/code.ts

@@ -988,14 +988,13 @@ function goReq(side: side, m: string) {
   let callBody = `return s.Conn.Call(ctx, "${m}", nil, nil)\n}`;
   if (b != '' && b != 'void') {
     const p2 = a == '' ? 'nil' : 'params';
-    let theRet = `result`;
-    if (indirect(b)) theRet = '&result';
-    callBody = `var result ${b}
+    const returnType = indirect(b) ? `*${b}` : b;
+    callBody = `var result ${returnType}
 			if err := s.Conn.Call(ctx, "${m}", ${
         p2}, &result); err != nil {
 				return nil, err
       }
-      return ${theRet}, nil
+      return result, nil
     }`;
   } else if (a != '') {
     callBody = `return s.Conn.Call(ctx, "${m}", params, nil) // Call, not Notify
@@ -1189,4 +1188,4 @@ function main() {
   output(server);
 }
 
-main()
+main()

internal/lsp/regtest/env.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/jsonrpc2/servertest"
 	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/debug"
@@ -232,11 +233,12 @@ type Env struct {
 	t   *testing.T
 	ctx context.Context
 
-	// Most tests should not need to access the workspace or editor, or server,
-	// but they are available if needed.
+	// Most tests should not need to access the workspace, editor, server, or
+	// connection, but they are available if needed.
 	W      *fake.Workspace
 	E      *fake.Editor
 	Server servertest.Connector
+	Conn   *jsonrpc2.Conn
 
 	// mu guards the fields below, for the purpose of checking conditions on
 	// every change to diagnostics.
@@ -269,6 +271,7 @@ func NewEnv(ctx context.Context, t *testing.T, ws *fake.Workspace, ts servertest
 		W:               ws,
 		E:               editor,
 		Server:          ts,
+		Conn:            conn,
 		lastDiagnostics: make(map[string]*protocol.PublishDiagnosticsParams),
 		waiters:         make(map[int]*diagnosticCondition),
 	}

internal/lsp/regtest/serialization_test.go

@@ -0,0 +1,44 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package regtest
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"golang.org/x/tools/internal/lsp/protocol"
+)
+
+const simpleProgram = `
+-- go.mod --
+module mod
+
+go 1.12
+-- main.go --
+package main
+
+import "fmt"
+
+func main() {
+	fmt.Println("Hello World.")
+}`
+
+func TestHoverSerialization(t *testing.T) {
+	runner.Run(t, exampleProgram, func(ctx context.Context, t *testing.T, env *Env) {
+		// Hover on an empty line.
+		params := protocol.HoverParams{}
+		params.TextDocument.URI = env.W.URI("main.go")
+		params.Position.Line = 3
+		params.Position.Character = 0
+		var resp json.RawMessage
+		if err := env.Conn.Call(ctx, "textDocument/hover", &params, &resp); err != nil {
+			t.Fatal(err)
+		}
+		if len(string(resp)) > 0 {
+			t.Errorf("got non-empty response for empty hover: %v", string(resp))
+		}
+	})
+}