Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Append the label value of the primary spans of diagnostics to the LSP diagnostics message if it is available. #447

Closed

Conversation

nelsonjchen
Copy link
Contributor

This is very useful in diagnostic messages where the label has specific
diagnostics information for the site and little in the message. The most
popular message and an example is detailed as follows:

In cases like E0308 or better known as "mismatched types", the message
is simply "mismatched types".

Users seeing "mismatched types" in their editing buffers would have had
to immediately step out of their buffers and run whatever compiles the
line in their terminals to take a look at what is described here as the
label in order to figure out what and how the types are in conflict.

Related: rust-lang/vscode-rust#103

@nelsonjchen
Copy link
Contributor Author

Not sure about tests.

Oh, and I guess here's a small screenshot.

untitled

@nelsonjchen
Copy link
Contributor Author

And here's a little bit of TS LSP's own one.
untitled2

@Xanewok
Copy link
Member

Xanewok commented Aug 20, 2017

This is great! I think we regressed at some point, so that's great to see it back. I know I found myself running cargo check manually to know the exact types in such situations 😄

Looking at the test_borrow_error, the test currently expects a "message":"cannot borrow x as mutable more than once at a time", however RLS returns "message":"cannot borrow `x` as mutable more than once at a time\nsecond mutable borrow occurs here". Just fixing the test is easy (either we shouldn't expect the last " or we should expect the whole message).

However, I'm not sure if appending label in case of every error is desired. The message second mutable borrow occurs here will not be useful in this case, because the user will be confused as to where's 'here' and RLS doesn't seem to be using secondary spans yet for the diagnostics from what I know.

@nelsonjchen
Copy link
Contributor Author

@Xanewok , thanks for the comment. This is indeed a bit heavy handed. I was kind of curious what would have gotten caught by that. I did this in the dead of night, frustrated at rust-lang/vscode-rust#103.

Hmm, not sure why that test passed on my Mac. It seemed really weird for it to pass and I usually intentionally break areas to understand what tests touch that area. Nothing broke, so I just plowed forward. It's a bit embarrassing for me to put a PR with a broken test out the gate. Anyway, I'll take a closer look.

@nelsonjchen
Copy link
Contributor Author

Ack, I was a bit too tired to realize I didn't actually run the test after the changes. That sequence of events is important.

… diagnostics message if it is available.

This is very useful in diagnostic messages where the label has specific
diagnostics information for the site and little in the message. The most
popular message and an example is detailed as follows:

In cases like E0308 or better known as "mismatched types", the message
is simply "mismatched types".

Users seeing "mismatched types" in their editing buffers would have had
to immediately step out of their buffers and run whatever compiles the
line in their terminals to take a look at what is described here as the
label in order to figure out what and how the types are in conflict.

Related: rust-lang/vscode-rust#103
…pans

The message is a bit useless at the moment but LSP does not have a good
way to do multi-span and it might be best to leave this mildly useless
message until they do so instead of special-casing it to not appear.

Related: microsoft/vscode#1927
@nelsonjchen
Copy link
Contributor Author

nelsonjchen commented Aug 20, 2017

I fixed the test. I'm not sure what to do about the story where there are two spans. Should I modify this to only append the label if there's just a singular span and punt the two span issue until the LSP/VSCode issue below is resolved? Or is this minimal logic good enough to get RLS more immediately useful for the moment?

As for properly displaying two spans, the multi-span story is a bit not developed at the moment. It doesn't appear LSP has a way to display that at the moment. Here's the related issue in VSCode:

microsoft/vscode#1927

Test for if the compiler message *AND* how the mismatched types failed
expectations are included in LSP's diagnostic message.
@nelsonjchen
Copy link
Contributor Author

nelsonjchen commented Aug 20, 2017

This might belong in another issue but a VSCode/LSP developer took a look at this situation nearly a year ago and suggested it might be just best to add more strings of messages like this to the LSP Diagnostic message with regards to the multiple span issue.

IMO I just wanted the primary span's label for now for the mismatched types issue. It would be a bit of scope creep to try to add more than that.

@nrc
Copy link
Member

nrc commented Aug 20, 2017

Hey, thanks for the PR! So, we used to do this and I deliberately changed to not doing it because there seemed to be more cases where it gave bad information and it meant that the errors in the 'problems' panel were a pain to read. Unfortunately there seems to be no good way to fix this. My plan for a 'least worst' solution to this is to record the extra information for each error (and the affected span) in the RLS and then return the info as part of the textDocument/hover request (rather than with the diagnostic info). This means that at least the 'problems' panel remains usable.

@nelsonjchen
Copy link
Contributor Author

Ah yeah, this is indeed too broad. Still, it would be nice if E0308 gave a nice message like Typescript's.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants