Rich mapping of cargo watch output #1439
Conversation
Currently we depend on the ASCII rendering string that `rustc` provides
to populate Visual Studio Code's diagnostic. This has a number of
shortcomings:
1. It's not a very good use of space in the error list
2. We can't jump to secondary spans (e.g. where a called function is
defined)
3. We can't use Code Actions aka Quick Fix
This moves all of the low-level parsing and mapping to a
`rust_diagnostics.ts`. This uses some heuristics to map Rust diagnostics
to VsCode:
1. As before, the Rust diagnostic message and primary span is used for
the root diagnostic. However, we now just use the message instead of
the rendered version.
2. Every secondary span is converted to "related information". This
shows as child in the error list and can be jumped to.
3. Every child diagnostic is categorised in to three buckets:
1. If they have no span they're treated as another line of the root
messages
2. If they have replacement text they're treated as a Code Action
3. If they have a span but no replacement text they're treated as
related information (same as secondary spans).
|
Here's some screenshots of the error list/Quick Fix: |
This happened to work because we always produce a single edit but this is obviously dubious.
|
Is there a way to avoid the O(N^2) duplicate check? |
The first cut was a bit rough with the blanket `unused_*` rule. This trigger for things like `unused_mut` where the code is used but it's suboptimal. It's misleading to grey out the code in those cases. Instead, use an explicit list of things known to be dead code.
|
It's certainly annoying but I don't think there's much we can do without generating a string hash for each diagnostic and Code Action. Just using e.g. the span isn't enough as I've justified the (On^2) loop to myself to myself two ways:
If this ends up burning CPU cycles in practice we could look at doing something more clever. |
|
This looks awesome! I personally wouldn't worry about O(N^2) here: even if you have a thousand of diagnostics, O(N^2) is still pretty fast. We could make this faster by using a hash table, but my understanding is that doing this in JS is a pain. @etaoins I don't have a lot of comments here, as I am not an expert in TypeScript, but the code looks really good and well thought out to me! I slightly worry that we now have a pretty huge chunk of logic in TypeScript, which is not tested at all. Do you think it would be a good idea to add some basic tests here? I don't think that actually running If that's too painful, I am ok with merging this as is! |
|
@matklad I tried just setting up a basic Mocha test but unless the code is actually running inside Visual Studio Code the Would you mind merging this now and I can take a look at how to get tests working locally and on Travis later in the week? |
|
Sure! bors r+ |
1439: Rich mapping of cargo watch output r=matklad a=etaoins
Currently we depend on the ASCII rendering string that `rustc` provides to populate Visual Studio Code's diagnostic. This has a number of shortcomings:
1. It's not a very good use of space in the error list
2. We can't jump to secondary spans (e.g. where a called function is defined)
3. We can't use Code Actions aka Quick Fix
This moves all of the low-level parsing and mapping to a `rust_diagnostics.ts`. This uses some heuristics to map Rust diagnostics to VsCode:
1. As before, the Rust diagnostic message and primary span is used for the root diagnostic. However, we now just use the message instead of the rendered version.
2. Every secondary span is converted to "related information". This shows as child in the error list and can be jumped to.
3. Every child diagnostic is categorised in to three buckets:
1. If they have no span they're treated as another line of the root messages
2. If they have replacement text they're treated as a Code Action
3. If they have a span but no replacement text they're treated as related information (same as secondary spans).
Co-authored-by: Ryan Cumming <[email protected]>
Build succeeded |
Do you think this whole thing should move to the server long-term, maybe with the persistent compiler architecture that's being considered for RLS? |
|
Good question! If we get persistent compiler architecture, than we definitely should move this to the server. In the current set-up, it's unclear what is the best course of action. If we move |
1446: Initial Visual Studio Code unit tests r=matklad a=etaoins As promised in #1439 this is an initial attempt at unit testing the VSCode extension. There are two separate parts to this: getting the test framework working and unit testing the code in #1439. The test framework nearly intact from the VSCode extension generator. The main thing missing was `test/index.ts` which acts as an entry point for Mocha. This was simply copied back in. I also needed to open the test VSCode instance inside a workspace as our file URI generation depends on a workspace being open. There are two ways to run the test framework: 1. Opening the extension's source in VSCode, pressing F5 and selecting the "Extensions Test" debug target. 2. Closing all copies of VSCode and running `npm test`. This is started from the command line but actually opens a temporary VSCode window to host the tests. This doesn't attempt to wire this up to CI. That requires running a headless X11 server which is a bit daunting. I'll assess the difficulty of that in a follow-up branch. This PR is at least helpful for local development without having to induce errors on a Rust project. For the actual tests this uses snapshots of `rustc` output from [a real Rust project](https://github.com/etaoins/arret) captured from the command line. Except for extracting the `message` object and reformatting they're copied verbatim into fixture JSON files. Only four different types of diagnostics are tested but they represent the main combinations of code actions and related information possible. They can be considered the happy path tests; as we encounter corner-cases we can introduce new tests fixtures. Co-authored-by: Ryan Cumming <[email protected]>
1454: Fix `cargo watch` code action filtering r=etaoins a=etaoins There are two issues with the implementation of `provideCodeActions` introduced in #1439: 1. We're returning the code action based on the file its diagnostic is in; not the file the suggested fix is in. I'm not sure how often fixes are suggested cross-file but it's something we should handle. 2. We're not filtering code actions based on the passed range. The means if there is any suggestion in a file we'll show an action for every line of the file. I naively thought that VS Code would filter for us but that was wrong. Unfortunately the VS Code `CodeAction` object is very complex - it can handle edits across multiple files, run commands, etc. This makes it complex to check them for equality or see if any of their edits intersects with a specified range. To make it easier to work with suggestions this introduces a `SuggestedFix` model object and a `SuggestFixCollection` code action provider. This is a layer between the raw Rust JSON and VS Code's `CodeAction`s. I was reluctant to introduce another layer of abstraction here but my attempt to work directly with VS Code's model objects was worse. Co-authored-by: Ryan Cumming <[email protected]>
Currently we depend on the ASCII rendering string that
rustcprovides to populate Visual Studio Code's diagnostic. This has a number of shortcomings:This moves all of the low-level parsing and mapping to a
rust_diagnostics.ts. This uses some heuristics to map Rust diagnostics to VsCode:As before, the Rust diagnostic message and primary span is used for the root diagnostic. However, we now just use the message instead of the rendered version.
Every secondary span is converted to "related information". This shows as child in the error list and can be jumped to.
Every child diagnostic is categorised in to three buckets: