[LSP] Fix data races on dirty state and script info version

internal/project/documentregistry.go

@@ -89,12 +89,14 @@ func (r *DocumentRegistry) getDocumentWorker(
 	key registryKey,
 ) *ast.SourceFile {
 	scriptTarget := core.IfElse(scriptInfo.scriptKind == core.ScriptKindJSON, core.ScriptTargetJSON, compilerOptions.GetEmitScriptTarget())
+	scriptInfoVersion := scriptInfo.Version()
+	scriptInfoText := scriptInfo.Text()
 	if entry, ok := r.documents.Load(key); ok {
 		// We have an entry for this file. However, it may be for a different version of
 		// the script snapshot. If so, update it appropriately.
-		if entry.sourceFile.Version != scriptInfo.Version() {
-			sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfo.text, scriptTarget, scanner.JSDocParsingModeParseAll)
-			sourceFile.Version = scriptInfo.Version()
+		if entry.sourceFile.Version != scriptInfoVersion {
+			sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfoText, scriptTarget, scanner.JSDocParsingModeParseAll)
+			sourceFile.Version = scriptInfoVersion
 			entry.mu.Lock()
 			defer entry.mu.Unlock()
 			entry.sourceFile = sourceFile
@@ -103,8 +105,8 @@ func (r *DocumentRegistry) getDocumentWorker(
 		return entry.sourceFile
 	} else {
 		// Have never seen this file with these settings. Create a new source file for it.
-		sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfo.text, scriptTarget, scanner.JSDocParsingModeParseAll)
-		sourceFile.Version = scriptInfo.Version()
+		sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfoText, scriptTarget, scanner.JSDocParsingModeParseAll)
+		sourceFile.Version = scriptInfoVersion
 		entry, _ := r.documents.LoadOrStore(key, &registryEntry{
 			sourceFile: sourceFile,
 			refCount:   0,

internal/project/project.go

@@ -7,6 +7,7 @@ import (
 	"slices"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/microsoft/typescript-go/internal/ast"
 	"github.com/microsoft/typescript-go/internal/collections"
@@ -92,15 +93,13 @@ type Project struct {
 	name string
 	kind Kind
 
-	dirtyStateMu              sync.Mutex
-	initialLoadPending        bool
-	dirty                     bool
-	version                   int
-	hasAddedOrRemovedFiles    bool
-	hasAddedOrRemovedSymlinks bool
-	deferredClose             bool
-	pendingReload             PendingReload
-	dirtyFilePath             tspath.Path
+	mu                 sync.Mutex
+	initialLoadPending bool
+	dirty              bool
+	version            int
+	deferredClose      bool
+	pendingReload      PendingReload
+	dirtyFilePath      tspath.Path
 
 	comparePathsOptions tspath.ComparePathsOptions
 	currentDirectory    string
@@ -114,7 +113,6 @@ type Project struct {
 	rootFileNames     *collections.OrderedMap[tspath.Path, string]
 	compilerOptions   *core.CompilerOptions
 	parsedCommandLine *tsoptions.ParsedCommandLine
-	programMu         sync.Mutex
 	program           *compiler.Program
 	checkerPool       *checkerPool
 
@@ -211,7 +209,7 @@ func (p *Project) GetSourceFile(fileName string, path tspath.Path, languageVersi
 
 // Updates the program if needed.
 func (p *Project) GetProgram() *compiler.Program {
-	p.updateIfDirty()
+	p.updateGraph()
 	return p.program
 }
 
@@ -372,9 +370,9 @@ func (p *Project) getScriptKind(fileName string) core.ScriptKind {
 	return core.GetScriptKindFromFileName(fileName)
 }
 
-func (p *Project) markFileAsDirty(path tspath.Path) {
-	p.dirtyStateMu.Lock()
-	defer p.dirtyStateMu.Unlock()
+func (p *Project) MarkFileAsDirty(path tspath.Path) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
 	if !p.dirty {
 		p.dirty = true
 		p.dirtyFilePath = path
@@ -385,69 +383,57 @@ func (p *Project) markFileAsDirty(path tspath.Path) {
 }
 
 func (p *Project) markAsDirty() {
-	p.dirtyStateMu.Lock()
-	defer p.dirtyStateMu.Unlock()
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.markAsDirtyLocked()
+}
+
+func (p *Project) markAsDirtyLocked() {
 	p.dirtyFilePath = ""
 	if !p.dirty {
 		p.dirty = true
 		p.version++
 	}
 }
 
-// updateIfDirty returns true if the project was updated.
-func (p *Project) updateIfDirty() bool {
-	// !!! p.invalidateResolutionsOfFailedLookupLocations()
-	return p.dirty && p.updateGraph()
-}
-
-func (p *Project) onFileAddedOrRemoved(isSymlink bool) {
-	p.dirtyStateMu.Lock()
-	defer p.dirtyStateMu.Unlock()
-	p.hasAddedOrRemovedFiles = true
-	if isSymlink {
-		p.hasAddedOrRemovedSymlinks = true
-	}
-}
-
 // updateGraph updates the set of files that contribute to the project.
 // Returns true if the set of files in has changed. NOTE: this is the
 // opposite of the return value in Strada, which was frequently inverted,
 // as in `updateProjectIfDirty()`.
 func (p *Project) updateGraph() bool {
-	p.programMu.Lock()
-	defer p.programMu.Unlock()
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
 	if !p.dirty {
 		return false
 	}
 
+	start := time.Now()
 	p.log("Starting updateGraph: Project: " + p.name)
+	var writeFileNames bool
 	oldProgram := p.program
-	hasAddedOrRemovedFiles := p.hasAddedOrRemovedFiles
 	p.initialLoadPending = false
 
 	if p.kind == KindConfigured && p.pendingReload != PendingReloadNone {
 		switch p.pendingReload {
 		case PendingReloadFileNames:
 			p.parsedCommandLine = tsoptions.ReloadFileNamesOfParsedCommandLine(p.parsedCommandLine, p.host.FS())
-			p.setRootFiles(p.parsedCommandLine.FileNames())
+			writeFileNames = p.setRootFiles(p.parsedCommandLine.FileNames())
 		case PendingReloadFull:
-			if err := p.LoadConfig(); err != nil {
+			if err := p.loadConfig(); err != nil {
 				panic(fmt.Sprintf("failed to reload config: %v", err))
 			}
 		}
 		p.pendingReload = PendingReloadNone
 	}
 
-	p.hasAddedOrRemovedFiles = false
-	p.hasAddedOrRemovedSymlinks = false
 	oldProgramReused := p.updateProgram()
 	p.dirty = false
 	p.dirtyFilePath = ""
-	p.log(fmt.Sprintf("Finishing updateGraph: Project: %s version: %d", p.name, p.version))
-	if hasAddedOrRemovedFiles {
+	if writeFileNames {
 		p.log(p.print(true /*writeFileNames*/, true /*writeFileExplanation*/, false /*writeFileVersionAndText*/))
 	} else if p.program != oldProgram {
-		p.log("Different program with same set of files")
+		p.log("Different program with same set of root files")
 	}
 	if !oldProgramReused {
 		if oldProgram != nil {
@@ -461,6 +447,7 @@ func (p *Project) updateGraph() bool {
 		// but in Strada we throttle, so at least sometimes this should be considered top-level?
 		p.updateWatchers(context.TODO())
 	}
+	p.log(fmt.Sprintf("Finishing updateGraph: Project: %s version: %d in %s", p.name, p.version, time.Since(start)))
 	return true
 }
 
@@ -509,6 +496,13 @@ func (p *Project) isRoot(info *ScriptInfo) bool {
 	return p.rootFileNames.Has(info.path)
 }
 
+func (p *Project) RemoveFile(info *ScriptInfo, fileExists bool, detachFromProject bool) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.removeFile(info, fileExists, detachFromProject)
+	p.markAsDirtyLocked()
+}
+
 func (p *Project) removeFile(info *ScriptInfo, fileExists bool, detachFromProject bool) {
 	if p.isRoot(info) {
 		switch p.kind {
@@ -530,7 +524,13 @@ func (p *Project) removeFile(info *ScriptInfo, fileExists bool, detachFromProjec
 	if detachFromProject {
 		info.detachFromProject(p)
 	}
-	p.markAsDirty()
+}
+
+func (p *Project) AddRoot(info *ScriptInfo) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.addRoot(info)
+	p.markAsDirtyLocked()
 }
 
 func (p *Project) addRoot(info *ScriptInfo) {
@@ -544,10 +544,17 @@ func (p *Project) addRoot(info *ScriptInfo) {
 	}
 	p.rootFileNames.Set(info.path, info.fileName)
 	info.attachToProject(p)
-	p.markAsDirty()
 }
 
 func (p *Project) LoadConfig() error {
+	if err := p.loadConfig(); err != nil {
+		return err
+	}
+	p.markAsDirty()
+	return nil
+}
+
+func (p *Project) loadConfig() error {
 	if p.kind != KindConfigured {
 		panic("loadConfig called on non-configured project")
 	}
@@ -582,12 +589,12 @@ func (p *Project) LoadConfig() error {
 		p.compilerOptions = &core.CompilerOptions{}
 		return fmt.Errorf("could not read file %q", p.configFileName)
 	}
-
-	p.markAsDirty()
 	return nil
 }
 
-func (p *Project) setRootFiles(rootFileNames []string) {
+// setRootFiles returns true if the set of root files has changed.
+func (p *Project) setRootFiles(rootFileNames []string) bool {
+	var hasChanged bool
 	newRootScriptInfos := make(map[tspath.Path]struct{}, len(rootFileNames))
 	for _, file := range rootFileNames {
 		scriptKind := p.getScriptKind(file)
@@ -597,6 +604,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
 		scriptInfo := p.host.GetOrCreateScriptInfoForFile(file, path, scriptKind)
 		newRootScriptInfos[path] = struct{}{}
 		isAlreadyRoot := p.rootFileNames.Has(path)
+		hasChanged = hasChanged || !isAlreadyRoot
 
 		if !isAlreadyRoot && scriptInfo != nil {
 			p.addRoot(scriptInfo)
@@ -610,6 +618,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
 	}
 
 	if p.rootFileNames.Size() > len(rootFileNames) {
+		hasChanged = true
 		for root := range p.rootFileNames.Keys() {
 			if _, ok := newRootScriptInfos[root]; !ok {
 				if info := p.host.GetScriptInfoByPath(root); info != nil {
@@ -620,6 +629,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
 			}
 		}
 	}
+	return hasChanged
 }
 
 func (p *Project) clearSourceMapperCache() {

internal/project/scriptinfo.go

@@ -110,7 +110,7 @@ func (s *ScriptInfo) setText(newText string) {
 
 func (s *ScriptInfo) markContainingProjectsAsDirty() {
 	for _, project := range s.containingProjects {
-		project.markFileAsDirty(s.path)
+		project.MarkFileAsDirty(s.path)
 	}
 }
 
@@ -122,7 +122,6 @@ func (s *ScriptInfo) attachToProject(project *Project) bool {
 		if project.compilerOptions.PreserveSymlinks != core.TSTrue {
 			s.ensureRealpath(project.FS())
 		}
-		project.onFileAddedOrRemoved(s.isSymlink())
 		return true
 	}
 	return false
@@ -132,11 +131,6 @@ func (s *ScriptInfo) isAttached(project *Project) bool {
 	return slices.Contains(s.containingProjects, project)
 }
 
-func (s *ScriptInfo) isSymlink() bool {
-	// !!!
-	return false
-}
-
 func (s *ScriptInfo) isOrphan() bool {
 	if s.deferredDelete {
 		return true
@@ -181,15 +175,13 @@ func (s *ScriptInfo) detachAllProjects() {
 		// if (isConfiguredProject(p)) {
 		// 	p.getCachedDirectoryStructureHost().addOrDeleteFile(this.fileName, this.path, FileWatcherEventKind.Deleted);
 		// }
-		project.removeFile(s, false /*fileExists*/, false /*detachFromProject*/)
-		project.onFileAddedOrRemoved(s.isSymlink())
+		project.RemoveFile(s, false /*fileExists*/, false /*detachFromProject*/)
 	}
 	s.containingProjects = nil
 }
 
 func (s *ScriptInfo) detachFromProject(project *Project) {
 	if index := slices.Index(s.containingProjects, project); index != -1 {
-		s.containingProjects[index].onFileAddedOrRemoved(s.isSymlink())
 		s.containingProjects = slices.Delete(s.containingProjects, index, index+1)
 	}
 }

internal/project/service.go

@@ -211,7 +211,7 @@ func (s *Service) CloseFile(fileName string) {
 		info.close(fileExists)
 		for _, project := range info.containingProjects {
 			if project.kind == KindInferred && project.isRoot(info) {
-				project.removeFile(info, fileExists, true /*detachFromProject*/)
+				project.RemoveFile(info, fileExists, true /*detachFromProject*/)
 			}
 		}
 		delete(s.openFiles, info.path)
@@ -309,17 +309,17 @@ func (s *Service) onConfigFileChanged(project *Project, changeKind lsproto.FileC
 		project.pendingReload = PendingReloadFull
 		project.markAsDirty()
 	}
-	project.updateIfDirty()
+	project.updateGraph()
 	return nil
 }
 
 func (s *Service) ensureProjectStructureUpToDate() {
 	var hasChanges bool
 	for _, project := range s.configuredProjects {
-		hasChanges = project.updateIfDirty() || hasChanges
+		hasChanges = project.updateGraph() || hasChanges
 	}
 	for _, project := range s.inferredProjects {
-		hasChanges = project.updateIfDirty() || hasChanges
+		hasChanges = project.updateGraph() || hasChanges
 	}
 	if hasChanges {
 		s.ensureProjectForOpenFiles()
@@ -342,7 +342,7 @@ func (s *Service) ensureProjectForOpenFiles() {
 		}
 	}
 	for _, project := range s.inferredProjects {
-		project.updateIfDirty()
+		project.updateGraph()
 	}
 
 	s.Log("After ensureProjectForOpenFiles:")
@@ -570,7 +570,7 @@ func (s *Service) assignProjectToOpenedScriptInfo(info *ScriptInfo) assignProjec
 		// result.configFileErrors = project.getAllProjectErrors()
 	}
 	for _, project := range info.containingProjects {
-		project.updateIfDirty()
+		project.updateGraph()
 	}
 	if info.isOrphan() {
 		// !!!
@@ -598,7 +598,7 @@ func (s *Service) assignOrphanScriptInfoToInferredProject(info *ScriptInfo, proj
 		project = s.getOrCreateUnrootedInferredProject()
 	}
 
-	project.addRoot(info)
+	project.AddRoot(info)
 	project.updateGraph()
 	// !!! old code ensures that scriptInfo is only part of one project
 }