Merge pull request #1762 from ahoppen/build-settings-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

@@ -139,6 +139,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(
@@ -156,7 +157,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: "\(type(of: buildSystem)) for \(projectRoot.asURL.lastPathComponent)"
@@ -190,7 +192,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,
@@ -203,7 +206,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,
@@ -216,7 +220,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)
       }
@@ -411,7 +416,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)
@@ -672,7 +678,12 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   /// Returns the target's module name as parsed from the `BuildTargetIdentifier`'s compiler arguments.
   package func moduleName(for document: DocumentURI, in target: BuildTargetIdentifier) async -> String? {
     guard let language = await self.defaultLanguage(for: document, in: target),
-      let buildSettings = await buildSettings(for: document, in: target, language: language)
+      let buildSettings = await buildSettings(
+        for: document,
+        in: target,
+        language: language,
+        fallbackAfterTimeout: false
+      )
     else {
       return nil
     }
@@ -715,11 +726,6 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       language: language
     )
 
-    // TODO: We should only wait `fallbackSettingsTimeout` for build settings
-    // and return fallback afterwards.
-    // For now, this should be fine because all build systems return
-    // very quickly from `settings(for:language:)`.
-    // https://github.com/apple/sourcekit-lsp/issues/1181
     let response = try await cachedSourceKitOptions.get(request, isolation: self) { request in
       try await buildSystemAdapter.send(request)
     }
@@ -740,19 +746,30 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   /// Only call this method if it is known that `document` is a main file. Prefer `buildSettingsInferredFromMainFile`
   /// otherwise. If `document` is a header file, this will most likely return fallback settings because header files
   /// don't have build settings by themselves.
+  ///
+  /// If `fallbackAfterTimeout` is true fallback build settings will be returned if no build settings can be found in
+  /// `SourceKitLSPOptions.buildSettingsTimeoutOrDefault`.
   package func buildSettings(
     for document: DocumentURI,
     in target: BuildTargetIdentifier?,
-    language: Language
+    language: Language,
+    fallbackAfterTimeout: Bool
   ) 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") {
+        if fallbackAfterTimeout {
+          try await withTimeout(options.buildSettingsTimeoutOrDefault) {
+            return try await self.buildSettingsFromBuildSystem(for: document, in: target, language: language)
+          } resultReceivedAfterTimeout: {
+            await self.delegate?.fileBuildSettingsChanged([document])
+          }
+        } else {
+          try await self.buildSettingsFromBuildSystem(for: document, in: target, language: language)
+        }
+      }
+      if let buildSettingsFromBuildSystem {
+        return buildSettingsFromBuildSystem
       }
-    } catch {
-      logger.error("Getting build settings failed: \(error.forLogging)")
     }
 
     guard
@@ -780,14 +797,27 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   /// by the header file.
   package func buildSettingsInferredFromMainFile(
     for document: DocumentURI,
-    language: Language
+    language: Language,
+    fallbackAfterTimeout: Bool
   ) async -> FileBuildSettings? {
     func mainFileAndSettings(
       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,
+          fallbackAfterTimeout: fallbackAfterTimeout
+        )
+      }
+      guard let settings else {
         return nil
       }
       return (mainFile, settings)

Sources/BuildSystemIntegration/BuildSystemTestHooks.swift

@@ -10,10 +10,25 @@
 //
 //===----------------------------------------------------------------------===//
 
+#if compiler(>=6)
+package import LanguageServerProtocol
+#else
+import LanguageServerProtocol
+#endif
+
 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

@@ -59,6 +59,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.
@@ -70,10 +72,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 {
@@ -116,6 +120,7 @@ actor BuiltInBuildSystemAdapter: QueueBasedMessageHandler {
     reply: @Sendable @escaping (LSPResult<Request.Response>) -> Void
   ) async {
     let request = RequestAndReply(request, reply: reply)
+    await buildSystemTestHooks.handleRequest?(request.params)
     switch request {
     case let request as RequestAndReply<BuildShutdownRequest>:
       await request.reply { VoidResponse() }

Sources/BuildSystemIntegration/CMakeLists.txt

@@ -14,7 +14,6 @@ add_library(BuildSystemIntegration STATIC
   ExternalBuildSystemAdapter.swift
   FallbackBuildSettings.swift
   FileBuildSettings.swift
-  Language+InferredFromFileExtension.swift
   LegacyBuildServerBuildSystem.swift
   MainFilesProvider.swift
   PathPrefixMapping.swift

Sources/SKOptions/SourceKitLSPOptions.swift

@@ -250,6 +250,13 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     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/SKSupport/CMakeLists.txt

@@ -8,6 +8,7 @@ add_library(SKSupport STATIC
   DocumentURI+CustomLogStringConvertible.swift
   DocumentURI+symlinkTarget.swift
   FileSystem.swift
+  Language+InferredFromFileExtension.swift
   LineTable.swift
   LocalConnection.swift
   Process+Run.swift

Sources/BuildSystemIntegration/Language+InferredFromFileExtension.swift renamed to Sources/SKSupport/Language+InferredFromFileExtension.swift

@@ -10,11 +10,16 @@
 //
 //===----------------------------------------------------------------------===//
 
+#if compiler(>=6)
+import Foundation
+package import LanguageServerProtocol
+#else
 import Foundation
 import LanguageServerProtocol
+#endif
 
 extension Language {
-  init?(inferredFromFileExtension uri: DocumentURI) {
+  package init?(inferredFromFileExtension uri: DocumentURI) {
     // URL.pathExtension is only set for file URLs but we want to also infer a file extension for non-file URLs like
     // untitled:file.cpp
     let pathExtension = uri.fileURL?.pathExtension ?? (uri.pseudoPath as NSString).pathExtension

Sources/SKTestSupport/WrappedSemaphore.swift

@@ -56,12 +56,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/SemanticIndex/UpdateIndexStoreTaskDescription.swift

@@ -254,7 +254,12 @@ package struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
       logger.error("Not indexing \(file.forLogging) because its language could not be determined")
       return
     }
-    let buildSettings = await buildSystemManager.buildSettings(for: file.mainFile, in: target, language: language)
+    let buildSettings = await buildSystemManager.buildSettings(
+      for: file.mainFile,
+      in: target,
+      language: language,
+      fallbackAfterTimeout: false
+    )
     guard let buildSettings else {
       logger.error("Not indexing \(file.forLogging) because it has no compiler arguments")
       return