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

Decouple completion engine internals #2143

Closed
wants to merge 10 commits into from
124 changes: 72 additions & 52 deletions autoload/ale/completion.vim
Expand Up @@ -17,7 +17,9 @@ let g:ale_completion_excluded_words = get(g:, 'ale_completion_excluded_words', [
let g:ale_completion_max_suggestions = get(g:, 'ale_completion_max_suggestions', 50)

let s:timer_id = -1
let s:timer_pos = []
let s:last_done_pos = []
let s:currently_queued_pos = []
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned before, you can remove this.


" CompletionItemKind values from the LSP protocol.
let s:LSP_COMPLETION_TEXT_KIND = 1
Expand Down Expand Up @@ -183,52 +185,71 @@ function! ale#completion#RestoreCompletionOptions() abort
endif
endfunction

function! ale#completion#FindCompletionStart() abort
let [l:line, l:column] = getcurpos()[1:2]
let l:regex = s:GetFiletypeValue(s:omni_start_map, &filetype)
let l:up_to_column = getline(l:line)[: l:column - 2]
let l:match = matchstr(l:up_to_column, l:regex)

let b:ale_completion_start = l:column - len(l:match) - 1

return b:ale_completion_start
endfunction

function! ale#completion#PollCompletionResults() abort
if ale#completion#Queue()
return []
else
return get(b:, 'ale_completion_result', [])
endif
endfunction

Copy link
Member

Choose a reason for hiding this comment

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

I think you can pretty much undo the changes here. The code is basically the same, and the changes don't really improve on what was there. The previous behavior of looking at the line and column as requested instead of the current cursor position may be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this referring to the whole block of changes above? ale#completion#PollCompletionResults and ale#completion#FindCompletionStart

Those were exactly what was before inside of ale#completion#OmniFunc, only turned into functions because external sources don't use the flow of omnifunctions to command their behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thank you for clarifying. That makes sense.

Remember to add tests for these function names.

Let's add the deoplete source file to ALE's repo itself.

function! ale#completion#OmniFunc(findstart, base) abort
if a:findstart
let l:line = b:ale_completion_info.line
let l:column = b:ale_completion_info.column
let l:regex = s:GetFiletypeValue(s:omni_start_map, &filetype)
let l:up_to_column = getline(l:line)[: l:column - 2]
let l:match = matchstr(l:up_to_column, l:regex)

return l:column - len(l:match) - 1
return ale#completion#FindCompletionStart()
else
" Parse a new response if there is one.
if exists('b:ale_completion_response')
\&& exists('b:ale_completion_parser')
let l:response = b:ale_completion_response
let l:parser = b:ale_completion_parser
return ale#completion#PollCompletionResults()
endif
endfunction

unlet b:ale_completion_response
unlet b:ale_completion_parser
function! ale#completion#QueryDone() abort
let s:last_done_pos = getcurpos()[1:2]

let b:ale_completion_result = function(l:parser)(l:response)
endif
let b:ale_completion_result = ALEProcessCompletionResults(get(b:, 'ale_completion_result', []))

call s:ReplaceCompletionOptions()
call ALEAfterCompletionResults()
endfunction

return get(b:, 'ale_completion_result', [])
endif
function! ALEProcessCompletionResults(completion_results) abort
" Users may override this function to act on the results however they
" see fit. Needs to be not namespaced.
return a:completion_results
endfunction

function! ale#completion#Show(response, completion_parser) abort
if ale#util#Mode() isnot# 'i'
function! ALEAfterCompletionResults() abort
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't deoplete accept autoload functions? Why not?

Copy link
Member

Choose a reason for hiding this comment

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

Does deoplete need to use this function at all? This function opens up the completion menu itself, so I'd imagine it's only useful when you aren't using deoplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a result is parsed into menu candidates, after that, the caller needs a way to know that the results are ready to be used on their end. For deoplete I've overrode this function with:

function! ALEAfterCompletionResults() abort
    call deoplete#send_event('CompleteDone', 'ale')
endfunction

For other completion plugins, people can override this "call be when it's done callback" with something else, hence the name _After_CompletionResults.

Copy link
Member

Choose a reason for hiding this comment

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

Replacing an existing function like that is bad design, and it doesn't make sense. Instead of doing that, let's add the deoplete source to ALE, and we don't need to call the function for opening the omnicomplete menu when ALE completion integration with LSP is being used as a completion source.

" Users may override this function to act on the results however they
" see fit. Needs to be not namespaced.
if !ale#util#Mode() is# 'i'
return
endif

" Set the list in the buffer, temporarily replace omnifunc with our
" function, and then start omni-completion.
let b:ale_completion_response = a:response
let b:ale_completion_parser = a:completion_parser
" Replace completion options shortly before opening the menu.
call s:ReplaceCompletionOptions()

call timer_start(0, {-> ale#util#FeedKeys("\<Plug>(ale_show_completion_menu)")})
" If the completion events are not managed by ALE, don't mess with
" the omnifunc option. Otherwise, replace completion options shortly
" before opening the menu.
if g:ale_completion_enabled
call s:ReplaceCompletionOptions()
call timer_start(0, {-> ale#util#FeedKeys("\<Plug>(ale_show_completion_menu)")})
endif
endfunction

function! s:CompletionStillValid(request_id) abort
let [l:line, l:column] = getcurpos()[1:2]

return has_key(b:, 'ale_completion_info')
\&& b:ale_completion_info.request_id == a:request_id
\&& b:ale_completion_info.line == l:line
\&& b:ale_completion_info.column == l:column
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove the return statement below. Something will have to be done about checking that the mode is 'i' still, for insert mode. Make sure we still check that the mode is 'i' (Using the Mode wrapper function so we can test it.) before using feedkeys() with <C-x><C-o>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a mistake that there are two return statements,

but the name CompletionStillValid speaks to me that it checks if completion is still valid, not if Vim is still in insert mode or if ALE is supposed to show this in the screen. The outside source shouldn't be bound by this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Remove the return statement below this one, as I said.


return ale#util#Mode() is# 'i'
\&& has_key(b:, 'ale_completion_info')
\&& b:ale_completion_info.request_id == a:request_id
Expand Down Expand Up @@ -418,10 +439,9 @@ function! ale#completion#HandleTSServerResponse(conn_id, response) abort
\)
endif
elseif l:command is# 'completionEntryDetails'
call ale#completion#Show(
\ a:response,
\ 'ale#completion#ParseTSServerCompletionEntryDetails',
\)
let b:ale_completion_result = ale#completion#ParseTSServerCompletionEntryDetails(a:response)
Copy link
Member

Choose a reason for hiding this comment

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

I had set it up so parsing of results was deferred until just before the menu is shown, to spread out the processing between frames a bit, but I think it should be fine to just parse the results right away.


call ale#completion#QueryDone()
endif
endfunction

Expand All @@ -431,10 +451,9 @@ function! ale#completion#HandleLSPResponse(conn_id, response) abort
return
endif

call ale#completion#Show(
\ a:response,
\ 'ale#completion#ParseLSPCompletions',
\)
let b:ale_completion_result = ale#completion#ParseLSPCompletions(a:response)

call ale#completion#QueryDone()
endfunction

function! s:OnReady(linter, lsp_details, ...) abort
Expand Down Expand Up @@ -505,10 +524,6 @@ function! s:GetLSPCompletions(linter) abort
endfunction

function! ale#completion#GetCompletions() abort
if !g:ale_completion_enabled
return
endif
Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably okay to remove this check. Completion results could still be retrieved if you insert some text, set the option to 0, and then the timer expires, but that shouldn't matter.


let [l:line, l:column] = getcurpos()[1:2]

let l:prefix = ale#completion#GetPrefix(&filetype, l:line, l:column)
Expand Down Expand Up @@ -557,18 +572,17 @@ function! ale#completion#StopTimer() abort
endfunction

function! ale#completion#Queue() abort
if !g:ale_completion_enabled
return
endif
Copy link
Member

Choose a reason for hiding this comment

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

You should undo the changes here. If you need to use what this function does for deoplete, you should create a second function called by this one. Maybe rename this function to OnTextChanged or similar, and name the new one Queue. The !g:ale_completion_enabled check is important for disabling completion if someone sets the flag to 0, but doesn't call the function to remove the autocmd event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't the person know that ale#completion#Disable() is the right way to do this? TextChangedI is a costly autocmd to trigger if it's not being used.

Copy link
Member

Choose a reason for hiding this comment

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

They should, but I'd leave the check in here anyway. It doesn't cost much, and it will be useful later if we add b:ale_completion_enabled.


let s:timer_pos = getcurpos()[1:2]

if s:timer_pos == s:last_done_pos
if s:timer_pos == s:last_done_pos || s:timer_pos == s:currently_queued_pos
" Do not ask for completions if the cursor rests on the position we
" last completed on.
return
" last completed on, or if the position is the one currently being
" queried by the engine.
return 0
endif

let s:currently_queued_pos = s:timer_pos
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a need to set this variable and check it. The timer is already being stopped and restarted, and this could cause bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Outside source asks for completion and it's already there (s:last_done_pos)
  2. Outside source asks for completion and it's not done yet, but it has been dispatched for this position so there's no need to call this function again (s:currently_queued_pos)

How do I signal situation two without this?

Copy link
Member

Choose a reason for hiding this comment

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

You should remove the s:currently_queued_pos disabled check, and let the code run through to stopping and restarting the timer. The timer should be reset from when the last request was made, say if the delay is 200ms, and you make two requests at 0ms and 100ms, then the timer function should be called at 300ms.


" If we changed the text again while we're still waiting for a response,
" then invalidate the requests before the timer ticks again.
if exists('b:ale_completion_info')
Expand All @@ -578,23 +592,29 @@ function! ale#completion#Queue() abort
call ale#completion#StopTimer()

let s:timer_id = timer_start(g:ale_completion_delay, function('s:TimerHandler'))

return 1
endfunction

function! ale#completion#Done() abort
function! ale#completion#Finished() abort
Copy link
Member

Choose a reason for hiding this comment

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

I think the old name was better. This is the function for handling CompleteDone, so it's named Done.

silent! pclose

call ale#completion#RestoreCompletionOptions()

let s:last_done_pos = getcurpos()[1:2]
endfunction

function! ale#completion#ClearBufferResults() abort
let b:ale_completion_result = []
endfunction

function! s:Setup(enabled) abort
augroup ALECompletionGroup
autocmd!

if a:enabled
autocmd TextChangedI * call ale#completion#Queue()
autocmd CompleteDone * call ale#completion#Done()
autocmd CompleteDone * call ale#completion#Finished()
endif
augroup END

Expand All @@ -605,10 +625,10 @@ endfunction

function! ale#completion#Enable() abort
let g:ale_completion_enabled = 1
call s:Setup(1)
call s:Setup(g:ale_completion_enabled)
endfunction

function! ale#completion#Disable() abort
let g:ale_completion_enabled = 0
call s:Setup(0)
call s:Setup(g:ale_completion_enabled)
endfunction
2 changes: 1 addition & 1 deletion plugin/ale.vim
Expand Up @@ -133,7 +133,7 @@ let g:ale_history_enabled = get(g:, 'ale_history_enabled', 1)
" A flag for storing the full output of commands in the history.
let g:ale_history_log_output = get(g:, 'ale_history_log_output', 1)

" Enable automatic completion with LSP servers and tsserver
" Enable automatic completion events with LSP servers and tsserver
let g:ale_completion_enabled = get(g:, 'ale_completion_enabled', 0)

" Enable automatic detection of pipenv for Python linters.
Expand Down
73 changes: 73 additions & 0 deletions rplugin/python3/deoplete/sources/deoplete_ale.py
@@ -0,0 +1,73 @@
# ============================================================================
# FILE: omni.py
# AUTHOR: Shougo Matsushita <Shougo.Matsu at gmail.com>
# License: MIT license
# ============================================================================

from deoplete.source.base import (
Base
)
from deoplete.util import (
convert2candidates
)


class Source(Base):

def __init__(self, vim):
super().__init__(vim)

self.name = 'ale'
self.mark = '[L]'
self.rank = 100
self.is_bytepos = True
self.min_pattern_length = 0
self.events = ['CompleteDone']

self.poll_func = 'ale#completion#PollCompletionResults'
self.position_func = 'ale#completion#FindCompletionStart'
self.reset_func = 'ale#completion#ClearBufferResults'

def on_event(self, context):
if context['event'] == 'CompleteDone':
context['completion_done'] = True

def get_completion_position(self):
return self.vim.call(self.position_func)

def patch_candidates(self, candidates):
if isinstance(candidates, dict):
candidates = candidates['words']
elif not isinstance(candidates, list):
candidates = convert2candidates(candidates)

for c in candidates:
c['dup'] = 1

return candidates

def gather_candidates(self, context):
candidates = []

try:
if 'initial_position' not in context:
self.vim.call(self.reset_func)
context['initial_position'] = self.get_completion_position()

if context['initial_position'] < 0:
return []
else:
context['is_async'] = True

candidates = self.vim.call(self.poll_func)

if 'completion_done' in context or candidates:
context['is_async'] = False
self.vim.call(self.reset_func)
candidates = self.patch_candidates(candidates)

# self.vim.command('echo localtime()')
return candidates
except Exception:
self.print_error('Error occurred calling ALE')
return None
17 changes: 17 additions & 0 deletions test/completion/vimrc/minimal_autocompletion_vimrc
@@ -0,0 +1,17 @@
" For testing ALE autocompletion with TSServer.
" You need to include ALE at $VIMRUNTIME/pack/completion/opt/ale

" Launch with VIMRUNTIME=[dummy directory] vim -u [path to this file] --noplugin new.js

" Example: VIMRUNTIME=~/vim/mock vim -u ~/vim/mock/pack/completion/opt/ale/test/completion/vimrc/minimal_autocompletion_vimrc --noplugin new.js

set nocompatible
filetype on
set filetype=javascript

" Should enable autocompletion automatically
let g:ale_completion_enabled = 1
" Set up TSServer to enable completion
let g:ale_linters = { 'javascript': ['tsserver'] }

packadd ale