From 3ded1b734dda93b8497ced99de9e917f75394741 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 10 Jan 2020 17:54:47 -0500 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cmd/check.go | 17 ++++------- internal/lsp/cmd/cmd.go | 51 +++++++++++++------------------ internal/lsp/cmd/suggested_fix.go | 11 ++----- internal/lsp/server.go | 18 +++++++++++ 4 files changed, 48 insertions(+), 49 deletions(-) diff --git a/internal/lsp/cmd/check.go b/internal/lsp/cmd/check.go index 1a8961e6c3..384a3239b5 100644 --- a/internal/lsp/cmd/check.go +++ b/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) diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index f36311bc4c..2c5573bc5c 100644 --- a/internal/lsp/cmd/cmd.go +++ b/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 diff --git a/internal/lsp/cmd/suggested_fix.go b/internal/lsp/cmd/suggested_fix.go index 15c6bc9983..034cd62a33 100644 --- a/internal/lsp/cmd/suggested_fix.go +++ b/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), diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 0d2532837c..417efe7ce8 100644 --- a/internal/lsp/server.go +++ b/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) }