Add options to filter candidate returned from server.

tigersoldier commented 5 years ago

A new custom option company-lsp-filter-candidates is added to allow customizing whether or not to filter candidates when company-lsp-cache-candidates is set to nil. The option can be either set to a boolean, or an alist for configuring it on a per-server basis.

When company-lsp-cache-candidates is set to non-nil values, this option doesn't affect the filtering behavior since candidates are always filtered in this case.

This should fix #79.

yyoncho commented 5 years ago

Afaik company itself could do the filtering if you return 'no-cache nil. It seems to me that In lsp-mode case we need to return nil when the server has returned isIncomplete=false which pretty much means "you could do the rest of the filtering client side" (cc @dgutov)

dgutov commented 5 years ago

company itself could do the filtering if you return 'no-cache nil

Only if you do prefix matching. Meaning, fuzzy matching won't work this way.

tigersoldier commented 5 years ago

We do return 'no-cache nil when company-lsp-cache-candidates is set to t.

It's unclear how to handle incomplete candidates here. We shouldn't let company to cache incomplete candidates here. @dgutov How does company behave if for prefix="foo" no-cache is t and for prefix="foob" no-cache is nil? When user deletes the trailing "b" from the prefix, does company use the cache or not?

stardiviner commented 5 years ago

I tried out your PR, confirmed it works. I didn't set any option like company-lsp-cache-candidates etc. Thanks for your work!!!

yyoncho commented 5 years ago

We do return 'no-cache nil when company-lsp-cache-candidates is set to t.

IMO we should do it only if isIncomplete is nil since the server might have returned partial results.

From what I can see, VScode handles isIncomplete the way I have described it in the previous comment and so it makes sense to have this as a default behaviour for all of the servers(having company-lsp-filter-candidates = t). This approach has also a performance benefit. You may ignore my concern about company-lsp doing the filtering since it seems like it doesn't matter much whether the filtering is performed in company or company-lsp(if it is in company-lsp we may implement flx matching as per your comment).

Example(using vscode and jdtls). The first screenshot is if I have invoked manually the completion, the second one is if I have typed .ode

String s = new String();
s.ode

selection_105 selection_107

dgutov commented 5 years ago

How does company behave if for prefix="foo" no-cache is t and for prefix="foob" no-cache is nil? When user deletes the trailing "b" from the prefix, does company use the cache or not?

It does. When the cache is cleared, it uses the previous value of prefix.

But you should probably do just do all cache management in the backend. The logic will be easier to follow, for one thing.

tigersoldier commented 5 years ago

@yyoncho If you set company-lsp-cache-candidates to 'auto it does mostly what you described. The difference is that company-lsp doesn't set no-cache to nil. Caching and filtering is done on company-lsp side. You can try it out.

Now, let me describe why the other two options, t and nil, exists:

t (always use cached candidates regardless the value of inComplete) isn't necessary needed. It exists there as a fallback because currently 'auto has some bug in invalidating caches.
nil (always send request to server regardless the value of inComplete) is useful for servers that have more sophisticated filtering and sorting logics. For example CCLS filters candidates by partial matching, while company-lsp only implemented prefix matching. By setting company-lsp-cache-candidates to nil users can disable client-side filtering and enjoy better server-side filtering.

'auto should be the default option for most servers. The current default is nil because 'auto has cache invalidation bug. I'm planning to make company-lsp-cache-candidates a per-server configuration as what I did for company-lsp-filter-candidates in this PR.

tigersoldier commented 5 years ago

But you should probably do just do all cache management in the backend. The logic will be easier to follow, for one thing.

@dgutov What's the best practice in invalidating caches? I'm listening to company-completion-cancelled-hook and company-completion-finished-hook when populating the cache:

https://github.com/tigersoldier/company-lsp/blob/4eb6949f19892be7bf682381cde005791a48583a/company-lsp.el#L311-L313

And invalidating and removing when the hooks are called:

https://github.com/tigersoldier/company-lsp/blob/4eb6949f19892be7bf682381cde005791a48583a/company-lsp.el#L320-L324

However from time to time the caches are not invalidated while the clean up function is removed from the hooks. I can always listen to the hooks and not remove them when invalidating caches. This will cause the function be invoked when other backends are used though. I wonder if there are better way to trigger cleanup function only when the company-lsp backend is used and finished?

yyoncho commented 5 years ago

@tigersoldier I am rooting for caching since it looks slicker when e. g. there is no flickering, it is instant. What you are saying make sense though. I guess once you implement flx matching we may rely more on client-side filtering, right? For me this seems to be the best solution, what do you think?

tigersoldier commented 5 years ago

@yyoncho I do think auto caching provides best out-of-box user experience, even without flx matching (which may not come any time soon). Just need to fix the annoying bug before making it the default option.

Before that, let's have this work around submitted for the current default, no caching.

dgutov commented 5 years ago

@tigersoldier

I'm listening to company-completion-cancelled-hook and company-completion-finished-hook

Starting with company-mode 0.9.9, you can replace them with company-after-completion-hook only.

I can always listen to the hooks and not remove them when invalidating caches

That's one option. If the cache is a global lsp-related variable, other backends will probably not mind anyway.

I wonder if there are better way to trigger cleanup function only when the company-lsp backend is used and finished?

Have you figured out why this is not the case? Since you're using the local values of the hooks, and populating them from the backend, it seems like it should work.

garyo commented 5 years ago

Sorry to revive this old thread. I'm using javascript-typescript-server which returns thousands of completions all the time, completely unfiltered so it seems. To filter those down, this is the only thing I've found that works:

      (use-package company-lsp
        :config
        (push 'company-lsp company-backends)
        ;; Disable client-side cache because the LSP server does a better job.
        (setq company-lsp-async t
              company-lsp-enable-recompletion t
              company-lsp-filter-candidates t
              ;; this is important for typescript language server which returns way too much!
              company-lsp-match-candidate-predicate #'company-lsp-match-candidate-prefix
              company-lsp-cache-candidates 'nil)
        )

In other words set prefix matching and don't cache candidates. Anything else seems to give me way too much.

tigersoldier / company-lsp

Add options to filter candidate returned from server. #86