-
Notifications
You must be signed in to change notification settings - Fork 373
add go-to-definition for inlay hints (#2318) #2436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
8e300ee
f46a31b
27cf04e
564a45b
3630ab9
9e12af1
81610b3
22e7ac0
0695d3c
ceaf87b
0b02672
45b1598
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -832,6 +832,8 @@ extension SourceKitLSPServer: QueueBasedMessageHandler { | |
| initialized = true | ||
| case let request as RequestAndReply<InlayHintRequest>: | ||
| await self.handleRequest(for: request, requestHandler: self.inlayHint) | ||
| case let request as RequestAndReply<InlayHintResolveRequest>: | ||
| await request.reply { try await inlayHintResolve(request: request.params) } | ||
| case let request as RequestAndReply<IsIndexingRequest>: | ||
| await request.reply { try await self.isIndexing(request.params) } | ||
| case let request as RequestAndReply<OutputPathsRequest>: | ||
|
|
@@ -1098,7 +1100,7 @@ extension SourceKitLSPServer { | |
| let inlayHintOptions = | ||
| await registry.clientHasDynamicInlayHintRegistration | ||
| ? nil | ||
| : ValueOrBool.value(InlayHintOptions(resolveProvider: false)) | ||
| : ValueOrBool.value(InlayHintOptions(resolveProvider: true)) | ||
|
|
||
| let semanticTokensOptions = | ||
| await registry.clientHasDynamicSemanticTokensRegistration | ||
|
|
@@ -1901,6 +1903,24 @@ extension SourceKitLSPServer { | |
| return try await languageService.inlayHint(req) | ||
| } | ||
|
|
||
| func inlayHintResolve( | ||
| request: InlayHintResolveRequest | ||
| ) async throws -> InlayHint { | ||
| // inlay hints store the uri in data for resolution | ||
| // extract uri from the lspany dictionary | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment on the comment here. This exactly explains what’s being done below, so I don’t think it provides much value. In general, I’m a fan of comments that describe why something is done, not what is being done. |
||
| guard case .dictionary(let dict) = request.inlayHint.data, | ||
| case .string(let uriString) = dict["uri"], | ||
| let uri = try? DocumentURI(string: uriString) | ||
| else { | ||
| return request.inlayHint | ||
| } | ||
| guard let workspace = await self.workspaceForDocument(uri: uri) else { | ||
| return request.inlayHint | ||
| } | ||
| let language = try documentManager.latestSnapshot(uri.buildSettingsFile).language | ||
| return try await primaryLanguageService(for: uri, language, in: workspace).inlayHintResolve(request) | ||
| } | ||
|
|
||
| func documentDiagnostic( | ||
| _ req: DocumentDiagnosticsRequest, | ||
| workspace: Workspace, | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add this file to CMakeLists.txt to make the tests pass on Windows? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors | ||
| // Licensed under Apache License v2.0 with Runtime Library Exception | ||
| // | ||
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| @_spi(SourceKitLSP) package import LanguageServerProtocol | ||
| import Foundation | ||
| import IndexStoreDB | ||
| import SemanticIndex | ||
| import SourceKitD | ||
| import SourceKitLSP | ||
|
|
||
| extension SwiftLanguageService { | ||
| /// resolves an inlay hint by looking up the type definition location | ||
| package func inlayHintResolve(_ req: InlayHintResolveRequest) async throws -> InlayHint { | ||
| var hint = req.inlayHint | ||
|
|
||
| // only resolve type hints that have stored data | ||
| // extract uri and position from the lspany dictionary | ||
| guard hint.kind == .type, | ||
| case .dictionary(let dict) = hint.data, | ||
| case .string(let uriString) = dict["uri"], | ||
| let uri = try? DocumentURI(string: uriString), | ||
| case .dictionary(let posDict) = dict["position"], | ||
| case .int(let line) = posDict["line"], | ||
| case .int(let character) = posDict["character"] | ||
| else { | ||
| return hint | ||
| } | ||
| let position = Position(line: line, utf16index: character) | ||
|
|
||
| // get the type usr by calling cursor info at the variable position | ||
| let typeLocation = try await lookupTypeDefinitionLocation( | ||
| uri: uri, | ||
| position: position | ||
| ) | ||
|
|
||
| guard let typeLocation else { | ||
| return hint | ||
| } | ||
|
|
||
| // return new hint with label parts that have location for go-to-definition | ||
| if case .string(let labelText) = hint.label { | ||
| return InlayHint( | ||
| position: hint.position, | ||
| label: .parts([InlayHintLabelPart(value: labelText, location: typeLocation)]), | ||
| kind: hint.kind, | ||
| textEdits: hint.textEdits, | ||
| tooltip: hint.tooltip, | ||
| paddingLeft: hint.paddingLeft, | ||
| paddingRight: hint.paddingRight, | ||
| data: hint.data | ||
| ) | ||
| } | ||
|
|
||
| return hint | ||
| } | ||
|
|
||
| /// looks up the definition location for the type at the given position | ||
| private func lookupTypeDefinitionLocation( | ||
| uri: DocumentURI, | ||
| position: Position | ||
| ) async throws -> Location? { | ||
| let snapshot = try await self.latestSnapshot(for: uri) | ||
| let compileCommand = await self.compileCommand(for: uri, fallbackAfterTimeout: false) | ||
|
|
||
| // call cursor info at the variable position to get the type usr | ||
| let skreq = sourcekitd.dictionary([ | ||
| keys.cancelOnSubsequentRequest: 0, | ||
| keys.offset: snapshot.utf8Offset(of: position), | ||
| keys.sourceFile: snapshot.uri.sourcekitdSourceFile, | ||
| keys.primaryFile: snapshot.uri.primaryFile?.pseudoPath, | ||
| keys.compilerArgs: compileCommand?.compilerArgs as [any SKDRequestValue]?, | ||
| ]) | ||
|
|
||
| let dict = try await send(sourcekitdRequest: \.cursorInfo, skreq, snapshot: snapshot) | ||
|
|
||
| guard let typeUsr: String = dict[keys.typeUsr] else { | ||
| return nil | ||
| } | ||
|
|
||
| // look up the type definition in the index | ||
| guard let workspace = await sourceKitLSPServer?.workspaceForDocument(uri: uri), | ||
| let index = await workspace.index(checkedFor: .deletedFiles) | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| guard let occurrence = index.primaryDefinitionOrDeclarationOccurrence(ofUSR: typeUsr) else { | ||
| return nil | ||
| } | ||
|
|
||
| let definitionUri = DocumentURI(filePath: occurrence.location.path, isDirectory: false) | ||
| let definitionPosition = Position( | ||
| line: occurrence.location.line - 1, | ||
| utf16index: occurrence.location.utf8Column - 1 | ||
| ) | ||
|
|
||
| return Location(uri: definitionUri, range: Range(definitionPosition)) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,23 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| ) | ||
| } | ||
|
|
||
| /// compares hints ignoring the data field (which contains implementation-specific resolve data) | ||
| private func assertHintsEqual( | ||
| _ actual: [InlayHint], | ||
| _ expected: [InlayHint], | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) { | ||
| XCTAssertEqual(actual.count, expected.count, "Hint count mismatch", file: file, line: line) | ||
| for (actualHint, expectedHint) in zip(actual, expected) { | ||
| XCTAssertEqual(actualHint.position, expectedHint.position, file: file, line: line) | ||
| XCTAssertEqual(actualHint.label, expectedHint.label, file: file, line: line) | ||
| XCTAssertEqual(actualHint.kind, expectedHint.kind, file: file, line: line) | ||
| XCTAssertEqual(actualHint.textEdits, expectedHint.textEdits, file: file, line: line) | ||
| XCTAssertEqual(actualHint.tooltip, expectedHint.tooltip, file: file, line: line) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Tests | ||
|
|
||
| func testEmpty() async throws { | ||
|
|
@@ -73,7 +90,7 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| var y2️⃣ = "test" + "123" | ||
| """ | ||
| ) | ||
| XCTAssertEqual( | ||
| assertHintsEqual( | ||
| hints, | ||
| [ | ||
| makeInlayHint( | ||
|
|
@@ -106,7 +123,7 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| """, | ||
| range: ("1️⃣", "4️⃣") | ||
| ) | ||
| XCTAssertEqual( | ||
| assertHintsEqual( | ||
| hints, | ||
| [ | ||
| makeInlayHint( | ||
|
|
@@ -141,7 +158,7 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| } | ||
| """ | ||
| ) | ||
| XCTAssertEqual( | ||
| assertHintsEqual( | ||
| hints, | ||
| [ | ||
| makeInlayHint( | ||
|
|
@@ -198,7 +215,7 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| } | ||
| """ | ||
| ) | ||
| XCTAssertEqual( | ||
| assertHintsEqual( | ||
| hints, | ||
| [ | ||
| makeInlayHint( | ||
|
|
@@ -267,4 +284,36 @@ final class InlayHintTests: SourceKitLSPTestCase { | |
| ) | ||
| XCTAssertEqual(hints, []) | ||
| } | ||
|
|
||
| func testInlayHintResolve() async throws { | ||
| // test that resolving an inlay hint returns label parts with type location | ||
| let project = try await IndexedSingleSwiftFileTestProject( | ||
| """ | ||
| struct MyType {} | ||
| let x1️⃣ = MyType() | ||
| """ | ||
| ) | ||
|
|
||
| // get inlay hints | ||
| let request = InlayHintRequest(textDocument: TextDocumentIdentifier(project.fileURI), range: nil) | ||
| let hints = try await project.testClient.send(request) | ||
|
|
||
| // find thee type hint for x | ||
| guard let typeHint = hints.first(where: { $0.kind == .type }) else { | ||
| XCTFail("Expected type hint") | ||
| return | ||
| } | ||
|
|
||
| XCTAssertNotNil(typeHint.data, "Expected type hint to have data for resolution") | ||
|
|
||
| // resolve the hint to get type location | ||
| let resolvedHint = try await project.testClient.send(InlayHintResolveRequest(inlayHint: typeHint)) | ||
|
|
||
| if case .parts(let parts) = resolvedHint.label { | ||
| XCTAssertEqual(parts.count, 1) | ||
| XCTAssertNotNil(parts.first?.location, "Expected label part to have location for go-to-definition") | ||
| } else if case .string = resolvedHint.label { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test only passes because we get into this case because there are no parts. AFAIK the problem is that the typeUSR field in the cursor info request isn’t actually a valid USR (looks like it hasn’t been used in a while). Furthermore, a purely USR-based implementation won’t be able to through the fallback locations in I think the correct implementation would be to extend the cursorinfo request to return line, column, filepath, module name and USR of the declaration’s type (maybe I forgot something else as well). The place to start debugging in order to do this would likely be https://github.com/swiftlang/swift/blob/f4e78a61dfbbaae7f1f03861d7ea8077546a4146/tools/SourceKit/tools/sourcekitd/lib/Service/Requests.cpp#L2736 The good news is that #548 would fall out for (almost) free and you would be able to fix the oldest open issue in this repo 😉
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm seems like im having a hard time figuring out how to go forward with this ( does this require me to work within the compiler i mean swift/swiftlang and handle edgecases and tests for those as well because thats gotta be long ig )
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if not ,i spent yesterday looking up fixes and found one ( idk if it works or has any more edgecases ) we could look up the type by its name string (e.g., "MyType") instead of the broken USR; it handles most casesig and would get this feature working immediately without me having to go through the compiler repo , that might take this pr a while , i mean i can do it but it will just take time maybe 2-4 days ig atleast to get fmailiar with the repo
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, the entry barrier is definitely a little higher there but if you’re in for a challenge, I think you’d be up for it. If you get stuck somewhere, also feel free to just open a PR with how far you got and we can help you go on. I’d prefer to proper compiler-backed solution over something hacky in SourceKit-LSP because it will likely be a lot more error-resistant and easier to maintain.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okey , i will try to learn the repo for a few days and try to fix the issue , should i convert this to a draft PR in the meantime
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, yes at some point you will need to know about sourcekitd when contributing to SourceKit-LSP. It’s where most of the actual semantic understanding meat is implemented. And because it’s a little harder to set up, those are the issues that don’t get tackled as quickly 😉
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will try and do my best :) |
||
| } | ||
| } | ||
|
loveucifer marked this conversation as resolved.
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think this comment provides much value, so I’d just remove it.