Extend copied file mapping to all LSP requests returning locations #2385

loveucifer · 2025-12-09T07:17:32Z

This addresses issue #2276 by ensuring that all LSP requests that return source file locations map copied header files back to their original locations, not just jump-to-definition.

Previously, only the definition request applied this mapping. Now, the following requests also adjust locations for copied files:

textDocument/references
textDocument/implementation
workspace/symbol
callHierarchy/prepare
callHierarchy/incomingCalls
callHierarchy/outgoingCalls
typeHierarchy/prepare
typeHierarchy/supertypes
typeHierarchy/subtypes

This provides consistent navigation behavior, ensuring users are always taken to the original source files instead of build artifacts when possible.

This addresses issue swiftlang#2276 by ensuring that all LSP requests that return source file locations map copied header files back to their original locations, not just jump-to-definition. Previously, only the definition request applied this mapping. Now, the following requests also adjust locations for copied files: - textDocument/references - textDocument/implementation - workspace/symbol - callHierarchy/prepare - callHierarchy/incomingCalls - callHierarchy/outgoingCalls - typeHierarchy/prepare - typeHierarchy/supertypes - typeHierarchy/subtypes This provides consistent navigation behavior, ensuring users are always taken to the original source files instead of build artifacts when possible.

ahoppen

Very cool 🤩 Excited to see this! I left a few comments inline.

Sources/BuildServerIntegration/BuildServerManager.swift

ahoppen · 2025-12-10T08:55:32Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

-      } else {
-        name = "\(symbol.name): \(conformances.map(\.symbol.name).sorted().joined(separator: ", "))"
-      }
+      switch symbol.kind {


This looks like incorrect indentation to me. Could you run swift-format on your change?

ahoppen · 2025-12-10T08:56:30Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

+        // Add the file name and line to the detail string
+        if let url = remappedLocation.uri.fileURL,
+          let basename = (try? AbsolutePath(validating: url.filePath))?.basename
+        {
+          detail = "Extension at \(basename):\(remappedLocation.range.lowerBound.line + 1)"
+        } else if !definition.location.moduleName.isEmpty {
+          detail = "Extension in \(definition.location.moduleName)"
+        } else {
+          detail = "Extension"
+        }


Did you unintentionally duplicate this code?

ahoppen · 2025-12-10T08:57:30Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

+        detail = info.location.moduleName
+      }
+
+       let item = TypeHierarchyItem(


Shouldn’t we be able to just all into the adjust method for TypeHierarchyItem here and not have these 40-ish lines of adjustment code? Same for the other TypeHierarchyItem returning functions.

Tests/SourceKitLSPTests/CopiedHeaderTests.swift

loveucifer · 2025-12-10T10:05:40Z

Very cool 🤩 Excited to see this! I left a few comments inline.

oh cool I will try to fix , this was my first time contributing to swift :)

- Remove async from workspaceEditAdjustedForCopiedFiles - Refactor to use uriAdjustedForCopiedFiles helper - Update dictionary update logic with += - Adjust LocationLink creation to use adjusted ranges - Ensure selectionRange adjustment in prepareCallHierarchy - Provide default WorkspaceEdit in ClangLanguageService - Revert asyncMap to map and remove await in SourceKitLSPServer - Chain workspace and index retrieval in incomingCalls - Use indexToLSPCallHierarchyItem and shared helper for CallHierarchyItem - Fix indentation and remove duplicated detail setting - Use shared helper for TypeHierarchyItem - Remove .sort() from expected array in tests - Enable testFindImplementationInCopiedHeader - Add await for actor-isolated BuildServerManager calls

- Refactor supertypes/subtypes to use indexToLSPTypeHierarchyItem helper instead of duplicating ~80 lines of TypeHierarchyItem creation code - Remove unused workaround helper functions (indexToLSPLocation2, indexToLSPTypeHierarchyItem2) - Fix test ordering: use deterministic sorted order instead of Set comparison - Enable testFindImplementationInCopiedHeader test - Add implementation request support for C/C++/ObjC functions with separate declaration and definition (finds definition when declarations exist without definitions at the same location) - Fix whitespace/indentation issues

Copilot

Pull request overview

This PR extends the remapping of copied header files to their original source locations across all LSP requests that return location information. Previously, only the textDocument/definition request applied this mapping; now all location-returning requests (references, implementation, workspace symbols, call hierarchy, and type hierarchy) consistently navigate users to original source files instead of build artifacts.

Key Changes:

Added location remapping to references, implementation, and workspace symbol requests
Implemented location remapping in call hierarchy (prepare, incoming, outgoing) operations
Implemented location remapping in type hierarchy (prepare, supertypes, subtypes) operations

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
Tests/SourceKitLSPTests/CopiedHeaderTests.swift	New test file covering references, implementation, declaration, and workspace symbols in copied headers
Sources/SourceKitLSP/SourceKitLSPServer.swift	Extended location remapping to all LSP location-returning requests and refactored call/type hierarchy handling
Sources/ClangLanguageService/ClangLanguageService.swift	Added location remapping for declaration and rename requests
Sources/BuildServerIntegration/BuildServerManager.swift	Added helper methods for remapping locations in workspace edits, location links, and type hierarchy items

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sources/SourceKitLSP/SourceKitLSPServer.swift

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

loveucifer · 2025-12-16T02:47:03Z

hey @ahoppen can i get another review :)

ahoppen

Thanks, I left two more comments inline but I think a few of my last comments are still outstanding (see the ones that I didn’t mark as resolved). In particular, I don’t think that SourceKitLSPServer.swift should contain any large-scale edits for this change, it should just call to adjust methods in BuildServerManager to take copied files into account.

ahoppen · 2025-12-16T15:52:01Z

Sources/BuildServerIntegration/BuildServerManager.swift

+    return edit
+  }
+
+  package func locationsOrLocationLinksAdjustedForCopiedFiles(_ response: LocationsOrLocationLinksResponse?) -> LocationsOrLocationLinksResponse? {


Could your run swift-format on your changes? https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#formatting

ahoppen · 2025-12-16T15:54:22Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

    )
  }

+  private func callHierarchyItemAdjustedForCopiedFiles(


Shouldn’t this be a method on BuildServerManager like the other methods that adjust for copied files?

loveucifer · 2025-12-16T16:00:51Z

Thanks, I left two more comments inline but I think a few of my last comments are still outstanding (see the ones that I didn’t mark as resolved). In particular, I don’t think that SourceKitLSPServer.swift should contain any large-scale edits for this change, it should just call to adjust methods in BuildServerManager to take copied files into account.

got it thanks !

ahoppen

Thanks for the update. The PR looks pretty good now, I left a few more stylistic-ish comments inline.

ahoppen · 2025-12-18T11:21:42Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

+        )
+        return Location(uri: symbolOccurrence.location.documentUri, range: Range(symbolPosition))
+      }
+      let remappedLocations = await workspace.buildServerManager.locationsAdjustedForCopiedFiles(locations)


Do we need to store the mapped location here? Wouldn’t it be easier if we just call await workspace.buildServerManager.locationsAdjustedForCopiedFiles inside symbolsAndIndex.sorted(by: { $0.symbol < $1.symbol }).map where we currently have let symbolLocation = Location?

ahoppen · 2025-12-18T11:22:29Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

+      // for C/C++/objC functions with separate declaration and definition,
+      // "implementation" means the definition. Only use this fallback if there's
+      // a declaration without a definition at the same location.
+      if occurrences.isEmpty {
+        let declarations = index.occurrences(ofUSR: usr, roles: .declaration)
+        let definitions = index.occurrences(ofUSR: usr, roles: .definition)
+        // check if there are declarations that don't have a definition at the same location
+        let hasDeclarationOnlyLocations = declarations.contains { decl in
+          !definitions.contains { def in
+            def.location.path == decl.location.path && def.location.line == decl.location.line
+          }
+        }
+        if hasDeclarationOnlyLocations {
+          occurrences = definitions
+        }
+      }


Is this change unrelated to the copied file mapping? If so, could you open a separate PR for it?

I think this is needed because the testFindImplementationInCopiedHeader - without it the test fails because C functions need this fallback to find the definition from a declaration , the tests kept failing without it i think the error was something like "Go to Implementation" on a C function returning empty because C functions don't have the inheritance relationships i might be wrong but now that i tried without using this the tests failed again : /

ahoppen · 2025-12-18T11:24:19Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

+    var callHierarchyItems: [CallHierarchyItem] = []
+    for usr in usrs {


Is there any particular reason why you switched from the compactMap based implementation to for … in? I’d prefer if we just sticked with the old compactMap-based approach. It it was to allow await calls in the body, we have the asyncCompactMap helper for that.

Same for the other locations where you switched from compactMap to for … in

The original change to for...in was because we needed await for the location remappingand stuff . I didn't quite think about asyncCompactMap at the time idk might've missed it my bad. I've now switched them all back to use asyncCompactMap changing all of them soon :D

Sources/SourceKitLSP/SourceKitLSPServer.swift

ahoppen · 2025-12-18T11:27:25Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

      {
        detail = "Extension at \(basename):\(location.range.lowerBound.line + 1)"
-      } else if let moduleName = moduleName {
+      } else if let moduleName = moduleName, !moduleName.isEmpty {


Is this change unrelated to the PR? Could you open a separate PR for it, so we can give this change its own dedicated discussion?

ahoppen · 2025-12-18T11:32:48Z

Sources/SourceKitLSP/SourceKitLSPServer.swift

-    // TODO: Remove this workaround once https://github.com/swiftlang/swift/issues/75600 is fixed
-    func indexToLSPLocation2(_ location: SymbolLocation) -> Location? {
-      return self.indexToLSPLocation(location)
-    }
-
-    // TODO: Remove this workaround once https://github.com/swiftlang/swift/issues/75600 is fixed
-    func indexToLSPTypeHierarchyItem2(
-      definition: SymbolOccurrence,
-      moduleName: String?,
-      index: CheckedIndex
-    ) -> TypeHierarchyItem? {
-      return self.indexToLSPTypeHierarchyItem(
-        definition: definition,
-        moduleName: moduleName,
-        index: index
-      )
-    }


It seems that these workarounds might indeed no longer needed when using Swift 6.3 snapshots but the issue does still persist in Swift 6.2. Since SourceKit-LSP still needs to compile using the released Swift 6.2 compiler, we still need them.

Even if the changes were no longer needed, I’d prefer to remove the workarounds in a separate PR.

loveucifer requested review from ahoppen, bnbarham, hamishknight and rintaro as code owners December 9, 2025 07:17

loveucifer marked this pull request as draft December 9, 2025 07:19

loveucifer force-pushed the feature/file-mapping-all-requests branch from 4fba69c to 70d900e Compare December 9, 2025 07:22

loveucifer marked this pull request as ready for review December 9, 2025 07:33

ahoppen reviewed Dec 10, 2025

View reviewed changes

loveucifer force-pushed the feature/file-mapping-all-requests branch from c4f0eb2 to 70d900e Compare December 10, 2025 16:32

loveucifer force-pushed the feature/file-mapping-all-requests branch from df65d94 to cae01c6 Compare December 10, 2025 18:17

Copilot AI review requested due to automatic review settings December 16, 2025 02:41

Copilot AI reviewed Dec 16, 2025

View reviewed changes

Sources/SourceKitLSP/SourceKitLSPServer.swift Outdated Show resolved Hide resolved

Sources/SourceKitLSP/SourceKitLSPServer.swift Outdated Show resolved Hide resolved

loveucifer and others added 2 commits December 16, 2025 08:15

Update Sources/SourceKitLSP/SourceKitLSPServer.swift

71374c1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update Sources/SourceKitLSP/SourceKitLSPServer.swift

d254ce1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ahoppen reviewed Dec 16, 2025

View reviewed changes

some more fixes

21adc24

ahoppen reviewed Dec 18, 2025

View reviewed changes

more and more fixes

f3237fa

		var callHierarchyItems: [CallHierarchyItem] = []
		for usr in usrs {

Extend copied file mapping to all LSP requests returning locations #2385

Are you sure you want to change the base?

Extend copied file mapping to all LSP requests returning locations #2385

Conversation

loveucifer commented Dec 9, 2025

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

loveucifer commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

loveucifer commented Dec 16, 2025

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loveucifer commented Dec 16, 2025

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants