gopls: add a new "subdirWatchPatterns" setting

As discovered in golang/go#60089, file watching patterns behave very
differently in different clients. We avoided a bad client-side bug in VS
Code by splitting our subdirectory watch pattern, but this appears to be
very expensive in other clients (notably coc.nvim, or any vim client
that uses watchman).

The subdirectory watch patterns were only known to be necessary for VS
Code, due to microsoft/vscode#109754. Other clients work as expected
when we watch e.g. **/*.go. For that reason, let's revert all other
clients to just use simple watch patterns, and only specialize to have
subdirectory watch patterns for VS Code.

It's truly unfortunate to have to specialize in this way. To paper over
this hole in the wall, add an internal setting that allows clients to
configure this behavior explicitly. The new "subdirWatchPatterns"
setting may accepts the following values:
 - "on": request watch patterns for each subdirectory (as before)
 - "off": do not request subdirectory watch patterns
 - "auto": same as "on" for VS Code, "off" for all others, based on the
   provided initializeParams.clientInfo.Name.

Includes some minor cleanup for the fake editor, and fixes some stale
comments.

Updates golang/go#golang/go#60089
Fixes golang/go#59635

Change-Id: I1eab5c08790bd86a5910657169edcb20511c0280
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496835
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>

Author: findleyr

gopls/internal/lsp/cache/snapshot.go

@@ -936,30 +936,56 @@ func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 		patterns[fmt.Sprintf("%s/**/*.{%s}", dirName, extensions)] = struct{}{}
 	}
 
-	// Some clients (e.g. VSCode) do not send notifications for
-	// changes to directories that contain Go code (golang/go#42348).
-	// To handle this, explicitly watch all of the directories in
-	// the workspace. We find them by adding the directories of
-	// every file in the snapshot's workspace directories.
-	// There may be thousands of patterns, each a single directory.
-	//
-	// (A previous iteration created a single glob pattern holding a
-	// union of all the directories, but this was found to cause
-	// VSCode to get stuck for several minutes after a buffer was
-	// saved twice in a workspace that had >8000 watched directories.)
-	//
-	// Some clients (notably coc.nvim, which uses watchman for
-	// globs) perform poorly with a large list of individual
-	// directories, though they work fine with one large
-	// comma-separated element. Sadly no one size fits all, so we
-	// may have to resort to sniffing the client to determine the
-	// best behavior, though that would set a poor precedent.
-	// TODO(adonovan): improve the nvim situation.
-	s.addKnownSubdirs(patterns, dirs)
+	if s.watchSubdirs() {
+		// Some clients (e.g. VS Code) do not send notifications for changes to
+		// directories that contain Go code (golang/go#42348). To handle this,
+		// explicitly watch all of the directories in the workspace. We find them
+		// by adding the directories of every file in the snapshot's workspace
+		// directories. There may be thousands of patterns, each a single
+		// directory.
+		//
+		// (A previous iteration created a single glob pattern holding a union of
+		// all the directories, but this was found to cause VS Code to get stuck
+		// for several minutes after a buffer was saved twice in a workspace that
+		// had >8000 watched directories.)
+		//
+		// Some clients (notably coc.nvim, which uses watchman for globs) perform
+		// poorly with a large list of individual directories.
+		s.addKnownSubdirs(patterns, dirs)
+	}
 
 	return patterns
 }
 
+// watchSubdirs reports whether gopls should request separate file watchers for
+// each relevant subdirectory. This is necessary only for clients (namely VS
+// Code) that do not send notifications for individual files in a directory
+// when the entire directory is deleted.
+func (s *snapshot) watchSubdirs() bool {
+	opts := s.view.Options()
+	switch p := opts.SubdirWatchPatterns; p {
+	case source.SubdirWatchPatternsOn:
+		return true
+	case source.SubdirWatchPatternsOff:
+		return false
+	case source.SubdirWatchPatternsAuto:
+		// See the documentation of InternalOptions.SubdirWatchPatterns for an
+		// explanation of why VS Code gets a different default value here.
+		//
+		// Unfortunately, there is no authoritative list of client names, nor any
+		// requirements that client names do not change. We should update the VS
+		// Code extension to set a default value of "subdirWatchPatterns" to "on",
+		// so that this workaround is only temporary.
+		if opts.ClientInfo != nil && opts.ClientInfo.Name == "Visual Studio Code" {
+			return true
+		}
+		return false
+	default:
+		bug.Reportf("invalid subdirWatchPatterns: %q", p)
+		return false
+	}
+}
+
 func (s *snapshot) addKnownSubdirs(patterns map[string]struct{}, wsDirs []span.URI) {
 	s.mu.Lock()
 	defer s.mu.Unlock()

gopls/internal/lsp/fake/client.go

@@ -94,9 +94,8 @@ func (c *Client) Configuration(_ context.Context, p *protocol.ParamConfiguration
 	results := make([]interface{}, len(p.Items))
 	for i, item := range p.Items {
 		if item.Section == "gopls" {
-			c.editor.mu.Lock()
-			results[i] = c.editor.settingsLocked()
-			c.editor.mu.Unlock()
+			config := c.editor.Config()
+			results[i] = makeSettings(c.editor.sandbox, config)
 		}
 	}
 	return results, nil

gopls/internal/lsp/fake/editor.go

@@ -37,7 +37,6 @@ type Editor struct {
 	serverConn jsonrpc2.Conn
 	client     *Client
 	sandbox    *Sandbox
-	defaultEnv map[string]string
 
 	// TODO(adonovan): buffers should be keyed by protocol.DocumentURI.
 	mu                 sync.Mutex
@@ -75,8 +74,14 @@ func (b buffer) text() string {
 // source.UserOptions, but we use a separate type here so that we expose only
 // that configuration which we support.
 //
-// The zero value for EditorConfig should correspond to its defaults.
+// The zero value for EditorConfig is the default configuration.
 type EditorConfig struct {
+	// ClientName sets the clientInfo.name for the LSP session (in the initialize request).
+	//
+	// Since this can only be set during initialization, changing this field via
+	// Editor.ChangeConfiguration has no effect.
+	ClientName string
+
 	// Env holds environment variables to apply on top of the default editor
 	// environment. When applying these variables, the special string
 	// $SANDBOX_WORKDIR is replaced by the absolute path to the sandbox working
@@ -109,10 +114,9 @@ type EditorConfig struct {
 // NewEditor creates a new Editor.
 func NewEditor(sandbox *Sandbox, config EditorConfig) *Editor {
 	return &Editor{
-		buffers:    make(map[string]buffer),
-		sandbox:    sandbox,
-		defaultEnv: sandbox.GoEnv(),
-		config:     config,
+		buffers: make(map[string]buffer),
+		sandbox: sandbox,
+		config:  config,
 	}
 }
 
@@ -198,19 +202,17 @@ func (e *Editor) Client() *Client {
 	return e.client
 }
 
-// settingsLocked builds the settings map for use in LSP settings RPCs.
-//
-// e.mu must be held while calling this function.
-func (e *Editor) settingsLocked() map[string]interface{} {
+// makeSettings builds the settings map for use in LSP settings RPCs.
+func makeSettings(sandbox *Sandbox, config EditorConfig) map[string]interface{} {
 	env := make(map[string]string)
-	for k, v := range e.defaultEnv {
+	for k, v := range sandbox.GoEnv() {
 		env[k] = v
 	}
-	for k, v := range e.config.Env {
+	for k, v := range config.Env {
 		env[k] = v
 	}
 	for k, v := range env {
-		v = strings.ReplaceAll(v, "$SANDBOX_WORKDIR", e.sandbox.Workdir.RootURI().SpanURI().Filename())
+		v = strings.ReplaceAll(v, "$SANDBOX_WORKDIR", sandbox.Workdir.RootURI().SpanURI().Filename())
 		env[k] = v
 	}
 
@@ -226,7 +228,7 @@ func (e *Editor) settingsLocked() map[string]interface{} {
 		"completionBudget": "10s",
 	}
 
-	for k, v := range e.config.Settings {
+	for k, v := range config.Settings {
 		if k == "env" {
 			panic("must not provide env via the EditorConfig.Settings field: use the EditorConfig.Env field instead")
 		}
@@ -237,20 +239,22 @@ func (e *Editor) settingsLocked() map[string]interface{} {
 }
 
 func (e *Editor) initialize(ctx context.Context) error {
+	config := e.Config()
+
 	params := &protocol.ParamInitialize{}
-	params.ClientInfo = &protocol.Msg_XInitializeParams_clientInfo{}
-	params.ClientInfo.Name = "fakeclient"
-	params.ClientInfo.Version = "v1.0.0"
-	e.mu.Lock()
-	params.WorkspaceFolders = e.makeWorkspaceFoldersLocked()
-	params.InitializationOptions = e.settingsLocked()
-	e.mu.Unlock()
-	params.Capabilities.Workspace.Configuration = true
-	params.Capabilities.Window.WorkDoneProgress = true
+	if e.config.ClientName != "" {
+		params.ClientInfo = &protocol.Msg_XInitializeParams_clientInfo{}
+		params.ClientInfo.Name = e.config.ClientName
+		params.ClientInfo.Version = "v1.0.0"
+	}
+	params.InitializationOptions = makeSettings(e.sandbox, config)
+	params.WorkspaceFolders = makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders)
+	params.Capabilities.Workspace.Configuration = true // support workspace/configuration
+	params.Capabilities.Window.WorkDoneProgress = true // support window/workDoneProgress
 
-	// TODO: set client capabilities
-	params.Capabilities.TextDocument.Completion.CompletionItem.TagSupport.ValueSet = []protocol.CompletionItemTag{protocol.ComplDeprecated}
+	// TODO(rfindley): set client capabilities (note from the future: why?)
 
+	params.Capabilities.TextDocument.Completion.CompletionItem.TagSupport.ValueSet = []protocol.CompletionItemTag{protocol.ComplDeprecated}
 	params.Capabilities.TextDocument.Completion.CompletionItem.SnippetSupport = true
 	params.Capabilities.TextDocument.SemanticTokens.Requests.Full.Value = true
 	// copied from lsp/semantic.go to avoid import cycle in tests
@@ -269,11 +273,12 @@ func (e *Editor) initialize(ctx context.Context) error {
 	// but really we should test both ways for older editors.
 	params.Capabilities.TextDocument.DocumentSymbol.HierarchicalDocumentSymbolSupport = true
 
-	// This is a bit of a hack, since the fake editor doesn't actually support
-	// watching changed files that match a specific glob pattern. However, the
-	// editor does send didChangeWatchedFiles notifications, so set this to
-	// true.
+	// Glob pattern watching is enabled.
 	params.Capabilities.Workspace.DidChangeWatchedFiles.DynamicRegistration = true
+
+	// "rename" operations are used for package renaming.
+	//
+	// TODO(rfindley): add support for other resource operations (create, delete, ...)
 	params.Capabilities.Workspace.WorkspaceEdit = &protocol.WorkspaceEditClientCapabilities{
 		ResourceOperations: []protocol.ResourceOperationKind{
 			"rename",
@@ -300,18 +305,15 @@ func (e *Editor) initialize(ctx context.Context) error {
 	return nil
 }
 
-// makeWorkspaceFoldersLocked creates a slice of workspace folders to use for
+// makeWorkspaceFolders creates a slice of workspace folders to use for
 // this editing session, based on the editor configuration.
-//
-// e.mu must be held while calling this function.
-func (e *Editor) makeWorkspaceFoldersLocked() (folders []protocol.WorkspaceFolder) {
-	paths := e.config.WorkspaceFolders
+func makeWorkspaceFolders(sandbox *Sandbox, paths []string) (folders []protocol.WorkspaceFolder) {
 	if len(paths) == 0 {
-		paths = append(paths, string(e.sandbox.Workdir.RelativeTo))
+		paths = []string{string(sandbox.Workdir.RelativeTo)}
 	}
 
 	for _, path := range paths {
-		uri := string(e.sandbox.Workdir.URI(path))
+		uri := string(sandbox.Workdir.URI(path))
 		folders = append(folders, protocol.WorkspaceFolder{
 			URI:  uri,
 			Name: filepath.Base(uri),
@@ -1329,14 +1331,18 @@ func (e *Editor) Config() EditorConfig {
 	return e.config
 }
 
+func (e *Editor) SetConfig(cfg EditorConfig) {
+	e.mu.Lock()
+	e.config = cfg
+	e.mu.Unlock()
+}
+
 // ChangeConfiguration sets the new editor configuration, and if applicable
 // sends a didChangeConfiguration notification.
 //
 // An error is returned if the change notification failed to send.
 func (e *Editor) ChangeConfiguration(ctx context.Context, newConfig EditorConfig) error {
-	e.mu.Lock()
-	e.config = newConfig
-	e.mu.Unlock() // don't hold e.mu during server calls
+	e.SetConfig(newConfig)
 	if e.Server != nil {
 		var params protocol.DidChangeConfigurationParams // empty: gopls ignores the Settings field
 		if err := e.Server.DidChangeConfiguration(ctx, &params); err != nil {
@@ -1351,12 +1357,13 @@ func (e *Editor) ChangeConfiguration(ctx context.Context, newConfig EditorConfig
 //
 // The given folders must all be unique.
 func (e *Editor) ChangeWorkspaceFolders(ctx context.Context, folders []string) error {
+	config := e.Config()
+
 	// capture existing folders so that we can compute the change.
-	e.mu.Lock()
-	oldFolders := e.makeWorkspaceFoldersLocked()
-	e.config.WorkspaceFolders = folders
-	newFolders := e.makeWorkspaceFoldersLocked()
-	e.mu.Unlock()
+	oldFolders := makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders)
+	newFolders := makeWorkspaceFolders(e.sandbox, folders)
+	config.WorkspaceFolders = folders
+	e.SetConfig(config)
 
 	if e.Server == nil {
 		return nil

gopls/internal/lsp/general.go

@@ -59,7 +59,7 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitializ
 	if err := s.handleOptionResults(ctx, source.SetOptions(options, params.InitializationOptions)); err != nil {
 		return nil, err
 	}
-	options.ForClientCapabilities(params.Capabilities)
+	options.ForClientCapabilities(params.ClientInfo, params.Capabilities)
 
 	if options.ShowBugReports {
 		// Report the next bug that occurs on the server.

gopls/internal/lsp/regtest/expectation.go

@@ -235,7 +235,7 @@ func ShownMessage(containing string) Expectation {
 	}
 	return Expectation{
 		Check:       check,
-		Description: "received ShowMessage",
+		Description: fmt.Sprintf("received window/showMessage containing %q", containing),
 	}
 }
 

gopls/internal/lsp/regtest/options.go

@@ -64,8 +64,14 @@ func WindowsLineEndings() RunOption {
 	})
 }
 
-// Settings is a RunOption that sets user-provided configuration for the LSP
-// server.
+// ClientName sets the LSP client name.
+func ClientName(name string) RunOption {
+	return optionSetter(func(opts *runConfig) {
+		opts.editor.ClientName = name
+	})
+}
+
+// Settings sets user-provided configuration for the LSP server.
 //
 // As a special case, the env setting must not be provided via Settings: use
 // EnvVars instead.