Remove the split between SourceKitD and DynamicallyLoadedSourceKitD

There is only one real class that implements the `SourceKitD` protocol, so there really isn’t any need for the protocol + class split at all. Unify them to make code simpler to reason about.

Author: ahoppen

Sources/BuildSystemIntegration/BuildSystemMessageDependencyTracker.swift

@@ -14,7 +14,7 @@ import BuildServerProtocol
 package import LanguageServerProtocol
 import LanguageServerProtocolExtensions
 import SKLogging
-package import SwiftExtensions
+import SwiftExtensions
 
 /// A lightweight way of describing tasks that are created from handling BSP
 /// requests or notifications for the purpose of dependency tracking.

Sources/Diagnose/RunSourcekitdRequestCommand.swift

@@ -54,7 +54,7 @@ package struct RunSourceKitdRequestCommand: AsyncParsableCommand {
         print("Did not find sourcekitd in the toolchain. Specify path to sourcekitd manually by passing --sourcekitd")
         throw ExitCode(1)
       }
-    let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
+    let sourcekitd = try await SourceKitD.getOrCreate(
       dylibPath: sourcekitdPath,
       pluginPaths: nil
     )

Sources/SKTestSupport/SkipUnless.swift

@@ -253,7 +253,7 @@ package actor SkipUnless {
       guard let sourcekitdPath = await ToolchainRegistry.forTesting.default?.sourcekitd else {
         throw GenericError("Could not find SourceKitD")
       }
-      let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
+      let sourcekitd = try await SourceKitD.getOrCreate(
         dylibPath: sourcekitdPath,
         pluginPaths: try sourceKitPluginPaths
       )

Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift

@@ -55,26 +55,37 @@ fileprivate func setenv(name: String, value: String, override: Bool) throws {
 ///
 /// 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.
-package actor DynamicallyLoadedSourceKitD: SourceKitD {
+package actor SourceKitD {
   /// The path to the sourcekitd dylib.
   package let path: URL
 
   /// The handle to the dylib.
   let dylib: DLHandle
 
   /// The sourcekitd API functions.
-  package let api: sourcekitd_api_functions_t
+  nonisolated package let api: sourcekitd_api_functions_t
 
-  private let pluginApiResult: Result<sourcekitd_plugin_api_functions_t, Error>
+  /// General API for the SourceKit service and client framework, eg. for plugin initialization and to set up custom
+  /// variant functions.
+  ///
+  /// This must not be referenced outside of `SwiftSourceKitPlugin`, `SwiftSourceKitPluginCommon`, or
+  /// `SwiftSourceKitClientPlugin`.
   package nonisolated var pluginApi: sourcekitd_plugin_api_functions_t { try! pluginApiResult.get() }
+  private let pluginApiResult: Result<sourcekitd_plugin_api_functions_t, Error>
 
-  private let servicePluginApiResult: Result<sourcekitd_service_plugin_api_functions_t, Error>
+  /// The API with which the SourceKit plugin handles requests.
+  ///
+  /// This must not be referenced outside of `SwiftSourceKitPlugin`.
   package nonisolated var servicePluginApi: sourcekitd_service_plugin_api_functions_t {
     try! servicePluginApiResult.get()
   }
+  private let servicePluginApiResult: Result<sourcekitd_service_plugin_api_functions_t, Error>
 
-  private let ideApiResult: Result<sourcekitd_ide_api_functions_t, Error>
+  /// The API with which the SourceKit plugin communicates with the type-checker in-process.
+  ///
+  /// This must not be referenced outside of `SwiftSourceKitPlugin`.
   package nonisolated var ideApi: sourcekitd_ide_api_functions_t { try! ideApiResult.get() }
+  private let ideApiResult: Result<sourcekitd_ide_api_functions_t, Error>
 
   /// Convenience for accessing known keys.
   ///
@@ -128,7 +139,7 @@ package actor DynamicallyLoadedSourceKitD: SourceKitD {
         )
       }
       #endif
-      return try DynamicallyLoadedSourceKitD(dylib: dylibPath, pluginPaths: pluginPaths)
+      return try SourceKitD(dylib: dylibPath, pluginPaths: pluginPaths)
     }
   }
 
@@ -224,60 +235,11 @@ package actor DynamicallyLoadedSourceKitD: SourceKitD {
     try await body()
   }
 
-  package func didSend(request: SKDRequestDictionary) {
+  func didSend(request: SKDRequestDictionary) {
     for hook in requestHandlingHooks.values {
       hook(request)
     }
   }
-
-  package nonisolated func log(request: SKDRequestDictionary) {
-    logger.info(
-      """
-      Sending sourcekitd request:
-      \(request.forLogging)
-      """
-    )
-  }
-
-  package nonisolated func log(response: SKDResponse) {
-    logger.log(
-      level: (response.error == nil || response.error == .requestCancelled) ? .debug : .error,
-      """
-      Received sourcekitd response:
-      \(response.forLogging)
-      """
-    )
-  }
-
-  package nonisolated func log(crashedRequest req: SKDRequestDictionary, fileContents: String?) {
-    let log = """
-      Request:
-      \(req.description)
-
-      File contents:
-      \(fileContents ?? "<nil>")
-      """
-    let chunks = splitLongMultilineMessage(message: log)
-    for (index, chunk) in chunks.enumerated() {
-      logger.fault(
-        """
-        sourcekitd crashed (\(index + 1)/\(chunks.count))
-        \(chunk)
-        """
-      )
-    }
-  }
-
-  package nonisolated func logRequestCancellation(request: SKDRequestDictionary) {
-    // We don't need to log which request has been cancelled because we can associate the cancellation log message with
-    // the send message via the log
-    logger.info(
-      """
-      Cancelling sourcekitd request:
-      \(request.forLogging)
-      """
-    )
-  }
 }
 
 struct WeakSKDNotificationHandler: Sendable {

Sources/SourceKitD/SKDRequestArray.swift

@@ -26,7 +26,7 @@ import Bionic
 
 extension SourceKitD {
   /// Create a `SKDRequestArray` from the given array.
-  package func array(_ array: [SKDRequestValue]) -> SKDRequestArray {
+  nonisolated package func array(_ array: [SKDRequestValue]) -> SKDRequestArray {
     let result = SKDRequestArray(sourcekitd: self)
     for element in array {
       result.append(element)

Sources/SourceKitD/SKDRequestDictionary.swift

@@ -44,7 +44,7 @@ extension Optional: SKDRequestValue where Wrapped: SKDRequestValue {}
 
 extension SourceKitD {
   /// Create a `SKDRequestDictionary` from the given dictionary.
-  package func dictionary(_ dict: [sourcekitd_api_uid_t: SKDRequestValue]) -> SKDRequestDictionary {
+  nonisolated package func dictionary(_ dict: [sourcekitd_api_uid_t: SKDRequestValue]) -> SKDRequestDictionary {
     let result = SKDRequestDictionary(sourcekitd: self)
     for (key, value) in dict {
       result.set(key, to: value)

Sources/SourceKitD/SourceKitD.swift

@@ -10,8 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-package import Csourcekitd
-import Dispatch
+import Csourcekitd
 package import Foundation
 import SKLogging
 import SwiftExtensions
@@ -39,76 +38,6 @@ package struct PluginPaths: Equatable, CustomLogStringConvertible {
   }
 }
 
-/// Access to sourcekitd API, taking care of initialization, shutdown, and notification handler
-/// multiplexing.
-///
-/// *Users* of this protocol should not call the api functions `initialize`, `shutdown`, or
-/// `set_notification_handler`, which are global state managed internally by this class.
-///
-/// *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.
-package protocol SourceKitD: AnyObject, Sendable {
-  /// The sourcekitd API functions.
-  var api: sourcekitd_api_functions_t { get }
-
-  /// General API for the SourceKit service and client framework, eg. for plugin initialization and to set up custom
-  /// variant functions.
-  ///
-  /// This must not be referenced outside of `SwiftSourceKitPlugin`, `SwiftSourceKitPluginCommon`, or
-  /// `SwiftSourceKitClientPlugin`.
-  var pluginApi: sourcekitd_plugin_api_functions_t { get }
-
-  /// The API with which the SourceKit plugin handles requests.
-  ///
-  /// This must not be referenced outside of `SwiftSourceKitPlugin`.
-  var servicePluginApi: sourcekitd_service_plugin_api_functions_t { get }
-
-  /// The API with which the SourceKit plugin communicates with the type-checker in-process.
-  ///
-  /// This must not be referenced outside of `SwiftSourceKitPlugin`.
-  var ideApi: sourcekitd_ide_api_functions_t { get }
-
-  /// Convenience for accessing known keys.
-  var keys: sourcekitd_api_keys { get }
-
-  /// Convenience for accessing known keys.
-  var requests: sourcekitd_api_requests { get }
-
-  /// Convenience for accessing known keys.
-  var values: sourcekitd_api_values { get }
-
-  /// Adds a new notification handler, which will be weakly referenced.
-  func addNotificationHandler(_ handler: SKDNotificationHandler) async
-
-  /// Removes a previously registered notification handler.
-  func removeNotificationHandler(_ handler: SKDNotificationHandler) async
-
-  /// A function that gets called after the request has been sent to sourcekitd but but does not wait for results to be
-  /// received. This can be used by clients to implement hooks that should be executed for every sourcekitd request.
-  func didSend(request: SKDRequestDictionary) async
-
-  /// Log the given request.
-  ///
-  /// This log call is issued during normal operation. It is acceptable for the logger to truncate the log message
-  /// to achieve good performance.
-  func log(request: SKDRequestDictionary)
-
-  /// Log the given request and file contents, ensuring they do not get truncated.
-  ///
-  /// This log call is used when a request has crashed. In this case we want the log to contain the entire request to be
-  /// able to reproduce it.
-  func log(crashedRequest: SKDRequestDictionary, fileContents: String?)
-
-  /// Log the given response.
-  ///
-  /// This log call is issued during normal operation. It is acceptable for the logger to truncate the log message
-  /// to achieve good performance.
-  func log(response: SKDResponse)
-
-  /// Log that the given request has been cancelled.
-  func logRequestCancellation(request: SKDRequestDictionary)
-}
-
 package enum SKDError: Error, Equatable {
   /// The service has crashed.
   case connectionInterrupted
@@ -130,8 +59,6 @@ package enum SKDError: Error, Equatable {
 }
 
 extension SourceKitD {
-  // MARK: - Convenience API for requests.
-
   /// - Parameters:
   ///   - request: The request to send to sourcekitd.
   ///   - timeout: The maximum duration how long to wait for a response. If no response is returned within this time,
@@ -145,7 +72,12 @@ extension SourceKitD {
   ) async throws -> SKDResponseDictionary {
     let sourcekitdResponse = try await withTimeout(timeout) {
       return try await withCancellableCheckedThrowingContinuation { (continuation) -> SourceKitDRequestHandle? in
-        self.log(request: request)
+        logger.info(
+          """
+          Sending sourcekitd request:
+          \(request.forLogging)
+          """
+        )
         var handle: sourcekitd_api_request_handle_t? = nil
         self.api.send_request(request.dict, &handle) { response in
           continuation.resume(returning: SKDResponse(response!, sourcekitd: self))
@@ -159,17 +91,43 @@ extension SourceKitD {
         return nil
       } cancel: { (handle: SourceKitDRequestHandle?) in
         if let handle {
-          self.logRequestCancellation(request: request)
+          logger.info(
+            """
+            Cancelling sourcekitd request:
+            \(request.forLogging)
+            """
+          )
           self.api.cancel_request(handle.handle)
         }
       }
     }
 
-    log(response: sourcekitdResponse)
+    logger.log(
+      level: (sourcekitdResponse.error == nil || sourcekitdResponse.error == .requestCancelled) ? .debug : .error,
+      """
+      Received sourcekitd response:
+      \(sourcekitdResponse.forLogging)
+      """
+    )
 
     guard let dict = sourcekitdResponse.value else {
       if sourcekitdResponse.error == .connectionInterrupted {
-        log(crashedRequest: request, fileContents: fileContents)
+        let log = """
+          Request:
+          \(request.description)
+
+          File contents:
+          \(fileContents ?? "<nil>")
+          """
+        let chunks = splitLongMultilineMessage(message: log)
+        for (index, chunk) in chunks.enumerated() {
+          logger.fault(
+            """
+            sourcekitd crashed (\(index + 1)/\(chunks.count))
+            \(chunk)
+            """
+          )
+        }
       }
       if sourcekitdResponse.error == .requestCancelled && !Task.isCancelled {
         throw SKDError.timedOut

Sources/SourceKitD/SourceKitDRegistry.swift

@@ -22,26 +22,25 @@ import SKLogging
 /// * To remove an existing instance, use `remove("path")`, but be aware that if there are any other
 ///   references to the instances in the program, it can be resurrected if `getOrAdd` is called with
 ///   the same path. See note on `remove(_:)`
-package actor SourceKitDRegistry {
+///
+/// `SourceKitDType` is usually `SourceKitD` but can be substituted for a different type for testing purposes.
+package actor SourceKitDRegistry<SourceKitDType: AnyObject> {
 
   /// Mapping from path to active SourceKitD instance.
-  private var active: [URL: (pluginPaths: PluginPaths?, sourcekitd: SourceKitD)] = [:]
+  private var active: [URL: (pluginPaths: PluginPaths?, sourcekitd: SourceKitDType)] = [:]
 
   /// Instances that have been unregistered, but may be resurrected if accessed before destruction.
-  private var cemetary: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD)] = [:]
+  private var cemetary: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD<SourceKitDType>)] = [:]
 
   /// Initialize an empty registry.
   package init() {}
 
-  /// The global shared SourceKitD registry.
-  package static let shared: SourceKitDRegistry = SourceKitDRegistry()
-
   /// Returns the existing SourceKitD for the given path, or creates it and registers it.
   package func getOrAdd(
     _ key: URL,
     pluginPaths: PluginPaths?,
-    create: () throws -> SourceKitD
-  ) async rethrows -> SourceKitD {
+    create: () throws -> SourceKitDType
+  ) async rethrows -> SourceKitDType {
     if let existing = active[key] {
       if existing.pluginPaths != pluginPaths {
         logger.fault(
@@ -72,7 +71,7 @@ package actor SourceKitDRegistry {
   /// is converted to a weak reference until it is no longer referenced anywhere by the program. If
   /// the same path is looked up again before the original service is deinitialized, the original
   /// service is resurrected rather than creating a new instance.
-  package func remove(_ key: URL) -> SourceKitD? {
+  package func remove(_ key: URL) -> SourceKitDType? {
     let existing = active.removeValue(forKey: key)
     if let existing = existing {
       assert(self.cemetary[key]?.sourcekitd.value == nil)
@@ -82,6 +81,11 @@ package actor SourceKitDRegistry {
   }
 }
 
-fileprivate struct WeakSourceKitD {
-  weak var value: SourceKitD?
+extension SourceKitDRegistry<SourceKitD> {
+  /// The global shared SourceKitD registry.
+  package static let shared: SourceKitDRegistry = SourceKitDRegistry()
+}
+
+fileprivate struct WeakSourceKitD<SourceKitDType: AnyObject> {
+  weak var value: SourceKitDType?
 }