Use build/taskStart, build/taskProgress and build/taskFinish to communicate progress from a BSP server to the client

Instead of defining BSP extensions for `window/workDoneProgress/create` and `$/progress`, we should be able to use the standard `build/taskStart`, `build/taskProgress` and `build/taskFinish` messages to the same effect, as suggested by https://forums.swift.org/t/extending-functionality-of-build-server-protocol-with-sourcekit-lsp/74400/9.

Fixes #1783
rdar://138653131

Author: ahoppen

Contributor Documentation/BSP Extensions.md

@@ -35,6 +35,10 @@ export interface SourceKitInitializeBuildResponseData {
 }
 ```
 
+## `build/taskStart`
+
+If `data` contains a string value for the `workDoneProgressTitle` key, then the task's message will be displayed in the client as a work done progress with that title.
+
 ## `buildTarget/didChange`
 
 `changes` can be `null` to indicate that all targets have changed.
@@ -119,12 +123,6 @@ export interface TextDocumentSourceKitOptionsResult {
 }
 ```
 
-## `window/workDoneProgress/create`
-
-Request from the build server to SourceKit-LSP to create a work done progress.
-
-Definition is the same as in LSP.
-
 ## `workspace/buildTargets`
 
 `BuildTargetTag` can have the following additional values
@@ -172,9 +170,3 @@ This request is a no-op and doesn't have any effects.
 If the build system is currently updating the build graph, this request should return after those updates have finished processing.
 
 - method: `workspace/waitForBuildSystemUpdates`
-
-## `$/progress`
-
-Notification from the build server to SourceKit-LSP to update a work done progress created using `window/workDoneProgress/create`.
-
-Definition is the same as in LSP.

Contributor Documentation/Implementing a BSP server.md

@@ -27,7 +27,6 @@ If the build system does not have a notion of targets, eg. because it provides b
 
 If the build system loads the entire build graph during initialization, it may immediately return from `workspace/waitForBuildSystemUpdates`.
 
-
 ## Supporting background indexing
 
 To support background indexing, the build system must set `data.prepareProvider: true` in the `build/initialize` response and implement the `buildTarget/prepare` method.
@@ -37,9 +36,8 @@ To support background indexing, the build system must set `data.prepareProvider:
 The following methods are not necessary to implement for SourceKit-LSP to work but might help with the implementation of the build server.
 
 - `build/logMessage`
-- `window/workDoneProgress/create`
+- `build/taskStart`, `build/taskProgress`, and `build/taskFinish`
 - `workspace/didChangeWatchedFiles`
-- `$/progress`
 
 ## Build server discovery
 

Sources/BuildServerProtocol/CMakeLists.txt

@@ -1,25 +1,29 @@
 add_library(BuildServerProtocol STATIC
   Messages.swift
 
-  Messages/TextDocumentSourceKitOptionsRequest.swift
-  Messages/OnBuildTargetDidChangeNotification.swift
-  Messages/InitializeBuildRequest.swift
+  Messages/BuildShutdownRequest.swift
+  Messages/BuildTargetPrepareRequest.swift
   Messages/BuildTargetSourcesRequest.swift
+  Messages/InitializeBuildRequest.swift
   Messages/OnBuildExitNotification.swift
-  Messages/RegisterForChangeNotifications.swift
+  Messages/OnBuildInitializedNotification.swift
   Messages/OnBuildLogMessageNotification.swift
-  Messages/WorkDoneProgress.swift
+  Messages/OnBuildTargetDidChangeNotification.swift
   Messages/OnWatchedFilesDidChangeNotification.swift
-  Messages/OnBuildInitializedNotification.swift
-  Messages/WorkspaceWaitForBuildSystemUpdates.swift
-  Messages/BuildShutdownRequest.swift
+  Messages/RegisterForChangeNotifications.swift
+  Messages/TaskFinishNotification.swift
+  Messages/TaskProgressNotification.swift
+  Messages/TaskStartNotification.swift
+  Messages/TextDocumentSourceKitOptionsRequest.swift
   Messages/WorkspaceBuildTargetsRequest.swift
-  Messages/BuildTargetPrepareRequest.swift
+  Messages/WorkspaceWaitForBuildSystemUpdates.swift
 
-  SupportTypes/TextDocumentIdentifier.swift
-  SupportTypes/TaskId.swift
   SupportTypes/BuildTarget.swift
-  SupportTypes/MessageType.swift)
+  SupportTypes/MessageType.swift
+  SupportTypes/MillisecondsSince1970Date.swift
+  SupportTypes/StatusCode.swift
+  SupportTypes/TaskId.swift
+  SupportTypes/TextDocumentIdentifier.swift)
 set_target_properties(BuildServerProtocol PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 target_link_libraries(BuildServerProtocol PRIVATE

Sources/BuildServerProtocol/Messages/TaskStartNotification.swift

@@ -137,3 +137,27 @@ public struct TestTaskData: Codable, Hashable, Sendable {
     self.target = target
   }
 }
+
+/// If `data` contains a string value for the `workDoneProgressTitle` key, then the task's message will be displayed in
+/// the client as a work done progress with that title.
+public struct WorkDoneProgressTask: LSPAnyCodable {
+  /// The title with which the work done progress should be created in the client.
+  public let title: String
+
+  public init(title: String) {
+    self.title = title
+  }
+
+  public init?(fromLSPDictionary dictionary: [String: LanguageServerProtocol.LSPAny]) {
+    guard case .string(let title) = dictionary["workDoneProgressTitle"] else {
+      return nil
+    }
+    self.title = title
+  }
+
+  public func encodeToLSPAny() -> LanguageServerProtocol.LSPAny {
+    return .dictionary([
+      "workDoneProgressTitle": .string(title)
+    ])
+  }
+}

Sources/BuildServerProtocol/Messages/WorkDoneProgress.swift

@@ -14,11 +14,11 @@ import LanguageServerProtocol
 
 public typealias TaskIdentifier = String
 
-public struct TaskId: Sendable, Codable {
+public struct TaskId: Sendable, Codable, Hashable {
   /// A unique identifier
   public var id: TaskIdentifier
 
-  ///  The parent task ids, if any. A non-empty parents field means
+  /// The parent task ids, if any. A non-empty parents field means
   /// this task is a sub-task of every parent task id. The child-parent
   /// relationship of tasks makes it possible to render tasks in
   /// a tree-like user interface or inspect what caused a certain task

Sources/BuildServerProtocol/SupportTypes/TaskId.swift

@@ -78,19 +78,19 @@ private struct BuildTargetInfo {
 
 fileprivate extension SourceItem {
   var sourceKitData: SourceKitSourceItemData? {
-    guard dataKind == .sourceKit, case .dictionary(let data) = data else {
+    guard dataKind == .sourceKit else {
       return nil
     }
-    return SourceKitSourceItemData(fromLSPDictionary: data)
+    return SourceKitSourceItemData(fromLSPAny: data)
   }
 }
 
 fileprivate extension BuildTarget {
   var sourceKitData: SourceKitBuildTarget? {
-    guard dataKind == .sourceKit, case .dictionary(let data) = data else {
+    guard dataKind == .sourceKit else {
       return nil
     }
-    return SourceKitBuildTarget(fromLSPDictionary: data)
+    return SourceKitBuildTarget(fromLSPAny: data)
   }
 }
 
@@ -99,10 +99,7 @@ fileprivate extension InitializeBuildResponse {
     guard dataKind == nil || dataKind == .sourceKit else {
       return nil
     }
-    guard case .dictionary(let data) = data else {
-      return nil
-    }
-    return SourceKitInitializeBuildResponseData(fromLSPDictionary: data)
+    return SourceKitInitializeBuildResponseData(fromLSPAny: data)
   }
 }
 
@@ -250,7 +247,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   /// get `fileBuildSettingsChanged` and `filesDependenciesUpdated` callbacks.
   private var watchedFiles: [DocumentURI: (mainFile: DocumentURI, language: Language)] = [:]
 
-  private var connectionToClient: BuildSystemManagerConnectionToClient?
+  private var connectionToClient: BuildSystemManagerConnectionToClient
 
   /// The build system adapter that is used to answer build system queries.
   private var buildSystemAdapter: BuildSystemAdapter?
@@ -302,6 +299,10 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
     }
   }
 
+  /// For tasks from the build system that should create a work done progress in the client, a mapping from the `TaskId`
+  /// in the build system to a `WorkDoneProgressManager` that manages that work done progress in the client.
+  private var workDoneProgressManagers: [TaskIdentifier: WorkDoneProgressManager] = [:]
+
   /// 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
@@ -443,6 +444,8 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   /// which could result in the connection being reported as a leak. To avoid this problem, we want to explicitly shut
   /// down the build server when the `SourceKitLSPServer` gets shut down.
   package func shutdown() async {
+    // Clear any pending work done progresses from the build server.
+    self.workDoneProgressManagers.removeAll()
     guard let buildSystemAdapter = await self.buildSystemAdapterAfterInitialized else {
       return
     }
@@ -488,8 +491,12 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       await self.didChangeBuildTarget(notification: notification)
     case let notification as OnBuildLogMessageNotification:
       await self.logMessage(notification: notification)
-    case let notification as BuildServerProtocol.WorkDoneProgress:
-      await self.workDoneProgress(notification: notification)
+    case let notification as TaskFinishNotification:
+      await self.taskFinish(notification: notification)
+    case let notification as TaskProgressNotification:
+      await self.taskProgress(notification: notification)
+    case let notification as TaskStartNotification:
+      await self.taskStart(notification: notification)
     default:
       logger.error("Ignoring unknown notification \(type(of: notification).method)")
     }
@@ -502,8 +509,6 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   ) async {
     let request = RequestAndReply(request, reply: reply)
     switch request {
-    case let request as RequestAndReply<BuildServerProtocol.CreateWorkDoneProgressRequest>:
-      await request.reply { try await self.createWorkDoneProgress(request: request.params) }
     default:
       await request.reply { throw ResponseError.methodNotFound(Request.method) }
     }
@@ -544,29 +549,51 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       } else {
         notification.message
       }
-    await connectionToClient?.send(
+    await connectionToClient.waitUntilInitialized()
+    connectionToClient.send(
       LanguageServerProtocol.LogMessageNotification(type: .info, message: message, logName: "SourceKit-LSP: Indexing")
     )
   }
 
-  private func workDoneProgress(notification: BuildServerProtocol.WorkDoneProgress) async {
-    guard let connectionToClient else {
-      logger.fault("Ignoring work done progress from build system because connection to client closed")
+  private func taskStart(notification: TaskStartNotification) async {
+    guard let workDoneProgressTitle = WorkDoneProgressTask(fromLSPAny: notification.data)?.title,
+      await connectionToClient.clientSupportsWorkDoneProgress
+    else {
+      return
+    }
+
+    guard workDoneProgressManagers[notification.taskId.id] == nil else {
+      logger.error("Client is already tracking a work done progress for task \(notification.taskId.id)")
       return
     }
-    await connectionToClient.send(notification as LanguageServerProtocol.WorkDoneProgress)
+    workDoneProgressManagers[notification.taskId.id] = WorkDoneProgressManager(
+      connectionToClient: connectionToClient,
+      waitUntilClientInitialized: connectionToClient.waitUntilInitialized,
+      tokenPrefix: notification.taskId.id,
+      initialDebounce: options.workDoneProgressDebounceDurationOrDefault,
+      title: workDoneProgressTitle
+    )
   }
 
-  private func createWorkDoneProgress(
-    request: BuildServerProtocol.CreateWorkDoneProgressRequest
-  ) async throws -> BuildServerProtocol.CreateWorkDoneProgressRequest.Response {
-    guard let connectionToClient else {
-      throw ResponseError.unknown("Connection to client closed")
+  private func taskProgress(notification: TaskProgressNotification) async {
+    guard let progressManager = workDoneProgressManagers[notification.taskId.id] else {
+      return
     }
-    guard await connectionToClient.clientSupportsWorkDoneProgress else {
-      throw ResponseError.unknown("Client does not support work done progress")
+    let percentage: Int? =
+      if let progress = notification.progress, let total = notification.total {
+        Int((Double(progress) / Double(total) * 100).rounded())
+      } else {
+        nil
+      }
+    await progressManager.update(message: notification.message, percentage: percentage)
+  }
+
+  private func taskFinish(notification: TaskFinishNotification) async {
+    guard let progressManager = workDoneProgressManagers[notification.taskId.id] else {
+      return
     }
-    return try await connectionToClient.send(request as LanguageServerProtocol.CreateWorkDoneProgressRequest)
+    await progressManager.end()
+    workDoneProgressManagers[notification.taskId.id] = nil
   }
 
   // MARK: Build System queries

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -42,13 +42,14 @@ package protocol BuildSystemManagerDelegate: AnyObject, Sendable {
 /// This is distinct from `BuildSystemManagerDelegate` because the delegate only gets set on the build system after the
 /// workspace that created it has been initialized (see `BuildSystemManager.setDelegate`). But the `BuildSystemManager`
 /// can send notifications to the client immediately.
-package protocol BuildSystemManagerConnectionToClient: Sendable {
+package protocol BuildSystemManagerConnectionToClient: Sendable, Connection {
   /// Whether the client can handle `WorkDoneProgress` requests.
   var clientSupportsWorkDoneProgress: Bool { get async }
 
-  func send(_ notification: some NotificationType) async
-
-  func send<R: RequestType>(_ request: R) async throws -> R.Response
+  /// Wait until the connection to the client has been initialized.
+  ///
+  /// No messages should be sent on this connection before this returns.
+  func waitUntilInitialized() async
 
   /// Start watching for file changes with the given glob patterns.
   func watchFiles(_ fileWatchers: [FileSystemWatcher]) async

Sources/BuildSystemIntegration/BuildSystemManagerDelegate.swift

@@ -36,7 +36,7 @@ package enum BuildSystemMessageDependencyTracker: QueueBasedMessageHandlerDepend
 
   /// A task that is responsible for logging information to the client. They can be run concurrently to any state read
   /// and changes but logging tasks must be ordered among each other.
-  case logging
+  case taskProgress
 
   /// Whether this request needs to finish before `other` can start executing.
   package func isDependency(of other: BuildSystemMessageDependencyTracker) -> Bool {
@@ -45,9 +45,9 @@ package enum BuildSystemMessageDependencyTracker: QueueBasedMessageHandlerDepend
     case (.stateChange, .stateRead): return true
     case (.stateRead, .stateChange): return true
     case (.stateRead, .stateRead): return false
-    case (.logging, .logging): return true
-    case (.logging, _): return false
-    case (_, .logging): return false
+    case (.taskProgress, .taskProgress): return true
+    case (.taskProgress, _): return false
+    case (_, .taskProgress): return false
     }
   }
 
@@ -60,7 +60,7 @@ package enum BuildSystemMessageDependencyTracker: QueueBasedMessageHandlerDepend
     case is OnBuildInitializedNotification:
       self = .stateChange
     case is OnBuildLogMessageNotification:
-      self = .logging
+      self = .taskProgress
     case is OnBuildTargetDidChangeNotification:
       self = .stateChange
     case is OnWatchedFilesDidChangeNotification:
@@ -84,8 +84,8 @@ package enum BuildSystemMessageDependencyTracker: QueueBasedMessageHandlerDepend
       self = .stateRead
     case is BuildTargetSourcesRequest:
       self = .stateRead
-    case is BuildServerProtocol.CreateWorkDoneProgressRequest:
-      self = .logging
+    case is TaskStartNotification, is TaskProgressNotification, is TaskFinishNotification:
+      self = .taskProgress
     case is InitializeBuildRequest:
       self = .stateChange
     case is RegisterForChanges:

Sources/BuildSystemIntegration/BuildSystemMessageDependencyTracker.swift

@@ -394,17 +394,18 @@ package actor SwiftPMBuildSystem: BuiltInBuildSystem {
   ///
   /// - Important: Must only be called on `packageLoadingQueue`.
   private func reloadPackageAssumingOnPackageLoadingQueue() async throws {
-    let progressManager = WorkDoneProgressManager(
-      connectionToClient: self.connectionToSourceKitLSP,
-      waitUntilClientInitialized: {},
-      tokenPrefix: "package-reloading",
-      initialDebounce: options.workDoneProgressDebounceDurationOrDefault,
-      title: "SourceKit-LSP: Reloading Package"
+    self.connectionToSourceKitLSP.send(
+      TaskStartNotification(
+        taskId: TaskId(id: "package-reloading"),
+        data: WorkDoneProgressTask(title: "SourceKit-LSP: Reloading Package").encodeToLSPAny()
+      )
     )
     await testHooks.reloadPackageDidStart?()
     defer {
       Task {
-        await progressManager?.end()
+        self.connectionToSourceKitLSP.send(
+          TaskFinishNotification(taskId: TaskId(id: "package-reloading"), status: .ok)
+        )
         await testHooks.reloadPackageDidFinish?()
       }
     }