From e56c71f4b31fbbe0b62fe5c00362878ec3f1c6dc Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 31 May 2024 07:38:08 -0700 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20run=20a=20`swift=20build`=20com?= =?UTF-8?q?mand=20to=20prepare=20a=20package=20manifest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Package manifests don’t have an associated target to prepare and are represented by a `ConfiguredTarget` with an empty target ID. We were mistakingly running `swift build` with an empty target name to prepare them, which failed. There is nothing to prepare. --- .../SwiftPMBuildSystem.swift | 5 +++ .../TestSourceKitLSPClient.swift | 43 ++++++++++++++----- .../SemanticIndex/SemanticIndexManager.swift | 14 ++++-- .../SourceKitDTests/CrashRecoveryTests.swift | 4 +- .../BackgroundIndexingTests.swift | 23 ++++++++++ .../SourceKitLSPTests/BuildSystemTests.swift | 2 +- Tests/SourceKitLSPTests/LocalSwiftTests.swift | 2 +- .../MainFilesProviderTests.swift | 2 +- 8 files changed, 76 insertions(+), 19 deletions(-) diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift index 19bf8a197..c3a24b451 100644 --- a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift +++ b/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 { diff --git a/Sources/SKTestSupport/TestSourceKitLSPClient.swift b/Sources/SKTestSupport/TestSourceKitLSPClient.swift index 83915ad10..80d07ea51 100644 --- a/Sources/SKTestSupport/TestSourceKitLSPClient.swift +++ b/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( 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( + 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 diff --git a/Sources/SemanticIndex/SemanticIndexManager.swift b/Sources/SemanticIndex/SemanticIndexManager.swift index 50bebba9e..dc2b663c0 100644 --- a/Sources/SemanticIndex/SemanticIndexManager.swift +++ b/Sources/SemanticIndex/SemanticIndexManager.swift @@ -394,6 +394,12 @@ 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 @@ -401,6 +407,10 @@ public final actor SemanticIndexManager { 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") @@ -408,10 +418,6 @@ public final actor SemanticIndexManager { inProgressPrepareForEditorTask?.state = .preparingTarget } await self.prepare(targets: [target], priority: nil) - if inProgressPrepareForEditorTask?.id == id { - inProgressPrepareForEditorTask = nil - self.indexProgressStatusDidChange() - } } } inProgressPrepareForEditorTask?.task.cancel() diff --git a/Tests/SourceKitDTests/CrashRecoveryTests.swift b/Tests/SourceKitDTests/CrashRecoveryTests.swift index eba61bf47..339387b54 100644 --- a/Tests/SourceKitDTests/CrashRecoveryTests.swift +++ b/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())) diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift index ada1ee3fb..215a48c5b 100644 --- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift +++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift @@ -930,4 +930,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") } + ) + } } diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index eec906818..44c4031e9 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -193,7 +193,7 @@ final class BuildSystemTests: XCTestCase { var receivedCorrectDiagnostic = false for _ in 0..