feat(lsp): Add textDocument/implementation

[hkmatsumoto]

Part of #8643

[CLAassistant]

All committers have signed the CLA.

[kitsonk]

Nice contribution... Thank you.

LGTM

We need to enable "ignoring" the same results from the built in language service in the vscode plugin: denoland/vscode_deno#301. I can do that if you wish, but also feel free to raise a PR for that.

[kitsonk]

Nice! 💯

[hkmatsumoto]

We need to enable "ignoring" the same results from the built in language service in the vscode plugin: denoland/vscode_deno#301. I can do that if you wish, but also feel free to raise a PR for that.

Can you do that instead of me please? I unfortunately have exams this and next week.

[rwols]

@matsujika when opening a remote file with textDocument/implementation, the server now returns an URI with a strange scheme:

:: --> Deno textDocument/implementation(4): {'position': {'character': 12, 'line': 0}, 'textDocument': {'uri': 'file:https:///Users/raoulwols/dev/denoserver/server.ts'}, 'workDoneToken': 'wd4'}
:: <<< Deno 4: [{'targetUri': 'deno:/https/deno.land/std/http/server.ts', 'targetRange': {'end': {'character': 1, 'line': 305}, 'start': {'character': 0, 'line': 298}}, 'targetSelectionRange': {'end': {'character': 21, 'line': 298}, 'start': {'character': 16, 'line': 298}}}]

I would expect targetUri to start with https://. Instead it starts with deno:/. What is the reasoning behind using deno:/?

[kitsonk]

deno: is used so that IDEs can ensure that the Deno LSP handles the documents as virtual text documents. It is necessary for the proper operation of LSP.

[rwols]

This seems to require client glue code for each separate client, which defeats the purpose of LSP. https://github.com/denoland/vscode_deno/blob/34b7130f3b810a635f88dd88e5927f3faaa1bffa/client/src/lsp_extensions.ts

Handling custom URI schemes is something we can account for I guess.

[kitsonk]

@rwols far from it. There is always still some client glue no matter what, for commands. Just passing the raw scheme would cause lots of other client issues, because for remote modules. Like you don't require extensions on URLs so how does the editor know the media type to route to the correct extension. By moving it to a custom scheme it is 100% clear to the editor who additional requests and notifications should go through. You could maybe argue it is a gap in the LSP protocol, but the protocol is designed to have extensions.

Those lsp extensions are documented in the README for the Deno LSP: https://github.com/denoland/deno/tree/main/cli/lsp

[rwols]

The choice could have been made to apply the following strategy:

1. Download the resource to a temp location on the filesystem.
2. Set targetUri to that temp location on the filesystem.
3. Reply to the client request with a file:https:// scheme.

Not every client has the notion of a "virtual text document".

[kitsonk]

@Rowls that does not work... we previously tried that in the v2 branch of the extension. URLs don't make great file systems paths, especially when accounting for queries, also consider data URLs which Deno supports. That means you have to hash the filename and then the hashed file name doesn't look anything like the source file, so you get these really ugly documents. The virtual document strategy is far more robust. Would be glad to work with any team trying to build a client for Deno LSP to understand how virtual text document support is critical, as remote modules are virtual documents that can be opened in the editor.

[rwols]

@kitsonk https://github.com/sublimelsp/LSP-Deno

with respect to deno/virtualTextDocument I am kind of missing a Language ID in the response that tells the client exactly what syntax to apply for the received buffer content. Consider adding that.

[lucacasonato]

@rwols Could you make a new issue for that? It is hard to keep track of things if they are just posted as comments on old PRs :-)