gopls/internal/cmd: cleanup progress handling
This CL adds connection.registerProgressHandler so that
each call to, say, executeCommand can temporarily install
a handler for progress notifications bearing the token
passed to executeCommand.
However, the initial workspace load must be treated a little
specially since it occurs during the initialize RPC, which
happens before the newConnection constructor has returned.
So, tracking the IWL is now a core feature of connection
instead of something local to the 'stats' subcommand.
Also, make the "asynchronous" property a declarative
attribute of each command.Command so that both the server
and the client can do the right thing--start a thread,
wait for the 'end' notification--based on the metadata.
Also:
- use connection.executeCommand in all cases instead of
direct call to ExecuteCommand.
- merge Application.connect{,remote} and factor common logic.
Change-Id: If7a000593ef4d4dc5658423a03c56b2f4f3a06ca
Reviewed-on: https://go-review.googlesource.com/c/tools/+/591095
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md
index c1c5bea..1e15310 100644
--- a/gopls/doc/commands.md
+++ b/gopls/doc/commands.md
@@ -534,6 +534,8 @@
Run vulnerability check (`govulncheck`).
+This command is asynchronous; clients must wait for the 'end' progress notification.
+
Args:
```
@@ -560,6 +562,8 @@
Runs `go test` for a specific set of test or benchmark functions.
+This command is asynchronous; clients must wait for the 'end' progress notification.
+
Args:
```
@@ -665,6 +669,13 @@
Runs `go test` for a specific set of test or benchmark functions.
+This command is asynchronous; wait for the 'end' progress notification.
+
+This command is an alias for RunTests; the only difference
+is the form of the parameters.
+
+TODO(adonovan): eliminate it.
+
Args:
```
diff --git a/gopls/internal/cmd/call_hierarchy.go b/gopls/internal/cmd/call_hierarchy.go
index 82c18d0..0ac6956 100644
--- a/gopls/internal/cmd/call_hierarchy.go
+++ b/gopls/internal/cmd/call_hierarchy.go
@@ -39,7 +39,7 @@
return tool.CommandLineErrorf("call_hierarchy expects 1 argument (position)")
}
- conn, err := c.app.connect(ctx, nil)
+ conn, err := c.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/capabilities_test.go b/gopls/internal/cmd/capabilities_test.go
index e043f68..97eb496 100644
--- a/gopls/internal/cmd/capabilities_test.go
+++ b/gopls/internal/cmd/capabilities_test.go
@@ -48,7 +48,7 @@
// Send an initialize request to the server.
ctx := context.Background()
- client := newClient(app, nil)
+ client := newClient(app)
options := settings.DefaultOptions(app.options)
server := server.New(cache.NewSession(ctx, cache.New(nil)), client, options)
result, err := server.Initialize(ctx, params)
diff --git a/gopls/internal/cmd/check.go b/gopls/internal/cmd/check.go
index 7a0d19f..a859ed2 100644
--- a/gopls/internal/cmd/check.go
+++ b/gopls/internal/cmd/check.go
@@ -51,7 +51,7 @@
opts.RelatedInformationSupported = true
}
- conn, err := c.app.connect(ctx, nil)
+ conn, err := c.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/cmd.go b/gopls/internal/cmd/cmd.go
index ba3a0b1..b3e5dbe 100644
--- a/gopls/internal/cmd/cmd.go
+++ b/gopls/internal/cmd/cmd.go
@@ -12,6 +12,7 @@
"flag"
"fmt"
"log"
+ "math/rand"
"os"
"path/filepath"
"reflect"
@@ -309,40 +310,31 @@
)
// connect creates and initializes a new in-process gopls session.
-//
-// If onProgress is set, it is called for each new progress notification.
-func (app *Application) connect(ctx context.Context, onProgress func(*protocol.ProgressParams)) (*connection, error) {
- switch {
- case app.Remote == "":
- client := newClient(app, onProgress)
+func (app *Application) connect(ctx context.Context) (*connection, error) {
+ client := newClient(app)
+ var svr protocol.Server
+ if app.Remote == "" {
+ // local
options := settings.DefaultOptions(app.options)
- server := server.New(cache.NewSession(ctx, cache.New(nil)), client, options)
- conn := newConnection(server, client)
- if err := conn.initialize(protocol.WithClient(ctx, client), app.options); err != nil {
+ svr = server.New(cache.NewSession(ctx, cache.New(nil)), client, options)
+ ctx = protocol.WithClient(ctx, client)
+
+ } else {
+ // remote
+ netConn, err := lsprpc.ConnectToRemote(ctx, app.Remote)
+ if err != nil {
return nil, err
}
- return conn, nil
-
- default:
- return app.connectRemote(ctx, app.Remote)
+ stream := jsonrpc2.NewHeaderStream(netConn)
+ jsonConn := jsonrpc2.NewConn(stream)
+ svr = protocol.ServerDispatcher(jsonConn)
+ ctx = protocol.WithClient(ctx, client)
+ jsonConn.Go(ctx,
+ protocol.Handlers(
+ protocol.ClientHandler(client, jsonrpc2.MethodNotFound)))
}
-}
-
-func (app *Application) connectRemote(ctx context.Context, remote string) (*connection, error) {
- conn, err := lsprpc.ConnectToRemote(ctx, remote)
- if err != nil {
- return nil, err
- }
- stream := jsonrpc2.NewHeaderStream(conn)
- cc := jsonrpc2.NewConn(stream)
- server := protocol.ServerDispatcher(cc)
- client := newClient(app, nil)
- connection := newConnection(server, client)
- ctx = protocol.WithClient(ctx, connection.client)
- cc.Go(ctx,
- protocol.Handlers(
- protocol.ClientHandler(client, jsonrpc2.MethodNotFound)))
- return connection, connection.initialize(ctx, app.options)
+ conn := newConnection(svr, client)
+ return conn, conn.initialize(ctx, app.options)
}
func (c *connection) initialize(ctx context.Context, options func(*settings.Options)) error {
@@ -368,12 +360,7 @@
params.Capabilities.TextDocument.SemanticTokens.Requests.Full = &protocol.Or_ClientSemanticTokensRequestOptions_full{Value: true}
params.Capabilities.TextDocument.SemanticTokens.TokenTypes = protocol.SemanticTypes()
params.Capabilities.TextDocument.SemanticTokens.TokenModifiers = protocol.SemanticModifiers()
-
- // If the subcommand has registered a progress handler, report the progress
- // capability.
- if c.client.onProgress != nil {
- params.Capabilities.Window.WorkDoneProgress = true
- }
+ params.Capabilities.Window.WorkDoneProgress = true
params.InitializationOptions = map[string]interface{}{
"symbolMatcher": string(opts.SymbolMatcher),
@@ -392,10 +379,35 @@
client *cmdClient
}
+// registerProgressHandler registers a handler for progress notifications.
+// The caller must call unregister when the handler is no longer needed.
+func (cli *cmdClient) registerProgressHandler(handler func(*protocol.ProgressParams)) (token protocol.ProgressToken, unregister func()) {
+ token = fmt.Sprintf("tok%d", rand.Uint64())
+
+ // register
+ cli.progressHandlersMu.Lock()
+ if cli.progressHandlers == nil {
+ cli.progressHandlers = make(map[protocol.ProgressToken]func(*protocol.ProgressParams))
+ }
+ cli.progressHandlers[token] = handler
+ cli.progressHandlersMu.Unlock()
+
+ unregister = func() {
+ cli.progressHandlersMu.Lock()
+ delete(cli.progressHandlers, token)
+ cli.progressHandlersMu.Unlock()
+ }
+ return token, unregister
+}
+
// cmdClient defines the protocol.Client interface behavior of the gopls CLI tool.
type cmdClient struct {
- app *Application
- onProgress func(*protocol.ProgressParams)
+ app *Application
+
+ progressHandlersMu sync.Mutex
+ progressHandlers map[protocol.ProgressToken]func(*protocol.ProgressParams)
+ iwlToken protocol.ProgressToken
+ iwlDone chan struct{}
filesMu sync.Mutex // guards files map
files map[protocol.DocumentURI]*cmdFile
@@ -409,11 +421,11 @@
diagnostics []protocol.Diagnostic
}
-func newClient(app *Application, onProgress func(*protocol.ProgressParams)) *cmdClient {
+func newClient(app *Application) *cmdClient {
return &cmdClient{
- app: app,
- onProgress: onProgress,
- files: make(map[protocol.DocumentURI]*cmdFile),
+ app: app,
+ files: make(map[protocol.DocumentURI]*cmdFile),
+ iwlDone: make(chan struct{}),
}
}
@@ -674,12 +686,43 @@
}
func (c *cmdClient) Progress(_ context.Context, params *protocol.ProgressParams) error {
- if c.onProgress != nil {
- c.onProgress(params)
+ token, ok := params.Token.(string)
+ if !ok {
+ return fmt.Errorf("unexpected progress token: %[1]T %[1]v", params.Token)
}
+
+ c.progressHandlersMu.Lock()
+ handler := c.progressHandlers[token]
+ c.progressHandlersMu.Unlock()
+ if handler == nil {
+ handler = c.defaultProgressHandler
+ }
+ handler(params)
return nil
}
+// defaultProgressHandler is the default handler of progress messages,
+// used during the initialize request.
+func (c *cmdClient) defaultProgressHandler(params *protocol.ProgressParams) {
+ switch v := params.Value.(type) {
+ case *protocol.WorkDoneProgressBegin:
+ if v.Title == server.DiagnosticWorkTitle(server.FromInitialWorkspaceLoad) {
+ c.progressHandlersMu.Lock()
+ c.iwlToken = params.Token
+ c.progressHandlersMu.Unlock()
+ }
+
+ case *protocol.WorkDoneProgressEnd:
+ c.progressHandlersMu.Lock()
+ iwlToken := c.iwlToken
+ c.progressHandlersMu.Unlock()
+
+ if params.Token == iwlToken {
+ close(c.iwlDone)
+ }
+ }
+}
+
func (c *cmdClient) ShowDocument(ctx context.Context, params *protocol.ShowDocumentParams) (*protocol.ShowDocumentResult, error) {
var success bool
if params.External {
@@ -781,10 +824,7 @@
if err != nil {
return err
}
- _, err = c.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
- Command: cmd.Command,
- Arguments: cmd.Arguments,
- })
+ _, err = c.executeCommand(ctx, &cmd)
return err
}
diff --git a/gopls/internal/cmd/codelens.go b/gopls/internal/cmd/codelens.go
index a7017d8..b07f154 100644
--- a/gopls/internal/cmd/codelens.go
+++ b/gopls/internal/cmd/codelens.go
@@ -83,10 +83,7 @@
opts.Codelenses[protocol.CodeLensTest] = true
}
- // TODO(adonovan): cleanup: factor progress with stats subcommand.
- cmdDone, onProgress := commandProgress()
-
- conn, err := r.app.connect(ctx, onProgress)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
@@ -125,7 +122,7 @@
// -exec: run the first matching code lens.
if r.Exec {
- _, err := conn.executeCommand(ctx, cmdDone, lens.Command)
+ _, err := conn.executeCommand(ctx, lens.Command)
return err
}
diff --git a/gopls/internal/cmd/definition.go b/gopls/internal/cmd/definition.go
index e5e119b..d9cd985 100644
--- a/gopls/internal/cmd/definition.go
+++ b/gopls/internal/cmd/definition.go
@@ -73,7 +73,7 @@
o.PreferredContentFormat = protocol.Markdown
}
}
- conn, err := d.app.connect(ctx, nil)
+ conn, err := d.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/execute.go b/gopls/internal/cmd/execute.go
index 381c2a7..485e6d0 100644
--- a/gopls/internal/cmd/execute.go
+++ b/gopls/internal/cmd/execute.go
@@ -76,14 +76,13 @@
e.app.editFlags = &e.EditFlags // in case command performs an edit
- cmdDone, onProgress := commandProgress()
- conn, err := e.app.connect(ctx, onProgress)
+ conn, err := e.app.connect(ctx)
if err != nil {
return err
}
defer conn.terminate(ctx)
- res, err := conn.executeCommand(ctx, cmdDone, &protocol.Command{
+ res, err := conn.executeCommand(ctx, &protocol.Command{
Command: cmd,
Arguments: jsonArgs,
})
@@ -100,55 +99,40 @@
return nil
}
-// -- shared command helpers --
-
-const cmdProgressToken = "cmd-progress"
-
-// TODO(adonovan): disentangle this from app.connect, and factor with
-// conn.executeCommand used by codelens and execute. Seems like
-// connection needs a way to register and unregister independent
-// handlers, later than at connect time.
-func commandProgress() (<-chan bool, func(p *protocol.ProgressParams)) {
- cmdDone := make(chan bool, 1)
- onProgress := func(p *protocol.ProgressParams) {
- switch v := p.Value.(type) {
+// executeCommand executes a protocol.Command, displaying progress
+// messages and awaiting completion of asynchronous commands.
+func (conn *connection) executeCommand(ctx context.Context, cmd *protocol.Command) (any, error) {
+ endStatus := make(chan string, 1)
+ token, unregister := conn.client.registerProgressHandler(func(params *protocol.ProgressParams) {
+ switch v := params.Value.(type) {
case *protocol.WorkDoneProgressReport:
- // TODO(adonovan): how can we segregate command's stdout and
- // stderr so that structure is preserved?
- fmt.Fprintln(os.Stderr, v.Message)
+ fmt.Fprintln(os.Stderr, v.Message) // combined std{out,err}
case *protocol.WorkDoneProgressEnd:
- if p.Token == cmdProgressToken {
- // commandHandler.run sends message = canceled | failed | completed
- cmdDone <- v.Message == server.CommandCompleted
- }
+ endStatus <- v.Message // = canceled | failed | completed
}
- }
- return cmdDone, onProgress
-}
+ })
+ defer unregister()
-func (conn *connection) executeCommand(ctx context.Context, done <-chan bool, cmd *protocol.Command) (any, error) {
res, err := conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
Command: cmd.Command,
Arguments: cmd.Arguments,
WorkDoneProgressParams: protocol.WorkDoneProgressParams{
- WorkDoneToken: cmdProgressToken,
+ WorkDoneToken: token,
},
})
if err != nil {
return nil, err
}
- // Wait for it to finish (by watching for a progress token).
- //
- // In theory this is only necessary for the two async
- // commands (RunGovulncheck and RunTests), but the tests
- // fail for Test as well (why?), and there is no cost to
- // waiting in all cases. TODO(adonovan): investigate.
- if success := <-done; !success {
- // TODO(adonovan): suppress this message;
- // the command's stderr should suffice.
- return nil, fmt.Errorf("command failed")
+ // Some commands are asynchronous, so clients
+ // must wait for the "end" progress notification.
+ enum := command.Command(strings.TrimPrefix(cmd.Command, "gopls."))
+ if enum.IsAsync() {
+ status := <-endStatus
+ if status != server.CommandCompleted {
+ return nil, fmt.Errorf("command %s", status)
+ }
}
return res, nil
diff --git a/gopls/internal/cmd/folding_range.go b/gopls/internal/cmd/folding_range.go
index 13f78c1..f48feee 100644
--- a/gopls/internal/cmd/folding_range.go
+++ b/gopls/internal/cmd/folding_range.go
@@ -36,7 +36,7 @@
return tool.CommandLineErrorf("folding_ranges expects 1 argument (file)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/format.go b/gopls/internal/cmd/format.go
index 75982c9..eb68d73 100644
--- a/gopls/internal/cmd/format.go
+++ b/gopls/internal/cmd/format.go
@@ -42,7 +42,7 @@
return nil
}
c.app.editFlags = &c.EditFlags
- conn, err := c.app.connect(ctx, nil)
+ conn, err := c.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/highlight.go b/gopls/internal/cmd/highlight.go
index 9c1488b..43af063 100644
--- a/gopls/internal/cmd/highlight.go
+++ b/gopls/internal/cmd/highlight.go
@@ -38,7 +38,7 @@
return tool.CommandLineErrorf("highlight expects 1 argument (position)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/implementation.go b/gopls/internal/cmd/implementation.go
index fcfb631..8580265 100644
--- a/gopls/internal/cmd/implementation.go
+++ b/gopls/internal/cmd/implementation.go
@@ -39,7 +39,7 @@
return tool.CommandLineErrorf("implementation expects 1 argument (position)")
}
- conn, err := i.app.connect(ctx, nil)
+ conn, err := i.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/imports.go b/gopls/internal/cmd/imports.go
index 12b49ef..c64c871 100644
--- a/gopls/internal/cmd/imports.go
+++ b/gopls/internal/cmd/imports.go
@@ -43,7 +43,7 @@
return tool.CommandLineErrorf("imports expects 1 argument")
}
t.app.editFlags = &t.EditFlags
- conn, err := t.app.connect(ctx, nil)
+ conn, err := t.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/links.go b/gopls/internal/cmd/links.go
index 0f1d671..3c14f4e 100644
--- a/gopls/internal/cmd/links.go
+++ b/gopls/internal/cmd/links.go
@@ -44,7 +44,7 @@
if len(args) != 1 {
return tool.CommandLineErrorf("links expects 1 argument")
}
- conn, err := l.app.connect(ctx, nil)
+ conn, err := l.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/prepare_rename.go b/gopls/internal/cmd/prepare_rename.go
index c7901e6..3ff3835 100644
--- a/gopls/internal/cmd/prepare_rename.go
+++ b/gopls/internal/cmd/prepare_rename.go
@@ -43,7 +43,7 @@
return tool.CommandLineErrorf("prepare_rename expects 1 argument (file)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/references.go b/gopls/internal/cmd/references.go
index 3c294c7..1483bf1 100644
--- a/gopls/internal/cmd/references.go
+++ b/gopls/internal/cmd/references.go
@@ -43,7 +43,7 @@
return tool.CommandLineErrorf("references expects 1 argument (position)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/rename.go b/gopls/internal/cmd/rename.go
index 6d83168..e96850c 100644
--- a/gopls/internal/cmd/rename.go
+++ b/gopls/internal/cmd/rename.go
@@ -45,7 +45,7 @@
return tool.CommandLineErrorf("rename expects 2 arguments (position, new name)")
}
r.app.editFlags = &r.EditFlags
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/semantictokens.go b/gopls/internal/cmd/semantictokens.go
index 5729621..77e8a03 100644
--- a/gopls/internal/cmd/semantictokens.go
+++ b/gopls/internal/cmd/semantictokens.go
@@ -71,7 +71,7 @@
}
opts.SemanticTokens = true
}
- conn, err := c.app.connect(ctx, nil)
+ conn, err := c.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/signature.go b/gopls/internal/cmd/signature.go
index cf976a6..601cfaa 100644
--- a/gopls/internal/cmd/signature.go
+++ b/gopls/internal/cmd/signature.go
@@ -38,7 +38,7 @@
return tool.CommandLineErrorf("signature expects 1 argument (position)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/stats.go b/gopls/internal/cmd/stats.go
index 8da1a1a..a788801 100644
--- a/gopls/internal/cmd/stats.go
+++ b/gopls/internal/cmd/stats.go
@@ -16,13 +16,11 @@
"reflect"
"runtime"
"strings"
- "sync"
"time"
"golang.org/x/tools/gopls/internal/filecache"
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/protocol/command"
- "golang.org/x/tools/gopls/internal/server"
"golang.org/x/tools/gopls/internal/settings"
bugpkg "golang.org/x/tools/gopls/internal/util/bug"
versionpkg "golang.org/x/tools/gopls/internal/version"
@@ -85,30 +83,6 @@
}
o.VerboseWorkDoneProgress = true
}
- var (
- iwlMu sync.Mutex
- iwlToken protocol.ProgressToken
- iwlDone = make(chan struct{})
- )
-
- onProgress := func(p *protocol.ProgressParams) {
- switch v := p.Value.(type) {
- case *protocol.WorkDoneProgressBegin:
- if v.Title == server.DiagnosticWorkTitle(server.FromInitialWorkspaceLoad) {
- iwlMu.Lock()
- iwlToken = p.Token
- iwlMu.Unlock()
- }
- case *protocol.WorkDoneProgressEnd:
- iwlMu.Lock()
- tok := iwlToken
- iwlMu.Unlock()
-
- if p.Token == tok {
- close(iwlDone)
- }
- }
- }
// do executes a timed section of the stats command.
do := func(name string, f func() error) (time.Duration, error) {
@@ -123,25 +97,25 @@
}
var conn *connection
- iwlDuration, err := do("Initializing workspace", func() error {
- var err error
- conn, err = s.app.connect(ctx, onProgress)
+ iwlDuration, err := do("Initializing workspace", func() (err error) {
+ conn, err = s.app.connect(ctx)
if err != nil {
return err
}
select {
- case <-iwlDone:
+ case <-conn.client.iwlDone:
case <-ctx.Done():
return ctx.Err()
}
return nil
})
- stats.InitialWorkspaceLoadDuration = fmt.Sprint(iwlDuration)
if err != nil {
return err
}
defer conn.terminate(ctx)
+ stats.InitialWorkspaceLoadDuration = fmt.Sprint(iwlDuration)
+
// Gather bug reports produced by any process using
// this executable and persisted in the cache.
do("Gathering bug reports", func() error {
@@ -153,7 +127,7 @@
})
if _, err := do("Querying memstats", func() error {
- memStats, err := conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
+ memStats, err := conn.executeCommand(ctx, &protocol.Command{
Command: command.MemStats.ID(),
})
if err != nil {
@@ -166,7 +140,7 @@
}
if _, err := do("Querying workspace stats", func() error {
- wsStats, err := conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
+ wsStats, err := conn.executeCommand(ctx, &protocol.Command{
Command: command.WorkspaceStats.ID(),
})
if err != nil {
diff --git a/gopls/internal/cmd/suggested_fix.go b/gopls/internal/cmd/suggested_fix.go
index e066460..a326c92 100644
--- a/gopls/internal/cmd/suggested_fix.go
+++ b/gopls/internal/cmd/suggested_fix.go
@@ -75,7 +75,7 @@
return tool.CommandLineErrorf("fix expects at least 1 argument")
}
s.app.editFlags = &s.EditFlags
- conn, err := s.app.connect(ctx, nil)
+ conn, err := s.app.connect(ctx)
if err != nil {
return err
}
@@ -136,10 +136,7 @@
// This may cause the server to make
// an ApplyEdit downcall to the client.
if a.Command != nil {
- if _, err := conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
- Command: a.Command.Command,
- Arguments: a.Command.Arguments,
- }); err != nil {
+ if _, err := conn.executeCommand(ctx, a.Command); err != nil {
return err
}
// The specification says that commands should
diff --git a/gopls/internal/cmd/symbols.go b/gopls/internal/cmd/symbols.go
index 249397d..663a08f 100644
--- a/gopls/internal/cmd/symbols.go
+++ b/gopls/internal/cmd/symbols.go
@@ -36,7 +36,7 @@
return tool.CommandLineErrorf("symbols expects 1 argument (position)")
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/cmd/workspace_symbol.go b/gopls/internal/cmd/workspace_symbol.go
index 9fa7526..aba33fa 100644
--- a/gopls/internal/cmd/workspace_symbol.go
+++ b/gopls/internal/cmd/workspace_symbol.go
@@ -59,7 +59,7 @@
}
}
- conn, err := r.app.connect(ctx, nil)
+ conn, err := r.app.connect(ctx)
if err != nil {
return err
}
diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json
index bcb2610..e170385 100644
--- a/gopls/internal/doc/api.json
+++ b/gopls/internal/doc/api.json
@@ -1089,14 +1089,14 @@
{
"Command": "gopls.run_govulncheck",
"Title": "Run vulncheck",
- "Doc": "Run vulnerability check (`govulncheck`).",
+ "Doc": "Run vulnerability check (`govulncheck`).\n\nThis command is asynchronous; clients must wait for the 'end' progress notification.",
"ArgDoc": "{\n\t// Any document in the directory from which govulncheck will run.\n\t\"URI\": string,\n\t// Package pattern. E.g. \"\", \".\", \"./...\".\n\t\"Pattern\": string,\n}",
"ResultDoc": "{\n\t// Token holds the progress token for LSP workDone reporting of the vulncheck\n\t// invocation.\n\t\"Token\": interface{},\n}"
},
{
"Command": "gopls.run_tests",
"Title": "Run test(s)",
- "Doc": "Runs `go test` for a specific set of test or benchmark functions.",
+ "Doc": "Runs `go test` for a specific set of test or benchmark functions.\n\nThis command is asynchronous; clients must wait for the 'end' progress notification.",
"ArgDoc": "{\n\t// The test file containing the tests to run.\n\t\"URI\": string,\n\t// Specific test names to run, e.g. TestFoo.\n\t\"Tests\": []string,\n\t// Specific benchmarks to run, e.g. BenchmarkFoo.\n\t\"Benchmarks\": []string,\n}",
"ResultDoc": ""
},
@@ -1124,7 +1124,7 @@
{
"Command": "gopls.test",
"Title": "Run test(s) (legacy)",
- "Doc": "Runs `go test` for a specific set of test or benchmark functions.",
+ "Doc": "Runs `go test` for a specific set of test or benchmark functions.\n\nThis command is asynchronous; wait for the 'end' progress notification.\n\nThis command is an alias for RunTests; the only difference\nis the form of the parameters.\n\nTODO(adonovan): eliminate it.",
"ArgDoc": "string,\n[]string,\n[]string",
"ResultDoc": ""
},
diff --git a/gopls/internal/protocol/command/interface.go b/gopls/internal/protocol/command/interface.go
index 0f44026..0934cf2 100644
--- a/gopls/internal/protocol/command/interface.go
+++ b/gopls/internal/protocol/command/interface.go
@@ -49,13 +49,20 @@
// Test: Run test(s) (legacy)
//
// Runs `go test` for a specific set of test or benchmark functions.
+ //
+ // This command is asynchronous; wait for the 'end' progress notification.
+ //
+ // This command is an alias for RunTests; the only difference
+ // is the form of the parameters.
+ //
+ // TODO(adonovan): eliminate it.
Test(context.Context, protocol.DocumentURI, []string, []string) error
- // TODO: deprecate Test in favor of RunTests below.
-
// Test: Run test(s)
//
// Runs `go test` for a specific set of test or benchmark functions.
+ //
+ // This command is asynchronous; clients must wait for the 'end' progress notification.
RunTests(context.Context, RunTestsArgs) error
// Generate: Run go generate
@@ -178,6 +185,8 @@
// RunGovulncheck: Run vulncheck
//
// Run vulnerability check (`govulncheck`).
+ //
+ // This command is asynchronous; clients must wait for the 'end' progress notification.
RunGovulncheck(context.Context, VulncheckArgs) (RunVulncheckResult, error)
// FetchVulncheckResult: Get known vulncheck result
diff --git a/gopls/internal/protocol/command/util.go b/gopls/internal/protocol/command/util.go
index f49e040..0d9799f 100644
--- a/gopls/internal/protocol/command/util.go
+++ b/gopls/internal/protocol/command/util.go
@@ -9,6 +9,11 @@
"fmt"
)
+// TODO(adonovan): use the "gopls."-prefix forms everywhere. The
+// existence of two forms is a nuisance. I think it should be a
+// straightforward fix, as all public APIs and docs use that form
+// already.
+
// ID returns the command name for use in the LSP.
func ID(name string) string {
return "gopls." + name
@@ -16,6 +21,18 @@
type Command string
+// IsAsync reports whether the command is asynchronous:
+// clients must wait for the "end" progress notification.
+func (c Command) IsAsync() bool {
+ switch string(c) {
+ // TODO(adonovan): derive this list from interface.go somewhow.
+ // Unfortunately we can't even reference the enum from here...
+ case "run_tests", "run_govulncheck", "test":
+ return true
+ }
+ return false
+}
+
// ID returns the command identifier to use in the executeCommand request.
func (c Command) ID() string {
return ID(string(c))
diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go
index f7bf1aa..ad2eb95 100644
--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -12,6 +12,7 @@
"fmt"
"go/ast"
"io"
+ "log"
"os"
"path/filepath"
"regexp"
@@ -93,18 +94,8 @@
// commandConfig configures common command set-up and execution.
type commandConfig struct {
- // TODO(adonovan): whether a command is synchronous or
- // asynchronous is part of the server interface contract, not
- // a mere implementation detail of the handler.
- // Export a (command.Command).IsAsync() property so that
- // clients can tell. (The tricky part is ensuring the handler
- // remains consistent with the command.Command metadata, as at
- // the point were we read the 'async' field below, we no
- // longer know that command.Command.)
-
- async bool // whether to run the command asynchronously. Async commands can only return errors.
requireSave bool // whether all files must be saved for the command to work
- progress string // title to use for progress reporting. If empty, no progress will be reported.
+ progress string // title to use for progress reporting. If empty, no progress will be reported. Mandatory for async commands.
forView string // view to resolve to a snapshot; incompatible with forURI
forURI protocol.DocumentURI // URI to resolve to a snapshot. If unset, snapshot will be nil.
}
@@ -115,7 +106,7 @@
type commandDeps struct {
snapshot *cache.Snapshot // present if cfg.forURI was set
fh file.Handle // present if cfg.forURI was set
- work *progress.WorkDone // present cfg.progress was set
+ work *progress.WorkDone // present if cfg.progress was set
}
type commandFunc func(context.Context, commandDeps) error
@@ -195,7 +186,13 @@
}
return err
}
- if cfg.async {
+
+ enum := command.Command(strings.TrimPrefix(c.params.Command, "gopls."))
+ if enum.IsAsync() {
+ if cfg.progress == "" {
+ log.Fatalf("asynchronous command %q does not enable progress reporting",
+ enum)
+ }
go func() {
if err := runcmd(); err != nil {
showMessage(ctx, c.s.client, protocol.Error, err.Error())
@@ -491,6 +488,7 @@
return protocol.EditsFromDiffEdits(pm.Mapper, diff)
}
+// Test is an alias for RunTests (with splayed arguments).
func (c *commandHandler) Test(ctx context.Context, uri protocol.DocumentURI, tests, benchmarks []string) error {
return c.RunTests(ctx, command.RunTestsArgs{
URI: uri,
@@ -583,9 +581,8 @@
func (c *commandHandler) RunTests(ctx context.Context, args command.RunTestsArgs) error {
return c.run(ctx, commandConfig{
- async: true,
- progress: "Running go test",
- requireSave: true, // go test honors overlays, but tests themselves cannot
+ progress: "Running go test", // (asynchronous)
+ requireSave: true, // go test honors overlays, but tests themselves cannot
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
return c.runTests(ctx, deps.snapshot, deps.work, args.URI, args.Tests, args.Benchmarks)
@@ -1088,9 +1085,8 @@
// synchronize the start of the run and return the token.
tokenChan := make(chan protocol.ProgressToken, 1)
err := c.run(ctx, commandConfig{
- async: true, // need to be async to be cancellable
- progress: "govulncheck",
- requireSave: true, // govulncheck cannot honor overlays
+ progress: "govulncheck", // (asynchronous)
+ requireSave: true, // govulncheck cannot honor overlays
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
tokenChan <- deps.work.Token()
