Inline Diagnostics

[pascalkuthe]

Closes https://github.com/helix-editor/helix/issues/3272 Closes #3078 closes https://github.com/helix-editor/helix/issues/1462 Closes https://github.com/helix-editor/helix/issues/1953 Supersedes #6059

This PR implements inline diagnostics just like #6059. However, it also contains a streamlined virtual text API and significantly improves on the implementation in #6059.

The various improvements and bugfixes made to the virtual text API are currently part of this PR until it's been tested more. I will spin those out into a separate PR later. These should address the various bugs and panics discovered by poeple in #6059. Only the last commit contains an implementation of inline diagnostics.

Due to the changes to the virtual text API I had to essentially rebuild #6059 as most changes in that PR became unnecessary by the improvements in the virtual text API.

I also rebuild the ASCII rendering to support soft wrapping. Diagnostics which exceed the width of the screen are now soft wrapped automatically. Additionally, there is a minimum width setting for diagnostics. Any diagnostics so far to the right that there is not this minimum width available are displayed using a backwards arching line. See screenshot below:

Additionally, this PR implements a severity-based filter for diagnostics. This allows users to choose one of the following:

• a minimum severity for inline diagnostics: "warning"
• a list of severities to allow (useful if a certain LSP spams one particular severity). ["warning", "errror"] Using an empty list disables inline diagnostics

The diagnostics to display can be separately configured for the line that currently contains the cursor. By disabling diagnostics for all other lines users can therefore only display diagnostics on the current line too. By default, only warnings/errors are displayed, on the cursor line all diagnostics are displayed. I am open to changing the default here, but this did feel quite nice to use in my testing.

Additionally, some details of the soft wrapping can be configured. Currently, a draft as this needs more documentation, a separate PR for most of the virtual text changes, and more testing. That said it should work quite well already, and I invite people to try this out.

[pascalkuthe]

(one test is failing for a type of virtual text that is currently not used yet. I forgot to clean that up, will look into that tomorrow, this shouldn't affect usage)

[carrascomj]

I noticed a bug (or strange behavior) trying this when combining wrapping with splits: on insert, the cursor moves away from the place where the actual insertion takes place.

See an example here: https://asciinema.org/a/DlnOVhiM8g2kXVtGBjwVFk8lo

(Maybe it is also a problem in master, mark as off-topic if that's the case).

[pascalkuthe]

thanks for testing! This is a new regression with this PR and doesn't occur on master.

[yuyidegit]

After merged this PR, LSP Type Hints #5934 have no effect.

[pascalkuthe]

Thank you for your feedback @yuyidegit] both bugs have been fixed (the test failures too).

This PR now depends on #6440 which fixes a bunch of bugs relevant to this PR. I will create another PR for the more complex virtual text API improvements once that one is merged

[gabydd]

found a pretty nasty bug to reproduce:

1. open helix running this pr in the helix repo
2. open helix-core/src/comment.rs
3. delete the s in lines on line 21
4. save the file so an error appears
5. scroll down the file till line 28 is the top line
6. when you go to the next line helix will crash with(some of the backtrace omitted that doesn't matter)

thread 'main' panicked at 'attempt to add with overflow', helix-term/src/ui/text_decorations.rs:136:16
 3: helix_term::ui::text_decorations::DecorationManager::render_virtual_lines
             at ./helix-term/src/ui/text_decorations.rs:136:16
   4: helix_term::ui::document::render_text
             at ./helix-term/src/ui/document.rs:166:17
   5: helix_term::ui::document::render_document
             at ./helix-term/src/ui/document.rs:79:5
   6: helix_term::ui::editor::EditorView::render_view
             at ./helix-term/src/ui/editor.rs:208:9
   7: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
             at ./helix-term/src/ui/editor.rs:1394:13
   8: helix_term::compositor::Compositor::render
             at ./helix-term/src/compositor.rs:170:13

I can reproduce this same panic for any inline diagnostic, seems like an edge case when the line that an error comes from does not fit on the screen. So the diagnostic would be the first line. But the first line rendered in this case would be line 27 which is technically row 2 so it has a visual_pos.row of 1 as well as nothing has been rendered last_line_pos.visual_line is equal to u16::MAX. because we are technically rendering the second line the check here passes https://github.com/pascalkuthe/helix/blob/c0f5627dd482de6266175e544c6ff2932c0f3925/helix-term/src/ui/document.rs#L160 and as last_line_pos.visual_line is u16::MAX and virt_off is 1, tada it panic.

[ocharles]

I just gave this a try and see to get a whopping big inline diagnostic that's much bigger than the diagnostic itself:

[pascalkuthe]

found a pretty nasty bug to reproduce:

1. open helix running this pr in the helix repo

2. open `helix-core/src/comment.rs`

3. delete the `s` in `lines` on line 21

4. save the file so an error appears

5. scroll down the file till line 28 is the top line

6. when you go to the next line helix will crash with(some of the backtrace omitted that doesn't matter)

thread 'main' panicked at 'attempt to add with overflow', helix-term/src/ui/text_decorations.rs:136:16
 3: helix_term::ui::text_decorations::DecorationManager::render_virtual_lines
             at ./helix-term/src/ui/text_decorations.rs:136:16
   4: helix_term::ui::document::render_text
             at ./helix-term/src/ui/document.rs:166:17
   5: helix_term::ui::document::render_document
             at ./helix-term/src/ui/document.rs:79:5
   6: helix_term::ui::editor::EditorView::render_view
             at ./helix-term/src/ui/editor.rs:208:9
   7: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
             at ./helix-term/src/ui/editor.rs:1394:13
   8: helix_term::compositor::Compositor::render
             at ./helix-term/src/compositor.rs:170:13

I can reproduce this same panic for any inline diagnostic, seems like an edge case when the line that an error comes from does not fit on the screen. So the diagnostic would be the first line. But the first line rendered in this case would be line 27 which is technically row 2 so it has a visual_pos.row of 1 as well as nothing has been rendered last_line_pos.visual_line is equal to u16::MAX. because we are technically rendering the second line the check here passes pascalkuthe/helix@c0f5627/helix-term/src/ui/document.rs#L160 and as last_line_pos.visual_line is u16::MAX and virt_off is 1, tada it panic.

thanks for pointing this out @gabydd , this is an oversight from #5420. I pushed a fix to this branch but I will not backport the fix since it can really only occur if LineDiagnostics are used (so not on master).

I just gave this a try and see to get a whopping big inline diagnostic that's much bigger than the diagnostic itself:

@ocharles I think the LS was just sending unneded whitespaces. I made sure to trim the diagnostic message, I hope that fixes the problem. Otherwise please run helix with hx -vv --log foo.log <file> and upload the foo.log file here so I can see the messages sent by the server

[pascalkuthe]

For people using this with rust-analyzer I would recommend also merging https://github.com/helix-editor/helix/pull/6447 locally. That PR (mostly) fixes the problem of rustc/clippy diagnostics seemily moving around while typing which makes this PR a lot more usable and less noisy IMO.

[ocharles]

@ocharles I think the LS was just sending unneded whitespaces. I made sure to trim the diagnostic message, I hope that fixes the problem. Otherwise please run helix with hx -vv --log foo.log and upload the foo.log file here so I can see the messages sent by the server

Unfortunately, I still seem to get the same behavior when I build Helix from the Nix flake github:pascalkuthe/helix/786c8c8bc84deb462a938eba7e4c2dc166666a27 (/nix/store/6sllcggc10c6r7bxlw3cmy222qh87na0-helix-term-0.6.0/bin/hx, for other Nix users).

Does this log help? https://gist.github.com/ocharles/97bd9e85f696224b6cad83833409ee0f

[pascalkuthe]

@gabydd I actually need to revisit the case you mentioned. I fixed the crash but right now the diagnostics disappear. in the case you described which is a bit harder to fix.

[ocharles]

https://asciinema.org/a/7wYFdZrRQnZNPZBH1GtqVAB1A

Quite a lot of funkiness with diagnostics at the start of the file. I notice:

• My cursor sometimes vanishes
• Sometimes I just have blank space at the top of the editor
• The scroll margin is all over the place - sometimes there's no margin, sometimes 1 line, sometimes 3

[aral]

@pascalkuthe Just built from this branch. Will daily drive it with JS/HTML/CSS and let you know if I run into any issues.

First impression: thank you! 💕

[aral]

Seems navigating over an error message can make the line disappear. It appears to happen if the message/line is wrapped. (I cannot reproduce it with the same line where the line is not wrapped.)

In the series of screenshots that follow, I’m moving the cursor right one position each time:

When the line is not wrapped, it still jumps when I go from before the single quote to on the single quote but the line doesn’t disappear like it does when wrapped:

[aral]

Suggestion: how about using the curved box drawing characters instead of the square ones? It would lead the eye better without the sharp turn ;)

(Brace yourselves, GitHub does not render box-drawing characters properly. The curved version actually looks worse here but try it out in your terminal – or anywhere with decent rendering – to see how they should look.)

e.g.,

  │
  ╰─ This is an error.

vs

  │
  └─ This is an error.

Related characters (U+256D - U+2570):

╭  ╮ ╯╰

[aral]

Panic when navigating completion drop-down while LSP error is showing:

❯ RUST_BACKTRACE=full hx .

thread 'main' panicked at 'cursor must be in view', helix-term/src/ui/completion.rs:445:22
stack backtrace:
   0:     0x555b67118195 - std::backtrace_rs::backtrace::libunwind::trace::h32eb3e08e874dd27
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1:     0x555b67118195 - std::backtrace_rs::backtrace::trace_unsynchronized::haa3f451d27bc11a5
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x555b67118195 - std::sys_common::backtrace::_print_fmt::h5b94a01bb4289bb5
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:66:5
   3:     0x555b67118195 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hb070b7fa7e3175df
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:45:22
   4:     0x555b66729a7e - core::fmt::write::hd5207aebbb9a86e9
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/fmt/mod.rs:1202:17
   5:     0x555b67111b75 - std::io::Write::write_fmt::h3bd699bbd129ab8a
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/io/mod.rs:1679:15
   6:     0x555b67119b13 - std::sys_common::backtrace::_print::h7a21be552fdf58da
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:48:5
   7:     0x555b67119b13 - std::sys_common::backtrace::print::ha85c41fe4dd80b13
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:35:9
   8:     0x555b67119b13 - std::panicking::default_hook::{{closure}}::h04cca40023d0eeca
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:295:22
   9:     0x555b6711981f - std::panicking::default_hook::haa3ca8c310ed5402
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:314:9
  10:     0x555b6711a1df - std::panicking::rust_panic_with_hook::h7b190ce1a948faac
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:702:17
  11:     0x555b6711a044 - std::panicking::begin_panic_handler::{{closure}}::hbafbfdc3e1b97f68
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:588:13
  12:     0x555b6711869c - std::sys_common::backtrace::__rust_end_short_backtrace::hda93e5fef243b4c0
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/sys_common/backtrace.rs:138:18
  13:     0x555b67119d92 - rust_begin_unwind
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
  14:     0x555b66690e13 - core::panicking::panic_fmt::h8d17ca1073d9a733
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
  15:     0x555b66727bc1 - core::panicking::panic_display::he956d1fbfe1d4c76
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:72:5
  16:     0x555b66727b6b - core::panicking::panic_str::h7d5968eea59d06d2
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:56:5
  17:     0x555b66690c86 - core::option::expect_failed::h03d502d0855ade10
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/option.rs:1880:5
  18:     0x555b66c69408 - <helix_term::ui::completion::Completion as helix_term::compositor::Component>::render::h7e3fd09461b59e14
  19:     0x555b66c2ff67 - <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render::hb123e6d264055603
  20:     0x555b66f03fe3 - <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h1e3517fdf12708e3
  21:     0x555b66f09e66 - <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::haa12b3d77b6fcc0c
  22:     0x555b66f7f3e6 - tokio::runtime::park::CachedParkThread::block_on::h718d3f300d83cfd0
  23:     0x555b66fbdbc3 - tokio::runtime::scheduler::multi_thread::MultiThread::block_on::h36f4c8f36ed99c25
  24:     0x555b66fd92cb - tokio::runtime::runtime::Runtime::block_on::h7d8c7b1f527e43b8
  25:     0x555b66f1929f - hx::main::h43e63b540d765159
  26:     0x555b66fb0393 - std::sys_common::backtrace::__rust_begin_short_backtrace::h2caf6200adf9da96
  27:     0x555b66ef5d09 - std::rt::lang_start::{{closure}}::h628a60f555c4193f
  28:     0x555b6710d16a - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::hb69be6e0857c6cfb
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:283:13
  29:     0x555b6710d16a - std::panicking::try::do_call::h396dfc441ee9c786
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:492:40
  30:     0x555b6710d16a - std::panicking::try::h6cdda972d28b3a4f
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:456:19
  31:     0x555b6710d16a - std::panic::catch_unwind::h376039ec264e8ef9
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panic.rs:137:14
  32:     0x555b6710d16a - std::rt::lang_start_internal::{{closure}}::hc94720ca3d4cb727
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:148:48
  33:     0x555b6710d16a - std::panicking::try::do_call::h2422fb95933fa2d5
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:492:40
  34:     0x555b6710d16a - std::panicking::try::h488286b5ec8333ff
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:456:19
  35:     0x555b6710d16a - std::panic::catch_unwind::h81636549836d2a25
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panic.rs:137:14
  36:     0x555b6710d16a - std::rt::lang_start_internal::h6ba1bb743c1e9df9
                               at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/rt.rs:148:20
  37:     0x555b66f19398 - main
  38:     0x7fa237201510 - __libc_start_call_main
  39:     0x7fa2372015c9 - __libc_start_main@GLIBC_2.2.5
  40:     0x555b666d1da5 - _start
  41:                0x0 - <unknown>

[yuyidegit]

autocomplete always located in the upper left corner

[aral]

@pascalkuthe Just letting you know I’m building 850441b and will let you know if I run into any issues. Been happily running 786c8c8 as my daily driver (apart from https://github.com/helix-editor/helix/pull/6417#issuecomment-1507044887).

Unless it’s unstable, I plan on using it while I live code at NewCrafts Paris on Thursday (hey, why not live dangerously?) :)

[aral]

@pascalkuthe Ah, error straight off the bat with it:

Bad language config: unknown field `language-server`, expected one of `name`, `language-id`, `scope`, `file-types`, `shebangs`, `roots`, `comment-token`, `text-width`, `soft-wrap`, `auto-format`, `formatter`, `diagnostic-severity`, `grammar`, `injection-regex`, `language-servers`, `indent`, `debugger`, `auto-pairs`, `rulers`, `workspace-lsp-roots`
in `language`

Press <ENTER> to continue with default language config

(At launch.)

Reverting to https://github.com/helix-editor/helix/commit/786c8c8bc84deb462a938eba7e4c2dc166666a27 for time being.

Update: Ah, I see: https://github.com/helix-editor/helix/discussions/7091 – let me see how much work it is to update my configuration as I don’t have much time left before I leave for the conference :)

Update 2: OK, I didn’t have anything important in languages.toml; commenting it out for now and will continue testing your latest commit :)

[pilucid]

[image] autocomplete always located in the upper left corner

I investigated this in my local copy and the issue appears to be caused by the cached cursor position always being set to (0, 0). Commenting out the cache retrieval and always recomputing the cursor position allows the autocomplete popup to appear in the correct location. Obviously not a fix, but thought the info might be useful.

[pascalkuthe]

I already have a fix locally but I don't have time to keep working on this PR right now as its a lot of work and some private things came up that have to take priority

[aral]

@pascalkuthe Of course, Pascal. Hope everything is all right.

(If you do get a moment, could you possibly push the fix as it is? I can’t revert to the previous commit as it as force pushed so I have to run main otherwise and your branch is already so much better. No worries if not.)

💕

[pascalkuthe]

Yeah everything is alright just a ton on my plate rn.

I will push the fixes in the next couple days. They are on a PC I don't have access to right now

[aral]

@pascalkuthe Thank you so much for pushing your latest changes. Working like a charm again :)

(I can’t imagine using Helix without inline diagnostics now. Can’t stress enough how much better it is at preventing me from making compile-time mistakes. With the regular layout, not only do they overlap text but I end up ignoring errors most of the time.)

[pilucid]

Seconded. The changes in this PR are a huge workflow improvement. Thank you for your work.

[xJonathanLEI]

Been daily driving for a while and here are some initial feedback.

Inline diagnostics are extremely useful when diving into a specific issue. There's no doubt we need this. Thank you so much for always doing god's work!

However, not all LSPs are instant. For something like rust-analzyer that takes a few seconds to recompute the diagnostic list after saving, the text becomes flashy. Previously, this delay would only result in text style changes, but now they might actually move your cursors depending on where they are.

Even for rather fast LSPs it can still get in the way, especially when you're in the middle of typing out a line of code. The still happened without inline diagnostics but it used to be happening on the top right corner, so it was much more bearable.

Overall, the feature works great but I think we need a way to turn it off. We can either fall back to the existing top-right corner solution (easier), or make it such that users can view the details in a popup like they do for documentation (better but maybe for another time).

Or, just make it such that nothing happens when it's off. Users would have no way to view diagnostics at all (other than text style changes of course) until they turn it back on.

[pascalkuthe]

Here are already config options to disable this feature (or only enable it for the current line and you can also filter by severity so that only errors are displa'ed for example) and there is also the option to turn the old diagnostic display back on. For RA #6447 helps a lot btw.

I have been considering to add some kind of collapsed opinion to only display diagnostic next to the current line but that doesn't work well for long/multiple diagnostics. I could try to implement something like that that would spill over to inline diagnostic on the current line. I am not sure if that is useful enough to warrant yet another way to display diagnostics

[xJonathanLEI]

@pascalkuthe Thanks for the pointer! Now I have to daily drive with #6447 too lol.

I saw the SeverityFilter options but I just wasn't able to toggle that on the fly. I understand that this works in the config file:

[editor.lsp.inline-diagnostics]
cursor-line = []
other-lines = []

But my ideal workflow involves enabling it when I need to look into an issue, and turn it back off when I'm done. So it needs to be toggled on the fly. Somehow none of these commands work:

:set lsp.inline-diagnostics.cursor-line []
:set lsp.inline-diagnostics.cursor-line "[]"
:set lsp.inline-diagnostics.cursor-line \"[]\"

They all just show an error. My guess is that this is actually another bug that has to do with the set command though.

Thanks in advance for your time.

[pascalkuthe]

yeah that is a bug with the set/toggle command I noticed too. That should be fixed in a separate PR. Although I am thinking that the additional way to display diagnostics might be worth implementing after all and that could have a simple bool toggle (so you could do :toggle lse.inline-diagnostics.collapse )

[xJonathanLEI]

Got it. If there's no open PR for that I might take a shot. Should be rather easy to fix. With that I can have custom key bindings to emulate toggling it so it'll be good enough for me for now.

As for the additional way of displaying diagnostics, what do you have in mind? Falling back to the top-right corner when collapse is true, or yet another way that's more compact and never gets in the way?