Merge pull request #1313 from ahoppen/only-mark-dependent-targets-as-out-of-date

When a file is changed, only mark targets that depend on it as out-of-date

Author: ahoppen

Sources/SKCore/BuildServerBuildSystem.swift

@@ -285,6 +285,10 @@ extension BuildServerBuildSystem: BuildSystem {
     return nil
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
+    return nil
+  }
+
   public func prepare(targets: [ConfiguredTarget]) async throws {
     throw PrepareNotSupportedError()
   }

Sources/SKCore/BuildSystem.swift

@@ -147,6 +147,15 @@ public protocol BuildSystem: AnyObject, Sendable {
   /// `nil` if the build system doesn't support topological sorting of targets.
   func topologicalSort(of targets: [ConfiguredTarget]) async -> [ConfiguredTarget]?
 
+  /// 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: [ConfiguredTarget]) async -> [ConfiguredTarget]?
+
   /// Prepare the given targets for indexing and semantic functionality. This should build all swift modules of target
   /// dependencies.
   func prepare(targets: [ConfiguredTarget]) async throws

Sources/SKCore/BuildSystemManager.swift

@@ -126,12 +126,17 @@ extension BuildSystemManager {
     }
   }
 
+  /// Returns all the `ConfiguredTarget`s that the document is part of.
+  public func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] {
+    return await buildSystem?.configuredTargets(for: document) ?? []
+  }
+
   /// Returns the `ConfiguredTarget` that should be used for semantic functionality of the given document.
   public func canonicalConfiguredTarget(for document: DocumentURI) async -> ConfiguredTarget? {
     // Sort the configured targets to deterministically pick the same `ConfiguredTarget` every time.
     // We could allow the user to specify a preference of one target over another. For now this is not necessary because
     // no build system currently returns multiple targets for a source file.
-    return await buildSystem?.configuredTargets(for: document)
+    return await configuredTargets(for: document)
       .sorted { ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID) }
       .first
   }
@@ -220,6 +225,10 @@ extension BuildSystemManager {
     return await buildSystem?.topologicalSort(of: targets)
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) async -> [ConfiguredTarget]? {
+    return await buildSystem?.targets(dependingOn: targets)
+  }
+
   public func prepare(targets: [ConfiguredTarget]) async throws {
     try await buildSystem?.prepare(targets: targets)
   }

Sources/SKCore/CompilationDatabaseBuildSystem.swift

@@ -135,6 +135,10 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     return nil
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
+    return nil
+  }
+
   public func registerForChangeNotifications(for uri: DocumentURI) async {
     self.watchedFiles.insert(uri)
   }

Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift

@@ -63,6 +63,14 @@ private func getDefaultToolchain(_ toolchainRegistry: ToolchainRegistry) async -
   return await toolchainRegistry.default
 }
 
+fileprivate extension ConfiguredTarget {
+  init(_ buildTarget: any SwiftBuildTarget) {
+    self.init(targetID: buildTarget.name, runDestinationID: "dummy")
+  }
+
+  static let forPackageManifest = ConfiguredTarget(targetID: "", runDestinationID: "")
+}
+
 /// Swift Package Manager build system and workspace support.
 ///
 /// This class implements the `BuildSystem` interface to provide the build settings for a Swift
@@ -101,10 +109,10 @@ public actor SwiftPMBuildSystem {
   var fileToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
   var sourceDirToTarget: [AbsolutePath: 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)] = [:]
+  /// Maps configured targets ids 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: [ConfiguredTarget: (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.
@@ -289,7 +297,7 @@ extension SwiftPMBuildSystem {
 
     self.targets = Dictionary(
       try buildDescription.allTargetsInTopologicalOrder(in: modulesGraph).enumerated().map { (index, target) in
-        return (key: target.name, (index, target))
+        return (key: ConfiguredTarget(target), (index, target))
       },
       uniquingKeysWith: { first, second in
         logger.fault("Found two targets with the same name \(first.buildTarget.name)")
@@ -362,11 +370,11 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
       return nil
     }
 
-    if configuredTarget.targetID == "" {
+    if configuredTarget == .forPackageManifest {
       return try settings(forPackageManifest: path)
     }
 
-    guard let buildTarget = self.targets[configuredTarget.targetID]?.buildTarget else {
+    guard let buildTarget = self.targets[configuredTarget]?.buildTarget else {
       logger.error("Did not find target with name \(configuredTarget.targetID)")
       return nil
     }
@@ -406,13 +414,13 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
     }
 
     if let target = try? buildTarget(for: path) {
-      return [ConfiguredTarget(targetID: target.name, runDestinationID: "dummy")]
+      return [ConfiguredTarget(target)]
     }
 
     if path.basename == "Package.swift" {
       // We use an empty target name to represent the package manifest since an empty target name is not valid for any
       // user-defined target.
-      return [ConfiguredTarget(targetID: "", runDestinationID: "dummy")]
+      return [ConfiguredTarget.forPackageManifest]
     }
 
     if let target = try? inferredTarget(for: path) {
@@ -424,19 +432,40 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
 
   public func topologicalSort(of targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
     return targets.sorted { (lhs: ConfiguredTarget, rhs: ConfiguredTarget) -> Bool in
-      let lhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
-      let rhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
+      let lhsIndex = self.targets[lhs]?.index ?? self.targets.count
+      let rhsIndex = self.targets[lhs]?.index ?? self.targets.count
       return lhsIndex < rhsIndex
     }
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
+    let targetIndices = targets.compactMap { self.targets[$0]?.index }
+    let minimumTargetIndex: Int?
+    if targetIndices.count == targets.count {
+      minimumTargetIndex = targetIndices.min()
+    } else {
+      // One of the targets didn't have an entry in self.targets. We don't know what might depend on it.
+      minimumTargetIndex = 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/apple/sourcekit-lsp/issues/1312).
+    return self.targets.compactMap { (configuredTarget, value) -> ConfiguredTarget? in
+      if let minimumTargetIndex, value.index <= minimumTargetIndex {
+        return nil
+      }
+      return configuredTarget
+    }
+  }
+
   public func prepare(targets: [ConfiguredTarget]) async throws {
     // TODO (indexing): Support preparation of multiple targets at once.
     // https://github.com/apple/sourcekit-lsp/issues/1262
     for target in targets {
       try await prepare(singleTarget: target)
     }
-    let filesInPreparedTargets = targets.flatMap { self.targets[$0.targetID]?.buildTarget.sources ?? [] }
+    let filesInPreparedTargets = targets.flatMap { self.targets[$0]?.buildTarget.sources ?? [] }
     await fileDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets.map(DocumentURI.init)))
   }
 
@@ -631,7 +660,7 @@ extension SwiftPMBuildSystem {
       var dir = path.parentDirectory
       while !dir.isRoot {
         if let buildTarget = sourceDirToTarget[dir] {
-          return ConfiguredTarget(targetID: buildTarget.name, runDestinationID: "dummy")
+          return ConfiguredTarget(buildTarget)
         }
         dir = dir.parentDirectory
       }

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -209,11 +209,19 @@ public final actor SemanticIndexManager {
     for uri in changedFiles {
       indexStatus[uri] = nil
     }
-    // Clear the preparation status so that we re-prepare them. If the target hasn't been affected by the file changes,
-    // we rely on a null build during preparation to fast re-preparation.
-    // Ideally, we would have more fine-grained dependency management here and only mark those targets out-of-date that
-    // might be affected by the changed files.
-    preparationStatus.removeAll()
+    // Note that configured targets are the right abstraction layer here (instead of a non-configured target) because a
+    // build system might have targets that include different source files. Hence a source file might be in target T
+    // configured for macOS but not in target T configured for iOS.
+    let targets = await changedFiles.asyncMap { await buildSystemManager.configuredTargets(for: $0) }.flatMap { $0 }
+    if let dependentTargets = await buildSystemManager.targets(dependingOn: targets) {
+      for dependentTarget in dependentTargets {
+        preparationStatus[dependentTarget] = nil
+      }
+    } else {
+      // We couldn't determine which targets depend on the modified targets. Be conservative and assume all of them do.
+      preparationStatus = [:]
+    }
+
     await scheduleBackgroundIndex(files: changedFiles)
   }
 

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -477,6 +477,10 @@ class ManualBuildSystem: BuildSystem {
     return nil
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
+    return nil
+  }
+
   func registerForChangeNotifications(for uri: DocumentURI) async {
   }
 

Tests/SourceKitLSPTests/BuildSystemTests.swift

@@ -69,6 +69,10 @@ actor TestBuildSystem: BuildSystem {
     return nil
   }
 
+  public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
+    return nil
+  }
+
   func registerForChangeNotifications(for uri: DocumentURI) async {
     watchedFiles.insert(uri)
   }