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()
}

[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.

[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.

[RafayAhmad7548]

i am waiting really hard for this to be merged, aaa

[odicho]

Sorry for the unrelated comment, but this PR is amazing. Great work from everyone involved 👏🚀

[SomeoneToIgnore]

Thank you, by stripping most of the topics that caused discussions, we've got a very solid first step, I expect this approach, modified slightly, will land as the first step for the diagnostics.

There's some work to do as, despite getting smaller, it's still a very dynamic hence complex part.
My feedback comes in these main topics:

• NITs about various ways to structure the code
• coordinates (Anchor <-> DisplayRow caveats)
• interaction with settings
• layouting with other elements

Last two items could be most interesting to fix, if you have time this week + a few days after, but hopefully starting next week, I'll be able to help to move this PR forward and push whatever ceremonies around the coordinates and related fixes along the way.

I think we're past the point of design concerns and this indeed looks quite close to be merged, time to think about the design, so we also plan to pair with @danilo-leal on a design part next week, welcome to work on this together, if you're interested.

[davisp]

@SomeoneToIgnore Just acking your latest round of review. That all sounds reasonable to me. I should hopefully be able to find some time later this week or this weekend to push through those changes.

@danilo-leal I tend to be fairly aggressive in rewriting my git commits. I'll try and keep your changes separate and on the tip, but if you plan on making any extensive changes give me a heads up and I'll avoid changing anything out from underneath you.

[danilo-leal]

@davisp no worries! Kirill & I were pairing on this earlier and went ahead and pushed a tiny change. Shouldn't break anything here.

[SomeoneToIgnore]

A small update: I've tried to push commits on top of this branch and got rejected for some reason.
So I've created https://github.com/zed-industries/zed/compare/inline-diagnostics-2?expand=1 and will push fixes there instead, gradually resolving the comments here.