tigersoldier / company-lsp

Company completion backend for lsp-mode
GNU General Public License v3.0
251 stars 26 forks source link

Broken java completion for candidates with the same identifier #132

Open aureumleonis opened 4 years ago

aureumleonis commented 4 years ago

When all candidates share the same class/method name but differ in imports/method arguments, company inserts the common prefix among these, including the method/argument signatures. Examples:

Given prefix @NotNul and candidates ("@NotNull - javax.validation.constraints" "@NotNull - org.jetbrains.annotations"), calling company-complete-common will insert @NotNull - without showing a popup to select from the candidates or inserting any imports. A similar issue happens with method completton with overloaded methods.

Another example, given prefix assertThro and candidates (" assertThrows(Class<T> expectedType, Executable executable) : T" "assertThrows(Class<T> expectedType, Executable executable, String message) : T"), company will insert assertThrows(Class<T> expectedType, Executable executable verbatim without going through snippet expansion.

I narrowed the issue down to company-update-candidates use of try-completion to compute the common part between candidates. It seems that once company gathers these, it inserts the common part, then attempts another completion. Since the text inserted is not completable (e.g. " - ") company cancels the completion.

I hacked my own company.el to avoid inserting text beyond the symbol name which seems to work

(defun company--try-completion ()
  (let* ((common (try-completion "" company-candidates)))
    (substring common 0 (string-match-p " \\|(" common))))
yyoncho commented 4 years ago

This can be fixed if:

  1. company-lsp shows real text in the label - this could be a configurable option.
  2. company-mode asks the backend to calculate what is the correct common prefix.

CC @dgutov

dgutov commented 4 years ago

@yyoncho If 1 is doable, why make it an option? Sounds like a good default behavior: use the "real text" as the completion string, and put the rest into the annotation.

Regarding 2, I suppose company-update-candidates could call the match backend command to calculate company-common. If your backend supports it anyway. But that would be an unexpected use of that command.

aureumleonis commented 4 years ago

the nice thing about having that text in the candidate is that you can filter by java packages for classes with the same name. With option 1 this feature would simply stop working altogether right? How difficult is to create another backend command in company for this purpose in a backwards compatible way?

dgutov commented 4 years ago

No immensely difficult, and your usage is interesting but unexpected too.

Further, being able to do 1. should also fix the company-tng problems...

yyoncho commented 4 years ago

If 1 is doable, why make it an option? Sounds like a good default behavior: use the "real text" as the completion string, and put the rest into the annotation.

It is doable but it is not TRT - as per spec we should show the label property. Consider that the insert text might be a snippet.

dgutov commented 4 years ago

I'm saying that you would show the rest of the text as annotation.

dgutov commented 4 years ago

So the whole string would be visible. And whatever text the snippet needs would be inserted in post-completion.