Merge pull request #1380 from ahoppen/dont-prepare-pacakge-manifest

Don’t run a `swift build` command to prepare a package manifest

Author: ahoppen

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -550,6 +550,11 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
     singleTarget target: ConfiguredTarget,
     indexProcessDidProduceResult: @Sendable (IndexProcessResult) -> Void
   ) async throws {
+    if target == .forPackageManifest {
+      // Nothing to prepare for package manifests.
+      return
+    }
+
     // TODO (indexing): Add a proper 'prepare' job in SwiftPM instead of building the target.
     // https://github.com/apple/sourcekit-lsp/issues/1254
     guard let toolchain = await toolchainRegistry.default else {

Sources/SKTestSupport/TestSourceKitLSPClient.swift

@@ -27,6 +27,10 @@ extension SourceKitLSPServer.Options {
   public static let testDefault = Self(swiftPublishDiagnosticsDebounceDuration: 0)
 }
 
+fileprivate struct NotificationTimeoutError: Error, CustomStringConvertible {
+  var description: String = "Failed to receive next notification within timeout"
+}
+
 /// A mock SourceKit-LSP client (aka. a mock editor) that behaves like an editor
 /// for testing purposes.
 ///
@@ -229,21 +233,17 @@ public final class TestSourceKitLSPClient: MessageHandler {
   ///
   /// - Note: This also returns any notifications sent before the call to
   ///   `nextNotification`.
-  public func nextNotification(timeout: TimeInterval = defaultTimeout) async throws -> any NotificationType {
-    struct TimeoutError: Error, CustomStringConvertible {
-      var description: String = "Failed to receive next notification within timeout"
-    }
-
+  public func nextNotification(timeout: Duration = .seconds(defaultTimeout)) async throws -> any NotificationType {
     return try await withThrowingTaskGroup(of: (any NotificationType).self) { taskGroup in
       taskGroup.addTask {
         for await notification in self.notifications {
           return notification
         }
-        throw TimeoutError()
+        throw NotificationTimeoutError()
       }
       taskGroup.addTask {
-        try await Task.sleep(nanoseconds: UInt64(timeout * 1_000_000_000))
-        throw TimeoutError()
+        try await Task.sleep(for: timeout)
+        throw NotificationTimeoutError()
       }
       let result = try await taskGroup.next()!
       taskGroup.cancelAll()
@@ -256,7 +256,7 @@ public final class TestSourceKitLSPClient: MessageHandler {
   /// If the next notification is not a `PublishDiagnosticsNotification`, this
   /// methods throws.
   public func nextDiagnosticsNotification(
-    timeout: TimeInterval = defaultTimeout
+    timeout: Duration = .seconds(defaultTimeout)
   ) async throws -> PublishDiagnosticsNotification {
     guard !usePullDiagnostics else {
       struct PushDiagnosticsError: Error, CustomStringConvertible {
@@ -272,7 +272,7 @@ public final class TestSourceKitLSPClient: MessageHandler {
   public func nextNotification<ExpectedNotificationType: NotificationType>(
     ofType: ExpectedNotificationType.Type,
     satisfying predicate: (ExpectedNotificationType) -> Bool = { _ in true },
-    timeout: TimeInterval = defaultTimeout
+    timeout: Duration = .seconds(defaultTimeout)
   ) async throws -> ExpectedNotificationType {
     while true {
       let nextNotification = try await nextNotification(timeout: timeout)
@@ -282,6 +282,29 @@ public final class TestSourceKitLSPClient: MessageHandler {
     }
   }
 
+  /// Asserts that the test client does not receive a notification of the given type and satisfying the given predicate
+  /// within the given duration.
+  ///
+  /// For stable tests, the code that triggered the notification should be run before this assertion instead of relying
+  /// on the duration.
+  ///
+  /// The duration should not be 0 because we need to allow `nextNotification` some time to get the notification out of
+  /// the `notifications` `AsyncStream`.
+  public func assertDoesNotReceiveNotification<ExpectedNotificationType: NotificationType>(
+    ofType: ExpectedNotificationType.Type,
+    satisfying predicate: (ExpectedNotificationType) -> Bool = { _ in true },
+    within duration: Duration = .seconds(0.2)
+  ) async throws {
+    do {
+      let notification = try await nextNotification(
+        ofType: ExpectedNotificationType.self,
+        satisfying: predicate,
+        timeout: duration
+      )
+      XCTFail("Did not expect to receive notification but received \(notification)")
+    } catch is NotificationTimeoutError {}
+  }
+
   /// Handle the next request of the given type that is sent to the client.
   ///
   /// The request handler will only handle a single request. If the request is called again, the request handler won't

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -394,24 +394,30 @@ public final actor SemanticIndexManager {
     let id = UUID()
     let task = Task(priority: priority) {
       await withLoggingScope("preparation") {
+        defer {
+          if inProgressPrepareForEditorTask?.id == id {
+            inProgressPrepareForEditorTask = nil
+            self.indexProgressStatusDidChange()
+          }
+        }
         // Should be kept in sync with `prepareFileForEditorFunctionality`
         guard let target = await buildSystemManager.canonicalConfiguredTarget(for: uri) else {
           return
         }
         if Task.isCancelled {
           return
         }
+        if await preparationUpToDateTracker.isUpToDate(target) {
+          // If the target is up-to-date, there is nothing to prepare.
+          return
+        }
         if inProgressPrepareForEditorTask?.id == id {
           if inProgressPrepareForEditorTask?.state != .determiningCanonicalConfiguredTarget {
             logger.fault("inProgressPrepareForEditorTask is in unexpected state")
           }
           inProgressPrepareForEditorTask?.state = .preparingTarget
         }
         await self.prepare(targets: [target], priority: nil)
-        if inProgressPrepareForEditorTask?.id == id {
-          inProgressPrepareForEditorTask = nil
-          self.indexProgressStatusDidChange()
-        }
       }
     }
     inProgressPrepareForEditorTask?.task.cancel()

Tests/SourceKitDTests/CrashRecoveryTests.swift

@@ -90,7 +90,7 @@ final class CrashRecoveryTests: XCTestCase {
 
     await swiftLanguageService._crash()
 
-    let crashedNotification = try await testClient.nextNotification(ofType: WorkDoneProgress.self, timeout: 5)
+    let crashedNotification = try await testClient.nextNotification(ofType: WorkDoneProgress.self, timeout: .seconds(5))
     XCTAssertEqual(
       crashedNotification.value,
       .begin(
@@ -107,7 +107,7 @@ final class CrashRecoveryTests: XCTestCase {
     _ = try? await testClient.send(hoverRequest)
     let semanticFunctionalityRestoredNotification = try await testClient.nextNotification(
       ofType: WorkDoneProgress.self,
-      timeout: 30
+      timeout: .seconds(30)
     )
     XCTAssertEqual(semanticFunctionalityRestoredNotification.value, .end(WorkDoneProgressEnd()))
 

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -931,4 +931,27 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual((result?.changes?.keys).map(Set.init), [uri, try project.uri(for: "Client.swift")])
   }
+
+  func testDontPreparePackageManifest() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Lib.swift": ""
+      ],
+      enableBackgroundIndexing: true
+    )
+
+    _ = try await project.testClient.nextNotification(
+      ofType: LogMessageNotification.self,
+      satisfying: { $0.message.contains("Preparing MyLibrary") }
+    )
+
+    // Opening the package manifest shouldn't cause any `swift build` calls to prepare them because they are not part of
+    // a target that can be prepared.
+    let (uri, _) = try project.openDocument("Package.swift")
+    _ = try await project.testClient.send(DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri)))
+    try await project.testClient.assertDoesNotReceiveNotification(
+      ofType: LogMessageNotification.self,
+      satisfying: { $0.message.contains("Preparing") }
+    )
+  }
 }

Tests/SourceKitLSPTests/BuildSystemTests.swift

@@ -193,7 +193,7 @@ final class BuildSystemTests: XCTestCase {
 
     var receivedCorrectDiagnostic = false
     for _ in 0..<Int(defaultTimeout) {
-      let refreshedDiags = try await testClient.nextDiagnosticsNotification(timeout: 1)
+      let refreshedDiags = try await testClient.nextDiagnosticsNotification(timeout: .seconds(1))
       if refreshedDiags.diagnostics.count == 0, try text == documentManager.latestSnapshot(doc).text {
         receivedCorrectDiagnostic = true
         break

Tests/SourceKitLSPTests/LocalSwiftTests.swift

@@ -1427,6 +1427,6 @@ final class LocalSwiftTests: XCTestCase {
     XCTAssertEqual(diag.message, "Cannot find 'bar' in scope")
 
     // Ensure that we don't get a second `PublishDiagnosticsNotification`
-    await assertThrowsError(try await testClient.nextDiagnosticsNotification(timeout: 2))
+    await assertThrowsError(try await testClient.nextDiagnosticsNotification(timeout: .seconds(2)))
   }
 }

Tests/SourceKitLSPTests/MainFilesProviderTests.swift

@@ -219,7 +219,7 @@ final class MainFilesProviderTests: XCTestCase {
     // diagnostics.
     var receivedCorrectDiagnostic = false
     for _ in 0..<Int(defaultTimeout) {
-      let refreshedDiags = try await project.testClient.nextDiagnosticsNotification(timeout: 1)
+      let refreshedDiags = try await project.testClient.nextDiagnosticsNotification(timeout: .seconds(1))
       if let diagnostic = refreshedDiags.diagnostics.only,
         diagnostic.message == "Unused variable 'fromMyFancyLibrary'"
       {