Ensure node_modules buckets are removed from auto-import registry once they no longer exist (#4303)

Author: andrewbranch

internal/ls/autoimport/registry.go

@@ -633,11 +633,6 @@ func (b *registryBuilder) updateBucketAndDirectoryExistence(change RegistryChang
 			})
 		}
 
-		if packageJsonChanged {
-			// package.json changes affecting node_modules are handled by comparing dependencies in updateIndexes
-			return
-		}
-
 		if hasNodeModules {
 			if _, ok := b.nodeModules.Get(dirPath); !ok {
 				b.nodeModules.Add(dirPath, newRegistryBucket())
@@ -648,12 +643,15 @@ func (b *registryBuilder) updateBucketAndDirectoryExistence(change RegistryChang
 	}
 
 	var addedNodeModulesDirs, removedNodeModulesDirs []tspath.Path
+	packageJsonChanged := func(dirName string) bool {
+		uri := lsconv.FileNameToDocumentURI(tspath.CombinePaths(dirName, "package.json"))
+		return change.Changed.Has(uri) || change.Deleted.Has(uri) || change.Created.Has(uri)
+	}
 	core.DiffMapsFunc(
 		b.base.directories,
 		neededDirectories,
 		func(dir *directory, dirName string) bool {
-			packageJsonUri := lsconv.FileNameToDocumentURI(tspath.CombinePaths(dirName, "package.json"))
-			return !change.Changed.Has(packageJsonUri) && !change.Deleted.Has(packageJsonUri) && !change.Created.Has(packageJsonUri)
+			return !packageJsonChanged(dirName) && dir.hasNodeModules == b.host.FS().DirectoryExists(tspath.CombinePaths(dirName, "node_modules"))
 		},
 		func(dirPath tspath.Path, dirName string) {
 			// Need and don't have
@@ -679,13 +677,13 @@ func (b *registryBuilder) updateBucketAndDirectoryExistence(change RegistryChang
 			}
 		},
 		func(dirPath tspath.Path, dir *directory, dirName string) {
-			// package.json may have changed
-			updateDirectory(dirPath, dirName, true)
+			updateDirectory(dirPath, dirName, packageJsonChanged(dirName))
 			if logger != nil {
 				logger.Logf("Changed directory: %s", dirPath)
 			}
 		},
 	)
+
 	if logger != nil {
 		for _, dirPath := range addedNodeModulesDirs {
 			logger.Logf("Added node_modules bucket: %s", dirPath)
@@ -797,7 +795,6 @@ func (b *registryBuilder) updateIndexes(ctx context.Context, change RegistryChan
 		packageNames          *collections.Set[string]
 		directoryPackageNames *collections.Set[string]
 		discovered            []*discoveredPackage
-		discoverErr           error
 	}
 
 	projectPath, _ := b.host.GetDefaultProject(change.RequestedFile)
@@ -890,12 +887,7 @@ func (b *registryBuilder) updateIndexes(ctx context.Context, change RegistryChan
 			if task.isUpdate {
 				task.packageNames = task.dirtyPackages
 			} else {
-				var err error
-				task.directoryPackageNames, err = getPackageNamesInNodeModules(tspath.CombinePaths(task.dirName, "node_modules"), b.host.FS())
-				if err != nil {
-					task.discoverErr = err
-					return
-				}
+				task.directoryPackageNames = getPackageNamesInNodeModules(tspath.CombinePaths(task.dirName, "node_modules"), b.host.FS())
 				task.packageNames = core.Coalesce(task.dependencyNames, task.directoryPackageNames)
 			}
 			task.discovered = b.discoverBucketPackages(task.packageNames, task.dirName, task.dirPath)
@@ -918,9 +910,6 @@ func (b *registryBuilder) updateIndexes(ctx context.Context, change RegistryChan
 	// filter to only those whose main extraction failed, then deduplicate by typesRealpath.
 	var typesFallbackCandidates []*discoveredPackage
 	for _, task := range nodeModulesTasks {
-		if task.discoverErr != nil {
-			continue
-		}
 		for _, pkg := range task.discovered {
 			if pkg.realpath != "" {
 				if !seen[pkg.realpath] {
@@ -1007,10 +996,6 @@ func (b *registryBuilder) updateIndexes(ctx context.Context, change RegistryChan
 	for _, task := range nodeModulesTasks {
 		br := &bucketBuildResult{entry: task.entry}
 		allResults = append(allResults, br)
-		if task.discoverErr != nil {
-			br.err = task.discoverErr
-			continue
-		}
 		wg.Go(func() {
 			if task.isUpdate {
 				b.updateNodeModulesBucket(

internal/ls/autoimport/registry_test.go

@@ -228,6 +228,127 @@ export const bar = 2;`,
 		assert.Equal(t, len(stats.ProjectBuckets), 1)
 	})
 
+	t.Run("deleting node_modules leaves the registry prepared for importing", func(t *testing.T) {
+		t.Parallel()
+		fixture := autoimporttestutil.SetupLifecycleSession(t, lifecycleProjectRoot, 1)
+		session := fixture.Session()
+		sessionUtils := fixture.Utils()
+		project := fixture.SingleProject()
+		mainFile := project.File(0)
+		ctx := context.Background()
+
+		preferences := lsutil.NewDefaultUserPreferences()
+		preferences.IncludeCompletionsForModuleExports = core.TSTrue
+		preferences.IncludeCompletionsForImportStatements = core.TSTrue
+
+		// Build auto-imports once so both buckets are clean and prepared.
+		session.DidOpenFile(ctx, mainFile.URI(), 1, mainFile.Content(), lsproto.LanguageKindTypeScript)
+		_, err := session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+
+		snapshot := session.Snapshot()
+		defaultProject := snapshot.GetDefaultProject(mainFile.URI())
+		assert.Assert(t, defaultProject != nil)
+		projectPath := defaultProject.ConfigFilePath()
+		assert.Assert(t, snapshot.AutoImportRegistry().IsPreparedForImportingFile(mainFile.FileName(), projectPath, preferences))
+		assert.Equal(t, len(autoImportStats(t, session).NodeModulesBuckets), 1)
+
+		// Simulate the user deleting node_modules: remove the directory from disk
+		// and notify the session of the deletion, which marks the node_modules
+		// bucket dirty.
+		nodeModulesDir := tspath.CombinePaths(project.Root(), "node_modules")
+		assert.NilError(t, sessionUtils.FS().Remove(nodeModulesDir))
+		session.DidChangeWatchedFiles(ctx, []*lsproto.FileEvent{
+			{Type: lsproto.FileChangeTypeDeleted, Uri: lsconv.FileNameToDocumentURI(nodeModulesDir)},
+		})
+
+		// Re-preparing auto-imports must succeed and leave the registry prepared.
+		// Before the fix, the node_modules bucket's rebuild bailed out with
+		// vfs.ErrNotExist, leaving the stale dirty bucket in place so the registry
+		// reported "needs rebuild" forever (which crashed the request handler).
+		_, err = session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+
+		snapshot = session.Snapshot()
+		assert.Assert(t, snapshot.AutoImportRegistry().IsPreparedForImportingFile(mainFile.FileName(), projectPath, preferences),
+			"registry should be prepared after node_modules is deleted")
+		// The node_modules bucket should be removed entirely, not left behind as an
+		// empty bucket.
+		assert.Equal(t, len(autoImportStats(t, session).NodeModulesBuckets), 0)
+	})
+
+	t.Run("deleting node_modules alongside a package.json change removes the bucket", func(t *testing.T) {
+		t.Parallel()
+		fixture := autoimporttestutil.SetupLifecycleSession(t, lifecycleProjectRoot, 1)
+		session := fixture.Session()
+		sessionUtils := fixture.Utils()
+		project := fixture.SingleProject()
+		mainFile := project.File(0)
+		packageJSON := project.PackageJSONFile()
+		ctx := context.Background()
+
+		preferences := lsutil.NewDefaultUserPreferences()
+		preferences.IncludeCompletionsForModuleExports = core.TSTrue
+		preferences.IncludeCompletionsForImportStatements = core.TSTrue
+
+		session.DidOpenFile(ctx, mainFile.URI(), 1, mainFile.Content(), lsproto.LanguageKindTypeScript)
+		_, err := session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+
+		snapshot := session.Snapshot()
+		defaultProject := snapshot.GetDefaultProject(mainFile.URI())
+		assert.Assert(t, defaultProject != nil)
+		projectPath := defaultProject.ConfigFilePath()
+		assert.Equal(t, len(autoImportStats(t, session).NodeModulesBuckets), 1)
+
+		// In a single changeset, edit package.json AND delete node_modules. The
+		// package.json change must not prevent the now-missing node_modules bucket
+		// from being removed.
+		assert.NilError(t, sessionUtils.FS().WriteFile(packageJSON.FileName(), `{"name": "app", "dependencies": {}}`))
+		nodeModulesDir := tspath.CombinePaths(project.Root(), "node_modules")
+		assert.NilError(t, sessionUtils.FS().Remove(nodeModulesDir))
+		session.DidChangeWatchedFiles(ctx, []*lsproto.FileEvent{
+			{Type: lsproto.FileChangeTypeChanged, Uri: packageJSON.URI()},
+			{Type: lsproto.FileChangeTypeDeleted, Uri: lsconv.FileNameToDocumentURI(nodeModulesDir)},
+		})
+
+		_, err = session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+
+		snapshot = session.Snapshot()
+		assert.Assert(t, snapshot.AutoImportRegistry().IsPreparedForImportingFile(mainFile.FileName(), projectPath, preferences))
+		assert.Equal(t, len(autoImportStats(t, session).NodeModulesBuckets), 0)
+	})
+
+	t.Run("deleting a package directory inside node_modules invalidates the bucket", func(t *testing.T) {
+		t.Parallel()
+		fixture := autoimporttestutil.SetupLifecycleSession(t, lifecycleProjectRoot, 1)
+		session := fixture.Session()
+		sessionUtils := fixture.Utils()
+		project := fixture.SingleProject()
+		mainFile := project.File(0)
+		nodePackage := project.NodeModules()[0]
+		ctx := context.Background()
+
+		session.DidOpenFile(ctx, mainFile.URI(), 1, mainFile.Content(), lsproto.LanguageKindTypeScript)
+		_, err := session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+		assert.Assert(t, singleBucket(t, autoImportStats(t, session).NodeModulesBuckets).ExportCount > 0)
+
+		// Delete just the package directory, leaving node_modules itself in place. The
+		// package's files are read transiently by the registry, so they are never tracked
+		// in diskFiles/diskDirectories; only the directory deletion event is reported, and
+		// it must survive snapshotfs filtering to invalidate the bucket.
+		assert.NilError(t, sessionUtils.FS().Remove(nodePackage.Directory))
+		session.DidChangeWatchedFiles(ctx, []*lsproto.FileEvent{
+			{Type: lsproto.FileChangeTypeDeleted, Uri: lsconv.FileNameToDocumentURI(nodePackage.Directory)},
+		})
+
+		_, err = session.GetCurrentLanguageServiceWithAutoImports(ctx, mainFile.URI())
+		assert.NilError(t, err)
+		assert.Equal(t, singleBucket(t, autoImportStats(t, session).NodeModulesBuckets).ExportCount, 0)
+	})
+
 	t.Run("node_modules bucket dependency selection changes with open files", func(t *testing.T) {
 		t.Parallel()
 		monorepoRoot := "/home/src/monorepo"

internal/ls/autoimport/util.go

@@ -85,14 +85,14 @@ func wordIndices(s string) []int {
 	return indices
 }
 
-func getPackageNamesInNodeModules(nodeModulesDir string, fs vfs.FS) (*collections.Set[string], error) {
+func getPackageNamesInNodeModules(nodeModulesDir string, fs vfs.FS) *collections.Set[string] {
 	packageNames := &collections.Set[string]{}
 	if tspath.GetBaseFileName(nodeModulesDir) != "node_modules" {
 		panic("nodeModulesDir is not a node_modules directory")
 	}
-	if !fs.DirectoryExists(nodeModulesDir) {
-		return nil, vfs.ErrNotExist
-	}
+	// A missing node_modules directory yields no entries (GetAccessibleEntries returns
+	// empty), so there's no need to check existence first: a deleted node_modules is
+	// handled upstream in updateBucketAndDirectoryExistence, which drops the bucket.
 	entries := fs.GetAccessibleEntries(nodeModulesDir)
 	for _, baseName := range entries.Directories {
 		if baseName[0] == '.' {
@@ -112,7 +112,7 @@ func getPackageNamesInNodeModules(nodeModulesDir string, fs vfs.FS) (*collection
 		}
 		packageNames.Add(baseName)
 	}
-	return packageNames, nil
+	return packageNames
 }
 
 func getDefaultLikeExportNameFromDeclaration(symbol *ast.Symbol) string {

internal/project/snapshotfs.go

@@ -554,7 +554,10 @@ func (s *snapshotFSBuilder) expandAndFilterWatchEvents(change FileChangeSummary)
 			path := s.toPath(uri.FileName())
 			if _, ok := s.diskDirectories.Get(path); ok {
 				s.collectFilesRecursive(path, &filteredDeleted)
-			} else if s.isRelevantFileName(uri) {
+			} else if s.isRelevantFileName(uri) || isNodeModulesPath(path) {
+				// node_modules deletions must always be preserved for auto-import registry change handlers.
+				// They won't be in diskDirectories since the registry doesn't use the snapshotFSBuilder for
+				// its file system, since we don't want to retain files read there.
 				filteredDeleted.Add(uri)
 			}
 		}
@@ -578,6 +581,14 @@ func (s *snapshotFSBuilder) expandAndFilterWatchEvents(change FileChangeSummary)
 	return change
 }
 
+// isNodeModulesPath reports whether path is a node_modules directory itself or
+// lives inside one. Used to preserve node_modules watch deletions, whose package
+// files are read transiently and therefore never tracked in diskDirectories.
+func isNodeModulesPath(path tspath.Path) bool {
+	s := string(path)
+	return strings.HasSuffix(s, "/node_modules") || strings.Contains(s, "/node_modules/")
+}
+
 // collectFilesRecursive recursively collects all cached file URIs under the
 // given directory path using the diskDirectories and diskFiles maps.
 func (s *snapshotFSBuilder) collectFilesRecursive(dirPath tspath.Path, files *collections.Set[lsproto.DocumentUri]) {

internal/project/snapshotfs_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/microsoft/typescript-go/internal/lsp/lsproto"
 	"github.com/microsoft/typescript-go/internal/project/dirty"
 	"github.com/microsoft/typescript-go/internal/tspath"
+	"github.com/microsoft/typescript-go/internal/vfs"
 	"github.com/microsoft/typescript-go/internal/vfs/vfstest"
 	"gotest.tools/v3/assert"
 )
@@ -1383,3 +1384,99 @@ func TestRealpathAliasLifecycle(t *testing.T) {
 			"snapshot1 must not contain alias added in snapshot2")
 	})
 }
+
+func TestExpandAndFilterWatchEvents(t *testing.T) {
+	t.Parallel()
+
+	toPath := func(fileName string) tspath.Path {
+		return tspath.Path(fileName)
+	}
+
+	newBuilder := func(testFS vfs.FS) *snapshotFSBuilder {
+		return newSnapshotFSBuilder(
+			testFS,
+			make(map[tspath.Path]*Overlay),
+			make(map[tspath.Path]*Overlay),
+			make(map[tspath.Path]*diskFile),
+			make(map[tspath.Path]dirty.CloneableMap[tspath.Path, string]),
+			nil,
+			lsproto.PositionEncodingKindUTF16,
+			toPath,
+		)
+	}
+
+	t.Run("preserves node_modules directory deletion even when untracked", func(t *testing.T) {
+		t.Parallel()
+		// node_modules package files are read transiently and never tracked in
+		// diskDirectories, so a node_modules directory deletion can neither be
+		// expanded nor matched by extension. It must still be preserved.
+		builder := newBuilder(vfstest.FromMap(map[string]string{
+			"/project/index.ts": "export const x = 1;",
+		}, false))
+
+		change := FileChangeSummary{}
+		change.Deleted.Add("file:///project/node_modules")
+
+		expanded := builder.expandAndFilterWatchEvents(change)
+		assert.Assert(t, expanded.Deleted.Has("file:///project/node_modules"),
+			"bare node_modules directory deletion should be preserved")
+	})
+
+	t.Run("preserves deletion of a package directory inside node_modules", func(t *testing.T) {
+		t.Parallel()
+		builder := newBuilder(vfstest.FromMap(map[string]string{
+			"/project/index.ts": "export const x = 1;",
+		}, false))
+
+		change := FileChangeSummary{}
+		change.Deleted.Add("file:///project/node_modules/@scope/pkg")
+
+		expanded := builder.expandAndFilterWatchEvents(change)
+		assert.Assert(t, expanded.Deleted.Has("file:///project/node_modules/@scope/pkg"),
+			"package directory deletion inside node_modules should be preserved")
+	})
+
+	t.Run("drops irrelevant untracked deletion outside node_modules", func(t *testing.T) {
+		t.Parallel()
+		builder := newBuilder(vfstest.FromMap(map[string]string{
+			"/project/index.ts": "export const x = 1;",
+		}, false))
+
+		change := FileChangeSummary{}
+		change.Deleted.Add("file:///project/build")
+
+		expanded := builder.expandAndFilterWatchEvents(change)
+		assert.Equal(t, expanded.Deleted.Len(), 0,
+			"untracked non-node_modules directory deletion should be dropped")
+	})
+
+	t.Run("expands tracked directory deletion into file deletions", func(t *testing.T) {
+		t.Parallel()
+		existingDiskFiles := map[tspath.Path]*diskFile{
+			tspath.Path("/src/foo.ts"): newDiskFile("/src/foo.ts", "const foo = 1;"),
+		}
+		existingDirs := map[tspath.Path]dirty.CloneableMap[tspath.Path, string]{
+			tspath.Path("/"):    {tspath.Path("/src"): "src"},
+			tspath.Path("/src"): {tspath.Path("/src/foo.ts"): "foo.ts"},
+		}
+		builder := newSnapshotFSBuilder(
+			vfstest.FromMap(map[string]string{"/src/foo.ts": "const foo = 1;"}, false),
+			make(map[tspath.Path]*Overlay),
+			make(map[tspath.Path]*Overlay),
+			existingDiskFiles,
+			existingDirs,
+			nil,
+			lsproto.PositionEncodingKindUTF16,
+			toPath,
+		)
+
+		change := FileChangeSummary{}
+		change.Deleted.Add("file:///src")
+
+		expanded := builder.expandAndFilterWatchEvents(change)
+		assert.Assert(t, expanded.Deleted.Has("file:///src/foo.ts"),
+			"tracked directory deletion should expand to contained file deletions")
+		assert.Assert(t, !expanded.Deleted.Has("file:///src"),
+			"the directory URI itself should be replaced by its files")
+	})
+}