Handle filesDependenciesUpdated in BuildSystemManager instead of the build system

The implementation of which file’s dependencies have been updated is common across all build systems and thus build systems shouldn’t need to implement this logic. This also allows us to remove `BuildSystemDelegate`.

Author: ahoppen

Sources/BuildSystemIntegration/BuildServerBuildSystem.swift

@@ -67,14 +67,6 @@ package actor BuildServerBuildSystem: MessageHandler {
   package private(set) var indexDatabasePath: AbsolutePath?
   package private(set) var indexStorePath: AbsolutePath?
 
-  /// Delegate to handle any build system events.
-  package weak var delegate: BuildSystemDelegate?
-
-  /// - Note: Needed to set the delegate from a different actor isolation context
-  package func setDelegate(_ delegate: BuildSystemDelegate?) async {
-    self.delegate = delegate
-  }
-
   package weak var messageHandler: BuiltInBuildSystemMessageHandler?
 
   /// The build settings that have been received from the build server.

Sources/BuildSystemIntegration/BuildSystemDelegate.swift

@@ -9,21 +9,10 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import BuildServerProtocol
 import LanguageServerProtocol
 
-/// Handles build system events, such as file build settings changes.
-// FIXME: (BSP migration) The build system should exclusively communicate back to SourceKit-LSP using BSP and this protocol should be deleted.
-package protocol BuildSystemDelegate: AnyObject, Sendable {
-  /// 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
-  /// that all watched files are affected.
-  ///
-  /// The callee should refresh ASTs unless it is able to determine that a
-  /// refresh is not necessary.
-  func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async
-}
-
 /// Handles build system events, such as file build settings changes.
 package protocol BuildSystemManagerDelegate: AnyObject, Sendable {
   /// Notify the delegate that the given files' build settings have changed.

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -95,6 +95,18 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   /// Used to determine which toolchain to use for a given document.
   private let toolchainRegistry: ToolchainRegistry
 
+  private let options: SourceKitLSPOptions
+
+  /// Debounces calls to `delegate.filesDependenciesUpdated`.
+  ///
+  /// This is to ensure we don't call `filesDependenciesUpdated` for the same file multiple time if the client does not
+  /// debounce `workspace/didChangeWatchedFiles` and sends a separate notification eg. for every file within a target as
+  /// it's being updated by a git checkout, which would cause other files within that target to receive a
+  /// `fileDependenciesUpdated` call once for every updated file within the target.
+  ///
+  /// Force-unwrapped optional because initializing it requires access to `self`.
+  private var filesDependenciesUpdatedDebouncer: Debouncer<Set<DocumentURI>>! = nil
+
   private var cachedTargetsForDocument = RequestCache<InverseSourcesRequest>()
 
   private var cachedSourceKitOptions = RequestCache<SourceKitOptionsRequest>()
@@ -123,6 +135,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   ) async {
     self.fallbackBuildSystem = FallbackBuildSystem(options: options.fallbackBuildSystemOrDefault)
     self.toolchainRegistry = toolchainRegistry
+    self.options = options
     self.projectRoot = buildSystemKind?.projectRoot
     self.buildSystem = await BuiltInBuildSystemAdapter(
       buildSystemKind: buildSystemKind,
@@ -132,16 +145,68 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
       reloadPackageStatusCallback: reloadPackageStatusCallback,
       messageHandler: self
     )
-    await self.buildSystem?.underlyingBuildSystem.setDelegate(self)
+    // The debounce duration of 500ms was chosen arbitrarily without any measurements.
+    self.filesDependenciesUpdatedDebouncer = Debouncer(
+      debounceDuration: .milliseconds(500),
+      combineResults: { $0.union($1) }
+    ) {
+      [weak self] (filesWithUpdatedDependencies) in
+      guard let self, let delegate = await self.delegate else {
+        logger.fault("Not calling filesDependenciesUpdated because no delegate exists in SwiftPMBuildSystem")
+        return
+      }
+      let changedWatchedFiles = await self.watchedFilesReferencing(mainFiles: filesWithUpdatedDependencies)
+      guard !changedWatchedFiles.isEmpty else {
+        return
+      }
+      await delegate.filesDependenciesUpdated(changedWatchedFiles)
+    }
   }
 
   package func filesDidChange(_ events: [FileEvent]) async {
     await self.buildSystem?.send(BuildServerProtocol.DidChangeWatchedFilesNotification(changes: events))
+
+    var targetsWithUpdatedDependencies: Set<BuildTargetIdentifier> = []
+    // If a Swift file within a target is updated, reload all the other files within the target since they might be
+    // referring to a function in the updated file.
+    let targetsWithChangedSwiftFiles =
+      await events
+      .filter { $0.uri.fileURL?.pathExtension == "swift" }
+      .asyncFlatMap { await self.targets(for: $0.uri) }
+    targetsWithUpdatedDependencies.formUnion(targetsWithChangedSwiftFiles)
+
+    // If a `.swiftmodule` file is updated, this means that we have performed a build / are
+    // performing a build and files that depend on this module have updated dependencies.
+    // We don't have access to the build graph from the SwiftPM API offered to SourceKit-LSP to figure out which files
+    // depend on the updated module, so assume that all files have updated dependencies.
+    // The file watching here is somewhat fragile as well because it assumes that the `.swiftmodule` files are being
+    // written to a directory within the workspace root. This is not necessarily true if the user specifies a build
+    // directory outside the source tree.
+    // If we have background indexing enabled, this is not necessary because we call `fileDependenciesUpdated` when
+    // preparation of a target finishes.
+    if !options.backgroundIndexingOrDefault,
+      events.contains(where: { $0.uri.fileURL?.pathExtension == "swiftmodule" })
+    {
+      let targets = await orLog("Getting build targets") {
+        try await self.buildTargets()
+      }
+      targetsWithUpdatedDependencies.formUnion(targets?.map(\.id) ?? [])
+    }
+
+    var filesWithUpdatedDependencies: Set<DocumentURI> = []
+
+    await orLog("Getting source files in targets") {
+      let sourceFiles = try await self.sourceFiles(in: Array(Set(targetsWithUpdatedDependencies)))
+      filesWithUpdatedDependencies.formUnion(sourceFiles.flatMap(\.sources).map(\.uri))
+    }
+
     if let mainFilesProvider {
       var mainFiles = await Set(events.asyncFlatMap { await mainFilesProvider.mainFilesContainingFile($0.uri) })
       mainFiles.subtract(events.map(\.uri))
-      await self.filesDependenciesUpdated(mainFiles)
+      filesWithUpdatedDependencies.formUnion(mainFiles)
     }
+
+    await self.filesDependenciesUpdatedDebouncer.scheduleCall(filesWithUpdatedDependencies)
   }
 
   /// Implementation of `MessageHandler`, handling notifications from the build system.
@@ -404,6 +469,10 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     logMessageToIndexLog: @escaping @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void
   ) async throws {
     let _: VoidResponse? = try await buildSystem?.send(PrepareTargetsRequest(targets: targets))
+    await orLog("Calling fileDependenciesUpdated") {
+      let filesInPreparedTargets = try await self.sourceFiles(in: targets).flatMap(\.sources).map(\.uri)
+      await filesDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets))
+    }
   }
 
   package func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
@@ -471,7 +540,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   }
 }
 
-extension BuildSystemManager: BuildSystemDelegate {
+extension BuildSystemManager {
   private func watchedFilesReferencing(mainFiles: Set<DocumentURI>) -> Set<DocumentURI> {
     return Set(
       watchedFiles.compactMap { (watchedFile, mainFileAndLanguage) in
@@ -484,22 +553,6 @@ extension BuildSystemManager: BuildSystemDelegate {
     )
   }
 
-  package func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async {
-    // Empty changes --> assume everything has changed.
-    guard !changedFiles.isEmpty else {
-      if let delegate = self.delegate {
-        await delegate.filesDependenciesUpdated(changedFiles)
-      }
-      return
-    }
-
-    // Need to map the changed main files back into changed watch files.
-    let changedWatchedFiles = watchedFilesReferencing(mainFiles: changedFiles)
-    if let delegate, !changedWatchedFiles.isEmpty {
-      await delegate.filesDependenciesUpdated(changedWatchedFiles)
-    }
-  }
-
   private func didChangeBuildTarget(notification: DidChangeBuildTargetNotification) async {
     // Every `DidChangeBuildTargetNotification` notification needs to invalidate the cache since the changed target
     // might gained a source file.

Sources/BuildSystemIntegration/BuiltInBuildSystem.swift

@@ -75,18 +75,6 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable {
   /// The path to put the index database, if any.
   var indexDatabasePath: AbsolutePath? { get async }
 
-  /// Delegate to handle any build system events such as file build settings initial reports as well as changes.
-  ///
-  /// The build system must not retain the delegate because the delegate can be the `BuildSystemManager`, which could
-  /// result in a retain cycle `BuildSystemManager` -> `BuildSystem` -> `BuildSystemManager`.
-  var delegate: BuildSystemDelegate? { get async }
-
-  /// Set the build system's delegate.
-  ///
-  /// - Note: Needed so we can set the delegate from a different actor isolation
-  ///   context.
-  func setDelegate(_ delegate: BuildSystemDelegate?) async
-
   /// Whether the build system is capable of preparing a target for indexing, ie. if the `prepare` methods has been
   /// implemented.
   var supportsPreparation: Bool { get }

Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift

@@ -38,16 +38,9 @@ package actor CompilationDatabaseBuildSystem {
     }
   }
 
-  /// Delegate to handle any build system events.
-  package weak var delegate: BuildSystemDelegate? = nil
-
   /// Callbacks that should be called if the list of possible test files has changed.
   package var testFilesDidChangeCallbacks: [() async -> Void] = []
 
-  package func setDelegate(_ delegate: BuildSystemDelegate?) async {
-    self.delegate = delegate
-  }
-
   package weak var messageHandler: BuiltInBuildSystemMessageHandler?
 
   package let projectRoot: AbsolutePath

Sources/BuildSystemIntegration/FallbackBuildSystem.swift

@@ -38,13 +38,6 @@ package actor FallbackBuildSystem {
     )
   }()
 
-  /// Delegate to handle any build system events.
-  package weak var delegate: BuildSystemDelegate? = nil
-
-  package func setDelegate(_ delegate: BuildSystemDelegate?) async {
-    self.delegate = delegate
-  }
-
   package var indexStorePath: AbsolutePath? { return nil }
 
   package var indexDatabasePath: AbsolutePath? { return nil }

Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift

@@ -173,13 +173,6 @@ package actor SwiftPMBuildSystem {
   /// issues in SwiftPM.
   private let packageLoadingQueue = AsyncQueue<Serial>()
 
-  /// Delegate to handle any build system events.
-  package weak var delegate: BuildSystemIntegration.BuildSystemDelegate? = nil
-
-  package func setDelegate(_ delegate: BuildSystemIntegration.BuildSystemDelegate?) async {
-    self.delegate = delegate
-  }
-
   package weak var messageHandler: BuiltInBuildSystemMessageHandler?
 
   /// This callback is informed when `reloadPackage` starts and ends executing.
@@ -188,16 +181,6 @@ package actor SwiftPMBuildSystem {
   /// Callbacks that should be called if the list of possible test files has changed.
   private var testFilesDidChangeCallbacks: [() async -> Void] = []
 
-  /// Debounces calls to `delegate.filesDependenciesUpdated`.
-  ///
-  /// This is to ensure we don't call `filesDependenciesUpdated` for the same file multiple time if the client does not
-  /// debounce `workspace/didChangeWatchedFiles` and sends a separate notification eg. for every file within a target as
-  /// it's being updated by a git checkout, which would cause other files within that target to receive a
-  /// `fileDependenciesUpdated` call once for every updated file within the target.
-  ///
-  /// Force-unwrapped optional because initializing it requires access to `self`.
-  private var fileDependenciesUpdatedDebouncer: Debouncer<Set<DocumentURI>>! = nil
-
   /// Whether the `SwiftPMBuildSystem` is pointed at a `.index-build` directory that's independent of the
   /// user's build.
   private var isForIndexBuild: Bool { options.backgroundIndexingOrDefault }
@@ -378,19 +361,6 @@ package actor SwiftPMBuildSystem {
     )
 
     self.reloadPackageStatusCallback = reloadPackageStatusCallback
-
-    // The debounce duration of 500ms was chosen arbitrarily without scientific research.
-    self.fileDependenciesUpdatedDebouncer = Debouncer(
-      debounceDuration: .milliseconds(500),
-      combineResults: { $0.union($1) }
-    ) {
-      [weak self] (filesWithUpdatedDependencies) in
-      guard let delegate = await self?.delegate else {
-        logger.fault("Not calling filesDependenciesUpdated because no delegate exists in SwiftPMBuildSystem")
-        return
-      }
-      await delegate.filesDependenciesUpdated(filesWithUpdatedDependencies)
-    }
   }
 
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
@@ -692,8 +662,6 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
     for target in request.targets {
       await orLog("Preparing") { try await prepare(singleTarget: target) }
     }
-    let filesInPreparedTargets = request.targets.flatMap { self.targets[$0]?.buildTarget.sources ?? [] }
-    await fileDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets.map(DocumentURI.init)))
     return VoidResponse()
   }
 
@@ -846,32 +814,6 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
         try await self.schedulePackageReload().value
       }
     }
-
-    var filesWithUpdatedDependencies: Set<DocumentURI> = []
-    // If a Swift file within a target is updated, reload all the other files within the target since they might be
-    // referring to a function in the updated file.
-    for event in notification.changes {
-      guard event.uri.fileURL?.pathExtension == "swift", let targets = fileToTargets[event.uri] else {
-        continue
-      }
-      for target in targets {
-        filesWithUpdatedDependencies.formUnion(self.targets[target]?.buildTarget.sources.map(DocumentURI.init) ?? [])
-      }
-    }
-
-    // If a `.swiftmodule` file is updated, this means that we have performed a build / are
-    // performing a build and files that depend on this module have updated dependencies.
-    // We don't have access to the build graph from the SwiftPM API offered to SourceKit-LSP to figure out which files
-    // depend on the updated module, so assume that all files have updated dependencies.
-    // The file watching here is somewhat fragile as well because it assumes that the `.swiftmodule` files are being
-    // written to a directory within the workspace root. This is not necessarily true if the user specifies a build
-    // directory outside the source tree.
-    // If we have background indexing enabled, this is not necessary because we call `fileDependenciesUpdated` when
-    // preparation of a target finishes.
-    if !isForIndexBuild, notification.changes.contains(where: { $0.uri.fileURL?.pathExtension == "swiftmodule" }) {
-      filesWithUpdatedDependencies.formUnion(self.fileToTargets.keys)
-    }
-    await self.fileDependenciesUpdatedDebouncer.scheduleCall(filesWithUpdatedDependencies)
   }
 
   package func sourceFiles() -> [SourceFileInfo] {

Sources/BuildSystemIntegration/TestBuildSystem.swift

@@ -28,14 +28,8 @@ package actor TestBuildSystem: BuiltInBuildSystem {
   package let indexStorePath: AbsolutePath? = nil
   package let indexDatabasePath: AbsolutePath? = nil
 
-  package weak var delegate: BuildSystemDelegate?
-
   private weak var messageHandler: BuiltInBuildSystemMessageHandler?
 
-  package func setDelegate(_ delegate: BuildSystemDelegate?) async {
-    self.delegate = delegate
-  }
-
   /// Build settings by file.
   private var buildSettingsByFile: [DocumentURI: SourceKitOptionsResponse] = [:]
 

Tests/BuildSystemIntegrationTests/BuildServerBuildSystemTests.swift

@@ -76,7 +76,6 @@ final class BuildServerBuildSystemTests: XCTestCase {
       _fixLifetime(buildSystemDelegate)
     }
     let buildSystem = try await BuildServerBuildSystem(projectRoot: root, messageHandler: buildSystemDelegate)
-    await buildSystem.setDelegate(buildSystemDelegate)
     _ = try await buildSystem.sourceKitOptions(
       request: SourceKitOptionsRequest(
         textDocument: TextDocumentIdentifier(uri: uri),
@@ -126,16 +125,11 @@ final class BuildServerBuildSystemTests: XCTestCase {
   }
 }
 
-final class TestDelegate: BuildSystemDelegate, BuiltInBuildSystemMessageHandler {
+final class TestDelegate: BuiltInBuildSystemMessageHandler {
   let targetExpectations: [(DidChangeBuildTargetNotification, XCTestExpectation)]
-  let dependenciesUpdatedExpectations: [DocumentURI: XCTestExpectation]
 
-  package init(
-    targetExpectations: [(DidChangeBuildTargetNotification, XCTestExpectation)] = [],
-    dependenciesUpdatedExpectations: [DocumentURI: XCTestExpectation] = [:]
-  ) {
+  package init(targetExpectations: [(DidChangeBuildTargetNotification, XCTestExpectation)] = []) {
     self.targetExpectations = targetExpectations
-    self.dependenciesUpdatedExpectations = dependenciesUpdatedExpectations
   }
 
   func didChangeBuildTarget(notification: DidChangeBuildTargetNotification) {
@@ -146,12 +140,6 @@ final class TestDelegate: BuildSystemDelegate, BuiltInBuildSystemMessageHandler
     }
   }
 
-  package func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {
-    for uri in changedFiles {
-      dependenciesUpdatedExpectations[uri]?.fulfill()
-    }
-  }
-
   func sendRequestToSourceKitLSP<R: RequestType>(_ request: R) async throws -> R.Response {
     throw ResponseError.methodNotFound(R.method)
   }