Enable full completions #47

tomv564 · 2017-08-25T22:44:47Z

For issue #25, where all completion requests (even manual) were ignored unless the cursor followed a trigger character.

The following improvements were made:

No more filtering on trigger chars (ctrl+space no longer ignored)
Switch to ViewEventListener (easier to disable, less risk with shared state)
Track completion states so we can discard and request new completions as the user keeps typing

To do:

verify only_complete_on_trigger_characters and make it a setting
also disable listener if server has no completionProvider
clean up debug logging
rename to CompletionHandler

rwols · 2017-08-26T09:36:56Z

main.py

-                return
+    @classmethod
+    def is_applicable(cls, settings):
+        syntax = settings.get('syntax')


In rare cases syntax may be None (annoying ST bug), check for this just in case.

In this case the behaviour would be as expected, but I'll add this fix anyways.
Is this documented somewhere?

rwols · 2017-08-26T09:39:13Z

main.py

+    def is_after_trigger_character(self, location):
+        if location > 0:
+            prev_char = self.view.substr(
+                sublime.Region(location - 1, location))


You can use sublime.Region(location - 1) I believe.

You can even call self.view.substr(location - 1) as substr understands both a Region and a Point

rwols · 2017-08-26T09:43:23Z

main.py

+                                                    dict) else response
+            names = list(item.get('label') for item in items)
+            debug('got completions', names)
+            self.completions = list(self.format_completion(item) for item in items)


Will you be doing anything with the sortText property of the CompletionItem or will you let ST sort the results? The clangd server uses sortText to sort completions by priority, so you'll see properties and methods first, base classes somewhere middle, keywords last.

Interesting, is there a way to control what ST does with the completions?

No there is not. ST merges and sorts all items it gets internally.

Not immediately, it just presents that list as-is if the user hasn't typed more characters yet, so there might be some benefit to using sortText.

But it could slow down LSP a lot if there are tons of completion items, you'll have to experiment with it.

For reference: https://github.com/llvm-mirror/clang-tools-extra/blob/c543235c134fbb02fb630eaa53661b188068b267/clangd/ClangdUnit.cpp#L316-L317

@rwols good to know!

rwols · 2017-08-26T09:44:50Z

main.py

-    def __init__(self):
-        self.completions = []  # type: List[Tuple[str, str]]
-        self.refreshing = False
+class CompletionState(object):


I had some trouble seeing "old" completions when I typed fast, I hope this class fixes that.

From some quick tests using the changes in this PR I don't see this problem anymore, nice job!

deathaxe · 2017-08-26T14:28:46Z

main.py

@@ -1605,7 +1605,7 @@ def format_completion(self, item) -> 'Tuple[str, str]':
            insertText = item.get("insertText")
        if insertText[0] == '$':  # sublime needs leading '$' escaped.
            insertText = '\$' + insertText[1:]
-        return ["{}\t{}".format(label, detail), insertText]
+        return "{}\t{}".format(label, detail), insertText


Not sure, whether it's to be handled by LSP or the pyls, but detail returns def function for a function completion, which makes completions look a bit too verbose:

function def function

This would be something for pyls to look at.

tomv564 · 2017-08-26T19:28:09Z

Is anyone interested in giving this branch a test? I'm not 100% convinced it's working correctly.

Example: sublime still shows for snippet completion sometimes when you type f, instead of pyls's completions containing from

rwols · 2017-08-26T21:07:16Z

Is anyone interested in giving this branch a test?

Yes, I'm gonna give this a spin.

EDIT: I don't see any problems at first glance (tested with clangd)

deathaxe · 2017-08-26T22:32:53Z

main.py

-                    sublime.Region(locations[0] - 1, locations[0]))
-                if prev_char not in autocomplete_triggers:
-                    return None
+            elif self.state == CompletionState.REQUESTING or self.state == CompletionState.CANCELLING:


How about: elif self.state in (CompletionState.REQUESTING, CompletionState.CANCELLING)

Hmm. Seems quite easy to get the state machine stalled in CANCELLING state, once a language-server does not respond. From this point I'll never get any completion any more.

Nice code suggestion. Why does the language server not respond?

deathaxe · 2017-08-27T16:44:02Z

Furthermore there seems to be a general issue with requesting completions. Sometimes a race condition between the onChange change notification and the completion request occures. If onChange is sent later then the completion response, python-language-server returns ArgumentError as it does not yet know about the content of the edited line.

On the other hand the onChange triggers diagnostics scan and causes python-language-server to delay completions as it is busy with linting. Especially mypy plugin causes heavy delays.

tomv564 · 2017-08-27T21:22:08Z

@deathaxe thanks for testing!
I too have seen pyls complain about some index being out of range. didChange is always called just before requesting completions, in what case does the language server receive it after the completions request?

In a discussion about the javascript-typescript server's completion performance the idea of processing diagnostics from a queue after a small delay was raised. This is worth proposing for pyls as well.

deathaxe · 2017-08-28T17:22:43Z

I didn't investigate too much, but I think the race condition is related to view.on_modified_async()' handler, which is used to push changes to the language server, while the view.on_query_completions()is a synchronous event. Therefore in some situations the async on_modified handler might be delayed by other operations taking place in the background. So at the point of time, the completion is requested, the queue you try to purge might still be empty. I'd suggest to use synchronous handlers inDocumentSyncListener` as each event handler doesn't take any time to be processed. Some of the operations to read the view content might even take longer due to inter process communication.

the idea of processing diagnostics from a queue after a small delay was raised

I hate delays. They are caused by processing anyway. But creating a multithread or multiprocess architecture with a pool of worker threads/processes might be a good idea for pyls to process multiple requests/notifications in parallel. This might help much as pyls may already be busy with multiple linters run after each other while a completion request is received at the moment.

This requires to categorize the received requests/notifications some how. A new didChange request would need to be queued, but not processed until all pending operations on the last version of the document have finished. A new request such as a completion or signatureHelp might be added to the queue and processed immediately to respond as fast as possible, while diagnostics may still be in process.

This requires at least one dedicated thread/process to handle the synchronization events such as didOpen/didChange/... and one dedicated thread/process to handle requests. A new didChange might would then be queued and being processed as soon as the running requests queue is empty.

deathaxe · 2017-08-28T20:24:12Z

here's a little implementation to get a reference how quick jedi really is without LSP overhead.

# -*- encoding: utf-8 -*-
import os
import sys

import sublime
import sublime_plugin


sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'lib'))
from jedi.api import Script
sys.path.pop(0)


def format_completion(complete):
    """ Returns a tuple of the string that would be visible in
    the completion dialogue and the completion word

    :type complete: jedi.api_classes.Completion
    :rtype: (str, str)
    """
    display, insert = complete.name + '\t' + complete.type, complete.name
    return display, insert


class EventListener(sublime_plugin.EventListener):

    def on_load_async(self, view):
        # just load to trigger interpreter and create cache
        self._script(view, 0)

    def on_query_completions(self, view, prefix, locations):

        point = locations[0]
        if not view.match_selector(point, "source.python"):
            return None

        script = self._script(view, point)
        completions = script.completions()
        return (
            [format_completion(complete) for complete in completions],
            sublime.INHIBIT_WORD_COMPLETIONS
        )

    def _script(self, view, point):
        path = view.file_name() or ''
        region = sublime.Region(0, view.size())
        source = view.substr(region)
        line, column = view.rowcol(point)
        return Script(source, line + 1, column, path=path)

rwols reviewed Aug 26, 2017

View reviewed changes

deathaxe reviewed Aug 26, 2017

View reviewed changes

tomv564 added 4 commits August 26, 2017 21:25

Send all completion requests to language server (WIP)

e704a7a

Recognize completion states, use ViewEventListener

ca03dce

Revert to tuple for completion items

b1db7e7

Add complete_all_chars setting, initialize listener once and early out

e0e7a38

tomv564 force-pushed the enable-full-completions branch from 16decfb to e0e7a38 Compare August 26, 2017 19:25

Always check view syntax as it may be None

e345136

deathaxe reviewed Aug 26, 2017

View reviewed changes

tomv564 added 2 commits August 27, 2017 23:35

Use tuple for valid request states

facb548

Clean up debugging

2fb7d07

tomv564 merged commit ec7c17a into master Aug 28, 2017

tomv564 mentioned this pull request Aug 28, 2017

Release 0.2 #52

Closed

4 tasks

tomv564 deleted the enable-full-completions branch October 14, 2017 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable full completions #47

Enable full completions #47

tomv564 commented Aug 25, 2017 •

edited

Loading

rwols Aug 26, 2017

tomv564 Aug 26, 2017

rwols Aug 26, 2017

deathaxe Aug 26, 2017

rwols Aug 26, 2017 •

edited

Loading

tomv564 Aug 26, 2017

deathaxe Aug 26, 2017

rwols Aug 26, 2017

rwols Aug 26, 2017

rwols Aug 26, 2017

tomv564 Aug 27, 2017

rwols Aug 26, 2017

rwols Aug 26, 2017

deathaxe Aug 26, 2017

tomv564 Aug 26, 2017

tomv564 commented Aug 26, 2017

rwols commented Aug 26, 2017 •

edited

Loading

deathaxe Aug 26, 2017

deathaxe Aug 27, 2017

tomv564 Aug 27, 2017

deathaxe commented Aug 27, 2017

tomv564 commented Aug 27, 2017

deathaxe commented Aug 28, 2017

deathaxe commented Aug 28, 2017

Enable full completions #47

Enable full completions #47

Conversation

tomv564 commented Aug 25, 2017 • 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

rwols Aug 26, 2017 • 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

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

Choose a reason for hiding this comment

tomv564 commented Aug 26, 2017

rwols commented Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deathaxe commented Aug 27, 2017

tomv564 commented Aug 27, 2017

deathaxe commented Aug 28, 2017

deathaxe commented Aug 28, 2017

tomv564 commented Aug 25, 2017 •

edited

Loading

rwols Aug 26, 2017 •

edited

Loading

rwols commented Aug 26, 2017 •

edited

Loading