Merge pull request #1309 from ahoppen/review-comments-1306

Address review comments to #1306

Author: ahoppen

Documentation/Files_To_Reindex.md

@@ -18,7 +18,9 @@ Alternatives would be:
 
 ## `.h`
 
-Affects all files that include the header (including via other headers). For existing headers, we know which files import a header from the existing index. For new header files, we assume that it hasn’t been included in any file yet and so we don’t re-index any file. If it is, it’s likely that the user will modify the file that includes the new header file shortly after, which will re-index it.
+All files that include the header (including via other headers) might be affected by the change, similar to how all `.swift` files that import a module might be affected. Similar to modules, we choose to not re-index all files that include the header because of the same considerations mentioned above.
+
+To re-index the header, we pick one main file that includes the header and re-index that, which will effectively update the index for the header. For existing headers, we know which files import a header from the existing index. For new header files, we assume that it hasn’t been included in any file yet and thus don't index it. If the user wrote an include to the new header before creating the header itself, we don't know about that include from the existing index. But it’s likely that the user will modify the file that includes the new header file shortly after, which will index the header and establish the header to main file connection.
 
 ## `.c` / `.cpp` / `.m`
 

Sources/LSPLogging/NonDarwinLogging.swift

@@ -195,6 +195,17 @@ public struct NonDarwinLogInterpolation: StringInterpolationProtocol, Sendable {
     append(description: message.description, redactedDescription: message.redactedDescription, privacy: privacy)
   }
 
+  public mutating func appendInterpolation(
+    _ message: (some CustomLogStringConvertibleWrapper & Sendable)?,
+    privacy: NonDarwinLogPrivacy = .private
+  ) {
+    if let message {
+      self.appendInterpolation(message, privacy: privacy)
+    } else {
+      self.appendLiteral("<nil>")
+    }
+  }
+
   public mutating func appendInterpolation(_ type: Any.Type, privacy: NonDarwinLogPrivacy = .public) {
     append(description: String(reflecting: type), redactedDescription: "<private>", privacy: privacy)
   }

Sources/SKCore/BuildSystemManager.swift

@@ -146,32 +146,36 @@ 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
     // settings and return fallback afterwards. I am not sure yet, how best to
     // implement that with Swift concurrency.
     // For now, this should be fine because all build systems return
     // very quickly from `settings(for:language:)`.
-    guard let settings = try await buildSystem.buildSettings(for: document, in: target, language: language) else {
-      return nil
-    }
-    return settings
+    return try await buildSystem.buildSettings(for: document, in: target, language: language)
   }
 
-  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 {
@@ -199,21 +203,19 @@ 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 {
+    let target = await canonicalConfiguredTarget(for: mainFile)
+    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

@@ -229,12 +229,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.
@@ -247,7 +249,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
   }
@@ -347,33 +349,33 @@ 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 filesAndTargets: [FileAndTarget], taskID: UUID, priority: TaskPriority?) async {
     let taskDescription = AnyIndexTaskDescription(
       UpdateIndexStoreTaskDescription(
-        filesToIndex: Set(files),
+        filesToIndex: filesAndTargets,
         buildSystemManager: self.buildSystemManager,
         index: index
       )
     )
     let updateIndexStoreTask = await self.indexTaskScheduler.schedule(priority: priority, taskDescription) { newState in
       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))
+        for fileAndTarget in filesAndTargets {
+          if case .scheduled((taskID, let task)) = self.indexStatus[fileAndTarget.file.sourceFile] {
+            self.indexStatus[fileAndTarget.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))
+        for fileAndTarget in filesAndTargets {
+          if case .executing((taskID, let task)) = self.indexStatus[fileAndTarget.file.sourceFile] {
+            self.indexStatus[fileAndTarget.file.sourceFile] = .scheduled((taskID, task))
           }
         }
       case .finished:
-        for file in files {
-          switch self.indexStatus[file.uri] {
+        for fileAndTarget in filesAndTargets {
+          switch self.indexStatus[fileAndTarget.file.sourceFile] {
           case .executing((taskID, _)):
-            self.indexStatus[file.uri] = .upToDate
+            self.indexStatus[fileAndTarget.file.sourceFile] = .upToDate
           default:
             break
           }
@@ -392,22 +394,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.uri) else {
-        logger.error("Not indexing \(file.uri.forLogging) because the target could not be determined")
+    for fileToIndex in outOfDateFiles {
+      guard let target = await buildSystemManager.canonicalConfiguredTarget(for: fileToIndex.mainFile) else {
+        logger.error(
+          "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] =
@@ -447,7 +452,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
+                )
               }
             }
           }
@@ -462,7 +471,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)
     }