Fix WatchedFiles misreporting target statuses (#52)

rockbruno · web-flow · commit 32aaa892e74c · 2025-08-29T13:17:55.000+02:00
* Fix WatchedFiles misreporting target statuses

* Lint changes
diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/PrepareHandler.swift b/Sources/SourceKitBazelBSP/RequestHandlers/PrepareHandler.swift
@@ -89,7 +89,7 @@ final class PrepareHandler {
 }
 
 extension PrepareHandler: InvalidatedTargetObserver {
-    func invalidate(targets: Set<AffectedTarget>) throws {
+    func invalidate(targets: [InvalidatedTarget]) throws {
         // Extract just the URIs from the affected targets for the build cache
         let targetURIs = Set(targets.map(\.uri))
         buildCache.subtract(targetURIs)
diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/SKOptions/SKOptionsHandler.swift b/Sources/SourceKitBazelBSP/RequestHandlers/SKOptions/SKOptionsHandler.swift
@@ -92,7 +92,7 @@ final class SKOptionsHandler: InvalidatedTargetObserver {
 
     // MARK: - InvalidatedTargetObserver
 
-    func invalidate(targets: Set<AffectedTarget>) throws {
+    func invalidate(targets: [InvalidatedTarget]) throws {
         // Only clear cache if at least one file was created or deleted
         if targets.contains(where: { $0.kind == .created || $0.kind == .deleted }) {
             extractor.clearCache()
diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/InvalidatedTargetObserver.swift b/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/InvalidatedTargetObserver.swift
@@ -20,13 +20,17 @@
 import BuildServerProtocol
 import LanguageServerProtocol
 
-/// Represents a target that was affected by a file change
-struct AffectedTarget: Hashable {
-    let uri: URI  // Build target URI, not document URI
+/// Represents a target that was affected by a file change.
+struct InvalidatedTarget: Hashable {
+    /// The URI of the build target that was invalidated.
+    let uri: URI
+    /// The URI of the file that triggered the invalidation.
+    let fileUri: URI
+    /// The kind of file change that triggered the invalidation.
     let kind: FileChangeType
 }
 
 /// Protocol for objects that need to be notified when build targets are invalidated
 protocol InvalidatedTargetObserver: AnyObject {
-    func invalidate(targets: Set<AffectedTarget>) throws
+    func invalidate(targets: [InvalidatedTarget]) throws
 }
diff --git a/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift b/Sources/SourceKitBazelBSP/RequestHandlers/WatchedFiles/WatchedFileChangeHandler.swift
@@ -67,12 +67,16 @@ final class WatchedFileChangeHandler {
 
         logger.info("Received \(changes.count) file changes")
 
-        // First, calculate deleted targets before we clear them from the targetStore
-        let deletedTargets = {
+        let deletedFiles = changes.filter { $0.type == .deleted }
+        let createdFiles = changes.filter { $0.type == .created }
+        let changedFiles = changes.filter { $0.type == .changed }
+
+        // First, determine which targets had removed files.
+        let targetsAffectedByDeletions: [InvalidatedTarget] = {
             do {
-                return try changes.filter { $0.type == .deleted }.flatMap { change -> [AffectedTarget] in
+                return try deletedFiles.flatMap { change in
                     try targetStore.bspURIs(containingSrc: change.uri).map {
-                        AffectedTarget(uri: $0, kind: change.type)
+                        InvalidatedTarget(uri: $0, fileUri: change.uri, kind: .deleted)
                     }
                 }
             } catch {
@@ -81,9 +85,10 @@ final class WatchedFileChangeHandler {
             }
         }()
 
-        // If there are any 'created' files, we need to clear the targetStore and fetch targets again
-        // Otherwise, the targetStore won't know about them
-        if changes.contains(where: { $0.type == .created }) {
+        // If there are any 'created' files, we need to clear the targetStore immediately and fetch targets again.
+        // Otherwise, the targetStore won't know about them.
+        // FIXME: This is quite expensive, but the easier thing to do. We can try improving this later.
+        if !createdFiles.isEmpty {
             let taskId = TaskId(id: "watchedFiles-\(UUID().uuidString)")
             connection?.startWorkTask(id: taskId, title: "Indexing: Re-processing build graph")
             targetStore.clearCache()
@@ -96,12 +101,13 @@ final class WatchedFileChangeHandler {
             connection?.finishTask(id: taskId, status: .ok)
         }
 
-        // Now that the targetStore knows about the newly created files, we can calculate the created targets
-        let createdTargets = {
+        // Now that the targetStore knows about the newly created files, we can determine which targets
+        // were affected by those creations.
+        let targetsAffectedByCreations: [InvalidatedTarget] = {
             do {
-                return try changes.filter { $0.type == .created }.flatMap { change -> [AffectedTarget] in
+                return try createdFiles.flatMap { change in
                     try targetStore.bspURIs(containingSrc: change.uri).map {
-                        AffectedTarget(uri: $0, kind: change.type)
+                        InvalidatedTarget(uri: $0, fileUri: change.uri, kind: .created)
                     }
                 }
             } catch {
@@ -110,12 +116,12 @@ final class WatchedFileChangeHandler {
             }
         }()
 
-        // Finally, calculate the changed targets
-        let changedTargets = {
+        // Finally, calculate the targets affected by regular changes.
+        let targetsAffectedByChanges: [InvalidatedTarget] = {
             do {
-                return try changes.filter { $0.type == .changed }.flatMap { change -> [AffectedTarget] in
+                return try changedFiles.flatMap { change in
                     try targetStore.bspURIs(containingSrc: change.uri).map {
-                        AffectedTarget(uri: $0, kind: change.type)
+                        InvalidatedTarget(uri: $0, fileUri: change.uri, kind: .changed)
                     }
                 }
             } catch {
@@ -124,24 +130,20 @@ final class WatchedFileChangeHandler {
             }
         }()
 
-        let affectedTargets: Set<AffectedTarget> = Set(deletedTargets + createdTargets + changedTargets)
+        let invalidatedTargets = targetsAffectedByDeletions + targetsAffectedByCreations + targetsAffectedByChanges
 
-        // Invalidate our observers about the affected targets
+        // Notify our observers about the affected targets
         for observer in observers {
-            do {
-                try observer.invalidate(targets: affectedTargets)
-            } catch {
-                logger.error("Error invalidating observer: \(error)")
-                // Continue with other observers
-            }
+            try? observer.invalidate(targets: invalidatedTargets)
         }
 
         // Notify SK-LSP about the affected targets
+        let uniqueInvalidatedTargets = Set(invalidatedTargets.map { $0.uri })
         let response = OnBuildTargetDidChangeNotification(
-            changes: affectedTargets.map { target in
+            changes: uniqueInvalidatedTargets.map { targetUri in
                 BuildTargetEvent(
-                    target: BuildTargetIdentifier(uri: target.uri),
-                    kind: target.kind.buildTargetEventKind,
+                    target: BuildTargetIdentifier(uri: targetUri),
+                    kind: .changed,  // FIXME: We should eventually detect here also if the target is new/deleted.
                     dataKind: nil,
                     data: nil
                 )
@@ -160,14 +162,3 @@ final class WatchedFileChangeHandler {
         return supportedFileExtensions.contains(String(result.reversed()))
     }
 }
-
-extension FileChangeType {
-    fileprivate var buildTargetEventKind: BuildTargetEventKind? {
-        switch self {
-        case .changed: return .changed
-        case .created: return .created
-        case .deleted: return .deleted
-        default: return nil
-        }
-    }
-}
diff --git a/Tests/SourceKitBazelBSPTests/Fakes/InvalidatedTargetObserverFake.swift b/Tests/SourceKitBazelBSPTests/Fakes/InvalidatedTargetObserverFake.swift
@@ -27,11 +27,11 @@ final class InvalidatedTargetObserverFake: InvalidatedTargetObserver {
         case intentional
     }
 
-    var invalidatedTargets: Set<AffectedTarget> = []
+    var invalidatedTargets: [InvalidatedTarget] = []
     var invalidateCalled = false
     var shouldThrowOnInvalidate = false
 
-    func invalidate(targets: Set<AffectedTarget>) throws {
+    func invalidate(targets: [InvalidatedTarget]) throws {
         invalidateCalled = true
         invalidatedTargets = targets
         if shouldThrowOnInvalidate {
diff --git a/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift b/Tests/SourceKitBazelBSPTests/WatchedFileChangeHandlerTests.swift
@@ -80,8 +80,12 @@ struct WatchedFileChangeHandlerTests {
         // Check that the observer was notified
         #expect(observer.invalidateCalled)
         #expect(observer.invalidatedTargets.count == 2)
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: targetURI1, kind: .deleted)))
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: targetURI2, kind: .deleted)))
+        #expect(
+            observer.invalidatedTargets.contains(InvalidatedTarget(uri: targetURI1, fileUri: fileURI, kind: .deleted))
+        )
+        #expect(
+            observer.invalidatedTargets.contains(InvalidatedTarget(uri: targetURI2, fileUri: fileURI, kind: .deleted))
+        )
 
         // Check that the LSP connection received the notification
         #expect(connection.sentNotifications.count == 1)
@@ -94,7 +98,7 @@ struct WatchedFileChangeHandlerTests {
                 #expect(actualTargetURIs == expectedTargetURIs)
 
                 for change in changes {
-                    #expect(change.kind == .deleted)
+                    #expect(change.kind == .changed)
                 }
             }
         } else {
@@ -129,14 +133,16 @@ struct WatchedFileChangeHandlerTests {
         // Check that the observer was notified
         #expect(observer.invalidateCalled)
         #expect(observer.invalidatedTargets.count == 1)
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: targetURI, kind: .created)))
+        #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 == .created)
+            #expect(sentNotification.changes?[0].kind == .changed)
         } else {
             Issue.record("Expected OnBuildTargetDidChangeNotification")
         }
@@ -169,7 +175,9 @@ struct WatchedFileChangeHandlerTests {
         // Check that the observer was notified
         #expect(observer.invalidateCalled)
         #expect(observer.invalidatedTargets.count == 1)
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: targetURI, kind: .changed)))
+        #expect(
+            observer.invalidatedTargets.contains(InvalidatedTarget(uri: targetURI, fileUri: fileURI, kind: .changed))
+        )
 
         // Check that the LSP connection received the notification
         #expect(connection.sentNotifications.count == 1)
@@ -217,9 +225,21 @@ struct WatchedFileChangeHandlerTests {
         // Check that the observer was notified with all affected targets
         #expect(observer.invalidateCalled)
         #expect(observer.invalidatedTargets.count == 3)
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: deletedTargetURI, kind: .deleted)))
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: createdTargetURI, kind: .created)))
-        #expect(observer.invalidatedTargets.contains(AffectedTarget(uri: changedTargetURI, kind: .changed)))
+        #expect(
+            observer.invalidatedTargets.contains(
+                InvalidatedTarget(uri: deletedTargetURI, fileUri: deletedFileURI, kind: .deleted)
+            )
+        )
+        #expect(
+            observer.invalidatedTargets.contains(
+                InvalidatedTarget(uri: createdTargetURI, fileUri: createdFileURI, kind: .created)
+            )
+        )
+        #expect(
+            observer.invalidatedTargets.contains(
+                InvalidatedTarget(uri: changedTargetURI, fileUri: changedFileURI, kind: .changed)
+            )
+        )
 
         // Check that the LSP connection received the notification with all changes
         #expect(connection.sentNotifications.count == 1)
@@ -228,8 +248,8 @@ struct WatchedFileChangeHandlerTests {
 
             if let notificationChanges = sentNotification.changes {
                 let changes = notificationChanges.map { (uri: $0.target.uri, kind: $0.kind) }
-                #expect(changes.contains { $0.uri == deletedTargetURI && $0.kind == .deleted })
-                #expect(changes.contains { $0.uri == createdTargetURI && $0.kind == .created })
+                #expect(changes.contains { $0.uri == deletedTargetURI && $0.kind == .changed })
+                #expect(changes.contains { $0.uri == createdTargetURI && $0.kind == .changed })
                 #expect(changes.contains { $0.uri == changedTargetURI && $0.kind == .changed })
             }
         } else {

Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ final class PrepareHandler {`
`89`	`89`	`}`
`90`	`90`
`91`	`91`	`extension PrepareHandler: InvalidatedTargetObserver {`
`92`		`- func invalidate(targets: Set<AffectedTarget>) throws {`
	`92`	`+ func invalidate(targets: [InvalidatedTarget]) throws {`
`93`	`93`	`// Extract just the URIs from the affected targets for the build cache`
`94`	`94`	`let targetURIs = Set(targets.map(\.uri))`
`95`	`95`	`buildCache.subtract(targetURIs)`