Reload a file when other files within the same module or a .swiftmodule file has been changed

When the client sends us `workspace/didChangeWatchedFiles` notification of an updated `.swift` file, we should refresh the other open files in that module since they might be referencing functions from that updated file.

If a `.swiftmodule` file has been updated, we refresh all the files within the package since they might import that module. Technically, we would only need to refresh files that are in module that are downstream of the updated module but we don’t currently have that information easily available from SwiftPM. Also, usually, if the client has a file from a low-level module open, he’ll be working on that module which means that such an optimization won’t help. The real solution here is to wait for us to finish preparation (which we would exactly know when it finishes since sourcekit-lsp would schedule it) but for that we need to implement background preparation.

Fixes swiftlang#620
Fixes swiftlang#1116
rdar://99329579
rdar://123971779

Author: ahoppen

Sources/SKCore/CMakeLists.txt

@@ -7,6 +7,7 @@ add_library(SKCore STATIC
   BuildSystemManager.swift
   CompilationDatabase.swift
   CompilationDatabaseBuildSystem.swift
+  Debouncer.swift
   FallbackBuildSystem.swift
   FileBuildSettings.swift
   MainFilesProvider.swift

Sources/SKCore/Debouncer.swift

@@ -0,0 +1,81 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+/// Debounces calls to a function/closure. If multiple calls to the closure are made, it allows aggregating the
+/// parameters.
+public actor Debouncer<Parameter> {
+  /// How long to wait for further `scheduleCall` calls before committing to actually calling `makeCall`.
+  private let debounceDuration: Duration
+
+  /// When `scheduleCall` is called while another `scheduleCall` was waiting to commit its call, combines the parameters
+  /// of those two calls.
+  ///
+  /// ### Example
+  ///
+  /// Two `scheduleCall` calls that are made within a time period shorter than `debounceDuration` like the following
+  /// ```swift
+  /// debouncer.scheduleCall(5)
+  /// debouncer.scheduleCall(10)
+  /// ```
+  /// will call `combineParameters(5, 10)`
+  private let combineParameters: (Parameter, Parameter) -> Parameter
+
+  /// After the debounce duration has elapsed, commit the call.
+  private let makeCall: (Parameter) async -> Void
+
+  /// In the time between the call to `scheduleCall` and the call actually being committed (ie. in the time that the
+  /// call can be debounced), the task that would commit the call (unless cancelled) and the parameter with which this
+  /// call should be made.
+  private var inProgressData: (Parameter, Task<Void, Never>)?
+
+  public init(
+    debounceDuration: Duration,
+    combineResults: @escaping (Parameter, Parameter) -> Parameter,
+    _ makeCall: @escaping (Parameter) async -> Void
+  ) {
+    self.debounceDuration = debounceDuration
+    self.combineParameters = combineResults
+    self.makeCall = makeCall
+  }
+
+  /// Schedule a debounced call. If `scheduleCall` is called within `debounceDuration`, the parameters of the two
+  /// `scheduleCall` calls will be combined using `combineParameters` and the new debounced call will be scheduled
+  /// `debounceDuration` after the second `scheduleCall` call.
+  public func scheduleCall(_ parameter: Parameter) {
+    var parameter = parameter
+    if let (inProgressParameter, inProgressTask) = inProgressData {
+      inProgressTask.cancel()
+      parameter = combineParameters(inProgressParameter, parameter)
+    }
+    let task = Task {
+      do {
+        try await Task.sleep(for: debounceDuration)
+        try Task.checkCancellation()
+      } catch {
+        return
+      }
+      inProgressData = nil
+      await makeCall(parameter)
+    }
+    inProgressData = (parameter, task)
+  }
+}
+
+extension Debouncer<Void> {
+  public init(debounceDuration: Duration, _ makeCall: @escaping () async -> Void) {
+    self.init(debounceDuration: debounceDuration, combineResults: { _, _ in }, makeCall)
+  }
+
+  public func scheduleCall() {
+    self.scheduleCall(())
+  }
+}

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -99,6 +99,16 @@ public actor SwiftPMBuildSystem {
   /// This callback is informed when `reloadPackage` starts and ends executing.
   var reloadPackageStatusCallback: (ReloadPackageStatus) async -> Void
 
+  /// Debounces calls to `delegate.filesDependenciesUpdated`.
+  ///
+  /// This is to ensure we don't call `filesDependenciesUpdated` for the same file multiple time if the client does not
+  /// debounce `workspace/didChangeWatchedFiles` and sends a separate notification eg. for every file within a target as
+  /// it's being updated by a git checkout, which would cause other files within that target to receive a
+  /// `fileDependenciesUpdated` call once for every updated file within the target.
+  ///
+  /// Force-unwrapped optional because initializing it requires access to `self`.
+  var fileDependenciesUpdatedDebouncer: Debouncer<Set<DocumentURI>>! = nil
+
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
   ///
   /// - Parameters:
@@ -166,6 +176,19 @@ public actor SwiftPMBuildSystem {
     self.modulesGraph = try ModulesGraph(rootPackages: [], dependencies: [], binaryArtifacts: [:])
     self.reloadPackageStatusCallback = reloadPackageStatusCallback
 
+    // The debounce duration of 500ms was chosen arbitrarily without scientific research.
+    self.fileDependenciesUpdatedDebouncer = Debouncer(
+      debounceDuration: .milliseconds(500),
+      combineResults: { $0.union($1) }
+    ) {
+      [weak self] (filesWithUpdatedDependencies) in
+      guard let delegate = await self?.delegate else {
+        logger.fault("Not calling filesDependenciesUpdated because no delegate exists in SwiftPMBuildSystem")
+        return
+      }
+      await delegate.filesDependenciesUpdated(filesWithUpdatedDependencies)
+    }
+
     try await reloadPackage()
   }
 
@@ -368,6 +391,34 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
         try await self.reloadPackage()
       }
     }
+
+    var filesWithUpdatedDependencies: Set<DocumentURI> = []
+    // If a Swift file within a target is updated, reload all the other files within the target since they might be
+    // referring to a function in the updated file.
+    for event in events {
+      guard let url = event.uri.fileURL,
+        url.pathExtension == "swift",
+        let absolutePath = try? AbsolutePath(validating: url.path),
+        let target = fileToTarget[absolutePath]
+      else {
+        continue
+      }
+      filesWithUpdatedDependencies.formUnion(target.sources.map { DocumentURI($0) })
+    }
+
+    // If a `.swiftmodule` file is updated, this means that we have performed a build / are
+    // performing a build and files that depend on this module have updated dependencies.
+    // We don't have access to the build graph from the SwiftPM API offered to SourceKit-LSP to figure out which files
+    // depend on the updated module, so assume that all files have updated dependencies.
+    // The file watching here is somewhat fragile as well because it assumes that the `.swiftmodule` files are being
+    // written to a directory within the workspace root. This is not necessarily true if the user specifies a build
+    // directory outside the source tree.
+    // All of this shouldn't be necessary once we have background preparation, in which case we know when preparation of
+    // a target has finished.
+    if events.contains(where: { $0.uri.fileURL?.pathExtension == "swiftmodule" }) {
+      filesWithUpdatedDependencies.formUnion(self.fileToTarget.keys.map { DocumentURI($0.asURL) })
+    }
+    await self.fileDependenciesUpdatedDebouncer.scheduleCall(filesWithUpdatedDependencies)
   }
 
   public func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {

Sources/SKTestSupport/FileManager+findFiles.swift

@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import Foundation
+
+extension FileManager {
+  /// Returns the URLs of all files with the given file extension in the given directory (recursively).
+  public func findFiles(withExtension extensionName: String, in directory: URL) -> [URL] {
+    var result: [URL] = []
+    let enumerator = self.enumerator(at: directory, includingPropertiesForKeys: nil)
+    while let url = enumerator?.nextObject() as? URL {
+      if url.pathExtension == extensionName {
+        result.append(url)
+      }
+    }
+    return result
+  }
+}

Sources/SKTestSupport/MultiFileTestProject.swift

@@ -142,4 +142,13 @@ public class MultiFileTestProject {
     }
     return DocumentPositions(markedText: fileData.markedText)[marker]
   }
+
+  public func range(from fromMarker: String, to toMarker: String, in fileName: String) throws -> Range<Position> {
+    return try position(of: fromMarker, in: fileName)..<position(of: toMarker, in: fileName)
+  }
+
+  public func location(from fromMarker: String, to toMarker: String, in fileName: String) throws -> Location {
+    let range = try self.range(from: fromMarker, to: toMarker, in: fileName)
+    return Location(uri: try self.uri(for: fileName), range: range)
+  }
 }

Sources/SKTestSupport/SkipUnless.swift

@@ -260,6 +260,7 @@ public enum SkipUnless {
     }
   }
 
+  /// A long test is a test that takes longer than 1-2s to execute.
   public static func longTestsEnabled() throws {
     if let value = ProcessInfo.processInfo.environment["SKIP_LONG_TESTS"], value == "1" || value == "YES" {
       throw XCTSkip("Long tests disabled using the `SKIP_LONG_TESTS` environment variable")

Sources/SKTestSupport/SwiftPMTestProject.swift

@@ -40,6 +40,7 @@ public class SwiftPMTestProject: MultiFileTestProject {
     manifest: String = SwiftPMTestProject.defaultPackageManifest,
     workspaces: (URL) -> [WorkspaceFolder] = { [WorkspaceFolder(uri: DocumentURI($0))] },
     build: Bool = false,
+    allowBuildFailure: Bool = false,
     usePullDiagnostics: Bool = true,
     testName: String = #function
   ) async throws {
@@ -66,7 +67,11 @@ public class SwiftPMTestProject: MultiFileTestProject {
     )
 
     if build {
-      try await Self.build(at: self.scratchDirectory)
+      if allowBuildFailure {
+        try? await Self.build(at: self.scratchDirectory)
+      } else {
+        try await Self.build(at: self.scratchDirectory)
+      }
     }
     // Wait for the indexstore-db to finish indexing
     _ = try await testClient.send(PollIndexRequest())

Sources/SKTestSupport/TestSourceKitLSPClient.swift

@@ -287,7 +287,6 @@ public final class TestSourceKitLSPClient: MessageHandler {
     reply: @escaping (LSPResult<Request.Response>) -> Void
   ) {
     guard let requestHandler = requestHandlers.first else {
-      XCTFail("Received unexpected request \(Request.method)")
       reply(.failure(.methodNotFound(Request.method)))
       return
     }
@@ -362,7 +361,7 @@ public struct DocumentPositions {
     }
   }
 
-  init(markedText: String) {
+  public init(markedText: String) {
     let (markers, textWithoutMarker) = extractMarkers(markedText)
     self.init(markers: markers, textWithoutMarkers: textWithoutMarker)
   }

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -1386,6 +1386,9 @@ extension SourceKitLSPServer {
     watchers.append(FileSystemWatcher(globPattern: "**/Package.swift", kind: [.change]))
     watchers.append(FileSystemWatcher(globPattern: "**/compile_commands.json", kind: [.create, .change, .delete]))
     watchers.append(FileSystemWatcher(globPattern: "**/compile_flags.txt", kind: [.create, .change, .delete]))
+    // Watch for changes to `.swiftmodule` files to detect updated modules during a build.
+    // See comments in `SwiftPMBuildSystem.filesDidChange``
+    watchers.append(FileSystemWatcher(globPattern: "**/*.swiftmodule", kind: [.create, .change, .delete]))
     await registry.registerDidChangeWatchedFiles(watchers: watchers, server: self)
   }
 

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

@@ -160,6 +160,15 @@ public actor SwiftLanguageService: LanguageService {
 
   private var diagnosticReportManager: DiagnosticReportManager!
 
+  /// Calling `scheduleCall` on `refreshDiagnosticsDebouncer` schedules a `DiagnosticsRefreshRequest` to be sent to
+  /// to the client.
+  ///
+  /// We debounce these calls because the `DiagnosticsRefreshRequest` is a workspace-wide request. If we discover that
+  /// the client should update diagnostics for file A and then discover that it should also update diagnostics for file
+  /// B, we don't want to send two `DiagnosticsRefreshRequest`s. Instead, the two should be unified into a single
+  /// request.
+  private let refreshDiagnosticsDebouncer: Debouncer<Void>
+
   /// Only exists to work around rdar://116221716.
   /// Once that is fixed, remove the property and make `diagnosticReportManager` non-optional.
   private var clientHasDiagnosticsCodeDescriptionSupport: Bool {
@@ -189,6 +198,17 @@ public actor SwiftLanguageService: LanguageService {
     self.generatedInterfacesPath = options.generatedInterfacesPath.asURL
     try FileManager.default.createDirectory(at: generatedInterfacesPath, withIntermediateDirectories: true)
     self.diagnosticReportManager = nil  // Needed to work around rdar://116221716
+
+    // The debounce duration of 500ms was chosen arbitrarily without scientific research.
+    self.refreshDiagnosticsDebouncer = Debouncer(debounceDuration: .milliseconds(500)) { [weak sourceKitLSPServer] in
+      guard let sourceKitLSPServer else {
+        logger.fault("Not sending DiagnosticRefreshRequest to client because sourceKitLSPServer has been deallocated")
+        return
+      }
+      _ = await orLog("Sending DiagnosticRefreshRequest to client after document dependencies updated") {
+        try await sourceKitLSPServer.sendRequestToClient(DiagnosticsRefreshRequest())
+      }
+    }
     self.diagnosticReportManager = DiagnosticReportManager(
       sourcekitd: self.sourcekitd,
       syntaxTreeManager: syntaxTreeManager,
@@ -304,7 +324,7 @@ extension SwiftLanguageService {
 
   private func reopenDocument(_ snapshot: DocumentSnapshot, _ compileCmd: SwiftCompileCommand?) async {
     cancelInFlightPublishDiagnosticsTask(for: snapshot.uri)
-    await diagnosticReportManager.removeItemsFromCache(with: snapshot.id.uri)
+    await diagnosticReportManager.removeItemsFromCache(with: snapshot.uri)
 
     let keys = self.keys
     let path = snapshot.uri.pseudoPath
@@ -324,7 +344,11 @@ extension SwiftLanguageService {
 
     _ = try? await self.sourcekitd.send(openReq, fileContents: snapshot.text)
 
-    await publishDiagnosticsIfNeeded(for: snapshot.uri)
+    if await capabilityRegistry.clientSupportsPullDiagnostics(for: .swift) {
+      await self.refreshDiagnosticsDebouncer.scheduleCall()
+    } else {
+      await publishDiagnosticsIfNeeded(for: snapshot.uri)
+    }
   }
 
   public func documentUpdatedBuildSettings(_ uri: DocumentURI) async {
@@ -339,13 +363,14 @@ extension SwiftLanguageService {
   }
 
   public func documentDependenciesUpdated(_ uri: DocumentURI) async {
-    guard let snapshot = try? self.documentManager.latestSnapshot(uri) else {
-      return
+    await orLog("Sending dependencyUpdated request to sourcekitd") {
+      let req = sourcekitd.dictionary([
+        keys.request: requests.dependencyUpdated
+      ])
+      _ = try await self.sourcekitd.send(req, fileContents: nil)
     }
-
-    // Forcefully reopen the document since the `BuildSystem` has informed us
-    // that the dependencies have changed and the AST needs to be reloaded.
-    await self.reopenDocument(snapshot, self.buildSettings(for: uri))
+    // `documentUpdatedBuildSettings` already handles reopening the document, so we do that here as well.
+    await self.documentUpdatedBuildSettings(uri)
   }
 
   // MARK: - Text synchronization

Tests/SKCoreTests/DebouncerTests.swift

@@ -0,0 +1,41 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SKCore
+import XCTest
+
+final class DebouncerTests: XCTestCase {
+  func testDebouncerDebounces() async throws {
+    let expectation = self.expectation(description: "makeCallCalled")
+    expectation.assertForOverFulfill = true
+    let debouncer = Debouncer<Void>(debounceDuration: .seconds(0.1)) {
+      expectation.fulfill()
+    }
+    await debouncer.scheduleCall()
+    await debouncer.scheduleCall()
+    try await self.fulfillmentOfOrThrow([expectation])
+    // Sleep for 0.2s to make sure the debouncer actually debounces and doesn't fulfill the expectation twice.
+    try await Task.sleep(for: .seconds(0.2))
+  }
+
+  func testDebouncerCombinesParameters() async throws {
+    let expectation = self.expectation(description: "makeCallCalled")
+    expectation.assertForOverFulfill = true
+    let debouncer = Debouncer<Int>(debounceDuration: .seconds(0.1), combineResults: { $0 + $1 }) { param in
+      XCTAssertEqual(param, 3)
+      expectation.fulfill()
+    }
+    await debouncer.scheduleCall(1)
+    await debouncer.scheduleCall(2)
+    try await self.fulfillmentOfOrThrow([expectation])
+  }
+}