Change the build system to only notify delegate about changed files, not about new build settings

This defines away an entire class of data races if delegate callbacks are delivered out-of-order. If we aren’t providing the new build settings in the delegate callback, then it doesn’t matter if two `fileBuildSettingsChanged` calls change order since they don’t carry any state.

Author: ahoppen

Sources/SKCore/BuildServerBuildSystem.swift

@@ -202,11 +202,7 @@ public actor BuildServerBuildSystem: MessageHandler {
   /// about the changed build settings.
   private func buildSettingsChanged(for document: DocumentURI, settings: FileBuildSettings?) async {
     buildSettings[document] = settings
-    if let settings {
-      await self.delegate?.fileBuildSettingsChanged([document: .modified(settings)])
-    } else {
-      await self.delegate?.fileBuildSettingsChanged([document: .removedOrUnavailable])
-    }
+    await self.delegate?.fileBuildSettingsChanged([document])
   }
 }
 

Sources/SKCore/BuildSystemDelegate.swift

@@ -21,11 +21,7 @@ public protocol BuildSystemDelegate: AnyObject {
   func buildTargetsChanged(_ changes: [BuildTargetEvent]) async
 
   /// Notify the delegate that the given files' build settings have changed.
-  ///
-  /// The delegate should cache the new build settings for any of the given
-  /// files that they are interested in.
-  func fileBuildSettingsChanged(
-    _ changedFiles: [DocumentURI: FileBuildSettingsChange]) async
+  func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async
 
   /// Notify the delegate that the dependencies of the given files have changed
   /// and that ASTs may need to be refreshed. If the given set is empty, assume

Sources/SKCore/BuildSystemManager.swift

@@ -203,10 +203,8 @@ extension BuildSystemManager {
 
     let newStatus = await self.cachedStatusOrRegisterForSettings(for: mainFile, language: language)
 
-    if let mainChange = newStatus.buildSettingsChange,
-       let delegate = self._delegate {
-      let change = self.convert(change: mainChange, ofMainFile: mainFile, to: uri)
-      await delegate.fileBuildSettingsChanged([uri: change])
+    if newStatus.buildSettingsChange != nil, let delegate = self._delegate {
+      await delegate.fileBuildSettingsChanged([uri])
     }
   }
 
@@ -287,7 +285,7 @@ extension BuildSystemManager {
   /// Update and notify our delegate for the given main file changes if they are
   /// convertible into `FileBuildSettingsChange`.
   func updateAndNotifyStatuses(changes: [DocumentURI: MainFileStatus]) async {
-    var changedWatchedFiles = [DocumentURI: FileBuildSettingsChange]()
+    var changedWatchedFiles = Set<DocumentURI>()
     for (mainFile, status) in changes {
       let watches = self.watchedFiles.filter { $1.mainFile == mainFile }
       guard !watches.isEmpty else {
@@ -303,12 +301,11 @@ extension BuildSystemManager {
       guard prevStatus == .waiting || status.buildSettings != prevStatus?.buildSettings else {
         continue
       }
-      if let change = status.buildSettingsChange {
-        for watch in watches {
-          let newChange =
-            self.convert(change: change, ofMainFile: mainFile, to: watch.key)
-          changedWatchedFiles[watch.key] = newChange
-        }
+      guard status.buildSettingsChange != nil else {
+        continue
+      }
+      for watch in watches {
+        changedWatchedFiles.insert(watch.key)
       }
     }
 
@@ -367,26 +364,26 @@ extension BuildSystemManager {
 
 extension BuildSystemManager: BuildSystemDelegate {
   // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has ben asyncified
-  public nonisolated func fileBuildSettingsChanged(_ changes: [DocumentURI: FileBuildSettingsChange]) {
+  public nonisolated func fileBuildSettingsChanged(_ changes: Set<DocumentURI>) {
     Task {
       await fileBuildSettingsChangedImpl(changes)
     }
   }
 
-  public func fileBuildSettingsChangedImpl(_ changes: [DocumentURI: FileBuildSettingsChange]) async {
-    let statusChanges: [DocumentURI: MainFileStatus] =
-        changes.reduce(into: [:]) { (result, entry) in
-      let mainFile = entry.key
-      let settingsChange = entry.value
+  public func fileBuildSettingsChangedImpl(_ changes: Set<DocumentURI>) async {
+    var statusChanges: [DocumentURI: MainFileStatus] = [:]
+    for mainFile in changes {
       let watches = self.watchedFiles.filter { $1.mainFile == mainFile }
       guard let firstWatch = watches.first else {
         // Possible notification after the file was unregistered. Ignore.
-        return
+        continue
       }
       let newStatus: MainFileStatus
 
-      if let newSettings = settingsChange.newSettings {
-        newStatus = settingsChange.isFallback ? .fallback(newSettings) : .primary(newSettings)
+      let settings = await self.buildSettings(for: mainFile, language: firstWatch.value.language)
+
+      if let settings {
+        newStatus = settings.isFallback ? .fallback(settings.buildSettings) : .primary(settings.buildSettings)
       } else if let fallback = self.fallbackBuildSystem {
         // FIXME: we need to stop threading the language everywhere, or we need the build system
         // itself to pass it in here. Or alternatively cache the fallback settings/language earlier?
@@ -399,7 +396,7 @@ extension BuildSystemManager: BuildSystemDelegate {
       } else {
         newStatus = .unsupported
       }
-      result[mainFile] = newStatus
+      statusChanges[mainFile] = newStatus
     }
     await self.updateAndNotifyStatuses(changes: statusChanges)
   }
@@ -467,7 +464,7 @@ extension BuildSystemManager: MainFilesDelegate {
   public func mainFilesChangedImpl() async {
     let origWatched = self.watchedFiles
     self.watchedFiles = [:]
-    var buildSettingsChanges = [DocumentURI: FileBuildSettingsChange]()
+    var buildSettingsChanges = Set<DocumentURI>()
 
     for (uri, state) in origWatched {
       let mainFiles = self._mainFilesProvider?.mainFilesContainingFile(uri) ?? []
@@ -482,9 +479,8 @@ extension BuildSystemManager: MainFilesDelegate {
 
         let newStatus = await self.cachedStatusOrRegisterForSettings(
             for: newMainFile, language: language)
-        if let change = newStatus.buildSettingsChange {
-          let newChange = self.convert(change: change, ofMainFile: newMainFile, to: uri)
-          buildSettingsChanges[uri] = newChange
+        if newStatus.buildSettingsChange != nil {
+          buildSettingsChanges.insert(uri)
         }
       }
     }

Sources/SKCore/CompilationDatabaseBuildSystem.swift

@@ -96,8 +96,7 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
 
     guard let delegate = self.delegate else { return }
 
-    let settings = self.settings(for: uri)
-    await delegate.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
+    await delegate.fileBuildSettingsChanged([uri])
   }
 
   /// We don't support change watching.
@@ -148,13 +147,9 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     self.compdb = tryLoadCompilationDatabase(directory: projectRoot, self.fileSystem)
 
     if let delegate = self.delegate {
-      var changedFiles: [DocumentURI: FileBuildSettingsChange] = [:]
+      var changedFiles = Set<DocumentURI>()
       for (uri, _) in self.watchedFiles {
-        if let settings = self.settings(for: uri) {
-          changedFiles[uri] = FileBuildSettingsChange(settings)
-        } else {
-          changedFiles[uri] = .removedOrUnavailable
-        }
+        changedFiles.insert(uri)
       }
       await delegate.fileBuildSettingsChanged(changedFiles)
     }

Sources/SKCore/FallbackBuildSystem.swift

@@ -62,9 +62,8 @@ public final class FallbackBuildSystem: BuildSystem {
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) {
     guard let delegate = self.delegate else { return }
 
-    let settings = self.buildSettings(for: uri, language: language)
     Task {
-      await delegate.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
+      await delegate.fileBuildSettingsChanged([uri])
     }
   }
 

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

@@ -242,15 +242,9 @@ extension SwiftPMWorkspace {
     })
 
     guard let delegate = self.delegate else { return }
-    var changedFiles: [DocumentURI: FileBuildSettingsChange] = [:]
-    for (uri, language) in self.watchedFiles {
-      orLog {
-        if let settings = try self.buildSettings(for: uri, language: language) {
-          changedFiles[uri] = FileBuildSettingsChange(settings)
-        } else {
-          changedFiles[uri] = .removedOrUnavailable
-        }
-      }
+    var changedFiles = Set<DocumentURI>()
+    for (uri, _) in self.watchedFiles {
+      changedFiles.insert(uri)
     }
     await delegate.fileBuildSettingsChanged(changedFiles)
     await delegate.fileHandlingCapabilityChanged()
@@ -316,11 +310,7 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
     } catch {
       log("error computing settings: \(error)")
     }
-    if let settings = settings {
-      await delegate.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
-    } else {
-      await delegate.fileBuildSettingsChanged([uri: .removedOrUnavailable])
-    }
+    await delegate.fileBuildSettingsChanged([uri])
   }
 
   /// Unregister the given file for build-system level change notifications, such as command

Sources/SourceKitLSP/Clang/ClangLanguageServer.swift

@@ -404,7 +404,7 @@ extension ClangLanguageServerShim {
     // Send clangd the build settings for the new file. We need to do this before
     // sending the open notification, so that the initial diagnostics already
     // have build settings.
-    await documentUpdatedBuildSettings(note.textDocument.uri, change: .removedOrUnavailable)
+    await documentUpdatedBuildSettings(note.textDocument.uri)
     clangd.send(note)
   }
 
@@ -427,7 +427,7 @@ extension ClangLanguageServerShim {
 
   // MARK: - Build System Integration
 
-  public func documentUpdatedBuildSettings(_ uri: DocumentURI, change: FileBuildSettingsChange) async {
+  public func documentUpdatedBuildSettings(_ uri: DocumentURI) async {
     guard let url = uri.fileURL else {
       // FIXME: The clang workspace can probably be reworked to support non-file URIs.
       log("Received updated build settings for non-file URI '\(uri)'. Ignoring the update.")

Sources/SourceKitLSP/SourceKitServer.swift

@@ -666,7 +666,7 @@ extension SourceKitServer: BuildSystemDelegate {
 
   // FIXME: (async) Make this method isolated once `BuildSystemDelegate` has been asyncified
   /// Non-async variant that executes `fileBuildSettingsChangedImpl` in a new task.
-  public nonisolated func fileBuildSettingsChanged(_ changedFiles: [DocumentURI: FileBuildSettingsChange]) {
+  public nonisolated func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) {
     Task {
       await self.fileBuildSettingsChangedImpl(changedFiles)
     }
@@ -676,10 +676,8 @@ extension SourceKitServer: BuildSystemDelegate {
   /// This has two primary cases:
   /// - Initial settings reported for a given file, now we can fully open it
   /// - Changed settings for an already open file
-  public func fileBuildSettingsChangedImpl(
-    _ changedFiles: [DocumentURI: FileBuildSettingsChange]
-  ) async {
-    for (uri, change) in changedFiles {
+  public func fileBuildSettingsChangedImpl(_ changedFiles: Set<DocumentURI>) async {
+    for uri in changedFiles {
       // Non-ready documents should be considered open even though we haven't
       // opened it with the language service yet.
       guard self.documentManager.openDocuments.contains(uri) else { continue }
@@ -704,7 +702,7 @@ extension SourceKitServer: BuildSystemDelegate {
         }
 
         // Notify the language server so it can apply the proper arguments.
-        await service.documentUpdatedBuildSettings(uri, change: change)
+        await service.documentUpdatedBuildSettings(uri)
 
         // Catch up on any queued notifications and requests.
         while !(documentToPendingQueue[uri]?.queue.isEmpty ?? true) {
@@ -725,7 +723,7 @@ extension SourceKitServer: BuildSystemDelegate {
       // Case 2: changed settings for an already open file.
       log("Build settings changed for opened file \(uri)")
       if let service = workspace.documentService[uri] {
-        await service.documentUpdatedBuildSettings(uri, change: change)
+        await service.documentUpdatedBuildSettings(uri)
       }
     }
   }

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

@@ -488,7 +488,7 @@ extension SwiftLanguageServer {
     self.updateSyntacticTokens(for: snapshot)
   }
 
-  public func documentUpdatedBuildSettings(_ uri: DocumentURI, change: FileBuildSettingsChange) async {
+  public func documentUpdatedBuildSettings(_ uri: DocumentURI) async {
     // We may not have a snapshot if this is called just before `openDocument`.
     guard let snapshot = self.documentManager.latestSnapshot(uri) else {
       return

Sources/SourceKitLSP/ToolchainLanguageServer.swift

@@ -72,7 +72,7 @@ public protocol ToolchainLanguageServer: AnyObject {
   /// Sent when the `BuildSystem` has resolved build settings, such as for the intial build settings
   /// or when the settings have changed (e.g. modified build system files). This may be sent before
   /// the respective `DocumentURI` has been opened.
-  func documentUpdatedBuildSettings(_ uri: DocumentURI, change: FileBuildSettingsChange) async
+  func documentUpdatedBuildSettings(_ uri: DocumentURI) async
 
   /// Sent when the `BuildSystem` has detected that dependencies of the given file have changed
   /// (e.g. header files, swiftmodule files, other compiler input files).

Tests/SKCoreTests/BuildServerBuildSystemTests.swift

@@ -98,9 +98,8 @@ final class TestDelegate: BuildSystemDelegate {
     }
   }
 
-  func fileBuildSettingsChanged(
-    _ changedFiles: [DocumentURI: FileBuildSettingsChange]) {
-    for (uri, _) in changedFiles {
+  func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) {
+    for uri in changedFiles {
       settingsExpectations[uri]?.fulfill()
     }
   }

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -110,7 +110,7 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[a] = nil
     let changed = expectation(description: "changed settings")
     await del.setExpected([(a, .swift, nil, changed, #file, #line)])
-    bsm.fileBuildSettingsChanged([a: .removedOrUnavailable])
+    bsm.fileBuildSettingsChanged([a])
     try await fulfillmentOfOrThrow([changed])
   }
 
@@ -133,7 +133,7 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
     let changed = expectation(description: "changed settings")
     await del.setExpected([(a, .swift, bs.map[a]!, changed, #file, #line)])
-    bsm.fileBuildSettingsChanged([a: .modified(bs.map[a]!)])
+    bsm.fileBuildSettingsChanged([a])
     try await fulfillmentOfOrThrow([changed])
   }
 
@@ -158,13 +158,13 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[a] = FileBuildSettings(compilerArguments: ["non-fallback", "args"])
     let changed = expectation(description: "changed settings")
     await del.setExpected([(a, .swift, bs.map[a]!, changed, #file, #line)])
-    bsm.fileBuildSettingsChanged([a: .modified(bs.map[a]!)])
+    bsm.fileBuildSettingsChanged([a])
     try await fulfillmentOfOrThrow([changed])
 
     bs.map[a] = nil
     let revert = expectation(description: "revert to fallback settings")
     await del.setExpected([(a, .swift, fallbackSettings, revert, #file, #line)])
-    bsm.fileBuildSettingsChanged([a: .removedOrUnavailable])
+    bsm.fileBuildSettingsChanged([a])
     try await fulfillmentOfOrThrow([revert])
   }
 
@@ -196,7 +196,7 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[b] = FileBuildSettings(compilerArguments: ["yy"])
     let changed = expectation(description: "changed settings")
     await del.setExpected([(a, .swift, bs.map[a]!, changed, #file, #line)])
-    bsm.fileBuildSettingsChanged([a: .modified(bs.map[a]!)])
+    bsm.fileBuildSettingsChanged([a])
     try await fulfillmentOfOrThrow([changed])
 
     // Test multiple changes.
@@ -208,10 +208,7 @@ final class BuildSystemManagerTests: XCTestCase {
       (a, .swift, bs.map[a]!, changedBothA, #file, #line),
       (b, .swift, bs.map[b]!, changedBothB, #file, #line),
     ])
-    bsm.fileBuildSettingsChanged([
-      a:. modified(bs.map[a]!),
-      b: .modified(bs.map[b]!)
-    ])
+    bsm.fileBuildSettingsChanged([a, b])
     try await fulfillmentOfOrThrow([changedBothA, changedBothB])
   }
 
@@ -245,9 +242,7 @@ final class BuildSystemManagerTests: XCTestCase {
     bs.map[b] = nil
     let changed = expectation(description: "changed settings")
     await del.setExpected([(b, .swift, nil, changed, #file, #line)])
-    bsm.fileBuildSettingsChanged([
-      b: .removedOrUnavailable
-    ])
+    bsm.fileBuildSettingsChanged([b])
     try await fulfillmentOfOrThrow([changed])
   }
 
@@ -354,7 +349,7 @@ final class BuildSystemManagerTests: XCTestCase {
       (h1, .c, newArgsH1, changed1, #file, #line),
       (h2, .c, newArgsH2, changed2, #file, #line),
     ])
-    bsm.fileBuildSettingsChanged([cpp: .modified(bs.map[cpp]!)])
+    bsm.fileBuildSettingsChanged([cpp])
 
     try await fulfillmentOfOrThrow([changed1, changed2])
   }
@@ -403,11 +398,7 @@ final class BuildSystemManagerTests: XCTestCase {
     await bsm.unregisterForChangeNotifications(for: c)
     // At this point only b is registered, but that can race with notifications,
     // so ensure nothing bad happens and we still get the notification for b.
-    bsm.fileBuildSettingsChanged([
-      a: .modified(bs.map[a]!),
-      b: .modified(bs.map[b]!),
-      c: .modified(bs.map[c]!)
-    ])
+    bsm.fileBuildSettingsChanged([a, b, c])
 
     try await fulfillmentOfOrThrow([changedB])
   }
@@ -484,8 +475,7 @@ class ManualBuildSystem: BuildSystem {
   }
 
   func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
-    let settings = self.buildSettings(for: uri, language: language)
-    await self.delegate?.fileBuildSettingsChanged([uri: FileBuildSettingsChange(settings)])
+    await self.delegate?.fileBuildSettingsChanged([uri])
   }
 
   func unregisterForChangeNotifications(for: DocumentURI) {
@@ -536,8 +526,8 @@ private actor BSMDelegate: BuildSystemDelegate {
     }()
   }
 
-  func fileBuildSettingsChanged(_ changes: [DocumentURI: FileBuildSettingsChange]) async {
-    for (uri, _) in changes {
+  func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async {
+    for uri in changedFiles {
       guard let expected = expected.first(where: { $0.uri == uri }) else {
         XCTFail("unexpected settings change for \(uri)")
         continue