helix icon indicating copy to clipboard operation
helix copied to clipboard

Track long lived diagnostics inside helix

Open pascalkuthe opened this issue 1 year ago • 1 comments

Fixes #701

This PR adds a configuration option (persistant-diagnostic-sources) that allows helix to ignore some diagnostics in publishDiagnostic. This option takes affect if all diagnostics from a single lsp_type::Diagnostic::source are identical to the last list of diagnostics received publishDiagnostic for this source. In that case (If the source is part of the persistant-diagnostic-sources list) all these diagnostics are ignored and instead the "old" diagnostics for that source are maintained.

Helix already automatically updates the position of diagnostics based on edits so by keeping the existing diagnostics around we can use this mapping mechanism of helix if the server doesn't map long lived diagnostics trough changes itself.

rust-analyzer is a good example here which has many diagnostics which update instantly, but diagnostics generated by the fly-check (rustc/clippy) can only be updated when a file is saved. That means that rust-analyzer will send a publishDiagnostic notification to update it's native diagnostics. It's very hard (and inefficient) to map diagnostics trough changes server side and therefore RA has no choice but to also resend the fly check diagnostics (otherwise they are removed) which undoes the work helix does to map them.

The real root cause of this problem is that the LSP standard currently doesn't allow the server to only update some diagnostics for a file (for example by adding a source attribute to the publishDiagnostic notification itself). Therefore, I came up with this client-side workaround. This works very well in practice but is not 100% correct because in theory the diagnostics could have actually moved and incidentally all end up at the same position as previously. However, the workaround here compares the entire diagnostic (including message)so the chances of that occurring are extremely slim. Perhaps an LSP extension can be proposed for this use case in the future to handle this properly.

As some of these diagnostics now stick around longer some limitations in helixes own diagnostic mapping became apparent. In 938d522 I therefore improved the handeling of these cases (detailed description in commit message). The logic implemented here can also be reused for better inlay-hint tracking in the future (in fact VScodes inlay hint tracking partially inspired the implementation).

pascalkuthe avatar Mar 26 '23 16:03 pascalkuthe

rebased after mege of #2507. I added a small commit to fix some small inconsistencies around diagnostic sorting (and to make sure its always deterministic) since I was touching that code anyways

pascalkuthe avatar May 20 '23 19:05 pascalkuthe

Could you explain why we would want this to be configurable, rather than just doing it for all diagnostics? Especially since we're not adding any defaults.

dead10ck avatar Jun 25 '23 13:06 dead10ck

Could you explain why we would want this to be configurable, rather than just doing it for all diagnostics? Especially since we're not adding any defaults.

The reason we don't do this for all diagnostics too is that we consider diagnostics unchanged if the server sends the same diagnostics (made line/col number, message,..) again. This is not really LSP compliant and can lead to situations where diagnostics are not updated when they should (text was edited and diagnostic is now somewhere else that only incidentilally corresponds to the same positions). This is a tradeoff and only with it for diagnostics that actually benefit from this. Namely diagnostics that case only updated on safe (or some other rare event) like rust-analyer checkOnSafe diagnostics.

I also added a default for rust-analyzer so this setting is used by default.

pascalkuthe avatar Jun 26 '23 23:06 pascalkuthe

I improved the word tracking mapping even further the implementation basically matches vscode in behavior and could be used to fix most of the weirdness surrounding inlay hints (except the low idle timeout)

pascalkuthe avatar Jul 02 '23 19:07 pascalkuthe

@pascalkuthe This PR has caused immediate problems (I am using 7e389b67c24dfe4466112c988b240c807e7e2414). Warnings are not shown on screen unless I first save a file. They are shown in the picker immediately. They were shown on the screen immediately before this change (I was using 510928618dfcddcd6d3086b278332fa93ee32998).

If you want an example:

  1. clone and switch to this branch https://github.com/David-Else/rusty-zombie/tree/crossterm
  2. open main, open workspace diagnostics and see the 2 warnings.
  3. go to the warnings and see nothing is displayed on the screen
  4. save the file and they appear, you will then see:

Screenshot from 2024-01-06 10-41-07

I have:

[language-server.rust-analyzer.config]
check.command = "clippy"
rustc 1.74.1 (a28077b28 2023-12-04)
rust-analyzer 1.74.1 (a28077b 2023-12-04)

David-Else avatar Jan 06 '24 10:01 David-Else

this is not caused by this PR and not a new problem either, see https://github.com/helix-editor/helix/pull/8873

pascalkuthe avatar Jan 06 '24 18:01 pascalkuthe

this is not caused by this PR and not a new problem either, see #8873

OK, but it reliably happens every time with this PR, and I have never seen the problem before. Hope to see that fix merged asap :)

David-Else avatar Jan 06 '24 18:01 David-Else