Merge pull request #2271 from ahoppen/copied-files

Add support for copied header files to SourceKit-LSP

Author: ahoppen

Contributor Documentation/BSP Extensions.md

@@ -95,6 +95,15 @@ export interface SourceKitSourceItemData {
    * `outputPathsProvider: true` in `SourceKitInitializeBuildResponseData`.
    */
   outputPath?: string;
+
+  /**
+   * If this source item gets copied to a different destination during preparation, the destinations it will be copied
+   * to.
+   *
+   * If a user action would jump to one of these copied files, this allows SourceKit-LSP to redirect the navigation to
+   * the original source file instead of jumping to a file in the build directory.
+   */
+  copyDestinations?: DocumentURI[];
 }
 ```
 

Contributor Documentation/LSP Extensions.md

@@ -795,12 +795,20 @@ export interface SynchronizeParams {
    * This option is experimental, guarded behind the `synchronize-for-build-system-updates` experimental feature, and
    * may be modified or removed in future versions of SourceKit-LSP without notice. Do not rely on it.
    */
-  buildServerUpdates?: bool
+  buildServerUpdates?: bool;
+
+  /**
+   *  Wait for the build server to update its internal mapping of copied files to their original location.
+   *
+   * This option is experimental, guarded behind the `synchronize-copy-file-map` experimental feature, and may be
+   * modified or removed in future versions of SourceKit-LSP without notice. Do not rely on it.
+   */
+  copyFileMap?: bool;
 
   /**
    * Wait for background indexing to finish and all index unit files to be loaded into indexstore-db.
    */
-  index?: bool
+  index?: bool;
 }
 ```
 

Sources/BuildServerIntegration/BuildServerManager.swift

@@ -75,6 +75,10 @@ package struct SourceFileInfo: Sendable {
   ///    compiler arguments for these files to provide semantic editor functionality but we can't build them.
   package var isBuildable: Bool
 
+  /// If this source item gets copied to a different destination during preparation, the destinations it will be copied
+  /// to.
+  package var copyDestinations: Set<DocumentURI>
+
   fileprivate func merging(_ other: SourceFileInfo?) -> SourceFileInfo {
     guard let other else {
       return self
@@ -99,7 +103,8 @@ package struct SourceFileInfo: Sendable {
       targetsToOutputPath: mergedTargetsToOutputPaths,
       isPartOfRootProject: other.isPartOfRootProject || isPartOfRootProject,
       mayContainTests: other.mayContainTests || mayContainTests,
-      isBuildable: other.isBuildable || isBuildable
+      isBuildable: other.isBuildable || isBuildable,
+      copyDestinations: copyDestinations.union(other.copyDestinations)
     )
   }
 }
@@ -436,6 +441,18 @@ package actor BuildServerManager: QueueBasedMessageHandler {
 
   private let cachedSourceFilesAndDirectories = Cache<SourceFilesAndDirectoriesKey, SourceFilesAndDirectories>()
 
+  /// The latest map of copied file URIs to their original source locations.
+  ///
+  /// We don't use a `Cache` for this because we can provide reasonable functionality even without or with an
+  /// out-of-date copied file map - in the worst case we jump to a file in the build directory instead of the source
+  /// directory.
+  /// We don't want to block requests like definition on receiving up-to-date index information from the build server.
+  private var cachedCopiedFileMap: [DocumentURI: DocumentURI] = [:]
+
+  /// The latest task to update the `cachedCopiedFileMap`. This allows us to cancel previous tasks to update the copied
+  /// file map when a new update is requested.
+  private var copiedFileMapUpdateTask: Task<Void, Never>?
+
   /// The `SourceKitInitializeBuildResponseData` received from the `build/initialize` request, if any.
   package var initializationData: SourceKitInitializeBuildResponseData? {
     get async {
@@ -675,6 +692,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
       return !updatedTargets.intersection(cacheKey.targets).isEmpty
     }
     self.cachedSourceFilesAndDirectories.clearAll(isolation: self)
+    self.scheduleRecomputeCopyFileMap()
 
     await delegate?.buildTargetsChanged(notification.changes)
     await filesBuildSettingsChangedDebouncer.scheduleCall(Set(watchedFiles.keys))
@@ -854,6 +872,43 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     }
   }
 
+  /// Check if the URI referenced by `location` has been copied during the preparation phase. If so, adjust the URI to
+  /// the original source file.
+  package func locationAdjustedForCopiedFiles(_ location: Location) -> Location {
+    guard let originalUri = cachedCopiedFileMap[location.uri] else {
+      return location
+    }
+    // If we regularly get issues that the copied file is out-of-sync with its original, we can check that the contents
+    // of the lines touched by the location match and only return the original URI if they do. For now, we avoid this
+    // check due to its performance cost of reading files from disk.
+    return Location(uri: originalUri, range: location.range)
+  }
+
+  /// Check if the URI referenced by `location` has been copied during the preparation phase. If so, adjust the URI to
+  /// the original source file.
+  package func locationsAdjustedForCopiedFiles(_ locations: [Location]) -> [Location] {
+    return locations.map { locationAdjustedForCopiedFiles($0) }
+  }
+
+  @discardableResult
+  package func scheduleRecomputeCopyFileMap() -> Task<Void, Never> {
+    let task = Task { [previousUpdateTask = copiedFileMapUpdateTask] in
+      previousUpdateTask?.cancel()
+      await orLog("Re-computing copy file map") {
+        let sourceFilesAndDirectories = try await self.sourceFilesAndDirectories()
+        var copiedFileMap: [DocumentURI: DocumentURI] = [:]
+        for (file, fileInfo) in sourceFilesAndDirectories.files {
+          for copyDestination in fileInfo.copyDestinations {
+            copiedFileMap[copyDestination] = file
+          }
+        }
+        self.cachedCopiedFileMap = copiedFileMap
+      }
+    }
+    copiedFileMapUpdateTask = task
+    return task
+  }
+
   /// Returns all the targets that the document is part of.
   package func targets(for document: DocumentURI) async -> [BuildTargetIdentifier] {
     guard let targets = await sourceFileInfo(for: document)?.targets else {
@@ -1307,7 +1362,8 @@ package actor BuildServerManager: QueueBasedMessageHandler {
             isPartOfRootProject: isPartOfRootProject,
             mayContainTests: mayContainTests,
             isBuildable: !(target?.tags.contains(.notBuildable) ?? false)
-              && (sourceKitData?.kind ?? .source) == .source
+              && (sourceKitData?.kind ?? .source) == .source,
+            copyDestinations: Set(sourceKitData?.copyDestinations ?? [])
           )
           switch sourceItem.kind {
           case .file:

Sources/BuildServerProtocol/Messages/BuildTargetSourcesRequest.swift

@@ -157,10 +157,23 @@ public struct SourceKitSourceItemData: LSPAnyCodable, Codable {
   /// `outputPathsProvider: true` in `SourceKitInitializeBuildResponseData`.
   public var outputPath: String?
 
-  public init(language: Language? = nil, kind: SourceKitSourceItemKind? = nil, outputPath: String? = nil) {
+  /// If this source item gets copied to a different destination during preparation, the destinations it will be copied
+  /// to.
+  ///
+  /// If a user action would jump to one of these copied files, this allows SourceKit-LSP to redirect the navigation to
+  /// the original source file instead of jumping to a file in the build directory.
+  public var copyDestinations: [DocumentURI]?
+
+  public init(
+    language: Language? = nil,
+    kind: SourceKitSourceItemKind? = nil,
+    outputPath: String? = nil,
+    copyDestinations: [DocumentURI]? = nil
+  ) {
     self.language = language
     self.kind = kind
     self.outputPath = outputPath
+    self.copyDestinations = copyDestinations
   }
 
   public init?(fromLSPDictionary dictionary: [String: LanguageServerProtocol.LSPAny]) {
@@ -177,6 +190,14 @@ public struct SourceKitSourceItemData: LSPAnyCodable, Codable {
     if case .string(let outputFilePath) = dictionary[CodingKeys.outputPath.stringValue] {
       self.outputPath = outputFilePath
     }
+    if case .array(let copyDestinations) = dictionary[CodingKeys.copyDestinations.stringValue] {
+      self.copyDestinations = copyDestinations.compactMap { entry in
+        guard case .string(let copyDestination) = entry else {
+          return nil
+        }
+        return try? DocumentURI(string: copyDestination)
+      }
+    }
   }
 
   public func encodeToLSPAny() -> LanguageServerProtocol.LSPAny {
@@ -190,6 +211,9 @@ public struct SourceKitSourceItemData: LSPAnyCodable, Codable {
     if let outputPath {
       result[CodingKeys.outputPath.stringValue] = .string(outputPath)
     }
+    if let copyDestinations {
+      result[CodingKeys.copyDestinations.stringValue] = .array(copyDestinations.map { .string($0.stringValue) })
+    }
     return .dictionary(result)
   }
 }

Sources/LanguageServerProtocol/Requests/SynchronizeRequest.swift

@@ -38,11 +38,18 @@ public struct SynchronizeRequest: RequestType {
   /// may be modified or removed in future versions of SourceKit-LSP without notice. Do not rely on it.
   public var buildServerUpdates: Bool?
 
+  /// Wait for the build server to update its internal mapping of copied files to their original location.
+  ///
+  /// This option is experimental, guarded behind the `synchronize-copy-file-map` experimental feature, and may be
+  /// modified or removed in future versions of SourceKit-LSP without notice. Do not rely on it.
+  public var copyFileMap: Bool?
+
   /// Wait for background indexing to finish and all index unit files to be loaded into indexstore-db.
   public var index: Bool?
 
-  public init(buildServerUpdates: Bool? = nil, index: Bool? = nil) {
+  public init(buildServerUpdates: Bool? = nil, copyFileMap: Bool? = nil, index: Bool? = nil) {
     self.buildServerUpdates = buildServerUpdates
+    self.copyFileMap = copyFileMap
     self.index = index
   }
 }

Sources/SKOptions/ExperimentalFeatures.swift

@@ -45,6 +45,11 @@ public enum ExperimentalFeature: String, Codable, Sendable, CaseIterable {
   /// - Note: Internal option, for testing only
   case synchronizeForBuildSystemUpdates = "synchronize-for-build-system-updates"
 
+  /// Enable the `copyFileMap` option in the `workspace/synchronize` request.
+  ///
+  /// - Note: Internal option, for testing only
+  case synchronizeCopyFileMap = "synchronize-copy-file-map"
+
   /// All non-internal experimental features.
   public static var allNonInternalCases: [ExperimentalFeature] {
     allCases.filter { !$0.isInternal }
@@ -67,6 +72,8 @@ public enum ExperimentalFeature: String, Codable, Sendable, CaseIterable {
       return true
     case .synchronizeForBuildSystemUpdates:
       return true
+    case .synchronizeCopyFileMap:
+      return true
     }
   }
 }

Sources/SKTestSupport/TestSourceKitLSPClient.swift

@@ -27,7 +27,7 @@ import XCTest
 extension SourceKitLSPOptions {
   package static func testDefault(
     backgroundIndexing: Bool = true,
-    experimentalFeatures: Set<ExperimentalFeature> = []
+    experimentalFeatures: Set<ExperimentalFeature> = [.synchronizeCopyFileMap]
   ) async throws -> SourceKitLSPOptions {
     let pluginPaths = try await sourceKitPluginPaths
     return SourceKitLSPOptions(

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -2128,7 +2128,8 @@ extension SourceKitLSPServer {
         return try await languageService.definition(req)
       }
     }
-    return .locations(indexBasedResponse)
+    let remappedLocations = await workspace.buildServerManager.locationsAdjustedForCopiedFiles(indexBasedResponse)
+    return .locations(remappedLocations)
   }
 
   /// Generate the generated interface for the given module, write it to disk and return the location to which to jump
@@ -2643,6 +2644,9 @@ extension SourceKitLSPServer {
     if req.buildServerUpdates != nil, !self.options.hasExperimentalFeature(.synchronizeForBuildSystemUpdates) {
       throw ResponseError.unknown("\(SynchronizeRequest.method).buildServerUpdates is an experimental request option")
     }
+    if req.copyFileMap != nil, !self.options.hasExperimentalFeature(.synchronizeCopyFileMap) {
+      throw ResponseError.unknown("\(SynchronizeRequest.method).copyFileMap is an experimental request option")
+    }
     for workspace in workspaces {
       await workspace.synchronize(req)
     }

Sources/SourceKitLSP/Workspace.swift

@@ -513,6 +513,18 @@ package final class Workspace: Sendable, BuildServerManagerDelegate {
       await buildServerManager.waitForUpToDateBuildGraph()
       await indexUnitOutputPathsUpdateQueue.async {}.value
     }
+    if request.copyFileMap ?? false {
+      // Not using `valuePropagatingCancellation` here because that could lead us to the following scenario:
+      //  - An update of the copy file map is scheduled because of a change in the build graph
+      //  - We get a synchronize request
+      //  - Scheduling a new recomputation of the copy file map cancels the previous recomputation
+      //  - We cancel the synchronize request, which would also cancel the copy file map recomputation, leaving us with
+      //    an outdated version
+      //
+      // Technically, we might be doing unnecessary work here if the output file map is already up-to-date. But since
+      // this option is mostly intended for testing purposes, this is acceptable.
+      await buildServerManager.scheduleRecomputeCopyFileMap().value
+    }
     if request.index ?? false {
       await semanticIndexManager?.waitForUpToDateIndex()
       uncheckedIndex?.pollForUnitChangesAndWait()

Tests/SourceKitLSPTests/DefinitionTests.swift

@@ -10,6 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+import BuildServerIntegration
+import BuildServerProtocol
 import LanguageServerProtocol
 @_spi(Testing) import SKLogging
 import SKTestSupport
@@ -587,4 +589,104 @@ class DefinitionTests: XCTestCase {
     )
     XCTAssertEqual(response?.locations, [Location(uri: project.fileURI, range: Range(project.positions["2️⃣"]))])
   }
+
+  func testJumpToCopiedHeader() async throws {
+    actor BuildServer: CustomBuildServer {
+      let inProgressRequestsTracker = CustomBuildServerInProgressRequestTracker()
+      private let projectRoot: URL
+
+      private var headerCopyDestination: URL {
+        projectRoot.appending(components: "header-copy", "CopiedTest.h")
+      }
+
+      init(projectRoot: URL, connectionToSourceKitLSP: any Connection) {
+        self.projectRoot = projectRoot
+      }
+
+      func initializeBuildRequest(_ request: InitializeBuildRequest) async throws -> InitializeBuildResponse {
+        return try initializationResponseSupportingBackgroundIndexing(
+          projectRoot: projectRoot,
+          outputPathsProvider: false
+        )
+      }
+
+      func buildTargetSourcesRequest(_ request: BuildTargetSourcesRequest) -> BuildTargetSourcesResponse {
+        return BuildTargetSourcesResponse(items: [
+          SourcesItem(
+            target: .dummy,
+            sources: [
+              SourceItem(
+                uri: DocumentURI(projectRoot.appendingPathComponent("Test.c")),
+                kind: .file,
+                generated: false,
+                dataKind: .sourceKit,
+                data: SourceKitSourceItemData(
+                  language: .c,
+                  kind: .source,
+                  outputPath: nil,
+                  copyDestinations: nil
+                ).encodeToLSPAny()
+              ),
+              SourceItem(
+                uri: DocumentURI(projectRoot.appendingPathComponent("Test.h")),
+                kind: .file,
+                generated: false,
+                dataKind: .sourceKit,
+                data: SourceKitSourceItemData(
+                  language: .c,
+                  kind: .header,
+                  outputPath: nil,
+                  copyDestinations: [DocumentURI(headerCopyDestination)]
+                ).encodeToLSPAny()
+              ),
+            ]
+          )
+        ])
+      }
+
+      func textDocumentSourceKitOptionsRequest(
+        _ request: TextDocumentSourceKitOptionsRequest
+      ) throws -> TextDocumentSourceKitOptionsResponse? {
+        return TextDocumentSourceKitOptionsResponse(compilerArguments: [
+          request.textDocument.uri.pseudoPath, "-I", try headerCopyDestination.deletingLastPathComponent().filePath,
+        ])
+      }
+
+      func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> VoidResponse {
+        try FileManager.default.createDirectory(
+          at: headerCopyDestination.deletingLastPathComponent(),
+          withIntermediateDirectories: true
+        )
+        try FileManager.default.copyItem(
+          at: projectRoot.appending(component: "Test.h"),
+          to: headerCopyDestination
+        )
+        return VoidResponse()
+      }
+    }
+
+    let project = try await CustomBuildServerTestProject(
+      files: [
+        "Test.h": """
+        void hello();
+        """,
+        "Test.c": """
+        #include <CopiedTest.h>
+
+        void test() {
+          1️⃣hello();
+        }
+        """,
+      ],
+      buildServer: BuildServer.self,
+      enableBackgroundIndexing: true,
+    )
+    try await project.testClient.send(SynchronizeRequest(copyFileMap: true))
+
+    let (uri, positions) = try project.openDocument("Test.c")
+    let response = try await project.testClient.send(
+      DefinitionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
+    )
+    XCTAssertEqual(response?.locations?.map(\.uri), [try project.uri(for: "Test.h")])
+  }
 }