Improve handling of main file vs header file during indexing

Essentially fix two issues in updating the index store:
1. If there was one task to index `HeaderA.h` through `main.c` and one to index `HeaderB.h` through `main.c`, we would not declare a dependency between them in the task scheduler, which meant that we could have two concurrent and racing index tasks for `main.c`. Declare a dependency between any two files that have the same main file
2. `UpdateIndexStoreTaskDescription` was computing the target to index a file in independently of `SemanticIndexManager`. While they currently always line up, we should pass the target in which to index a file to the `UpdateIndexStoreTaskDescription`. Only this way can we guarantee that we actually prepared the target that the file will be indexed in.

Author: ahoppen

Sources/SKCore/BuildSystemManager.swift

@@ -141,13 +141,10 @@ extension BuildSystemManager {
   /// Implementation detail of `buildSettings(for:language:)`.
   private func buildSettingsFromPrimaryBuildSystem(
     for document: DocumentURI,
+    in target: ConfiguredTarget?,
     language: Language
   ) async throws -> FileBuildSettings? {
-    guard let buildSystem else {
-      return nil
-    }
-    guard let target = await canonicalConfiguredTarget(for: document) else {
-      logger.error("Failed to get target for \(document.forLogging)")
+    guard let buildSystem, let target else {
       return nil
     }
     // FIXME: (async) We should only wait `fallbackSettingsTimeout` for build
@@ -161,12 +158,22 @@ extension BuildSystemManager {
     return settings
   }
 
-  private func buildSettings(
+  /// Returns the build settings for the given file in the given target.
+  ///
+  /// Only call this method if it is known that `document` is a main file. Prefer `buildSettingsInferredFromMainFile`
+  /// otherwise. If `document` is a header file, this will most likely return fallback settings because header files
+  /// don't have build settings by themselves.
+  public func buildSettings(
     for document: DocumentURI,
+    in target: ConfiguredTarget?,
     language: Language
   ) async -> FileBuildSettings? {
     do {
-      if let buildSettings = try await buildSettingsFromPrimaryBuildSystem(for: document, language: language) {
+      if let buildSettings = try await buildSettingsFromPrimaryBuildSystem(
+        for: document,
+        in: target,
+        language: language
+      ) {
         return buildSettings
       }
     } catch {
@@ -194,21 +201,22 @@ extension BuildSystemManager {
   /// references to that C file in the build settings by the header file.
   public func buildSettingsInferredFromMainFile(
     for document: DocumentURI,
-    language: Language,
-    logBuildSettings: Bool = true
+    language: Language
   ) async -> FileBuildSettings? {
     let mainFile = await mainFile(for: document, language: language)
-    guard var settings = await buildSettings(for: mainFile, language: language) else {
+    guard let target = await canonicalConfiguredTarget(for: document) else {
+      logger.error("Failed to get target for \(document.forLogging)")
+      return nil
+    }
+    guard var settings = await buildSettings(for: mainFile, in: target, language: language) else {
       return nil
     }
     if mainFile != document {
       // If the main file isn't the file itself, we need to patch the build settings
       // to reference `document` instead of `mainFile`.
       settings = settings.patching(newFile: document.pseudoPath, originalFile: mainFile.pseudoPath)
     }
-    if logBuildSettings {
-      await BuildSettingsLogger.shared.log(settings: settings, for: document)
-    }
+    await BuildSettingsLogger.shared.log(settings: settings, for: document)
     return settings
   }
 

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -221,12 +221,14 @@ public final actor SemanticIndexManager {
   ///
   /// If `files` contains a header file, this will return a `FileToIndex` that re-indexes a main file which includes the
   /// header file to update the header file's index.
-  private func filesToIndex(toCover files: some Collection<DocumentURI>) async -> [FileToIndex] {
+  private func filesToIndex(
+    toCover files: some Collection<DocumentURI>
+  ) async -> [FileToIndex] {
     let sourceFiles = Set(await buildSystemManager.sourceFiles().map(\.uri))
     let filesToReIndex = await files.asyncCompactMap { (uri) -> FileToIndex? in
       if sourceFiles.contains(uri) {
         // If this is a source file, just index it.
-        return FileToIndex(uri: uri, mainFile: nil)
+        return .indexableFile(uri)
       }
       // Otherwise, see if it is a header file. If so, index a main file that that imports it to update header file's
       // index.
@@ -239,7 +241,7 @@ public final actor SemanticIndexManager {
       guard let mainFile else {
         return nil
       }
-      return FileToIndex(uri: uri, mainFile: mainFile)
+      return .headerFile(header: uri, mainFile: mainFile)
     }
     return filesToReIndex
   }
@@ -338,10 +340,10 @@ public final actor SemanticIndexManager {
   }
 
   /// Update the index store for the given files, assuming that their targets have already been prepared.
-  private func updateIndexStore(for files: [FileToIndex], taskID: UUID, priority: TaskPriority?) async {
+  private func updateIndexStore(for files: [FileAndTarget], taskID: UUID, priority: TaskPriority?) async {
     let taskDescription = AnyIndexTaskDescription(
       UpdateIndexStoreTaskDescription(
-        filesToIndex: Set(files),
+        filesToIndex: files,
         buildSystemManager: self.buildSystemManager,
         index: index
       )
@@ -350,21 +352,21 @@ public final actor SemanticIndexManager {
       switch newState {
       case .executing:
         for file in files {
-          if case .scheduled((taskID, let task)) = self.indexStatus[file.uri] {
-            self.indexStatus[file.uri] = .executing((taskID, task))
+          if case .scheduled((taskID, let task)) = self.indexStatus[file.file.sourceFile] {
+            self.indexStatus[file.file.sourceFile] = .executing((taskID, task))
           }
         }
       case .cancelledToBeRescheduled:
         for file in files {
-          if case .executing((taskID, let task)) = self.indexStatus[file.uri] {
-            self.indexStatus[file.uri] = .scheduled((taskID, task))
+          if case .executing((taskID, let task)) = self.indexStatus[file.file.sourceFile] {
+            self.indexStatus[file.file.sourceFile] = .scheduled((taskID, task))
           }
         }
       case .finished:
         for file in files {
-          switch self.indexStatus[file.uri] {
+          switch self.indexStatus[file.file.sourceFile] {
           case .executing((taskID, _)):
-            self.indexStatus[file.uri] = .upToDate
+            self.indexStatus[file.file.sourceFile] = .upToDate
           default:
             break
           }
@@ -383,24 +385,25 @@ public final actor SemanticIndexManager {
     priority: TaskPriority?
   ) async -> Task<Void, Never> {
     let outOfDateFiles = await filesToIndex(toCover: files).filter {
-      if case .upToDate = indexStatus[$0.uri] {
+      if case .upToDate = indexStatus[$0.sourceFile] {
         return false
       }
       return true
     }
-    .sorted(by: { $0.uri.stringValue < $1.uri.stringValue })  // sort files to get deterministic indexing order
+    // sort files to get deterministic indexing order
+    .sorted(by: { $0.sourceFile.stringValue < $1.sourceFile.stringValue })
 
     // Sort the targets in topological order so that low-level targets get built before high-level targets, allowing us
     // to index the low-level targets ASAP.
     var filesByTarget: [ConfiguredTarget: [FileToIndex]] = [:]
-    for file in outOfDateFiles {
-      guard let target = await buildSystemManager.canonicalConfiguredTarget(for: file.mainFile ?? file.uri) else {
+    for fileToIndex in outOfDateFiles {
+      guard let target = await buildSystemManager.canonicalConfiguredTarget(for: fileToIndex.mainFile) else {
         logger.error(
-          "Not indexing \(file.uri.forLogging) because the target could not be determined for main file \(file.mainFile?.forLogging)"
+          "Not indexing \(fileToIndex.forLogging) because the target could not be determined"
         )
         continue
       }
-      filesByTarget[target, default: []].append(file)
+      filesByTarget[target, default: []].append(fileToIndex)
     }
 
     var sortedTargets: [ConfiguredTarget] =
@@ -440,7 +443,11 @@ public final actor SemanticIndexManager {
             // https://github.com/apple/sourcekit-lsp/issues/1268
             for fileBatch in filesByTarget[target]!.partition(intoBatchesOfSize: 1) {
               taskGroup.addTask {
-                await self.updateIndexStore(for: fileBatch, taskID: taskID, priority: priority)
+                await self.updateIndexStore(
+                  for: fileBatch.map { FileAndTarget(file: $0, target: target) },
+                  taskID: taskID,
+                  priority: priority
+                )
               }
             }
           }
@@ -455,7 +462,7 @@ public final actor SemanticIndexManager {
         // setting it to `.scheduled` because we don't have an `await` call between the creation of `indexTask` and
         // this loop, so we still have exclusive access to the `SemanticIndexManager` actor and hence `updateIndexStore`
         // can't execute until we have set all index statuses to `.scheduled`.
-        indexStatus[file.uri] = .scheduled((taskID, indexTask))
+        indexStatus[file.sourceFile] = .scheduled((taskID, indexTask))
       }
       indexTasksWereScheduled(filesToIndex.count)
     }