Merge pull request #1180 from ahoppen/refresh-file-on-updated-dependencies

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

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])
+  }
+}