Make SourceKitD build with strict concurrency enabled

Author: ahoppen

Package.swift

@@ -270,7 +270,8 @@ let package = Package(
         "SKSupport",
         .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
       ],
-      exclude: ["CMakeLists.txt"]
+      exclude: ["CMakeLists.txt", "sourcekitd_uids.swift.gyb"],
+      swiftSettings: [.enableExperimentalFeature("StrictConcurrency")]
     ),
 
     .testTarget(

Sources/SKSupport/AsyncQueue.swift

@@ -84,8 +84,8 @@ private class PendingTasks<TaskMetadata: Sendable>: @unchecked Sendable {
 }
 
 /// A queue that allows the execution of asynchronous blocks of code.
-public final class AsyncQueue<TaskMetadata: DependencyTracker> {
-  private var pendingTasks: PendingTasks<TaskMetadata> = PendingTasks()
+public final class AsyncQueue<TaskMetadata: DependencyTracker>: Sendable {
+  private let pendingTasks: PendingTasks<TaskMetadata> = PendingTasks()
 
   public init() {}
 

Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift

@@ -16,12 +16,17 @@ import SKSupport
 
 import struct TSCBasic.AbsolutePath
 
+extension DLHandle: @unchecked @retroactive Sendable {}
+extension sourcekitd_api_keys: @unchecked Sendable {}
+extension sourcekitd_api_requests: @unchecked Sendable {}
+extension sourcekitd_api_values: @unchecked Sendable {}
+
 /// Wrapper for sourcekitd, taking care of initialization, shutdown, and notification handler
 /// multiplexing.
 ///
 /// Users of this class should not call the api functions `initialize`, `shutdown`, or
 /// `set_notification_handler`, which are global state managed internally by this class.
-public final class DynamicallyLoadedSourceKitD: SourceKitD {
+public actor DynamicallyLoadedSourceKitD: SourceKitD {
 
   /// The path to the sourcekitd dylib.
   public let path: AbsolutePath
@@ -41,11 +46,10 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
   /// Convenience for accessing known keys.
   public let values: sourcekitd_api_values
 
-  /// Lock protecting private state.
-  let lock: NSLock = NSLock()
+  private nonisolated let notificationHandlingQueue = AsyncQueue<Serial>()
 
   /// List of notification handlers that will be called for each notification.
-  private var _notificationHandlers: [WeakSKDNotificationHandler] = []
+  private var notificationHandlers: [WeakSKDNotificationHandler] = []
 
   public static func getOrCreate(dylibPath: AbsolutePath) async throws -> SourceKitD {
     try await SourceKitDRegistry.shared
@@ -67,11 +71,13 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
     self.api.initialize()
     self.api.set_notification_handler { [weak self] rawResponse in
       guard let self, let rawResponse else { return }
-      let handlers = self.lock.withLock { self._notificationHandlers.compactMap(\.value) }
-
       let response = SKDResponse(rawResponse, sourcekitd: self)
-      for handler in handlers {
-        handler.notification(response)
+      self.notificationHandlingQueue.async {
+        let handlers = await self.notificationHandlers.compactMap(\.value)
+
+        for handler in handlers {
+          handler.notification(response)
+        }
       }
     }
   }
@@ -85,20 +91,16 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
 
   /// Adds a new notification handler (referenced weakly).
   public func addNotificationHandler(_ handler: SKDNotificationHandler) {
-    lock.withLock {
-      _notificationHandlers.removeAll(where: { $0.value == nil })
-      _notificationHandlers.append(.init(handler))
-    }
+    notificationHandlers.removeAll(where: { $0.value == nil })
+    notificationHandlers.append(.init(handler))
   }
 
   /// Removes a previously registered notification handler.
   public func removeNotificationHandler(_ handler: SKDNotificationHandler) {
-    lock.withLock {
-      _notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler })
-    }
+    notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler })
   }
 
-  public func log(request: SKDRequestDictionary) {
+  public nonisolated func log(request: SKDRequestDictionary) {
     logger.info(
       """
       Sending sourcekitd request:
@@ -107,7 +109,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
     )
   }
 
-  public func log(response: SKDResponse) {
+  public nonisolated func log(response: SKDResponse) {
     logger.log(
       level: (response.error == nil || response.error == .requestCancelled) ? .debug : .error,
       """
@@ -117,7 +119,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
     )
   }
 
-  public func log(crashedRequest req: SKDRequestDictionary, fileContents: String?) {
+  public nonisolated func log(crashedRequest req: SKDRequestDictionary, fileContents: String?) {
     let log = """
       Request:
       \(req.description)
@@ -138,7 +140,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
 
 }
 
-struct WeakSKDNotificationHandler {
+struct WeakSKDNotificationHandler: Sendable {
   weak private(set) var value: SKDNotificationHandler?
   init(_ value: SKDNotificationHandler) {
     self.value = value

Sources/SourceKitD/SKDResponse.swift

@@ -21,10 +21,17 @@ import Musl
 import CRT
 #endif
 
-public final class SKDResponse {
-  public let response: sourcekitd_api_response_t
+public final class SKDResponse: Sendable {
+  /// - Note: `sourcekitd_api_response_t` is a typedef for `void *`, so we can't mark it as sendable. But we know that
+  ///   it stays alive until we deinit this `SKDResponse`. We also require that only a single `SKDResponse` may manage
+  ///   the `sourcekitd_api_response_t`, so raw response cannot be modified or disposed in any other way.
+  private nonisolated(unsafe) let response: sourcekitd_api_response_t
   public let sourcekitd: SourceKitD
 
+  /// Creates a new `SKDResponse` that exclusively manages the raw `sourcekitd_api_response_t`.
+  ///
+  /// - Important: When this `SKDResponse` object gets destroyed, it will dispose the response. It is thus illegal to
+  ///   have two `SDKResponse` objects managing the same `sourcekitd_api_response_t`.
   public init(_ response: sourcekitd_api_response_t, sourcekitd: SourceKitD) {
     self.response = response
     self.sourcekitd = sourcekitd

Sources/SourceKitD/SourceKitD.swift

@@ -23,7 +23,7 @@ import SKSupport
 ///
 /// *Implementors* are expected to handle initialization and shutdown, e.g. during `init` and
 /// `deinit` or by wrapping an existing sourcekitd session that outlives this object.
-public protocol SourceKitD: AnyObject {
+public protocol SourceKitD: AnyObject, Sendable {
   /// The sourcekitd API functions.
   var api: sourcekitd_api_functions_t { get }
 
@@ -37,10 +37,10 @@ public protocol SourceKitD: AnyObject {
   var values: sourcekitd_api_values { get }
 
   /// Adds a new notification handler, which will be weakly referenced.
-  func addNotificationHandler(_ handler: SKDNotificationHandler)
+  func addNotificationHandler(_ handler: SKDNotificationHandler) async
 
   /// Removes a previously registered notification handler.
-  func removeNotificationHandler(_ handler: SKDNotificationHandler)
+  func removeNotificationHandler(_ handler: SKDNotificationHandler) async
 
   /// Log the given request.
   ///
@@ -115,6 +115,6 @@ extension SourceKitD {
 }
 
 /// A sourcekitd notification handler in a class to allow it to be uniquely referenced.
-public protocol SKDNotificationHandler: AnyObject {
+public protocol SKDNotificationHandler: AnyObject, Sendable {
   func notification(_: SKDResponse) -> Void
 }

Sources/SourceKitD/SourceKitDRegistry.swift

@@ -49,7 +49,7 @@ public actor SourceKitDRegistry {
   /// Returns the existing SourceKitD for the given path, or creates it and registers it.
   public func getOrAdd(
     _ key: AbsolutePath,
-    create: () throws -> SourceKitD
+    create: @Sendable () throws -> SourceKitD
   ) rethrows -> SourceKitD {
     if let existing = active[key] {
       return existing

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

@@ -232,8 +232,8 @@ public actor SwiftLanguageServer: ToolchainLanguageServer {
 
 extension SwiftLanguageServer {
 
-  public func initialize(_ initialize: InitializeRequest) throws -> InitializeResult {
-    sourcekitd.addNotificationHandler(self)
+  public func initialize(_ initialize: InitializeRequest) async throws -> InitializeResult {
+    await sourcekitd.addNotificationHandler(self)
 
     return InitializeResult(
       capabilities: ServerCapabilities(
@@ -287,7 +287,7 @@ extension SwiftLanguageServer {
   }
 
   public func shutdown() async {
-    self.sourcekitd.removeNotificationHandler(self)
+    await self.sourcekitd.removeNotificationHandler(self)
   }
 
   /// Tell sourcekitd to crash itself. For testing purposes only.

Tests/SourceKitDTests/SourceKitDTests.swift

@@ -51,7 +51,7 @@ final class SourceKitDTests: XCTestCase {
     defer {
       _fixLifetime(handler1)
     }
-    sourcekitd.addNotificationHandler(handler1)
+    await sourcekitd.addNotificationHandler(handler1)
 
     let expectation2 = expectation(description: "handler 2")
     let handler2 = ClosureNotificationHandler { response in
@@ -63,7 +63,7 @@ final class SourceKitDTests: XCTestCase {
     defer {
       _fixLifetime(handler2)
     }
-    sourcekitd.addNotificationHandler(handler2)
+    await sourcekitd.addNotificationHandler(handler2)
 
     let args = SKDRequestArray(sourcekitd: sourcekitd)
     if case .darwin? = Platform.current,