lsp: debounced textChanges are not segmented by buffer #16424
Comments
|
@mjlbach suggested I patch rust-analyzer to show the old content. I did so as follows: diff --git a/crates/rust-analyzer/src/lsp_utils.rs b/crates/rust-analyzer/src/lsp_utils.rs
index 194d03032..4f1080a14 100644
--- a/crates/rust-analyzer/src/lsp_utils.rs
b/crates/rust-analyzer/src/lsp_utils.rs
@@ -143,6 143,11 @@ pub(crate) fn apply_document_changes(
}
}
let sys_time =
std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_millis();
let str = format!("/tmp/nvim-lsp-{}", sys_time);
std::fs::write(str, &old_text).expect("Unable to write file");
let mut index_valid = IndexValid::All;
for change in content_changes {
match change.range {When incremental sync is enabled and I rename the function, the following files are produced (in this order):
Using the second file we can see the buffer state is messed up. For example, this code is incorrect in rust-analyzer, though the code shown in the corresponding nvim buffer is fine. |
|
It's possible this is a bug with rust-analyzer, but I had similar issues with lua-language-server in the past, though there they'd happen when typing not when renaming. I haven't been able to reproduce a similar issue with Lua. |
|
It's not unlikely this is a rust-analyzer issue, but I think part of it may be due to nvim's behaviour, as disabling incremental sync doesn't trigger this problem. |
|
LSP debug logs from a rename session, with most Rust warnings silenced to not clutter the log files: https://gist.github.com/YorickPeterse/a6c5262241ca03ebef63adac18e6e142 |
|
It appears this may be caused my me using |
|
Even with |
|
I'm not sure then, maybe RA is not respecting the order in which we send the textdocument/didChanged (batched from the debounce) and the rename request |
|
I managed to reduce this down to the following way to reproduce the issue:
pub mod types;
use types::{Database, ModuleId};
fn main() {
let id = ModuleId(42);
let db = Database;
let cons1 = id.get_constant(&db, "Foo");
let cons2 = id.get_constant(&db, "Foo");
let cons3 = id.get_constant(&db, "Foo");
}
pub struct ModuleId(pub usize);
impl ModuleId {
pub fn get_constant(self, db: &Database, name: &str) -> Option<()> {
None
}
}
pub struct Database;If you then use |
|
If I use these file contents, I don't get syntax errors but instead rust-analyzer straight up crashes:
pub mod types;
use types::ModuleId;
fn main() {
let id = ModuleId(42);
let cons1 = id.get_constant("Foo");
let cons2 = id.get_constant("Foo");
let cons3 = id.get_constant("Foo");
}
pub struct ModuleId(pub usize);
impl ModuleId {
pub fn get_constant(&self, name: &str) -> Option<()> {
None
}
}If you rename With debug logging enabled, the logs triggered by the rename are as follows: |
|
With debouncing disabled, I can't reproduce these issues; including the crash. The debug logs for the rename in that case are as follows: |
|
Based on my testing so far, it seems one requirement is that the symbol you are renaming exists in one file, while you also have occurrences of that symbol in another file. If both the symbol and references are in the same file, I can't seem to reproduce the problem. |
|
With debouncing enabled, the RPC send payload before the crash looks like this: {
jsonrpc = "2.0",
method = "textDocument/didChange",
params = {
contentChanges = {
{
range = {
["end"] = {
character = 31,
line = 8,
},
start = {
character = 19,
line = 8,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 31,
line = 7,
},
start = {
character = 19,
line = 7,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 31,
line = 6,
},
start = {
character = 19,
line = 6,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 23,
line = 3,
},
start = {
character = 11,
line = 3,
},
},
rangeLength = 12,
text = "blorb",
},
},
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/types.rs",
version = 3,
},
},
}Without debouncing, this payload looks like this: {
jsonrpc = "2.0",
method = "textDocument/didChange",
params = {
contentChanges = {
{
range = {
["end"] = { character = 31, line = 8 },
start = { character = 19, line = 8 },
},
rangeLength = 12,
text = "blorb",
},
},
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/main.rs",
version = 5,
},
},
}Looking at the logs, it seems that with debouncing disabled the content changes are sent as separate RPC commands, while with debouncing enabled they are batched. One thing I find interesting is this: When debouncing is enabled, the textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/types.rs",
version = 3,
},But when debouncing is disabled, it instead is: textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/main.rs",
version = 5,
},Could it be that perhaps nvim is reporting the wrong document here, resulting in rust-analyzer modifying the wrong buffers internally? |
|
I think this is missing some contentChanges in the non-debounced version. The sequence should be the same. Looking at the RA source RN but I didn’t think it uses the version specifier internally (or at least, it was optional)
… On Nov 24, 2021, at 4:57 PM, Yorick Peterse ***@***.***> wrote:
With debouncing enabled, the RPC send payload before the crash looks like this:
{
jsonrpc = "2.0",
method = "textDocument/didChange",
params = {
contentChanges = {
{
range = {
["end"] = {
character = 31,
line = 8,
},
start = {
character = 19,
line = 8,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 31,
line = 7,
},
start = {
character = 19,
line = 7,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 31,
line = 6,
},
start = {
character = 19,
line = 6,
},
},
rangeLength = 12,
text = "blorb",
},
{
range = {
["end"] = {
character = 23,
line = 3,
},
start = {
character = 11,
line = 3,
},
},
rangeLength = 12,
text = "blorb",
},
},
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/types.rs",
version = 3,
},
},
}
Without debouncing, this payload looks like this:
{
jsonrpc = "2.0",
method = "textDocument/didChange",
params = {
contentChanges = {
{
range = {
["end"] = { character = 31, line = 8 },
start = { character = 19, line = 8 },
},
rangeLength = 12,
text = "blorb",
},
},
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/main.rs",
version = 5,
},
},
}
Looking at the logs, it seems that with debouncing disabled the content changes are sent as separate RPC commands, while with debouncing enabled they are batched.
One thing I find interesting is this:
When debouncing is enabled, the textDocument object is as follows:
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/types.rs",
version = 3,
},
But when debouncing is disabled, it instead is:
textDocument = {
uri = "file:///home/yorickpeterse/Projects/rust/playground/src/main.rs",
version = 5,
},
Could it be that perhaps nvim is reporting the wrong document here, resulting in rust-analyzer modifying the wrong buffers internally?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#16424 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFTBJS63VWK5G3XIPKW24DUNVNU7ANCNFSM5IWTMJLQ>.
|
|
Looking at the log data, it seems nvim is incorrectly grouping requests. With debouncing, nvim seems to include the changes of all buffers in one payload, using {
range = {
["end"] = {
character = 23,
line = 3,
},
start = {
character = 11,
line = 3,
},
},
rangeLength = 12,
text = "blorb",
},These are the changes for {
range = {
["end"] = {
character = 31,
line = 8,
},
start = {
character = 19,
line = 8,
},
},
rangeLength = 12,
text = "blorb",
},These are the changes for Based on this I think nvim is incorrectly grouping requests across all buffers into a single one, rather than grouping them per buffer. |
mjlbach
changed the title
|
Just to add, though I think at this point it's clear this is an nvim issue: With the above knowledge, I can also reproduce this with lua-language-server. It's a bit more tricky there as function renaming is wonky (e.g. lua-language-server often just doesn't rename occurrences), but you can reproduce it roughly like so:
Following these steps you'll likely end up with syntax errors in the buffer where you made the harmless changes, even though the code is totally fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Neovim version (nvim -v)
NVIM v0.6.0-dev 619-gcfa5d0680
Vim (not Nvim) behaves the same?
No
Operating system/version
Arch Linux
Terminal name/version
nvim-qt
$TERM environment variable
nvim-qt
Installation
Git
How to reproduce the issue
Reproducing this is a bit tricky, and I haven't been able to narrow it down to
something small. I can however reliably reproduce the issue with a project of
mine. You'll need to have rust-analyzer configured for this to work. For
reference, I use the following settings (minus settings not relevant for this
issue):
First, clone said project:
Then run the following:
Next, follow these steps:
compiler/src/types.rscompiler/src/type_check.rstypes.rs, navigate to theget_constantfunction on line 254:lua vim.lsp.buf.rename()blorbas the new name and confirm the renametype_check.rsyou'll see a bunch of syntax errors beingreported, in spite of the code in question being valid
nothing happens
With incremental sync disabled, I can't reproduce this. Instead I get a
different problem: after the first rename of
get_constanttoblorb, if I tryto rename the function again the replacement text includes a
(. If you thentry to rename the function, your code gets all messed up.
Here is a recording that shows what happens when incremental sync is enabled:
And this is what happens when you disable incremental sync:
Expected behavior
Renaming works fine, and no syntax errors are produced
Actual behavior
Either NeoVim, rust-analyzer or both end up getting messed up when incremental sync is enabled
The text was updated successfully, but these errors were encountered: