-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Panics with 'index out of bounds' at crates/ide_db/src/line_index.rs:106 in neovim #11081
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
Comments
Panic workaround implemented in #11182 |
11182: fix: don't panic on seeing an unexpected offset r=Veykril a=dimbleby Intended as a fix, or at least a sticking plaster, for #11081. I have arranged that [offset()](https://github.com./rust-analyzer/rust-analyzer/blob/1ba9a924d7b161c52e605e157ee16d582e4a8684/crates/ide_db/src/line_index.rs#L105-L107) returns `Option<TextSize>` instead of going out of bounds; other changes are the result of following the compiler after doing this. Perhaps there's still an issue here - I suppose the server and client have gotten out of sync and that probably shouldn't happen in the first place? I see that #10138 (comment) suggests what sounds like a more substantial fix which I think might be aimed in this direction. So perhaps that one should be left open to cover such things? Meanwhile, I hope that not-crashing is a good improvement: and I can confirm that it works out just fine in the repro I have at #11081. Co-authored-by: David Hotham <[email protected]>
Let's reopen this, as it sounds like something serious which we should fix! To give a broad context here, in general rust-analyzer is carefully architectured such that panics do not crash the process or corrupt state. Panics happen fairly often, but they should just display an annoying message, and not require restarting of the process. So, what we are seeing here, a panic bringing down the whole process, is a very serious issue. The problem isn't the panic per se, but rather the fact that the panic compromizes user experience. According to the log, the error happens in I have two hypothesis here:
@dimbleby Am I correct that you are using neovim's built-in client? |
yes |
Got the LSP log:
I think it's pretty clear what the problem, these two messages are to blame (reformatted and lightly edited for clarity):
Note how the second message refers to the third line, but, given the |
Given that the panic has an implemented workaround, and this is not our bug, removing the Broken Window label |
SGTM! Though, we should revert
#11182 once the client fix
propagates. We shouldn’t be masking client errors. We probably shouldn’t
panic on invalid input either, but we at least need to send an LSP error
message
…On Friday, 22 April 2022, Jonas Schievink ***@***.***> wrote:
Given that the panic has an implemented workaround, and this is not our
bug, removing the *Broken Window* label
—
Reply to this email directly, view it on GitHub
<#11081 (comment)>,
or unsubscribe
<https://github.com./notifications/unsubscribe-auth/AANB3M7I4FENUQKRF5JY74DVGKP3RANCNFSM5KP3ZG6Q>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Hey @matklad ,
Edit: just noticed the VSCode extension has migrated to another page (it wasn't showing further updates). Removing the previous one and re-installing the new one works as expected, sorry for bothering you :) |
We seem to already be sending an error message back in these cases now ( |
rust-analyzer version: (eg. output of "Rust Analyzer: Show RA Version" command)
rust-analyzer 0add6e9 2021-12-20 stable
rustc version: (eg. output of
rustc -V
)rustc 1.57.0
I frequently have to restart rust-analyzer because it has panics like this:
I can reliably provoke this in neovim as follows:
Similar to #10138, but reporting separately because (i) it looks like a different stack and (ii) I have a clear repro.
The text was updated successfully, but these errors were encountered: