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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>

gopls/internal/lsp/cmd/cmd.go

@@ -22,6 +22,7 @@ import (
 	"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 @@ type connection struct {
 	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 @@ func (c *cmdClient) Progress(_ context.Context, params *protocol.ProgressParams)
 	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 {

gopls/internal/lsp/cmd/serve.go

@@ -109,6 +109,23 @@ func (s *Serve) Run(ctx context.Context, args ...string) error {
 	}
 	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 != "" {

gopls/internal/lsp/command.go

@@ -839,6 +839,7 @@ func (c *commandHandler) StartDebugging(ctx context.Context, args command.Debugg
 		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 @@ func (c *commandHandler) invokeGoWork(ctx context.Context, viewDir, gowork strin
 	}
 	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))
+	}
+}

gopls/internal/lsp/fake/client.go

@@ -22,6 +22,7 @@ type ClientHooks struct {
 	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 @@ func (c *Client) WorkDoneProgressCreate(ctx context.Context, params *protocol.Wo
 	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
 }
 

gopls/internal/lsp/protocol/context.go

@@ -38,6 +38,8 @@ func LogEvent(ctx context.Context, ev core.Event, lm label.Map, mt MessageType)
 	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
 }

gopls/internal/lsp/regtest/env.go

@@ -69,6 +69,7 @@ func (a *Awaiter) Hooks() fake.ClientHooks {
 		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 @@ type State struct {
 	// 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 @@ func (a *Awaiter) onDiagnostics(_ context.Context, d *protocol.PublishDiagnostic
 	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()

gopls/internal/lsp/regtest/expectation.go

@@ -206,6 +206,23 @@ func NoOutstandingWork() Expectation {
 	}
 }
 
+// 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 {

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 @@ func TestBugNotification(t *testing.T) {
 		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
+}