Merge pull request #2054 from ahoppen/file-change-freestanding

Handle file did change notifications as freestanding messages

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(
@@ -587,15 +572,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
@@ -703,6 +680,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
@@ -1657,19 +1636,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.
@@ -1718,16 +1692,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️⃣"])
@@ -1781,9 +1754,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(
@@ -2080,22 +2051,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

@@ -354,12 +354,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

@@ -199,13 +199,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(