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

Track long lived diagnostics inside helix #6447

Merged
merged 3 commits into from
Dec 27, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Mar 26, 2023

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 through 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 b3145f9 I therefore improved the handling 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 pascalkuthe added C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 26, 2023
@pascalkuthe pascalkuthe mentioned this pull request Mar 26, 2023
languages.toml Outdated Show resolved Hide resolved
@pascalkuthe
Copy link
Member Author

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 pascalkuthe force-pushed the longterm_diagnostics branch 2 times, most recently from ae5106c to 352cad6 Compare June 20, 2023 16:36
@dead10ck
Copy link
Member

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.

@pascalkuthe
Copy link
Member Author

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 pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 27, 2023
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2023
@pascalkuthe
Copy link
Member Author

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)

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. I've been using it for a while and I like the assoc changes, especially when stacked with #6417. Just a small comment about the docs

helix-core/src/syntax.rs Show resolved Hide resolved
book/src/languages.md Outdated Show resolved Hide resolved
Diagnostics are currently extended if text is inserted at their end. This is
desirable when inserting text after an identifier. For example consider:

let foo = 2;
    --- unused variable

Renaming the identifier should extend the diagnostic:

let foobar = 2;
    ------ unused variable

This is currently implemented in helix but as a consequence adding whitespaces
or a type hint also extends the diagnostic:

let foo      = 2;
    -------- unused variable
let foo: Bar = 2;
    -------- unused variable

In these cases the diagnostic should remain unchanged:

let foo      = 2;
    --- unused variable
let foo: Bar = 2;
    --- unused variable

As a heuristic helix will now only extend diagnostics that end on a word char
if new chars are appended to the word (so not for punctuation/ whitespace).
The idea for this mapping was inspired for the word level tracking vscode uses
for many positions. While VSCode doesn't currently update diagnostics after
receiving publishDiagnostic it does use this system for inlay hints for example.
Similarly, the new association mechanism implemented here can be used for word
level tracking of inlay hints.

A similar mapping function is implemented for word starts. Together
these can be used to make a diagnostic stick to a word. If that word
is removed that diagnostic is automatically removed too. This is the exact
same behavior VSCode inlay hints eixibit.
@David-Else
Copy link
Contributor

David-Else commented Jan 6, 2024

@pascalkuthe This PR has caused immediate problems (I am using 7e389b6). 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 5109286).

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)

@pascalkuthe
Copy link
Member Author

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

@David-Else
Copy link
Contributor

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing text offsets diagnostics that are after text typed
6 participants