Add formatted compiler message labels & children to diagnostics

[alexheretic]

Related to #649, #662 this pr adds CompilerMessage secondary .span & .children information to diagnostics. This really helps the diagnostics in being useful.

Atom

before

vvv

Vscode

before

vvv

With vscode it cuts off the message mid-word at the max width. Atom wraps properly. I think this is the responsibility of the client, as wrapping isn't too arduous. Even so the messages are quite usable in vscode and I'm sure people will appreciate them even cut in half.

Implementation notes

I've gone to some pains to avoid showing messages meant for different areas of the code by only including children-spans that are 'within' the primary span and secondary spans that are on the same line. There is some more work for generating additional diagnostics for the other spans.

Testing

Since there will be many compiler messages to get right I've added unit tests for my cases (which are fast / can scale to many tests). The compiler output generated by cargo with --mesage-format=json are stored in test_data/compiler_message/*.json files. We can add more cases as we discover suboptimal output.

[Xanewok]

Looks like we're getting our own ui-test suite!
LGTM in the current form and it's a definite improvement over what we had previously.

However, there's also another reason to consider using textDocument/hover (as per @nrc's suggestion) for displaying secondary diagnostics: it supports Markdown (which means we'll get some more formatting for free, that's already there), whereas diagnostic messages does not. Also it'd possibly mean less clutter in Problems pane in vscode.
On the other hand, we'll have to do more pre-processing in lowering analysis stage and ideally store the pre-formatted messages somewhere (or maybe just format them from rls-analysis data as we go?).

@alexheretic @nrc thoughts?

[alexheretic]

I'm not against showing this info via a hover response if that improves the ux.

I'm more keen to try and improve the feedback by generating more diagnostics errors/warnings. For example this error now includes the info that it was the second borrow.

But we're not showing the user where the first borrow is and where it ends. In this sense users are losing info by using rls over a terminal running cargo check.

error[E0499]: cannot borrow `string` as mutable more than once at a time
   --> src/lib.rs:134:24
    |
133 |         let _s1 = &mut string;
    |                        ------ first mutable borrow occurs here
134 |         let _s2 = &mut string;
    |                        ^^^^^^ second mutable borrow occurs here
135 |     }
    |     - first borrow ends here

[Xanewok]

I wanted to see if it was feasible to generate additional diagnostic per each secondary span, and I'd say the result is good enough (my branch):

Atom:

VSCode:

Each span, primary or secondary, has the compiler diagnostic message on top, then spaced out, is a label for each span (I found it most readable, personally) and on the bottom there are notes and whatnot, like before.
Used Information severity as it fits the use case best, IMO.

@alexheretic what do you think?

Here's the file I've been testing on:

use std::env;

fn takes_string(_: String) { }
fn takes_borrow(_: &String) { }

fn main() {
    // Case #1 (multiline)
    let string = String::new();
    takes_string(string);
    takes_string(string);

    // Case #2 (same line, multiple secondaries)
    let _shim = env::current_exe().ok().and_then(|path| path.to_str()).map(String::from);

    // Case #3 (primary/secondary overlap, secondary has "suggestions" -
    // actually it's only a label, with no machine-readable suggestions)
    match &Some(String::new()) {
        &Some(string) => takes_borrow(&string),
        &None => {},
    }
}

[alexheretic]

@Xanewok great stuff!

I've removed the heading from secondary spans that are within the primary (ie your 'cannot move out of borrowed context' example), fixed up the tests and added a test case for that.

[sebastiencs]

@alexheretic, @Xanewok Thanks for working on this !

@alexheretic I like the additionals diagnostics per each secondary span.
ATM (in your branch) there is no way to know to which error is linked an Information diagnostic, it would be nice if they could be "connected".
It would then be possible, for example, to display differents tooltips for the primary and all the secondary spans (for the same error) at the same time.

[alexheretic]

@sebastiencs They do have common headings so should in most cases be clear what they relate to.

What you suggest would be really nice, but I'm not sure the language server protocol can do it. The Diagnostic spec doesn't seem to have a linking concept and only accepts a single Range of code.

Perhaps this could be added to the protocol or there's another way. But first lets just try to do all that we can within the current protocol.

[sebastiencs]

Correct me if I'm wrong, but differents errors could have the same heading, no ?
I was thinking about the code field in Diagnostic, currently it is only the rustc error, but nothing in the protocol stops us from define it to, for example, E0300:N where N could be a number related to the error (which primary and secondary spans would share)

[alexheretic]

That's right, most of the clients seem to not use the code field. You could have custom logic in the client to highlight all equal code messages & make sure the server gives each distinct message a unique code.

But it would be non-standard client behaviour, requiring custom code in each client. I think if the functionality is desired it should be added to the protocol, that way the client libraries will naturally pick it up.

In any case shall we say it's out of this pr's scope, maybe raise a seperate issue here and/or on https://github.com/Microsoft/language-server-protocol after we merge this pr in?

[sebastiencs]

Yes sure, I see your point.
I thought it was something interesting to discuss before merging and without necessarily having it in the protocol

[Xanewok]

(...) there is no way to know to which error is linked an Information diagnostic, it would be nice if they could be "connected".

The exact discussion about linking the diagnostics can be found in #17. There are some proposals to extend the LSP for this and to see how that works out for us.

Thanks for updating the tests! I think we can merge this now.

One thing keeps me wondering is, how should this interact with show_warnings config option? The secondary spans and info should be relatively straightforward, sometimes it might be easy to be overwhelmed (e.g. when a simple change causes a chain of errors, each with multiple spans). Providing a way to also disable them seems like a reasonable idea, then again it's a core part of errors, which we always show to the user.

I'd go with what we have and see if the secondary spans really lead to too much noise - if so, we can implement a (separate to show_warnings?) config knob.

[Xanewok]

Once again, @alexheretic thanks for working on this!