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

References to vim positions can't handle non-ASCII chars #425

Closed
clason opened this issue Jul 4, 2019 · 10 comments · Fixed by #490
Closed

References to vim positions can't handle non-ASCII chars #425

clason opened this issue Jul 4, 2019 · 10 comments · Fixed by #490
Labels

Comments

@clason
Copy link
Contributor

clason commented Jul 4, 2019

The issue is that the encoding of non-English characters (cue UTF-16 rant) breaks position information in vim-lsp, leading to all sorts of issues. Consider the following minimal example using the pyls language server for Python (I use neovim HEAD for this):

  1. Edit a fresh Python file, e.g., with nvim test.py
  2. Type the following:
var1 = 1.0
var2 = var1**2
  1. Move the cursor to var1 on the first line, which should correctly highlight var1 on the second line as well (cursor is on character after |, highlight indicated by [...]):
|var1 = 1.0
var2 = [var1]**2
  1. Change var2 to vär2, move cursor back to first line:
v|ar1 = 1.0
vär2 =[ var]1**2

Note how the highlight is shifted left by one character. This is not just a cosmetic problem; the location of the symbol var1 on the second line is calculated incorrectly, which means that, e.g., hover (which triggers on the same -- incorrect -- characters as are highlighted) and identification of completion triggers for candidate selection and replacement do not work.

EDIT It seems that the issue is with the range_to_position function in references.vim; it's not clear to me how to change it to account for non-ASCII chars outside the given range.

  1. (Bonus EDIT: this is fixed by Re-porting vim-lsc's diff logic (for multibyte character) #447, but I'm leaving it here since the error message might contain useful information to someone.) Now change vär2 to vör2, go back to var1, and get the error message
Error detected while processing function lsp#ui#vim#references#highlight[31]..lsp#send_requ
est[4]..lsp#utils#step#start[1]..<SNR>75_next[9]..<lambda>121[1]..<SNR>70_ensure_flush[1]..
lsp#utils#step#start[1]..<SNR>75_next[9]..<lambda>123[1]..<SNR>70_ensure_start[15]..<SNR>75
_callback[1]..<SNR>75_next[9]..<lambda>124[1]..<SNR>70_ensure_init[6]..<SNR>75_callback[1].
.<SNR>75_next[9]..<lambda>125[1]..<SNR>70_ensure_conf[14]..<SNR>75_callback[1]..<SNR>75_nex
t[9]..<lambda>126[1]..<SNR>70_ensure_open[16]..<SNR>75_callback[1]..<SNR>75_next[9]..<lambd
a>127[1]..<SNR>70_ensure_changed[19]..<SNR>70_send_notification[6]..lsp#log_verbose[2]..lsp
#log:
line    2:
E474: String "<b6>" contains byte that does not start any UTF-8 character
@clason clason changed the title UTF-8 chars break vim-lsp Non-ASCII chars break vim-lsp Jul 30, 2019
@clason clason changed the title Non-ASCII chars break vim-lsp References to vim positions can't handle non-ASCII chars Aug 13, 2019
@clason
Copy link
Contributor Author

clason commented Sep 1, 2019

Closed by #467 (Thanks!)

@clason clason closed this as completed Sep 1, 2019
@clason
Copy link
Contributor Author

clason commented Sep 1, 2019

Sorry, wrote too soon. Something is different between the merged version and the one I tested.

It's fixed on the @machakann's https://github.com/machakann/vim-lsp/tree/issue425 branch, but not on vim-lsp master. It seems that not all changes made it from that branch to the one the actually merged PR was based on. I'll see if I can extract the missing changes and open a fixup PR.

@clason clason reopened this Sep 1, 2019
@clason
Copy link
Contributor Author

clason commented Sep 1, 2019

@machakann It seems that you're still working on that branch -- do you plan on merging it soon, or should I cherry-pick your fix that didn't make it into the PR?

@machakann
Copy link
Contributor

Hi @clason, yes I'm working on it. I inserted character-byte index interconversion at the places necessary (maybe). Currently, I'm searching for a language server for testing some features; my language server won't work with rename command... At least, it seems textDocument/completion is working.

I will open a new PR soon, I would be glad if you could help to test the branch.

@clason
Copy link
Contributor Author

clason commented Sep 1, 2019

@machakann Great, thank you! I tested your branch, and hover and Goto/PeekDefinition seems to be working as well. I have a language server (texlab) that does support renaming, and I'll be happy to test it. (Have to find an example that doesn't work on master first.)

EDIT: Yes, rename seems to be fixed, as well.

@clason
Copy link
Contributor Author

clason commented Sep 7, 2019

@machakann If you wish, I can make a PR on top of master based on your branch now so we can get the fixes in, and you can continue on top of that?

@machakann
Copy link
Contributor

@clason Sorry for being late. I submitted! #490

@clason
Copy link
Contributor Author

clason commented Sep 8, 2019

@machakann No need to apologize! I really appreciate your kind help!

I can confirm that #490 fixes my issues (including hover, Definition, References, completion, and rename).

prabirshrestha pushed a commit that referenced this issue Sep 8, 2019
* Multibyte character support for lsp#get_position()

* Multibyte character support for "textDocument/documentHighlight"

* Fix `lsp#utils#byteindex()` would fails if a file is not yet loaded

* Add tests

* Rename lsp#utils#byteindex() and lsp#utils#charindex()

lsp#utils#byteindex() -> lsp#utils#to_col()
lsp#utils#charindex() -> lsp#utils#to_char()

* Multibyte character support for textEdit

* Remove unused line

* Multibyte character support for  "textDocument/rangeFormatting"

* Multibyte character support for  "textDocument/publishDiagnostics"
@prabirshrestha
Copy link
Owner

#490 has been merged. please try again.

@clason
Copy link
Contributor Author

clason commented Sep 8, 2019

Yep, still works! Thank you!

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

Successfully merging a pull request may close this issue.

3 participants