gopls/internal/lsp/cache: simplify tracking of snapshot directories

Great care was taken to track known directories in the snapshot without
blocking in snapshot.Clone, introducing significant complexity.

This complexity can be avoided by instead keeping track of observed
directories as files are set in the snapshot. These directories need
only be reset when files are deleted from the snapshot, which is a
relatively rare event.

Also rename filesMap->fileMap, and move to filemap.go, with a new unit
test.

This reduces some path dependence on seen files, as the set of
directories is well defined and depends only on the files in the
snapshot. Previously, when a file was removed, gopls called Stat to
check if the directory still existed, which leads to path dependence: an
add+remove was not the same as nothing at all.

Updates golang/go#57558

Change-Id: I5fd89ce870fa7d8afd19471d150396b1e4ea8875
Reviewed-on: https://go-review.googlesource.com/c/tools/+/525616
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopls/internal/lsp/cache/filemap.go

@@ -0,0 +1,134 @@
+// Copyright 2022 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 cache
+
+import (
+	"path/filepath"
+
+	"golang.org/x/tools/gopls/internal/lsp/source"
+	"golang.org/x/tools/gopls/internal/span"
+	"golang.org/x/tools/internal/persistent"
+)
+
+// A fileMap maps files in the snapshot, with some additional bookkeeping:
+// It keeps track of overlays as well as directories containing any observed
+// file.
+type fileMap struct {
+	files    *persistent.Map[span.URI, source.FileHandle]
+	overlays *persistent.Map[span.URI, *Overlay] // the subset of files that are overlays
+	dirs     *persistent.Set[string]             // all dirs containing files; if nil, dirs have not been initialized
+}
+
+func newFileMap() *fileMap {
+	return &fileMap{
+		files:    new(persistent.Map[span.URI, source.FileHandle]),
+		overlays: new(persistent.Map[span.URI, *Overlay]),
+		dirs:     new(persistent.Set[string]),
+	}
+}
+
+func (m *fileMap) Clone() *fileMap {
+	m2 := &fileMap{
+		files:    m.files.Clone(),
+		overlays: m.overlays.Clone(),
+	}
+	if m.dirs != nil {
+		m2.dirs = m.dirs.Clone()
+	}
+	return m2
+}
+
+func (m *fileMap) Destroy() {
+	m.files.Destroy()
+	m.overlays.Destroy()
+	if m.dirs != nil {
+		m.dirs.Destroy()
+	}
+}
+
+// Get returns the file handle mapped by the given key, or (nil, false) if the
+// key is not present.
+func (m *fileMap) Get(key span.URI) (source.FileHandle, bool) {
+	return m.files.Get(key)
+}
+
+// Range calls f for each (uri, fh) in the map.
+func (m *fileMap) Range(f func(uri span.URI, fh source.FileHandle)) {
+	m.files.Range(f)
+}
+
+// Set stores the given file handle for key, updating overlays and directories
+// accordingly.
+func (m *fileMap) Set(key span.URI, fh source.FileHandle) {
+	m.files.Set(key, fh, nil)
+
+	// update overlays
+	if o, ok := fh.(*Overlay); ok {
+		m.overlays.Set(key, o, nil)
+	} else {
+		// Setting a non-overlay must delete the corresponding overlay, to preserve
+		// the accuracy of the overlay set.
+		m.overlays.Delete(key)
+	}
+
+	// update dirs
+	if m.dirs == nil {
+		m.initDirs()
+	} else {
+		m.addDirs(key)
+	}
+}
+
+func (m *fileMap) initDirs() {
+	m.dirs = new(persistent.Set[string])
+	m.files.Range(func(u span.URI, _ source.FileHandle) {
+		m.addDirs(u)
+	})
+}
+
+// addDirs adds all directories containing u to the dirs set.
+func (m *fileMap) addDirs(u span.URI) {
+	dir := filepath.Dir(u.Filename())
+	for dir != "" && !m.dirs.Contains(dir) {
+		m.dirs.Add(dir)
+		dir = filepath.Dir(dir)
+	}
+}
+
+// Delete removes a file from the map, and updates overlays and dirs
+// accordingly.
+func (m *fileMap) Delete(key span.URI) {
+	m.files.Delete(key)
+	m.overlays.Delete(key)
+
+	// Deleting a file may cause the set of dirs to shrink; therefore we must
+	// re-evaluate the dir set.
+	//
+	// Do this lazily, to avoid work if there are multiple deletions in a row.
+	if m.dirs != nil {
+		m.dirs.Destroy()
+		m.dirs = nil
+	}
+}
+
+// Overlays returns a new unordered array of overlay files.
+func (m *fileMap) Overlays() []*Overlay {
+	var overlays []*Overlay
+	m.overlays.Range(func(_ span.URI, o *Overlay) {
+		overlays = append(overlays, o)
+	})
+	return overlays
+}
+
+// Dirs reports returns the set of dirs observed by the fileMap.
+//
+// This operation mutates the fileMap.
+// The result must not be mutated by the caller.
+func (m *fileMap) Dirs() *persistent.Set[string] {
+	if m.dirs == nil {
+		m.initDirs()
+	}
+	return m.dirs
+}

gopls/internal/lsp/cache/filemap_test.go

@@ -0,0 +1,108 @@
+// 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 cache
+
+import (
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"golang.org/x/tools/gopls/internal/lsp/source"
+	"golang.org/x/tools/gopls/internal/span"
+)
+
+func TestFileMap(t *testing.T) {
+	const (
+		set = iota
+		del
+	)
+	type op struct {
+		op      int // set or remove
+		path    string
+		overlay bool
+	}
+	tests := []struct {
+		label        string
+		ops          []op
+		wantFiles    []string
+		wantOverlays []string
+		wantDirs     []string
+	}{
+		{"empty", nil, nil, nil, nil},
+		{"singleton", []op{
+			{set, "/a/b", false},
+		}, []string{"/a/b"}, nil, []string{"/", "/a"}},
+		{"overlay", []op{
+			{set, "/a/b", true},
+		}, []string{"/a/b"}, []string{"/a/b"}, []string{"/", "/a"}},
+		{"replace overlay", []op{
+			{set, "/a/b", true},
+			{set, "/a/b", false},
+		}, []string{"/a/b"}, nil, []string{"/", "/a"}},
+		{"multi dir", []op{
+			{set, "/a/b", false},
+			{set, "/c/d", false},
+		}, []string{"/a/b", "/c/d"}, nil, []string{"/", "/a", "/c"}},
+		{"empty dir", []op{
+			{set, "/a/b", false},
+			{set, "/c/d", false},
+			{del, "/a/b", false},
+		}, []string{"/c/d"}, nil, []string{"/", "/c"}},
+	}
+
+	// Normalize paths for windows compatibility.
+	normalize := func(path string) string {
+		return strings.TrimPrefix(filepath.ToSlash(path), "C:") // the span packages adds 'C:'
+	}
+
+	for _, test := range tests {
+		t.Run(test.label, func(t *testing.T) {
+			m := newFileMap()
+			for _, op := range test.ops {
+				uri := span.URIFromPath(filepath.FromSlash(op.path))
+				switch op.op {
+				case set:
+					var fh source.FileHandle
+					if op.overlay {
+						fh = &Overlay{uri: uri}
+					} else {
+						fh = &DiskFile{uri: uri}
+					}
+					m.Set(uri, fh)
+				case del:
+					m.Delete(uri)
+				}
+			}
+
+			var gotFiles []string
+			m.Range(func(uri span.URI, _ source.FileHandle) {
+				gotFiles = append(gotFiles, normalize(uri.Filename()))
+			})
+			sort.Strings(gotFiles)
+			if diff := cmp.Diff(test.wantFiles, gotFiles); diff != "" {
+				t.Errorf("Files mismatch (-want +got):\n%s", diff)
+			}
+
+			var gotOverlays []string
+			for _, o := range m.Overlays() {
+				gotOverlays = append(gotOverlays, normalize(o.URI().Filename()))
+			}
+			if diff := cmp.Diff(test.wantOverlays, gotOverlays); diff != "" {
+				t.Errorf("Overlays mismatch (-want +got):\n%s", diff)
+			}
+
+			var gotDirs []string
+			m.Dirs().Range(func(dir string) {
+				gotDirs = append(gotDirs, normalize(dir))
+			})
+			sort.Strings(gotDirs)
+			if diff := cmp.Diff(test.wantDirs, gotDirs); diff != "" {
+				t.Errorf("Dirs mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

gopls/internal/lsp/cache/maps.go

@@ -1,78 +0,0 @@
-// Copyright 2022 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 cache
-
-import (
-	"golang.org/x/tools/gopls/internal/lsp/source"
-	"golang.org/x/tools/gopls/internal/span"
-	"golang.org/x/tools/internal/persistent"
-)
-
-type filesMap struct {
-	impl       *persistent.Map[span.URI, source.FileHandle]
-	overlayMap map[span.URI]*Overlay // the subset that are overlays
-}
-
-func newFilesMap() filesMap {
-	return filesMap{
-		impl:       new(persistent.Map[span.URI, source.FileHandle]),
-		overlayMap: make(map[span.URI]*Overlay),
-	}
-}
-
-func (m filesMap) Clone() filesMap {
-	overlays := make(map[span.URI]*Overlay, len(m.overlayMap))
-	for k, v := range m.overlayMap {
-		overlays[k] = v
-	}
-	return filesMap{
-		impl:       m.impl.Clone(),
-		overlayMap: overlays,
-	}
-}
-
-func (m filesMap) Destroy() {
-	m.impl.Destroy()
-}
-
-func (m filesMap) Get(key span.URI) (source.FileHandle, bool) {
-	value, ok := m.impl.Get(key)
-	if !ok {
-		return nil, false
-	}
-	return value.(source.FileHandle), true
-}
-
-func (m filesMap) Range(do func(key span.URI, value source.FileHandle)) {
-	m.impl.Range(do)
-}
-
-func (m filesMap) Set(key span.URI, value source.FileHandle) {
-	m.impl.Set(key, value, nil)
-
-	if o, ok := value.(*Overlay); ok {
-		m.overlayMap[key] = o
-	} else {
-		// Setting a non-overlay must delete the corresponding overlay, to preserve
-		// the accuracy of the overlay set.
-		delete(m.overlayMap, key)
-	}
-}
-
-func (m *filesMap) Delete(key span.URI) {
-	m.impl.Delete(key)
-	delete(m.overlayMap, key)
-}
-
-// overlays returns a new unordered array of overlay files.
-func (m filesMap) overlays() []*Overlay {
-	// In practice we will always have at least one overlay, so there is no need
-	// to optimize for the len=0 case by returning a nil slice.
-	overlays := make([]*Overlay, 0, len(m.overlayMap))
-	for _, o := range m.overlayMap {
-		overlays = append(overlays, o)
-	}
-	return overlays
-}

gopls/internal/lsp/cache/session.go

@@ -172,7 +172,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		store:                s.cache.store,
 		packages:             new(persistent.Map[PackageID, *packageHandle]),
 		meta:                 new(metadataGraph),
-		files:                newFilesMap(),
+		files:                newFileMap(),
 		activePackages:       new(persistent.Map[PackageID, *Package]),
 		symbolizeHandles:     new(persistent.Map[span.URI, *memoize.Promise]),
 		workspacePackages:    make(map[PackageID]PackagePath),
@@ -182,7 +182,6 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		modTidyHandles:       new(persistent.Map[span.URI, *memoize.Promise]),
 		modVulnHandles:       new(persistent.Map[span.URI, *memoize.Promise]),
 		modWhyHandles:        new(persistent.Map[span.URI, *memoize.Promise]),
-		knownSubdirs:         new(persistent.Set[span.URI]),
 		workspaceModFiles:    wsModFiles,
 		workspaceModFilesErr: wsModFilesErr,
 		pkgIndex:             typerefs.NewPackageIndex(),
@@ -594,15 +593,23 @@ func (s *Session) ExpandModificationsToDirectories(ctx context.Context, changes
 	}
 	s.viewMu.Unlock()
 
-	knownDirs := knownDirectories(ctx, snapshots)
-	defer knownDirs.Destroy()
-
+	// Expand the modification to any file we could care about, which we define
+	// to be any file observed by any of the snapshots.
+	//
+	// There may be other files in the directory, but if we haven't read them yet
+	// we don't need to invalidate them.
 	var result []source.FileModification
 	for _, c := range changes {
-		if !knownDirs.Contains(c.URI) {
+		expanded := make(map[span.URI]bool)
+		for _, snapshot := range snapshots {
+			for _, uri := range snapshot.filesInDir(c.URI) {
+				expanded[uri] = true
+			}
+		}
+		if len(expanded) == 0 {
 			result = append(result, c)
 		} else {
-			for uri := range knownFilesInDir(ctx, snapshots, c.URI) {
+			for uri := range expanded {
 				result = append(result, source.FileModification{
 					URI:        uri,
 					Action:     c.Action,
@@ -616,36 +623,6 @@ func (s *Session) ExpandModificationsToDirectories(ctx context.Context, changes
 	return result
 }
 
-// knownDirectories returns all of the directories known to the given
-// snapshots, including workspace directories and their subdirectories.
-// It is responsibility of the caller to destroy the returned set.
-func knownDirectories(ctx context.Context, snapshots []*snapshot) *persistent.Set[span.URI] {
-	result := new(persistent.Set[span.URI])
-	for _, snapshot := range snapshots {
-		dirs := snapshot.dirs(ctx)
-		for _, dir := range dirs {
-			result.Add(dir)
-		}
-		knownSubdirs := snapshot.getKnownSubdirs(dirs)
-		result.AddAll(knownSubdirs)
-		knownSubdirs.Destroy()
-	}
-	return result
-}
-
-// knownFilesInDir returns the files known to the snapshots in the session.
-// It does not respect symlinks.
-func knownFilesInDir(ctx context.Context, snapshots []*snapshot, dir span.URI) map[span.URI]struct{} {
-	files := map[span.URI]struct{}{}
-
-	for _, snapshot := range snapshots {
-		for _, uri := range snapshot.knownFilesInDir(ctx, dir) {
-			files[uri] = struct{}{}
-		}
-	}
-	return files
-}
-
 // Precondition: caller holds s.viewMu lock.
 // TODO(rfindley): move this to fs_overlay.go.
 func (fs *overlayFS) updateOverlays(ctx context.Context, changes []source.FileModification) error {

gopls/internal/lsp/cache/snapshot.go

@@ -97,7 +97,7 @@ type snapshot struct {
 
 	// files maps file URIs to their corresponding FileHandles.
 	// It may invalidated when a file's content changes.
-	files filesMap
+	files *fileMap
 
 	// symbolizeHandles maps each file URI to a handle for the future
 	// result of computing the symbols declared in that file.
@@ -150,15 +150,6 @@ type snapshot struct {
 	modWhyHandles  *persistent.Map[span.URI, *memoize.Promise] // *memoize.Promise[modWhyResult]
 	modVulnHandles *persistent.Map[span.URI, *memoize.Promise] // *memoize.Promise[modVulnResult]
 
-	// knownSubdirs is the set of subdirectory URIs in the workspace,
-	// used to create glob patterns for file watching.
-	knownSubdirs      *persistent.Set[span.URI]
-	knownSubdirsCache map[string]struct{} // memo of knownSubdirs as a set of filenames
-	// unprocessedSubdirChanges are any changes that might affect the set of
-	// subdirectories in the workspace. They are not reflected to knownSubdirs
-	// during the snapshot cloning step as it can slow down cloning.
-	unprocessedSubdirChanges []*fileChange
-
 	// workspaceModFiles holds the set of mod files active in this snapshot.
 	//
 	// This is either empty, a single entry for the workspace go.mod file, or the
@@ -262,7 +253,6 @@ func (s *snapshot) destroy(destroyedBy string) {
 	s.packages.Destroy()
 	s.activePackages.Destroy()
 	s.files.Destroy()
-	s.knownSubdirs.Destroy()
 	s.symbolizeHandles.Destroy()
 	s.parseModHandles.Destroy()
 	s.parseWorkHandles.Destroy()
@@ -629,7 +619,7 @@ func (s *snapshot) overlays() []*Overlay {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	return s.files.overlays()
+	return s.files.Overlays()
 }
 
 // Package data kinds, identifying various package data that may be stored in
@@ -899,10 +889,8 @@ func (s *snapshot) resetActivePackagesLocked() {
 	s.activePackages = new(persistent.Map[PackageID, *Package])
 }
 
-const fileExtensions = "go,mod,sum,work"
-
 func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]struct{} {
-	extensions := fileExtensions
+	extensions := "go,mod,sum,work"
 	for _, ext := range s.Options().TemplateExtensions {
 		extensions += "," + ext
 	}
@@ -920,19 +908,17 @@ func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 	}
 
 	// Add a pattern for each Go module in the workspace that is not within the view.
-	dirs := s.dirs(ctx)
+	dirs := s.workspaceDirs(ctx)
 	for _, dir := range dirs {
-		dirName := dir.Filename()
-
 		// If the directory is within the view's folder, we're already watching
 		// it with the first pattern above.
-		if source.InDir(s.view.folder.Filename(), dirName) {
+		if source.InDir(s.view.folder.Filename(), dir) {
 			continue
 		}
 		// TODO(rstambler): If microsoft/vscode#3025 is resolved before
 		// microsoft/vscode#101042, we will need a work-around for Windows
 		// drive letter casing.
-		patterns[fmt.Sprintf("%s/**/*.{%s}", dirName, extensions)] = struct{}{}
+		patterns[fmt.Sprintf("%s/**/*.{%s}", dir, extensions)] = struct{}{}
 	}
 
 	if s.watchSubdirs() {
@@ -943,6 +929,11 @@ func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 		// directories. There may be thousands of patterns, each a single
 		// directory.
 		//
+		// We compute this set by looking at files that we've previously observed.
+		// This may miss changed to directories that we haven't observed, but that
+		// shouldn't matter as there is nothing to invalidate (if a directory falls
+		// in forest, etc).
+		//
 		// (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
@@ -956,6 +947,46 @@ func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]stru
 	return patterns
 }
 
+func (s *snapshot) addKnownSubdirs(patterns map[string]unit, wsDirs []string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	s.files.Dirs().Range(func(dir string) {
+		for _, wsDir := range wsDirs {
+			if source.InDir(wsDir, dir) {
+				patterns[dir] = unit{}
+			}
+		}
+	})
+}
+
+// workspaceDirs returns the workspace directories for the loaded modules.
+//
+// A workspace directory is, roughly speaking, a directory for which we care
+// about file changes.
+func (s *snapshot) workspaceDirs(ctx context.Context) []string {
+	dirSet := make(map[string]unit)
+
+	// Dirs should, at the very least, contain the working directory and folder.
+	dirSet[s.view.workingDir().Filename()] = unit{}
+	dirSet[s.view.folder.Filename()] = unit{}
+
+	// Additionally, if e.g. go.work indicates other workspace modules, we should
+	// include their directories too.
+	if s.workspaceModFilesErr == nil {
+		for modFile := range s.workspaceModFiles {
+			dir := filepath.Dir(modFile.Filename())
+			dirSet[dir] = unit{}
+		}
+	}
+	var dirs []string
+	for d := range dirSet {
+		dirs = append(dirs, d)
+	}
+	sort.Strings(dirs)
+	return dirs
+}
+
 // 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
@@ -985,127 +1016,19 @@ func (s *snapshot) watchSubdirs() bool {
 	}
 }
 
-func (s *snapshot) addKnownSubdirs(patterns map[string]struct{}, wsDirs []span.URI) {
+// filesInDir returns all files observed by the snapshot that are contained in
+// a directory with the provided URI.
+func (s *snapshot) filesInDir(uri span.URI) []span.URI {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	// First, process any pending changes and update the set of known
-	// subdirectories.
-	// It may change list of known subdirs and therefore invalidate the cache.
-	s.applyKnownSubdirsChangesLocked(wsDirs)
-
-	// TODO(adonovan): is it still necessary to memoize the Range
-	// and URI.Filename operations?
-	if s.knownSubdirsCache == nil {
-		s.knownSubdirsCache = make(map[string]struct{})
-		s.knownSubdirs.Range(func(uri span.URI) {
-			s.knownSubdirsCache[uri.Filename()] = struct{}{}
-		})
+	dir := uri.Filename()
+	if !s.files.Dirs().Contains(dir) {
+		return nil
 	}
-
-	for pattern := range s.knownSubdirsCache {
-		patterns[pattern] = struct{}{}
-	}
-}
-
-// collectAllKnownSubdirs collects all of the subdirectories within the
-// snapshot's workspace directories. None of the workspace directories are
-// included.
-func (s *snapshot) collectAllKnownSubdirs(ctx context.Context) {
-	dirs := s.dirs(ctx)
-
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	s.knownSubdirs.Destroy()
-	s.knownSubdirs = new(persistent.Set[span.URI])
-	s.knownSubdirsCache = nil
-	s.files.Range(func(uri span.URI, fh source.FileHandle) {
-		s.addKnownSubdirLocked(uri, dirs)
-	})
-}
-
-func (s *snapshot) getKnownSubdirs(wsDirs []span.URI) *persistent.Set[span.URI] {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	// First, process any pending changes and update the set of known
-	// subdirectories.
-	s.applyKnownSubdirsChangesLocked(wsDirs)
-
-	return s.knownSubdirs.Clone()
-}
-
-func (s *snapshot) applyKnownSubdirsChangesLocked(wsDirs []span.URI) {
-	for _, c := range s.unprocessedSubdirChanges {
-		if c.isUnchanged {
-			continue
-		}
-		if !c.exists {
-			s.removeKnownSubdirLocked(c.fileHandle.URI())
-		} else {
-			s.addKnownSubdirLocked(c.fileHandle.URI(), wsDirs)
-		}
-	}
-	s.unprocessedSubdirChanges = nil
-}
-
-func (s *snapshot) addKnownSubdirLocked(uri span.URI, dirs []span.URI) {
-	dir := filepath.Dir(uri.Filename())
-	// First check if the directory is already known, because then we can
-	// return early.
-	if s.knownSubdirs.Contains(span.URIFromPath(dir)) {
-		return
-	}
-	var matched span.URI
-	for _, wsDir := range dirs {
-		if source.InDir(wsDir.Filename(), dir) {
-			matched = wsDir
-			break
-		}
-	}
-	// Don't watch any directory outside of the workspace directories.
-	if matched == "" {
-		return
-	}
-	for {
-		if dir == "" || dir == matched.Filename() {
-			break
-		}
-		uri := span.URIFromPath(dir)
-		if s.knownSubdirs.Contains(uri) {
-			break
-		}
-		s.knownSubdirs.Add(uri)
-		dir = filepath.Dir(dir)
-		s.knownSubdirsCache = nil
-	}
-}
-
-func (s *snapshot) removeKnownSubdirLocked(uri span.URI) {
-	dir := filepath.Dir(uri.Filename())
-	for dir != "" {
-		uri := span.URIFromPath(dir)
-		if !s.knownSubdirs.Contains(uri) {
-			break
-		}
-		if info, _ := os.Stat(dir); info == nil {
-			s.knownSubdirs.Remove(uri)
-			s.knownSubdirsCache = nil
-		}
-		dir = filepath.Dir(dir)
-	}
-}
-
-// knownFilesInDir returns the files known to the given snapshot that are in
-// the given directory. It does not respect symlinks.
-func (s *snapshot) knownFilesInDir(ctx context.Context, dir span.URI) []span.URI {
 	var files []span.URI
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	s.files.Range(func(uri span.URI, fh source.FileHandle) {
-		if source.InDir(dir.Filename(), uri.Filename()) {
+	s.files.Range(func(uri span.URI, _ source.FileHandle) {
+		if source.InDir(dir, uri.Filename()) {
 			files = append(files, uri)
 		}
 	})
@@ -1976,7 +1899,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		modTidyHandles:       s.modTidyHandles.Clone(),
 		modWhyHandles:        s.modWhyHandles.Clone(),
 		modVulnHandles:       s.modVulnHandles.Clone(),
-		knownSubdirs:         s.knownSubdirs.Clone(),
 		workspaceModFiles:    wsModFiles,
 		workspaceModFilesErr: wsModFilesErr,
 		importGraph:          s.importGraph,
@@ -2000,15 +1922,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 	//
 	// Maybe not, as major configuration changes cause a new view.
 
-	// Add all of the known subdirectories, but don't update them for the
-	// changed files. We need to rebuild the workspace module to know the
-	// true set of known subdirectories, but we don't want to do that in clone.
-	result.knownSubdirs = s.knownSubdirs.Clone()
-	result.knownSubdirsCache = s.knownSubdirsCache
-	for _, c := range changes {
-		result.unprocessedSubdirChanges = append(result.unprocessedSubdirChanges, c)
-	}
-
 	// directIDs keeps track of package IDs that have directly changed.
 	// Note: this is not a set, it's a map from id to invalidateMetadata.
 	directIDs := map[PackageID]bool{}
@@ -2099,14 +2012,17 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 
 		result.parseModHandles.Delete(uri)
 		result.parseWorkHandles.Delete(uri)
+
 		// Handle the invalidated file; it may have new contents or not exist.
+		//
+		// Note, we can't simply delete the file unconditionally and let it be
+		// re-read, as (1) the snapshot must observe all overlays, and (2) deleting
+		// a file forces directories to be reevaluated, as it may be the last file
+		// in a directory. We want to avoid that work in the common case where a
+		// file has simply changed.
 		if !change.exists {
 			result.files.Delete(uri)
 		} else {
-			// TODO(golang/go#57558): the line below is strictly necessary to ensure
-			// that snapshots have each overlay, but it is problematic that we must
-			// set any content in snapshot.clone: if the file has changed, let it be
-			// re-read.
 			result.files.Set(uri, change.fileHandle)
 		}
 

gopls/internal/lsp/cache/view.go

@@ -778,7 +778,6 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 	}
 
 	s.loadWorkspace(ctx, firstAttempt)
-	s.collectAllKnownSubdirs(ctx)
 }
 
 func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadErr error) {

gopls/internal/lsp/cache/workspace.go

@@ -11,7 +11,6 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
-	"sort"
 	"strings"
 
 	"golang.org/x/mod/modfile"
@@ -60,38 +59,6 @@ func computeWorkspaceModFiles(ctx context.Context, gomod, gowork span.URI, go111
 	return nil, nil
 }
 
-// dirs returns the workspace directories for the loaded modules.
-//
-// A workspace directory is, roughly speaking, a directory for which we care
-// about file changes. This is used for the purpose of registering file
-// watching patterns, and expanding directory modifications to their adjacent
-// files.
-//
-// TODO(rfindley): move this to snapshot.go.
-// TODO(rfindley): can we make this abstraction simpler and/or more accurate?
-func (s *snapshot) dirs(ctx context.Context) []span.URI {
-	dirSet := make(map[span.URI]struct{})
-
-	// Dirs should, at the very least, contain the working directory and folder.
-	dirSet[s.view.workingDir()] = struct{}{}
-	dirSet[s.view.folder] = struct{}{}
-
-	// Additionally, if e.g. go.work indicates other workspace modules, we should
-	// include their directories too.
-	if s.workspaceModFilesErr == nil {
-		for modFile := range s.workspaceModFiles {
-			dir := filepath.Dir(modFile.Filename())
-			dirSet[span.URIFromPath(dir)] = struct{}{}
-		}
-	}
-	var dirs []span.URI
-	for d := range dirSet {
-		dirs = append(dirs, d)
-	}
-	sort.Slice(dirs, func(i, j int) bool { return dirs[i] < dirs[j] })
-	return dirs
-}
-
 // isGoMod reports if uri is a go.mod file.
 func isGoMod(uri span.URI) bool {
 	return filepath.Base(uri.Filename()) == "go.mod"

internal/persistent/map.go

@@ -30,8 +30,8 @@ import (
 // Map is an associative mapping from keys to values.
 //
 // Maps can be Cloned in constant time.
-// Get, Store, and Delete operations are done on average in logarithmic time.
-// Maps can be Updated in O(m log(n/m)) time for maps of size n and m, where m < n.
+// Get, Set, and Delete operations are done on average in logarithmic time.
+// Maps can be merged (via SetAll) in O(m log(n/m)) time for maps of size n and m, where m < n.
 //
 // Values are reference counted, and a client-supplied release function
 // is called when a value is no longer referenced by a map or any clone.