Index targets top-down instead of bottom up

Contributor Documentation/BSP Extensions.md

@@ -100,21 +100,32 @@ To do so, the build server should perform any work that is necessary to typechec
 The server communicates during the initialize handshake whether this method is supported or not by setting `prepareProvider: true` in `SourceKitInitializeBuildResponseData`.
 
 - method: `sourcekit/buildTarget/prepare`
-- params: `PrepareParams`
-- result: `void`
+- params: `BuildTargetPrepareRequest`
+- result: `BuildTargetPrepareResponse`
 
 > [!NOTE]
 > This request was previously named `buildTarget/prepare`. The old name is still accepted for backward compatibility.
 
 ```ts
-export interface PrepareParams {
+export interface BuildTargetPrepareRequest {
   /** A list of build targets to prepare. */
   targets: BuildTargetIdentifier[];
 
   /** A unique identifier generated by the client to identify this request.
    * The server may include this id in triggered notifications or responses. **/
   originId?: OriginId;
 }
+
+export interface BuildTargetPrepareResponse {
+  /**
+   * Targets that were implicitly prepared by the prepare request.
+   *
+   * For example, a target may be implicitly prepared if one if its dependents gets prepared.
+   *
+   * When omitted, this is the same as an empty array.
+   */
+  implicitlyPreparedTargets?: BuildTargetIdentifier[]
+}
 ```
 
 ## `buildTarget/sources`

Sources/BuildServerIntegration/BuildServerManager.swift

@@ -1466,24 +1466,55 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     return (depths, dependents)
   }
 
-  /// 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.
-  package func topologicalSort(of targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] {
+  /// Sort the targets in the order they should be indexed in for best performance.
+  package func targetsSortedForIndexing(_ targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] {
     guard let buildTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() })
     else {
       return targets.sorted { $0.uri.stringValue < $1.uri.stringValue }
     }
 
-    return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
-      let lhsDepth = buildTargets[lhs]?.depth ?? 0
-      let rhsDepth = buildTargets[rhs]?.depth ?? 0
+    // Generate a preliminary work list of targets to index in which we prefer top-level targets over low-level targets
+    // and targets of the root package over targets in dependencies.
+    // We want to index targets in the root package first because those are likely the files that the user is interested
+    // in editing. We want to index top-level targets first, because preparing those likely implies preparation of the
+    // low-level targets.
+    let workList = targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
+      let lhsTarget = buildTargets[lhs]
+      let rhsTarget = buildTargets[rhs]
+
+      switch (lhsTarget?.target.tags.contains(.dependency), rhsTarget?.target.tags.contains(.dependency)) {
+      case (true, false): return false
+      case (false, true): return true
+      default: break
+      }
+
+      let lhsDepth = lhsTarget?.depth ?? 0
+      let rhsDepth = rhsTarget?.depth ?? 0
       if lhsDepth != rhsDepth {
-        return lhsDepth > rhsDepth
+        return lhsDepth < rhsDepth
       }
+      // Use the target's name as a tie-breaker
       return lhs.uri.stringValue < rhs.uri.stringValue
     }
+
+    // Now walk through the list of targets in the work list. For each target from the work list, index all of its
+    // transitive dependencies next. We do this because preparing a top-level target likely also prepared all of its
+    // dependencies, so we should be able to index all files in the target's dependencies without needing to perform any
+    // target preparation.
+    var sorted: [BuildTargetIdentifier] = []
+    // A `Set` representation of `sorted` to efficiently check if `target` is already in `sorted` and should be skipped.
+    var visited: Set<BuildTargetIdentifier> = []
+    for target in workList where !visited.contains(target) {
+      sorted.append(target)
+      visited.insert(target)
+
+      let transitiveDependencies = transitiveClosure(of: [target]) { Set(buildTargets[$0]?.target.dependencies ?? []) }
+      let dependenciesInWorkList = workList.filter { transitiveDependencies.contains($0) }
+      sorted += dependenciesInWorkList
+      visited.formUnion(dependenciesInWorkList)
+    }
+
+    return sorted
   }
 
   /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in
@@ -1499,14 +1530,18 @@ package actor BuildServerManager: QueueBasedMessageHandler {
       .sorted { $0.uri.stringValue < $1.uri.stringValue }
   }
 
-  package func prepare(targets: Set<BuildTargetIdentifier>) async throws {
-    let _: BuildTargetPrepareResponse? = try await buildServerAdapterAfterInitialized?.send(
+  package func prepare(targets: Set<BuildTargetIdentifier>) async throws -> BuildTargetPrepareResponse {
+    guard let buildServerAdapterAfterInitialized = try await buildServerAdapterAfterInitialized else {
+      throw ResponseError.unknown("No connection to build server")
+    }
+    let response = try await buildServerAdapterAfterInitialized.send(
       BuildTargetPrepareRequest(targets: targets.sorted { $0.uri.stringValue < $1.uri.stringValue })
     )
     await orLog("Calling fileDependenciesUpdated") {
       let filesInPreparedTargets = try await self.sourceFiles(in: targets).flatMap(\.sources).map(\.uri)
       await filesDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets))
     }
+    return response
   }
 
   package func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {

Sources/BuildServerIntegration/SwiftPMBuildServer.swift

@@ -781,10 +781,15 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
 
   package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
     // TODO: Support preparation of multiple targets at once. (https://github.com/swiftlang/sourcekit-lsp/issues/1262)
+    var implicitlyPreparedTargets: [BuildTargetIdentifier] = []
     for target in request.targets {
-      await orLog("Preparing") { try await prepare(singleTarget: target) }
+      await orLog("Preparing") {
+        try await prepare(singleTarget: target)
+        implicitlyPreparedTargets += transitiveClosure(of: [target], successors: { targetDependencies[$0] ?? [] })
+          .filter { !request.targets.contains($0) }
+      }
     }
-    return BuildTargetPrepareResponse()
+    return BuildTargetPrepareResponse(implicitlyPreparedTargets: implicitlyPreparedTargets)
   }
 
   private func prepare(singleTarget target: BuildTargetIdentifier) async throws {

Sources/SemanticIndex/PreparationTaskDescription.swift

@@ -111,14 +111,19 @@ package struct PreparationTaskDescription: IndexTaskDescription {
         )
         signposter.endInterval("Preparing", state)
       }
+      let prepareResponse: BuildTargetPrepareResponse?
       do {
-        try await buildServerManager.prepare(targets: Set(targetsToPrepare))
+        prepareResponse = try await buildServerManager.prepare(targets: Set(targetsToPrepare))
       } catch {
         logger.error("Preparation failed: \(error.forLogging)")
+        prepareResponse = nil
       }
       await hooks.preparationTaskDidFinish?(self)
       if !Task.isCancelled {
-        await preparationUpToDateTracker.markUpToDate(targetsToPrepare, updateOperationStartDate: startDate)
+        await preparationUpToDateTracker.markUpToDate(
+          targetsToPrepare + (prepareResponse?.implicitlyPreparedTargets ?? []),
+          updateOperationStartDate: startDate
+        )
       }
     }
   }

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -960,7 +960,9 @@ package final actor SemanticIndexManager {
     // The targets sorted in reverse topological order, low-level targets before high-level targets. If topological
     // sorting fails, sorted in another deterministic way where the actual order doesn't matter.
     var sortedTargets: [BuildTargetIdentifier] =
-      await orLog("Sorting targets") { try await buildServerManager.topologicalSort(of: Array(filesByTarget.keys)) }
+      await orLog("Sorting targets") {
+        try await buildServerManager.targetsSortedForIndexing(Array(filesByTarget.keys))
+      }
       ?? Array(filesByTarget.keys).sorted { $0.uri.stringValue < $1.uri.stringValue }
 
     if Set(sortedTargets) != Set(filesByTarget.keys) {

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -528,13 +528,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase {
   func testPrepareTargetAfterEditToDependency() async throws {
     var testHooks = Hooks()
     let expectedPreparationTracker = ExpectedIndexTaskTracker(expectedPreparations: [
-      [
-        try ExpectedPreparation(target: "LibA", destination: .target),
-        try ExpectedPreparation(target: "LibB", destination: .target),
-      ],
-      [
-        try ExpectedPreparation(target: "LibB", destination: .target)
-      ],
+      [try ExpectedPreparation(target: "LibB", destination: .target)],
+      [try ExpectedPreparation(target: "LibB", destination: .target)],
     ])
     testHooks.indexHooks = expectedPreparationTracker.testHooks
 
@@ -626,9 +621,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase {
 
     var testHooks = Hooks()
     let expectedPreparationTracker = ExpectedIndexTaskTracker(expectedPreparations: [
-      // Preparation of targets during the initial of the target
+      // Preparation of targets during the initial indexing of the target
       [
-        try ExpectedPreparation(target: "LibA", destination: .target),
         try ExpectedPreparation(target: "LibB", destination: .target),
         try ExpectedPreparation(target: "LibC", destination: .target),
         try ExpectedPreparation(target: "LibD", destination: .target),
@@ -2637,33 +2631,58 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase {
         }
       })
     )
-    let project = try await SwiftPMTestProject(
+    let project = try await MultiFileTestProject(
       files: [
-        "LibA/LibA.swift": "",
-        "LibB/LibB.swift": "",
-      ],
-      manifest: """
+        "MyDependency/Sources/DependencyA/DependencyA.swift": "",
+        "MyDependency/Package.swift": """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
         let package = Package(
-          name: "MyLibrary",
+          name: "MyDependency",
+          products: [.library(name: "DependencyA", targets: ["DependencyA"])],
           targets: [
-           .target(name: "LibA"),
-           .target(name: "LibB", dependencies: ["LibA"])
+            .target(name: "DependencyA"),
           ]
         )
         """,
+
+        "MyPackage/Sources/LibA/LibA.swift": "",
+        "MyPackage/Sources/LibB/LibB.swift": "",
+        "MyPackage/Sources/LibC/LibC.swift": "",
+        "MyPackage/Package.swift": """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
+        let package = Package(
+          name: "MyPackage",
+          dependencies: [.package(path: "../MyDependency")],
+          targets: [
+           .target(name: "LibA", dependencies: [.product(name: "DependencyA", package: "MyDependency")]),
+           .target(name: "LibB", dependencies: ["LibA"]),
+           .target(name: "LibC")
+          ]
+        )
+        """,
+      ],
+      workspaces: { scratchDirectory in
+        [WorkspaceFolder(uri: DocumentURI(scratchDirectory.appending(component: "MyPackage")))]
+      },
       hooks: testHooks,
-      enableBackgroundIndexing: true,
-      pollIndex: false
+      enableBackgroundIndexing: true
     )
     // We can't poll the index using `workspace/synchronize` because that elevates the priority of the indexing requests
-    // in a non-deterministic order (due to the way ). If LibB's priority gets elevated before LibA's, then LibB will
-    // get prepared first, which is contrary to the background behavior we want to check here.
+    // in a non-deterministic order (due to the way `withTaskPriorityChangedHandler` is implemented). If LibB's priority
+    // gets elevated before LibA's, then LibB will get prepared first, which is contrary to the background behavior we
+    // want to check here.
     try await fulfillmentOfOrThrow(twoPreparationRequestsReceived)
     XCTAssertEqual(
       preparationRequests.value.flatMap(\.targets),
       [
-        try BuildTargetIdentifier(target: "LibA", destination: .target),
         try BuildTargetIdentifier(target: "LibB", destination: .target),
+        try BuildTargetIdentifier(target: "LibC", destination: .target),
       ]
     )
     withExtendedLifetime(project) {}
@@ -2717,8 +2736,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase {
         return TextDocumentSourceKitOptionsResponse(compilerArguments: arguments)
       }
 
-      func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> VoidResponse {
-        return VoidResponse()
+      func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
+        return BuildTargetPrepareResponse()
       }
     }