Add support for children in Diagnostic object for complex errors

[trixnz]

While working with languages like Rust with rich information in its errors, it's become apparent that not all of this information can be conveyed cleanly with the current Problems window. Take, for example, this error:

warning: this `if` has the same condition as a previous if, #[warn(ifs_same_cond)] on by default
 --> src\main.rs:6:15
  |
6 |     } else if a == b {
  |               ^^^^^^
  |
note: same as this
 --> src\main.rs:4:8
  |
4 |     if a == b {
  |        ^^^^^^
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond

The note referencing where the first condition was used would be helpful to the user, but we must omit it because the error list is sorted on the Range of the error, resulting in the note being sorted before the actual error:

While the above example doesn't look too ambiguous, when there are several errors spread out through the window, it quickly becomes extremely difficult to figure out what relates to each other.

If Diagnostic had a children: Diagnostic[] field where we could add additional context around the error, we would be able to convey more information to the user about the events that led to the error/warning.

[dbaeumer]

The same as this is IMO not a separate error / problem. The solution to this should be a more complex message instead of a diagnostic tree. @sandy081 what happens with multi line messages today in the problems view.

[sophiajt]

When @nikomatsakis and I were working on the new errors, one of the ways we envisioned them possibly working may be close to what @dbaeumer (also, hi Dirk! long time no see!) is suggesting.

As a first step, I think a diagnostic tree is an okay fix. But I think ideally (and apologies I don't have a mockup so you'll have to visualize), is something like this:

• Errors/Warnings are squiggled, but notes aren't shown
• When you click on an Error/Warn squiggle, you get a popup that is like peek. It shows you the rest of the notes that are important, but otherwise these are hidden

@dbaeumer - does vscode support lightbulbs or additional info notifications? Maybe this is something where the lightbulb could pop up if there are additional notes.

[dbaeumer]

@jonathandturner (Hi Jonathon, indeed long time no see. See you have fun with rust now) Yes, there is support for a lightbulb in VS Code. Have a look at the code action API. This allows you to provide code actions for every position in the document. We then show the light bulb with the corresponding actions to run.

The peak UI for errors is already there. You can pressed F8 in the editor to activate this.

[sandy081]

Long messages are not shown completely in the problems view. A partial message with a ... is shown at the end. Complete message is shown as title (on hover). For more information refer to this feature request - #1927

I would make this a dup of #1927 if this is only about showing multi line messages in the problems view.

[trixnz]

@sandy081 I don't think this issue is necessarily about multi line messages. Consider the following error:

error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable
  --> src\main.rs:29:20
   |
27 |     let y = &mut x;
   |                  - mutable borrow occurs here
28 |     *y += 1;
29 |     println!("{}", x);
   |                    ^ immutable borrow occurs here
30 | }
   | - mutable borrow ends here

Presently there doesn't appear (unless I am mistaken) to be a way to add squiggles to lines 27 and 29 without adding a new diagnostic entry for each, which would be ambiguous when there are other errors to look through. As a result, we can only add the cannot borrowxas immutable because it is also borrowed as mutable line to the diagnostic list.

Allowing for children in the tree (or an alternative means) would allow us to highlight the two borrows which would lead back to the original error. As I understand, the lightbulbs are for code actions and wouldn't really fit the context here as there is no action to be taken.

[sandy081]

@trixnz In the above example, is not it enough to add a single diagnostic entry to line 29 providing a detailed message and mentioning where mutable borrow occurred. I do not think we need to add a squiggle or a child entry for line 27. IMO line 27 is a reference that has to be made in diagnostic entry of line 29.

[kumarharsh]

+1 - the Flow parser also returns errors in two places - one in the React component which was used, and one in the place where it was used. Right now, the flow plugin shows the error in the first place, but not the second, which is rather pointless. It'd be very helpful if VSCode did have this functionality.

[jrieken]

Fair request and I believe child-diagnostics is powerful to cover various scenarios... Biggest challenge is the slightly bogus design of today-diagnostic, esp. because it doesn't have a location but depends on the DiagnosticsCollection... That needs some careful refactoring.

[jrieken]

@trixnz @jonathandturner Now that we have the API, what UI expectations do you have this?

• For the problems view we have child nodes in mind (which I think is pretty obvious).
• In editors would you expect squiggles to be shown for related diagnostics (I tend towards no).
• In hovers and F8 do you expect a concatenation of the messages with a source link or more?

For the latter in might be cool to actually preview the source code (line) unless the message itself already contains a preview...

[sophiajt]

Let me pull in @nrc who is leading the Rust dev tools team and is a good point of contact. They're currently working to push the Rust Language Server work to 1.0, and getting some polish into the VS Code experience for Rust would be an awesome bonus.

[nrc]

I'll describe what feels like my ideal UI, I have no idea about the implementation difficulty though:

In the problem view, only 'primary' diagnostics are displayed, those with related info can be expaned to toggle display of related info:

  error message with no related info
> error message with related info
  error message...

on-click:

  error message with no related info
v error message with related info
    related info part one
    related info part two
  error message...

Only 'primary' diagnostics have squiggles, however, if a primary diagnostic is hovered or clicked in the problem view then squiggles are also shown under the related locations. While the user continues to interact with a primary or related diagnostics, all diagnostics related to that primary are squiggled. Likewise, interacting with any primary or related diagnostic in the problems panel causes all primary/related diagnostics in the group (with the same 'parent' diagnostic) to be squiggled.

I would not expect to concatenate related info to the hover, but I would expect some indicator that related info exists (some highlighted ... or something). On F8, I would love to have something like the 'find all references' panel, so the primary diagnostic message is delayed first then there is a list of related diagnostics with 'peek' views of the code.

For Rust, we never include code in the diagnostic messages directly, we keep spans for all relevant locations and then synthesize the code preview into the error message when it is rendered. So you wouldn't ever need to worry about displaying code twice.

[sophiajt]

Just to throw another possibility onto the pile that might be worth exploring:

For the error messages with multiple locations, like @nrc points out, the primary one is the squiggle. If you click or hover the squiggle, maybe you can get peeks that show off the other locations. I figure a peek (or multiple peeks) might work best since you may not have all the other locations on the same page of code and may require you to jump back and forth.

[kumarharsh]

Great news! I think it'd also be helpful to take in flow-for-vscode team (@orta ?) to share their requirements, as Flow also has quite a verbose multi-line multi-file diagnostic output.

[sandy081]

Will start exposing this in the Problems Panel tomorrow.

[orta]

I've not really used Flow in a year, and since then there's been some amazing improvements on how they report errors - so I'm going to tag in @calebmer who improved them.

[sandy081]

This is how diagnostics with children (relatedInformation) is shown in problems panel

[sandy081]

Another version

This looks simpler and cleaner and also aligned with F8. Hence, I would go for this.

[jrieken]

Some early work to start the discussion on how to present related diagnostics in F8. The upper parts is what you have today (the main message) and the lower part is navigable locations and their corresponding message.

[sandy081]

Another version closer to what @jrieken has for F8

@jrieken Opinions?

[sandy081]

@jrieken @kieferrm

I set up a rust example

Problems generated by the Rust (rls) extension

Problems with related information generated by my test extension

[nrc]

@sandy081 that looks great! It would be really nice if we could squiggle under let x = 5 while this message was shown and/or display a snippet of line 2 in the panel (this might be useful if the message refers to code that is not currently visible, which does often happen).

[agauniyal]

The squiggle on related line might not be visible if the lines are too far apart to be visible in the same viewport, so if only related line is visible then it might look as if let x = 5 has the error instead.

[sophiajt]

@agauniyal @sandy081 - can we show the squiggle in the peek window, rather than the (2, 9)?

[agauniyal]

Maybe peek window can contain a link to the related line(I've seen this behavior with codelens)?

[nrc]

The squiggle on related line might not be visible if the lines are too far apart to be visible in the same viewport, so if only related line is visible then it might look as if let x = 5 has the error instead.

Is it possible to see the f8 panel without starting on the main error? If the user has to scroll to only see the related squiggle, then it doesn't seem too bad. Perhaps we could also use different colour squiggles to clarify?

Maybe peek window can contain a link to the related line(I've seen this behavior with codelens)?

This would be great!

[jrieken]

I'd like encourage to continue the discussion about F8 and related diagnostics in #46562 because this issue has become quite large already. I am also going ahead and I am closing this one so that we can test and ship the first versions of the various pieces involved (API, LSP, problem panel, hover, and F8)