add go-to-definition for inlay hints (#2318)#2436
Conversation
Implements resolveProvider for inlay hints to enable navigating to type definitions. When an inlay hint showing a type is resolved, the server looks up the type's definition location using cursorInfo and the index. - store variable position in InlayHint.data for resolution - add inlayHintResolve to LanguageService protocol - implement resolve handler using cursorInfo and index lookup - enable resolveProvider: true in capabilities - add test for resolve functionality Addresses swiftlang#2318
| XCTAssertEqual(parts.count, 1) | ||
| XCTAssertNotNil(parts.first?.location, "Expected label part to have location for go-to-definition") | ||
| } else if case .string = resolvedHint.label { | ||
|
|
There was a problem hiding this comment.
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 indexBasedDefinition and definitionLocations. In particular, it won’t be able to jump to a generated interface or jump to a definition if background indexing is disabled.
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 😉
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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
There was a problem hiding this comment.
does this require me to work within the compiler i mean swift/swiftlang
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
will try and do my best :)
|
new commit requires swiftlang/swift#86381 for compiler changes |
update inlay hint resolution to use the new type declaration location fields from cursorInfo, with fallback to index lookup using the new typeDeclUsr (a proper declaration USR). Changes i made : - Regenerated sourcekitd_uids.swift with new keys - InlayHintResolve.swift: Try direct location first, fallback to index - InlayHintTests.swift: Fixed test to properly verify location This Requires: swift/swiftlang PR## #86381 for compiler changes
bd7d3e0 to
f46a31b
Compare
Add support for the textDocument/typeDefinition LSP request, which finds the type of the symbol at a given position and returns the location of that type's definition. This uses the same type definition lookup mechanism as the inlay hint resolution feature, which queries cursorInfo for the new type declaration location fields (typeDeclFilePath/Line/Column) with fallback to index lookup using typeDeclUsr. Fixes swiftlang#548
|
Very nice, I was expecting you to take longer to create a sourcekitd PR, good job 🌟 Let’s wait for swiftlang/swift#86381 to be approved and then I’ll review this PR in detail. Looked good from a first glance. |
yess i thought so too but i did have some previous experience with c++ while wriitng my game engine and I spent some time yesterday and today reading through the repo and thought might as well do it and learn more as i go on :D , thanks for your kind wordsss ❤️ |
|
following the discussion in swiftlang/swift#86381. with hamishknight , ultimately reached the conclusion that we dont want any compiler changes , i am sending a pr that oughtta fix this but if not back to the compiler repo again we go :D |
|
as mentinoed above, i discovered that no changes are needed in the Swift repo. The existing cursorInfo already supports looking up types by USR via key.usr. Index lookups via primaryDefinitionOrDeclarationOccurrence(ofUSR:) added convertMangledTypeToUSR() helper for the conversion also i closed the Swift repo PR #86381 as it's no longer needed. |
ahoppen
left a comment
There was a problem hiding this comment.
Uhh, very nice that we can do this without modifying the cursor info request. I know about the USR-based cursor info but totally forgot about it again. I hope you still learned something worthwhile setting up you compiler development environment 😉
|
|
||
| # Used exclusively within the SourceKit Plugin | ||
| KIND('DiagRemark', 'source.diagnostic.severity.remark'), | ||
| # Note: DiagRemark was moved to UID_KINDS in UIDs.py |
There was a problem hiding this comment.
I don’t think this comment is providing any value. Let’s just remove it.
| func definition(_ request: DefinitionRequest) async throws -> LocationsOrLocationLinksResponse? | ||
|
|
||
| func declaration(_ request: DeclarationRequest) async throws -> LocationsOrLocationLinksResponse? | ||
| func typeDefinition(_ request: TypeDefinitionRequest) async throws -> LocationsOrLocationLinksResponse? |
There was a problem hiding this comment.
I think typeDefinition doesn’t have any tests at the moment yet. I would suggest we move its implementation to a follow-up PR and focus this one on the inlay hints.
| } | ||
|
|
||
| func inlayHintResolve(_ req: InlayHintResolveRequest) async throws -> InlayHint { | ||
| // default: return hint unchanged |
There was a problem hiding this comment.
I don’t think this comment provides much value, so I’d just remove it.
| // inlay hints store the uri in data for resolution | ||
| // extract uri from the lspany dictionary |
There was a problem hiding this comment.
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.
- Use cursorInfo USR lookup instead of index (more accurate) - Add document version tracking to reject stale resolve requests - Make InlayHintResolveData conform to LSPAnyCodable - Reference swiftlang/swift#86432 for mangled type workaround - cursorInfoFromTypeUSR takes DocumentSnapshot for version safety - Remove TypeDefinition.swift (defer to follow-up PR) - Remove unnecessary comments - Tests work without index
Yess totally and i bet it will come of use while fixing other issues ,i have noticed that some of the issues needs me to work within it so its good . |
| // Strip trailing 'D' (type mangling suffix) to get declaration USR | ||
| if result.hasSuffix("D") { | ||
| result = String(result.dropLast()) | ||
| } |
There was a problem hiding this comment.
You shouldn't need to do this, cursor info should be able to handle a type USR with it
There was a problem hiding this comment.
Hmm but the tests fail without the D stripping.
There was a problem hiding this comment.
Oh interesting, I saw handling of Demangle::Node::Kind::Type in the code and assumed that was referring to D but it's actually a different kind. I guess this is okay for now but we really ought to fix this on the sourcekitd side, ideally at the same time as the $s -> s: change.
There was a problem hiding this comment.
Just to add my 2cts, while I’m not a huge fan, I’m fine with having the stripping of D as a temporary workaround for now as long as we try to adjust sourcekitd to work without this hack in the future.
ahoppen
left a comment
There was a problem hiding this comment.
Left a few more comments inline. I think the best way forward might be:
- Remove all remainders of the type definition request from this PR
- Get this merged
- Support jumping to a generated interface from inlay hints. I suspect that this might require some careful re-shuffling of code which might be easier to review if done on its own without having to worry the changes that are currently part of this PR. And this PR already provides great value on its own
Also, just want to express how extremely excited I am to use the type definition request once it gets in. I keep wanting that multiple times a week 🙂
c91c9d7 to
9e12af1
Compare
|
there is already a follow up PR in #2445 , should i include things that i left out here on that pr or work on an entirely new one ? |
I just noticed that we eg. still have |
ahoppen
left a comment
There was a problem hiding this comment.
Good to go, let’s just remove all references to typeDefinition from the diff.
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
hmm failed ig 😵💫 |
|
swift-format validation failed with the following errors because the Could you add them back in, ideally in a way that they don’t get removed when re-generating the file 😉 |
Head branch was pushed to by a user without write access
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
There was a problem hiding this comment.
Could you add this file to CMakeLists.txt to make the tests pass on Windows?
Head branch was pushed to by a user without write access
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
hmm another macos fail :/ |
|
Failures is unrelated. Let’s try again. @swift-ci Please test macOS |
|
hmm failed again ig |
|
Oh, interesting. Almost all the tests failed with @swift-ci Please test macOS |
|
Oh lol , lets see also i did apply for the member program , been 6 days didnt hear back seems like not upto it :/ |
|
seems like a github issue again @swift-ci please test macos |
|
Looking through the logs after the We are seeing a sourcekitd crash in I tested if this might have been introduced by a recent change in SwiftPM but have been unable to reproduce the issue by building SwiftPM from source so far. I’m now testing if this issue is caused by changes in this PR (which I can’t really believe) or if it’s caused by something else by re-triggering CI on #2450. I fear this will boil down to having to build an entire toolchain locally and trying to reproduce it that way. All that is to say, I’ll take care of it and come back once I know more. |
|
Update: Also seeing the same failure in swiftlang/indexstore-db#276, so it’s not related to this PR. Now the debugging fun begins. |
|
Crash should be fixed by: swiftlang/swift#86583. Let's try again. @swift-ci Please test macOS |
|
@swift-ci Please test macOS |
|
Finally 🎉🎉🎉 When you have time, do you mind rebasing the type definition PR and I'll review it next. |
doing it rn hold on :D |
Implements resolveProvider for inlay hints to enable navigating to type definitions. When an inlay hint showing a type is resolved, the server looks up the type's definition location using cursorInfo and the index.
Addresses #2318