Merge pull request #1135 from ahoppen/ahoppen/fix-leaks

Fix memory leaks

Author: ahoppen

Sources/SKCore/BuildSystem.swift

@@ -52,8 +52,10 @@ public protocol BuildSystem: AnyObject {
   /// Path remappings for remapping index data for local use.
   var indexPrefixMappings: [PathPrefixMapping] { get async }
 
-  /// Delegate to handle any build system events such as file build settings
-  /// initial reports as well as changes.
+  /// 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.

Sources/SKCore/BuildSystemDelegate.swift

@@ -12,7 +12,7 @@
 import BuildServerProtocol
 import LanguageServerProtocol
 
-/// Handles  build system events, such as file build settings changes.
+/// Handles build system events, such as file build settings changes.
 public protocol BuildSystemDelegate: AnyObject {
   /// Notify the delegate that the build targets have changed.
   ///

Sources/SKCore/BuildSystemManager.swift

@@ -238,7 +238,7 @@ extension BuildSystemManager: BuildSystemDelegate {
   }
 }
 
-extension BuildSystemManager: MainFilesDelegate {
+extension BuildSystemManager {
   // FIXME: Consider debouncing/limiting this, seems to trigger often during a build.
   /// Checks if there are any files in `mainFileAssociations` where the main file
   /// that we have stored has changed.

Sources/SKCore/MainFilesProvider.swift

@@ -25,10 +25,3 @@ public protocol MainFilesProvider: AnyObject {
   /// ```
   func mainFilesContainingFile(_: DocumentURI) async -> Set<DocumentURI>
 }
-
-/// Delegate that responds to possible main file changes.
-public protocol MainFilesDelegate: AnyObject {
-
-  /// The mapping from files to main files (may have) changed.
-  func mainFilesChanged() async
-}

Sources/SourceKitLSP/SourceKitIndexDelegate.swift

@@ -23,7 +23,7 @@ public actor SourceKitIndexDelegate: IndexDelegate {
   let queue = AsyncQueue<Serial>()
 
   /// Registered `MainFilesDelegate`s to notify when main files change.
-  var mainFilesDelegates: [MainFilesDelegate] = []
+  var mainFilesChangedCallbacks: [() async -> Void] = []
 
   /// The count of pending unit events. Whenever this transitions to 0, it represents a time where
   /// the index finished processing known events. Of course, that may have already changed by the
@@ -62,20 +62,16 @@ public actor SourceKitIndexDelegate: IndexDelegate {
   }
 
   func _indexChanged() {
-    for delegate in mainFilesDelegates {
+    for callback in mainFilesChangedCallbacks {
       queue.async {
-        await delegate.mainFilesChanged()
+        await callback()
       }
     }
   }
 
   /// Register a delegate to receive notifications when main files change.
-  public func registerMainFileChanged(_ delegate: MainFilesDelegate) {
-    mainFilesDelegates.append(delegate)
+  public func addMainFileChangedCallback(_ callback: @escaping () async -> Void) {
+    mainFilesChangedCallbacks.append(callback)
   }
 
-  /// Un-register a delegate to receive notifications when main files change.
-  public func unregisterMainFileChanged(_ delegate: MainFilesDelegate) {
-    mainFilesDelegates.removeAll(where: { $0 === delegate })
-  }
 }

Sources/SourceKitLSP/SourceKitServer.swift

@@ -1129,7 +1129,8 @@ extension SourceKitServer {
       buildSetup: self.options.buildSetup.merging(workspaceBuildSetup),
       compilationDatabaseSearchPaths: self.options.compilationDatabaseSearchPaths,
       indexOptions: self.options.indexOptions,
-      reloadPackageStatusCallback: { status in
+      reloadPackageStatusCallback: { [weak self] status in
+        guard let self else { return }
         guard capabilityRegistry.clientCapabilities.window?.workDoneProgress ?? false else {
           // Client doesn’t support work done progress
           return

Sources/SourceKitLSP/Workspace.swift

@@ -88,7 +88,9 @@ public final class Workspace {
       fallbackBuildSystem: FallbackBuildSystem(buildSetup: buildSetup),
       mainFilesProvider: index
     )
-    await indexDelegate?.registerMainFileChanged(buildSystemManager)
+    await indexDelegate?.addMainFileChangedCallback { [weak self] in
+      await self?.buildSystemManager.mainFilesChanged()
+    }
   }
 
   /// Creates a workspace for a given root `URL`, inferring the `ExternalWorkspace` if possible.

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -429,7 +429,7 @@ class ManualBuildSystem: BuildSystem {
 
   var map: [DocumentURI: FileBuildSettings] = [:]
 
-  var delegate: BuildSystemDelegate? = nil
+  weak var delegate: BuildSystemDelegate? = nil
 
   func setDelegate(_ delegate: SKCore.BuildSystemDelegate?) async {
     self.delegate = delegate