Skip to content

Commit

Permalink
Reload a file when other files within the same module or a `.swiftmod…
Browse files Browse the repository at this point in the history
…ule` 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
  • Loading branch information
ahoppen committed Apr 17, 2024
1 parent 5a5b501 commit 2b78854
Show file tree
Hide file tree
Showing 13 changed files with 575 additions and 13 deletions.
81 changes: 81 additions & 0 deletions Sources/SKCore/Debouncer.swift
Original file line number Diff line number Diff line change
@@ -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(())
}
}
52 changes: 52 additions & 0 deletions Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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()
}

Expand Down Expand Up @@ -368,6 +391,35 @@ 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. Currently, this does not matter
// because `SwiftLanguageService` sends a `dependencyUpdated` request to sourcekitd, which is global anyway.
// 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 {
Expand Down
27 changes: 27 additions & 0 deletions Sources/SKTestSupport/FileManager+findFiles.swift
Original file line number Diff line number Diff line change
@@ -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
}
}
9 changes: 9 additions & 0 deletions Sources/SKTestSupport/MultiFileTestProject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
1 change: 1 addition & 0 deletions Sources/SKTestSupport/SkipUnless.swift
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,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")
Expand Down
7 changes: 6 additions & 1 deletion Sources/SKTestSupport/SwiftPMTestProject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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())
Expand Down
3 changes: 1 addition & 2 deletions Sources/SKTestSupport/TestSourceKitLSPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down
3 changes: 3 additions & 0 deletions Sources/SourceKitLSP/SourceKitLSPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1373,6 +1373,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)
}

Expand Down
44 changes: 35 additions & 9 deletions Sources/SourceKitLSP/Swift/SwiftLanguageService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -339,13 +363,15 @@ extension SwiftLanguageService {
}

public func documentDependenciesUpdated(_ uri: DocumentURI) async {
guard let snapshot = try? self.documentManager.latestSnapshot(uri) else {
return
// FIXME: Debounce, otherwise it will get called over and over
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
Expand Down Expand Up @@ -845,7 +871,7 @@ extension SwiftLanguageService {
// Instead of returning an error, return empty results.
logger.error(
"""
Loading diagnostic failed with the following error. Returning empty diagnostics.
Loading diagnostic failed with the following error. Returning empty diagnostics.
\(error.forLogging)
"""
)
Expand Down
41 changes: 41 additions & 0 deletions Tests/SKCoreTests/DebouncerTests.swift
Original file line number Diff line number Diff line change
@@ -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])
}
}
Loading

0 comments on commit 2b78854

Please sign in to comment.