internal/lsp: fix broken lsp logs

ID's are now by value not pointer, which caused it to not use the Format
method, resulting in broken id strings
The id maps need to be crossover (set and get go to different maps for a given direction of message)

Change-Id: Ide2b63ec1b009ae3587ee10e8bce018732ea342c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229244
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>

gopls/integration/replay/main.go

@@ -145,7 +145,8 @@ func send(ctx context.Context, l *parse.Logmsg, stream jsonrpc2.Stream, id *json
 		if err != nil {
 			n = 0
 		}
-		id = jsonrpc2.NewIntID(int64(n))
+		nid := jsonrpc2.NewIntID(int64(n))
+		id = &nid
 	}
 	var msg jsonrpc2.Message
 	var err error

internal/jsonrpc2/wire.go

@@ -125,22 +125,20 @@ func (wireVersionTag) UnmarshalJSON(data []byte) error {
 const invalidID int64 = math.MaxInt64
 
 // NewIntID returns a new numerical request ID.
-func NewIntID(v int64) *ID { return &ID{number: v} }
+func NewIntID(v int64) ID { return ID{number: v} }
 
 // NewStringID returns a new string request ID.
-func NewStringID(v string) *ID { return &ID{name: v} }
+func NewStringID(v string) ID { return ID{name: v} }
 
 // Format writes the ID to the formatter.
 // If the rune is q the representation is non ambiguous,
 // string forms are quoted, number forms are preceded by a #
-func (id *ID) Format(f fmt.State, r rune) {
+func (id ID) Format(f fmt.State, r rune) {
 	numF, strF := `%d`, `%s`
 	if r == 'q' {
 		numF, strF = `#%d`, `%q`
 	}
 	switch {
-	case id == nil:
-		fmt.Fprintf(f, numF, invalidID)
 	case id.name != "":
 		fmt.Fprintf(f, strF, id.name)
 	default:

internal/jsonrpc2/wire_test.go

@@ -8,7 +8,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"math"
 	"testing"
 
 	"golang.org/x/tools/internal/jsonrpc2"
@@ -16,20 +15,13 @@ import (
 
 var wireIDTestData = []struct {
 	name    string
-	id      *jsonrpc2.ID
+	id      jsonrpc2.ID
 	encoded []byte
 	plain   string
 	quoted  string
 }{
 	{
-		name:    `nil`,
-		id:      nil,
-		encoded: []byte(`null`),
-		plain:   fmt.Sprintf(`%d`, int64(math.MaxInt64)),
-		quoted:  fmt.Sprintf(`#%d`, int64(math.MaxInt64)),
-	}, {
 		name:    `empty`,
-		id:      &jsonrpc2.ID{},
 		encoded: []byte(`0`),
 		plain:   `0`,
 		quoted:  `#0`,
@@ -64,7 +56,7 @@ func TestIDFormat(t *testing.T) {
 func TestIDEncode(t *testing.T) {
 	for _, test := range wireIDTestData {
 		t.Run(test.name, func(t *testing.T) {
-			data, err := json.Marshal(test.id)
+			data, err := json.Marshal(&test.id)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -81,12 +73,8 @@ func TestIDDecode(t *testing.T) {
 				t.Fatal(err)
 			}
 			if got == nil {
-				if test.id != nil {
-					t.Errorf("got nil want %s", test.id)
-				}
-			} else if test.id == nil {
-				t.Errorf("got %s want nil", got)
-			} else if *got != *test.id {
+				t.Errorf("got nil want %s", test.id)
+			} else if *got != test.id {
 				t.Errorf("got %s want %s", got, test.id)
 			}
 		})

internal/lsp/protocol/log.go

@@ -27,7 +27,7 @@ func (s *loggingStream) Read(ctx context.Context) (jsonrpc2.Message, int64, erro
 	if err == nil {
 		s.logMu.Lock()
 		defer s.logMu.Unlock()
-		logIn(s.log, msg)
+		logCommon(s.log, msg, true)
 	}
 	return msg, count, err
 }
@@ -35,7 +35,7 @@ func (s *loggingStream) Read(ctx context.Context) (jsonrpc2.Message, int64, erro
 func (s *loggingStream) Write(ctx context.Context, msg jsonrpc2.Message) (int64, error) {
 	s.logMu.Lock()
 	defer s.logMu.Unlock()
-	logOut(s.log, msg)
+	logCommon(s.log, msg, false)
 	count, err := s.stream.Write(ctx, msg)
 	return count, err
 }
@@ -60,23 +60,19 @@ var maps = &mapped{
 // these 4 methods are each used exactly once, but it seemed
 // better to have the encapsulation rather than ad hoc mutex
 // code in 4 places
-func (m *mapped) client(id string, del bool) req {
+func (m *mapped) client(id string) req {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	v := m.clientCalls[id]
-	if del {
-		delete(m.clientCalls, id)
-	}
+	delete(m.clientCalls, id)
 	return v
 }
 
-func (m *mapped) server(id string, del bool) req {
+func (m *mapped) server(id string) req {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	v := m.serverCalls[id]
-	if del {
-		delete(m.serverCalls, id)
-	}
+	delete(m.serverCalls, id)
 	return v
 }
 
@@ -94,7 +90,13 @@ func (m *mapped) setServer(id string, r req) {
 
 const eor = "\r\n\r\n\r\n"
 
-func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense string) {
+func logCommon(outfd io.Writer, msg jsonrpc2.Message, isRead bool) {
+	direction, pastTense := "Received", "Received"
+	get, set := maps.client, maps.setServer
+	if isRead {
+		direction, pastTense = "Sending", "Sent"
+		get, set = maps.server, maps.setClient
+	}
 	if msg == nil || outfd == nil {
 		return
 	}
@@ -108,7 +110,7 @@ func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense strin
 		id := fmt.Sprint(msg.ID())
 		fmt.Fprintf(&buf, "%s request '%s - (%s)'.\n", direction, msg.Method(), id)
 		fmt.Fprintf(&buf, "Params: %s%s", msg.Params(), eor)
-		maps.setServer(id, req{method: msg.Method(), start: tm})
+		set(id, req{method: msg.Method(), start: tm})
 	case *jsonrpc2.Notification:
 		fmt.Fprintf(&buf, "%s notification '%s'.\n", direction, msg.Method())
 		fmt.Fprintf(&buf, "Params: %s%s", msg.Params(), eor)
@@ -118,21 +120,11 @@ func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense strin
 			fmt.Fprintf(outfd, "[Error - %s] %s #%s %s%s", pastTense, tmfmt, id, err, eor)
 			return
 		}
-		cc := maps.client(id, true)
+		cc := get(id)
 		elapsed := tm.Sub(cc.start)
-		fmt.Fprintf(&buf, "Received response '%s - (%s)' in %dms.\n",
-			cc.method, id, elapsed/time.Millisecond)
+		fmt.Fprintf(&buf, "%s response '%s - (%s)' in %dms.\n",
+			direction, cc.method, id, elapsed/time.Millisecond)
 		fmt.Fprintf(&buf, "Result: %s%s", msg.Result(), eor)
 	}
 	outfd.Write([]byte(buf.String()))
 }
-
-// Writing a message to the client, log it
-func logOut(outfd io.Writer, msg jsonrpc2.Message) {
-	logCommon(outfd, msg, "Received", "Received")
-}
-
-// Got a message from the client, log it
-func logIn(outfd io.Writer, msg jsonrpc2.Message) {
-	logCommon(outfd, msg, "Sending", "Sent")
-}

internal/lsp/protocol/protocol.go

@@ -49,9 +49,9 @@ func CancelHandler(handler jsonrpc2.Handler) jsonrpc2.Handler {
 			return sendParseError(ctx, reply, err)
 		}
 		if n, ok := params.ID.(float64); ok {
-			canceller(*jsonrpc2.NewIntID(int64(n)))
+			canceller(jsonrpc2.NewIntID(int64(n)))
 		} else if s, ok := params.ID.(string); ok {
-			canceller(*jsonrpc2.NewStringID(s))
+			canceller(jsonrpc2.NewStringID(s))
 		} else {
 			return sendParseError(ctx, reply, fmt.Errorf("request ID %v malformed", params.ID))
 		}