Merge pull request #1817 from ahoppen/bsp-review

Author: ahoppen

Sources/BuildServerProtocol/Messages/BuildTargetSourcesRequest.swift

@@ -17,7 +17,7 @@ import LanguageServerProtocol
 #endif
 
 /// The build target sources request is sent from the client to the server to
-/// query for the list of text documents and directories that are belong to a
+/// query for the list of text documents and directories that belong to a
 /// build target. The sources response must not include sources that are
 /// external to the workspace.
 public struct BuildTargetSourcesRequest: RequestType, Hashable {

Sources/BuildServerProtocol/SupportTypes/BuildTarget.swift

@@ -34,7 +34,7 @@ public struct BuildTarget: Codable, Hashable, Sendable {
   /// A human readable name for this target.
   /// May be presented in the user interface.
   /// Should be unique if possible.
-  /// The id.uri is used if None.
+  /// The id.uri is used if `nil`.
   public var displayName: String?
 
   /// The directory where this target belongs to. Multiple build targets are

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -136,7 +136,7 @@ private enum BuildSystemAdapter {
   }
 }
 
-private extension BuildSystemKind {
+private extension BuildSystemSpec {
   private static func createBuiltInBuildSystemAdapter(
     projectRoot: AbsolutePath,
     messagesToSourceKitLSPHandler: any MessageHandler,
@@ -176,8 +176,8 @@ private extension BuildSystemKind {
     buildSystemTestHooks testHooks: BuildSystemTestHooks,
     messagesToSourceKitLSPHandler: any MessageHandler
   ) async -> BuildSystemAdapter? {
-    switch self {
-    case .buildServer(projectRoot: let projectRoot):
+    switch self.kind {
+    case .buildServer:
       let buildSystem = await orLog("Creating external build system") {
         try await ExternalBuildSystemAdapter(
           projectRoot: projectRoot,
@@ -190,7 +190,7 @@ private extension BuildSystemKind {
       }
       logger.log("Created external build server at \(projectRoot.pathString)")
       return .external(buildSystem)
-    case .compilationDatabase(projectRoot: let projectRoot):
+    case .compilationDatabase:
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
         messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
@@ -204,7 +204,7 @@ private extension BuildSystemKind {
           connectionToSourceKitLSP: connectionToSourceKitLSP
         )
       }
-    case .swiftPM(projectRoot: let projectRoot):
+    case .swiftPM:
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
         messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
@@ -218,7 +218,7 @@ private extension BuildSystemKind {
           testHooks: testHooks.swiftPMTestHooks
         )
       }
-    case .testBuildSystem(projectRoot: let projectRoot):
+    case .testBuildSystem:
       return await Self.createBuiltInBuildSystemAdapter(
         projectRoot: projectRoot,
         messagesToSourceKitLSPHandler: messagesToSourceKitLSPHandler,
@@ -353,7 +353,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
   }
 
   package init(
-    buildSystemKind: BuildSystemKind?,
+    buildSystemSpec: BuildSystemSpec?,
     toolchainRegistry: ToolchainRegistry,
     options: SourceKitLSPOptions,
     connectionToClient: BuildSystemManagerConnectionToClient,
@@ -362,8 +362,8 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
     self.toolchainRegistry = toolchainRegistry
     self.options = options
     self.connectionToClient = connectionToClient
-    self.projectRoot = buildSystemKind?.projectRoot
-    self.buildSystemAdapter = await buildSystemKind?.createBuildSystemAdapter(
+    self.projectRoot = buildSystemSpec?.projectRoot
+    self.buildSystemAdapter = await buildSystemSpec?.createBuildSystemAdapter(
       toolchainRegistry: toolchainRegistry,
       options: options,
       buildSystemTestHooks: buildSystemTestHooks,
@@ -392,8 +392,8 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       guard let buildSystemAdapter else {
         return nil
       }
-      guard let buildSystemKind else {
-        logger.fault("If we have a connectionToBuildSystem, we must have had a buildSystemKind")
+      guard let buildSystemSpec else {
+        logger.fault("If we have a connectionToBuildSystem, we must have had a buildSystemSpec")
         return nil
       }
       let initializeResponse = await orLog("Initializing build system") {
@@ -402,7 +402,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
             displayName: "SourceKit-LSP",
             version: "",
             bspVersion: "2.2.0",
-            rootUri: URI(buildSystemKind.projectRoot.asURL),
+            rootUri: URI(buildSystemSpec.projectRoot.asURL),
             capabilities: BuildClientCapabilities(languageIds: [.c, .cpp, .objective_c, .objective_cpp, .swift])
           )
         )
@@ -415,7 +415,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
         // the build server.
         logger.log("Launched a legacy BSP server. Using push-based build settings model.")
         let legacyBuildServer = await LegacyBuildServerBuildSystem(
-          projectRoot: buildSystemKind.projectRoot,
+          projectRoot: buildSystemSpec.projectRoot,
           initializationData: initializeResponse,
           externalBuildSystemAdapter
         )
@@ -651,7 +651,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
     let sourcesItems = try await self.sourceFiles(in: [target])
     let sourceFiles = sourcesItems.flatMap(\.sources)
     var result: Language? = nil
-    for sourceFile in sourceFiles {
+    for sourceFile in sourceFiles where sourceFile.uri == document {
       guard let language = sourceFile.sourceKitData?.language else {
         continue
       }

Sources/BuildSystemIntegration/BuiltInBuildSystemAdapter.swift

@@ -27,19 +27,22 @@ import struct TSCBasic.AbsolutePath
 import struct TSCBasic.RelativePath
 #endif
 
-package enum BuildSystemKind {
-  case buildServer(projectRoot: AbsolutePath)
-  case compilationDatabase(projectRoot: AbsolutePath)
-  case swiftPM(projectRoot: AbsolutePath)
-  case testBuildSystem(projectRoot: AbsolutePath)
-
-  package var projectRoot: AbsolutePath {
-    switch self {
-    case .buildServer(let projectRoot): return projectRoot
-    case .compilationDatabase(let projectRoot): return projectRoot
-    case .swiftPM(let projectRoot): return projectRoot
-    case .testBuildSystem(let projectRoot): return projectRoot
-    }
+/// The details necessary to create a `BuildSystemAdapter`.
+package struct BuildSystemSpec {
+  package enum Kind {
+    case buildServer
+    case compilationDatabase
+    case swiftPM
+    case testBuildSystem
+  }
+
+  package var kind: Kind
+
+  package var projectRoot: AbsolutePath
+
+  package init(kind: BuildSystemSpec.Kind, projectRoot: AbsolutePath) {
+    self.kind = kind
+    self.projectRoot = projectRoot
   }
 }
 

Sources/BuildSystemIntegration/DetermineBuildSystem.swift

@@ -35,7 +35,7 @@ import struct TSCBasic.AbsolutePath
 package func determineBuildSystem(
   forWorkspaceFolder workspaceFolder: DocumentURI,
   options: SourceKitLSPOptions
-) -> BuildSystemKind? {
+) -> BuildSystemSpec? {
   var buildSystemPreference: [WorkspaceType] = [
     .buildServer, .swiftPM, .compilationDatabase,
   ]
@@ -52,17 +52,17 @@ package func determineBuildSystem(
     switch buildSystemType {
     case .buildServer:
       if let projectRoot = ExternalBuildSystemAdapter.projectRoot(for: workspaceFolderPath, options: options) {
-        return .buildServer(projectRoot: projectRoot)
+        return BuildSystemSpec(kind: .buildServer, projectRoot: projectRoot)
       }
     case .compilationDatabase:
       if let projectRoot = CompilationDatabaseBuildSystem.projectRoot(for: workspaceFolderPath, options: options) {
-        return .compilationDatabase(projectRoot: projectRoot)
+        return BuildSystemSpec(kind: .compilationDatabase, projectRoot: projectRoot)
       }
     case .swiftPM:
       if let projectRootURL = SwiftPMBuildSystem.projectRoot(for: workspaceFolderUrl, options: options),
         let projectRoot = try? AbsolutePath(validating: projectRootURL.filePath)
       {
-        return .swiftPM(projectRoot: projectRoot)
+        return BuildSystemSpec(kind: .swiftPM, projectRoot: projectRoot)
       }
     }
   }

Sources/SourceKitLSP/SharedWorkDoneProgressManager.swift

@@ -49,8 +49,8 @@ extension WorkDoneProgressManager {
 actor SharedWorkDoneProgressManager {
   private weak var sourceKitLSPServer: SourceKitLSPServer?
 
-  /// The number of in-progress operations. When greater than 0 `workDoneProgress` non-nil and a work done progress is
-  /// displayed to the user.
+  /// The number of in-progress operations. When greater than 0 `workDoneProgress` is non-nil and a work done progress
+  /// is displayed to the user.
   private var inProgressOperations = 0
   private var workDoneProgress: WorkDoneProgressManager?
 

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -229,16 +229,16 @@ package actor SourceKitLSPServer {
       // was added to it and thus currently doesn't know that it can handle that file. In that case, we shouldn't open
       // a new workspace for the same root. Instead, the existing workspace's build system needs to be reloaded.
       let uri = DocumentURI(url)
-      guard let buildSystemKind = determineBuildSystem(forWorkspaceFolder: uri, options: self.options) else {
+      guard let buildSystemSpec = determineBuildSystem(forWorkspaceFolder: uri, options: self.options) else {
         continue
       }
-      guard !projectRoots.contains(buildSystemKind.projectRoot) else {
+      guard !projectRoots.contains(buildSystemSpec.projectRoot) else {
         continue
       }
       guard
         let workspace = await orLog(
           "Creating workspace",
-          { try await createWorkspace(workspaceFolder: uri, buildSystemKind: buildSystemKind) }
+          { try await createWorkspace(workspaceFolder: uri, buildSystemSpec: buildSystemSpec) }
         )
       else {
         continue
@@ -811,7 +811,7 @@ extension SourceKitLSPServer {
   /// If the build system that was determined for the workspace does not satisfy `condition`, `nil` is returned.
   private func createWorkspace(
     workspaceFolder: DocumentURI,
-    buildSystemKind: BuildSystemKind?
+    buildSystemSpec: BuildSystemSpec?
   ) async throws -> Workspace {
     guard let capabilityRegistry = capabilityRegistry else {
       struct NoCapabilityRegistryError: Error {}
@@ -835,7 +835,7 @@ extension SourceKitLSPServer {
       documentManager: self.documentManager,
       rootUri: workspaceFolder,
       capabilityRegistry: capabilityRegistry,
-      buildSystemKind: buildSystemKind,
+      buildSystemSpec: buildSystemSpec,
       toolchainRegistry: self.toolchainRegistry,
       options: options,
       testHooks: testHooks,
@@ -858,6 +858,15 @@ extension SourceKitLSPServer {
     return workspace
   }
 
+  /// Determines the build system for the given workspace folder and creates a `Workspace` that uses this inferred build
+  /// system.
+  private func createWorkspaceWithInferredBuildSystem(workspaceFolder: DocumentURI) async throws -> Workspace {
+    return try await self.createWorkspace(
+      workspaceFolder: workspaceFolder,
+      buildSystemSpec: determineBuildSystem(forWorkspaceFolder: workspaceFolder, options: self.options)
+    )
+  }
+
   func initialize(_ req: InitializeRequest) async throws -> InitializeResult {
     logger.logFullObjectInMultipleLogMessages(header: "Initialize request", AnyRequestType(request: req))
     // If the client can handle `PeekDocumentsRequest`, they can enable the
@@ -918,34 +927,25 @@ extension SourceKitLSPServer {
       if let workspaceFolders = req.workspaceFolders {
         self.workspacesAndIsImplicit += await workspaceFolders.asyncCompactMap { workspaceFolder in
           await orLog("Creating workspace from workspaceFolders") {
-            let workspace = try await self.createWorkspace(
-              workspaceFolder: workspaceFolder.uri,
-              buildSystemKind: determineBuildSystem(forWorkspaceFolder: workspaceFolder.uri, options: self.options)
+            return (
+              workspace: try await self.createWorkspaceWithInferredBuildSystem(workspaceFolder: workspaceFolder.uri),
+              isImplicit: false
             )
-            return (workspace: workspace, isImplicit: false)
           }
         }
       } else if let uri = req.rootURI {
-        let workspace = await orLog("Creating workspace from rootURI") {
-          try await self.createWorkspace(
-            workspaceFolder: uri,
-            buildSystemKind: determineBuildSystem(forWorkspaceFolder: uri, options: self.options)
+        await orLog("Creating workspace from rootURI") {
+          self.workspacesAndIsImplicit.append(
+            (workspace: try await self.createWorkspaceWithInferredBuildSystem(workspaceFolder: uri), isImplicit: false)
           )
         }
-        if let workspace {
-          self.workspacesAndIsImplicit.append((workspace: workspace, isImplicit: false))
-        }
       } else if let path = req.rootPath {
         let uri = DocumentURI(URL(fileURLWithPath: path))
-        let workspace = await orLog("Creating workspace from rootPath") {
-          try await self.createWorkspace(
-            workspaceFolder: uri,
-            buildSystemKind: determineBuildSystem(forWorkspaceFolder: uri, options: self.options)
+        await orLog("Creating workspace from rootPath") {
+          self.workspacesAndIsImplicit.append(
+            (workspace: try await self.createWorkspaceWithInferredBuildSystem(workspaceFolder: uri), isImplicit: false)
           )
         }
-        if let workspace {
-          self.workspacesAndIsImplicit.append((workspace: workspace, isImplicit: false))
-        }
       }
 
       if self.workspaces.isEmpty {
@@ -957,7 +957,7 @@ extension SourceKitLSPServer {
           documentManager: self.documentManager,
           rootUri: req.rootURI,
           capabilityRegistry: self.capabilityRegistry!,
-          buildSystemKind: nil,
+          buildSystemSpec: nil,
           toolchainRegistry: self.toolchainRegistry,
           options: options,
           testHooks: testHooks,
@@ -1343,10 +1343,7 @@ extension SourceKitLSPServer {
       if let added = notification.event.added {
         let newWorkspaces = await added.asyncCompactMap { workspaceFolder in
           await orLog("Creating workspace after workspace folder change") {
-            try await self.createWorkspace(
-              workspaceFolder: workspaceFolder.uri,
-              buildSystemKind: determineBuildSystem(forWorkspaceFolder: workspaceFolder.uri, options: self.options)
-            )
+            try await self.createWorkspaceWithInferredBuildSystem(workspaceFolder: workspaceFolder.uri)
           }
         }
         self.workspacesAndIsImplicit += newWorkspaces.map { (workspace: $0, isImplicit: false) }

Sources/SourceKitLSP/Workspace.swift

@@ -174,7 +174,7 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
     documentManager: DocumentManager,
     rootUri: DocumentURI?,
     capabilityRegistry: CapabilityRegistry,
-    buildSystemKind: BuildSystemKind?,
+    buildSystemSpec: BuildSystemSpec?,
     toolchainRegistry: ToolchainRegistry,
     options: SourceKitLSPOptions,
     testHooks: TestHooks,
@@ -224,15 +224,15 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
     }
 
     let buildSystemManager = await BuildSystemManager(
-      buildSystemKind: buildSystemKind,
+      buildSystemSpec: buildSystemSpec,
       toolchainRegistry: toolchainRegistry,
       options: options,
       connectionToClient: ConnectionToClient(sourceKitLSPServer: sourceKitLSPServer),
       buildSystemTestHooks: testHooks.buildSystemTestHooks
     )
 
     logger.log(
-      "Created workspace at \(rootUri.forLogging) with project root \(buildSystemKind?.projectRoot.pathString ?? "<nil>")"
+      "Created workspace at \(rootUri.forLogging) with project root \(buildSystemSpec?.projectRoot.pathString ?? "<nil>")"
     )
 
     var index: IndexStoreDB? = nil

Sources/SwiftExtensions/Sequence+AsyncMap.swift

@@ -71,7 +71,7 @@ extension Sequence {
 
   /// Just like `Sequence.first` but allows an `async` predicate function.
   package func asyncFirst(
-    @_inheritActorContext _ predicate: @Sendable (Element) async throws -> Bool
+    @_inheritActorContext where predicate: @Sendable (Element) async throws -> Bool
   ) async rethrows -> Element? {
     for element in self {
       if try await predicate(element) {
@@ -86,6 +86,6 @@ extension Sequence {
   package func asyncContains(
     @_inheritActorContext where predicate: @Sendable (Element) async throws -> Bool
   ) async rethrows -> Bool {
-    return try await asyncFirst(predicate) != nil
+    return try await asyncFirst(where: predicate) != nil
   }
 }