Merge pull request #1700 from ahoppen/build-settings-timeout

Use fallback build settings if build system doesn’t provide build settings within a timeout

Author: ahoppen

Documentation/Configuration File.md

@@ -31,6 +31,7 @@ The structure of the file is currently not guaranteed to be stable. Options may
   - `cxxCompilerFlags: string[]`: Extra arguments passed to the compiler for C++ files
   - `swiftCompilerFlags: string[]`: Extra arguments passed to the compiler for Swift files
   - `sdk: string`: The SDK to use for fallback arguments. Default is to infer the SDK using `xcrun`.
+- `buildSettingsTimeout: int`: Number of milliseconds to wait for build settings from the build system before using fallback build settings.
 - `clangdOptions: string[]`: Extra command line arguments passed to `clangd` when launching it
 - `index`: Dictionary with the following keys, defining options related to indexing
     - `indexStorePath: string`: Directory in which a separate compilation stores the index store. By default, inferred from the build system.

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -137,6 +137,7 @@ private extension BuildSystemKind {
   private static func createBuiltInBuildSystemAdapter(
     projectRoot: AbsolutePath,
     messagesToSourceKitLSPHandler: any MessageHandler,
+    buildSystemTestHooks: BuildSystemTestHooks,
     _ createBuildSystem: @Sendable (_ connectionToSourceKitLSP: any Connection) async throws -> BuiltInBuildSystem?
   ) async -> BuildSystemAdapter? {
     let connectionToSourceKitLSP = LocalConnection(receiverName: "BuildSystemManager")
@@ -152,7 +153,8 @@ private extension BuildSystemKind {
     logger.log("Created \(type(of: buildSystem), privacy: .public) at \(projectRoot.pathString)")
     let buildSystemAdapter = BuiltInBuildSystemAdapter(
       underlyingBuildSystem: buildSystem,
-      connectionToSourceKitLSP: connectionToSourceKitLSP
+      connectionToSourceKitLSP: connectionToSourceKitLSP,
+      buildSystemTestHooks: buildSystemTestHooks
     )
     let connectionToBuildSystem = LocalConnection(receiverName: "Build system")
     connectionToBuildSystem.start(handler: buildSystemAdapter)
@@ -184,7 +186,8 @@ private extension BuildSystemKind {
     case .compilationDatabase(projectRoot: let projectRoot):
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
-        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler
+        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
+        buildSystemTestHooks: testHooks
       ) { connectionToSourceKitLSP in
         CompilationDatabaseBuildSystem(
           projectRoot: projectRoot,
@@ -197,7 +200,8 @@ private extension BuildSystemKind {
     case .swiftPM(projectRoot: let projectRoot):
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
-        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler
+        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
+        buildSystemTestHooks: testHooks
       ) { connectionToSourceKitLSP in
         try await SwiftPMBuildSystem(
           projectRoot: projectRoot,
@@ -210,7 +214,8 @@ private extension BuildSystemKind {
     case .testBuildSystem(projectRoot: let projectRoot):
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
-        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler
+        messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
+        buildSystemTestHooks: testHooks
       ) { connectionToSourceKitLSP in
         TestBuildSystem(projectRoot: projectRoot, connectionToSourceKitLSP: connectionToSourceKitLSP)
       }
@@ -399,7 +404,8 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
         )
         let adapter = BuiltInBuildSystemAdapter(
           underlyingBuildSystem: legacyBuildServer,
-          connectionToSourceKitLSP: legacyBuildServer.connectionToSourceKitLSP
+          connectionToSourceKitLSP: legacyBuildServer.connectionToSourceKitLSP,
+          buildSystemTestHooks: buildSystemTestHooks
         )
         let connectionToBuildSystem = LocalConnection(receiverName: "Legacy BSP server")
         connectionToBuildSystem.start(handler: adapter)
@@ -705,14 +711,17 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
     in target: BuildTargetIdentifier?,
     language: Language
   ) async -> FileBuildSettings? {
-    do {
-      if let target,
-        let buildSettings = try await buildSettingsFromBuildSystem(for: document, in: target, language: language)
-      {
-        return buildSettings
+    if let target {
+      let buildSettingsFromBuildSystem = await orLog("Getting build settings") {
+        try await withTimeout(options.buildSettingsTimeoutOrDefault) {
+          try await self.buildSettingsFromBuildSystem(for: document, in: target, language: language)
+        } resultReceivedAfterTimeout: {
+          await self.delegate?.fileBuildSettingsChanged([document])
+        }
+      }
+      if let buildSettingsFromBuildSystem {
+        return buildSettingsFromBuildSystem
       }
-    } catch {
-      logger.error("Getting build settings failed: \(error.forLogging)")
     }
 
     guard
@@ -746,8 +755,15 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       basedOn document: DocumentURI
     ) async -> (mainFile: DocumentURI, settings: FileBuildSettings)? {
       let mainFile = await self.mainFile(for: document, language: language)
-      let target = await canonicalTarget(for: mainFile)
-      guard let settings = await buildSettings(for: mainFile, in: target, language: language) else {
+      let settings = await orLog("Getting build settings") {
+        let target = try await withTimeout(options.buildSettingsTimeoutOrDefault) {
+          await self.canonicalTarget(for: mainFile)
+        } resultReceivedAfterTimeout: {
+          await self.delegate?.fileBuildSettingsChanged([document])
+        }
+        return await self.buildSettings(for: mainFile, in: target, language: language)
+      }
+      guard let settings else {
         return nil
       }
       return (mainFile, settings)

Sources/BuildSystemIntegration/BuildSystemTestHooks.swift

@@ -10,10 +10,21 @@
 //
 //===----------------------------------------------------------------------===//
 
+import LanguageServerProtocol
+
 package struct BuildSystemTestHooks: Sendable {
   package var swiftPMTestHooks: SwiftPMTestHooks
 
-  package init(swiftPMTestHooks: SwiftPMTestHooks = SwiftPMTestHooks()) {
+  /// A hook that will be executed before a request is handled by a `BuiltInBuildSystem`.
+  ///
+  /// This allows requests to be artificially delayed.
+  package var handleRequest: (@Sendable (any RequestType) async -> Void)?
+
+  package init(
+    swiftPMTestHooks: SwiftPMTestHooks = SwiftPMTestHooks(),
+    handleRequest: (@Sendable (any RequestType) async -> Void)? = nil
+  ) {
     self.swiftPMTestHooks = swiftPMTestHooks
+    self.handleRequest = handleRequest
   }
 }

Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift

@@ -51,6 +51,8 @@ actor BuiltInBuildSystemAdapter: QueueBasedMessageHandler {
   /// The connection with which messages are sent to `BuildSystemManager`.
   private let connectionToSourceKitLSP: LocalConnection
 
+  private let buildSystemTestHooks: BuildSystemTestHooks
+
   /// If the underlying build system is a `TestBuildSystem`, return it. Otherwise, `nil`
   ///
   /// - Important: For testing purposes only.
@@ -62,10 +64,12 @@ actor BuiltInBuildSystemAdapter: QueueBasedMessageHandler {
   /// from the build system to SourceKit-LSP.
   init(
     underlyingBuildSystem: BuiltInBuildSystem,
-    connectionToSourceKitLSP: LocalConnection
+    connectionToSourceKitLSP: LocalConnection,
+    buildSystemTestHooks: BuildSystemTestHooks
   ) {
     self.underlyingBuildSystem = underlyingBuildSystem
     self.connectionToSourceKitLSP = connectionToSourceKitLSP
+    self.buildSystemTestHooks = buildSystemTestHooks
   }
 
   deinit {
@@ -102,6 +106,7 @@ actor BuiltInBuildSystemAdapter: QueueBasedMessageHandler {
   }
 
   package func handleImpl<Request: RequestType>(_ request: RequestAndReply<Request>) async {
+    await buildSystemTestHooks.handleRequest?(request.params)
     switch request {
     case let request as RequestAndReply<BuildShutdownRequest>:
       await request.reply { VoidResponse() }

Sources/SKOptions/SourceKitLSPOptions.swift

@@ -241,6 +241,13 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable, CustomLogString
     set { fallbackBuildSystem = newValue }
   }
 
+  /// Number of milliseconds to wait for build settings from the build system before using fallback build settings.
+  public var buildSettingsTimeout: Int?
+  public var buildSettingsTimeoutOrDefault: Duration {
+    // The default timeout of 500ms was chosen arbitrarily without any measurements.
+    get { .milliseconds(buildSettingsTimeout ?? 500) }
+  }
+
   public var clangdOptions: [String]?
 
   private var index: IndexOptions?

Sources/SKTestSupport/WrappedSemaphore.swift

@@ -51,12 +51,16 @@ package struct WrappedSemaphore: Sendable {
   }
 
   /// Wait for a signal and emit an XCTFail if the semaphore is not signaled within `timeout`.
-  package func waitOrXCTFail(timeout: DispatchTime = DispatchTime.now() + .seconds(Int(defaultTimeout))) {
+  package func waitOrXCTFail(
+    timeout: DispatchTime = DispatchTime.now() + .seconds(Int(defaultTimeout)),
+    file: StaticString = #filePath,
+    line: UInt = #line
+  ) {
     switch self.wait(timeout: timeout) {
     case .success:
       break
     case .timedOut:
-      XCTFail("\(name) timed out")
+      XCTFail("\(name) timed out", file: file, line: line)
     }
   }
 }

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

@@ -989,6 +989,7 @@ extension SwiftLanguageService {
       )
       let snapshot = try await self.latestSnapshot(for: req.textDocument.uri)
       let buildSettings = await self.buildSettings(for: req.textDocument.uri)
+      try Task.checkCancellation()
       let diagnosticReport = try await self.diagnosticReportManager.diagnosticReport(
         for: snapshot,
         buildSettings: buildSettings

Sources/SwiftExtensions/AsyncUtils.swift

@@ -169,6 +169,8 @@ extension Collection where Element: Sendable {
 
 package struct TimeoutError: Error, CustomStringConvertible {
   package var description: String { "Timed out" }
+
+  package init() {}
 }
 
 /// Executes `body`. If it doesn't finish after `duration`, throws a `TimeoutError`.
@@ -184,10 +186,56 @@ package func withTimeout<T: Sendable>(
     taskGroup.addTask {
       return try await body()
     }
-    for try await value in taskGroup {
+    defer {
       taskGroup.cancelAll()
+    }
+    for try await value in taskGroup {
       return value
     }
     throw CancellationError()
   }
 }
+
+/// Executes `body`. If it doesn't finish after `duration`, return `nil` and continue running body. When `body` returns
+/// a value after the timeout, `resultReceivedAfterTimeout` is called.
+///
+/// - Important: `body` will not be cancelled when the timeout is received. Use the other overload of `withTimeout` if
+///   `body` should be cancelled after `timeout`.
+package func withTimeout<T: Sendable>(
+  _ timeout: Duration,
+  body: @escaping @Sendable () async throws -> T?,
+  resultReceivedAfterTimeout: @escaping @Sendable () async -> Void
+) async throws -> T? {
+  let didHitTimeout = AtomicBool(initialValue: false)
+
+  let stream = AsyncThrowingStream<T?, Error> { continuation in
+    Task {
+      try await Task.sleep(for: timeout)
+      didHitTimeout.value = true
+      continuation.yield(nil)
+    }
+
+    Task {
+      do {
+        let result = try await body()
+        if didHitTimeout.value {
+          await resultReceivedAfterTimeout()
+        }
+        continuation.yield(result)
+      } catch {
+        continuation.yield(with: .failure(error))
+      }
+    }
+  }
+
+  for try await value in stream {
+    return value
+  }
+  // The only reason for the loop above to terminate is if the Task got cancelled or if the continuation finishes
+  // (which it never does).
+  if Task.isCancelled {
+    throw CancellationError()
+  } else {
+    preconditionFailure("Continuation never finishes")
+  }
+}

Tests/BuildSystemIntegrationTests/FallbackBuildSettingsTests.swift

@@ -10,10 +10,12 @@
 //
 //===----------------------------------------------------------------------===//
 
+import BuildServerProtocol
 @_spi(Testing) import BuildSystemIntegration
 import LanguageServerProtocol
 import SKOptions
 import SKTestSupport
+import SourceKitLSP
 import TSCBasic
 import XCTest
 
@@ -160,4 +162,47 @@ final class FallbackBuildSystemTests: XCTestCase {
 
     XCTAssertNil(fallbackBuildSettings(for: source, language: Language(rawValue: "unknown"), options: .init()))
   }
+
+  func testFallbackBuildSettingsWhileBuildSystemIsComputingBuildSettings() async throws {
+    let fallbackResultsReceived = WrappedSemaphore(name: "Fallback results received")
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Test.swift": """
+        let x: 1️⃣String = 1
+        """
+      ],
+      testHooks: TestHooks(
+        buildSystemTestHooks: BuildSystemTestHooks(
+          handleRequest: { request in
+            if request is TextDocumentSourceKitOptionsRequest {
+              fallbackResultsReceived.waitOrXCTFail()
+            }
+          }
+        )
+      )
+    )
+
+    let (uri, positions) = try project.openDocument("Test.swift")
+
+    let diagsBeforeBuildSettings = try await project.testClient.send(
+      DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+    )
+    XCTAssertEqual(diagsBeforeBuildSettings.fullReport?.items, [])
+
+    let hoverBeforeBuildSettings = try await project.testClient.send(
+      HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
+    )
+    XCTAssertNotNil(hoverBeforeBuildSettings?.contents)
+
+    fallbackResultsReceived.signal()
+
+    try await repeatUntilExpectedResult {
+      let diagsAfterBuildSettings = try await project.testClient.send(
+        DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+      )
+      return diagsAfterBuildSettings.fullReport?.items.map(\.message) == [
+        "Cannot convert value of type 'Int' to specified type 'String'"
+      ]
+    }
+  }
 }