Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send completion request to multiple sessions #1582

Merged
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
cfecfdc
send completion request to multiple sessions
predragnikolic Feb 13, 2021
ebd124a
must call the function to return the promise :)
predragnikolic Feb 13, 2021
20dd3e7
add types for CompletionItem and CompletionList
predragnikolic Feb 13, 2021
633d9ca
changes needed to fix LspResolveDocsCommand.completions
predragnikolic Feb 14, 2021
66d0666
mypy and flake fixes
predragnikolic Feb 14, 2021
6c6837d
fix failing test - I expect the popup to not be visible if there are …
predragnikolic Feb 14, 2021
9074230
fix flake
predragnikolic Feb 14, 2021
08c4f3d
not all keys are required when constructing a CompletionItem
predragnikolic Feb 15, 2021
f8dc0ea
add markup content
predragnikolic Feb 15, 2021
c1d1948
separate lines to be consistent
predragnikolic Feb 15, 2021
14c0e41
resolve earlier
predragnikolic Feb 16, 2021
c0a93c7
move sessions, and session_by_name methods to functions for easier re…
predragnikolic Feb 16, 2021
64e3e29
use session_name to get the session
predragnikolic Feb 16, 2021
adda5e4
Merge branch 'st4000-exploration' into send_completion_request_to_mul…
predragnikolic Feb 16, 2021
4432bc9
remove promise if statement
predragnikolic Feb 16, 2021
f4ba032
make mypy happy
predragnikolic Feb 16, 2021
1f608ec
remove global session_by_name function and add it as a method to Docu…
predragnikolic Feb 20, 2021
0183d43
fix pyflake
predragnikolic Feb 20, 2021
d19284c
use send_request_task
predragnikolic Feb 20, 2021
8f0d43e
remove types from protocol, but leave some
predragnikolic Feb 20, 2021
fa61e6a
remove unused import
predragnikolic Feb 20, 2021
369c895
fix import order
predragnikolic Feb 20, 2021
d2deb69
remove empty lines
predragnikolic Feb 20, 2021
04345e2
omit "[" and "]"
predragnikolic Feb 20, 2021
a6f7189
fix possible error message overlaping, instead show all error messages
predragnikolic Feb 20, 2021
4281463
Merge branch 'st4000-exploration' into send_completion_request_to_mul…
predragnikolic Feb 20, 2021
60b8869
fix: mutability bug
predragnikolic Feb 24, 2021
01a9813
save recieved completions in a dict grouped by session name, instead …
predrag-codetribe Mar 2, 2021
934db7a
fix flake
predrag-codetribe Mar 2, 2021
64a60e8
move the clearing of completions to on_all_settled
predrag-codetribe Mar 3, 2021
d6897e7
Merge branch 'st4000-exploration' into send_completion_request_to_mul…
rwols Mar 7, 2021
1a02f82
Account for view-specific capabilities
rwols Mar 7, 2021
4f0d591
Forgot a return statement
rwols Mar 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions plugin/completion.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import webbrowser
from .core.logging import debug
from .core.edit import parse_text_edit
from .core.protocol import Request, InsertTextFormat, Range
from .core.protocol import Request, InsertTextFormat, Range, CompletionItem
from .core.registry import LspTextCommand
from .core.typing import Any, List, Dict, Optional, Generator, Union
from .core.views import FORMAT_STRING, FORMAT_MARKUP_CONTENT, minihtml
Expand All @@ -14,9 +14,9 @@

class LspResolveDocsCommand(LspTextCommand):

completions = [] # type: List[Dict[str, Any]]
completions = [] # type: List[CompletionItem]

def run(self, edit: sublime.Edit, index: int, event: Optional[dict] = None) -> None:
def run(self, edit: sublime.Edit, index: int, session_name: str, event: Optional[dict] = None) -> None:
item = self.completions[index]
detail = self.format_documentation(item.get('detail') or "")
documentation = self.format_documentation(item.get("documentation") or "")
Expand All @@ -25,8 +25,8 @@ def run(self, edit: sublime.Edit, index: int, event: Optional[dict] = None) -> N
if not detail or not documentation:
# To make sure that the detail or documentation fields doesn't exist we need to resove the completion item.
# If those fields appear after the item is resolved we show them in the popup.
session = self.best_session('completionProvider.resolveProvider')
if session:
session = self.session_by_name(session_name)
if session and session.has_capability('completionProvider.resolveProvider'):
rwols marked this conversation as resolved.
Show resolved Hide resolved
session.send_request(Request.resolveCompletionItem(item, self.view), self.handle_resolve_response)
return
minihtml_content = self.get_content(documentation, detail)
Expand Down
9 changes: 7 additions & 2 deletions plugin/core/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,11 @@ class SignatureHelpTriggerKind:
'targetSelectionRange': Dict[str, Any]
}, total=False)


DiagnosticRelatedInformation = TypedDict('DiagnosticRelatedInformation', {
'location': Location,
'message': str
}, total=False)


Diagnostic = TypedDict('Diagnostic', {
'range': RangeLsp,
'severity': int,
Expand All @@ -169,6 +167,13 @@ class SignatureHelpTriggerKind:
'relatedInformation': List[DiagnosticRelatedInformation]
}, total=False)

CompletionItem = Dict[str, Any]

CompletionList = TypedDict('CompletionList', {
'isIncomplete': bool,
'items': List[CompletionItem],
}, total=True)


class Request:

Expand Down
7 changes: 4 additions & 3 deletions plugin/core/views.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from .css import css as lsp_css
from .protocol import CompletionItem
from .protocol import CompletionItemTag
from .protocol import Diagnostic
from .protocol import DiagnosticRelatedInformation
Expand Down Expand Up @@ -672,7 +673,7 @@ def format_diagnostic_for_html(view: sublime.View, diagnostic: Diagnostic, base_
return "".join(formatted)


def _is_completion_item_deprecated(item: dict) -> bool:
def _is_completion_item_deprecated(item: CompletionItem) -> bool:
if item.get("deprecated", False):
return True
tags = item.get("tags")
Expand All @@ -682,7 +683,7 @@ def _is_completion_item_deprecated(item: dict) -> bool:


def format_completion(
item: dict, index: int, can_resolve_completion_items: bool, session_name: str
item: CompletionItem, index: int, can_resolve_completion_items: bool, session_name: str
) -> sublime.CompletionItem:
# This is a hot function. Don't do heavy computations or IO in this function.
item_kind = item.get("kind")
Expand All @@ -707,7 +708,7 @@ def format_completion(

st_details = ""
if can_resolve_completion_items or item.get("documentation"):
st_details += make_command_link("lsp_resolve_docs", "More", {"index": index})
st_details += make_command_link("lsp_resolve_docs", "More", {"index": index, "session_name": session_name})
st_details += " | " if lsp_detail else ""

st_details += "<p>{}</p>".format(lsp_detail)
Expand Down
97 changes: 65 additions & 32 deletions plugin/documents.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
from .code_actions import CodeActionsByConfigName
from .completion import LspResolveDocsCommand
from .core.logging import debug
from .core.promise import Promise
from .core.protocol import CodeLens
from .core.protocol import Command
from .core.protocol import CompletionItem
from .core.protocol import CompletionList
from .core.protocol import Diagnostic
from .core.protocol import DocumentHighlightKind
from .core.protocol import Error
from .core.protocol import Notification
from .core.protocol import Range
from .core.protocol import Request
Expand Down Expand Up @@ -52,7 +56,12 @@
DocumentHighlightKind.Write: "write"
rwols marked this conversation as resolved.
Show resolved Hide resolved
}

ResolveCompletionsFn = Callable[[List[sublime.CompletionItem], int], None]
Flags = int
ResolveCompletionsFn = Callable[[List[sublime.CompletionItem], Flags], None]

SessionName = str
CompletionResponse = Union[List[CompletionItem], CompletionList, None]
ResolvedCompletions = Tuple[Union[CompletionResponse, Error], SessionName]


def is_regular_view(v: sublime.View) -> bool:
Expand Down Expand Up @@ -606,46 +615,64 @@ def render_highlights_on_main_thread() -> None:

# --- textDocument/complete ----------------------------------------------------------------------------------------

def _on_query_completions_async(self, resolve: ResolveCompletionsFn, location: int) -> None:
session = self.session('completionProvider', location)
if not session:
resolve([], 0)
def _on_query_completions_async(self, resolve_completion_list: ResolveCompletionsFn, location: int) -> None:
sessions = self.sessions('completionProvider')
if not sessions:
resolve_completion_list([], 0)
return
self.purge_changes_async()
can_resolve_completion_items = bool(session.get_capability('completionProvider.resolveProvider'))
config_name = session.config.name
session.send_request_async(
Request.complete(text_document_position_params(self.view, location), self.view),
lambda res: self._on_complete_result(res, resolve, can_resolve_completion_items, config_name),
lambda res: self._on_complete_error(res, resolve))

def _on_complete_result(self, response: Optional[Union[dict, List]], resolve: ResolveCompletionsFn,
can_resolve_completion_items: bool, session_name: str) -> None:
response_items = [] # type: List[Dict]
flags = 0
completion_promises = [] # type: List[Promise[ResolvedCompletions]]
for session in sessions:

def completion_request() -> Promise[ResolvedCompletions]:
return session.send_request_task(
Request.complete(text_document_position_params(self.view, location), self.view)
).then(lambda response: (response, session.config.name))

completion_promises.append(completion_request())
LspResolveDocsCommand.completions = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@predragnikolic predragnikolic Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@predragnikolic predragnikolic Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) never clear the completions.

How can this work if we never clear them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Promise.all(completion_promises).then(
lambda responses: self._on_all_settled(responses, resolve_completion_list))

def _on_all_settled(
self,
responses: List[ResolvedCompletions],
resolve_completion_list: ResolveCompletionsFn
) -> None:
items = [] # type: List[sublime.CompletionItem]
errors = [] # type: List[Error]
flags = 0 # int
prefs = userprefs()
if prefs.inhibit_snippet_completions:
flags |= sublime.INHIBIT_EXPLICIT_COMPLETIONS
if prefs.inhibit_word_completions:
flags |= sublime.INHIBIT_WORD_COMPLETIONS
if isinstance(response, dict):
response_items = response["items"] or []
if response.get("isIncomplete", False):
flags |= sublime.DYNAMIC_COMPLETIONS
elif isinstance(response, list):
response_items = response
response_items = sorted(response_items, key=lambda item: item.get("sortText") or item["label"])
LspResolveDocsCommand.completions = response_items
items = [format_completion(response_item, index, can_resolve_completion_items, session_name)
for index, response_item in enumerate(response_items)]
for response, session_name in responses:
if isinstance(response, Error):
errors.append(response)
continue
session = self.session_by_name(session_name)
if not session:
continue
response_items = [] # type: List[CompletionItem]
if isinstance(response, dict):
response_items = response["items"] or []
if response.get("isIncomplete", False):
flags |= sublime.DYNAMIC_COMPLETIONS
elif isinstance(response, list):
response_items = response
response_items = sorted(response_items, key=lambda item: item.get("sortText") or item["label"])
rwols marked this conversation as resolved.
Show resolved Hide resolved
LspResolveDocsCommand.completions.extend(response_items)
can_resolve_completion_items = session.has_capability('completionProvider.resolveProvider')
items.extend(
format_completion(response_item, len(items) + index, can_resolve_completion_items, session.config.name)
for index, response_item in enumerate(response_items))
if items:
flags |= sublime.INHIBIT_REORDER
resolve(items, flags)

def _on_complete_error(self, error: dict, resolve: ResolveCompletionsFn) -> None:
resolve([], 0)
LspResolveDocsCommand.completions = []
sublime.status_message('Completion error: ' + str(error.get('message')))
if errors:
error_messages = ", ".join(str(error) for error in errors)
sublime.status_message('Completion error: {}'.format(error_messages))
resolve_completion_list(items, flags)

# --- Public utility methods ---------------------------------------------------------------------------------------

Expand All @@ -665,6 +692,12 @@ def sessions(self, capability: Optional[str]) -> Generator[Session, None, None]:
def session(self, capability: str, point: Optional[int] = None) -> Optional[Session]:
return best_session(self.view, self.sessions(capability), point)

def session_by_name(self, name: Optional[str] = None) -> Optional[Session]:
for sb in self.session_buffers_async():
if sb.session.config.name == name:
return sb.session
return None

def get_capability_async(self, session: Session, capability_path: str) -> Optional[Any]:
for sv in self.session_views_async():
if sv.session == session:
Expand Down
2 changes: 1 addition & 1 deletion tests/test_completion.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def verify(self, *, completion_items: List[Dict[str, Any]], insert_text: str, ex
def test_none(self) -> 'Generator':
self.set_response("textDocument/completion", None)
self.view.run_command('auto_complete')
yield lambda: self.view.is_auto_complete_visible()
yield lambda: self.view.is_auto_complete_visible() is False
rwols marked this conversation as resolved.
Show resolved Hide resolved

def test_simple_label(self) -> 'Generator':
yield from self.verify(
Expand Down