Skip to content

Don’t cancel in-progress diagnostic generation when calling DiagnosticReportManager.removeItemsFromCache #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/LanguageServerProtocol/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ add_library(LanguageServerProtocol STATIC
Notifications/LogMessageNotification.swift
Notifications/LogTraceNotification.swift
Notifications/PublishDiagnosticsNotification.swift
Notifications/ReopenTextDocumentNotifications.swift
Notifications/SetTraceNotification.swift
Notifications/ShowMessageNotification.swift
Notifications/TextSynchronizationNotifications.swift
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//

/// Re-open the given document, discarding any in-memory state.
///
/// This notification is designed to be used internally in SourceKit-LSP: When build setting have changed, we re-open
/// the document in sourcekitd to re-build its AST. This needs to be handled via a notification to ensure that no other
/// request for this document is executing at the same time.
///
/// **(LSP Extension)**
public struct ReopenTextDocumentNotification: NotificationType, Hashable {
public static let method: String = "textDocument/reopen"

/// The document identifier and initial contents.
public var textDocument: TextDocumentIdentifier

public init(textDocument: TextDocumentIdentifier) {
self.textDocument = textDocument
}
}
2 changes: 2 additions & 0 deletions Sources/SourceKitLSP/Clang/ClangLanguageService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,8 @@ extension ClangLanguageService {
clangd.send(notification)
}

func reopenDocument(_ notification: ReopenTextDocumentNotification) {}

public func changeDocument(_ notification: DidChangeTextDocumentNotification) {
clangd.send(notification)
}
Expand Down
8 changes: 8 additions & 0 deletions Sources/SourceKitLSP/LanguageService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ public protocol LanguageService: AnyObject, Sendable {

/// Sent to close a document on the Language Server.
func closeDocument(_ notification: DidCloseTextDocumentNotification) async

/// Re-open the given document, discarding any in-memory state and forcing an AST to be re-built after build settings
/// have been changed. This needs to be handled via a notification to ensure that no other request for this document
/// is executing at the same time.
///
/// Only intended for `SwiftLanguageService`.
func reopenDocument(_ notification: ReopenTextDocumentNotification) async

func changeDocument(_ notification: DidChangeTextDocumentNotification) async
func willSaveDocument(_ notification: WillSaveTextDocumentNotification) async
func didSaveDocument(_ notification: DidSaveTextDocumentNotification) async
Expand Down
2 changes: 2 additions & 0 deletions Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ enum MessageHandlingDependencyTracker: DependencyTracker {
self = .freestanding
case is PublishDiagnosticsNotification:
self = .freestanding
case let notification as ReopenTextDocumentNotification:
self = .documentUpdate(notification.textDocument.uri)
case is SetTraceNotification:
self = .globalConfigurationChange
case is ShowMessageNotification:
Expand Down
13 changes: 13 additions & 0 deletions Sources/SourceKitLSP/SourceKitLSPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,8 @@ extension SourceKitLSPServer: MessageHandler {
await self.openDocument(notification)
case let notification as DidCloseTextDocumentNotification:
await self.closeDocument(notification)
case let notification as ReopenTextDocumentNotification:
await self.reopenDocument(notification)
case let notification as DidChangeTextDocumentNotification:
await self.changeDocument(notification)
case let notification as DidChangeWorkspaceFoldersNotification:
Expand Down Expand Up @@ -1300,6 +1302,17 @@ extension SourceKitLSPServer {
await self.closeDocument(notification, workspace: workspace)
}

func reopenDocument(_ notification: ReopenTextDocumentNotification) async {
let uri = notification.textDocument.uri
guard let workspace = await workspaceForDocument(uri: uri) else {
logger.error(
"received reopen notification for file '\(uri.forLogging)' without a corresponding workspace, ignoring..."
)
return
}
await workspace.documentService.value[uri]?.reopenDocument(notification)
}

func closeDocument(_ notification: DidCloseTextDocumentNotification, workspace: Workspace) async {
// Immediately close the document. We need to be sure to clear our pending work queue in case
// the build system still isn't ready.
Expand Down
4 changes: 0 additions & 4 deletions Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,7 @@ actor DiagnosticReportManager {
}

func removeItemsFromCache(with uri: DocumentURI) async {
let tasksToCancel = reportTaskCache.filter { $0.snapshotID.uri == uri }
reportTaskCache.removeAll(where: { $0.snapshotID.uri == uri })
for task in tasksToCancel {
await task.reportTask.cancel()
}
Comment on lines -89 to -93
Copy link
Contributor

@hamishknight hamishknight Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could still do the cancellation on close/open (just not re-open), would that be beneficial?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There wouldn’t be any point. The close and open are classified as MessageHandlingDependencyTracker.documentUpdate, so they will only execute after the diagnostic requests have finished.

Also, I don’t like implicit cancellation and think that the client should explicitly cancel a request if they no longer need it.

}

private func requestReport(
Expand Down
30 changes: 18 additions & 12 deletions Sources/SourceKitLSP/Swift/SwiftLanguageService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -327,15 +327,24 @@ extension SwiftLanguageService {

// MARK: - Build System Integration

private func reopenDocument(_ snapshot: DocumentSnapshot, _ compileCmd: SwiftCompileCommand?) async {
public func reopenDocument(_ notification: ReopenTextDocumentNotification) async {
let snapshot = orLog("Getting snapshot to re-open document") {
try documentManager.latestSnapshot(notification.textDocument.uri)
}
guard let snapshot else {
return
}
cancelInFlightPublishDiagnosticsTask(for: snapshot.uri)
await diagnosticReportManager.removeItemsFromCache(with: snapshot.uri)

let closeReq = closeDocumentSourcekitdRequest(uri: snapshot.uri)
_ = try? await self.sourcekitd.send(closeReq, fileContents: nil)
_ = await orLog("Closing document to re-open it") { try await self.sourcekitd.send(closeReq, fileContents: nil) }

let openReq = openDocumentSourcekitdRequest(snapshot: snapshot, compileCommand: compileCmd)
_ = try? await self.sourcekitd.send(openReq, fileContents: snapshot.text)
let openReq = openDocumentSourcekitdRequest(
snapshot: snapshot,
compileCommand: await buildSettings(for: snapshot.uri)
)
_ = await orLog("Re-opening document") { try await self.sourcekitd.send(openReq, fileContents: snapshot.text) }

if await capabilityRegistry.clientSupportsPullDiagnostics(for: .swift) {
await self.refreshDiagnosticsDebouncer.scheduleCall()
Expand All @@ -345,14 +354,11 @@ extension SwiftLanguageService {
}

public func documentUpdatedBuildSettings(_ uri: DocumentURI) async {
// We may not have a snapshot if this is called just before `openDocument`.
guard let snapshot = try? self.documentManager.latestSnapshot(uri) else {
return
}

// Close and re-open the document internally to inform sourcekitd to update the compile
// command. At the moment there's no better way to do this.
await self.reopenDocument(snapshot, await self.buildSettings(for: uri))
// Close and re-open the document internally to inform sourcekitd to update the compile command. At the moment
// there's no better way to do this.
// Schedule the document re-open in the SourceKit-LSP server. This ensures that the re-open happens exclusively with
// no other request running at the same time.
sourceKitLSPServer?.handle(ReopenTextDocumentNotification(textDocument: TextDocumentIdentifier(uri)))
}

public func documentDependenciesUpdated(_ uri: DocumentURI) async {
Expand Down