Skip to content

Commit 1286407

Browse files
committed
Remove sourcekitd test hooks
Turns out that sourcekitd test hooks were a bad idea because of the following comment that I wrote: ``` `testHooks` are only considered when an instance is being created. If a sourcekitd instance at the given path already exists, its test hooks will be used. ``` During test execution in Xcode, we generate a bunch of `SourceKitServer` instances in the same process that all call `DynamicallyLoadedSourceKitD.getOrCreate`. Now, if `testDontReturnEmptyDiagnosticsIfDiagnosticRequestIsCancelled` is not the first test being executed in the process (which usually it is not), the test hooks in it won’t get used. Switch back to using the preparation hooks, essentially reverting #1412 and keeping the following snippet to fix the underlying issue ```swift // Poll until the `CancelRequestNotification` has been propagated to the request handling. for _ in 0..<Int(defaultTimeout * 100) { if Task.isCancelled { break } usleep(10_000) } ```
1 parent 6d42f07 commit 1286407

File tree

9 files changed

+14
-51
lines changed

9 files changed

+14
-51
lines changed

Sources/Diagnose/RunSourcekitdRequestCommand.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,7 @@ public struct RunSourceKitdRequestCommand: AsyncParsableCommand {
5555
throw ExitCode(1)
5656
}
5757
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
58-
dylibPath: try! AbsolutePath(validating: sourcekitdPath),
59-
testHooks: SourceKitDTestHooks()
58+
dylibPath: try! AbsolutePath(validating: sourcekitdPath)
6059
)
6160

6261
if let lineColumn = position?.split(separator: ":", maxSplits: 2).map(Int.init),

Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -26,27 +26,17 @@ extension sourcekitd_api_keys: @unchecked Sendable {}
2626
extension sourcekitd_api_requests: @unchecked Sendable {}
2727
extension sourcekitd_api_values: @unchecked Sendable {}
2828

29-
public struct SourceKitDTestHooks: Sendable {
30-
public var sourcekitdRequestDidStart: (@Sendable (SKDRequestDictionary) -> Void)?
31-
32-
public init(sourcekitdRequestDidStart: (@Sendable (SKDRequestDictionary) -> Void)? = nil) {
33-
self.sourcekitdRequestDidStart = sourcekitdRequestDidStart
34-
}
35-
}
36-
3729
/// Wrapper for sourcekitd, taking care of initialization, shutdown, and notification handler
3830
/// multiplexing.
3931
///
4032
/// Users of this class should not call the api functions `initialize`, `shutdown`, or
4133
/// `set_notification_handler`, which are global state managed internally by this class.
4234
public actor DynamicallyLoadedSourceKitD: SourceKitD {
4335
/// The path to the sourcekitd dylib.
44-
private let path: AbsolutePath
36+
public let path: AbsolutePath
4537

4638
/// The handle to the dylib.
47-
private let dylib: DLHandle
48-
49-
public let testHooks: SourceKitDTestHooks
39+
let dylib: DLHandle
5040

5141
/// The sourcekitd API functions.
5242
public let api: sourcekitd_api_functions_t
@@ -65,23 +55,18 @@ public actor DynamicallyLoadedSourceKitD: SourceKitD {
6555
/// List of notification handlers that will be called for each notification.
6656
private var notificationHandlers: [WeakSKDNotificationHandler] = []
6757

68-
/// If there is already a `sourcekitd` instance from the given return it, otherwise create a new one.
69-
///
70-
/// `testHooks` are only considered when an instance is being created. If a sourcekitd instance at the given path
71-
/// already exists, its test hooks will be used.
72-
public static func getOrCreate(dylibPath: AbsolutePath, testHooks: SourceKitDTestHooks) async throws -> SourceKitD {
58+
public static func getOrCreate(dylibPath: AbsolutePath) async throws -> SourceKitD {
7359
try await SourceKitDRegistry.shared
74-
.getOrAdd(dylibPath, create: { try DynamicallyLoadedSourceKitD(dylib: dylibPath, testHooks: testHooks) })
60+
.getOrAdd(dylibPath, create: { try DynamicallyLoadedSourceKitD(dylib: dylibPath) })
7561
}
7662

77-
init(dylib path: AbsolutePath, testHooks: SourceKitDTestHooks) throws {
63+
init(dylib path: AbsolutePath) throws {
7864
self.path = path
7965
#if os(Windows)
8066
self.dylib = try dlopen(path.pathString, mode: [])
8167
#else
8268
self.dylib = try dlopen(path.pathString, mode: [.lazy, .local, .first])
8369
#endif
84-
self.testHooks = testHooks
8570
self.api = try sourcekitd_api_functions_t(self.dylib)
8671
self.keys = sourcekitd_api_keys(api: self.api)
8772
self.requests = sourcekitd_api_requests(api: self.api)

Sources/SourceKitD/SourceKitD.swift

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,6 @@ extension sourcekitd_api_request_handle_t: @unchecked Sendable {}
3131
/// *Implementors* are expected to handle initialization and shutdown, e.g. during `init` and
3232
/// `deinit` or by wrapping an existing sourcekitd session that outlives this object.
3333
public protocol SourceKitD: AnyObject, Sendable {
34-
var testHooks: SourceKitDTestHooks { get }
35-
3634
/// The sourcekitd API functions.
3735
var api: sourcekitd_api_functions_t { get }
3836

@@ -101,8 +99,6 @@ extension SourceKitD {
10199
public func send(_ request: SKDRequestDictionary, fileContents: String?) async throws -> SKDResponseDictionary {
102100
log(request: request)
103101

104-
testHooks.sourcekitdRequestDidStart?(request)
105-
106102
let sourcekitdResponse: SKDResponse = try await withCancellableCheckedThrowingContinuation { continuation in
107103
var handle: sourcekitd_api_request_handle_t? = nil
108104
api.send_request(request.dict, &handle) { response in

Sources/SourceKitLSP/SourceKitLSPServer+Options.swift

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import LanguageServerProtocol
1515
import SKCore
1616
import SKSupport
1717
import SemanticIndex
18-
import SourceKitD
1918

2019
import struct TSCBasic.AbsolutePath
2120
import struct TSCBasic.RelativePath
@@ -52,8 +51,6 @@ extension SourceKitLSPServer {
5251
/// Experimental features that are enabled.
5352
public var experimentalFeatures: Set<ExperimentalFeature>
5453

55-
public var sourcekitdTestHooks: SourceKitDTestHooks
56-
5754
public var indexTestHooks: IndexTestHooks
5855

5956
public init(
@@ -65,7 +62,6 @@ extension SourceKitLSPServer {
6562
generatedInterfacesPath: AbsolutePath = defaultDirectoryForGeneratedInterfaces,
6663
swiftPublishDiagnosticsDebounceDuration: TimeInterval = 2, /* 2s */
6764
experimentalFeatures: Set<ExperimentalFeature> = [],
68-
sourcekitdTestHooks: SourceKitDTestHooks = SourceKitDTestHooks(),
6965
indexTestHooks: IndexTestHooks = IndexTestHooks()
7066
) {
7167
self.buildSetup = buildSetup
@@ -76,7 +72,6 @@ extension SourceKitLSPServer {
7672
self.generatedInterfacesPath = generatedInterfacesPath
7773
self.swiftPublishDiagnosticsDebounceDuration = swiftPublishDiagnosticsDebounceDuration
7874
self.experimentalFeatures = experimentalFeatures
79-
self.sourcekitdTestHooks = sourcekitdTestHooks
8075
self.indexTestHooks = indexTestHooks
8176
}
8277
}

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,7 @@ public actor SwiftLanguageService: LanguageService, Sendable {
195195
guard let sourcekitd = toolchain.sourcekitd else { return nil }
196196
self.sourceKitLSPServer = sourceKitLSPServer
197197
self.swiftFormat = toolchain.swiftFormat
198-
self.sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
199-
dylibPath: sourcekitd,
200-
testHooks: options.sourcekitdTestHooks
201-
)
198+
self.sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(dylibPath: sourcekitd)
202199
self.capabilityRegistry = workspace.capabilityRegistry
203200
self.semanticIndexManager = workspace.semanticIndexManager
204201
self.serverOptions = options

Tests/DiagnoseTests/DiagnoseTests.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,8 +318,7 @@ private class InProcessSourceKitRequestExecutor: SourceKitRequestExecutor {
318318
logger.info("Sending request: \(requestString)")
319319

320320
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
321-
dylibPath: try! AbsolutePath(validating: sourcekitd.path),
322-
testHooks: SourceKitDTestHooks()
321+
dylibPath: try! AbsolutePath(validating: sourcekitd.path)
323322
)
324323
let response = try await sourcekitd.run(requestYaml: requestString)
325324

Tests/SourceKitDTests/SourceKitDRegistryTests.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ private nonisolated(unsafe) var nextToken = AtomicUInt32(initialValue: 0)
6363

6464
final class FakeSourceKitD: SourceKitD {
6565
let token: UInt32
66-
var testHooks: SourceKitDTestHooks { SourceKitDTestHooks() }
6766
var api: sourcekitd_api_functions_t { fatalError() }
6867
var keys: sourcekitd_api_keys { fatalError() }
6968
var requests: sourcekitd_api_requests { fatalError() }

Tests/SourceKitDTests/SourceKitDTests.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,7 @@ import class TSCBasic.Process
2828
final class SourceKitDTests: XCTestCase {
2929
func testMultipleNotificationHandlers() async throws {
3030
let sourcekitdPath = await ToolchainRegistry.forTesting.default!.sourcekitd!
31-
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(
32-
dylibPath: sourcekitdPath,
33-
testHooks: SourceKitDTestHooks()
34-
)
31+
let sourcekitd = try await DynamicallyLoadedSourceKitD.getOrCreate(dylibPath: sourcekitdPath)
3532
let keys = sourcekitd.keys
3633
let path = DocumentURI(for: .swift).pseudoPath
3734

Tests/SourceKitLSPTests/PullDiagnosticsTests.swift

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -310,15 +310,10 @@ final class PullDiagnosticsTests: XCTestCase {
310310
}
311311

312312
func testDontReturnEmptyDiagnosticsIfDiagnosticRequestIsCancelled() async throws {
313-
let diagnosticSourcekitdRequestDidStart = self.expectation(description: "diagnostic sourcekitd request did start")
314313
let diagnosticRequestCancelled = self.expectation(description: "diagnostic request cancelled")
315314
var serverOptions = SourceKitLSPServer.Options.testDefault
316-
serverOptions.sourcekitdTestHooks.sourcekitdRequestDidStart = { request in
317-
guard request.description.contains("source.request.diagnostics") else {
318-
return
319-
}
320-
diagnosticSourcekitdRequestDidStart.fulfill()
321-
self.wait(for: [diagnosticRequestCancelled], timeout: defaultTimeout)
315+
serverOptions.indexTestHooks.preparationTaskDidStart = { _ in
316+
await self.fulfillment(of: [diagnosticRequestCancelled], timeout: defaultTimeout)
322317
// Poll until the `CancelRequestNotification` has been propagated to the request handling.
323318
for _ in 0..<Int(defaultTimeout * 100) {
324319
if Task.isCancelled {
@@ -331,7 +326,9 @@ final class PullDiagnosticsTests: XCTestCase {
331326
files: [
332327
"Lib.swift": "let x: String = 1"
333328
],
334-
serverOptions: serverOptions
329+
serverOptions: serverOptions,
330+
enableBackgroundIndexing: true,
331+
pollIndex: false
335332
)
336333
let (uri, _) = try project.openDocument("Lib.swift")
337334

@@ -342,7 +339,6 @@ final class PullDiagnosticsTests: XCTestCase {
342339
XCTAssertEqual(result, .failure(ResponseError.cancelled))
343340
diagnosticResponseReceived.fulfill()
344341
}
345-
try await fulfillmentOfOrThrow([diagnosticSourcekitdRequestDidStart])
346342
project.testClient.send(CancelRequestNotification(id: requestID))
347343
diagnosticRequestCancelled.fulfill()
348344
try await fulfillmentOfOrThrow([diagnosticResponseReceived])

0 commit comments

Comments
 (0)