editor: Add inline diagnostics feature

[davisp]

Closes #4901

This adds the ability to configure the display of diagnostic messages inline with code similar to how the inline git blame feature works. I'm fairly confident in the implementation in terms of performance/efficiency. I've not noticed any obvious slowdowns due to it even while scrolling quickly through large source files where I've introduced diagnostics to be rendered.

However, I'll be the first to admit that my UI design chops are slim to none. Which is to say I'm expecting to have requests for tweaking the actual UI which I'd be more than happy to integrate.

I'd also like to thank @nilskch for the initial UI mockup code. Having that was rather invaluable in getting this to an actual PR.

A few examples of the feature in use (the scrolling is much smoother than shown, the stutter is an effect of the GIF conversion):

Release Notes:

• Added configurable inline diagnostic support.

[nervenes]

Awesome work! Love this 👍

I'll just leave the link of my discord message for suggested opinions on some of the UI/UX concerns: https://discord.com/channels/869392257814519848/1106226198494859355/1320019106539241543

[ConradIrwin]

Assigning to @SomeoneToIgnore who knows most about Inlays.

Also needs @danilo-leal's input on aesthetics.

[SomeoneToIgnore]

Thank you for another stab at the topic.

I've tinkered with it for some time and can tell that this definitely feels more stable than the previous attempt, I guess due to more quick updates of the diagnostics, so we're on the right track.

The inline tooltip placement feels also better, as does not disrupt the line order, so I feel if we polish its logic enough and make the visuals better, we can merge this as a first iteration.

I have two groups of notes on top of this PR:

• "arhitectural" ones, related to how data for diagnostics rendering is created and stored, and various "when to update this" questions I've posted in the review comments below.

I think we can work on this right away without waiting for Danilo.

• presentation-wise, I'm not a good expert and leave to @danilo-leal to comment more here, but two important things I wanted to note:

1. There's no way to use this feature via the keyboard, which seems odd.

2. I've found the square and + icons more flashy and confusing than helpful.

I would propose to check out the solution Helix did:

https://helix-editor.com/news/release-25-01-highlights/#diagnostics
(notice the settings)

helix.mov

(single errors are not expanded, multiple errors are expanded on selections and have ... trimming overly long lines, no extra icons near the text, almost no flashing and very fast)

In case you're interested to have the file I've used for testing, to repro certain flashing issues:

src/main.rs

// #![allow(unused)];
use std::collections::{HashMap, HashSet};

fn main() {
  if true {
      loop {
          //
      }
      return;
  }

  // let foo = multiply()

  let long_params = LongParams;
  #[rustfmt::skip]
  {
      long_params.layout_visible_cursors(snapshot, selections, visible_display_row_range, line_layouts, text_hitbox, content_origin, scroll_position, scroll_pixel_position, line_height, em_width, autoscroll_containing_element, cx);
  }
  long_params.layout_visible_cursors(
      snapshot,
      selections,
      visible_display_row_range,
      line_layouts,
      text_hitbox,
      content_origin,
      scroll_position,
      scroll_pixel_position,
      line_height,
      em_width,
      autoscroll_containing_element,
      cx,
  );

  let aa = vec![String::new()];
  // testdasds_something();

  // let aa = Test();
  //� let aa = Test();

  let two = "string w";
  // let two = 2;
  // let two = 2;
  // let two = 2;
  // let two = 2;
  // let two = 2;
  // //√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // new
  // new
  // new
  // new
  // new
  // new
  // new
  // new
  // new
  // new
  // //√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
  // //√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√

  // let path = Path::new("/one/two");
  // dbg!(path.strip_prefix(path));

  let st = StructWithDeliberatelyLongNameToBreakThings;
  let mut test_map_identifier_that_is_also_long_to_try_things_more_and_more_and_more_again_because_we_need_a_wrap_agaaaaain
      // this goes after the hint
      = HashMap::new();
  test_map_identifier_that_is_also_long_to_try_things_more_and_more_and_more_again_because_we_need_a_wrap_agaaaaain.insert(st, "test");

  let mut my_random_integers = vec![42, 87, 13, 76, 29, 8, 97, 63, 52, 31];
  my_random_integers.sort_by(|a, b| a.cmp(b));
  dbg!(&my_random_integers);
  my_random_integers.sort_by(|a, b| b.cmp(a));
  dbg!(&my_random_integers);

  let mut my_random_chars = vec!['a', 'x', 'e', 'q', 'z', 'j', 'u', 'l', 'h', 'c'];
  my_random_chars.sort_by(|a, b| a.cmp(b));
  dbg!(&my_random_chars);
  my_random_chars.sort_by(|a, b| b.cmp(a));
  dbg!(&my_random_chars);

  if true {
      // panic!("((((");
      // loop {}
  }
}

fn multiply(one: i32, two: i32) -> i32 {
  one * two
}

///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
///√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√|√√√√√√√√√√√√√√√√
pub fn test_something() {
  //√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√√
}

#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
struct StructWithDeliberatelyLongNameToBreakThings;

#[cfg(test)]
mod tests {
  use super::*;

  #[test]
  fn test_name_1() {}

  #[test]
  fn test_name2() {}

  #[test]
  fn test_name4() {
      //
  }
}

trait DebugT {}

// impl<
// impl<T> DebugT for T {}

fn foo2<T>() {
  // let a = 1 ;
}

/// some

fn foo3<T>() {
  // let a = 1 ;
}

fn foo4<T>() {
  // let a = 1 ;
}

fn foo5<T>() {
  // let a = 1 ;
}

fn foo6<T>() {
  // let a = 1 ;
}

fn foo7<T>() {
  // let a = 1 ;
}

fn foo8<T>() {
  // let a = 1 ;
}

fn foo9<T>() {
  // let a = 1 ;
}

fn foo10<T>() {
  // let a = 1 ;
}

fn foo11<T>() {
  // let a = 1 ;
}

fn foo12<T>() {
  // let a = 1 ;
}

fn foo13<T>() {
  // let a = 1 ;
}

fn foo14<T>() {
  // let a = 1 ;
}

fn foo15<T>() {
  // let a = 1 ;
}

fn foo16<T>() {
  // let a = 1 ;
}

fn foo17<T>() {
  // let a = 1 ;
}

fn foo18<T>() {
  // let a = 1 ;
}

fn foo19<T>() {
  // let a = 1 ;
}

fn foo20<T>() {
  // let a = 1 ;
}

fn foo21<T>() {
  // let a = 1 ;
}

fn foo22<T>() {
  // let a = 1 ;
}

fn foo23<T>() {
  // let a = 1 ;
}

fn foo24<T>() {
  // let a = 1 ;
}

fn foo25<T>() {
  // let a = 1 ;
}

// test something too

#[test]
fn feature_1() {
  dbg!("111111111111");
}

#[test]
fn feature_2() {
  dbg!("222222222222");
}

struct LongParams;
impl LongParams {
  fn layout_visible_cursors(
      &self,
      _snapshot: &(),
      _selections: &[((), Vec<()>)],
      _visible_display_row_range: Range<()>,
      _line_layouts: &[()],
      _text_hitbox: &(),
      _content_origin: (),
      _scroll_position: (),
      _scroll_pixel_position: (),
      _line_height: (),
      _em_width: (),
      _autoscroll_containing_element: bool,
      _cx: &mut (),
  ) -> Vec<()> {
      Vec::new()
  }
}

fn long_params(
  _snapshot: &(),
  _selections: &[((), Vec<()>)],
  _visible_display_row_range: Range<()>,
  _line_layouts: &[()],
  _text_hitbox: &(),
  _content_origin: (),
  _scroll_position: (),
  _scroll_pixel_position: (),
  _line_height: (),
  _em_width: (),
  _autoscroll_containing_element: bool,
  _cx: &mut (),
) -> Vec<()> {
  Vec::new()
}

[SomeoneToIgnore]

I felt this description and the property name are somewhat confusing: could it be column_padding and "Number of columns between the right end of the line and the inline diagnostics information" or something different but also a bit more elaborate?

[davisp]

I've made the padding between the end of the line and the start of the inline diagnostic configurable. This setting has its docs updated to read as:

      // The minimum column to display inline diagnostics. This setting can be
      // used to horizontally align inline diagnostics at some column. Lines
      // longer than this value will still push diagnostics further to the right.

[joe-p]

I think a specific setting for padding, perhaps in addtion to min_column, would be useful. For example, "min_column": 0 + "column_padding": 5 would always place a diagnostic 5 columns after the end of the line regardless of length. "min_column": 50 + "column_padding": 5 will put all diagnostics at col 50, with longer lines being at length + 5.

[davisp]

Thats exactly where I ended up other than just calling it “padding”.

[SomeoneToIgnore]

Undo behaves quite oddly, apparently aggregating the lines into the same "group":

odd_grouping_on_undo.mov

[davisp]

Huh. How many times did you save in there (or did clippy update). I don't have a good theory for why the diagnostics would behave that way. Though I'm fairly certain it hasn't got anything to do with how I'm rendering them, just that its yet another example on why we should probably have a pre-processing step separate from rendering.

[SomeoneToIgnore]

How many times did you save in there (or did clippy update)

IIRC, only after duplicating the lines, I definitely did not save between undo/redo frequent calls shown in the late part of the video.

I presume this is related to DisplayPoint management and do not insist on fixing it right now, but needed to mention this.

[SomeoneToIgnore]

NOTE: I'm not entirely sure why this works, as display_point seems to be converted from a diagnostics range start, hence should be ok without any subtractions.

Overall, we're safer with multi buffer anchors user as coordinates, esp. if we start to store date between frames.

[davisp]

You and me both! All of this logic is basically the result of me squinting cross eyed at the various similar places I found and comparing type signatures of function arguments to get a conversion chain that appears to work. I have no idea how costly these calls are other than to say I've not been able to notice any lagging of the editor regardless of file size or diagnostic count (zero actual measurements here, I just haven't noticed anything).

That said, I've also not tested this thoroughly to see if loading up a buffer with tens of thousands of diagnostics would slow things down considerably. The only way I can think that it would is if there were many thousands of diagnostics that were somehow all visible in a single on screen view of the editor.

[davisp]

@SomeoneToIgnore Thanks for the review!

I won't be able to get to all of your points until tomorrow or Sunday, but its definitely on the agenda for this weekend.

I did spend some time last night reading through the Helix implementation and also played a bit with it to see how it'd behave in various situations. I'd say my two main take aways from this are that we should absolutely steal their UI mechanics for interacting with the diagnostics and that I'm absolutely not going to invest the time into reimplementing their diagnostic renderer.

I've been using this branch full time for over a week now and have to agree that the hover UI and the square/+ indicators are rather useless. In day to day work I just haven't found any use from the indicators. Just knowing there's an diagnostic to look at is more than enough. Though I do think we should probably add an option for highlighting the entire line to make the diagnostics easier to spot while skimming code during refactoring as its easy to miss things on lines nearly as wide as the editor (or soft-wrapped lines as you spotted, oops).

For the actual diagnostic rendering, I'm not about to follow the Helix path to implementing a custom algorithm. I was hoping they'd just figured out how to get rust-analyzer to given diagnostics with more/better structure/mark up, but instead they went all Sisyphus and wrote their own from scratch (which is fairly impressive, not gonna lie). For instance, here's one random quirk that I found playing with your test file:

Notice that they've rendered right-to-left as top-to-bottom with overlapping lines so its confusing what goes where. Making that work reliably in Rust would be hard enough. Making it work reliably for every language/LSP pair is my personal brand of nightmare fuel.

Covering a few other issues at a high level, there's definitely some funkiness with diagnostics that change or don't on user input. Some diagnostics are instantaneous (i.e., syntax errors) while other things like unused imports only update when clippy runs which obviously depends on the project size. I definitely think there's room for improvement here and you made a number of suggestions that I think will help quite nicely on that front. I'll have to contemplate the ideas on storing more state between frames, I certainly agree that there's some work that needs to be done there, I'm not entirely sure I see how that would work (though if you have a pointer on somewhere in the code base I can reference that'd be helpful).

One last thing was that bit in the flickering example with diagnostics changing on every keypress. What I think you're calling flickering is the fact that the diagnostics are rendered ~instantly, but with differing content so they're changing oddly. I can completely understand why we'd want to avoid that in a UI, but I just wanted to make sure we're on the same page as to the underlying behavior. As near as I can tell, the diagnostics are being rendered ~instantly, its just that the what is rendered is changing so they're going back and forth. I couldn't immediately reproduce this (I'll try again when I dig further) which makes me worry that its a rust-analyzer issue that we'll have to code around. Not the end of the world but not great either.

Also on the topic of performance, you mentioned Helix's speed of rendering. In my (very light) poking, its significantly slower than this PR. I was getting roughly 100ms delays (very unscientifically measured) for the inline diagnostics. No idea if that's just my machine or something with your All The Diagnostics test file.

[davisp]

That's probably it for me for today. I've made some progress and I think responded to all/most comments with hopefully something at least semi rational. Please, do let me know if this comment is a correct summary of what you're suggesting for the architectural changes. I'm pretty sure that I'm reasonably close after reading the entire review a couple times now. And it certainly makes sense.

I didn't quite get to the cursor selection logic so will try and get that knocked out tomorrow and have this PR updated with at least what the final version will look like rendering even if we do go back and update the implementation to be more efficient.

Also, I mentioned it a couple times, but the "select nearest diagnostic" logic I was referring to is this bit that I saw in the hover_popover code. I'm mostly linking that for my own benefit so apologies if that was already obvious.

[SomeoneToIgnore]

Thank you, I've restrained from commenting or resolving things too eagerly as there was no new code pushed, so even the things obsoleted are still hanging in the review.
Feel free to ping me more explicitly if I've missed something important, otherwise I'll try to do minimal activity here until the next code batch arrives.

To me, makes total sense to reuse the existing infra and avoid chasing Helix approach to every detail, that example served as an illustration to two crucial things to me:

• less visual noise
• better keyboard flow

Seems that we agree on both, so it's great for the first iteration.

[davisp]

@SomeoneToIgnore The updates from today include the new UI approach for using the active diagnostics to display what ever is under the cursor or optionally using an action to toggle the active diagnostics. This feels a lot more useful than the hover approach and has the nice benefit of being significantly more modular.

A short probably incomplete list of changes include:

1. The "are inline diagnostics enabled" is now a single boolean that is updated when the settings.json is edited
2. Allow users the ability to activate a diagnostic block based on the cursor movement (does not require inline diagnostics)
3. Allow users to bind a key to toggle the active diagnostic under the cursor (does not require inline diagnostics)
4. Allow users to the ability to show the rendered diagnostic, if it exists (does not require inline diagnostics)
5. Allow users to disable non-primary diagnostic activation (does not require inline diagnostics, non-primary aren't super useful in conjunction with using the rendered diagnostic).
6. Removed all hover behavior and indicator icons

This update does not include changes for the optimized architecture we've discussed, but I'll be working on that either in the evenings this week or perhaps next weekend depending on how real life shakes out.

[SomeoneToIgnore]

Thank you, I really like the presentation of the current hints — zero mouse involvement is so natural, and selection-based reveal seems great, especially the fact that it's so close to existing f8/shift-f8 that even ESC-to-hide and copying works with it.

Besides the recompute-during-render story, a long line bug and new, selection-related issues that I've commented, it seems quite pleasant to work with.

Thank you for keeping with this — there's quite a chunk of work to do still.

[SomeoneToIgnore]

Suggested change

	// "delay_ms": 0,
	"delay_ms": 50,

I would propose a small delay here, as otherwise the diagnostics might update quite quickly during rapid typing.
With some debounce, it will be less flashy which we prefer overall (see inlay hint defaults for example).

[davisp]

Absolutely agree. I didn't bother addressing this yet solely because this will be debounced on the "update inline diagnostics to render" step we have planned for the more efficient implementation. But my general plan is that this setting will be re-purposed for the whole "only recalculate diagnostics after $debounce_ms" logic exists.

[SomeoneToIgnore]

This 6 is somewhat confusing.
Is it a default? Then we can uncomment the value.
Is it a minimum allowed? Then we can explain this fact.

[davisp]

This is absolutely just me not having great UI/UX experience. The unit I ended up with is "columns" which was easy to implement and (to me at leasts) easier to reason about than whatever an "em" is. I'm totally on board for a better name/unit combination for "amount of white space between the end of your source code and the start of the inline diagnostic" which is all this is for.

[jansol]

"em" is the width of the "M" glyph. In practice you can mostly think of it as equivalent to a "column", although you should probably still base your calculations on the em_width() function used e.g. in the inline git blame so that the padding scales with font size in the same way as elsewhere. In general I'd crib as much as possible from the inline git blame label anyway.

(you may have heard of something called "em-dash", which is a dash that is 1em in length, i.e. "—" as opposed to the shorter minus sign "-" and the "en-dash" "–")

[davisp]

Huh! Today I learned!

FWIW, this is the code that currently converts a number of columns to pixels which is what we need for drawing.

[SomeoneToIgnore]

I'm actually somewhat confused by this, as we have

zed/assets/keymaps/default-macos.json

Lines 377 to 378 in 1467d8b

	"f8": "editor::GoToDiagnostic",
	"shift-f8": "editor::GoToPrevDiagnostic",

that seem to do the work better:

f8.mov

they may be triggered at same places as ToggleActiveDiagnosticAtCursor and cycle through all applicable errors at the same range — the new action only shows a single one for the same range, and in the end of the video, the new action does quite odd things by replacing the diagnostic — it resets my diagnostics selection to the a particular one always, instead of cycling through them.

Also, I notice that selections and multiple carets are not treated fairly: I'd expect all places touched to open their diagnostics, yet I get a single diagnostics all the time instead:

selections.mov

I wonder, is it really worth it to fix such bugs for the new action, or should we stick to the old ones for this task instead?

What I really would find useful is the way previous action worked, that toggled all inline diagnostics on and off for a particular editor.
We have something similar for inlay hints already:

and such toggling might enable a different way of using the inline diagnostics: instead of changing the settings, users can turn them off the by default, and use a keybinding to toggle noisy visuals on when needed.

[SomeoneToIgnore]

Also, unfortunately, it seems that we need to special-case f8/shift-f8 case, as it starts to break with "update_with_cursor": true,:

stuck.mov

Seems that only particular lines cause that, though, so should not be hard to catch this bug.

[davisp]

Quite a few things going on in these two comments and I'm still contemplating all of them. But I think there's a few top level things I can comment on directly:

Toggle vs Next/Prev are two distinct concepts for me. The existing Next/Prev behavior can be useful, but I find is rather distinct from the "What is this one?" that I find myself needing more often. Specifically, if I'm working on something and I get a diagnostic, its generally either "Yes, I see my typo, I'll get to it in a second" or "Wait why is rustc/analyzer telling me I'm wrong?". Where as the Next/Prev behavior is "I changed something, now I gotta clean up all the places affected by my change".

I wonder, is it really worth it to fix such bugs for the new action, or should we stick to the old ones for this task instead?

I've contemplated this as well. I don't use F-keys naturally and haven't (yet) bothered remapping the next/prev diagnostics. However, I do think there's still a material difference between "jump" and "what is this" so I'm not 100% on whether that'd be doable.

What I really would find useful is the way previous action worked, that toggled all inline diagnostics on and off for a particular editor.

I'm not gonna actually link you the carseat girl giving side eye meme, but I'm not gonna not tell you I didn't want to. Having the inline diagnostics setting be a default with the cmd-shift-d toggling them per-editor is something I've already removed from this PR so perhaps I'm missing a subtlety here?

and use a keybinding to toggle noisy visuals on when needed.

Just to re-iterate, the entire point of inline diagnostics is that they're "noisy visuals". Think of it like the preference between insisting on -Werror and "Well, it compiled, hopefully the compiler spewing megabytes of warnings isn't important".

To be clear, this is exactly why I was fine removing the default/per-editor-state difference. I rock this in all languages all the time because the entire point is "there should be no diagnostics reported" as a default. If my automated tooling is telling me there's an issue, it means I need to address it. That might mean disabling a lint or whatever, but it at least drives specific decisions.

Also, unfortunately, it seems that we need to special-case f8/shift-f8 case

I'll look harder when I get into actually working on this but I'm not surprised. I leaned into the existing diagnostic machinery so its rather probable the new settings changed things in wonky ways I haven't thought of.

[SomeoneToIgnore]

• Can we use a diagnostics level here?
  LSP defines that there are 4 types of these: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticSeverity and a single boolean cannot cover them all.

I think it's better to be flexible and allow users to show all or almost none of them, if the want to.
We could also move that into inline part of the settings?

• If we do that, I wonder how useful primary_only would be: even now it seems redundant, have you found it useful?

I feel that we remove use_rendered unless you have a good reason to leave it be, if we remove primary_only, we'll have only 3 settings knobs: on/off, diagnostics level, and diagnostics interactivity.
That set looks quite small and good to me.

[davisp]

To be clear, the include_warnings setting has nothing to do with this PR other than I have decided to just re-use the existing top level diagnostics key in the settings because I was tired of trying to come up with a better name.

For the level filtering part of this, I'm on board with the end goal, but I'm not focusing on it precisely for the same reasons I referenced with -Werror. For me, this is a "Thou shall not pass (while diagnostics are active)" sort of default. Allowing other folks to ignore diagnostics is fine, but its the sort of thing I'll focus on once we get to the polishing steps.

I'll discuss the primary_only/use_rendered stuff in separate response.

[SomeoneToIgnore]

This explanation is recursive, and I have to go into the code, as I did not understand what does it mean.
I suspect it will be worse for regular users, can we describe a bit more elaborately this and all new knobs above?

[davisp]

Yeah. You caught me (kind of). Exposition in the next reply.

[SomeoneToIgnore]

Can you elaborate on why is this needed?
Settings description is quite sparse.

From the LSP standpoint, this is a crime and sort of a spec violation: only a particular version of the language server knows what's inside a particular pub data: Option<Value>, is.
It is meant to be used by that server only, usually for resolving more data.

There are no guarantees that this field contents won't change or even exists in other servers, so exposing it into our settings looks very odd.

[davisp]

Can you elaborate on why is this needed?

Absolutely.

Here's the current default:

Here's with use_rendered:

Zed already introduces its own primary/non-primary distinction (including keeping the rendered version on all of them, hence the primary_only setting) which I take to mean that we're willing to play within the major LSP decisions on what to use, especially when other LSP's aren't using this particular area.

From the LSP standpoint, this is a crime and sort of a spec violation

No, the crime is that LSP protocols haven't kept pace to be able to take advantage of the amazing information density provided by these diagnostics.

Also, it works with all the other versions when they haven't got a key there. I have been using Python, Go, Rust, and C++ and these config options just allow for Rust's messages to be included while not penalizing anything else.

[SomeoneToIgnore]

I agree that the latter works and looks much better, but still think that parsing arbitrary data json is not a good approach.

https://rust-lang.zulipchat.com/#narrow/channel/185405-t-compiler.2Frust-analyzer/topic/Better.20Diagnostic.20Rendering.20in.20Zed had noticed a better approach that might be available this year even? (hard to make predictions knowing how slow LSP is developed)

Looks like LSP 3.18 will have a textDocument.diagnostic.MarkupMessageSupport capability.
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#diagnostic

microsoft/language-server-protocol#1905

Does anyone know whether VsCode tends to get updated to support new LSP capabilities?

Feel free to leave this bit in if you disagree, but please be aware that I will remove it from our codebase, one way or another.

[davidbarsky]

To surface some discussion from the Zulip thread, I think the rust-analyzer team would be okay with either of the following options:

1. Using textDocument.diagnostic.MarkupMessageSupport. It seems like a pretty 90% of a fit, but I don't know if overloading Markdown is the right move. It might very well be, though!
2. Adding an LSP extension inspired by, but independent of, LSP 3.18 to render errors. As Lukas said, "If you come up with an extension, we'd happily implement it (or merge an implementation)".

As to which option is better, I imagine that y'all know better than I do.

[SomeoneToIgnore]

Yeah, I can follow either of these paths later, we do not need to bloat the PR.
If you want to, let's leave the data access in this PR, and I will try to make it different later — still, wanted to try and prevent this from getting into main.

Thank you for keeping this all afloat meanwhile.

[SomeoneToIgnore]

This is the best part of the impl to me, honestly, I wonder if we could

• move it into inline as this settings knob is related to this part of the functionality only? (apart from the fact that it spoils f8 navigation sometimes 🙂, see another comment )
• enable it by default, and explain that when disabled, same effect could be achieved with f8/shift-f8 or the new action (if we decide to leave it)

[davisp]

I'm absolutely on board enabling by default, but not at all on board moving it to the inline settings. I actually think this almost a separate feature because its so useful for the whole "what's that squiggly line" sort of interaction.

I'm suddenly wondering if I don't have a bug here. I very much had a few because it was originally tied to the inline diagnostics implementation but I thought I removed the dependency there. Regardless, I definitely agree that this particular behavior is probably something we should add regardless of the inline diagnostics aspect.

[SomeoneToIgnore]

Multiple carets (alt+click, for example) and a chunk of text selected do not reveal multiple diagnostics, only the "last" one is.

I think we want to reveal anything that any selection touches or includes instead.

[SomeoneToIgnore]

On a similar thought, should we expand all diagnostic groups that are related?
Both rust-analyzer and rust can send diagnostics, and current version always shows the ones from rust-analyzer to me only.

[davisp]

I have no preference here. I'm personally good with just the one at a time because that's how I use diagnostics. Given where I think things are going I can't fathom a reason we can't find a decent solution to these issues though so I'm more than willing to do whatever is requested by the powers that be in this instance.

[davisp]

@SomeoneToIgnore Thanks for the in-progress re-review on this! Reading through I think we're mostly in agreement on most everything with what feels like fewer open questions. I'll probably not have time to get to more in-depth work on this until Sunday/Monday when I've not got $real_job commitments but so far I think we're coming to a consensus on behaviors so that seems like good progress.

I'm gonna respond to most/all of your comments just to make sure that we're all on the same page for when I get a chunk of time to focus on this. So apologies for the incoming email spam.

[davisp]

I have no idea why GitHub isn't giving me a reply box to this comment about the relative performance traces in Instruments. However, given that we're already planning on optimizing things I'm not sure how much to focus on it.

[SomeoneToIgnore]

I'm not sure how much to focus on it.

I'm almost sure that if we follow the "update the rendering data after debouncing, in the editor, on the DiagnosticsUpdated/other specific event", we're good without any specific optimizations.

[davisp]

@SomeoneToIgnore New update. Highlights include:

1. Remove everything that's not purely just showing diagnostics inline
2. Move all coordinate transformations out of the Element::prepaint method
3. Fix the wrapped line display bug

I've been using this as a daily driver and the automatic diagnostic jumping turned out to be terrible. I've written Rust, Java, Python, and Go and the only place it was remotely useful was in simplistic Rust cases. All the other languages I wrote in would just show the same simple message which just led to a lot of code bouncing around.

I'm pretty sure I've got the heavy parts you were worried about moved out of the painting code paths. The only math that happens while painting is some simple arithmetic to account for element layouts calculated earlier in the painting pipeline.

And lastly, I've got the long wrapped line logic figured out as best as I can. There's still the issue where the line just barely fits in the window without wrapping, but it matches the behavior of the inline git blame so I figure its good enough for now.

[nilskch]

Suggested change

	.bg(sev_to_color(&diagnostic.severity).color(cx).opacity(0.07))
	.bg(sev_to_color(&diagnostic.severity).color(cx).opacity(0.07))
	.px_1()

I really like how this feels now! Can we add a small padding here? I think that makes it a little bit nicer.

Before	After