apple
Rename `getReferenceDocument` request to `textDocumentContent`
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:
- https://github.com/microsoft/language-server-protocol/issues/336
- https://github.com/microsoft/language-server-protocol/pull/1994
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 upstream workspace/textDocumentContent request.
The branch compiles and already works with the sibling PR for the VSCode extension:
- https://github.com/swiftlang/vscode-swift/pull/1050
Once LSP 3.18 ships, we can remove the client-side integration completely in favor of upstream LSP support:
- https://github.com/swiftlang/vscode-swift/pull/1027
Assuming 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 query clientCapabilities.workspace?.textDocumentContent[^1]:
https://github.com/swiftlang/sourcekit-lsp/blob/99836e08c3e362cae6876e6e678a531fc73fce93/Sources/SourceKitLSP/Swift/MacroExpansion.swift#L220-L222
[^1]: 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.
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.
@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.
Given that the LSP now supports
TextDocumentContentRequest, I think we should now remove the code that generates temporary files for editors that doesn't supportPeekDocumentsRequest, and ensure that we generate macro expansions on the fly for that and display them by passing aReferenceDocumentURLto theShowDocumentRequest.
I think it’s still valuable to have that logic for editors that don’t support the workspace/textDocumentContent request, which will probably be most editors for now. All these newer requests are optional and it’s possible for editors to not declare that capability.
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.
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 workspace/textDocumentContent request now, implement the middleware to handle in in VS Code and then if other editors start implementing LSP 3.18 early, great and if not, we aren’t off any worse than we are right now?
We could do that, but I wasn't sure if using a beta version of vscode-languageclient for the extension is acceptable (and I'm not sure if the request ships there before it is stabilized in LSP). If it is, then sure.
Oh, I was thinking that the Swift VS Code extension could continue intercepting the workspace/textDocumentContent request in the middleware for now with the existing logic. And once VS Code adds native support for workspace/textDocumentContent, we can just remove that middleware. Would that work?
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.
I'm marking the PR as ready for review again, since I have successfully tested it together with https://github.com/swiftlang/vscode-swift/pull/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.
Tested with the VSCode extension and could successfully verify that it works. I had to re-add some of the experimental capability logic in 99836e08c3e362cae6876e6e678a531fc73fce93, but we'll need that anyway until the request is fully supported by upstream LSP.
I was wondering how we got away without the capabilities
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) 😄