Skip to content

Commit 8c5ae7c

Browse files
ahoppenahoppen
authored
Merge pull request #1127 from ahoppen/ahoppen/sourcekitd-strict-concurrency
Make the `SourceKitD` module build with strict concurrency enabled
2 parents a6628d0 + e8d0a0d commit 8c5ae7c

File tree

11 files changed

+89
-87
lines changed

11 files changed

+89
-87
lines changed

‎Package.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,8 @@ let package = Package(
271271
"SKSupport",
272272
.product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
273273
],
274-
exclude: ["CMakeLists.txt"]
274+
exclude: ["CMakeLists.txt", "sourcekitd_uids.swift.gyb"],
275+
swiftSettings: [.enableExperimentalFeature("StrictConcurrency")]
275276
),
276277

277278
.testTarget(

‎Sources/Diagnose/SourcekitdRequestCommand.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public struct SourceKitdRequestCommand: AsyncParsableCommand {
4444
public func run() async throws {
4545
var requestString = try String(contentsOf: URL(fileURLWithPath: sourcekitdRequestPath))
4646

47-
let sourcekitd = try DynamicallyLoadedSourceKitD.getOrCreate(
47+
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
4848
dylibPath: try! AbsolutePath(validating: sourcekitdPath)
4949
)
5050

‎Sources/SKSupport/AsyncQueue.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ private class PendingTasks<TaskMetadata: Sendable>: @unchecked Sendable {
8484
}
8585

8686
/// A queue that allows the execution of asynchronous blocks of code.
87-
public final class AsyncQueue<TaskMetadata: DependencyTracker> {
88-
private var pendingTasks: PendingTasks<TaskMetadata> = PendingTasks()
87+
public final class AsyncQueue<TaskMetadata: DependencyTracker>: Sendable {
88+
private let pendingTasks: PendingTasks<TaskMetadata> = PendingTasks()
8989

9090
public init() {}
9191

‎Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,17 @@ import SKSupport
1616

1717
import struct TSCBasic.AbsolutePath
1818

19+
extension DLHandle: @unchecked @retroactive Sendable {}
20+
extension sourcekitd_api_keys: @unchecked Sendable {}
21+
extension sourcekitd_api_requests: @unchecked Sendable {}
22+
extension sourcekitd_api_values: @unchecked Sendable {}
23+
1924
/// Wrapper for sourcekitd, taking care of initialization, shutdown, and notification handler
2025
/// multiplexing.
2126
///
2227
/// Users of this class should not call the api functions `initialize`, `shutdown`, or
2328
/// `set_notification_handler`, which are global state managed internally by this class.
24-
public final class DynamicallyLoadedSourceKitD: SourceKitD {
29+
public actor DynamicallyLoadedSourceKitD: SourceKitD {
2530

2631
/// The path to the sourcekitd dylib.
2732
public let path: AbsolutePath
@@ -41,14 +46,13 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
4146
/// Convenience for accessing known keys.
4247
public let values: sourcekitd_api_values
4348

44-
/// Lock protecting private state.
45-
let lock: NSLock = NSLock()
49+
private nonisolated let notificationHandlingQueue = AsyncQueue<Serial>()
4650

4751
/// List of notification handlers that will be called for each notification.
48-
private var _notificationHandlers: [WeakSKDNotificationHandler] = []
52+
private var notificationHandlers: [WeakSKDNotificationHandler] = []
4953

50-
public static func getOrCreate(dylibPath: AbsolutePath) throws -> SourceKitD {
51-
try SourceKitDRegistry.shared
54+
public static func getOrCreate(dylibPath: AbsolutePath) async throws -> SourceKitD {
55+
try await SourceKitDRegistry.shared
5256
.getOrAdd(dylibPath, create: { try DynamicallyLoadedSourceKitD(dylib: dylibPath) })
5357
}
5458

@@ -67,11 +71,13 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
6771
self.api.initialize()
6872
self.api.set_notification_handler { [weak self] rawResponse in
6973
guard let self, let rawResponse else { return }
70-
let handlers = self.lock.withLock { self._notificationHandlers.compactMap(\.value) }
71-
7274
let response = SKDResponse(rawResponse, sourcekitd: self)
73-
for handler in handlers {
74-
handler.notification(response)
75+
self.notificationHandlingQueue.async {
76+
let handlers = await self.notificationHandlers.compactMap(\.value)
77+
78+
for handler in handlers {
79+
handler.notification(response)
80+
}
7581
}
7682
}
7783
}
@@ -85,20 +91,16 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
8591

8692
/// Adds a new notification handler (referenced weakly).
8793
public func addNotificationHandler(_ handler: SKDNotificationHandler) {
88-
lock.withLock {
89-
_notificationHandlers.removeAll(where: { $0.value == nil })
90-
_notificationHandlers.append(.init(handler))
91-
}
94+
notificationHandlers.removeAll(where: { $0.value == nil })
95+
notificationHandlers.append(.init(handler))
9296
}
9397

9498
/// Removes a previously registered notification handler.
9599
public func removeNotificationHandler(_ handler: SKDNotificationHandler) {
96-
lock.withLock {
97-
_notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler })
98-
}
100+
notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler })
99101
}
100102

101-
public func log(request: SKDRequestDictionary) {
103+
public nonisolated func log(request: SKDRequestDictionary) {
102104
logger.info(
103105
"""
104106
Sending sourcekitd request:
@@ -107,7 +109,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
107109
)
108110
}
109111

110-
public func log(response: SKDResponse) {
112+
public nonisolated func log(response: SKDResponse) {
111113
logger.log(
112114
level: (response.error == nil || response.error == .requestCancelled) ? .debug : .error,
113115
"""
@@ -117,7 +119,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
117119
)
118120
}
119121

120-
public func log(crashedRequest req: SKDRequestDictionary, fileContents: String?) {
122+
public nonisolated func log(crashedRequest req: SKDRequestDictionary, fileContents: String?) {
121123
let log = """
122124
Request:
123125
\(req.description)
@@ -138,7 +140,7 @@ public final class DynamicallyLoadedSourceKitD: SourceKitD {
138140

139141
}
140142

141-
struct WeakSKDNotificationHandler {
143+
struct WeakSKDNotificationHandler: Sendable {
142144
weak private(set) var value: SKDNotificationHandler?
143145
init(_ value: SKDNotificationHandler) {
144146
self.value = value

‎Sources/SourceKitD/SKDResponse.swift

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,17 @@ import Musl
2121
import CRT
2222
#endif
2323

24-
public final class SKDResponse {
25-
public let response: sourcekitd_api_response_t
24+
public final class SKDResponse: Sendable {
25+
/// - Note: `sourcekitd_api_response_t` is a typedef for `void *`, so we can't mark it as sendable. But we know that
26+
/// it stays alive until we deinit this `SKDResponse`. We also require that only a single `SKDResponse` may manage
27+
/// the `sourcekitd_api_response_t`, so raw response cannot be modified or disposed in any other way.
28+
private nonisolated(unsafe) let response: sourcekitd_api_response_t
2629
public let sourcekitd: SourceKitD
2730

31+
/// Creates a new `SKDResponse` that exclusively manages the raw `sourcekitd_api_response_t`.
32+
///
33+
/// - Important: When this `SKDResponse` object gets destroyed, it will dispose the response. It is thus illegal to
34+
/// have two `SDKResponse` objects managing the same `sourcekitd_api_response_t`.
2835
public init(_ response: sourcekitd_api_response_t, sourcekitd: SourceKitD) {
2936
self.response = response
3037
self.sourcekitd = sourcekitd

‎Sources/SourceKitD/SourceKitD.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import SKSupport
2323
///
2424
/// *Implementors* are expected to handle initialization and shutdown, e.g. during `init` and
2525
/// `deinit` or by wrapping an existing sourcekitd session that outlives this object.
26-
public protocol SourceKitD: AnyObject {
26+
public protocol SourceKitD: AnyObject, Sendable {
2727
/// The sourcekitd API functions.
2828
var api: sourcekitd_api_functions_t { get }
2929

@@ -37,10 +37,10 @@ public protocol SourceKitD: AnyObject {
3737
var values: sourcekitd_api_values { get }
3838

3939
/// Adds a new notification handler, which will be weakly referenced.
40-
func addNotificationHandler(_ handler: SKDNotificationHandler)
40+
func addNotificationHandler(_ handler: SKDNotificationHandler) async
4141

4242
/// Removes a previously registered notification handler.
43-
func removeNotificationHandler(_ handler: SKDNotificationHandler)
43+
func removeNotificationHandler(_ handler: SKDNotificationHandler) async
4444

4545
/// Log the given request.
4646
///
@@ -115,6 +115,6 @@ extension SourceKitD {
115115
}
116116

117117
/// A sourcekitd notification handler in a class to allow it to be uniquely referenced.
118-
public protocol SKDNotificationHandler: AnyObject {
118+
public protocol SKDNotificationHandler: AnyObject, Sendable {
119119
func notification(_: SKDResponse) -> Void
120120
}

‎Sources/SourceKitD/SourceKitDRegistry.swift

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,7 @@ extension NSLock {
3232
/// * To remove an existing instance, use `remove("path")`, but be aware that if there are any other
3333
/// references to the instances in the program, it can be resurrected if `getOrAdd` is called with
3434
/// the same path. See note on `remove(_:)`
35-
public final class SourceKitDRegistry {
36-
37-
/// Mutex protecting mutable state in the registry.
38-
let lock: NSLock = NSLock()
35+
public actor SourceKitDRegistry {
3936

4037
/// Mapping from path to active SourceKitD instance.
4138
var active: [AbsolutePath: SourceKitD] = [:]
@@ -47,26 +44,24 @@ public final class SourceKitDRegistry {
4744
public init() {}
4845

4946
/// The global shared SourceKitD registry.
50-
public static var shared: SourceKitDRegistry = SourceKitDRegistry()
47+
public static let shared: SourceKitDRegistry = SourceKitDRegistry()
5148

5249
/// Returns the existing SourceKitD for the given path, or creates it and registers it.
5350
public func getOrAdd(
5451
_ key: AbsolutePath,
55-
create: () throws -> SourceKitD
52+
create: @Sendable () throws -> SourceKitD
5653
) rethrows -> SourceKitD {
57-
try lock.withLock {
58-
if let existing = active[key] {
59-
return existing
60-
}
61-
if let resurrected = cemetary[key]?.value {
62-
cemetary[key] = nil
63-
active[key] = resurrected
64-
return resurrected
65-
}
66-
let newValue = try create()
67-
active[key] = newValue
68-
return newValue
54+
if let existing = active[key] {
55+
return existing
56+
}
57+
if let resurrected = cemetary[key]?.value {
58+
cemetary[key] = nil
59+
active[key] = resurrected
60+
return resurrected
6961
}
62+
let newValue = try create()
63+
active[key] = newValue
64+
return newValue
7065
}
7166

7267
/// Removes the SourceKitD instance registered for the given path, if any, from the set of active
@@ -77,22 +72,18 @@ public final class SourceKitDRegistry {
7772
/// the same path is looked up again before the original service is deinitialized, the original
7873
/// service is resurrected rather than creating a new instance.
7974
public func remove(_ key: AbsolutePath) -> SourceKitD? {
80-
lock.withLock {
81-
let existing = active.removeValue(forKey: key)
82-
if let existing = existing {
83-
assert(self.cemetary[key]?.value == nil)
84-
cemetary[key] = WeakSourceKitD(value: existing)
85-
}
86-
return existing
75+
let existing = active.removeValue(forKey: key)
76+
if let existing = existing {
77+
assert(self.cemetary[key]?.value == nil)
78+
cemetary[key] = WeakSourceKitD(value: existing)
8779
}
80+
return existing
8881
}
8982

9083
/// Remove all SourceKitD instances, including weak ones.
9184
public func clear() {
92-
lock.withLock {
93-
active.removeAll()
94-
cemetary.removeAll()
95-
}
85+
active.removeAll()
86+
cemetary.removeAll()
9687
}
9788
}
9889

‎Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ public actor SwiftLanguageServer: ToolchainLanguageServer {
179179
guard let sourcekitd = toolchain.sourcekitd else { return nil }
180180
self.sourceKitServer = sourceKitServer
181181
self.swiftFormat = toolchain.swiftFormat
182-
self.sourcekitd = try DynamicallyLoadedSourceKitD.getOrCreate(dylibPath: sourcekitd)
182+
self.sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(dylibPath: sourcekitd)
183183
self.capabilityRegistry = workspace.capabilityRegistry
184184
self.serverOptions = options
185185
self.documentManager = DocumentManager()
@@ -232,8 +232,8 @@ public actor SwiftLanguageServer: ToolchainLanguageServer {
232232

233233
extension SwiftLanguageServer {
234234

235-
public func initialize(_ initialize: InitializeRequest) throws -> InitializeResult {
236-
sourcekitd.addNotificationHandler(self)
235+
public func initialize(_ initialize: InitializeRequest) async throws -> InitializeResult {
236+
await sourcekitd.addNotificationHandler(self)
237237

238238
return InitializeResult(
239239
capabilities: ServerCapabilities(
@@ -287,7 +287,7 @@ extension SwiftLanguageServer {
287287
}
288288

289289
public func shutdown() async {
290-
self.sourcekitd.removeNotificationHandler(self)
290+
await self.sourcekitd.removeNotificationHandler(self)
291291
}
292292

293293
/// Tell sourcekitd to crash itself. For testing purposes only.

‎Tests/DiagnoseTests/DiagnoseTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ private struct InProcessSourceKitRequestExecutor: SourceKitRequestExecutor {
211211
func run(request requestYaml: String) async throws -> SourceKitDRequestResult {
212212
logger.info("Sending request: \(requestYaml)")
213213

214-
let sourcekitd = try DynamicallyLoadedSourceKitD.getOrCreate(
214+
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
215215
dylibPath: try! AbsolutePath(validating: sourcekitd.path)
216216
)
217217
let response = try await sourcekitd.run(requestYaml: requestYaml)

‎Tests/SourceKitDTests/SourceKitDRegistryTests.swift

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,49 +10,50 @@
1010
//
1111
//===----------------------------------------------------------------------===//
1212

13+
import LSPTestSupport
1314
import SourceKitD
1415
import TSCBasic
1516
import XCTest
1617

1718
final class SourceKitDRegistryTests: XCTestCase {
1819

19-
func testAdd() throws {
20+
func testAdd() async throws {
2021
let registry = SourceKitDRegistry()
2122

22-
let a = try FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/a"), in: registry)
23-
let b = try FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/b"), in: registry)
24-
let a2 = try FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/a"), in: registry)
23+
let a = try await FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/a"), in: registry)
24+
let b = try await FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/b"), in: registry)
25+
let a2 = try await FakeSourceKitD.getOrCreate(AbsolutePath(validating: "/a"), in: registry)
2526

2627
XCTAssert(a === a2)
2728
XCTAssert(a !== b)
2829
}
2930

30-
func testRemove() throws {
31+
func testRemove() async throws {
3132
let registry = SourceKitDRegistry()
3233

33-
let a = FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
34-
XCTAssert(registry.remove(try AbsolutePath(validating: "/a")) === a)
35-
XCTAssertNil(registry.remove(try AbsolutePath(validating: "/a")))
34+
let a = await FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
35+
await assertTrue(registry.remove(try AbsolutePath(validating: "/a")) === a)
36+
await assertNil(registry.remove(try AbsolutePath(validating: "/a")))
3637
}
3738

38-
func testRemoveResurrect() throws {
39+
func testRemoveResurrect() async throws {
3940
let registry = SourceKitDRegistry()
4041

4142
@inline(never)
42-
func scope(registry: SourceKitDRegistry) throws -> Int {
43-
let a = FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
43+
func scope(registry: SourceKitDRegistry) async throws -> Int {
44+
let a = await FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
4445

45-
XCTAssert(a === FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry))
46-
XCTAssert(registry.remove(try AbsolutePath(validating: "/a")) === a)
46+
await assertTrue(a === FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry))
47+
await assertTrue(registry.remove(try AbsolutePath(validating: "/a")) === a)
4748
// Resurrected.
48-
XCTAssert(a === FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry))
49+
await assertTrue(a === FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry))
4950
// Remove again.
50-
XCTAssert(registry.remove(try AbsolutePath(validating: "/a")) === a)
51+
await assertTrue(registry.remove(try AbsolutePath(validating: "/a")) === a)
5152
return (a as! FakeSourceKitD).token
5253
}
5354

54-
let id = try scope(registry: registry)
55-
let a2 = FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
55+
let id = try await scope(registry: registry)
56+
let a2 = await FakeSourceKitD.getOrCreate(try AbsolutePath(validating: "/a"), in: registry)
5657
XCTAssertNotEqual(id, (a2 as! FakeSourceKitD).token)
5758
}
5859
}
@@ -72,8 +73,8 @@ final class FakeSourceKitD: SourceKitD {
7273
nextToken += 1
7374
}
7475

75-
static func getOrCreate(_ path: AbsolutePath, in registry: SourceKitDRegistry) -> SourceKitD {
76-
return registry.getOrAdd(path, create: { Self.init() })
76+
static func getOrCreate(_ path: AbsolutePath, in registry: SourceKitDRegistry) async -> SourceKitD {
77+
return await registry.getOrAdd(path, create: { Self.init() })
7778
}
7879

7980
public func log(request: SKDRequestDictionary) {}

0 commit comments

Comments
 (0)