Skip to content

Don’t implicitly cancel code completion requests on document edits #1857

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
32 changes: 24 additions & 8 deletions Sources/SourceKitLSP/SourceKitLSPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,9 @@ package actor SourceKitLSPServer {
}
}

/// For all currently handled text document requests a mapping from the document to the corresponding request ID.
private var inProgressTextDocumentRequests: [DocumentURI: Set<RequestID>] = [:]
/// For all currently handled text document requests a mapping from the document to the corresponding request ID and
/// the method of the request (ie. the value of `TextDocumentRequest.method`).
private var inProgressTextDocumentRequests: [DocumentURI: [(id: RequestID, requestMethod: String)]] = [:]

var onExit: () -> Void

Expand Down Expand Up @@ -550,18 +551,26 @@ package actor SourceKitLSPServer {
// MARK: - MessageHandler

extension SourceKitLSPServer: QueueBasedMessageHandler {
private enum ImplicitTextDocumentRequestCancellationReason {
case documentChanged
case documentClosed
}

package nonisolated func didReceive(notification: some NotificationType) {
let textDocumentUri: DocumentURI
let cancellationReason: ImplicitTextDocumentRequestCancellationReason
switch notification {
case let params as DidChangeTextDocumentNotification:
textDocumentUri = params.textDocument.uri
cancellationReason = .documentChanged
case let params as DidCloseTextDocumentNotification:
textDocumentUri = params.textDocument.uri
cancellationReason = .documentClosed
default:
return
}
textDocumentTrackingQueue.async(priority: .high) {
await self.cancelTextDocumentRequests(for: textDocumentUri)
await self.cancelTextDocumentRequests(for: textDocumentUri, reason: cancellationReason)
}
}

Expand All @@ -582,11 +591,18 @@ extension SourceKitLSPServer: QueueBasedMessageHandler {
///
/// - Important: Should be invoked on `textDocumentTrackingQueue` to ensure that new text document requests are
/// registered before a notification that triggers cancellation might come in.
private func cancelTextDocumentRequests(for uri: DocumentURI) {
private func cancelTextDocumentRequests(for uri: DocumentURI, reason: ImplicitTextDocumentRequestCancellationReason) {
guard self.options.cancelTextDocumentRequestsOnEditAndCloseOrDefault else {
return
}
for requestID in self.inProgressTextDocumentRequests[uri, default: []] {
for (requestID, requestMethod) in self.inProgressTextDocumentRequests[uri, default: []] {
if reason == .documentChanged && requestMethod == CompletionRequest.method {
// As the user types, we filter the code completion results. Cancelling the completion request on every
// keystroke means that we will never build the initial list of completion results for this code
// completion session if building that list takes longer than the user's typing cadence (eg. for global
// completions) and we will thus not show any completions.
continue
}
logger.info("Implicitly cancelling request \(requestID)")
self.messageHandlingHelper.cancelRequest(id: requestID)
}
Expand Down Expand Up @@ -633,8 +649,8 @@ extension SourceKitLSPServer: QueueBasedMessageHandler {

/// - Important: Should be invoked on `textDocumentTrackingQueue` to ensure that new text document requests are
/// registered before a notification that triggers cancellation might come in.
private func registerInProgressTextDocumentRequest(_ request: any TextDocumentRequest, id: RequestID) {
self.inProgressTextDocumentRequests[request.textDocument.uri, default: []].insert(id)
private func registerInProgressTextDocumentRequest<T: TextDocumentRequest>(_ request: T, id: RequestID) {
self.inProgressTextDocumentRequests[request.textDocument.uri, default: []].append((id: id, requestMethod: T.method))
}

package func handle<Request: RequestType>(
Expand All @@ -644,7 +660,7 @@ extension SourceKitLSPServer: QueueBasedMessageHandler {
) async {
defer {
if let request = params as? any TextDocumentRequest {
self.inProgressTextDocumentRequests[request.textDocument.uri, default: []].remove(id)
self.inProgressTextDocumentRequests[request.textDocument.uri, default: []].removeAll { $0.id == id }
}
}

Expand Down
10 changes: 0 additions & 10 deletions Sources/SwiftExtensions/AsyncQueue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,6 @@ package final class AsyncQueue<TaskMetadata: DependencyTracker>: Sendable {

package init() {}

package func cancelTasks(where filter: (TaskMetadata) -> Bool) {
pendingTasks.withLock { pendingTasks in
for task in pendingTasks {
if filter(task.metadata) {
task.task.cancel()
}
}
}
}

/// Schedule a new closure to be executed on the queue.
///
/// If this is a serial queue, all previously added tasks are guaranteed to
Expand Down