Merge pull request #1659 from ahoppen/more-bsp

Migrate `fileHandlingCapability` and `prepare` to BSP and remove `registerForChangeNotifications`

Author: ahoppen

Sources/BuildServerProtocol/CMakeLists.txt

@@ -4,7 +4,9 @@ add_library(BuildServerProtocol STATIC
   DidChangeWatchedFilesNotification.swift
   InitializeBuild.swift
   InverseSourcesRequest.swift
+  LogMessageNotification.swift
   Messages.swift
+  PrepareTargetsRequest.swift
   RegisterForChangeNotifications.swift
   ShutdownBuild.swift
   SourceKitOptionsRequest.swift)

Sources/BuildServerProtocol/LogMessageNotification.swift

@@ -0,0 +1,70 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 LanguageServerProtocol
+
+public enum LogMessageType: Int, Sendable, Codable {
+  /// An error message.
+  case error = 1
+
+  /// A warning message.
+  case warning = 2
+
+  /// An information message.
+  case info = 3
+
+  /// A log message.
+  case log = 4
+}
+
+public typealias TaskIdentifier = String
+
+public struct TaskId: Sendable, Codable {
+  /// A unique identifier
+  public var id: TaskIdentifier
+
+  ///  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
+  /// execution.
+  /// OriginId should not be included in the parents field, there is a separate
+  /// field for that.
+  public var parents: [TaskIdentifier]?
+
+  public init(id: TaskIdentifier, parents: [TaskIdentifier]? = nil) {
+    self.id = id
+    self.parents = parents
+  }
+}
+
+/// The log message notification is sent from a server to a client to ask the client to log a particular message in its console.
+///
+/// A `build/logMessage`` notification is similar to LSP's `window/logMessage``, except for a few additions like id and originId.
+public struct LogMessageNotification: NotificationType {
+  public static let method: String = "build/logMessage"
+
+  /// The message type.
+  public var type: LogMessageType
+
+  /// The task id if any.
+  public var task: TaskId?
+
+  /// The actual message.
+  public var message: String
+
+  public init(type: LogMessageType, task: TaskId? = nil, message: String) {
+    self.type = type
+    self.task = task
+    self.message = message
+  }
+}

Sources/BuildServerProtocol/PrepareTargetsRequest.swift

@@ -0,0 +1,30 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 LanguageServerProtocol
+
+public typealias OriginId = String
+
+public struct PrepareTargetsRequest: RequestType, Hashable {
+  public static let method: String = "buildTarget/prepare"
+  public typealias Response = VoidResponse
+
+  /// A list of build targets to prepare.
+  public var targets: [BuildTargetIdentifier]
+
+  public var originId: OriginId?
+
+  public init(targets: [BuildTargetIdentifier], originId: OriginId? = nil) {
+    self.targets = targets
+    self.originId = originId
+  }
+}

Sources/BuildServerProtocol/RegisterForChangeNotifications.swift

@@ -9,6 +9,7 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import LanguageServerProtocol
 
 /// The register for changes request is sent from the language

Sources/BuildSystemIntegration/BuildServerBuildSystem.swift

@@ -80,6 +80,8 @@ package actor BuildServerBuildSystem: MessageHandler {
   /// The build settings that have been received from the build server.
   private var buildSettings: [DocumentURI: SourceKitOptionsResponse] = [:]
 
+  private var urisRegisteredForChanges: Set<URI> = []
+
   package init(
     projectRoot: AbsolutePath,
     messageHandler: BuiltInBuildSystemMessageHandler?,
@@ -269,7 +271,38 @@ extension BuildServerBuildSystem: BuiltInBuildSystem {
   package nonisolated var supportsPreparation: Bool { false }
 
   package func sourceKitOptions(request: SourceKitOptionsRequest) async throws -> SourceKitOptionsResponse? {
-    return buildSettings[request.textDocument.uri]
+    // FIXME: (BSP Migration) If the BSP server supports it, send the `SourceKitOptions` request to it. Only do the
+    // `RegisterForChanges` dance if we are in the legacy mode.
+
+    // Support the pre Swift 6.1 build settings workflow where SourceKit-LSP registers for changes for a file and then
+    // expects updates to those build settings to get pushed to SourceKit-LSP with `FileOptionsChangedNotification`.
+    // We do so by registering for changes when requesting build settings for a document for the first time. We never
+    // unregister for changes. The expectation is that all BSP servers migrate to the `SourceKitOptionsRequest` soon,
+    // which renders this code path dead.
+    let uri = request.textDocument.uri
+    if !urisRegisteredForChanges.contains(uri) {
+      let request = RegisterForChanges(uri: uri, action: .register)
+      _ = self.buildServer?.send(request) { result in
+        if let error = result.failure {
+          logger.error("Error registering \(request.uri): \(error.forLogging)")
+
+          Task {
+            // BuildServer registration failed, so tell our delegate that no build
+            // settings are available.
+            await self.buildSettingsChanged(for: request.uri, settings: nil)
+          }
+        }
+      }
+    }
+
+    guard let buildSettings = buildSettings[uri] else {
+      return nil
+    }
+
+    return SourceKitOptionsResponse(
+      compilerArguments: buildSettings.compilerArguments,
+      workingDirectory: buildSettings.workingDirectory
+    )
   }
 
   package func defaultLanguage(for document: DocumentURI) async -> Language? {
@@ -296,64 +329,12 @@ extension BuildServerBuildSystem: BuiltInBuildSystem {
     return nil
   }
 
-  package func prepare(
-    targets: [BuildTargetIdentifier],
-    logMessageToIndexLog: @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void
-  ) async throws {
+  package func prepare(request: PrepareTargetsRequest) async throws -> VoidResponse {
     throw PrepareNotSupportedError()
   }
 
-  package func registerForChangeNotifications(for uri: DocumentURI) {
-    let request = RegisterForChanges(uri: uri, action: .register)
-    _ = self.buildServer?.send(request) { result in
-      if let error = result.failure {
-        logger.error("Error registering \(uri): \(error.forLogging)")
-
-        Task {
-          // BuildServer registration failed, so tell our delegate that no build
-          // settings are available.
-          await self.buildSettingsChanged(for: uri, settings: nil)
-        }
-      }
-    }
-  }
-
-  /// Unregister the given file for build-system level change notifications, such as command
-  /// line flag changes, dependency changes, etc.
-  package func unregisterForChangeNotifications(for uri: DocumentURI) {
-    let request = RegisterForChanges(uri: uri, action: .unregister)
-    _ = self.buildServer?.send(request) { result in
-      if let error = result.failure {
-        logger.error("Error unregistering \(uri.forLogging): \(error.forLogging)")
-      }
-    }
-  }
-
   package func didChangeWatchedFiles(notification: BuildServerProtocol.DidChangeWatchedFilesNotification) {}
 
-  package func fileHandlingCapability(for uri: DocumentURI) -> FileHandlingCapability {
-    guard
-      let fileUrl = uri.fileURL,
-      let path = try? AbsolutePath(validating: fileUrl.path)
-    else {
-      return .unhandled
-    }
-
-    // TODO: We should not make any assumptions about which files the build server can handle.
-    // Instead we should query the build server which files it can handle
-    // (https://github.com/swiftlang/sourcekit-lsp/issues/492).
-
-    if projectRoot.isAncestorOfOrEqual(to: path) {
-      return .handled
-    }
-
-    if let realpath = try? resolveSymlinks(path), realpath != path, projectRoot.isAncestorOfOrEqual(to: realpath) {
-      return .handled
-    }
-
-    return .unhandled
-  }
-
   package func sourceFiles() async -> [SourceFileInfo] {
     // BuildServerBuildSystem does not support syntactic test discovery or background indexing.
     // (https://github.com/swiftlang/sourcekit-lsp/issues/1173).

Sources/BuildSystemIntegration/BuildSystemDelegate.swift

@@ -22,11 +22,6 @@ package protocol BuildSystemDelegate: AnyObject, Sendable {
   /// The callee should refresh ASTs unless it is able to determine that a
   /// refresh is not necessary.
   func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async
-
-  /// Notify the delegate that the file handling capability of this build system
-  /// for some file has changed. The delegate should discard any cached file
-  /// handling capability.
-  func fileHandlingCapabilityChanged() async
 }
 
 /// Handles build system events, such as file build settings changes.
@@ -42,8 +37,10 @@ package protocol BuildSystemManagerDelegate: AnyObject, Sendable {
   /// refresh is not necessary.
   func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async
 
-  /// Notify the delegate that the file handling capability of this build system
-  /// for some file has changed. The delegate should discard any cached file
-  /// handling capability.
-  func fileHandlingCapabilityChanged() async
+  /// Notify the delegate that some information about the given build targets has changed and that it should recompute
+  /// any information based on top of it.
+  func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async
+
+  /// Log the given message to the client's index log.
+  func logMessageToIndexLog(taskID: IndexTaskID, message: String)
 }

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -81,7 +81,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
 
   /// The fallback build system. If present, used when the `buildSystem` is not
   /// set or cannot provide settings.
-  let fallbackBuildSystem: FallbackBuildSystem?
+  let fallbackBuildSystem: FallbackBuildSystem
 
   /// Provider of file to main file mappings.
   var mainFilesProvider: MainFilesProvider?
@@ -101,11 +101,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   /// The root of the project that this build system manages. For example, for SwiftPM packages, this is the folder
   /// containing Package.swift. For compilation databases it is the root folder based on which the compilation database
   /// was found.
-  package var projectRoot: AbsolutePath? {
-    get async {
-      return await buildSystem?.underlyingBuildSystem.projectRoot
-    }
-  }
+  package let projectRoot: AbsolutePath?
 
   package var supportsPreparation: Bool {
     get async {
@@ -122,6 +118,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   ) async {
     self.fallbackBuildSystem = FallbackBuildSystem(options: options.fallbackBuildSystemOrDefault)
     self.toolchainRegistry = toolchainRegistry
+    self.projectRoot = buildSystemKind?.projectRoot
     self.buildSystem = await BuiltInBuildSystemAdapter(
       buildSystemKind: buildSystemKind,
       toolchainRegistry: toolchainRegistry,
@@ -149,6 +146,8 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     switch notification {
     case let notification as DidChangeBuildTargetNotification:
       await self.didChangeBuildTarget(notification: notification)
+    case let notification as BuildServerProtocol.LogMessageNotification:
+      await self.logMessage(notification: notification)
     default:
       logger.error("Ignoring unknown notification \(type(of: notification).method)")
     }
@@ -315,7 +314,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
       logger.error("Getting build settings failed: \(error.forLogging)")
     }
 
-    guard var settings = await fallbackBuildSystem?.buildSettings(for: document, language: language) else {
+    guard var settings = await fallbackBuildSystem.buildSettings(for: document, language: language) else {
       return nil
     }
     if buildSystem == nil {
@@ -372,40 +371,17 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     targets: [BuildTargetIdentifier],
     logMessageToIndexLog: @escaping @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void
   ) async throws {
-    try await buildSystem?.underlyingBuildSystem.prepare(targets: targets, logMessageToIndexLog: logMessageToIndexLog)
+    let _: VoidResponse? = try await buildSystem?.send(PrepareTargetsRequest(targets: targets))
   }
 
   package func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
     logger.debug("registerForChangeNotifications(\(uri.forLogging))")
     let mainFile = await mainFile(for: uri, language: language)
     self.watchedFiles[uri] = (mainFile, language)
-
-    // Register for change notifications of the main file in the underlying build
-    // system. That way, iff the main file changes, we will also notify the
-    // delegate about build setting changes of all header files that are based
-    // on that main file.
-    await buildSystem?.underlyingBuildSystem.registerForChangeNotifications(for: mainFile)
   }
 
   package func unregisterForChangeNotifications(for uri: DocumentURI) async {
-    guard let mainFile = self.watchedFiles[uri]?.mainFile else {
-      logger.fault("Unbalanced calls for registerForChangeNotifications and unregisterForChangeNotifications")
-      return
-    }
     self.watchedFiles[uri] = nil
-
-    if watchedFilesReferencing(mainFiles: [mainFile]).isEmpty {
-      // Nobody is interested in this main file anymore.
-      // We are no longer interested in change notifications for it.
-      await self.buildSystem?.underlyingBuildSystem.unregisterForChangeNotifications(for: mainFile)
-    }
-  }
-
-  package func fileHandlingCapability(for uri: DocumentURI) async -> FileHandlingCapability {
-    return max(
-      await buildSystem?.underlyingBuildSystem.fileHandlingCapability(for: uri) ?? .unhandled,
-      fallbackBuildSystem != nil ? .fallback : .unhandled
-    )
   }
 
   package func sourceFiles() async -> [SourceFileInfo] {
@@ -451,12 +427,6 @@ extension BuildSystemManager: BuildSystemDelegate {
     }
   }
 
-  package func fileHandlingCapabilityChanged() async {
-    if let delegate = self.delegate {
-      await delegate.fileHandlingCapabilityChanged()
-    }
-  }
-
   private func didChangeBuildTarget(notification: DidChangeBuildTargetNotification) async {
     // Every `DidChangeBuildTargetNotification` notification needs to invalidate the cache since the changed target
     // might gained a source file.
@@ -476,10 +446,21 @@ extension BuildSystemManager: BuildSystemDelegate {
       return updatedTargets.contains(cacheKey.target)
     }
 
+    await delegate?.buildTargetsChanged(notification.changes)
     // FIXME: (BSP Migration) Communicate that the build target has changed to the `BuildSystemManagerDelegate` and make
     // it responsible for figuring out which files are affected.
     await delegate?.fileBuildSettingsChanged(Set(watchedFiles.keys))
   }
+
+  private func logMessage(notification: BuildServerProtocol.LogMessageNotification) async {
+    // FIXME: (BSP Integration) Remove the restriction that task IDs need to have a raw value that can be parsed by
+    // `IndexTaskID.init`.
+    guard let task = notification.task, let taskID = IndexTaskID(rawValue: task.id) else {
+      logger.error("Ignoring log message notification with unknown task \(notification.task?.id ?? "<nil>")")
+      return
+    }
+    delegate?.logMessageToIndexLog(taskID: taskID, message: notification.message)
+  }
 }
 
 extension BuildSystemManager {