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

Use textEdit of completionItem unconditionally #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

machitgarha
Copy link

@machitgarha machitgarha commented Jul 21, 2022

Identify the Bug

php-ide-serenata#527

Description of the Change

The description is explained in the commit. Please read that beforehand.

Give priority to the server's CompletionItem.textEdit rather than the replacementPrefix extracted in the client. These cases shouldn't be different in most cases, but not some. For example, in the case of a trigger character, the server may decide to replace it along with the prefix, but the client ignores it, resulting in wrong output, because newText is created on the server and it is the server who actually knows where it must be placed.

Alternate Designs

There are many alternatives actually. But the current change is minimal and it only affects if the server set the CompletionItem.textEdit.

Possible Drawbacks

Possibly zero. If a package using this library get impacted by this, then the LSP server it uses is doing something wrong, so a hidden bug will be exposed. But this is a rare case, at least theoretically.

Verification Process

Tested with php-ide-serenata. The mentioned issue is fixed there (e.g. autocompletion of $this leads to $$this).

Release Notes

  • Fix CompletionItem.textEdit of the server being ignored.

The server's CompletionItem.textEdit should always be respected,
because in recent versions of the LSP, it is the server who should
tell what to replace and what not.

Before this change, a trigger character was always ignored, whether
the server requested to replace it or not. This could be problematic
in some cases.

For instance, in PHP, you have a dollar sign beforeevery variable.
If the server request to replace say `$th` with `$this` using a
TextEdit interface, the client translates this to replacing `th`
with `$this`, which results in a wrong `$$this` result.
Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

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

The tests fail

@machitgarha
Copy link
Author

Yes, I realized it. As soon as I find time, I'll look into it. Thanks for your attention.

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.

2 participants