Merge pull request #1723 from ahoppen/revert-build-settings-timeout

Author: ahoppen

Documentation/Configuration File.md

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

Sources/BuildSystemIntegration/BuildSystemTestHooks.swift

@@ -10,21 +10,10 @@
 //
 //===----------------------------------------------------------------------===//
 
-import LanguageServerProtocol
-
 package struct BuildSystemTestHooks: Sendable {
   package var 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
-  ) {
+  package init(swiftPMTestHooks: SwiftPMTestHooks = SwiftPMTestHooks()) {
     self.swiftPMTestHooks = swiftPMTestHooks
-    self.handleRequest = handleRequest
   }
 }

Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift

@@ -51,8 +51,6 @@ 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.
@@ -64,12 +62,10 @@ actor BuiltInBuildSystemAdapter: QueueBasedMessageHandler {
   /// from the build system to SourceKit-LSP.
   init(
     underlyingBuildSystem: BuiltInBuildSystem,
-    connectionToSourceKitLSP: LocalConnection,
-    buildSystemTestHooks: BuildSystemTestHooks
+    connectionToSourceKitLSP: LocalConnection
   ) {
     self.underlyingBuildSystem = underlyingBuildSystem
     self.connectionToSourceKitLSP = connectionToSourceKitLSP
-    self.buildSystemTestHooks = buildSystemTestHooks
   }
 
   deinit {
@@ -106,7 +102,6 @@ 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,13 +241,6 @@ 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,16 +51,12 @@ 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)),
-    file: StaticString = #filePath,
-    line: UInt = #line
-  ) {
+  package func waitOrXCTFail(timeout: DispatchTime = DispatchTime.now() + .seconds(Int(defaultTimeout))) {
     switch self.wait(timeout: timeout) {
     case .success:
       break
     case .timedOut:
-      XCTFail("\(name) timed out", file: file, line: line)
+      XCTFail("\(name) timed out")
     }
   }
 }

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

@@ -989,7 +989,6 @@ 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,8 +169,6 @@ 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`.
@@ -186,56 +184,10 @@ package func withTimeout<T: Sendable>(
     taskGroup.addTask {
       return try await body()
     }
-    defer {
-      taskGroup.cancelAll()
-    }
     for try await value in taskGroup {
+      taskGroup.cancelAll()
       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,12 +10,10 @@
 //
 //===----------------------------------------------------------------------===//
 
-import BuildServerProtocol
 @_spi(Testing) import BuildSystemIntegration
 import LanguageServerProtocol
 import SKOptions
 import SKTestSupport
-import SourceKitLSP
 import TSCBasic
 import XCTest
 
@@ -162,47 +160,4 @@ 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'"
-      ]
-    }
-  }
 }