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>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 948912a..de9524b 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -936,30 +936,56 @@
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()
diff --git a/gopls/internal/lsp/fake/client.go b/gopls/internal/lsp/fake/client.go
index b619ef5..555428e 100644
--- a/gopls/internal/lsp/fake/client.go
+++ b/gopls/internal/lsp/fake/client.go
@@ -94,9 +94,8 @@
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
diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go
index ae9338d..45def8f 100644
--- a/gopls/internal/lsp/fake/editor.go
+++ b/gopls/internal/lsp/fake/editor.go
@@ -37,7 +37,6 @@
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 @@
// 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 @@
// 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 @@
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 @@
"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) 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
+ // 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 @@
// 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 @@
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 @@
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, ¶ms); err != nil {
@@ -1351,12 +1357,13 @@
//
// 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
diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go
index 4baf98c..04fe713 100644
--- a/gopls/internal/lsp/general.go
+++ b/gopls/internal/lsp/general.go
@@ -59,7 +59,7 @@
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.
diff --git a/gopls/internal/lsp/regtest/expectation.go b/gopls/internal/lsp/regtest/expectation.go
index 9d9f023..7cfee8b 100644
--- a/gopls/internal/lsp/regtest/expectation.go
+++ b/gopls/internal/lsp/regtest/expectation.go
@@ -235,7 +235,7 @@
}
return Expectation{
Check: check,
- Description: "received ShowMessage",
+ Description: fmt.Sprintf("received window/showMessage containing %q", containing),
}
}
diff --git a/gopls/internal/lsp/regtest/options.go b/gopls/internal/lsp/regtest/options.go
index 7a41696..f55fd5b 100644
--- a/gopls/internal/lsp/regtest/options.go
+++ b/gopls/internal/lsp/regtest/options.go
@@ -64,8 +64,14 @@
})
}
-// 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.
diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go
index 2ca8895..23d6e9a 100644
--- a/gopls/internal/lsp/source/options.go
+++ b/gopls/internal/lsp/source/options.go
@@ -169,6 +169,7 @@
DeepCompletion: true,
ChattyDiagnostics: true,
NewDiff: "both",
+ SubdirWatchPatterns: SubdirWatchPatternsAuto,
},
Hooks: Hooks{
// TODO(adonovan): switch to new diff.Strings implementation.
@@ -198,6 +199,7 @@
// ClientOptions holds LSP-specific configuration that is provided by the
// client.
type ClientOptions struct {
+ ClientInfo *protocol.Msg_XInitializeParams_clientInfo
InsertTextFormat protocol.InsertTextFormat
ConfigurationSupported bool
DynamicConfigurationSupported bool
@@ -536,6 +538,9 @@
// average user. These may be settings used by tests or outdated settings that
// will soon be deprecated. Some of these settings may not even be configurable
// by the user.
+//
+// TODO(rfindley): even though these settings are not intended for
+// modification, we should surface them in our documentation.
type InternalOptions struct {
// LiteralCompletions controls whether literal candidates such as
// "&someStruct{}" are offered. Tests disable this flag to simplify
@@ -599,8 +604,42 @@
// file change. If unset, gopls only reports diagnostics when they change, or
// when a file is opened or closed.
ChattyDiagnostics bool
+
+ // SubdirWatchPatterns configures the file watching glob patterns registered
+ // by gopls.
+ //
+ // Some clients (namely VS Code) do not send workspace/didChangeWatchedFile
+ // notifications for files contained in a directory when that directory is
+ // deleted:
+ // https://github.com/microsoft/vscode/issues/109754
+ //
+ // In this case, gopls would miss important notifications about deleted
+ // packages. To work around this, gopls registers a watch pattern for each
+ // directory containing Go files.
+ //
+ // Unfortunately, other clients experience performance problems with this
+ // many watch patterns, so there is no single behavior that works well for
+ // all clients.
+ //
+ // The "subdirWatchPatterns" setting allows configuring this behavior. Its
+ // default value of "auto" attempts to guess the correct behavior based on
+ // the client name. We'd love to avoid this specialization, but as described
+ // above there is no single value that works for all clients.
+ //
+ // If any LSP client does not behave well with the default value (for
+ // example, if like VS Code it drops file notifications), please file an
+ // issue.
+ SubdirWatchPatterns SubdirWatchPatterns
}
+type SubdirWatchPatterns string
+
+const (
+ SubdirWatchPatternsOn SubdirWatchPatterns = "on"
+ SubdirWatchPatternsOff SubdirWatchPatterns = "off"
+ SubdirWatchPatternsAuto SubdirWatchPatterns = "auto"
+)
+
type ImportShortcut string
const (
@@ -742,7 +781,8 @@
return results
}
-func (o *Options) ForClientCapabilities(caps protocol.ClientCapabilities) {
+func (o *Options) ForClientCapabilities(clientName *protocol.Msg_XInitializeParams_clientInfo, caps protocol.ClientCapabilities) {
+ o.ClientInfo = clientName
// Check if the client supports snippets in completion items.
if caps.Workspace.WorkspaceEdit != nil {
o.SupportedResourceOperations = caps.Workspace.WorkspaceEdit.ResourceOperations
@@ -1159,6 +1199,15 @@
case "chattyDiagnostics":
result.setBool(&o.ChattyDiagnostics)
+ case "subdirWatchPatterns":
+ if s, ok := result.asOneOf(
+ string(SubdirWatchPatternsOn),
+ string(SubdirWatchPatternsOff),
+ string(SubdirWatchPatternsAuto),
+ ); ok {
+ o.SubdirWatchPatterns = SubdirWatchPatterns(s)
+ }
+
// Replaced settings.
case "experimentalDisabledAnalyses":
result.deprecated("analyses")
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index f8e59a0..c765cb0 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -1506,7 +1506,13 @@
bob.Hello()
}
`
- Run(t, mod, func(t *testing.T, env *Env) {
+ WithOptions(
+ Settings{
+ // Now that we don't watch subdirs by default (except for VS Code),
+ // we must explicitly ask gopls to requests subdir watch patterns.
+ "subdirWatchPatterns": "on",
+ },
+ ).Run(t, mod, func(t *testing.T, env *Env) {
env.OnceMet(
InitialWorkspaceLoad,
FileWatchMatching("bob"),
diff --git a/gopls/internal/regtest/watch/setting_test.go b/gopls/internal/regtest/watch/setting_test.go
new file mode 100644
index 0000000..9ed7fde
--- /dev/null
+++ b/gopls/internal/regtest/watch/setting_test.go
@@ -0,0 +1,85 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package regtest
+
+import (
+ "fmt"
+ "testing"
+
+ . "golang.org/x/tools/gopls/internal/lsp/regtest"
+)
+
+func TestSubdirWatchPatterns(t *testing.T) {
+ const files = `
+-- go.mod --
+module mod.test
+
+go 1.18
+-- subdir/subdir.go --
+package subdir
+`
+
+ tests := []struct {
+ clientName string
+ subdirWatchPatterns string
+ wantWatched bool
+ }{
+ {"other client", "on", true},
+ {"other client", "off", false},
+ {"other client", "auto", false},
+ {"Visual Studio Code", "auto", true},
+ }
+
+ for _, test := range tests {
+ t.Run(fmt.Sprintf("%s_%s", test.clientName, test.subdirWatchPatterns), func(t *testing.T) {
+ WithOptions(
+ ClientName(test.clientName),
+ Settings{
+ "subdirWatchPatterns": test.subdirWatchPatterns,
+ },
+ ).Run(t, files, func(t *testing.T, env *Env) {
+ var expectation Expectation
+ if test.wantWatched {
+ expectation = FileWatchMatching("subdir")
+ } else {
+ expectation = NoFileWatchMatching("subdir")
+ }
+ env.OnceMet(
+ InitialWorkspaceLoad,
+ expectation,
+ )
+ })
+ })
+ }
+}
+
+// This test checks that we surface errors for invalid subdir watch patterns,
+// as the triple of ("off"|"on"|"auto") may be confusing to users inclined to
+// use (true|false) or some other truthy value.
+func TestSubdirWatchPatterns_BadValues(t *testing.T) {
+ tests := []struct {
+ badValue interface{}
+ wantMessage string
+ }{
+ {true, "invalid type bool, expect string"},
+ {false, "invalid type bool, expect string"},
+ {"yes", `invalid option "yes"`},
+ }
+
+ for _, test := range tests {
+ t.Run(fmt.Sprint(test.badValue), func(t *testing.T) {
+ WithOptions(
+ Settings{
+ "subdirWatchPatterns": test.badValue,
+ },
+ ).Run(t, "", func(t *testing.T, env *Env) {
+ env.OnceMet(
+ InitialWorkspaceLoad,
+ ShownMessage(test.wantMessage),
+ )
+ })
+ })
+ }
+}
