Merge pull request #1655 from ahoppen/bsp-settings

Use BSP requests to get build settings of a source file

Author: ahoppen

Sources/BuildServerProtocol/CMakeLists.txt

@@ -2,12 +2,12 @@ add_library(BuildServerProtocol STATIC
   BuildTargets.swift
   DidChangeBuildTargetNotification.swift
   DidChangeWatchedFilesNotification.swift
-  FileOptions.swift
   InitializeBuild.swift
   InverseSourcesRequest.swift
   Messages.swift
   RegisterForChangeNotifications.swift
-  ShutdownBuild.swift)
+  ShutdownBuild.swift
+  SourceKitOptionsRequest.swift)
 set_target_properties(BuildServerProtocol PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 target_link_libraries(BuildServerProtocol PRIVATE

Sources/BuildServerProtocol/FileOptions.swift

@@ -19,7 +19,7 @@ fileprivate let requestTypes: [_RequestType.Type] = [
   InverseSourcesRequest.self,
   RegisterForChanges.self,
   ShutdownBuild.self,
-  SourceKitOptions.self,
+  SourceKitOptionsRequest.self,
 ]
 
 fileprivate let notificationTypes: [NotificationType.Type] = [

Sources/BuildServerProtocol/Messages.swift

@@ -40,11 +40,19 @@ public enum RegisterAction: String, Hashable, Codable, Sendable {
 /// build server to the language server when it detects
 /// changes to a registered files build settings.
 public struct FileOptionsChangedNotification: NotificationType {
+  public struct Options: ResponseType, Hashable {
+    /// The compiler options required for the requested file.
+    public var options: [String]
+
+    /// The working directory for the compile command.
+    public var workingDirectory: String?
+  }
+
   public static let method: String = "build/sourceKitOptionsChanged"
 
   /// The URI of the document that has changed settings.
   public var uri: URI
 
   /// The updated options for the registered file.
-  public var updatedOptions: SourceKitOptionsResult
+  public var updatedOptions: Options
 }

Sources/BuildServerProtocol/RegisterForChangeNotifications.swift

@@ -0,0 +1,52 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 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
+
+/// The SourceKitOptions request is sent from the client to the server to query for the list of compiler options
+/// necessary to compile this file in the given target.
+///
+/// The build settings are considered up-to-date and can be cached by SourceKit-LSP until a
+/// `DidChangeBuildTargetNotification` is sent for the requested target.
+///
+/// The request may return `nil` if it doesn't have any build settings for this file in the given target.
+public struct SourceKitOptionsRequest: RequestType, Hashable {
+  public static let method: String = "textDocument/sourceKitOptions"
+  public typealias Response = SourceKitOptionsResponse?
+
+  /// The URI of the document to get options for
+  public var textDocument: TextDocumentIdentifier
+
+  /// The target for which the build setting should be returned.
+  ///
+  /// A source file might be part of multiple targets and might have different compiler arguments in those two targets,
+  /// thus the target is necessary in this request.
+  public var target: BuildTargetIdentifier
+
+  public init(textDocument: TextDocumentIdentifier, target: BuildTargetIdentifier) {
+    self.textDocument = textDocument
+    self.target = target
+  }
+}
+
+public struct SourceKitOptionsResponse: ResponseType, Hashable {
+  /// The compiler options required for the requested file.
+  public var compilerArguments: [String]
+
+  /// The working directory for the compile command.
+  public var workingDirectory: String?
+
+  public init(compilerArguments: [String], workingDirectory: String? = nil) {
+    self.compilerArguments = compilerArguments
+    self.workingDirectory = workingDirectory
+  }
+}

Sources/BuildServerProtocol/SourceKitOptionsRequest.swift

@@ -78,7 +78,7 @@ package actor BuildServerBuildSystem: MessageHandler {
   package weak var messageHandler: BuiltInBuildSystemMessageHandler?
 
   /// The build settings that have been received from the build server.
-  private var buildSettings: [DocumentURI: FileBuildSettings] = [:]
+  private var buildSettings: [DocumentURI: SourceKitOptionsResponse] = [:]
 
   package init(
     projectRoot: AbsolutePath,
@@ -178,10 +178,10 @@ package actor BuildServerBuildSystem: MessageHandler {
     logger.log("Initialized build server \(response.displayName)")
 
     // see if index store was set as part of the server metadata
-    if let indexDbPath = readReponseDataKey(data: response.data, key: "indexDatabasePath") {
+    if let indexDbPath = readResponseDataKey(data: response.data, key: "indexDatabasePath") {
       self.indexDatabasePath = try AbsolutePath(validating: indexDbPath, relativeTo: self.projectRoot)
     }
-    if let indexStorePath = readReponseDataKey(data: response.data, key: "indexStorePath") {
+    if let indexStorePath = readResponseDataKey(data: response.data, key: "indexStorePath") {
       self.indexStorePath = try AbsolutePath(validating: indexStorePath, relativeTo: self.projectRoot)
     }
     self.buildServer = buildServer
@@ -228,11 +228,9 @@ package actor BuildServerBuildSystem: MessageHandler {
     await self.messageHandler?.sendNotificationToSourceKitLSP(notification)
   }
 
-  func handleFileOptionsChanged(
-    _ notification: FileOptionsChangedNotification
-  ) async {
+  func handleFileOptionsChanged(_ notification: FileOptionsChangedNotification) async {
     let result = notification.updatedOptions
-    let settings = FileBuildSettings(
+    let settings = SourceKitOptionsResponse(
       compilerArguments: result.options,
       workingDirectory: result.workingDirectory
     )
@@ -241,13 +239,16 @@ package actor BuildServerBuildSystem: MessageHandler {
 
   /// Record the new build settings for the given document and inform the delegate
   /// about the changed build settings.
-  private func buildSettingsChanged(for document: DocumentURI, settings: FileBuildSettings?) async {
+  private func buildSettingsChanged(for document: DocumentURI, settings: SourceKitOptionsResponse?) async {
     buildSettings[document] = settings
-    await self.delegate?.fileBuildSettingsChanged([document])
+    // FIXME: (BSP migration) When running in the legacy mode where teh BSP server pushes build settings to us, we could
+    // consider having a separate target for each source file so that we can update individual targets instead of having
+    // to send an update for all targets.
+    await self.messageHandler?.sendNotificationToSourceKitLSP(DidChangeBuildTargetNotification(changes: nil))
   }
 }
 
-private func readReponseDataKey(data: LSPAny?, key: String) -> String? {
+private func readResponseDataKey(data: LSPAny?, key: String) -> String? {
   if case .dictionary(let dataDict)? = data,
     case .string(let stringVal)? = dataDict[key]
   {
@@ -267,16 +268,8 @@ extension BuildServerBuildSystem: BuiltInBuildSystem {
 
   package nonisolated var supportsPreparation: Bool { false }
 
-  /// The build settings for the given file.
-  ///
-  /// Returns `nil` if no build settings have been received from the build
-  /// server yet or if no build settings are available for this file.
-  package func buildSettings(
-    for document: DocumentURI,
-    in target: BuildTargetIdentifier,
-    language: Language
-  ) async -> FileBuildSettings? {
-    return buildSettings[document]
+  package func sourceKitOptions(request: SourceKitOptionsRequest) async throws -> SourceKitOptionsResponse? {
+    return buildSettings[request.textDocument.uri]
   }
 
   package func defaultLanguage(for document: DocumentURI) async -> Language? {
@@ -287,7 +280,7 @@ extension BuildServerBuildSystem: BuiltInBuildSystem {
     return nil
   }
 
-  package func inverseSources(_ request: InverseSourcesRequest) -> InverseSourcesResponse {
+  package func inverseSources(request: InverseSourcesRequest) -> InverseSourcesResponse {
     return InverseSourcesResponse(targets: [BuildTargetIdentifier.dummy])
   }
 

Sources/BuildSystemIntegration/BuildServerBuildSystem.swift

@@ -15,9 +15,6 @@ import LanguageServerProtocol
 /// Handles build system events, such as file build settings changes.
 // FIXME: (BSP migration) The build system should exclusively communicate back to SourceKit-LSP using BSP and this protocol should be deleted.
 package protocol BuildSystemDelegate: AnyObject, Sendable {
-  /// Notify the delegate that the given files' build settings have changed.
-  func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async
-
   /// Notify the delegate that the dependencies of the given files have changed
   /// and that ASTs may need to be refreshed. If the given set is empty, assume
   /// that all watched files are affected.

Sources/BuildSystemIntegration/BuildSystemDelegate.swift

@@ -96,6 +96,8 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
 
   private var cachedTargetsForDocument = RequestCache<InverseSourcesRequest>()
 
+  private var cachedSourceKitOptions = RequestCache<SourceKitOptionsRequest>()
+
   /// 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.
@@ -112,7 +114,7 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
   }
 
   package init(
-    buildSystemKind: (WorkspaceType, projectRoot: AbsolutePath)?,
+    buildSystemKind: BuildSystemKind?,
     toolchainRegistry: ToolchainRegistry,
     options: SourceKitLSPOptions,
     swiftpmTestHooks: SwiftPMTestHooks,
@@ -131,41 +133,17 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     await self.buildSystem?.underlyingBuildSystem.setDelegate(self)
   }
 
-  /// Create a BuildSystemManager that wraps the given build system.
-  /// The new manager will modify the delegate of the underlying build system.
-  ///
-  /// - Important: For testing purposes only
-  package init(
-    testBuildSystem: BuiltInBuildSystem?,
-    fallbackBuildSystem: FallbackBuildSystem?,
-    mainFilesProvider: MainFilesProvider?,
-    toolchainRegistry: ToolchainRegistry
-  ) async {
-    let buildSystemHasDelegate = await testBuildSystem?.delegate != nil
-    precondition(!buildSystemHasDelegate)
-    self.fallbackBuildSystem = fallbackBuildSystem
-    self.mainFilesProvider = mainFilesProvider
-    self.toolchainRegistry = toolchainRegistry
-    self.buildSystem =
-      if let testBuildSystem {
-        await BuiltInBuildSystemAdapter(testBuildSystem: testBuildSystem, messageHandler: self)
-      } else {
-        nil
-      }
-    await self.buildSystem?.underlyingBuildSystem.setDelegate(self)
-  }
-
   package func filesDidChange(_ events: [FileEvent]) async {
     await self.buildSystem?.send(BuildServerProtocol.DidChangeWatchedFilesNotification(changes: events))
   }
 
   /// Implementation of `MessageHandler`, handling notifications from the build system.
   ///
   /// - Important: Do not call directly.
-  package func handle(_ notification: some LanguageServerProtocol.NotificationType) {
+  package func handle(_ notification: some LanguageServerProtocol.NotificationType) async {
     switch notification {
     case let notification as DidChangeBuildTargetNotification:
-      self.didChangeTextDocumentTargets(notification: notification)
+      await self.didChangeBuildTarget(notification: notification)
     default:
       logger.error("Ignoring unknown notification \(type(of: notification).method)")
     }
@@ -290,12 +268,24 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     guard let buildSystem, let target else {
       return nil
     }
+    let request = SourceKitOptionsRequest(textDocument: TextDocumentIdentifier(uri: document), target: target)
+
     // TODO: We should only wait `fallbackSettingsTimeout` for build settings
     // and return fallback afterwards.
     // For now, this should be fine because all build systems return
     // very quickly from `settings(for:language:)`.
     // https://github.com/apple/sourcekit-lsp/issues/1181
-    return try await buildSystem.underlyingBuildSystem.buildSettings(for: document, in: target, language: language)
+    let response = try await cachedSourceKitOptions.get(request) { request in
+      try await buildSystem.send(request)
+    }
+    guard let response else {
+      return nil
+    }
+    return FileBuildSettings(
+      compilerArguments: response.compilerArguments,
+      workingDirectory: response.workingDirectory,
+      isFallback: false
+    )
   }
 
   /// Returns the build settings for the given file in the given target.
@@ -440,14 +430,6 @@ extension BuildSystemManager: BuildSystemDelegate {
     )
   }
 
-  package func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) async {
-    let changedWatchedFiles = watchedFilesReferencing(mainFiles: changedFiles)
-
-    if !changedWatchedFiles.isEmpty, let delegate = self.delegate {
-      await delegate.fileBuildSettingsChanged(changedWatchedFiles)
-    }
-  }
-
   package func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) async {
     // Empty changes --> assume everything has changed.
     guard !changedFiles.isEmpty else {
@@ -470,10 +452,28 @@ extension BuildSystemManager: BuildSystemDelegate {
     }
   }
 
-  private func didChangeTextDocumentTargets(notification: DidChangeBuildTargetNotification) {
+  private func didChangeBuildTarget(notification: DidChangeBuildTargetNotification) async {
     // Every `DidChangeBuildTargetNotification` notification needs to invalidate the cache since the changed target
     // might gained a source file.
     self.cachedTargetsForDocument.clearAll()
+
+    let updatedTargets: Set<BuildTargetIdentifier>? =
+      if let changes = notification.changes {
+        Set(changes.map(\.target))
+      } else {
+        nil
+      }
+    self.cachedSourceKitOptions.clear { cacheKey in
+      guard let updatedTargets else {
+        // All targets might have changed
+        return true
+      }
+      return updatedTargets.contains(cacheKey.target)
+    }
+
+    // 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))
   }
 }
 

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -108,14 +108,10 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable {
   ///
   /// Returns `nil` if the build system can't provide build settings for this
   /// file or if it hasn't computed build settings for the file yet.
-  func buildSettings(
-    for document: DocumentURI,
-    in target: BuildTargetIdentifier,
-    language: Language
-  ) async throws -> FileBuildSettings?
+  func sourceKitOptions(request: SourceKitOptionsRequest) async throws -> SourceKitOptionsResponse?
 
   /// Return the list of targets that the given document can be built for.
-  func inverseSources(_ request: InverseSourcesRequest) async -> InverseSourcesResponse
+  func inverseSources(request: InverseSourcesRequest) async throws -> InverseSourcesResponse
 
   /// Schedule a task that re-generates the build graph. The function may return before the build graph has finished
   /// being generated. If clients need to wait for an up-to-date build graph, they should call