Send completion request to multiple sessions

[predragnikolic]

resolves sublimelsp/LSP-stylelint#11

Added:

• send completion request to all sessions that has the completion capability
• added LSP protocol types for CompletionList and CompletionItem

Fixed:

• fixed a failing test(that test was failing even before this PR)

I am in the process of testing this, but while I test, here is the code. :)

[rchl]

I expected the tailwind css language server to return a response for the completions, but it returned tailwindcss/getConfiguration.

It's not that it responded to the completion request with that. It just triggered this separate request while handling the completion request. That shouldn't prevent completion result from coming after that. But I can see how failing to handle the custom request can "soft-crash" the server and make it unable to return completions. I think it's a pure server (plugin helper) issue and not really related to this change.

Unless we would want to handle something like that with a timeout but I don't think we should.

[predragnikolic]

I think it's a pure server (plugin helper) issue and not really related to this change.
Unless we would want to handle something like that with a timeout but I don't think we should.

Yes, I would avoid adding a timeout.

[rwols]

IMO this pull request should be split into two pull requests:

1. introduce more protocol types
2. implement "promise.all" for completions

[predragnikolic]

If you want me to split this PR, just confirm and I will split it :)

[rwols]

Please do :)

[predragnikolic]

Ok I split out the PR.

I wont close this PR, because this PR will be responsible for:

2. implement "promise.all" for completions

Another PR will be created after this and will be responsible for:

1. introduce more protocol types

[rwols]

Alright let's try this out

[rwols]

Please resolve the merge conflicts

[rchl]

I'm thinking it would be better to clear the list from _on_all_settled. In theory, with slow-running requests, it could happen that some stale completions would sneak into the latest request results.

[predragnikolic]

In theory, with slow-running requests, it could happen that some stale completions would sneak into the latest request results.

To add on this, here is how the code currently behaves.

If a language server has isIncomplete completions and we:

>> type "a"
... request#1 pending, set LspResolveDocsCommand.completions = []
<< get completions from server, set LspResolveDocsCommand.completions = response1
>> type "b"
... request#2 pending, set LspResolveDocsCommand.completions = []
>> user triggers documentation popup for completion item from request#1, before the response for the request "b" was received.

In that case an index error will be thrown.
In that case moving the clearing logic to _on_all_settled would solve the problem.

I'm thinking it would be better to clear the list from _on_all_settled

But then we would clear the list and then immediately populate it: (why would we clear the list before assigning a new completion, when we can just assign the new completions)

for response, session_name in responses:
   LspResolveDocsCommand.completions = []
   # ... couple of lines later
   LspResolveDocsCommand.completions = response

https://github.com/sublimelsp/LSP/pull/1582/files#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R675

[rchl]

Good point. I thought that with incomplete completions the popup will close on typing the next character but it doesn't.

But on the other hand, with the current code, there could be another issue:

With isIncomplete completions, the user types "a" and then "b". Both requests are started but are slow to respond so when they both resolve, they both append items to the LspResolveDocsCommand.completions list, and triggering documentation could then fail in some cases.

[predragnikolic]

There a couple of approaches that came to my mind:

a) never clear the completions. (currently we only clear them when we trigger the AC)
LspResolveDocsCommand.completions = []

b) pass the completion item instead of the index: (feels like the a right approach, but performance might be affected)

- st_details += make_command_link("lsp_resolve_docs", "More", {"index": index, "session_name": session_name})
+ st_details += make_command_link("lsp_resolve_docs", "More", {"completion_item": completion_item, "session_name": session_name})

[predragnikolic]

I would choose a) because it is simpler and not that different from the current behaviuor.

[rchl]

a) never clear the completions.

How can this work if we never clear them?

[predragnikolic]

We assign the response to LspResolveDocsCommand.completions (keep in mind that I no longer use LspResolveDocsCommand.completions.entend(response_items) like I used to. Instead I just use = as this will prevent the list from growing to infinity)

LspResolveDocsCommand.completions[session_name] = response_items

01a9813#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R675

When we want to resolve the completion item,
we will look up the completion item in the LspResolveDocsCommand.completions

    def run(self, edit: sublime.Edit, index: int, session_name: str, event: Optional[dict] = None) -> None:
        item = self.completions[session_name][index]

01a9813#diff-1e31f37d39e534dca2dca7fd81d29d7e5a039a4b55a5765fdb4292c7a87760aaR21

That is it.

There is no need to clear the LspResolveDocsCommand.completions = {} (changed to dict, previously was an array)

We can just delete this line, to fix the index error that would be thrown.
01a9813#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R643

[rchl]

Another day, maybe I lost some context already, but clearing from _on_query_completions_async seems wrong because of this case:

(isIncomplete == true)

1. A completion request is completed and a completion popup is shown.
2. User types another letter - LspResolveDocsCommand.completions is cleared immediately
3. The previous request is slow to respond and the user tries to invoke documentation on some existing item - an error because LspResolveDocsCommand.completions is empty.

I think after the change to a dict it makes even more sense to clear it from the _on_all_settled because new completion request might not trigger for the same session names so at least from the memory/GC point of view it would make sense to clear that object at some point.

[predragnikolic]

I moved it to _on_all_settled to avoid errors :)

it would make sense to clear that object at some point

I guess the "ideal" place to clear the dict would be when the AC closes(either by the user selecting the completion item, or by pressing the esc key, or by pressing left right arrows). I know that there is a is_auto_complete_visible() api, but I don't yet have a simple solution for that approach(so I would avoid complicating the logic now).

But I believe that moving it to _on_all_settled improves things a bit.

[rchl]

I've noticed that there is an empty popup shown for a brief moment now when resolving documentation.

EDIT: Looks like missing return in the resolving logic.