-
Notifications
You must be signed in to change notification settings - Fork 280
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
Rename getReferenceDocument
request to textDocumentContent
#1639
base: main
Are you sure you want to change the base?
Conversation
CC @lokesh-tr |
Wow! It got merged 14 days back. @fwcd You rock, bro! 🔥 When I get some time, I will look into this in much detail. I do believe that It won't be hard to migrate. |
This includes advertising our custom "sourcekit-lsp" scheme to the client.
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 just have a few minor comments but, everything else looks good to me.
One more thought:
Given that the LSP now supports TextDocumentContentRequest
, I think we should now remove the code that generates temporary files for editors that doesn't support PeekDocumentsRequest
, and ensure that we generate macro expansions on the fly for that and display them by passing a ReferenceDocumentURL
to the ShowDocumentRequest
.
Sources/LanguageServerProtocol/Requests/TextDocumentContentRequest.swift
Outdated
Show resolved
Hide resolved
case .bool(true) = experimentalCapabilities["workspace/peekDocuments"], | ||
case .bool(true) = experimentalCapabilities["workspace/getReferenceDocument"] | ||
case .bool(true) = experimentalCapabilities["workspace/peekDocuments"] | ||
// TODO: Check if client supports LSP 3.18's workspace/textDocumentContent |
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 guess this check should be in place before we merge the PR
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.
Could you address this TODO? I don’t like having TODOs in the codebase without an associated issue because in practice they never get fixed.
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.
Yeah, I think we have to revert this for now. Once the request is upstreamed, we can relax this condition to support clients that don't explicitly declare it as an experimental capability.
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've re-added this and added a short remark to the PR description for how this check has to be updated once we wish to support the upstream client capability.
@fwcd As per what we discussed, I guess it would be nice to mark this as "Draft" (or atleast with some "Do NOT Merge") just to ensure that this doesn't get merged before the LSP spec gets finalised. |
I think it’s still valuable to have that logic for editors that don’t support the
Just so I understand, what was the reasoning why we want to wait for LSP 3.18 to get finalized before merging this? Would there be any harm in switching to the |
We could do that, but I wasn't sure if using a beta version of |
Oh, I was thinking that the Swift VS Code extension could continue intercepting the |
That's a nice idea, I don't see why that wouldn't work and as far as I understand we haven't shipped a version of sourcekit-lsp with this yet, so we wouldn't need to keep the legacy request name around for compatibility. |
Exactly 😉 |
I'm marking the PR as ready for review again, since I have successfully tested it together with swiftlang/vscode-swift#1050, which just adapts our existing client-side integration to the new request name/format. This means we should be able to safely merge it before LSP 3.18 is stabilized. We should probably still keep an eye on upstream, so we can incorporate any changes to the API as it is updated, but that could be done in future PRs. |
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.
Looks great. Just a few minor comments.
Sources/LanguageServerProtocol/Requests/TextDocumentContentRequest.swift
Show resolved
Hide resolved
case .bool(true) = experimentalCapabilities["workspace/peekDocuments"], | ||
case .bool(true) = experimentalCapabilities["workspace/getReferenceDocument"] | ||
case .bool(true) = experimentalCapabilities["workspace/peekDocuments"] | ||
// TODO: Check if client supports LSP 3.18's workspace/textDocumentContent |
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.
Could you address this TODO? I don’t like having TODOs in the codebase without an associated issue because in practice they never get fixed.
Once the request is upstreamed to LSP 3.18 we should relax this condition to support clients that don't declare it as an experimental capability (since it will become a standard LSP request).
Since we 'vendor' it as an LSP extension for now, we'll document it here until it is fully upstreamed to LSP 3.18.
3e38261
to
dd7669a
Compare
getReferenceDocument
to upstream textDocumentContent
requestgetReferenceDocument
request to textDocumentContent
Tested with the VSCode extension and could successfully verify that it works. I had to re-add some of the experimental capability logic in 99836e0, but we'll need that anyway until the request is fully supported by upstream LSP. |
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.
Thanks!
I was wondering how we got away without the capabilities in initializationOptions
but I took your word for it 😆 But guess it means that we do need them 🤷🏽
@swift-ci Please test |
By removing the capability check, which I still did under the assumption that we'd hold off with this PR until the request is landed upstream (i.e. where I wrote the todo comment) 😄 |
@swift-ci Please test Windows |
This is great! 🚀 🚀 🚀 |
Support for retrieving documents via custom URI schemes has recently been upstreamed and will be supported in the upcoming version 3.18 of the Language Server Protocol:
sourcekit-lsp currently uses a non-standard
workspace/getReferenceDocument
LSP extension for this, specifically for retrieving macro expansions. Since this requires either client-side support or workarounds (e.g. generation of temporary files), this PR migrates our LSP extension to the upstreamworkspace/textDocumentContent
request.The branch compiles and already works with the sibling PR for the VSCode extension:
workspace/getReferenceDocument
toworkspace/textDocumentContent
vscode-swift#1050Once LSP 3.18 ships, we can remove the client-side integration completely in favor of upstream LSP support:
workspace/textDocumentContent
vscode-swift#1027Assuming the proposed request does not change further, all we need to do to support LSP clients implementing the standardized
textDocumentContent
request is to relax the capability check slightly to additionally queryclientCapabilities.workspace?.textDocumentContent
1:sourcekit-lsp/Sources/SourceKitLSP/Swift/MacroExpansion.swift
Lines 220 to 222 in 99836e0
Footnotes
We'll do this in a future PR since it is not clear how this capability is structured, given that VSCode doesn't send it yet. ↩