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

Allow clients to customize how to get prefix bounds. #281

Merged
merged 1 commit into from
Mar 3, 2018

Conversation

tigersoldier
Copy link
Contributor

Add a new :prefix-function parameter to the define functions.

Currently lsp-mode graps the symbol before point as the completion prefix.
The prefix is used for matching completion candidates and will be replaced by
the selected completion candidate. Some servers use different methods to
calculate prefixes. If the prefixes between the server and the client don't
match possible results are:

  • Candidates are filtered out unexpecedly, because the client side prefix has
    extra leading characters.
  • Completed candidate has duplicated leading characters, because the server
    side prefix has extra leading characters and all the candidates also have
    the leading characters.

Only the clients knows which server they are talking to, so it's the clients'
responsibility to implement custom prefix funtions if necessary.

@tigersoldier
Copy link
Contributor Author

@vibhavp @gpittarelli mind reviewing this PR? I have pending PRs in company-lsp and lsp-javacomp showcasing how this will be used:
tigersoldier/company-lsp#26
tigersoldier/lsp-javacomp#1

This would provide the foundation for fixing a cquery-related bug: tigersoldier/company-lsp#21

@vibhavp
Copy link
Member

vibhavp commented Feb 9, 2018

Isn't getting the bounds of the identifier under point a feature provided by the language major mode? Dealing with the semantics of language syntax should not be lsp-mode's job, IMO.

@tigersoldier
Copy link
Contributor Author

The definition of the bound under the point is different between emacs major mode and the language servers.


For example, consider the the C++ code below:

#include|

| denotes the cursor position. cc-mode considers the bounds to be around include, while cquery considers the bounds to be around #include. The candidates cquery provides all start with #. When a candidate is chosen, say #include <stdio>, include is replaced with the candidate, resulting the content to be

##include <stdio>|

with unexpected redundant #.


A second case is Java. Consider the following code:

@Over|

java-mode considers the bounds to be @Over. However JavaComp considers the bounds to be Over. All candidates don't have the @ prefix and are filtered out because none of them match @Over.


It doesn't make sense for the language servers to match the bounds algorithm in emacs because the servers are editor-independent.

It doesn't make sense for lsp-mode to match the bounds algorithm in language servers because lsp-mode is language-agnostic.

It doesn't make sense for major modes to match the bounds algorithm in language servers because a language can have multiple servers and they may have different algorithms.

So it should be clients' responsibility to provide the bounds algorithm for the server they talk to. It's optional. If a client doesn't want to bother providing bounds, lsp-mode still provide a default one. But lsp-mode should allow clients to customize it if they want to.

@tigersoldier
Copy link
Contributor Author

@vibhavp Do you have any other concerns on this PR?

@vibhavp
Copy link
Member

vibhavp commented Feb 16, 2018

I haven't been able to find time for reviewing this, I'll do so the coming week. Sorry for the delay!

@MaskRay
Copy link
Member

MaskRay commented Feb 19, 2018

LG. In the cquery case, triggering completion-at-point at #| results in the following response from the server:

"textEdit":{"range":{"start":{"line":2,"character":0},"end":{"line":2,"character":1}},"newText"
:"#include <aio.h>"}}

The first character # is supposed to be replaced by #include <aio.h>

@vibhavp
Copy link
Member

vibhavp commented Feb 26, 2018

@tigersoldier Looks good. I'll merge this if you can fix the conflicts.

lsp-methods.el Outdated
@@ -39,6 +39,7 @@
(get-root nil :read-only t)
(ignore-regexps nil :read-only t)
(ignore-messages nil :read-only t)
(prefix-function nil :read-only t)
Copy link
Member

Choose a reason for hiding this comment

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

Could you make prefix-function the last struct member? Adding a new member in the middle of the struct breaks binary compatibilities with byte-compiled client packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Add a new :prefix-function parameter to the define functions.

Currently lsp-mode graps the symbol before point as the completion prefix.
The prefix is used for matching completion candidates and will be replaced by
the selected completion candidate. Some servers use different methods to
calculate prefixes. If the prefixes between the server and the client don't
match possible results are:

 * Candidates are filtered out unexpecedly, because the client side prefix has
   extra leading characters.
 * Completed candidate has duplicated leading characters, because the server
   side prefix has extra leading characters and all the candidates also have
   the leading characters.

Only the clients knows which server they are talking to, so it's the clients'
responsibility to implement custom prefix funtions if necessary.
@tigersoldier
Copy link
Contributor Author

Please take a look again. I rebased to HEAD of master. Addressed comments.

@vibhavp vibhavp merged commit 4f14de3 into emacs-lsp:master Mar 3, 2018
@vibhavp
Copy link
Member

vibhavp commented Mar 3, 2018

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants