diff --git a/Example/.bazelrc b/Example/.bazelrc index e6771f06..f89cfcf8 100644 --- a/Example/.bazelrc +++ b/Example/.bazelrc @@ -31,4 +31,4 @@ build --spawn_strategy=remote,worker,local # Only for BSP builds common:index_build --experimental_convenience_symlinks=ignore common:index_build --bes_backend= --bes_results_url= -common:index_build --show_result=0 \ No newline at end of file +common:index_build --show_result=0 --noshow_loading_progress --noshow_progress \ No newline at end of file diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BazelTargetStore.swift b/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BazelTargetStore.swift index 6739fdc8..c2cbc36b 100644 --- a/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BazelTargetStore.swift +++ b/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BazelTargetStore.swift @@ -30,6 +30,7 @@ private let logger = makeFileLevelBSPLogger() // the project's dependency graph and its files. protocol BazelTargetStore: AnyObject { var stateLock: OSAllocatedUnfairLock { get } + var isInitialized: Bool { get } func fetchTargets() throws -> [BuildTarget] func bazelTargetLabel(forBSPURI uri: URI) throws -> String func bazelTargetSrcs(forBSPURI uri: URI) throws -> [URI] @@ -100,6 +101,11 @@ final class BazelTargetStoreImpl: BazelTargetStore, @unchecked Sendable { self.bazelTargetQuerier = bazelTargetQuerier } + /// Returns true if the store has actually processed something. + var isInitialized: Bool { + return cachedTargets != nil + } + /// Converts a BSP BuildTarget URI to its underlying Bazel target label. func bazelTargetLabel(forBSPURI uri: URI) throws -> String { guard let label = cqueryResult?.bspURIsToBazelLabelsMap[uri] else { diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BuildTargetsHandler.swift b/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BuildTargetsHandler.swift index d91d9e7e..cd2f8a31 100644 --- a/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BuildTargetsHandler.swift +++ b/Sources/SourceKitBazelBSP/RequestHandlers/BuildTargets/BuildTargetsHandler.swift @@ -46,28 +46,41 @@ final class BuildTargetsHandler { let taskId = TaskId(id: "buildTargets-\(id.description)") connection?.startWorkTask(id: taskId, title: "sourcekit-bazel-bsp: Processing the build graph...") do { - nonisolated(unsafe) var shouldDispatchNotification = false + nonisolated(unsafe) var shouldReplyEmpty = false let result = try targetStore.stateLock.withLockUnchecked { - shouldDispatchNotification = isFirstTime + shouldReplyEmpty = isFirstTime isFirstTime = false - return try targetStore.fetchTargets() + // If this is the first time we're responding to buildTargets, sourcekit-lsp will expect us to return an empty list + // and then later send a notification containing the changes for performance reasons. + // See https://github.com/spotify/sourcekit-bazel-bsp/issues/102 + if shouldReplyEmpty { + reply(.success(WorkspaceBuildTargetsResponse(targets: []))) + } + let result = try targetStore.fetchTargets() + logger.debug("Found \(result.count, privacy: .public) targets") + logger.logFullObjectInMultipleLogMessages( + level: .debug, + header: "Target list", + result.map { $0.id.uri.stringValue }.joined(separator: ", "), + ) + return result } - logger.debug("Found \(result.count, privacy: .public) targets") - logger.logFullObjectInMultipleLogMessages( - level: .debug, - header: "Target list", - result.map { $0.id.uri.stringValue }.joined(separator: ", "), - ) connection?.finishTask(id: taskId, status: .ok) - reply(.success(WorkspaceBuildTargetsResponse(targets: result))) - // If this is the first time we're responding to buildTargets, send an empty notification. - // This triggers sourcekit-lsp to calculate the file mappings which enables jump-to-definition to work. - // We only need to do this because we're replying to this request incorrectly. - // We should also be able to drop this if we figure out how to make the actual LSP --index-prefix-map flag work. - // See https://github.com/spotify/sourcekit-bazel-bsp/issues/102 - if shouldDispatchNotification { - let notification = OnBuildTargetDidChangeNotification(changes: []) + if shouldReplyEmpty { + let targetEvents = result.map { target in + BuildTargetEvent( + target: target.id, + kind: .created, + dataKind: nil, + data: nil + ) + } + let notification = OnBuildTargetDidChangeNotification( + changes: targetEvents + ) connection?.send(notification) + } else { + reply(.success(WorkspaceBuildTargetsResponse(targets: result))) } } catch { connection?.finishTask(id: taskId, status: .error) diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift b/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift index b5cd447d..7deeaa6d 100644 --- a/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift +++ b/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift @@ -78,7 +78,13 @@ final class WatchedFileChangeHandler { // In this case, we keep the lock until the very end of the notification to avoid race conditions // with how the LSP follows up with this by calling waitForBuildSystemUpdates and buildTargets again. // Also because we need the targetStore at multiple points of this function. - let invalidatedTargets = targetStore.stateLock.withLockUnchecked { + let invalidatedTargets: [InvalidatedTarget] = targetStore.stateLock.withLockUnchecked { + + // If we received this notification before the build graph was calculated, we should stop. + guard targetStore.isInitialized else { + logger.info("Received file changes before the build graph was calculated. Ignoring.") + return [] + } logger.info("Received \(changes.count, privacy: .public) file changes") diff --git a/Tests/SourceKitBazelBSPTests/Fakes/BazelTargetStoreFake.swift b/Tests/SourceKitBazelBSPTests/Fakes/BazelTargetStoreFake.swift index 9342a8ec..7eb9b538 100644 --- a/Tests/SourceKitBazelBSPTests/Fakes/BazelTargetStoreFake.swift +++ b/Tests/SourceKitBazelBSPTests/Fakes/BazelTargetStoreFake.swift @@ -32,6 +32,10 @@ final class BazelTargetStoreFake: BazelTargetStore { var fetchTargetsError: Error? var mockSrcToBspURIs: [DocumentURI: [DocumentURI]] = [:] + var isInitialized: Bool { + return fetchTargetsCalled || !mockSrcToBspURIs.isEmpty + } + func fetchTargets() throws -> [BuildTarget] { fetchTargetsCalled = true if let error = fetchTargetsError { diff --git a/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift b/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift index a4632a23..c66a5257 100644 --- a/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift +++ b/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift @@ -27,8 +27,6 @@ import Testing @Suite struct WatchedFileChangeHandlerTests { - // MARK: - Helper Methods - private func createHandler() -> ( handler: WatchedFileChangeHandler, targetStore: BazelTargetStoreFake, @@ -50,18 +48,13 @@ struct WatchedFileChangeHandlerTests { try DocumentURI(string: path) } - // MARK: - Tests - @Test - func deletedFiles() throws { - // Arrange + func canHandleDeletedFiles() throws { let (handler, targetStore, connection, observer) = createHandler() let fileURI = try makeURI("file:///path/to/project/Sources/MyFile.swift") let targetURI1 = try makeURI("build://target1") let targetURI2 = try makeURI("build://target2") - - // Set up the target store to return targets for the deleted file targetStore.mockSrcToBspURIs[fileURI] = [targetURI1, targetURI2] let notification = OnWatchedFilesDidChangeNotification( @@ -70,14 +63,10 @@ struct WatchedFileChangeHandlerTests { ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - #expect(targetStore.clearCacheCalled, "clearCache should be called for deleted files") - #expect(targetStore.fetchTargetsCalled, "fetchTargets should be called for deleted files") - - // Check that the observer was notified + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) #expect(observer.invalidateCalled) #expect(observer.invalidatedTargets.count == 2) #expect( @@ -87,33 +76,22 @@ struct WatchedFileChangeHandlerTests { observer.invalidatedTargets.contains(InvalidatedTarget(uri: targetURI2, fileUri: fileURI, kind: .deleted)) ) - // Check that the LSP connection received the notification #expect(connection.sentNotifications.count == 1) - if let sentNotification = connection.sentNotifications.first as? OnBuildTargetDidChangeNotification { - #expect(sentNotification.changes?.count == 2) - let expectedTargetURIs = Set([targetURI1, targetURI2]) - if let changes = sentNotification.changes { - let actualTargetURIs = Set(changes.map { $0.target.uri }) - #expect(actualTargetURIs == expectedTargetURIs) - - for change in changes { - #expect(change.kind == .changed) - } - } - } else { - Issue.record("Expected OnBuildTargetDidChangeNotification") - } + let sentNotification = try #require( + connection.sentNotifications.first as? OnBuildTargetDidChangeNotification + ) + let changes = try #require(sentNotification.changes) + #expect(changes.count == 2) + #expect(Set(changes.map { $0.target.uri }) == Set([targetURI1, targetURI2])) + #expect(changes.allSatisfy { $0.kind == .changed }) } @Test - func createdFiles() throws { - // Arrange + func canHandleCreatedFiles() throws { let (handler, targetStore, connection, observer) = createHandler() let fileURI = try makeURI("file:///path/to/project/Sources/NewFile.swift") let targetURI = try makeURI("build://newTarget") - - // Set up the target store to return a target for the created file after fetchTargets is called targetStore.mockSrcToBspURIs[fileURI] = [targetURI] let notification = OnWatchedFilesDidChangeNotification( @@ -122,40 +100,32 @@ struct WatchedFileChangeHandlerTests { ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - #expect(targetStore.clearCacheCalled, "clearCache should be called for created files") - #expect(targetStore.fetchTargetsCalled, "fetchTargets should be called for created files") - - // Check that the observer was notified + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) #expect(observer.invalidateCalled) #expect(observer.invalidatedTargets.count == 1) #expect( observer.invalidatedTargets.contains(InvalidatedTarget(uri: targetURI, fileUri: fileURI, kind: .created)) ) - // Check that the LSP connection received the notification #expect(connection.sentNotifications.count == 1) - if let sentNotification = connection.sentNotifications.first as? OnBuildTargetDidChangeNotification { - #expect(sentNotification.changes?.count == 1) - #expect(sentNotification.changes?[0].target.uri == targetURI) - #expect(sentNotification.changes?[0].kind == .changed) - } else { - Issue.record("Expected OnBuildTargetDidChangeNotification") - } + let sentNotification = try #require( + connection.sentNotifications.first as? OnBuildTargetDidChangeNotification + ) + let changes = try #require(sentNotification.changes) + #expect(changes.count == 1) + #expect(changes[0].target.uri == targetURI) + #expect(changes[0].kind == .changed) } @Test - func updatedFiles() throws { - // Arrange + func updatedFilesAreIgnored() throws { let (handler, targetStore, connection, observer) = createHandler() let fileURI = try makeURI("file:///path/to/project/Sources/UpdatedFile.swift") let targetURI = try makeURI("build://updatedTarget") - - // Set up the target store to return a target for the updated file targetStore.mockSrcToBspURIs[fileURI] = [targetURI] let notification = OnWatchedFilesDidChangeNotification( @@ -164,24 +134,17 @@ struct WatchedFileChangeHandlerTests { ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - #expect(!targetStore.clearCacheCalled, "clearCache should not be called for changed files") - #expect(!targetStore.fetchTargetsCalled, "fetchTargets should not be called for changed files") - - // Check that the observer was NOT notified (we currently ignore such cases) - #expect(observer.invalidateCalled == false) - #expect(observer.invalidatedTargets.count == 0) - - // Check that the LSP connection did NOT receive a notification - #expect(connection.sentNotifications.count == 0) + #expect(!targetStore.clearCacheCalled) + #expect(!targetStore.fetchTargetsCalled) + #expect(!observer.invalidateCalled) + #expect(observer.invalidatedTargets.isEmpty) + #expect(connection.sentNotifications.isEmpty) } @Test func mixedFileChanges() throws { - // Arrange - test with multiple file changes of different types in one notification let (handler, targetStore, connection, observer) = createHandler() let deletedFileURI = try makeURI("file:///path/to/project/Sources/DeletedFile.swift") @@ -204,14 +167,10 @@ struct WatchedFileChangeHandlerTests { ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - #expect(targetStore.clearCacheCalled, "clearCache should be called when there are created or deleted files") - #expect(targetStore.fetchTargetsCalled, "fetchTargets should be called when there are created or deleted files") - - // Check that the observer was notified with all affected targets + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) #expect(observer.invalidateCalled) #expect(observer.invalidatedTargets.count == 2) #expect( @@ -225,111 +184,89 @@ struct WatchedFileChangeHandlerTests { ) ) - // Check that the LSP connection received the notification with all changes #expect(connection.sentNotifications.count == 1) - if let sentNotification = connection.sentNotifications.first as? OnBuildTargetDidChangeNotification { - #expect(sentNotification.changes?.count == 2) - - if let notificationChanges = sentNotification.changes { - let changes = notificationChanges.map { (uri: $0.target.uri, kind: $0.kind) } - #expect(changes.contains { $0.uri == deletedTargetURI && $0.kind == .changed }) - #expect(changes.contains { $0.uri == createdTargetURI && $0.kind == .changed }) - } - } else { - Issue.record("Expected OnBuildTargetDidChangeNotification") - } + let sentNotification = try #require( + connection.sentNotifications.first as? OnBuildTargetDidChangeNotification + ) + let changes = try #require(sentNotification.changes) + #expect(changes.count == 2) + #expect(changes.contains { $0.target.uri == deletedTargetURI && $0.kind == .changed }) + #expect(changes.contains { $0.target.uri == createdTargetURI && $0.kind == .changed }) } @Test - func fileWithNoAssociatedTargets() throws { - // Arrange - test handling of files that don't belong to any target - let (handler, _, connection, observer) = createHandler() + func createdFileWithNoAssociatedTargets() throws { + let (handler, targetStore, connection, observer) = createHandler() - let fileURI = try makeURI("file:///path/to/project/Sources/OrphanFile.swift") - // Don't set up any targets for this file - bspURIs will throw an error + let otherFileURI = try makeURI("file:///path/to/project/Sources/OtherFile.swift") + let otherTargetURI = try makeURI("build://otherTarget") + targetStore.mockSrcToBspURIs[otherFileURI] = [otherTargetURI] + let orphanFileURI = try makeURI("file:///path/to/project/Sources/OrphanFile.swift") let notification = OnWatchedFilesDidChangeNotification( changes: [ - FileEvent(uri: fileURI, type: .created) + FileEvent(uri: orphanFileURI, type: .created) ] ) - // Act + #expect(targetStore.isInitialized) handler.onWatchedFilesDidChange(notification) - // Assert - should handle gracefully without targets - // The observer is still called but with an empty set when files have no targets - #expect(observer.invalidateCalled == false, "Observer shouldn't be called if there are no changes") - #expect(observer.invalidatedTargets.isEmpty, "No targets should be invalidated for orphan files") - - #expect(connection.sentNotifications.count == 0) + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) + #expect(!observer.invalidateCalled) + #expect(observer.invalidatedTargets.isEmpty) + #expect(connection.sentNotifications.isEmpty) } @Test - func errorHandlingDuringFetchTargets() throws { - // Arrange - test that errors during fetchTargets are handled gracefully + func earlyExitsWhenTargetStoreIsNotInitialized() throws { let (handler, targetStore, connection, observer) = createHandler() let fileURI = try makeURI("file:///path/to/project/Sources/NewFile.swift") - let targetURI = try makeURI("build://newTarget") - - targetStore.mockSrcToBspURIs[fileURI] = [targetURI] - targetStore.fetchTargetsError = InvalidatedTargetObserverFake.TestError.intentional - let notification = OnWatchedFilesDidChangeNotification( changes: [ FileEvent(uri: fileURI, type: .created) ] ) - // Act + #expect(!targetStore.isInitialized) handler.onWatchedFilesDidChange(notification) - // Assert - the handler should continue processing despite the error - #expect(targetStore.clearCacheCalled) - #expect(targetStore.fetchTargetsCalled) - - // The observer should still be notified - #expect(observer.invalidateCalled) - #expect(observer.invalidatedTargets.count == 1) - - // The LSP connection should still receive a notification - #expect(connection.sentNotifications.count == 1) + #expect(!observer.invalidateCalled) + #expect(connection.sentNotifications.isEmpty) } @Test - func errorHandlingDuringObserverInvalidation() throws { - // Arrange - test that errors from observers don't stop processing + func sendsNotificationDespiteObserverErrors() throws { let (handler, targetStore, connection, observer) = createHandler() - let fileURI = try makeURI("file:///path/to/project/Sources/File.swift") - let targetURI = try makeURI("build://target") - + let fileURI = try makeURI("file:///path/to/project/Sources/NewFile.swift") + let targetURI = try makeURI("build://newTarget") targetStore.mockSrcToBspURIs[fileURI] = [targetURI] observer.shouldThrowOnInvalidate = true let notification = OnWatchedFilesDidChangeNotification( changes: [ - FileEvent(uri: fileURI, type: .changed) + FileEvent(uri: fileURI, type: .created) ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - despite the observer error, the LSP connection should still receive notification - #expect(observer.invalidateCalled == false) - #expect(connection.sentNotifications.count == 0) + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) + #expect(observer.invalidateCalled) + #expect(connection.sentNotifications.count == 1) } @Test - func fileWithValidExtensions() throws { - // Arrange - test with multiple file changes of different types in one notification + func handlesMultipleValidFileExtensions() throws { let (handler, targetStore, connection, observer) = createHandler() - let swiftFileURI = try makeURI("file:///path/to/project/Sources/ChangedFile.swift") - let objcFileURI = try makeURI("file:///path/to/project/Sources/ChangedFile.m") - let objcppFileURL = try makeURI("file:///path/to/project/Sources/ChangedFile.mm") + let swiftFileURI = try makeURI("file:///path/to/project/Sources/File.swift") + let objcFileURI = try makeURI("file:///path/to/project/Sources/File.m") + let objcppFileURI = try makeURI("file:///path/to/project/Sources/File.mm") let swiftTarget = try makeURI("build://swiftTarget") let objcTarget = try makeURI("build://objcTarget") @@ -337,24 +274,20 @@ struct WatchedFileChangeHandlerTests { targetStore.mockSrcToBspURIs[swiftFileURI] = [swiftTarget] targetStore.mockSrcToBspURIs[objcFileURI] = [objcTarget] - targetStore.mockSrcToBspURIs[objcppFileURL] = [objcppTarget] + targetStore.mockSrcToBspURIs[objcppFileURI] = [objcppTarget] let notification = OnWatchedFilesDidChangeNotification( changes: [ FileEvent(uri: swiftFileURI, type: .created), FileEvent(uri: objcFileURI, type: .created), - FileEvent(uri: objcppFileURL, type: .created), + FileEvent(uri: objcppFileURI, type: .created), ] ) - // Act handler.onWatchedFilesDidChange(notification) - // Assert - #expect(targetStore.clearCacheCalled, "clearCache should be called when there are created or deleted files") - #expect(targetStore.fetchTargetsCalled, "fetchTargets should be called when there are created or deleted files") - - // Check that the observer was notified with all affected targets + #expect(targetStore.clearCacheCalled) + #expect(targetStore.fetchTargetsCalled) #expect(observer.invalidateCalled) #expect(observer.invalidatedTargets.count == 3) #expect( @@ -369,23 +302,18 @@ struct WatchedFileChangeHandlerTests { ) #expect( observer.invalidatedTargets.contains( - InvalidatedTarget(uri: objcppTarget, fileUri: objcppFileURL, kind: .created) + InvalidatedTarget(uri: objcppTarget, fileUri: objcppFileURI, kind: .created) ) ) - // Check that the LSP connection received the notification with all changes #expect(connection.sentNotifications.count == 1) - if let sentNotification = connection.sentNotifications.first as? OnBuildTargetDidChangeNotification { - #expect(sentNotification.changes?.count == 3) - - if let notificationChanges = sentNotification.changes { - let changes = notificationChanges.map { (uri: $0.target.uri, kind: $0.kind) } - #expect(changes.contains { $0.uri == swiftTarget && $0.kind == .changed }) - #expect(changes.contains { $0.uri == objcTarget && $0.kind == .changed }) - #expect(changes.contains { $0.uri == objcppTarget && $0.kind == .changed }) - } - } else { - Issue.record("Expected OnBuildTargetDidChangeNotification") - } + let sentNotification = try #require( + connection.sentNotifications.first as? OnBuildTargetDidChangeNotification + ) + let changes = try #require(sentNotification.changes) + #expect(changes.count == 3) + #expect(changes.contains { $0.target.uri == swiftTarget && $0.kind == .changed }) + #expect(changes.contains { $0.target.uri == objcTarget && $0.kind == .changed }) + #expect(changes.contains { $0.target.uri == objcppTarget && $0.kind == .changed }) } }