gopls/internal/lsp: make gopls.start_debugging open browser
This change makes the server's implementation of the
gopls.start_debugging command send a window/showDocument request
to the client to cause it to open the debug page in a browser.
It also makes the command-line client implement showDocument
by calling browser.Open (if External) or simply logging a
message (otherwise). It also makes the tests' fake client
implement showDocument by recording the event (similar to
showMessage), adds test expectations for such events, and
uses them in an integration test of gopls.start_debugging.
Tested interactively in Emacs+eglot,
and in VS Code with CL 506516 patched in.
Change-Id: I8481c6974425343f14164ed4bee08c129ee7008d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/505875
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/cmd/cmd.go b/gopls/internal/lsp/cmd/cmd.go
index 3a28dc0..3474ed7 100644
--- a/gopls/internal/lsp/cmd/cmd.go
+++ b/gopls/internal/lsp/cmd/cmd.go
@@ -22,6 +22,7 @@
"time"
"golang.org/x/tools/gopls/internal/lsp"
+ "golang.org/x/tools/gopls/internal/lsp/browser"
"golang.org/x/tools/gopls/internal/lsp/cache"
"golang.org/x/tools/gopls/internal/lsp/debug"
"golang.org/x/tools/gopls/internal/lsp/filecache"
@@ -402,6 +403,7 @@
client *cmdClient
}
+// cmdClient defines the protocol.Client interface behavior of the gopls CLI tool.
type cmdClient struct {
app *Application
onProgress func(*protocol.ProgressParams)
@@ -570,8 +572,19 @@
return nil
}
-func (c *cmdClient) ShowDocument(context.Context, *protocol.ShowDocumentParams) (*protocol.ShowDocumentResult, error) {
- return nil, nil
+func (c *cmdClient) ShowDocument(ctx context.Context, params *protocol.ShowDocumentParams) (*protocol.ShowDocumentResult, error) {
+ var success bool
+ if params.External {
+ // Open URI in external browser.
+ success = browser.Open(string(params.URI))
+ } else {
+ // Open file in editor, optionally taking focus and selecting a range.
+ // (cmdClient has no editor. Should it fork+exec $EDITOR?)
+ log.Printf("Server requested that client editor open %q (takeFocus=%t, selection=%+v)",
+ params.URI, params.TakeFocus, params.Selection)
+ success = true
+ }
+ return &protocol.ShowDocumentResult{Success: success}, nil
}
func (c *cmdClient) WorkDoneProgressCreate(context.Context, *protocol.WorkDoneProgressCreateParams) error {
diff --git a/gopls/internal/lsp/cmd/serve.go b/gopls/internal/lsp/cmd/serve.go
index 03cc187..a04e6dc 100644
--- a/gopls/internal/lsp/cmd/serve.go
+++ b/gopls/internal/lsp/cmd/serve.go
@@ -109,6 +109,23 @@
}
if s.Port != 0 {
network = "tcp"
+ // TODO(adonovan): should gopls ever be listening on network
+ // sockets, or only local ones?
+ //
+ // Ian says this was added in anticipation of
+ // something related to "VS Code remote" that turned
+ // out to be unnecessary. So I propose we limit it to
+ // localhost, if only so that we avoid the macOS
+ // firewall prompt.
+ //
+ // Hana says: "s.Address is for the remote access (LSP)
+ // and s.Port is for debugging purpose (according to
+ // the Server type documentation). I am not sure why the
+ // existing code here is mixing up and overwriting addr.
+ // For debugging endpoint, I think localhost makes perfect sense."
+ //
+ // TODO(adonovan): disentangle Address and Port,
+ // and use only localhost for the latter.
addr = fmt.Sprintf(":%v", s.Port)
}
if addr != "" {
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index 7bbadc1..7c6bcfa 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -839,6 +839,7 @@
return result, fmt.Errorf("starting debug server: %w", err)
}
result.URLs = []string{"http://" + listenedAddr}
+ openClientBrowser(ctx, c.s.client, result.URLs[0])
return result, nil
}
@@ -1144,3 +1145,47 @@
}
return nil
}
+
+// openClientBrowser causes the LSP client to open the specified URL
+// in an external browser.
+func openClientBrowser(ctx context.Context, cli protocol.Client, url protocol.URI) {
+ showDocumentImpl(ctx, cli, url, nil)
+}
+
+// openClientEditor causes the LSP client to open the specified document
+// and select the indicated range.
+func openClientEditor(ctx context.Context, cli protocol.Client, loc protocol.Location) {
+ showDocumentImpl(ctx, cli, protocol.URI(loc.URI), &loc.Range)
+}
+
+func showDocumentImpl(ctx context.Context, cli protocol.Client, url protocol.URI, rangeOpt *protocol.Range) {
+ // In principle we shouldn't send a showDocument request to a
+ // client that doesn't support it, as reported by
+ // ShowDocumentClientCapabilities. But even clients that do
+ // support it may defer the real work of opening the document
+ // asynchronously, to avoid deadlocks due to rentrancy.
+ //
+ // For example: client sends request to server; server sends
+ // showDocument to client; client opens editor; editor causes
+ // new RPC to be sent to server, which is still busy with
+ // previous request. (This happens in eglot.)
+ //
+ // So we can't rely on the success/failure information.
+ // That's the reason this function doesn't return an error.
+
+ // "External" means run the system-wide handler (e.g. open(1)
+ // on macOS or xdg-open(1) on Linux) for this URL, ignoring
+ // TakeFocus and Selection. Note that this may still end up
+ // opening the same editor (e.g. VSCode) for a file: URL.
+ res, err := cli.ShowDocument(ctx, &protocol.ShowDocumentParams{
+ URI: url,
+ External: rangeOpt == nil,
+ TakeFocus: true,
+ Selection: rangeOpt, // optional
+ })
+ if err != nil {
+ event.Error(ctx, "client.showDocument: %v", err)
+ } else if res != nil && !res.Success {
+ event.Log(ctx, fmt.Sprintf("client declined to open document %v", url))
+ }
+}
diff --git a/gopls/internal/lsp/fake/client.go b/gopls/internal/lsp/fake/client.go
index 555428e..1c07372 100644
--- a/gopls/internal/lsp/fake/client.go
+++ b/gopls/internal/lsp/fake/client.go
@@ -22,6 +22,7 @@
OnDiagnostics func(context.Context, *protocol.PublishDiagnosticsParams) error
OnWorkDoneProgressCreate func(context.Context, *protocol.WorkDoneProgressCreateParams) error
OnProgress func(context.Context, *protocol.ProgressParams) error
+ OnShowDocument func(context.Context, *protocol.ShowDocumentParams) error
OnShowMessage func(context.Context, *protocol.ShowMessageParams) error
OnShowMessageRequest func(context.Context, *protocol.ShowMessageRequestParams) error
OnRegisterCapability func(context.Context, *protocol.RegistrationParams) error
@@ -162,7 +163,13 @@
return nil
}
-func (c *Client) ShowDocument(context.Context, *protocol.ShowDocumentParams) (*protocol.ShowDocumentResult, error) {
+func (c *Client) ShowDocument(ctx context.Context, params *protocol.ShowDocumentParams) (*protocol.ShowDocumentResult, error) {
+ if c.hooks.OnShowDocument != nil {
+ if err := c.hooks.OnShowDocument(ctx, params); err != nil {
+ return nil, err
+ }
+ return &protocol.ShowDocumentResult{Success: true}, nil
+ }
return nil, nil
}
diff --git a/gopls/internal/lsp/protocol/context.go b/gopls/internal/lsp/protocol/context.go
index 487e4df..a9ef48d 100644
--- a/gopls/internal/lsp/protocol/context.go
+++ b/gopls/internal/lsp/protocol/context.go
@@ -38,6 +38,8 @@
if event.IsError(ev) {
msg.Type = Error
}
+ // TODO(adonovan): the goroutine here could cause log
+ // messages to be delivered out of order! Use a queue.
go client.LogMessage(xcontext.Detach(ctx), msg)
return ctx
}
diff --git a/gopls/internal/lsp/protocol/tsserver.go b/gopls/internal/lsp/protocol/tsserver.go
index 004a2e6..2e78550 100644
--- a/gopls/internal/lsp/protocol/tsserver.go
+++ b/gopls/internal/lsp/protocol/tsserver.go
@@ -1110,6 +1110,13 @@
return s.sender.Notify(ctx, "workspace/didRenameFiles", params)
}
func (s *serverDispatcher) ExecuteCommand(ctx context.Context, params *ExecuteCommandParams) (interface{}, error) {
+ // TODO(adonovan): allow the caller to pass in the result
+ // pointer so they can avoid an unnecessary JSON-to-any
+ // decoding and re-encoding to JSON before finally decoding to
+ // the correct Go struct type. (Also: the specific result
+ // type depends on the implementation of ExecuteCommand:
+ // (*Server).ExecuteCommand returns the specific Go type!
+ // This is not a meaningful interface.)
var result interface{}
if err := s.sender.Call(ctx, "workspace/executeCommand", params, &result); err != nil {
return nil, err
diff --git a/gopls/internal/lsp/regtest/env.go b/gopls/internal/lsp/regtest/env.go
index 29cb288..344e5e7 100644
--- a/gopls/internal/lsp/regtest/env.go
+++ b/gopls/internal/lsp/regtest/env.go
@@ -69,6 +69,7 @@
OnLogMessage: a.onLogMessage,
OnWorkDoneProgressCreate: a.onWorkDoneProgressCreate,
OnProgress: a.onProgress,
+ OnShowDocument: a.onShowDocument,
OnShowMessage: a.onShowMessage,
OnShowMessageRequest: a.onShowMessageRequest,
OnRegisterCapability: a.onRegisterCapability,
@@ -82,6 +83,7 @@
// diagnostics are a map of relative path->diagnostics params
diagnostics map[string]*protocol.PublishDiagnosticsParams
logs []*protocol.LogMessageParams
+ showDocument []*protocol.ShowDocumentParams
showMessage []*protocol.ShowMessageParams
showMessageRequest []*protocol.ShowMessageRequestParams
@@ -201,6 +203,15 @@
return nil
}
+func (a *Awaiter) onShowDocument(_ context.Context, params *protocol.ShowDocumentParams) error {
+ a.mu.Lock()
+ defer a.mu.Unlock()
+
+ a.state.showDocument = append(a.state.showDocument, params)
+ a.checkConditionsLocked()
+ return nil
+}
+
func (a *Awaiter) onShowMessage(_ context.Context, m *protocol.ShowMessageParams) error {
a.mu.Lock()
defer a.mu.Unlock()
diff --git a/gopls/internal/lsp/regtest/expectation.go b/gopls/internal/lsp/regtest/expectation.go
index a770616..92fda40 100644
--- a/gopls/internal/lsp/regtest/expectation.go
+++ b/gopls/internal/lsp/regtest/expectation.go
@@ -206,6 +206,23 @@
}
}
+// ShownDocument asserts that the client has received a
+// ShowDocumentRequest for the given URI.
+func ShownDocument(uri protocol.URI) Expectation {
+ check := func(s State) Verdict {
+ for _, params := range s.showDocument {
+ if params.URI == uri {
+ return Met
+ }
+ }
+ return Unmet
+ }
+ return Expectation{
+ Check: check,
+ Description: fmt.Sprintf("received window/showDocument for URI %s", uri),
+ }
+}
+
// NoShownMessage asserts that the editor has not received a ShowMessage.
func NoShownMessage(subString string) Expectation {
check := func(s State) Verdict {
diff --git a/gopls/internal/regtest/debug/debug_test.go b/gopls/internal/regtest/debug/debug_test.go
index dc39f81..261abf9 100644
--- a/gopls/internal/regtest/debug/debug_test.go
+++ b/gopls/internal/regtest/debug/debug_test.go
@@ -5,10 +5,17 @@
package debug
import (
+ "context"
+ "encoding/json"
+ "io"
+ "net/http"
+ "strings"
"testing"
"golang.org/x/tools/gopls/internal/bug"
"golang.org/x/tools/gopls/internal/hooks"
+ "golang.org/x/tools/gopls/internal/lsp/command"
+ "golang.org/x/tools/gopls/internal/lsp/protocol"
. "golang.org/x/tools/gopls/internal/lsp/regtest"
)
@@ -28,3 +35,67 @@
env.Await(ShownMessage(desc))
})
}
+
+// TestStartDebugging executes a gopls.start_debugging command to
+// start the internal web server.
+func TestStartDebugging(t *testing.T) {
+ WithOptions(
+ Modes(Default|Experimental), // doesn't work in Forwarded mode
+ ).Run(t, "", func(t *testing.T, env *Env) {
+ // Start a debugging server.
+ res, err := startDebugging(env.Ctx, env.Editor.Server, &command.DebuggingArgs{
+ Addr: "", // any free port
+ })
+ if err != nil {
+ t.Fatalf("startDebugging: %v", err)
+ }
+
+ // Assert that the server requested that the
+ // client show the debug page in a browser.
+ debugURL := res.URLs[0]
+ env.Await(ShownDocument(debugURL))
+
+ // Send a request to the debug server and ensure it responds.
+ resp, err := http.Get(debugURL)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer resp.Body.Close()
+ data, err := io.ReadAll(resp.Body)
+ if err != nil {
+ t.Fatalf("reading HTTP response body: %v", err)
+ }
+ const want = "<title>GoPls"
+ if !strings.Contains(string(data), want) {
+ t.Errorf("GET %s response does not contain %q: <<%s>>", debugURL, want, data)
+ }
+ })
+}
+
+// startDebugging starts a debugging server.
+// TODO(adonovan): move into command package?
+func startDebugging(ctx context.Context, server protocol.Server, args *command.DebuggingArgs) (*command.DebuggingResult, error) {
+ rawArgs, err := command.MarshalArgs(args)
+ if err != nil {
+ return nil, err
+ }
+ res0, err := server.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{
+ Command: command.StartDebugging.ID(),
+ Arguments: rawArgs,
+ })
+ if err != nil {
+ return nil, err
+ }
+ // res0 is the result of a schemaless (map[string]any) JSON decoding.
+ // Re-encode and decode into the correct Go struct type.
+ // TODO(adonovan): fix (*serverDispatcher).ExecuteCommand.
+ data, err := json.Marshal(res0)
+ if err != nil {
+ return nil, err
+ }
+ var res *command.DebuggingResult
+ if err := json.Unmarshal(data, &res); err != nil {
+ return nil, err
+ }
+ return res, nil
+}
