[lsp] Report progress using $/progress notifications

[weirdan]

Currently, we're using ad-hoc solution to report progress to vscode extension, using telemetry events. This requires specific client-side support. There's now a better option: reporting the progress using $/progress notifications, that are allowed during the initialize phase if client sends workDoneToken in initialize request. Using it would allow reporting the progress to any compliant client, not just our vscode ext.

Spec links:

• initialize request
• workdone notifications

Open questions:

1. How do we prevent duplicate reports when older vscode ext connects to newer psalm server? Client sniffing?
2. Neovim doesn't seem to be sending workDoneToken in initialize requests (this can probably be overridden somewhere).

[psalm-github-bot]

Hey @weirdan, can you reproduce the issue on https://psalm.dev ?

[weirdan]

/cc: @tm1000

[tm1000]

@weirdan so I believe trying to get $ prefixes to work with https://github.com/felixfbecker/php-advanced-json-rpc is part of the issue. The way json-rpc works is it maps the RPC requests to methods and a method/function can't start with a $ however I haven't looked in depth at it to see if it maps $ to something like _

How do we prevent duplicate reports when older vscode ext connects to newer psalm server? Client sniffing?

So the VSCode client is sent to the server but I believe that isn't in the LSP spec it's just something VSCode does.

If I (we, whomever) can figure out a way to get the $ to work it would be nice to also get $/cancelRequest to work which is useful since we run everything through a defer throttler now in amp.

You can assign this to me and I can look into the JSON rpc server and step through it using xdebug

[weirdan]

it maps the RPC requests to methods

Doesn't it only happen for incoming requests though? $/progress is something we send, not receive.

[weirdan]

As a quick test, I added (1) to initialize handler and got (2):

[weirdan]

it maps the RPC requests to methods

The way mapping works it splits the request name using a separator (we use /), and then uses the last part as a method to call and all preceding parts as nested property access. So $/cancelRequest becomes a call to $server->{"$"}->cancelRequest().

Unlike methods, property names can be arbitrary, so $ as a property name is possible. The following worked for me (in LanguageServer::__construct()):

        $this->{"$"} = new class($this) {
            private object $parent;
            public function __construct(object $parent)
            {
                $this->parent = $parent;
            }
            public function cancelRequest(mixed ...$args): mixed
            {
                return $this->parent->cancelRequest(...$args);
            }
        };

to forward the call to LanguageServer::cancelRequest().

Alternatively, we could define __isset() and __get() magic methods. __isset() would return true if property being accessed was $, and __get() would return $this for that same property.

[tm1000]

Oh. Wow. I never knew that about Php!!

[weirdan]

So the VSCode client is sent to the server but I believe that isn't in the LSP spec it's just something VSCode does.

On the other hand it won't work to detect the extension version, as it will likely contain something like VSCode 1.2.3 (at least neovim sends name = "Neovim", version = "0.9.1").

Perhaps something like the following could work

local reporting_type = 'progress'
if client.name == 'VSCode' then
  reporting_type = 'telemetry'
  if get_hidden_config_param('psalm-vscode.version') > '1.2.3' then
    reporting_type = 'progress'
  end
end

[tm1000]

@weirdan look at how the go LSP does it. It does it based on a client capability being checked. That's what the psalm LSP should do as well

See: https://go.googlesource.com/tools/+/refs/tags/gopls/v0.5.0/internal/lsp/progress.go#35

[weirdan]

So we would use $/progress (and not telemetry) even when old extension is installed with a vscode version that supports $/progress. For clients that do not support $/progress we will use telemetry, in hope that it's an old vscode with an old extension talking to us (or another client understanding our ad-hoc reporting method).

Sounds good to me.

[weirdan]

Fixed in #10050