Use Cargo's JSON diagnostics for suggestions

[hafeoz]

Fix #688 .

Uses JSON message format to communicate with cargo when building and running. This allows suggestions to be prompted correctly.

Example

Currently

New output

Possible TO-DOs

• Any mode other than "building" and "running" is still using human-readable mode since I'm not sure how are they different from the normal cargo output.
• cargo_metadata crate has been used. Not clear if the library can work with different cargo versions, or we need to do some kind of versioning. As far as I tested it works for all three variants at least for now.
• No multi-line diagnostics have been tested. I wasn't able to find or think of any fixes with more than one line.
• The standard error output's timeline is not correct. Currently, all cargo's output is located at the bottom of the standard error output, regardless of the time arrangement between outputs. Related cargo comment
• Better prompt. Maybe discard the "rendered_message" and render the diagnostics message ourselves? This way we can have suggestions in message clickable, instead of having a list of suggestions under the message, which might be confusing.

[hafeoz]

Should we leave the cargo message in stdout instead of moving it to stderr? This way the timeline will not be disrupted.

[shepmaster]

I just wanted to swing by to let you know that I've seen this PR, but I'm not sure when I'll have time to give it the review focus it deserves. I'm super excited about this change, however!

Random thoughts that may or may not be useful or even relevant (as I haven't even looked at the implementation)...

We'll want to (as best we can!) preserve the structure of the API response/requests. e.g. we might want to add a new structured field for the compiler response, instead of repurposing the existing fields. Sadly, people use the existing APIs and assume they will be stable, even though almost no one has ever asked. 😢

Maybe discard the "rendered_message" and render the diagnostics message ourselves? This way we can have suggestions in message clickable, instead of having a list of suggestions under the message, which might be confusing.

Yeah, I think this is the ultimate end goal. Obviously it's more of a PITA, but would allow removing the custom regex-based error / warning formatting as well. Maybe this would also allow splitting out Cargo's output from the programs stderr/stdout as well.

Not clear if [cargo_metadata] can work with different cargo versions

I'd expect that as long as we are using the v1 of the output (our whatever is the current version) that it should be compatible.

[hafeoz]

We'll want to (as best we can!) preserve the structure of the API response/requests

So should we preserve (at least to our best) the stdout and stderr field in the JSON response for URL /execute? If that's the case, then we might need to add a new field to the response (like actions):

Current response example

{
  "success": false,
  "stdout": "",
  "stderr": "   Compiling playground v0.0.1 (/playground)\nerror[E0433]: failed to resolve: use of undeclared type `HashMap`\n --> src/main.rs:3:13\n  |\n3 |     let a = HashMap::new();\n  |             ^^^^^^^ not found in this scope\n  |\nhelp: consider importing one of these items\n  |\n1 | use hashbrown::HashMap;\n  |\n1 | use std::collections::HashMap;\n  |\n\nFor more information about this error, try `rustc --explain E0433`.\nerror: could not compile `playground` due to previous error\n"
}

Proposed response example

{
  "success": false,
  "stdout": "",
  "stderr": "   Compiling playground v0.0.1 (/playground)\nerror[E0433]: failed to resolve: use of undeclared type `HashMap`\n --> src/main.rs:3:13\n  |\n3 |     let a = HashMap::new();\n  |             ^^^^^^^ not found in this scope\n  |\nhelp: consider importing one of these items\n  |\n1 | use hashbrown::HashMap;\n  |\n1 | use std::collections::HashMap;\n  |\n\nFor more information about this error, try `rustc --explain E0433`.\nerror: could not compile `playground` due to previous error\n",
  "actions": [
    {
      "link": {
        "startline": 9,
        "endline": 9,
        "startcol": 4,
        "endcol": 27,
        "target": "stderr"
      },
      "solution": {
        "startline": 1,
        "endline": 1,
        "startcol": 1,
        "endcol": 1,
        "replacement": "use hashbrown::HashMap;"
      }
    },
    {
      "link": {
        "startline": 9,
        "endline": 9,
        "startcol": 4,
        "endcol": 34,
        "target": "stderr"
      },
      "solution": {
        "startline": 1,
        "endline": 1,
        "startcol": 1,
        "endcol": 1,
        "replacement": "use std::collections::HashMap;"
      }
    }
  ]
}

It's definitely going to be more complex than the implementation this PR proposed (especially for front-end). An advantage for such response is that we might be able to remove almost all regex (not only the "import" one) since highlighting and error location (currently handled here) can be extracted directly from Cargo JSON output and passed to front-end using the new field in HTTP JSON response.

[shepmaster]

An advantage for such response is that we might be able to remove almost all regex (not only the "import" one)

Yep, that's what I meant by

but would allow removing the custom regex-based error / warning formatting as well

😉

[shepmaster]

This is an encouraging start, thanks! I added comments, but many of them are around the piece of code that's most likely to change as we push more structured information to the frontend, so don't feel like you need to adhere to it now.

[shepmaster]

Instead of building a brand new string here, it'd be a bit more efficient to accept a &mut String argument and append to it directly.

[shepmaster]

Not a required changed, but this could be something along the lines of this (maybe with some .as_ref or as_deref calls added):

Suggested change

	if let Some(rendered_msg) = diagnostic.rendered {
	diagnostic_string.push_str(&rendered_msg);
	} else {
	diagnostic_string.push_str(&diagnostic.message);
	}
	let x = diagnostic.rendered.or(&diagnostic.message)
	diagnostic_string.push_str(x);

[shepmaster]

My brain is slow at parsing this. Maybe we could use

Suggested change

	if span.file_name != "src/lib.rs" && span.file_name != "src/main.rs" {
	if !matches!("src/lib.rs" | "src/main.rs", span.file_name) {

[shepmaster]

Instead of creating the intermediate String via format!, you can format directly to the &mut String using write!. See also How can I append a formatted string to an existing String?

[shepmaster]

The same idea for using write! can be applied here.

[shepmaster]

I'd have expected you could elide the [..]:

Suggested change

	let metadata_stream = cargo_metadata::Message::parse_stream(&output[..]);
	let metadata_stream = cargo_metadata::Message::parse_stream(&output);

[shepmaster]

Suggested change

	#[snafu(display("Unable to parse cargo output: {}", source))]
	#[snafu(display("Unable to parse Cargo output: {}", source))]

[shepmaster]

This logic is tricky enough that I can't just glance at it and feel very confident about what it's doing. It'd be helpful to extract out the core logic here and write some unit tests to exercise the various ifs. Something like this sketch:

    case ActionType.ApplySuggestion:
       const startline = action.startline - 1;
       const endline = action.endline - 1;
       const startcol = action.startcol - 1;
       const endcol = action.endcol - 1;
       return someLogic(state);

[hafeoz]

I've been poking around and writing custom diagnostic renderer for the last few days and it's way more complicated than I expected. In order to not break existing API, we basically need to port this (or somehow intercept this call to manipulate JSON output) so that it can not only output formatted message but also export "actions" linked to part of the message. A port would be at least 1000+ lines and include some dirty hacks (mainly because we need to parse cargo's metadata back to rustc's one). Therefore, I think continue to use pre-rendered output and do some matching would be a better idea, like this (pseudo-code)

fn add_suggestion_actions(pre_rendered: &str, diagnostic: cargo_metadata::diagnostic::Diagnostic, actions: &mut Vec<Action>) {
    if let Some(position) = pre_rendered.find(diagnostic.suggested_replacement) {
        actions.push(Action(position, ActionType::Replacement(diagnostic.suggested_replacement)));
    }
}
fn add_filejump_actions(pre_rendered: &str, diagnostic: cargo_metadata::diagnostic::Diagnostic, actions: &mut Vec<Actions>) {
    let file_loc = format!("{}:{}:{}", diagnostic.file_name, diagnostic.line_start, diagnostic.column_start);
    if let Some(position) = pre_rendered.find(file_loc) {
        actions.push(Action(position, ActionType::Jumpto(diagnostic.line_start, diagnostic.column_start)));
    }
}