gopls/internal/server: don't call window/showDocument if unsupported

Don't unconditionally call window/showDocument; check the client
capability. In the case of opening a browser window, use showMessage if
showDocument is unsupported.

Also somewhat clean up the way that we check for shown documents.

Fixes golang/go#68904

Change-Id: I29b281b615185f2bbda2f00e7c17575678177cd0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/626616
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/server/command.go

@@ -719,7 +719,7 @@ func (c *commandHandler) Doc(ctx context.Context, args command.DocArgs) (protoco
 		// Direct the client to open the /pkg page.
 		result = web.PkgURL(deps.snapshot.View().ID(), pkgpath, fragment)
 		if args.ShowDocument {
-			openClientBrowser(ctx, c.s.client, result)
+			openClientBrowser(ctx, c.s.client, "Doc", result, c.s.Options())
 		}
 
 		return nil
@@ -1158,7 +1158,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])
+	openClientBrowser(ctx, c.s.client, "Debug", result.URLs[0], c.s.Options())
 	return result, nil
 }
 
@@ -1560,20 +1560,39 @@ func showMessage(ctx context.Context, cli protocol.Client, typ protocol.MessageT
 
 // 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)
+//
+// If the client does not support window/showDocument, a window/showMessage
+// request is instead used, with the format "$title: open your browser to $url".
+func openClientBrowser(ctx context.Context, cli protocol.Client, title string, url protocol.URI, opts *settings.Options) {
+	if opts.ShowDocumentSupported {
+		showDocumentImpl(ctx, cli, url, nil, opts)
+	} else {
+		params := &protocol.ShowMessageParams{
+			Type:    protocol.Info,
+			Message: fmt.Sprintf("%s: open your browser to %s", title, url),
+		}
+		if err := cli.ShowMessage(ctx, params); err != nil {
+			event.Error(ctx, "failed to show brower url", err)
+		}
+	}
 }
 
 // openClientEditor causes the LSP client to open the specified document
 // and select the indicated range.
 //
 // Note that VS Code 1.87.2 doesn't currently raise the window; this is
 // https://github.com/microsoft/vscode/issues/207634
-func openClientEditor(ctx context.Context, cli protocol.Client, loc protocol.Location) {
-	showDocumentImpl(ctx, cli, protocol.URI(loc.URI), &loc.Range)
+func openClientEditor(ctx context.Context, cli protocol.Client, loc protocol.Location, opts *settings.Options) {
+	if !opts.ShowDocumentSupported {
+		return // no op
+	}
+	showDocumentImpl(ctx, cli, protocol.URI(loc.URI), &loc.Range, opts)
 }
 
-func showDocumentImpl(ctx context.Context, cli protocol.Client, url protocol.URI, rangeOpt *protocol.Range) {
+func showDocumentImpl(ctx context.Context, cli protocol.Client, url protocol.URI, rangeOpt *protocol.Range, opts *settings.Options) {
+	if !opts.ShowDocumentSupported {
+		return // no op
+	}
 	// 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
@@ -1690,7 +1709,7 @@ func (c *commandHandler) FreeSymbols(ctx context.Context, viewID string, loc pro
 		return err
 	}
 	url := web.freesymbolsURL(viewID, loc)
-	openClientBrowser(ctx, c.s.client, url)
+	openClientBrowser(ctx, c.s.client, "Free symbols", url, c.s.Options())
 	return nil
 }
 
@@ -1700,12 +1719,14 @@ func (c *commandHandler) Assembly(ctx context.Context, viewID, packageID, symbol
 		return err
 	}
 	url := web.assemblyURL(viewID, packageID, symbol)
-	openClientBrowser(ctx, c.s.client, url)
+	openClientBrowser(ctx, c.s.client, "Assembly", url, c.s.Options())
 	return nil
 }
 
 func (c *commandHandler) ClientOpenURL(ctx context.Context, url string) error {
-	openClientBrowser(ctx, c.s.client, url)
+	// Fall back to "Gopls: open your browser..." if we must send a showMessage
+	// request, since we don't know the context of this command.
+	openClientBrowser(ctx, c.s.client, "Gopls", url, c.s.Options())
 	return nil
 }
 

gopls/internal/server/server.go

@@ -303,7 +303,7 @@ func (s *server) initWeb() (*web, error) {
 		openClientEditor(req.Context(), s.client, protocol.Location{
 			URI:   uri,
 			Range: protocol.Range{Start: posn, End: posn},
-		})
+		}, s.Options())
 	})
 
 	// The /pkg/PATH&view=... handler shows package documentation for PATH.

gopls/internal/settings/settings.go

@@ -76,6 +76,7 @@ type ClientOptions struct {
 	CompletionDeprecated                       bool
 	SupportedResourceOperations                []protocol.ResourceOperationKind
 	CodeActionResolveOptions                   []string
+	ShowDocumentSupported                      bool
 }
 
 // ServerOptions holds LSP-specific configuration that is provided by the
@@ -872,6 +873,9 @@ func (o *Options) ForClientCapabilities(clientInfo *protocol.ClientInfo, caps pr
 		o.InsertTextFormat = protocol.SnippetTextFormat
 	}
 	o.InsertReplaceSupported = caps.TextDocument.Completion.CompletionItem.InsertReplaceSupport
+	if caps.Window.ShowDocument != nil {
+		o.ShowDocumentSupported = caps.Window.ShowDocument.Support
+	}
 	// Check if the client supports configuration messages.
 	o.ConfigurationSupported = caps.Workspace.Configuration
 	o.DynamicConfigurationSupported = caps.Workspace.DidChangeConfiguration.DynamicRegistration

gopls/internal/test/integration/env.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 
 	"golang.org/x/tools/gopls/internal/protocol"
@@ -34,6 +35,10 @@ type Env struct {
 	Awaiter *Awaiter
 }
 
+// nextAwaiterRegistration is used to create unique IDs for various Awaiter
+// registrations.
+var nextAwaiterRegistration atomic.Uint64
+
 // An Awaiter keeps track of relevant LSP state, so that it may be asserted
 // upon with Expectations.
 //
@@ -46,9 +51,13 @@ type Awaiter struct {
 
 	mu sync.Mutex
 	// For simplicity, each waiter gets a unique ID.
-	nextWaiterID int
-	state        State
-	waiters      map[int]*condition
+	state   State
+	waiters map[uint64]*condition
+
+	// collectors map a registration to the collection of messages that have been
+	// received since the registration was created.
+	docCollectors     map[uint64][]*protocol.ShowDocumentParams
+	messageCollectors map[uint64][]*protocol.ShowMessageParams
 }
 
 func NewAwaiter(workdir *fake.Workdir) *Awaiter {
@@ -60,7 +69,7 @@ func NewAwaiter(workdir *fake.Workdir) *Awaiter {
 			startedWork:   make(map[string]uint64),
 			completedWork: make(map[string]uint64),
 		},
-		waiters: make(map[int]*condition),
+		waiters: make(map[uint64]*condition),
 	}
 }
 
@@ -79,9 +88,6 @@ func (a *Awaiter) Hooks() fake.ClientHooks {
 	}
 }
 
-// ResetShownDocuments resets the set of accumulated ShownDocuments seen so far.
-func (a *Awaiter) ResetShownDocuments() { a.state.showDocument = nil }
-
 // State encapsulates the server state TODO: explain more
 type State struct {
 	// diagnostics are a map of relative path->diagnostics params
@@ -171,20 +177,78 @@ func (a *Awaiter) onShowDocument(_ context.Context, params *protocol.ShowDocumen
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
+	// Update any outstanding listeners.
+	for id, s := range a.docCollectors {
+		a.docCollectors[id] = append(s, params)
+	}
+
 	a.state.showDocument = append(a.state.showDocument, params)
 	a.checkConditionsLocked()
 	return nil
 }
 
-func (a *Awaiter) onShowMessage(_ context.Context, m *protocol.ShowMessageParams) error {
+// ListenToShownDocuments registers a listener to incoming showDocument
+// notifications. Call the resulting func to deregister the listener and
+// receive all notifications that have occurred since the listener was
+// registered.
+func (a *Awaiter) ListenToShownDocuments() func() []*protocol.ShowDocumentParams {
+	id := nextAwaiterRegistration.Add(1)
+
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
-	a.state.showMessage = append(a.state.showMessage, m)
+	if a.docCollectors == nil {
+		a.docCollectors = make(map[uint64][]*protocol.ShowDocumentParams)
+	}
+	a.docCollectors[id] = nil
+
+	return func() []*protocol.ShowDocumentParams {
+		a.mu.Lock()
+		defer a.mu.Unlock()
+		params := a.docCollectors[id]
+		delete(a.docCollectors, id)
+		return params
+	}
+}
+
+func (a *Awaiter) onShowMessage(_ context.Context, params *protocol.ShowMessageParams) error {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	// Update any outstanding listeners.
+	for id, s := range a.messageCollectors {
+		a.messageCollectors[id] = append(s, params)
+	}
+
+	a.state.showMessage = append(a.state.showMessage, params)
 	a.checkConditionsLocked()
 	return nil
 }
 
+// ListenToShownDocuments registers a listener to incoming showDocument
+// notifications. Call the resulting func to deregister the listener and
+// receive all notifications that have occurred since the listener was
+// registered.
+func (a *Awaiter) ListenToShownMessages() func() []*protocol.ShowMessageParams {
+	id := nextAwaiterRegistration.Add(1)
+
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	if a.messageCollectors == nil {
+		a.messageCollectors = make(map[uint64][]*protocol.ShowMessageParams)
+	}
+	a.messageCollectors[id] = nil
+
+	return func() []*protocol.ShowMessageParams {
+		a.mu.Lock()
+		defer a.mu.Unlock()
+		params := a.messageCollectors[id]
+		delete(a.messageCollectors, id)
+		return params
+	}
+}
+
 func (a *Awaiter) onShowMessageRequest(_ context.Context, m *protocol.ShowMessageRequestParams) error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
@@ -332,8 +396,7 @@ func (a *Awaiter) Await(ctx context.Context, expectations ...Expectation) error
 		expectations: expectations,
 		verdict:      make(chan Verdict),
 	}
-	a.waiters[a.nextWaiterID] = cond
-	a.nextWaiterID++
+	a.waiters[nextAwaiterRegistration.Add(1)] = cond
 	a.mu.Unlock()
 
 	var err error

gopls/internal/test/integration/fake/editor.go

@@ -342,7 +342,8 @@ func clientCapabilities(cfg EditorConfig) (protocol.ClientCapabilities, error) {
 	capabilities.TextDocument.Completion.CompletionItem.SnippetSupport = true
 	capabilities.TextDocument.Completion.CompletionItem.InsertReplaceSupport = true
 	capabilities.TextDocument.SemanticTokens.Requests.Full = &protocol.Or_ClientSemanticTokensRequestOptions_full{Value: true}
-	capabilities.Window.WorkDoneProgress = true // support window/workDoneProgress
+	capabilities.Window.WorkDoneProgress = true                                                // support window/workDoneProgress
+	capabilities.Window.ShowDocument = &protocol.ShowDocumentClientCapabilities{Support: true} // support window/showDocument
 	capabilities.TextDocument.SemanticTokens.TokenTypes = []string{
 		"namespace", "type", "class", "enum", "interface",
 		"struct", "typeParameter", "parameter", "variable", "property", "enumMember",