From 882c990caea25b0e17f2e447e4027c7cc659e086 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 30 Aug 2025 23:35:05 +0200 Subject: [PATCH] Add support for copied header files to SourceKit-LSP If a build server copies files (eg. header) to the build directory during preparation and those copied files are referenced for semantic functionality, we would currently jump to the file in the build directory. Teach SourceKit-LSP about files that are copied during preparation and if we detect that we are jumping to such a file, jump to the original file instead. So far only the definition request checks the copied file paths. Adding support for copied file paths in the other requests will be a follow-up change. --- Contributor Documentation/BSP Extensions.md | 9 ++ Contributor Documentation/LSP Extensions.md | 12 ++- .../BuildServerManager.swift | 60 ++++++++++- .../Messages/BuildTargetSourcesRequest.swift | 26 ++++- .../Requests/SynchronizeRequest.swift | 9 +- Sources/SKOptions/ExperimentalFeatures.swift | 7 ++ .../TestSourceKitLSPClient.swift | 2 +- Sources/SourceKitLSP/SourceKitLSPServer.swift | 6 +- Sources/SourceKitLSP/Workspace.swift | 12 +++ Tests/SourceKitLSPTests/DefinitionTests.swift | 102 ++++++++++++++++++ 10 files changed, 237 insertions(+), 8 deletions(-) diff --git a/Contributor Documentation/BSP Extensions.md b/Contributor Documentation/BSP Extensions.md index cb28c6f3d..bb3cd71e2 100644 --- a/Contributor Documentation/BSP Extensions.md +++ b/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[]; } ``` diff --git a/Contributor Documentation/LSP Extensions.md b/Contributor Documentation/LSP Extensions.md index f8d2e5b2c..0ecfb52cc 100644 --- a/Contributor Documentation/LSP Extensions.md +++ b/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; } ``` diff --git a/Sources/BuildServerIntegration/BuildServerManager.swift b/Sources/BuildServerIntegration/BuildServerManager.swift index 0f648180e..3c671b41d 100644 --- a/Sources/BuildServerIntegration/BuildServerManager.swift +++ b/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 + 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) ) } } @@ -434,6 +439,18 @@ package actor BuildServerManager: QueueBasedMessageHandler { private let cachedSourceFilesAndDirectories = Cache() + /// 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? + /// The `SourceKitInitializeBuildResponseData` received from the `build/initialize` request, if any. package var initializationData: SourceKitInitializeBuildResponseData? { get async { @@ -660,6 +677,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)) @@ -839,6 +857,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 { + 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 { @@ -1292,7 +1347,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: diff --git a/Sources/BuildServerProtocol/Messages/BuildTargetSourcesRequest.swift b/Sources/BuildServerProtocol/Messages/BuildTargetSourcesRequest.swift index 6e952a5c7..ec3722b9f 100644 --- a/Sources/BuildServerProtocol/Messages/BuildTargetSourcesRequest.swift +++ b/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) } } diff --git a/Sources/LanguageServerProtocol/Requests/SynchronizeRequest.swift b/Sources/LanguageServerProtocol/Requests/SynchronizeRequest.swift index 4d25ac467..33ba049bb 100644 --- a/Sources/LanguageServerProtocol/Requests/SynchronizeRequest.swift +++ b/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 } } diff --git a/Sources/SKOptions/ExperimentalFeatures.swift b/Sources/SKOptions/ExperimentalFeatures.swift index 8eae864bc..455f5b516 100644 --- a/Sources/SKOptions/ExperimentalFeatures.swift +++ b/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 } } } diff --git a/Sources/SKTestSupport/TestSourceKitLSPClient.swift b/Sources/SKTestSupport/TestSourceKitLSPClient.swift index d8466f73b..0f6434697 100644 --- a/Sources/SKTestSupport/TestSourceKitLSPClient.swift +++ b/Sources/SKTestSupport/TestSourceKitLSPClient.swift @@ -27,7 +27,7 @@ import XCTest extension SourceKitLSPOptions { package static func testDefault( backgroundIndexing: Bool = true, - experimentalFeatures: Set = [] + experimentalFeatures: Set = [.synchronizeCopyFileMap] ) async throws -> SourceKitLSPOptions { let pluginPaths = try await sourceKitPluginPaths return SourceKitLSPOptions( diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 58b5eb835..491ebed67 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/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 @@ -2632,6 +2633,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) } diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index a6a9c67ac..0b0e1bf08 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/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() diff --git a/Tests/SourceKitLSPTests/DefinitionTests.swift b/Tests/SourceKitLSPTests/DefinitionTests.swift index 78ec580f4..f87213230 100644 --- a/Tests/SourceKitLSPTests/DefinitionTests.swift +++ b/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 + + 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")]) + } }