Decouple completion engine internals

[resolritter]

I will fix documentation and tests after the API changes are discussed and reviewed

Notions

• The completion engine and the Autocmd events should be decoupled.
• Users can override the behavior of what happens to the results.
• Allow for interaction with other plugins, like deoplete (mentioned on Integration with deoplete #1753).

Changes

• The process for polling/queuing activation has been separated from the Omnifunc itself.
• The old ale#completion#Show method called at the end of the completion parsing chain has been repurposed as ale#completion#QueryDone, which calls
  • Overridable ALEProcessCompletionResults for processing the parsed candidates for completion.
  • Overridable ALEAfterCompletionResults for adding flexibility to what happens after the processing is finished.

How to test

Follow the instructions inside of minimal_autocompletion_vimrc.

[w0rp]

Here's my feedback. Have a look at the tests and make sure everything is tested.

Thank you for your work on this. I think a lot of people will appreciate some form of deoplete interaction.

[w0rp]

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

[w0rp]

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.

[resolritter]

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.

[w0rp]

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.

[w0rp]

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.

[resolritter]

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?

[w0rp]

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.

[w0rp]

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.

[w0rp]

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.

[w0rp]

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>.

[resolritter]

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.

[w0rp]

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

[w0rp]

Doesn't deoplete accept autoload functions? Why not?

[w0rp]

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.

[resolritter]

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.

[w0rp]

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.

[w0rp]

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.

[resolritter]

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.

[w0rp]

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.

[w0rp]

As mentioned before, you can remove this.

[resolritter]

@w0rp as with #1889 it seems like we have a lot of different opinions on the internal design of this file. You are able to push commits to this branch too, so can you redesign those changes when you have the time? It will be clear to me if I can adjust your changes to my deoplete plugin, or if it needs some tweaking.

I've pushed the plugin code (it's only this 70 line file) to deoplete_ale.py for you to take a look.

It stops the ALE source when 'completion_done' is set (line 32-33) from a manual event dispatched with

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

[w0rp]

See if you can implement deoplete integration without defining any global functions at all, that aren't autoload functions. I don't want to pollute the global namespace, and nothing should ever replace ALE's functions.

You can push the changes, and I can review them later.

[resolritter]

Finally got some room back to act on this. I'll try to follow up on your inquiries but let me explain why it's set up this way and also comment on your comments.

---

Python code for reference

Line 57 and forward of the Python code reads like this

    context['initial_position'] = self.get_completion_position()

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

When the position is invalid we return right away without setting the ['is_async'] = True flag in the context object. What happens is that at the end of gather_candidates, if this flag is turned off, deoplete won't try to get results from the source again. The idea is to let it keep calling this function until we trigger the manual stop event at line 33.

By default, deoplete polls every source with is_async every Nms until the async flag to false, regardless of it having results or not. This is useful as compared to stopping/restarting timers because you could use those "heartbeats" to process LSP results sparsely and keep adding to deoplete, similar to what you said about

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.

---

On line 53 a check is done for

if 'initial_position' not in context:

Checking if this source has just been initialized.

I thought that instead of repeatedly calling ale#completion#PollCompletionResults and returning its result until the manual trigger is received (relies on s:currently_queued_pos), I could also use this block of code to call dispatch Queue only once, and then not call ALE again until we get the manual trigger back.

The problem with the latter is the lack of coordination; with the current approach, if the user types again halfway through the function, we incidentally call Queue again which will invalidate the results and keep the whole alive and continuing smoothly. Otherwise we'd have to go through, over and over

• Deoplete activation delay
• Deoplete async setup (refreshes every async source every Nms, specified by auto_refresh_delay)
• ALE restarting timers
• ALE completion delay

Plus other potential "unknown problems" of inconsistency between the menu candidates.

Furthermore I don't agree with the notion of "polluting the global namespace" because those functions add more flexibility to use the engine without having to be bound by the behavior of what ALE considers as autocompletion. For instance, you can't call the omnifunction manually as of now and this could more broadly remedy that to some extent.

---

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.

ALEAfterCompletionResults is opening as an avenue for users (in general) to customize what happens after completion results, it could be anything related to any other plugin, like MUComplete which does not support asynchronous calls.

---

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

Doable as shown by OmniSharp-vim, but the path will have to be created at the root of the repo. I agree that it is way better, so that the ALE community can contribute directly to it.

[resolritter]

Like I said I will try to follow on your proposals and see if it clicks with this setup, but it already syncs up decently well after the tweaks I have made. This is how it is currently with

let g:ale_completion_enabled = 0
let g:ale_completion_delay = 200
call deoplete#custom#source('ale', 'min_pattern_length', 0)

Demo

It can be very much improved and fine grained as far as controls over results go using some deoplete filters or with a good strategy for clearing lingering results from previous completions, if any... But it is smooth as of now. I can control how "spammy" it is directly through

let g:ale_completion_delay = X

Changing the behavior could make it more clunky but I don't know that as of now, so I'll use it some more in the next few days and see if I can come up with something more conforming to your requests.

Like I said previously, feel free to re-scheme this however you want and I can adjust the plugin to your changes.

[w0rp]

Defining a global function like that to be replaced is bad design. ALE's function could be loaded after some user-supplied function, so it's difficult to replace. We can't support users replacing particular functions, that will make maintenance very difficult. It's much better to offer a documented setting, like a variable naming a function to call, or an autocmd event.

Could you add the Python part of this to the pull request? I'll look at getting all of this to work with tests for everything, including the Python code.

[resolritter]

@w0rp here you go, I have added the source in respect to the filepath needed by deoplete to load and recognize the plugin. I have also not touched the code in order to not disrupt your previous review.

If you have more questions about deoplete, though I don't know much, I can venture deeper into the source. I appreciate the time you invested in the discussion and the work done in the whole of ALE. : D

[w0rp]

Thanks! I'll have a look when I can. Christmas holiday family time is essentially over now.

[andreypopp]

Would it be possible to provide synchronous completion interface via omnifunction? That would greatly benefit those not using large completion frameworks but just plain omnicompletion.

[resolritter]

@andreypopp that was a smaller goal of this PR. If you look at completion.vim#L149, you'll see that ALE does some trickery to keep the popup window open without interfering with your native completion options, which can be a good thing IMO.

Aside from that, multiple if !g:ale_completion_enabled checks scattered across the file made it so you couldn't have completion without autocompletion by default.

Try to manually disable the autocmd events (ALECompletionGroup) after ale#completion#Enable and see if it clicks for you. Due to the replacement trick I mentioned above, which is embedded directly into the omnifunction, it might not work.

[w0rp]

@andreypopp It would still be asynchronous, but yes, that is possible, and that's a separate problem. This PR is just about making it so ALE can be used as a source for deoplete or something else.

[resolritter]

Kinda related: does TSServer have any issues on providing completions around the dot? I've introduced some variables to debug how the prefix was being sent after some changes and it seemed to be identified correctly (e.g. Array. , Object. , etc...) and the word seemed to be captured correctly, but it still didn't work 100% of the time.

Below recording is from the latest ALE master, without any adulteration, using g:ale_completion_enabled = 1 (not deoplete).

https://asciinema.org/a/sV3tAmgg07MrcX7OWeHeIiO3Q

I can reproduce it kinda easily there by going back-and-forth on the dot after showing the suggestions for the position, but I've felt like this also happened "at random" sometimes, where I had to type the last letter from the preceding word and the dot again for the suggestions to show up.

Obviously I don't suggest that people become too submissive on completing from LSP (they can tune their needs with delay and manual completion when it comes around), but I wish it to always be reliable when needed. I wonder if it also happens for other sources.

Perhaps related to #2041?

[w0rp]

I don't know why that might be happening, but it might be that tsserver just dones't send a response sometimes. That's a separate issue. This pull request is only about decoupling some internals so the code can be used as a Deoplete source, or more.

[w0rp]

I would just like to note that working on deoplete integration with your help is near the top of the list of my priorities for ALE, alongside the unrelated task of working on more asynchronous programming support for ALE fixers and linters that require complicated logic. The next version of ALE will be tagged as 3.0.0 due to a breaking change I have already added for fixers, which doesn't seem to have caused any problems for anyone, as maybe only I used the feature in particular.

For 3.0.0, I would like to accomplish the following for completion.

1. Get support for getting completions via a custom command in.
2. Get support for using ALE as a deoplete source in, complete with unit tests for the Python code.
3. Maybe support a documented omnicomplete function. (I'm not entirely certain this is possible, or a good idea yet.)
4. Ensure the asynchronous completion support that exists already, which I use pretty much every day at work, continues to function properly.

[resolritter]

Cool. I'll recap the aftermath from the previous discussion from my understanding.

ALEProcessCompletionResults

The function that will get all of the results and filter them according to some criteria (e.g. the deoplete filter API, any user defined function to his choosing). It's misplaced in the code because it needs access to the results before ALE caps the count with g:ale_completion_max_suggestions.

ALEAfterCompletionResults

What gets done after the LSP server has handed the candidates over. I could not be interested in what ALE plans to do when that happens, so I want to replace the behavior with my own. In the case of deoplete, it could be:

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

MUComplete, on the other hand, doesn't work asynchronously, so even if the omnifunction could be triggered manually (which is what the plugins does, trigger each completion method manually until one of them has something to show), due to ALE it returning an empty array in the first call, it would simply skip it forever and not check again. A way to get around that could be something like:

function! ALEAfterCompletionResults() abort
    let b:mucomplete_ale_ready = 1
endfunction

From the user's point of view, I think this is needed to differentiate between empty list (not ready) and empty list (I've tried, but got nothing). If you don't want to "pollute the global scope" then it could be two user supplied lambdas like:

let g/b:ale_completion_filter_func = function('something')
let g/b:ale_completion_after_func = function('something')

Then put in if checks in the relevant parts to see if the behavior needs to be overriden.

s:currently_queued_pos

The way I chose to open up the completion querying was through queueing/polling. The consumer will queue the current position and keep asking for results until ALE tells him to stop (ALEAfterCompletionResults); he will do so involuntarily (which is why resetting the timers and sending new requests won't work) using timer_start or something else until he chooses to stop, which, in the case of deoplete, is setting context['is_async'] = False, as I already mentioned above.

The deoplete source will receive the event sent in ALEAfterCompletionResults and set the custom property

context['completion_done'] = True

If we're halfway through the cycle or just starting gather_candidates when this happens, it does not matter since the check happens near the end. If the user has typed something else before the last step, since we called

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

The poll function would have invalidated the previous results just before the check, because candidates got overridden with the empty array returned on line 201; queue knows it's invalid because the position is kept track of in s:currently_queued_pos, and that has changed with user input.

Nevertheless the custom flag ('completion_done') is still set, so it would enter the final if condition and set is_async to false. I think this could be a bug in some situations and the decision to continue should be presented if the position is still valid. We'd need to either:

1. Notify deoplete that the position has changed with another event, set another custom flag, check for that before shutting off.
2. Call get_completion_position() again, as it's done at the start, and check if the position is still valid; that being the case, still invalidate the results for the previous position, but keep going.

---

Overall this is the way I chose to go about making the two coordinate together, but you can come up with your own scheme and see if it works well.

[resolritter]

Get support for using ALE as a deoplete source in, complete with unit tests for the Python code.

It already works to some extent if you want to test it. The setup needed is

call deoplete#custom#option({
... other options
  \ 'sources': {
  \   'javascript': ['ale']
  \ }
\ }

call deoplete#custom#source('ale', 'min_pattern_length', 0)

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

call deoplete#enable()

Granted I've only used it with tsserver. It has the problems I mentioned with the dot trigger but I don't know exactly who's at fault in that case, it could be some bug in there.

Maybe support a documented omnicomplete function. (I'm not entirely certain this is possible, or a good idea yet.)

I think people looking to use the completion capabilities are not interested in the omnicomplete pattern, which is something particular to Vim. If I'm an outside caller I need to know:

• How to trigger the queue
• How to know when they're done
• How to fetch the results when they're done

These need to be as direct as possible.

Ensure the asynchronous completion support that exists already, which I use pretty much every day at work, continues to function properly.

I've included the minimal vimrc with instructions for testing default ALE completion, with as little variable usage as possible, and it's not broken or anything. You're free to see for yourself with test/completion/vimrc/minimal_autocompletion_vimrc.

[resolritter]

I've recently discovered that deoplete doesn't ask every source each second, it instead uses the configurable auto_refresh_delay (which might be disabled in some custom completion setups), so there's still value in manually sending the event when completion is done. I've updated my previous posts to reflect this discovery.

To avoid infinitely polling when something goes awry (e.g. server timeout or error), a counter could be done along those lines:

if 'initial_position' not in context:
    context['time'] = 0
    context['timeout'] = 2000
    context['refresh_time'] = self.vim.call('deoplete#custom#_get_option', 'auto_complete_delay')
    context['initial_position'] = self.get_completion_position()

And during each cycle increment context['time'] by context[refresh_time] until the timeout threshold is reached, in which case disable the is_async flag and return None.

Another option is to catch those exceptions and create another event for triggering a premature stop.

---

If you want to drop both ALEProcessCompletionResults and ALEAfterCompletionResults, an option would be

• Remove ALEProcessCompletionResults completely
• Change ALEAfterCompletionResults into an autocmd event and require the interested listener to know about b:ale_completion_results.

[w0rp]

Do whatever, as long as the following criteria are met.

1. Make sure everything currently supported continues to work.
2. Don't over-complicate the code too much, and don't design any completion functions to be something you can replace.
3. Add unit tests to cover all of the changes in VimL, and unit tests for Python code.

The thread for this PR is way, way too long. Just write some code and don't overthink it.

[resolritter]

@w0rp Oh, I'm sorry, but since you said

Could you add the Python part of this to the pull request? I'll look at getting all of this to work with tests for everything, including the Python code.

I had understood that you would take care of handling this yourself and were already working out some code. After the seemingly unending back-and-forth and the period of inactivity I've given up on the idea of driving this forward myself.

Since then I'm merely providing information that might be useful for the next person (I thought, you) to use it. The only reason why I commented after this week of a downtime is because I thought you might be still trying to figure out the best way to do this, so I tried to chime in more, as there's no hurt to it.

Let me stress that I don't have the will or the availability to go forward with this anymore, as I've already spent time and thought adjusting to not having it. Best of luck and I hope this is understandable.

[w0rp]

Okay, thank you for all of the information. I will work on this eventually, but that could be weeks away from now. I'm slowly working on adding more async support in general, and I'll either finish that first or get part of the way into that and then work on this.

[w0rp]

This will be implemented later.