Reply faster to BuildTargets (#126)

Author: rockbruno

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
+common:index_build --show_result=0 --noshow_loading_progress --noshow_progress

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<Void> { 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 {

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)

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")
 

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 {