Rename `getReferenceDocument` request to `textDocumentContent` #1639

fwcd · 2024-08-22T16:38:54Z

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:

Proposal: Allow a language server to provide a reference content for a specific uri scheme microsoft/language-server-protocol#336
Document text document content request and update changelog microsoft/language-server-protocol#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:

Rename workspace/getReferenceDocument to workspace/textDocumentContent vscode-swift#1050

Once LSP 3.18 ships, we can remove the client-side integration completely in favor of upstream LSP support:

Migrate to upstream workspace/textDocumentContent vscode-swift#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¹:

sourcekit-lsp/Sources/SourceKitLSP/Swift/MacroExpansion.swift

Lines 220 to 222 in 99836e0

    
           if case .dictionary(let experimentalCapabilities) = self.capabilityRegistry.clientCapabilities.experimental, 
        
             case .bool(true) = experimentalCapabilities["workspace/peekDocuments"], 
        
             case .bool(true) = experimentalCapabilities["workspace/textDocumentContent"]

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. ↩

ahoppen · 2024-08-22T18:11:04Z

CC @lokesh-tr

lokesh-tr · 2024-08-22T18:35:10Z

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.

fwcd · 2024-08-23T02:13:14Z

Can confirm that this works (in conjunction with the VSCode extension PR):

lokesh-tr

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

lokesh-tr · 2024-08-23T10:40:28Z

Sources/SourceKitLSP/Swift/MacroExpansion.swift

-      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


I guess this check should be in place before we merge the PR

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.

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.

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.

lokesh-tr · 2024-08-23T15:20:11Z

@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.

ahoppen · 2024-08-26T15:56:09Z

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.

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?

fwcd · 2024-08-26T20:34:01Z

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.

ahoppen · 2024-08-26T21:24:01Z

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?

fwcd · 2024-08-26T21:25:50Z

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.

ahoppen · 2024-08-26T21:27:37Z

Exactly 😉

fwcd · 2024-09-03T18:33:26Z

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.

ahoppen

Looks great. Just a few minor comments.

Contributor Documentation/LSP Extensions.md

Sources/LanguageServerProtocol/Requests/TextDocumentContentRequest.swift

ahoppen · 2024-09-04T21:12:02Z

Sources/SourceKitLSP/Swift/MacroExpansion.swift

-      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


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.

fwcd · 2024-09-04T21:52:08Z

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.

ahoppen

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 🤷🏽

ahoppen · 2024-09-04T22:32:15Z

@swift-ci Please test

fwcd · 2024-09-04T22:35:24Z

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) 😄

ahoppen · 2024-09-04T22:40:46Z

@swift-ci Please test Windows

lokesh-tr · 2024-09-07T16:04:46Z

This is great! 🚀 🚀 🚀

fwcd added 3 commits August 22, 2024 18:12

Migrate getReferenceDocument to upstream textDocumentContent

5db1179

Remove getReferenceDocument capability

49d5134

Add a todo note

fd417c7

fwcd added 3 commits August 23, 2024 03:23

Add TextDocumentContentOptions to WorkspaceServerCapabilities

066c385

Declare textDocumentContent server capabilities

a7f9382

This includes advertising our custom "sourcekit-lsp" scheme to the client.

Add WorkspaceClientCapabilities.textDocumentContent

1dc0213

fwcd mentioned this pull request Aug 23, 2024

Migrate to upstream workspace/textDocumentContent swiftlang/vscode-swift#1027

Draft

fwcd marked this pull request as ready for review August 23, 2024 02:13

fwcd requested a review from ahoppen as a code owner August 23, 2024 02:13

Fix typo

6e07e52

lokesh-tr reviewed Aug 23, 2024

View reviewed changes

fwcd marked this pull request as draft August 23, 2024 15:26

Remove outdated LSP extension comment

af99d89

fwcd mentioned this pull request Sep 3, 2024

Rename workspace/getReferenceDocument to workspace/textDocumentContent swiftlang/vscode-swift#1050

Draft

fwcd marked this pull request as ready for review September 3, 2024 18:32

ahoppen reviewed Sep 4, 2024

View reviewed changes

fwcd added 4 commits September 4, 2024 23:35

Re-add check for textDocumentContent capability

6ca4d95

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).

Re-add documentation for textDocumentContent request

88ca0c6

Since we 'vendor' it as an LSP extension for now, we'll document it here until it is fully upstreamed to LSP 3.18.

Add doc comment from LSP to TextDocumentContentResponse

9702a28

Update documentation on workspace/textDocumentContent

dd7669a

fwcd force-pushed the text-document-content-request branch from 3e38261 to dd7669a Compare September 4, 2024 21:35

Re-parse experimental client capability for textDocumentContent

99836e0

fwcd changed the title ~~Migrate getReferenceDocument to upstream textDocumentContent request~~ Rename getReferenceDocument request to textDocumentContent Sep 4, 2024

ahoppen reviewed Sep 4, 2024

View reviewed changes

ahoppen enabled auto-merge September 4, 2024 22:32

ahoppen approved these changes Sep 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `getReferenceDocument` request to `textDocumentContent` #1639

Rename `getReferenceDocument` request to `textDocumentContent` #1639

fwcd commented Aug 22, 2024 •

edited

Loading

ahoppen commented Aug 22, 2024

lokesh-tr commented Aug 22, 2024 •

edited

Loading

fwcd commented Aug 23, 2024

lokesh-tr left a comment

lokesh-tr Aug 23, 2024

ahoppen Sep 4, 2024

fwcd Sep 4, 2024 •

edited

Loading

fwcd Sep 4, 2024

lokesh-tr commented Aug 23, 2024

ahoppen commented Aug 26, 2024

fwcd commented Aug 26, 2024 •

edited

Loading

ahoppen commented Aug 26, 2024

fwcd commented Aug 26, 2024

ahoppen commented Aug 26, 2024

fwcd commented Sep 3, 2024 •

edited

Loading

ahoppen left a comment

ahoppen Sep 4, 2024

fwcd commented Sep 4, 2024

ahoppen left a comment

ahoppen commented Sep 4, 2024

fwcd commented Sep 4, 2024

ahoppen commented Sep 4, 2024

lokesh-tr commented Sep 7, 2024

	if case .dictionary(let experimentalCapabilities) = self.capabilityRegistry.clientCapabilities.experimental,
	case .bool(true) = experimentalCapabilities["workspace/peekDocuments"],
	case .bool(true) = experimentalCapabilities["workspace/textDocumentContent"]

Rename getReferenceDocument request to textDocumentContent #1639

Are you sure you want to change the base?

Rename getReferenceDocument request to textDocumentContent #1639

Conversation

fwcd commented Aug 22, 2024 • edited Loading

Footnotes

ahoppen commented Aug 22, 2024

lokesh-tr commented Aug 22, 2024 • edited Loading

fwcd commented Aug 23, 2024

lokesh-tr left a comment

Choose a reason for hiding this comment

lokesh-tr Aug 23, 2024

Choose a reason for hiding this comment

ahoppen Sep 4, 2024

Choose a reason for hiding this comment

fwcd Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

fwcd Sep 4, 2024

Choose a reason for hiding this comment

lokesh-tr commented Aug 23, 2024

ahoppen commented Aug 26, 2024

fwcd commented Aug 26, 2024 • edited Loading

ahoppen commented Aug 26, 2024

fwcd commented Aug 26, 2024

ahoppen commented Aug 26, 2024

fwcd commented Sep 3, 2024 • edited Loading

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen Sep 4, 2024

Choose a reason for hiding this comment

fwcd commented Sep 4, 2024

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented Sep 4, 2024

fwcd commented Sep 4, 2024

ahoppen commented Sep 4, 2024

lokesh-tr commented Sep 7, 2024

Rename `getReferenceDocument` request to `textDocumentContent` #1639

Rename `getReferenceDocument` request to `textDocumentContent` #1639

fwcd commented Aug 22, 2024 •

edited

Loading

lokesh-tr commented Aug 22, 2024 •

edited

Loading

fwcd Sep 4, 2024 •

edited

Loading

fwcd commented Aug 26, 2024 •

edited

Loading

fwcd commented Sep 3, 2024 •

edited

Loading