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

OLS autocomplete with completion-replace = true removes next char #11933

Closed
roignpar opened this issue Oct 24, 2024 · 9 comments
Closed

OLS autocomplete with completion-replace = true removes next char #11933

roignpar opened this issue Oct 24, 2024 · 9 comments
Labels
C-bug Category: This is a bug

Comments

@roignpar
Copy link

Summary

When using ols (the Odin Language Server) with Helix, every time an autocomplete option is selected and inserted the next character is removed. If there is no next character Helix crashes. This only seems to happen when completion-replace = true.
I have seen the crash with other LSPs, but not the removal.

Reproduction Steps

Screencast From 2024-10-24 12-41-54
With ols installed and available to Helix, open an .odin file and try to insert an autocomplete suggestion.

Helix log

The full log file is very large, I will only post the messages around the time the screencast was recorded. I tried activating RUST_BACKTRACE, but that didn't help much apparently.

~/.cache/helix/helix.log
2024-10-24T12:41:17.431 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:19.978 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:19.978 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:21.866 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:21.866 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:22.629 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:22.926 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:24.584 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:41:38.119 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:41:38.660 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:38.990 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:41:57.379 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:41:57.379 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:01.148 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:01.149 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:01.149 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:03.816 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:04.561 helix_lsp::transport [ERROR] ols err: <- StreamClosed
2024-10-24T12:42:04.561 helix_lsp::transport [ERROR] err: <- Other(failed to send a message to server

Caused by:
    channel closed)
2024-10-24T12:42:45.058 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:42:46.389 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:42:46.389 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:42:47.742 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:42:48.607 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:50.366 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:50.501 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:50.851 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:42:51.683 helix_lsp::transport [ERROR] ols err: <- StreamClosed
2024-10-24T12:42:51.684 helix_lsp::transport [ERROR] err: <- Other(failed to send a message to server

Caused by:
    channel closed

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>)
2024-10-24T12:43:06.437 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:43:09.421 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:43:09.421 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:43:11.226 helix_lsp::util [WARN] LSP position Position { line: 7, character: 0 } out of range assuming EOF
2024-10-24T12:43:12.131 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:43:12.945 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:43:13.689 helix_lsp::util [WARN] LSP position Position { line: 6, character: 0 } out of range assuming EOF
2024-10-24T12:43:15.082 helix_lsp::transport [ERROR] ols err: <- StreamClosed
2024-10-24T12:43:15.082 helix_lsp::transport [ERROR] err: <- Other(failed to send a message to server

Caused by:
    channel closed

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>)

Platform

Linux

Terminal Emulator

alacritty 0.14.0 (22a44757) + zellij 0.40.1

Installation Method

pacman

Helix Version

helix 24.7

@roignpar roignpar added the C-bug Category: This is a bug label Oct 24, 2024
@RoloEdits
Copy link
Contributor

This issue might have been fixed in #11266. If you are able to build from source to try on or after that commit that would be helpful to confirm so.

@roignpar
Copy link
Author

Indeed, the issue seems to be fixed on current master. Any clue when the next release will be?

@RoloEdits
Copy link
Contributor

Hmm, actually, maybe the panic was fixed, but I think there could still be an issue here.

Using rust-analyzer I am also seeing the eating of characters when completion-replace = true.

Can you confirm that the character removal was also fixed, not just the panic?

@roignpar
Copy link
Author

roignpar commented Nov 5, 2024

I think it depends on what you are completing. completion-replace = true does exactly that, replace any word with the completion.

std::|Something

| being the cursor. If a completion is inserted at cursor Something will be removed.

On the other hand:

std::|()

In this case () shouldn't be removed when inserting a completion at |.

Another possible bug I'm seeing now while testing again:

std::|Something

When inserting a completion at | the cursor is placed one character before the end of the inserted word:

std::syn|c // I expect this to be std::sync|

Just tried this again with latest master and rust-analyzer.

@RoloEdits
Copy link
Contributor

Doing some testing of

helix/helix-lsp/src/lib.rs

Lines 292 to 313 in b53dafe

fn completion_range(
text: RopeSlice,
edit_offset: Option<(i128, i128)>,
replace_mode: bool,
cursor: usize,
) -> Option<(usize, usize)> {
let res = match edit_offset {
Some((start_offset, end_offset)) => {
let start_offset = cursor as i128 + start_offset;
if start_offset < 0 {
return None;
}
let end_offset = cursor as i128 + end_offset;
if end_offset > text.len_chars() as i128 {
return None;
}
(start_offset as usize, end_offset as usize)
}
None => find_completion_range(text, replace_mode, cursor),
};
Some(res)
}

and

helix/helix-lsp/src/lib.rs

Lines 276 to 291 in b53dafe

fn find_completion_range(text: RopeSlice, replace_mode: bool, cursor: usize) -> (usize, usize) {
let start = cursor
- text
.chars_at(cursor)
.reversed()
.take_while(|ch| chars::char_is_word(*ch))
.count();
let mut end = cursor;
if replace_mode {
end += text
.chars_at(cursor)
.take_while(|ch| chars::char_is_word(*ch))
.count();
}
(start, end)
}

and found that the values of the calculated function(the second one) don't always match the other where the value is supposed to be passed in.

Checking if its replace mode and always calculating seemed to always produced the desired outcome(at least I haven't found any yet outliers yet):

    fn completion_range(
        text: RopeSlice,
        edit_offset: Option<(i128, i128)>,
        replace_mode: bool,
        cursor: usize,
    ) -> Option<(usize, usize)> {
        let res = if replace_mode {
            find_completion_range(text, replace_mode, cursor)
        } else {
            match edit_offset {
                Some((start_offset, end_offset)) => {
                    let start_offset = cursor as i128 + start_offset;
                    if start_offset < 0 {
                        return None;
                    }
                    let end_offset = cursor as i128 + end_offset;
                    if end_offset > text.len_chars() as i128 {
                        return None;
                    }

                    (start_offset as usize, end_offset as usize)
                }
                None => find_completion_range(text, replace_mode, cursor),
            }
        };

        Some(res)
    }

Im not entirely sure but it seems like the passed in values are from the lsp itself? Kind of hard to follow exactly where the .range is coming from as I followed this up the call stack. I did notice as well that in some of the logs from rust-analyzer have an error were it mentions truncation being done as there was some out of bounds happening?

So i'm not sure if this is a helix thing or a rust analyzer thing, or both. Have you noticed this behavior with odin's lsp as well?

I build rust-analyzer from source so I can enable more native optimizations, but when trying the downloaded binary I also noticed it. But the one that ships with rustup thats dated to the 1.82 release cycle didn't appear to have the same issues.

I think we can either open this issue again, or even make a new one, so that we can get other people looking for root of this issue?

@RoloEdits
Copy link
Contributor

RoloEdits commented Nov 6, 2024

Here are some gif's of the behavior I am experiencing:

helix_rust-analyzer

helix_rust-analyzer-2

Getting this often too:

helix_lsp::transport [ERROR] rust-analyzer <- ServerError(-32801): content modified

and this

helix_lsp::transport [ERROR] rust-analyzer err <- "2024-11-05T17:22:58.3730986-08:00 ERROR Overloaded deref on type str is not a projection\n"

@roignpar
Copy link
Author

roignpar commented Nov 8, 2024

It looks just like the original issue I had. My guess is it might be related to the version of rust-analyzer you are using, but maybe it is worth opening a new issue to get the attention of someone more knowledgeable.

@RoloEdits
Copy link
Contributor

That is to say you no longer experience this in OLS? If so, this might be a rust-analyzer bug.

@roignpar
Copy link
Author

roignpar commented Nov 9, 2024

That is to say you no longer experience this in OLS? If so, this might be a rust-analyzer bug.

Yes, when using the master branch of Helix I no longer experience this with OLS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

3 participants