PublishDiagnostics not sent when inserting whitespace or comments with Helix

[kirawi]

helix-editor/helix#701
Log:

2021-10-24T18:11:44.609 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":11,"line":2},"start":{"character":11,"line":2}},"text":" "}],"textDocument":{"uri":"file:///C:/Users/there/Desktop/derp/src/main.rs","version":38}}}
2021-10-24T18:11:44.610 helix_lsp::transport [ERROR] err <- "[INFO rust_analyzer::main_loop] handle_event(Notification { method: \"textDocument/didChange\" })\n"
2021-10-24T18:11:44.697 helix_lsp::transport [ERROR] err <- "[INFO rust_analyzer::main_loop] handle_event(Diagnostics([(FileId(0), [])]))\n"
(No PublishDiagnostics?)
2021-10-24T18:11:48.620 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"range":{"end":{"character":12,"line":2},"start":{"character":12,"line":2}},"text":"a"}],"textDocument":{"uri":"file:///C:/Users/there/Desktop/derp/src/main.rs","version":39}}}
2021-10-24T18:11:48.620 helix_lsp::transport [ERROR] err <- "[INFO rust_analyzer::main_loop] handle_event(Notification { method: \"textDocument/didChange\" })\n"
2021-10-24T18:11:48.684 helix_lsp::transport [ERROR] err <- "[INFO rust_analyzer::main_loop] handle_event(Diagnostics([(FileId(0), [Diagnostic { range: Range { start: Position { line: 2, character: 13 }, end: Position { line: 2, character: 13 } }, severity: Some(Error), code: Some(String(\"syntax-error\")), code_description: Some(CodeDescription { href: Url { scheme: \"https\", cannot_be_a_base: false, username: \"\", password: None, host: Some(Domain(\"rust-analyzer.github.io\")), port: None, path: \"/manual.html\", query: None, fragment: Some(\"syntax-error\") } }), source: Some(\"rust-analyzer\"), message: \"Syntax Error: expected SEMICOLON\", related_information: None, tags: None, data: None }])]))\n"
2021-10-24T18:11:48.685 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///c:/Users/there/Desktop/derp/src/main.rs","diagnostics":[{"range":{"start":{"line":2,"character":13},"end":{"line":2,"character":13}},"severity":1,"code":"syntax-error","codeDescription":{"href":"https://rust-analyzer.github.io/manual.html#syntax-error"},"source":"rust-analyzer","message":"Syntax Error: expected SEMICOLON"},{"range":{"start":{"line":3,"character":4},"end":{"line":3,"character":9}},"severity":1,"code":"E0425","codeDescription":{"href":"https://doc.rust-lang.org/error-index.html#E0425"},"source":"rustc","message":"cannot find function `error` in this scope\nnot found in this scope"},{"range":{"start":{"line":0,"character":4},"end":{"line":0,"character":25}},"severity":2,"code":"unused_imports","source":"rustc","message":"unused import: `std::process::Command`\n`#[warn(unused_imports)]` on by default","relatedInformation":[{"location":{"uri":"file:///c:/Users/there/Desktop/derp/src/main.rs","range":{"start":{"line":0,"character":0},"end":{"line":0,"character":26}}},"message":"remove the whole `use` item"}],"tags":[1]},{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":26}},"severity":4,"code":"unused_imports","source":"rustc","message":"remove the whole `use` item","relatedInformation":[{"location":{"uri":"file:///c:/Users/there/Desktop/derp/src/main.rs","range":{"start":{"line":0,"character":4},"end":{"line":0,"character":25}}},"message":"original diagnostic"}]}],"version":39}}

The file is the same as the one in the video in the linked issue. I'm assuming it's something we're doing wrong, but I don't know what.

[bjorn3]

Based on the asciinema it seems like helix translates the line+column sent by rust-analyzer to an offset from the start of the file once and then reuses it every time the file changes even if changing the length of an earlier line would change this file offset for the original line+column pair.

[bjorn3]

Looking at the code I can't find anything that suggests this is indeed the case though. Thisvis the first time I look at it's code though, so I may be missing something.

[kirawi]

Based on the asciinema it seems like helix translates the line+column sent by rust-analyzer to an offset from the start of the file once and then reuses it every time the file changes even if changing the length of an earlier line would change this file offset for the original line+column pair.

I don't think so (though maybe I'm misunderstanding) as the diagnostics should be updated every time there is an LSP event:
https://github.com/helix-editor/helix/blob/acc5ac5e731ea978fdd8a0f6762f2cd5101780b2/helix-term/src/application.rs#L339-L401

Though perhaps you meant something like this?
https://github.com/helix-editor/helix/blob/acc5ac5e731ea978fdd8a0f6762f2cd5101780b2/helix-lsp/src/lib.rs#L61-L104

Helix won't flush the diagnostics for the buffer and depends upon the LSP sending updated diagnostics to do that. Is Rust-Analyzer (and LSPs in general) not meant to send PublishDiagnostics on whitespace/comments?

[bjorn3]

https://github.com/rust-analyzer/rust-analyzer/blob/5ce9b046696846099de8f052818aae5c18d3cfe3/crates/rust-analyzer/src/main_loop.rs#L456

Rust-analyzer only publishes diagnostics when the diagnostics actually change it seems. The file offset doesn't matter, only the line+column pair. I think helix should handle this gracefully as rust-analyzer may take a while for the new publish diagnostics to be sent due to being busy even if it were to sent it at all. I think the fix would be to translate from line+column to file offset every time the file changes, or only use line+column internally. Showing the error requires translating from file offset to line+column anyway, doesn't it?

[kirawi]

Ah I see, that makes sense!

[archseer]

I can't fully remember but I think the issue was with diagnostics that only get calculated on file save (not simple syntax issues). We get one set of diagnostics from rust-analyzer, and while I had code that would adjust diagnostics positioning according to edits, on each change rust-analyzer would also re-send the original diagnostics with outdated offsets, effectively clobbering the old set. I don't think there's much we can do there other than track what the original diagnostics were before remapping and then de-duplicate.

[bjorn3]

and while I had code that would adjust diagnostics positioning according to edits

Vscode doesn't do this.

on each change rust-analyzer would also re-send the original diagnostics with outdated offsets, effectively clobbering the old set.

Yeah, this is a known issue in rust-analyzer (#8836). In vscode it will keep showing the outdated location.

I don't think there's much we can do there other than track what the original diagnostics were before remapping and then de-duplicate.

For the cargo check diagnostics arguably the outdated location problem should be fixed in rust-analyzer. For all other diagnostics I think helix should recalculate file offsets from the original line+column sent by rust-analyzer when the file is changed.

[archseer]

Ah okay, yeah it's the same issue then.

I think helix should recalculate file offsets from the original line+column sent by rust-analyzer when the file is changed.

Yeah we accomplish this via the position mapping. The edits are OT-like so we're able to transform selections from offsets in the old document to the new one.