gopls: use relative watched file patterns if supported

The relative pattern support added in version 3.17 of the LSP solves several problems related to watched file patterns. Notably, relative patterns do not suffer from windows drive letter casing problems (since they are expressed relative to a URI), and generally work more consistently across clients. Absolute glob patterns do not work well on many clients. Fixes golang/go#64763 Change-Id: I040560f236463c71dc0efaac1ea0951fb8d98fab Reviewed-on: https://go-review.googlesource.com/c/tools/+/557499 Auto-Submit: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
golang · Jan 23, 2024 · eed1997 · eed1997
1 parent fa791ac
commit eed1997
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 85 deletions.
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
@@ -1025,9 +1025,9 @@ func (b brokenFile) SameContentsOnDisk() bool  { return false }
 func (b brokenFile) Version() int32            { return 0 }
 func (b brokenFile) Content() ([]byte, error)  { return nil, b.err }
 
-// FileWatchingGlobPatterns returns a set of glob patterns patterns that the
-// client is required to watch for changes, and notify the server of them, in
-// order to keep the server's state up to date.
+// FileWatchingGlobPatterns returns a set of glob patterns that the client is
+// required to watch for changes, and notify the server of them, in order to
+// keep the server's state up to date.
 //
 // This set includes
 //  1. all go.mod and go.work files in the workspace; and
@@ -1043,25 +1043,35 @@ func (b brokenFile) Content() ([]byte, error)  { return nil, b.err }
 // The watch for workspace directories in (2) should keep each View up to date,
 // as it should capture any newly added/modified/deleted Go files.
 //
+// Patterns are returned as a set of protocol.RelativePatterns, since they can
+// always be later translated to glob patterns (i.e. strings) if the client
+// lacks relative pattern support. By convention, any pattern returned with
+// empty baseURI should be served as a glob pattern.
+//
+// In general, we prefer to serve relative patterns, as they work better on
+// most clients that support both, and do not have issues with Windows driver
+// letter casing:
+// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#relativePattern
+//
 // TODO(golang/go#57979): we need to reset the memoizedFS when a view changes.
 // Consider the case where we incidentally read a file, then it moved outside
 // of an active module, and subsequently changed: we would still observe the
 // original file state.
-func (s *Session) FileWatchingGlobPatterns(ctx context.Context) map[string]unit {
+func (s *Session) FileWatchingGlobPatterns(ctx context.Context) map[protocol.RelativePattern]unit {
 	s.viewMu.Lock()
 	defer s.viewMu.Unlock()
 
 	// Always watch files that may change the set of views.
-	patterns := map[string]unit{
-		"**/*.{mod,work}": {},
+	patterns := map[protocol.RelativePattern]unit{
+		{Pattern: "**/*.{mod,work}"}: {},
 	}
 
 	for _, view := range s.views {
 		snapshot, release, err := view.Snapshot()
 		if err != nil {
 			continue // view is shut down; continue with others
 		}
-		for k, v := range snapshot.fileWatchingGlobPatterns(ctx) {
+		for k, v := range snapshot.fileWatchingGlobPatterns() {
 			patterns[k] = v
 		}
 		release()

diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
@@ -16,6 +16,7 @@ import (
 	"go/types"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -938,21 +939,25 @@ func (s *Snapshot) resetActivePackagesLocked() {
 
 // See Session.FileWatchingGlobPatterns for a description of gopls' file
 // watching heuristic.
-func (s *Snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]struct{} {
-	extensions := "go,mod,sum,work"
-	for _, ext := range s.Options().TemplateExtensions {
-		extensions += "," + ext
-	}
-
+func (s *Snapshot) fileWatchingGlobPatterns() map[protocol.RelativePattern]unit {
 	// Always watch files that may change the view definition.
-	patterns := make(map[string]unit)
+	patterns := make(map[protocol.RelativePattern]unit)
 
 	// If GOWORK is outside the folder, ensure we are watching it.
 	if s.view.gowork != "" && !s.view.folder.Dir.Encloses(s.view.gowork) {
-		// TODO(rfindley): use RelativePatterns here as well (see below).
-		patterns[filepath.ToSlash(s.view.gowork.Path())] = unit{}
+		workPattern := protocol.RelativePattern{
+			BaseURI: s.view.gowork.Dir(),
+			Pattern: path.Base(string(s.view.gowork)),
+		}
+		patterns[workPattern] = unit{}
 	}
 
+	extensions := "go,mod,sum,work"
+	for _, ext := range s.Options().TemplateExtensions {
+		extensions += "," + ext
+	}
+	watchGoFiles := fmt.Sprintf("**/*.{%s}", extensions)
+
 	var dirs []string
 	if s.view.moduleMode() {
 		// In module mode, watch directories containing active modules, and collect
@@ -964,21 +969,17 @@ func (s *Snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 			dir := filepath.Dir(modFile.Path())
 			dirs = append(dirs, dir)
 
-			// TODO(golang/go#64763): Switch to RelativePatterns if RelativePatternSupport
-			// is available. Relative patterns do not have issues with Windows drive
-			// letter casing.
-			// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#relativePattern
-			//
-			// TODO(golang/go#64724): thoroughly test, particularly on on Windows.
+			// TODO(golang/go#64724): thoroughly test these patterns, particularly on
+			// on Windows.
 			//
 			// Note that glob patterns should use '/' on Windows:
 			// https://code.visualstudio.com/docs/editor/glob-patterns
-			patterns[fmt.Sprintf("%s/**/*.{%s}", filepath.ToSlash(dir), extensions)] = unit{}
+			patterns[protocol.RelativePattern{BaseURI: modFile.Dir(), Pattern: watchGoFiles}] = unit{}
 		}
 	} else {
 		// In non-module modes (GOPATH or AdHoc), we just watch the workspace root.
 		dirs = []string{s.view.root.Path()}
-		patterns[fmt.Sprintf("**/*.{%s}", extensions)] = unit{}
+		patterns[protocol.RelativePattern{Pattern: watchGoFiles}] = unit{}
 	}
 
 	if s.watchSubdirs() {
@@ -1007,14 +1008,14 @@ func (s *Snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 	return patterns
 }
 
-func (s *Snapshot) addKnownSubdirs(patterns map[string]unit, wsDirs []string) {
+func (s *Snapshot) addKnownSubdirs(patterns map[protocol.RelativePattern]unit, wsDirs []string) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
 	s.files.getDirs().Range(func(dir string) {
 		for _, wsDir := range wsDirs {
 			if pathutil.InDir(wsDir, dir) {
-				patterns[filepath.ToSlash(dir)] = unit{}
+				patterns[protocol.RelativePattern{Pattern: filepath.ToSlash(dir)}] = unit{}
 			}
 		}
 	})

diff --git a/gopls/internal/lsp/protocol/generate/tables.go b/gopls/internal/lsp/protocol/generate/tables.go
@@ -133,10 +133,9 @@ var goplsType = map[string]string{
 
 	"Or_Declaration": "[]Location",
 	"Or_DidChangeConfigurationRegistrationOptions_section": "OrPSection_workspace_didChangeConfiguration",
-	"Or_GlobPattern":                "string",
-	"Or_InlayHintLabelPart_tooltip": "OrPTooltipPLabel",
-	"Or_InlayHint_tooltip":          "OrPTooltip_textDocument_inlayHint",
-	"Or_LSPAny":                     "interface{}",
+	"Or_InlayHintLabelPart_tooltip":                        "OrPTooltipPLabel",
+	"Or_InlayHint_tooltip":                                 "OrPTooltip_textDocument_inlayHint",
+	"Or_LSPAny":                                            "interface{}",
 
 	"Or_ParameterInformation_documentation":            "string",
 	"Or_ParameterInformation_label":                    "string",
@@ -151,6 +150,7 @@ var goplsType = map[string]string{
 	"Or_Result_textDocument_typeDefinition":            "[]Location",
 	"Or_Result_workspace_symbol":                       "[]SymbolInformation",
 	"Or_TextDocumentContentChangeEvent":                "TextDocumentContentChangePartial",
+	"Or_RelativePattern_baseUri":                       "DocumentURI",
 
 	"Or_WorkspaceFoldersServerCapabilities_changeNotifications": "string",
 	"Or_WorkspaceSymbol_location":                               "OrPLocation_workspace_symbol",

diff --git a/gopls/internal/lsp/protocol/tsjson.go b/gopls/internal/lsp/protocol/tsjson.go
diff --git a/gopls/internal/lsp/protocol/tsprotocol.go b/gopls/internal/lsp/protocol/tsprotocol.go
diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go
@@ -28,6 +28,7 @@ import (
 	"golang.org/x/tools/gopls/internal/telemetry"
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/goversion"
+	"golang.org/x/tools/gopls/internal/util/maps"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/jsonrpc2"
 )
@@ -362,7 +363,7 @@ func (s *server) updateWatchedDirectories(ctx context.Context) error {
 	defer s.watchedGlobPatternsMu.Unlock()
 
 	// Nothing to do if the set of workspace directories is unchanged.
-	if equalURISet(s.watchedGlobPatterns, patterns) {
+	if maps.SameKeys(s.watchedGlobPatterns, patterns) {
 		return nil
 	}
 
@@ -390,32 +391,32 @@ func watchedFilesCapabilityID(id int) string {
 	return fmt.Sprintf("workspace/didChangeWatchedFiles-%d", id)
 }
 
-func equalURISet(m1, m2 map[string]struct{}) bool {
-	if len(m1) != len(m2) {
-		return false
-	}
-	for k := range m1 {
-		_, ok := m2[k]
-		if !ok {
-			return false
-		}
-	}
-	return true
-}
-
 // registerWatchedDirectoriesLocked sends the workspace/didChangeWatchedFiles
 // registrations to the client and updates s.watchedDirectories.
 // The caller must not subsequently mutate patterns.
-func (s *server) registerWatchedDirectoriesLocked(ctx context.Context, patterns map[string]struct{}) error {
+func (s *server) registerWatchedDirectoriesLocked(ctx context.Context, patterns map[protocol.RelativePattern]unit) error {
 	if !s.Options().DynamicWatchedFilesSupported {
 		return nil
 	}
+
+	supportsRelativePatterns := s.Options().RelativePatternsSupported
+
 	s.watchedGlobPatterns = patterns
 	watchers := make([]protocol.FileSystemWatcher, 0, len(patterns)) // must be a slice
 	val := protocol.WatchChange | protocol.WatchDelete | protocol.WatchCreate
 	for pattern := range patterns {
+		var value any
+		if supportsRelativePatterns && pattern.BaseURI != "" {
+			value = pattern
+		} else {
+			p := pattern.Pattern
+			if pattern.BaseURI != "" {
+				p = path.Join(filepath.ToSlash(pattern.BaseURI.Path()), p)
+			}
+			value = p
+		}
 		watchers = append(watchers, protocol.FileSystemWatcher{
-			GlobPattern: pattern,
+			GlobPattern: protocol.GlobPattern{Value: value},
 			Kind:        &val,
 		})
 	}

diff --git a/gopls/internal/server/server.go b/gopls/internal/server/server.go
@@ -89,7 +89,7 @@ type server struct {
 	// that the server should watch changes.
 	// The map field may be reassigned but the map is immutable.
 	watchedGlobPatternsMu  sync.Mutex
-	watchedGlobPatterns    map[string]unit
+	watchedGlobPatterns    map[protocol.RelativePattern]unit
 	watchRegistrationCount int
 
 	diagnosticsMu sync.Mutex

diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go
@@ -121,6 +121,7 @@ type ClientOptions struct {
 	DynamicConfigurationSupported              bool
 	DynamicRegistrationSemanticTokensSupported bool
 	DynamicWatchedFilesSupported               bool
+	RelativePatternsSupported                  bool
 	PreferredContentFormat                     protocol.MarkupKind
 	LineFoldingOnly                            bool
 	HierarchicalDocumentSymbolSupport          bool
@@ -718,6 +719,7 @@ func (o *Options) ForClientCapabilities(clientName *protocol.ClientInfo, caps pr
 	o.DynamicConfigurationSupported = caps.Workspace.DidChangeConfiguration.DynamicRegistration
 	o.DynamicRegistrationSemanticTokensSupported = caps.TextDocument.SemanticTokens.DynamicRegistration
 	o.DynamicWatchedFilesSupported = caps.Workspace.DidChangeWatchedFiles.DynamicRegistration
+	o.RelativePatternsSupported = caps.Workspace.DidChangeWatchedFiles.RelativePatternSupport
 
 	// Check which types of content format are supported by this client.
 	if hover := caps.TextDocument.Hover; hover != nil && len(hover.ContentFormat) > 0 {

diff --git a/gopls/internal/test/integration/fake/client.go b/gopls/internal/test/integration/fake/client.go
@@ -8,6 +8,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"path"
+	"path/filepath"
 
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/test/integration/fake/glob"
@@ -130,8 +132,15 @@ func (c *Client) RegisterCapability(ctx context.Context, params *protocol.Regist
 			}
 			var globs []*glob.Glob
 			for _, watcher := range opts.Watchers {
+				var globPattern string
+				switch pattern := watcher.GlobPattern.Value.(type) {
+				case protocol.Pattern:
+					globPattern = pattern
+				case protocol.RelativePattern:
+					globPattern = path.Join(filepath.ToSlash(pattern.BaseURI.Path()), pattern.Pattern)
+				}
 				// TODO(rfindley): honor the watch kind.
-				g, err := glob.Parse(watcher.GlobPattern)
+				g, err := glob.Parse(globPattern)
 				if err != nil {
 					return fmt.Errorf("error parsing glob pattern %q: %v", watcher.GlobPattern, err)
 				}