Merge pull request #1664 from ahoppen/buildgraph-generation-targets

Implicitly trigger build graph generation when creating a `SwiftPMBuildSystem` and migrate `targets(dependingOn:)` and `topologicalSort` to BSP

Author: ahoppen

Sources/BuildSystemIntegration/BuildServerBuildSystem.swift

@@ -334,18 +334,8 @@ extension BuildServerBuildSystem: BuiltInBuildSystem {
     return nil
   }
 
-  package func scheduleBuildGraphGeneration() {}
-
   package func waitForUpToDateBuildGraph() async {}
 
-  package func topologicalSort(of targets: [BuildTargetIdentifier]) async -> [BuildTargetIdentifier]? {
-    return nil
-  }
-
-  package func targets(dependingOn targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    return nil
-  }
-
   package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) {
     // BuildServerBuildSystem does not support syntactic test discovery or background indexing.
     // (https://github.com/swiftlang/sourcekit-lsp/issues/1173).

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -118,6 +118,8 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
 
   private var cachedTargetSources = RequestCache<BuildTargetSourcesRequest>()
 
+  private var cachedTargetDepths: (buildTargets: [BuildTarget], depths: [BuildTargetIdentifier: Int])? = nil
+
   /// 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.
@@ -481,20 +483,64 @@ package actor BuildSystemManager: BuiltInBuildSystemAdapterDelegate {
     return settings
   }
 
-  package func scheduleBuildGraphGeneration() async throws {
-    try await self.buildSystem?.underlyingBuildSystem.scheduleBuildGraphGeneration()
-  }
-
   package func waitForUpToDateBuildGraph() async {
     await self.buildSystem?.underlyingBuildSystem.waitForUpToDateBuildGraph()
   }
 
+  /// The root targets of the project have depth of 0 and all target dependencies have a greater depth than the target
+  // itself.
+  private func targetDepths(for buildTargets: [BuildTarget]) -> [BuildTargetIdentifier: Int] {
+    if let cachedTargetDepths, cachedTargetDepths.buildTargets == buildTargets {
+      return cachedTargetDepths.depths
+    }
+    var nonRoots: Set<BuildTargetIdentifier> = []
+    for buildTarget in buildTargets {
+      nonRoots.formUnion(buildTarget.dependencies)
+    }
+    let targetsById = Dictionary(elements: buildTargets, keyedBy: \.id)
+    var depths: [BuildTargetIdentifier: Int] = [:]
+    let rootTargets = buildTargets.filter { !nonRoots.contains($0.id) }
+    var worksList: [(target: BuildTargetIdentifier, depth: Int)] = rootTargets.map { ($0.id, 0) }
+    while let (target, depth) = worksList.popLast() {
+      depths[target] = max(depths[target, default: 0], depth)
+      for dependency in targetsById[target]?.dependencies ?? [] {
+        // Check if we have already recorded this target with a greater depth, in which case visiting it again will
+        // not increase its depth or any of its children.
+        if depths[target, default: 0] < depth + 1 {
+          worksList.append((dependency, depth + 1))
+        }
+      }
+    }
+    cachedTargetDepths = (buildTargets, depths)
+    return depths
+  }
+
+  /// Sort the targets so that low-level targets occur before high-level targets.
+  ///
+  /// This sorting is best effort but allows the indexer to prepare and index low-level targets first, which allows
+  /// index data to be available earlier.
+  ///
+  /// `nil` if the build system doesn't support topological sorting of targets.
   package func topologicalSort(of targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier]? {
-    return await buildSystem?.underlyingBuildSystem.topologicalSort(of: targets)
+    guard let workspaceTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() })
+    else {
+      return nil
+    }
+
+    let depths = targetDepths(for: workspaceTargets)
+    return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
+      return depths[lhs, default: 0] > depths[rhs, default: 0]
+    }
   }
 
-  package func targets(dependingOn targets: [BuildTargetIdentifier]) async -> [BuildTargetIdentifier]? {
-    return await buildSystem?.underlyingBuildSystem.targets(dependingOn: targets)
+  /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in
+  /// `target` is modified.
+  package func targets(dependingOn targetIds: Set<BuildTargetIdentifier>) async -> [BuildTargetIdentifier] {
+    guard let buildTargets = await orLog("Getting build targets for dependencies", { try await self.buildTargets() })
+    else {
+      return []
+    }
+    return buildTargets.filter { $0.dependencies.contains(anyIn: targetIds) }.map { $0.id }
   }
 
   package func prepare(

Sources/BuildSystemIntegration/BuiltInBuildSystem.swift

@@ -102,31 +102,9 @@ package protocol BuiltInBuildSystem: AnyObject, Sendable {
   /// file or if it hasn't computed build settings for the file yet.
   func sourceKitOptions(request: SourceKitOptionsRequest) async throws -> SourceKitOptionsResponse?
 
-  /// 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
-  /// `waitForUpToDateBuildGraph` afterwards.
-  func scheduleBuildGraphGeneration() async throws
-
   /// Wait until the build graph has been loaded.
   func waitForUpToDateBuildGraph() async
 
-  /// Sort the targets so that low-level targets occur before high-level targets.
-  ///
-  /// This sorting is best effort but allows the indexer to prepare and index low-level targets first, which allows
-  /// index data to be available earlier.
-  ///
-  /// `nil` if the build system doesn't support topological sorting of targets.
-  func topologicalSort(of targets: [BuildTargetIdentifier]) async -> [BuildTargetIdentifier]?
-
-  /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in
-  /// `target` is modified.
-  ///
-  /// The returned list can be an over-approximation, in which case the indexer will perform more work than strictly
-  /// necessary by scheduling re-preparation of a target where it isn't necessary.
-  ///
-  /// Returning `nil` indicates that all targets should be considered depending on the given target.
-  func targets(dependingOn targets: [BuildTargetIdentifier]) async -> [BuildTargetIdentifier]?
-
   /// The toolchain that should be used to open the given document.
   ///
   /// If `nil` is returned, then the default toolchain for the given language is used.

Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift

@@ -157,18 +157,8 @@ extension CompilationDatabaseBuildSystem: BuiltInBuildSystem {
     return nil
   }
 
-  package func scheduleBuildGraphGeneration() {}
-
   package func waitForUpToDateBuildGraph() async {}
 
-  package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    return nil
-  }
-
-  package func targets(dependingOn targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    return nil
-  }
-
   private func database(for uri: DocumentURI) -> CompilationDatabase? {
     if let url = uri.fileURL, let path = try? AbsolutePath(validating: url.path) {
       return database(for: path)

Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift

@@ -210,10 +210,10 @@ package actor SwiftPMBuildSystem {
   /// Maps source and header files to the target that include them.
   private var fileToTargets: [DocumentURI: Set<BuildTargetIdentifier>] = [:]
 
-  /// Maps target ids to their SwiftPM build target as well as the depth at which they occur in the build
-  /// graph. Top level targets on which no other target depends have a depth of `1`. Targets with dependencies have a
-  /// greater depth.
-  private var targets: [BuildTargetIdentifier: (buildTarget: SwiftBuildTarget, depth: Int)] = [:]
+  /// Maps target ids to their SwiftPM build target.
+  private var swiftPMTargets: [BuildTargetIdentifier: SwiftBuildTarget] = [:]
+
+  private var targetDependencies: [BuildTargetIdentifier: Set<BuildTargetIdentifier>] = [:]
 
   static package func projectRoot(
     for path: TSCBasic.AbsolutePath,
@@ -361,6 +361,15 @@ package actor SwiftPMBuildSystem {
     )
 
     self.reloadPackageStatusCallback = reloadPackageStatusCallback
+
+    packageLoadingQueue.async {
+      await orLog("Initial package loading") {
+        // Schedule an initial generation of the build graph. Once the build graph is loaded, the build system will send
+        // call `fileHandlingCapabilityChanged`, which allows us to move documents to a workspace with this build
+        // system.
+        try await self.reloadPackageAssumingOnPackageLoadingQueue()
+      }
+    }
   }
 
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
@@ -398,15 +407,9 @@ package actor SwiftPMBuildSystem {
 extension SwiftPMBuildSystem {
   /// (Re-)load the package settings by parsing the manifest and resolving all the targets and
   /// dependencies.
-  @discardableResult
-  package func schedulePackageReload() -> Task<Void, Swift.Error> {
-    return packageLoadingQueue.asyncThrowing {
-      try await self.reloadPackageImpl()
-    }
-  }
-
+  ///
   /// - Important: Must only be called on `packageLoadingQueue`.
-  private func reloadPackageImpl() async throws {
+  private func reloadPackageAssumingOnPackageLoadingQueue() async throws {
     await reloadPackageStatusCallback(.start)
     await testHooks.reloadPackageDidStart?()
     defer {
@@ -437,22 +440,26 @@ extension SwiftPMBuildSystem {
     /// properties because otherwise we might end up in an inconsistent state
     /// with only some properties modified.
 
-    self.targets = [:]
+    self.swiftPMTargets = [:]
     self.fileToTargets = [:]
+    self.targetDependencies = [:]
+
     buildDescription.traverseModules { buildTarget, parent, depth in
       let targetIdentifier = orLog("Getting build target identifier") { try BuildTargetIdentifier(buildTarget) }
       guard let targetIdentifier else {
         return
       }
-      var depth = depth
-      if let existingDepth = targets[targetIdentifier]?.depth {
-        depth = max(existingDepth, depth)
-      } else {
+      if swiftPMTargets[targetIdentifier] == nil {
         for source in buildTarget.sources + buildTarget.headers {
           fileToTargets[DocumentURI(source), default: []].insert(targetIdentifier)
         }
       }
-      targets[targetIdentifier] = (buildTarget, depth)
+      if let parent,
+        let parentIdentifier = orLog("Getting parent build target identifier", { try BuildTargetIdentifier(parent) })
+      {
+        self.targetDependencies[parentIdentifier, default: []].insert(targetIdentifier)
+      }
+      swiftPMTargets[targetIdentifier] = buildTarget
     }
 
     await messageHandler?.sendNotificationToSourceKitLSP(DidChangeBuildTargetNotification(changes: nil))
@@ -511,9 +518,9 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
   }
 
   package func buildTargets(request: BuildTargetsRequest) async throws -> BuildTargetsResponse {
-    let targets = self.targets.map { (targetId, target) in
+    let targets = self.swiftPMTargets.map { (targetId, target) in
       var tags: [BuildTargetTag] = [.test]
-      if target.depth != 1 {
+      if !target.isPartOfRootPackage {
         tags.append(.dependency)
       }
       return BuildTarget(
@@ -524,8 +531,7 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
         capabilities: BuildTargetCapabilities(),
         // Be conservative with the languages that might be used in the target. SourceKit-LSP doesn't use this property.
         languageIds: [.c, .cpp, .objective_c, .objective_cpp, .swift],
-        // FIXME: (BSP migration) List the target's dependencies
-        dependencies: []
+        dependencies: self.targetDependencies[targetId, default: []].sorted { $0.uri.stringValue < $1.uri.stringValue }
       )
     }
     return BuildTargetsResponse(targets: targets)
@@ -536,10 +542,10 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
     // TODO: Query The SwiftPM build system for the document's language and add it to SourceItem.data
     // (https://github.com/swiftlang/sourcekit-lsp/issues/1267)
     for target in request.targets {
-      guard let swiftPMTarget = self.targets[target] else {
+      guard let swiftPMTarget = self.swiftPMTargets[target] else {
         continue
       }
-      let sources = swiftPMTarget.buildTarget.sources.map {
+      let sources = swiftPMTarget.sources.map {
         SourceItem(uri: DocumentURI($0), kind: .file, generated: false)
       }
       result.append(SourcesItem(target: target, sources: sources))
@@ -557,13 +563,13 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
       return try settings(forPackageManifest: path)
     }
 
-    guard let buildTarget = self.targets[request.target]?.buildTarget else {
+    guard let swiftPMTarget = self.swiftPMTargets[request.target] else {
       logger.fault("Did not find target \(request.target.forLogging)")
       return nil
     }
 
-    if !buildTarget.sources.lazy.map(DocumentURI.init).contains(request.textDocument.uri),
-      let substituteFile = buildTarget.sources.sorted(by: { $0.path < $1.path }).first
+    if !swiftPMTarget.sources.lazy.map(DocumentURI.init).contains(request.textDocument.uri),
+      let substituteFile = swiftPMTarget.sources.sorted(by: { $0.path < $1.path }).first
     {
       logger.info("Getting compiler arguments for \(url) using substitute file \(substituteFile)")
       // If `url` is not part of the target's source, it's most likely a header file. Fake compiler arguments for it
@@ -574,7 +580,7 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
       // getting its compiler arguments and then patching up the compiler arguments by replacing the substitute file
       // with the `.cpp` file.
       let buildSettings = FileBuildSettings(
-        compilerArguments: try await compilerArguments(for: DocumentURI(substituteFile), in: buildTarget),
+        compilerArguments: try await compilerArguments(for: DocumentURI(substituteFile), in: swiftPMTarget),
         workingDirectory: projectRoot.pathString
       ).patching(newFile: try resolveSymlinks(path).pathString, originalFile: substituteFile.absoluteString)
       return SourceKitOptionsResponse(
@@ -584,7 +590,7 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
     }
 
     return SourceKitOptionsResponse(
-      compilerArguments: try await compilerArguments(for: request.textDocument.uri, in: buildTarget),
+      compilerArguments: try await compilerArguments(for: request.textDocument.uri, in: swiftPMTarget),
       workingDirectory: projectRoot.pathString
     )
   }
@@ -620,43 +626,10 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
     return InverseSourcesResponse(targets: targets(for: request.textDocument.uri))
   }
 
-  package func scheduleBuildGraphGeneration() async throws {
-    self.schedulePackageReload()
-  }
-
   package func waitForUpToDateBuildGraph() async {
     await self.packageLoadingQueue.async {}.valuePropagatingCancellation
   }
 
-  package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
-      let lhsDepth = self.targets[lhs]?.depth ?? 0
-      let rhsDepth = self.targets[rhs]?.depth ?? 0
-      return lhsDepth > rhsDepth
-    }
-  }
-
-  package func targets(dependingOn targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    let targetDepths = targets.compactMap { self.targets[$0]?.depth }
-    let minimumTargetDepth: Int?
-    if targetDepths.count == targets.count {
-      minimumTargetDepth = targetDepths.max()
-    } else {
-      // One of the targets didn't have an entry in self.targets. We don't know what might depend on it.
-      minimumTargetDepth = nil
-    }
-
-    // Files that occur before the target in the topological sorting don't depend on it.
-    // Ideally, we should consult the dependency graph here for more accurate dependency analysis instead of relying on
-    // a flattened list (https://github.com/swiftlang/sourcekit-lsp/issues/1312).
-    return self.targets.compactMap { (targets, value) -> BuildTargetIdentifier? in
-      if let minimumTargetDepth, value.depth >= minimumTargetDepth {
-        return nil
-      }
-      return targets
-    }
-  }
-
   package func prepare(request: PrepareTargetsRequest) async throws -> VoidResponse {
     // TODO: Support preparation of multiple targets at once. (https://github.com/swiftlang/sourcekit-lsp/issues/1262)
     for target in request.targets {
@@ -810,25 +783,12 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuiltInBuildSystem {
   package func didChangeWatchedFiles(notification: BuildServerProtocol.DidChangeWatchedFilesNotification) async {
     if notification.changes.contains(where: { self.fileEventShouldTriggerPackageReload(event: $0) }) {
       logger.log("Reloading package because of file change")
-      await orLog("Reloading package") {
-        try await self.schedulePackageReload().value
-      }
-    }
-  }
-
-  package func sourceFiles() -> [SourceFileInfo] {
-    var sourceFiles: [DocumentURI: SourceFileInfo] = [:]
-    for (buildTarget, depth) in self.targets.values {
-      for sourceFile in buildTarget.sources {
-        let uri = DocumentURI(sourceFile)
-        sourceFiles[uri] = SourceFileInfo(
-          uri: uri,
-          isPartOfRootProject: depth == 1 || (sourceFiles[uri]?.isPartOfRootProject ?? false),
-          mayContainTests: true
-        )
-      }
+      await packageLoadingQueue.async {
+        await orLog("Reloading package") {
+          try await self.reloadPackageAssumingOnPackageLoadingQueue()
+        }
+      }.valuePropagatingCancellation
     }
-    return sourceFiles.values.sorted { $0.uri.pseudoPath < $1.uri.pseudoPath }
   }
 
   package func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async {

Sources/BuildSystemIntegration/TestBuildSystem.swift

@@ -74,17 +74,11 @@ package actor TestBuildSystem: BuiltInBuildSystem {
     return nil
   }
 
-  package func scheduleBuildGraphGeneration() {}
-
   package func waitForUpToDateBuildGraph() async {}
 
   package func topologicalSort(of targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
     return nil
   }
 
-  package func targets(dependingOn targets: [BuildTargetIdentifier]) -> [BuildTargetIdentifier]? {
-    return nil
-  }
-
   package func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) async {}
 }