Extend copied file mapping to all LSP requests returning locations #2385
Conversation
loveucifer
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
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.
loveucifer
force-pushed
the
feature/file-mapping-all-requests
branch
from
4fba69c to
70d900e
Compare
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Very cool 🤩 Excited to see this! I left a few comments inline.
| return locations.map { locationAdjustedForCopiedFiles($0) } | ||
| } | ||
|
|
||
| package func workspaceEditAdjustedForCopiedFiles(_ workspaceEdit: WorkspaceEdit?) async -> WorkspaceEdit? { |
There was a problem hiding this comment.
I don’t think any of these functions need to be async.
| var newChanges: [DocumentURI: [TextEdit]] = [:] | ||
| for (uri, edits) in changes { | ||
| let newUri = self.locationAdjustedForCopiedFiles( | ||
| Location(uri: uri, range: Position(line: 0, utf16index: 0)..<Position(line: 0, utf16index: 0)) |
There was a problem hiding this comment.
Instead of synthesizing a Location with an irrelevant position, I think we should just have a function that adjusts a DocumentURI for copied files.
| let newUri = self.locationAdjustedForCopiedFiles( | ||
| Location(uri: uri, range: Position(line: 0, utf16index: 0)..<Position(line: 0, utf16index: 0)) | ||
| ).uri | ||
| newChanges[newUri, default: []].append(contentsOf: edits) |
There was a problem hiding this comment.
Just personal preference 😉
| newChanges[newUri, default: []].append(contentsOf: edits) | |
| newChanges[newUri, default: []] += edits |
| remappedLinks.append(LocationLink( | ||
| originSelectionRange: link.originSelectionRange, | ||
| targetUri: newUri, | ||
| targetRange: link.targetRange, |
There was a problem hiding this comment.
I think we should use the target range of the adjusted location here. Just in case the location adjustment in the future also modifies the range. Same for the selection range.
| detail: item.detail, | ||
| uri: adjustedLocation.uri, | ||
| range: adjustedLocation.range, | ||
| selectionRange: item.selectionRange, |
There was a problem hiding this comment.
It might seem redundant now but I’d like to run the selection range through locationAdjustedForCopiedFiles as well, just so we have a centralized place that might also adjust the range eg. if it discovers that copying the file removed the first line and thus all line number need to be shifted by 1 or something like that (not proposing that we do this now but it might be something we could consider doing in the future).
| } else { | ||
| name = "\(symbol.name): \(conformances.map(\.symbol.name).sorted().joined(separator: ", "))" | ||
| } | ||
| switch symbol.kind { |
There was a problem hiding this comment.
This looks like incorrect indentation to me. Could you run swift-format on your change?
| // 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" | ||
| } |
There was a problem hiding this comment.
Did you unintentionally duplicate this code?
| detail = info.location.moduleName | ||
| } | ||
|
|
||
| let item = TypeHierarchyItem( |
There was a problem hiding this comment.
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.
| response.sort() | ||
| var expected = [ | ||
| try project.location(from: "1️⃣", to: "1️⃣", in: "Test.h"), | ||
| try project.location(from: "2️⃣", to: "2️⃣", in: "Test.c"), | ||
| ] | ||
| expected.sort() | ||
| XCTAssertEqual(response, expected) |
There was a problem hiding this comment.
Ordering should be deterministic, so I don’t think that .sort() should be needed.
| XCTAssertEqual(response, expected) | ||
| } | ||
|
|
||
| func DISABLED_testFindImplementationInCopiedHeader() async throws { |
There was a problem hiding this comment.
Why is this disabled? Artifact of local development?
oh cool I will try to fix , this was my first time contributing to swift :) |
loveucifer
force-pushed
the
feature/file-mapping-all-requests
branch
from
c4f0eb2 to
70d900e
Compare
- 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
loveucifer
force-pushed
the
feature/file-mapping-all-requests
branch
from
df65d94 to
cae01c6
Compare
- 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
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
hey @ahoppen can i get another review :) |
2 participants
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:
This provides consistent navigation behavior, ensuring users are always taken to the original source files instead of build artifacts when possible.