Handle file did change notifications as freestanding messages

Technically, the watched files notification can change the response of any other request (eg. because a target needs to be re-prepared). But treating it as a `globalConfiguration` inserts a lot of barriers in request  handling and significantly prevents parallelism. Since many editors batch file change notifications already, they might have delayed the file change notification even more, which is equivalent to handling the  notification a little later inside SourceKit-LSP. Thus, treating it as `freestanding` should be acceptable.

Also, simplify the logic needed in tests to write modified files to disk because we now need to run a barrier request in tests to ensure that the watched file notification has been handled.

Author: ahoppen

Sources/SKTestSupport/MultiFileTestProject.swift

@@ -12,6 +12,7 @@
 
 package import Foundation
 package import LanguageServerProtocol
+import SKLogging
 package import SKOptions
 package import SourceKitLSP
 import SwiftExtensions
@@ -66,7 +67,10 @@ package class MultiFileTestProject {
 
   enum Error: Swift.Error {
     /// No file with the given filename is known to the `MultiFileTestProject`.
-    case fileNotFound
+    case fileNotFound(name: String)
+
+    /// Trying to delete a file in `changeDocument` that doesn't exist.
+    case deleteNonExistentFile
   }
 
   /// The directory in which the temporary files are being placed.
@@ -168,7 +172,7 @@ package class MultiFileTestProject {
     language: Language? = nil
   ) throws -> (uri: DocumentURI, positions: DocumentPositions) {
     guard let fileData = self.fileData[fileName] else {
-      throw Error.fileNotFound
+      throw Error.fileNotFound(name: fileName)
     }
     let positions = testClient.openDocument(fileData.markedText, uri: fileData.uri, language: language)
     return (fileData.uri, positions)
@@ -177,15 +181,15 @@ package class MultiFileTestProject {
   /// Returns the URI of the file with the given name.
   package func uri(for fileName: String) throws -> DocumentURI {
     guard let fileData = self.fileData[fileName] else {
-      throw Error.fileNotFound
+      throw Error.fileNotFound(name: fileName)
     }
     return fileData.uri
   }
 
   /// Returns the position of the given marker in the given file.
   package func position(of marker: String, in fileName: String) throws -> Position {
     guard let fileData = self.fileData[fileName] else {
-      throw Error.fileNotFound
+      throw Error.fileNotFound(name: fileName)
     }
     return DocumentPositions(markedText: fileData.markedText)[marker]
   }
@@ -198,4 +202,50 @@ package class MultiFileTestProject {
     let range = try self.range(from: fromMarker, to: toMarker, in: fileName)
     return Location(uri: try self.uri(for: fileName), range: range)
   }
+
+  /// Modify the given file on disk, send a watched files did change notification to SourceKit-LSP and wait for that
+  /// notification to get handled.
+  ///
+  /// When `newMarkedContents` is `nil`, the file is deleted.
+  ///
+  /// Returns the URI and the document positions of the new document.
+  @discardableResult
+  package func changeFileOnDisk(
+    _ fileName: String,
+    newMarkedContents: String?
+  ) async throws -> (uri: DocumentURI, positions: DocumentPositions) {
+    let uri = try self.uri(for: fileName)
+    guard let url = uri.fileURL else {
+      throw Error.fileNotFound(name: fileName)
+    }
+    let positions: DocumentPositions
+    let newContents: String?
+    if let newMarkedContents {
+      (positions, newContents) = DocumentPositions.extract(from: newMarkedContents)
+    } else {
+      positions = DocumentPositions(markedText: "")
+      newContents = nil
+    }
+
+    let fileExists = FileManager.default.fileExists(at: url)
+    let changeType: FileChangeType
+    switch (fileExists, newContents) {
+    case (false, let newContents?):
+      try await newContents.writeWithRetry(to: url)
+      changeType = .created
+    case (false, nil):
+      throw Error.deleteNonExistentFile
+    case (true, let newContents?):
+      try await newContents.writeWithRetry(to: url)
+      changeType = .changed
+    case (true, nil):
+      try FileManager.default.removeItem(at: url)
+      changeType = .deleted
+    }
+    testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: uri, type: changeType)]))
+    // Ensure that we handle the `DidChangeWatchedFilesNotification`.
+    try await testClient.send(BarrierRequest())
+
+    return (uri, positions)
+  }
 }

Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift

@@ -103,7 +103,12 @@ package enum MessageHandlingDependencyTracker: QueueBasedMessageHandlerDependenc
     case let notification as DidChangeTextDocumentNotification:
       self = .documentUpdate(notification.textDocument.uri)
     case is DidChangeWatchedFilesNotification:
-      self = .globalConfigurationChange
+      // Technically, the watched files notification can change the response of any other request (eg. because a target
+      // needs to be re-prepared). But treating it as a `globalConfiguration` inserts a lot of barriers in request
+      // handling and significantly prevents parallelism. Since many editors batch file change notifications already,
+      // they might have delayed the file change notification even more, which is equivalent to handling the
+      // notification a little later inside SourceKit-LSP. Thus, treating it as `freestanding` should be acceptable.
+      self = .freestanding
     case is DidChangeWorkspaceFoldersNotification:
       self = .globalConfigurationChange
     case let notification as DidCloseNotebookDocumentNotification:

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -1482,9 +1482,7 @@ extension SourceKitLSPServer {
     // a file doesn't mean a file can't affect the build system's build settings
     // (e.g. Package.swift doesn't have build settings but affects build
     // settings). Inform the build system about all file changes.
-    for workspace in workspaces {
-      await workspace.filesDidChange(notification.changes)
-    }
+    await workspaces.concurrentForEach { await $0.filesDidChange(notification.changes) }
   }
 
   func setBackgroundIndexingPaused(_ request: SetOptionsRequest) async throws -> VoidResponse {

Sources/SourceKitLSP/Workspace.swift

@@ -382,8 +382,10 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
 
     // Notify all clients about the reported and inferred edits.
     await buildSystemManager.filesDidChange(events)
-    await syntacticTestIndex.filesDidChange(events)
-    await semanticIndexManager?.filesDidChange(events)
+
+    async let updateSyntacticIndex: Void = await syntacticTestIndex.filesDidChange(events)
+    async let updateSemanticIndex: Void? = await semanticIndexManager?.filesDidChange(events)
+    _ = await (updateSyntacticIndex, updateSemanticIndex)
   }
 
   func documentService(for uri: DocumentURI) -> LanguageService? {

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -430,24 +430,14 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual(callsBeforeEdit, [])
 
-    let (otherFilePositions, otherFileMarkedContents) = DocumentPositions.extract(
-      from: """
+    let (otherFileUri, otherFilePositions) = try await project.changeFileOnDisk(
+      "MyOtherFile.swift",
+      newMarkedContents: """
         func 2️⃣bar() {
           3️⃣foo()
         }
         """
     )
-
-    let otherFileUri = try project.uri(for: "MyOtherFile.swift")
-    let otherFileUrl = try XCTUnwrap(otherFileUri.fileURL)
-
-    try otherFileMarkedContents.write(
-      to: otherFileUrl,
-      atomically: true,
-      encoding: .utf8
-    )
-
-    project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: otherFileUri, type: .changed)]))
     try await project.testClient.send(PollIndexRequest())
 
     let callsAfterEdit = try await project.testClient.send(
@@ -498,21 +488,16 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual(callsBeforeEdit, [])
 
-    let (newPositions, headerNewMarkedContents) = DocumentPositions.extract(
-      from: """
+    let (_, newPositions) = try await project.changeFileOnDisk(
+      "Header.h",
+      newMarkedContents: """
         void someFunc();
 
         void 2️⃣test() {
           3️⃣someFunc();
         };
         """
     )
-
-    // clangd might have Header.h open, which prevents us from updating it. Keep retrying until we get a successful
-    // write. This matches what a user would do.
-    try await headerNewMarkedContents.writeWithRetry(to: try XCTUnwrap(uri.fileURL))
-
-    project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: uri, type: .changed)]))
     try await project.testClient.send(PollIndexRequest())
 
     let callsAfterEdit = try await project.testClient.send(
@@ -584,15 +569,7 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertNotEqual(initialDiagnostics.fullReport?.items, [])
 
-    try "public func foo() {}".write(
-      to: try XCTUnwrap(project.uri(for: "MyFile.swift").fileURL),
-      atomically: true,
-      encoding: .utf8
-    )
-
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: try project.uri(for: "MyFile.swift"), type: .changed)])
-    )
+    try await project.changeFileOnDisk("MyFile.swift", newMarkedContents: "public func foo() {}")
 
     let receivedEmptyDiagnostics = self.expectation(description: "Received diagnostic refresh request")
     receivedEmptyDiagnostics.assertForOverFulfill = false
@@ -700,6 +677,8 @@ final class BackgroundIndexingTests: XCTestCase {
     project.testClient.send(
       DidChangeWatchedFilesNotification(changes: [FileEvent(uri: try project.uri(for: "LibA.swift"), type: .changed)])
     )
+    // Ensure that we handle the `DidChangeWatchedFilesNotification`.
+    try await project.testClient.send(BarrierRequest())
 
     // Quickly flip through all files. The way the test is designed to work is as follows:
     //  - LibB.swift gets opened and prepared. Preparation is simulated to take a long time until both LibC.swift and
@@ -1654,19 +1633,14 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertNil(definitionBeforeEdit)
 
-    let libAUri = try project.uri(for: "LibA.swift")
-    let (newAMarkers, newAContents) = DocumentPositions.extract(
-      from: """
+    let (libAUri, newAMarkers) = try await project.changeFileOnDisk(
+      "LibA.swift",
+      newMarkedContents: """
         public struct LibA {
           public func 2️⃣test() {}
         }
         """
     )
-    try await newAContents.writeWithRetry(to: XCTUnwrap(libAUri.fileURL))
-
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: libAUri, type: .changed)])
-    )
 
     // Triggering a definition request causes `LibC` to be re-prepared. Repeat the request until LibC has been prepared
     // and we get the expected result.
@@ -1715,16 +1689,15 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual(completionBeforeEdit.items.map(\.label), ["self"])
 
-    let libAUri = try project.uri(for: "LibA.swift")
-    try await """
-    public struct LibA {
-      public func test() {}
-    }
-    """.writeWithRetry(to: XCTUnwrap(libAUri.fileURL))
-
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: libAUri, type: .changed)])
+    try await project.changeFileOnDisk(
+      "LibA.swift",
+      newMarkedContents: """
+        public struct LibA {
+          public func test() {}
+        }
+        """
     )
+
     try await repeatUntilExpectedResult {
       let completionAfterEdit = try await project.testClient.send(
         CompletionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
@@ -1778,9 +1751,7 @@ final class BackgroundIndexingTests: XCTestCase {
       "Pre edit hover content '\(preEditHoverContent)' does not contain 'String'"
     )
 
-    let libAUri = try project.uri(for: "LibA.swift")
-    try await "public let myVar: Int".writeWithRetry(to: try XCTUnwrap(libAUri.fileURL))
-    project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: libAUri, type: .changed)]))
+    try await project.changeFileOnDisk("LibA.swift", newMarkedContents: "public let myVar: Int")
 
     try await repeatUntilExpectedResult {
       let postEditHover = try await project.testClient.send(
@@ -2076,22 +2047,22 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertNil(hoverWithMissingDependencyDeclaration)
 
-    let manifestUri = try project.uri(for: "Package.swift")
-    try """
-    // swift-tools-version: 5.7
+    try await project.changeFileOnDisk(
+      "Package.swift",
+      newMarkedContents: """
+        // swift-tools-version: 5.7
 
-    import PackageDescription
+        import PackageDescription
 
-    let package = Package(
-      name: "MyLibrary",
-      targets: [
-       .target(name: "LibA", swiftSettings: [.define("ENABLE_FOO")]),
-       .target(name: "LibB", dependencies: ["LibA"]),
-      ]
+        let package = Package(
+          name: "MyLibrary",
+          targets: [
+           .target(name: "LibA", swiftSettings: [.define("ENABLE_FOO")]),
+           .target(name: "LibB", dependencies: ["LibA"]),
+          ]
+        )
+        """
     )
-    """.write(to: XCTUnwrap(project.uri(for: "Package.swift").fileURL), atomically: true, encoding: .utf8)
-
-    project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: manifestUri, type: .changed)]))
 
     try await repeatUntilExpectedResult {
       let hoverAfterAddingDependencyDeclaration = try await project.testClient.send(

Tests/SourceKitLSPTests/CompilationDatabaseTests.swift

@@ -56,17 +56,7 @@ final class CompilationDatabaseTests: XCTestCase {
 
     // Remove -DFOO from the compile commands.
 
-    let compileFlagsUri = try project.uri(for: FixedCompilationDatabaseBuildSystem.dbName)
-    try await "".writeWithRetry(to: XCTUnwrap(compileFlagsUri.fileURL))
-
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [
-        FileEvent(uri: compileFlagsUri, type: .changed)
-      ])
-    )
-
-    // Ensure that the DidChangeWatchedFilesNotification is handled before we continue.
-    try await project.testClient.send(PollIndexRequest())
+    try await project.changeFileOnDisk(FixedCompilationDatabaseBuildSystem.dbName, newMarkedContents: "")
 
     // DocumentHighlight should now point to the definition in the `#else` block.
 

Tests/SourceKitLSPTests/DefinitionTests.swift

@@ -351,12 +351,9 @@ class DefinitionTests: XCTestCase {
     )
     XCTAssertNil(beforeChangingFileA)
 
-    let (updatedAPositions, updatedACode) = DocumentPositions.extract(from: "func 2️⃣sayHello() {}")
-
-    let aUri = try project.uri(for: "FileA.swift")
-    try await updatedACode.writeWithRetry(to: XCTUnwrap(aUri.fileURL))
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: aUri, type: .changed)])
+    let (aUri, updatedAPositions) = try await project.changeFileOnDisk(
+      "FileA.swift",
+      newMarkedContents: "func 2️⃣sayHello() {}"
     )
 
     // Wait until SourceKit-LSP has handled the `DidChangeWatchedFilesNotification` (which it only does after a delay

Tests/SourceKitLSPTests/MainFilesProviderTests.swift

@@ -188,13 +188,11 @@ final class MainFilesProviderTests: XCTestCase {
     let preEditDiag = try XCTUnwrap(preEditDiags.diagnostics.first)
     XCTAssertEqual(preEditDiag.message, "Unused variable 'fromMyLibrary'")
 
-    let newFancyLibraryContents = """
-      #include "\(try project.scratchDirectory.filePath)/Sources/shared.h"
-      """
-    let fancyLibraryUri = try project.uri(for: "MyFancyLibrary.c")
-    try await newFancyLibraryContents.writeWithRetry(to: XCTUnwrap(fancyLibraryUri.fileURL))
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: fancyLibraryUri, type: .changed)])
+    try await project.changeFileOnDisk(
+      "MyFancyLibrary.c",
+      newMarkedContents: """
+        #include "\(try project.scratchDirectory.filePath)/Sources/shared.h"
+        """
     )
 
     // 'MyFancyLibrary.c' now also includes 'shared.h'. Since it lexicographically precedes MyLibrary, we should use its

Tests/SourceKitLSPTests/PublishDiagnosticsTests.swift

@@ -146,12 +146,7 @@ final class PublishDiagnosticsTests: XCTestCase {
       return false
     }
 
-    let updatedACode = "func sayHello() {}"
-    let aUri = try project.uri(for: "FileA.swift")
-    try await updatedACode.writeWithRetry(to: XCTUnwrap(aUri.fileURL))
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: aUri, type: .changed)])
-    )
+    try await project.changeFileOnDisk("FileA.swift", newMarkedContents: "func sayHello() {}")
 
     let diagnosticsAfterChangingFileA = try await project.testClient.nextDiagnosticsNotification()
     XCTAssertEqual(diagnosticsAfterChangingFileA.diagnostics, [])

Tests/SourceKitLSPTests/PullDiagnosticsTests.swift

@@ -196,13 +196,7 @@ final class PullDiagnosticsTests: XCTestCase {
       return VoidResponse()
     }
 
-    let updatedACode = "func sayHello() {}"
-    let aUri = try project.uri(for: "FileA.swift")
-    try await updatedACode.writeWithRetry(to: XCTUnwrap(aUri.fileURL))
-    project.testClient.send(
-      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: aUri, type: .changed)])
-    )
-
+    try await project.changeFileOnDisk("FileA.swift", newMarkedContents: "func sayHello() {}")
     try await fulfillmentOfOrThrow([diagnosticsRefreshRequestReceived])
 
     let afterChangingFileA = try await project.testClient.send(