internal/lsp: add and use nonstandard gopls/diagnoseFiles

Switch the command line client, and its tests, away from the hardcoded
30-second timeout and to a newly-added custom request.

Inconveniently for us, the jsonrpc2 package only serializes requests,
not replies. (Notifications are requests for this purpose.) So, for a
flow like this:

diagnoseFiles -->
  <-- publishDiagnostics
  <-- publishDiagnostics
diagnoseFiles <-- (reply)

...there's actually no guarantee that the incoming requests will be
processed before the reply comes in -- it gets to jump the
serialization. The only way to guarantee previous notifications have
been processed is to send another request. I didn't feel like adding
nonstandard notification support, so I just send a fake diagnostic.

Error handling for untyped JSON is hideous so for now we just panic.
Nobody else should be calling these, or if they do it's at their own
risk.

Fixes golang/go#36518.

Change-Id: I63f8df5bb627c9783314a0d38d2dac8721b3c320
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214583
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cmd/check.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"time"
 
 	"golang.org/x/tools/internal/span"
 	errors "golang.org/x/xerrors"
@@ -41,6 +40,7 @@ func (c *check) Run(ctx context.Context, args ...string) error {
 		return nil
 	}
 	checking := map[span.URI]*cmdFile{}
+	var uris []span.URI
 	// now we ready to kick things off
 	conn, err := c.app.connect(ctx)
 	if err != nil {
@@ -49,23 +49,18 @@ func (c *check) Run(ctx context.Context, args ...string) error {
 	defer conn.terminate(ctx)
 	for _, arg := range args {
 		uri := span.FileURI(arg)
+		uris = append(uris, uri)
 		file := conn.AddFile(ctx, uri)
 		if file.err != nil {
 			return file.err
 		}
 		checking[uri] = file
 	}
-	// now wait for results
-	// TODO: maybe conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{Command: "gopls-wait-idle"})
-	ctx, cancel := context.WithTimeout(ctx, time.Second*30)
-	defer cancel()
-
+	if err := conn.diagnoseFiles(ctx, uris); err != nil {
+		return err
+	}
 	for _, file := range checking {
-		diagnostics, err := file.waitForDiagnostics(ctx)
-		if err != nil {
-			return err
-		}
-		for _, d := range diagnostics {
+		for _, d := range file.diagnostics {
 			spn, err := file.mapper.RangeSpan(d.Range)
 			if err != nil {
 				return errors.Errorf("Could not convert position %v for %q", d.Range, d.Message)

internal/lsp/cmd/cmd.go

@@ -263,18 +263,19 @@ type cmdClient struct {
 	app  *Application
 	fset *token.FileSet
 
+	diagnosticsMu   sync.Mutex
+	diagnosticsDone chan struct{}
+
 	filesMu sync.Mutex
 	files   map[span.URI]*cmdFile
 }
 
 type cmdFile struct {
-	uri            span.URI
-	mapper         *protocol.ColumnMapper
-	err            error
-	added          bool
-	hasDiagnostics chan struct{}
-	diagnosticsMu  sync.Mutex
-	diagnostics    []protocol.Diagnostic
+	uri         span.URI
+	mapper      *protocol.ColumnMapper
+	err         error
+	added       bool
+	diagnostics []protocol.Diagnostic
 }
 
 func newConnection(app *Application) *connection {
@@ -356,6 +357,9 @@ func (c *cmdClient) ApplyEdit(ctx context.Context, p *protocol.ApplyWorkspaceEdi
 }
 
 func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishDiagnosticsParams) error {
+	if p.URI == "gopls://diagnostics-done" {
+		close(c.diagnosticsDone)
+	}
 	// Don't worry about diagnostics without versions.
 	if p.Version == 0 {
 		return nil
@@ -366,15 +370,7 @@ func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishD
 
 	uri := span.URI(p.URI)
 	file := c.getFile(ctx, uri)
-
-	file.diagnosticsMu.Lock()
-	defer file.diagnosticsMu.Unlock()
-
-	hadDiagnostics := file.diagnostics != nil
 	file.diagnostics = p.Diagnostics
-	if !hadDiagnostics {
-		close(file.hasDiagnostics)
-	}
 	return nil
 }
 
@@ -382,8 +378,7 @@ func (c *cmdClient) getFile(ctx context.Context, uri span.URI) *cmdFile {
 	file, found := c.files[uri]
 	if !found || file.err != nil {
 		file = &cmdFile{
-			uri:            uri,
-			hasDiagnostics: make(chan struct{}),
+			uri: uri,
 		}
 		c.files[uri] = file
 	}
@@ -406,18 +401,6 @@ func (c *cmdClient) getFile(ctx context.Context, uri span.URI) *cmdFile {
 	return file
 }
 
-func (file *cmdFile) waitForDiagnostics(ctx context.Context) ([]protocol.Diagnostic, error) {
-	select {
-	case <-ctx.Done():
-		return nil, ctx.Err()
-	case <-file.hasDiagnostics:
-	}
-
-	file.diagnosticsMu.Lock()
-	defer file.diagnosticsMu.Unlock()
-	return file.diagnostics, nil
-}
-
 func (c *connection) AddFile(ctx context.Context, uri span.URI) *cmdFile {
 	c.Client.filesMu.Lock()
 	defer c.Client.filesMu.Unlock()
@@ -448,6 +431,16 @@ func (c *connection) AddFile(ctx context.Context, uri span.URI) *cmdFile {
 	return file
 }
 
+func (c *connection) diagnoseFiles(ctx context.Context, files []span.URI) error {
+	c.Client.diagnosticsMu.Lock()
+	defer c.Client.diagnosticsMu.Unlock()
+
+	c.Client.diagnosticsDone = make(chan struct{})
+	_, err := c.Server.NonstandardRequest(ctx, "gopls/diagnoseFiles", map[string]interface{}{"files": files})
+	<-c.Client.diagnosticsDone
+	return err
+}
+
 func (c *connection) terminate(ctx context.Context) {
 	if c.Client.app.Remote == "internal" {
 		// internal connections need to be left alive for the next test

internal/lsp/cmd/suggested_fix.go

@@ -9,7 +9,6 @@ import (
 	"flag"
 	"fmt"
 	"io/ioutil"
-	"time"
 
 	"golang.org/x/tools/internal/lsp/diff"
 	"golang.org/x/tools/internal/lsp/protocol"
@@ -63,16 +62,10 @@ func (s *suggestedfix) Run(ctx context.Context, args ...string) error {
 		return file.err
 	}
 
-	// Wait for diagnostics results
-	select {
-	case <-file.hasDiagnostics:
-	case <-time.After(30 * time.Second):
-		return errors.Errorf("timed out waiting for results from %v", file.uri)
+	if err := conn.diagnoseFiles(ctx, []span.URI{uri}); err != nil {
+		return err
 	}
 
-	file.diagnosticsMu.Lock()
-	defer file.diagnosticsMu.Unlock()
-
 	p := protocol.CodeActionParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: protocol.NewURI(uri),

internal/lsp/server.go

@@ -290,6 +290,24 @@ func (s *Server) SelectionRange(context.Context, *protocol.SelectionRangeParams)
 
 // Nonstandard requests
 func (s *Server) NonstandardRequest(ctx context.Context, method string, params interface{}) (interface{}, error) {
+	paramMap := params.(map[string]interface{})
+	if method == "gopls/diagnoseFiles" {
+		files := paramMap["files"].([]interface{})
+		for _, file := range files {
+			uri := span.URI(file.(string))
+			view, err := s.session.ViewOf(uri)
+			if err != nil {
+				return nil, err
+			}
+			s.diagnoseFile(view.Snapshot(), view.Session().GetFile(span.URI(uri)))
+		}
+		if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
+			URI: "gopls://diagnostics-done",
+		}); err != nil {
+			return nil, err
+		}
+		return struct{}{}, nil
+	}
 	return nil, notImplemented(method)
 }