Address review comments

Author: ahoppen

Sources/SKCore/BuildSystemManager.swift

@@ -212,8 +212,8 @@ extension BuildSystemManager {
     try await self.buildSystem?.generateBuildGraph()
   }
 
-  public func topologicalSort(of targets: [ConfiguredTarget]) async throws -> [ConfiguredTarget] {
-    return await buildSystem?.topologicalSort(of: targets) ?? targets
+  public func topologicalSort(of targets: [ConfiguredTarget]) async throws -> [ConfiguredTarget]? {
+    return await buildSystem?.topologicalSort(of: targets)
   }
 
   public func prepare(targets: [ConfiguredTarget]) async throws {

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -100,7 +100,11 @@ public actor SwiftPMBuildSystem {
 
   var fileToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
   var sourceDirToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
-  var targets: [SwiftBuildTarget] = []
+
+  /// Maps target ids (aka. `ConfiguredTarget.targetID`) to their SwiftPM build target as well as an index in their
+  /// topological sorting. Targets with lower index are more low level, ie. targets with higher indices depend on
+  /// targets with lower indices.
+  var targets: [String: (index: Int, buildTarget: SwiftBuildTarget)] = [:]
 
   /// The URIs for which the delegate has registered for change notifications,
   /// mapped to the language the delegate specified when registering for change notifications.
@@ -119,6 +123,11 @@ public actor SwiftPMBuildSystem {
   /// Force-unwrapped optional because initializing it requires access to `self`.
   var fileDependenciesUpdatedDebouncer: Debouncer<Set<DocumentURI>>! = nil
 
+  /// A `ObservabilitySystem` from `SwiftPM` that logs.
+  private let observabilitySystem = ObservabilitySystem({ scope, diagnostic in
+    logger.log(level: diagnostic.severity.asLogLevel, "SwiftPM log: \(diagnostic.description)")
+  })
+
   /// Creates a build system using the Swift Package Manager, if this workspace is a package.
   ///
   /// - Parameters:
@@ -238,9 +247,6 @@ public actor SwiftPMBuildSystem {
 
 extension SwiftPMBuildSystem {
   public func generateBuildGraph() async throws {
-    let observabilitySystem = ObservabilitySystem({ scope, diagnostic in
-      logger.log(level: diagnostic.severity.asLogLevel, "SwiftPM log: \(diagnostic.description)")
-    })
     try self.workspace.resolve(
       root: PackageGraphRootInput(packages: [AbsolutePath(projectRoot)]),
       observabilityScope: observabilitySystem.topScope
@@ -258,10 +264,6 @@ extension SwiftPMBuildSystem {
       }
     }
 
-    let observabilitySystem = ObservabilitySystem({ scope, diagnostic in
-      logger.log(level: diagnostic.severity.asLogLevel, "SwiftPM log: \(diagnostic.description)")
-    })
-
     let modulesGraph = try self.workspace.loadPackageGraph(
       rootInput: PackageGraphRootInput(packages: [AbsolutePath(projectRoot)]),
       forceResolvedVersions: true,
@@ -282,7 +284,15 @@ extension SwiftPMBuildSystem {
     /// with only some properties modified.
     self.modulesGraph = modulesGraph
 
-    self.targets = try buildDescription.allTargetsInTopologicalOrder(in: modulesGraph)
+    self.targets = Dictionary(
+      try buildDescription.allTargetsInTopologicalOrder(in: modulesGraph).enumerated().map { (index, target) in
+        return (key: target.name, (index, target))
+      },
+      uniquingKeysWith: { first, second in
+        logger.fault("Found two targets with the same name \(first.buildTarget.name)")
+        return second
+      }
+    )
 
     self.fileToTarget = [AbsolutePath: SwiftBuildTarget](
       modulesGraph.allTargets.flatMap { target in
@@ -353,14 +363,8 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
       return try settings(forPackageManifest: path)
     }
 
-    let buildTargets = self.targets.filter({ $0.name == configuredTarget.targetID })
-    if buildTargets.count > 1 {
-      logger.error("Found multiple targets with name \(configuredTarget.targetID). Picking the first one")
-    }
-    guard let buildTarget = buildTargets.first else {
-      if buildTargets.isEmpty {
-        logger.error("Did not find target with name \(configuredTarget.targetID)")
-      }
+    guard let buildTarget = self.targets[configuredTarget.targetID]?.buildTarget else {
+      logger.error("Did not find target with name \(configuredTarget.targetID)")
       return nil
     }
 
@@ -408,8 +412,8 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
 
   public func topologicalSort(of targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
     return targets.sorted { (lhs: ConfiguredTarget, rhs: ConfiguredTarget) -> Bool in
-      let lhsIndex = self.targets.firstIndex(where: { $0.name == lhs.targetID }) ?? self.targets.count
-      let rhsIndex = self.targets.firstIndex(where: { $0.name == rhs.targetID }) ?? self.targets.count
+      let lhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
+      let rhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
       return lhsIndex < rhsIndex
     }
   }
@@ -443,7 +447,7 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
     ]
     let process = Process(
       arguments: arguments,
-      workingDirectory: try TSCBasic.AbsolutePath(validating: workspacePath.pathString)
+      workingDirectory: workspacePath
     )
     try process.launch()
     let result = try await process.waitUntilExitSendingSigIntOnTaskCancellation()
@@ -456,7 +460,7 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
       let stderr = (try? String(bytes: result.stderrOutput.get(), encoding: .utf8)) ?? "<no stderr>"
       logger.debug(
         """
-        Preparation of targets \(target.targetID) terminated with non-zero exit code \(code)
+        Preparation of target \(target.targetID) terminated with non-zero exit code \(code)
         Stderr:
         \(stderr)
         Stdout:
@@ -468,11 +472,11 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
         // The indexing job finished with a signal. Could be because the compiler crashed.
         // Ignore signal exit codes if this task has been cancelled because the compiler exits with SIGINT if it gets
         // interrupted.
-        logger.error("Preparation of targets \(target.targetID) signaled \(signal)")
+        logger.error("Preparation of target \(target.targetID) signaled \(signal)")
       }
     case .abnormal(exception: let exception):
       if !Task.isCancelled {
-        logger.error("Preparation of targets \(target.targetID) exited abnormally \(exception)")
+        logger.error("Preparation of target \(target.targetID) exited abnormally \(exception)")
       }
     }
   }

Sources/SKTestSupport/FileManager+findFiles.swift

@@ -25,7 +25,7 @@ extension FileManager {
     return result
   }
 
-  /// Returns the URLs of all files with the given file extension in the given directory (recursively).
+  /// Returns the URLs of all files with the given file name in the given directory (recursively).
   public func findFiles(named name: String, in directory: URL) -> [URL] {
     var result: [URL] = []
     let enumerator = self.enumerator(at: directory, includingPropertiesForKeys: nil)

Sources/SemanticIndex/PreparationTaskDescription.swift

@@ -21,13 +21,13 @@ import class TSCBasic.Process
 
 private var preparationIDForLogging = AtomicUInt32(initialValue: 1)
 
-/// Describes a task to index a set of source files.
+/// Describes a task to prepare a set of targets.
 ///
 /// This task description can be scheduled in a `TaskScheduler`.
 public struct PreparationTaskDescription: TaskDescriptionProtocol {
   public let id = preparationIDForLogging.fetchAndIncrement()
 
-  /// The files that should be indexed.
+  /// The targets that should be prepared.
   private let targetsToPrepare: [ConfiguredTarget]
 
   /// The build system manager that is used to get the toolchain and build settings for the files to index.
@@ -38,7 +38,7 @@ public struct PreparationTaskDescription: TaskDescriptionProtocol {
   /// Intended for testing purposes.
   private let didFinishCallback: @Sendable (PreparationTaskDescription) -> Void
 
-  /// The task is idempotent because indexing the same file twice produces the same result as indexing it once.
+  /// The task is idempotent because preparing the same target twice produces the same result as preparing it once.
   public var isIdempotent: Bool { true }
 
   public var estimatedCPUCoreCount: Int { 1 }
@@ -73,7 +73,10 @@ public struct PreparationTaskDescription: TaskDescriptionProtocol {
       let targetsToPrepare = targetsToPrepare.sorted(by: {
         ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID)
       })
-      let targetsToPrepareDescription = targetsToPrepare.map { $0.targetID }.joined(separator: ", ")
+      let targetsToPrepareDescription =
+        targetsToPrepare
+        .map { "\($0.targetID)-\($0.runDestinationID)" }
+        .joined(separator: ", ")
       logger.log(
         "Starting preparation with priority \(Task.currentPriority.rawValue, privacy: .public): \(targetsToPrepareDescription)"
       )

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -191,7 +191,9 @@ public final actor SemanticIndexManager {
 
     var sortedTargets: [ConfiguredTarget] =
       await orLog("Sorting targets") { try await buildSystemManager.topologicalSort(of: Array(filesByTarget.keys)) }
-      ?? Array(filesByTarget.keys).sorted(by: { $0.targetID < $1.targetID })
+      ?? Array(filesByTarget.keys).sorted(by: {
+        ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID)
+      })
 
     if Set(sortedTargets) != Set(filesByTarget.keys) {
       logger.fault(
@@ -200,7 +202,9 @@ public final actor SemanticIndexManager {
         \(sortedTargets.map(\.targetID).joined(separator: ", ")) != \(filesByTarget.keys.map(\.targetID).joined(separator: ", "))
         """
       )
-      sortedTargets = Array(filesByTarget.keys).sorted(by: { $0.targetID < $1.targetID })
+      sortedTargets = Array(filesByTarget.keys).sorted(by: {
+        ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID)
+      })
     }
 
     var indexTasks: [Task<Void, Never>] = []

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -106,6 +106,7 @@ final class BackgroundIndexingTests: XCTestCase {
   }
 
   func testBackgroundIndexingOfMultiModuleProject() async throws {
+    try await SkipUnless.swiftpmStoresModulesInSubdirectory()
     let project = try await SwiftPMTestProject(
       files: [
         "LibA/MyFile.swift": """
@@ -211,6 +212,7 @@ final class BackgroundIndexingTests: XCTestCase {
   }
 
   func testBackgroundIndexingOfPackageDependency() async throws {
+    try await SkipUnless.swiftpmStoresModulesInSubdirectory()
     let dependencyContents = """
       public func 1️⃣doSomething() {}
       """