Allow to disable selected capabilities by the client #58

przepompownia · 2022-01-22T22:17:31Z

Closes #56

…etion`

przepompownia · 2022-01-22T22:27:54Z

I still do not understand why SignatureHelpOptions is set in two places.

przepompownia · 2022-01-22T22:44:24Z

Is $provideTextEdit left as default false intentionally in CompletionHandler's constructor?

przepompownia · 2022-01-23T01:27:57Z

testHandleWithDisabledCapability() should not pass in 619ad40 (it currently is almost copy of testHandleACompleteListOfSuggestions but I expect that completion() won't be called (or return an empty result?)). @dantleech do you have idea how to fix it?

dantleech

Looks like some 7.3 issues with the build. Away from keyboard at the moment, but if the test is of a different category to the other test it's "fine" to have another test method, possibly the duplication could be reduced? But otherwise it is what it is.

dantleech · 2022-01-23T16:21:15Z

tests/LanguageServerCompletion/Unit/Handler/CompletionHandlerTest.php

+        bool $supportSnippets = true,
+    ): ClientCapabilities {
+        $capabilities = new ClientCapabilities();
+        $capabilities->textDocument = new TextDocumentClientCapabilities();


You can call Client capabilities::fromArray() or similar if it helps to generalize.

I know about it but in this case building the array would look similar.

…rzepompownia:przepompownia/language-server-phpactor-extensions into allow-disable-selected-capabilities-by-the-client

przepompownia · 2022-01-23T18:54:49Z

lib/LanguageServerReferenceFinder/Handler/GotoDefinitionHandler.php

+    /**
+     * @var ClientCapabilities
+     */
+    private ClientCapabilities $clientCapabilities;


I forgotten that the whole project still depends on (unsupported) PHP 7.3 and typed properties are not allowed.

yeah, it would be good to bump it again. just a big PITA with 50 packages.

dantleech · 2022-01-25T12:10:03Z

I still do not understand why SignatureHelpOptions is set in two places.

There is no reason. I think the CompletionHandler pre-dated the SignatureHelp handler.

dantleech · 2022-01-25T12:11:04Z

lib/AbstractHandler.php

+
+    public function __construct(ClientCapabilities $clientCapabilities)
+    {
+        $this->clientCapabilities = $clientCapabilities;


I'd rather avoid generalizing abstract classes. Not all handlers need to know the client capabilities, and chances are you need to override the constructor anyways.

I can move back those duplicates if you want.

Indeed, I forgotten to update some handlers to be depend on the client capabilities.

dantleech · 2022-01-25T12:11:59Z

lib/AbstractExtension.php

+    protected function clientCapabilities(Container $container): ClientCapabilities
+    {
+        return $container->get(ClientCapabilities::class);
+    }


as below, i'd rather avoid this...

dantleech · 2022-01-25T12:13:34Z

lib/LanguageServerReferenceFinder/Handler/ReferencesHandler.php

@@ -157,7 +161,7 @@ public function references(

    public function registerCapabiltiies(ServerCapabilities $capabilities): void
    {
-        $capabilities->referencesProvider = true;
+        $capabilities->referencesProvider = null !== $this->clientCapabilities->textDocument->references;


is this correct? the server is still capable, the client is not? I would think this would only hide the fact the server supports it with no benefit to the client?

I did not found any better solution to prevent the client from calling to phpactor's given method.

ah I see.

the goal here is f.e. disable completion and use another LS for that?

I don't think this is about client capabilities (I don't think that can necessarily be controlled by the user in every IDE?). I think this would then need to be a Phpactor level config, or mechanism to enable / disable extensions/handlers.

I still work on a legacy project mentioned in phpactor/phpactor#1125 Calling for completion or references still takes unacceptable time (with index on in-memory FS, JIT and ComposerAutoloaderExtension) and warms up CPU.

It forces me to use intelephense instead for these costly requests.

This change allows me to use phpactor anywhere on that project (and then possibly learn it, find bugs etc.).

Maybe not every IDE can make this selection but LSP seems to ~~allow~~ require it:

ClientCapabilities define capabilities for dynamic registration, workspace and text document features the client supports. The experimental can be used to pass experimental capabilities under development. For future compatibility a ClientCapabilities object literal can have more properties set than currently defined. Servers receiving a ClientCapabilities object literal with unknown properties should ignore these properties. A missing property should be interpreted as an absence of the capability. If a missing property normally defines sub properties, all missing sub properties should be interpreted as an absence of the corresponding capability.

(from https://microsoft.github.io/language-server-protocol/specification#initialize)

text document features the client supports

but the client defines what the client supports not the user? in your case you want to disable (f.e.) text completion. the client still supports it.

in this PR the client determines what the server supports, which seems wrong.

(i definitely support enabling/disabling functionality btw, there is a good use case for disabling completion and using another LS for that)

the client still supports it.

I understand the word "client" as a concrete instance (so it supports at most those capabilities which it declared at initialization) - not as a software able to create such (variously configured) instances. LSP-compliant server seems to know only about the declared set.

A missing property should be interpreted as an absence of the capability.

in this PR the client determines what the server supports

Yes, it (at this PR finish) would restrict the set according to the rule quoted above.

$capabilities->referencesProvider = null !== $this->clientCapabilities->textDocument->references;

read literally, this says "I support references only if the client supports it". But that's a lie, it supports references regardless of if the client supports it.

As far as I understand it (and I can't see any contraditction in that spec extract). Capabilties braodcast what the server/client support. They do not intefere with eachother. We use the client capabilities to see if the client supports certain messages from the server. If the client doesn't support certain features, the server won't send those messages.

Unless I'm wrong I would rather we added explicit Phpactor configuration to enable/disable handlers somehow, as this would implicitly inform the ServerCapabilties (because they simply would not be registered, and the RPC methods would not be exposed) and the Client would then not use the server for f.e. text completion.

If the client doesn't support certain features, the server won't send those messages.

It's what I try to implement there. For example, client (understood as a configured instance) does not support textDocument/references and then the server in the response says that it does not provide it to the client on its demand. Maybe, technically, in this PR the server can still response for textDocument/references by some tricky way (is it mentioned lie?). Maybe we need to prevent from execution this method in such ways.

What else is the purpose of sending client LSP-defined capabilities than making an intersection with the server-provided set in the result?

See also

https://www.reddit.com/r/neovim/comments/nvds8f/disable_single_capabilities_for_a_given_lsp_server/

Add support for CodeAction.disabled microsoft/language-server-protocol#940

I still do not understand if dynamicRegistration can be related to this issue:
https://medium.com/@malintha1996/understanding-the-language-server-protocol-5c0ba3ac83d2#6c58

dantleech · 2022-01-26T09:27:29Z

What else is the purpose of sending client LSP-defined capabilities than making an intersection with the server-provided set in the result?

So that the server knows what the client supports so it can avoid sending messages to the client that would result in an error I think.

In that Reddit post the servers do not support the capabilities, it's a different problem I think and the goal is to stop the client sending messages.

Would it not also make equal sense that the client could be configured to disable certain features and not request them? If you go to the trouble of disabling the Client Capabilty, surely the client can also simply not use that capability (although to me that also makes no sense, as capabilities are not, or do not have to be, user defined)

dantleech · 2022-01-26T09:28:43Z

I still do not understand if dynamicRegistration can be related to this issue:

I don't think it is. It's a way to say "from now on, I don't want the client to send me these messages" I think. Dynamic registration is sever => client. IIRC

przepompownia · 2022-01-26T12:11:37Z

So that the server knows what the client supports so it can avoid sending messages to the client that would result in an error I think.

Yes, and if I understand you mean support as the set of possible client capabilities and I mean those chosen from them for a single instance.

For example, nvim built-in client allows to start with vim.lsp.protocol.make_client_capabilities() and change this table before sending to the server. Sometimes we can extend this table but this is not the case.

Do you have concerns that this is LSP incompatible?

In summary we have three possible independent solutions:

client-side implementation - requires additional implementation from the client app and additional configuration options. If you start using another client then you can have the same problem again (I found an example https://lsp.sublimetext.io/guides/client_configuration/ with phpactor)
server side non-LSP implementation - requires also specific server-side configuration per project
server-side LSP implementation (as there) - assuming that is it LSP-compliant, it requires no client or server specific configuration options - only the standard capability set.

dantleech · 2022-01-26T23:32:03Z

Do you have concerns that this is LSP incompatible?

yes, it's an interpretation of the spec which is not specific on this (or I missed something). I would assume that other LS servers do not disable capabilities based on the client configuration, and then this is not a general solution, but a "hack" for Phpactor

e.g. I would like to disable completion, but there is not a client capabiltiy for that, only a server one.

interesting that the Sublime LSP server has the disable_capabilities feature. I tend to think it should be client side.

przepompownia · 2022-01-27T00:33:41Z

Neither of us seems to be 100% sure how it should be. I also would not like to propose a hack. The source is my understanding that this completes the missing protocol behavior. Maybe it's good moment to ask someone on https://github.com/microsoft/language-server-protocol before close or merge it.

interesting that the Sublime LSP server has the disable_capabilities feature. I tend to think it should be client side.

I take into account that this client-side solution was implemented to workaround the lack of such feature ;-)

przepompownia · 2022-01-27T09:14:21Z

Please look at microsoft/language-server-protocol#1403

dantleech · 2022-01-27T09:28:38Z

Thanks for that. Given that behavior I would simply not register the handler rather than have the handler decide if it exposes that capability. But, at least in my case, it doesn't help with disabling completion or reference etc.

(have to run now, will have a think)

przepompownia · 2022-01-27T09:45:17Z

Initially I was wondering how to turn off the whole handler but I did not sure if we have one-to-one relation between handler and a corresponding LSP capability.

…-client

dantleech · 2022-02-13T22:31:45Z

Sorry for not getting back on this. I still think that deciding to support/not support a given feature is a client-side decision and should be incorporated there. The client makes the calls. If we were to implement this using the approach here, there would still be no way of f.e. disabling completion, and what's more not all clients will allow the capabilities to be overridden.

Disabling the server side capabilities based on client capabilities in the referenced issue seems more about "not wasting unneeded resources".

I think having Phpactor options to enable/disable a capability would still be beneficial, but I currently don't think that using the client capablities is a satisfactory way of doing that.

przepompownia · 2022-02-15T11:24:58Z

Sorry for not getting back on this.

No problem. While it works I simply use this branch and can focus on other things in the meantime. Independently of your choice and testing other approaches (including your propositions) there is also no problem for me to keep this branch up to date and, possibly, find some time to check how to free unused resources.

If we were to implement this using the approach here, there would still be no way of f.e. disabling completion, and what's more not all clients will allow the capabilities to be overridden.

Lack of user access to restricting capabilities at a particular client side is an issue of such client.

Allow to disable textDocument/signatureHelp and `textDocument/compl…

d3213db

…etion`

przepompownia mentioned this pull request Jan 22, 2022

How to restrict the server capabilities by the client? #56

Open

przepompownia added 5 commits January 23, 2022 00:09

Fix CompletionHandlerTest

c5f4d7b

fixup! Fix CompletionHandlerTest

ec58d09

Fix SignatureHelpHandlerTest

7248124

Avoid cs-fixer deprecations

94c2e59

Add test that should not pass

619ad40

dantleech reviewed Jan 23, 2022

View reviewed changes

przepompownia added 3 commits January 23, 2022 19:00

Allow to disable the rest of method

c34bac9

Merge branch 'allow-disable-selected-capabilities-by-the-client' of p…

06b3755

…rzepompownia:przepompownia/language-server-phpactor-extensions into allow-disable-selected-capabilities-by-the-client

Move duplicated clientCapabilities() to AbstractExtension

5aa58fe

przepompownia commented Jan 23, 2022

View reviewed changes

przepompownia added 10 commits January 23, 2022 20:27

Move $clientCapabilities to AbstractHandler

9a097f3

Fix unit tests

1ce025f

fixup! Fix unit tests

d79cb93

Remove trailig comma

937fdfe

fixup! Remove trailig comma

8e2f6ef

Remove unused imports

5f7fdcb

PSR

a633937

Do not override setting signatureHelpProvider in CompletionHandler

e8433f6

Remove unused import

479e4fa

Add tests for CompletionHandler

da4abe5

przepompownia marked this pull request as ready for review January 23, 2022 21:23

dantleech reviewed Jan 25, 2022

View reviewed changes

Merge branch 'master' into allow-disable-selected-capabilities-by-the…

5fdc1e1

…-client

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to disable selected capabilities by the client #58

Allow to disable selected capabilities by the client #58

przepompownia commented Jan 22, 2022 •

edited

Loading

przepompownia commented Jan 22, 2022

przepompownia commented Jan 22, 2022

przepompownia commented Jan 23, 2022 •

edited

Loading

dantleech left a comment

dantleech Jan 23, 2022

przepompownia Jan 23, 2022

przepompownia Jan 23, 2022

dantleech Jan 25, 2022

dantleech commented Jan 25, 2022

dantleech Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022

dantleech Jan 25, 2022

dantleech Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022 •

edited

Loading

dantleech Jan 25, 2022

przepompownia Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022 •

edited

Loading

dantleech Jan 25, 2022

dantleech Jan 25, 2022

przepompownia Jan 25, 2022

dantleech Jan 25, 2022

przepompownia Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

przepompownia commented Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

przepompownia commented Jan 27, 2022

przepompownia commented Jan 27, 2022

dantleech commented Jan 27, 2022 •

edited

Loading

przepompownia commented Jan 27, 2022

dantleech commented Feb 13, 2022 •

edited

Loading

przepompownia commented Feb 15, 2022 •

edited

Loading

Allow to disable selected capabilities by the client #58

Are you sure you want to change the base?

Allow to disable selected capabilities by the client #58

Conversation

przepompownia commented Jan 22, 2022 • edited Loading

przepompownia commented Jan 22, 2022

przepompownia commented Jan 22, 2022

przepompownia commented Jan 23, 2022 • edited Loading

dantleech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Jan 25, 2022

dantleech Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

przepompownia Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przepompownia Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

przepompownia Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przepompownia Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

dantleech commented Jan 26, 2022 • edited Loading

dantleech commented Jan 26, 2022 • edited Loading

przepompownia commented Jan 26, 2022 • edited Loading

dantleech commented Jan 26, 2022 • edited Loading

przepompownia commented Jan 27, 2022

przepompownia commented Jan 27, 2022

dantleech commented Jan 27, 2022 • edited Loading

przepompownia commented Jan 27, 2022

dantleech commented Feb 13, 2022 • edited Loading

przepompownia commented Feb 15, 2022 • edited Loading

przepompownia commented Jan 22, 2022 •

edited

Loading

przepompownia commented Jan 23, 2022 •

edited

Loading

dantleech Jan 25, 2022 •

edited

Loading

dantleech Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022 •

edited

Loading

przepompownia Jan 25, 2022 •

edited

Loading

przepompownia Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

przepompownia commented Jan 26, 2022 •

edited

Loading

dantleech commented Jan 26, 2022 •

edited

Loading

dantleech commented Jan 27, 2022 •

edited

Loading

dantleech commented Feb 13, 2022 •

edited

Loading

przepompownia commented Feb 15, 2022 •

edited

Loading