Allow passthrough when `--message-format` is specified #15

ian-h-chamberlain · 2022-04-08T15:19:03Z

I use rust-analyzer with VSCode and it requires --message-format=json when using a nonstandard cargo check command.

This change allows me to configure something like

{
  "rust-analyzer.cargo.target": "armv6k-nintendo-3ds",
  "rust-analyzer.checkOnSave.overrideCommand": [
    "cargo",
    "3ds",
    "check",
    "--message-format=json",
  ]
}

to get proper diagnostics from cargo 3ds check in the editor.

I think this used to work normally, before I changed to using Message::parse_stream and --message-format=json-render-diagnostics.

Tested both to verify JSON output is printed:

cargo 3ds check --message-format=json
cargo 3ds check --message-format json

cc @AzureMarker @Meziu

AzureMarker · 2022-04-08T23:59:43Z

It would be interesting to check how this is done in similar cargo extensions.

ian-h-chamberlain · 2022-04-10T22:06:08Z

It would be interesting to check how this is done in similar cargo extensions.

I tried to look around a bit for other cases of this, but didn't seem to find too many examples (most cargo extensions seem to operate fairly independently of the usual cargo build, etc). I did find this issue which asks for the messages from Cargo to be available as part of the JSON message format: rust-lang/cargo#8283 (comment)

If it were provided like that, cargo-3ds could just display the rendered messages in cases like that, I suppose. Otherwise, I'm not sure what options we have – maybe some kind of tee-like pattern that copies the JSON into Message::parse_stream and also prints it to stdout? In this case we might still need to parse the CLI looking for a --message-format and it would only work for json-render-diagnostics or json formats, I think, but that probably covers most use cases...

AzureMarker · 2022-04-15T03:12:26Z

An alternative approach I just thought of would be to run the first command with the user's preference (defaulting to an unspecified message-format parameter), then immediately run it a second time with the JSON format we need. But this seems pretty hacky.

src/main.rs

AzureMarker · 2022-04-15T03:14:26Z

I also took a look at cargo-psp (the original inspiration for this crate) and they seem to iterate over all of the binaries in the metadata:
https://github.com/overdrivenpotato/rust-psp/blob/master/cargo-psp/src/main.rs

ian-h-chamberlain · 2022-04-21T13:33:52Z

For one other example in the wild, I looked at cargo-miri: https://github.com/rust-lang/miri/blob/master/cargo-miri/bin.rs#L553

It looks like they use a custom iteratator over the flags to extract the ones they want to read or modify. There is a slight difference I think, since miri is also invoked as RUSTC_WRAPPER and they do some hacks there to prevent rustc from emitting actual executables.

We might be able to borrow that flag-reading code, but otherwise it's not a great example since they don't actually care about the emitted binary paths. I'll investigate using a tee-like solution to see if we can avoid having to run cargo more than necessary.

We can still parse the JSON output, even if Also: - remove some unused deps. - make sure we use eprintln so it doesn't interfere with any JSON output the user might want

ian-h-chamberlain · 2022-05-12T01:31:57Z

Phew, this has been on backburner for a while but I finally got around to trying a different approach with tee. Basically, we can still parse the JSON compiler messages and pass them through to the end user stdout.

I tested with

cargo 3ds build --message-format json
cargo 3ds clippy --message-format json

Best I can tell, this is the easiest solution that will work for both use cases, for now.

Also,

I also took a look at cargo-psp (the original inspiration for this crate) and they seem to iterate over all of the binaries in the metadata

I wonder if we will want to do that at some point, to support something like e.g. cargo 3ds build --examples for multiple bin targets?

AzureMarker · 2022-05-13T04:32:40Z

src/main.rs

+        for (i, arg) in args.iter().enumerate() {
+            if arg.starts_with("--message-format") {
+                return {
+                    let arg = args.remove(i);


Wow, rustc understands that the immutable borrow from iter is no longer needed at this point due to the return and not referencing arg, and lets you call remove (mutable borrow). Sweet!

Yeah, I wasn't really expecting this to compile when I first wrote it, but was impressed that that it actually did!

AzureMarker · 2022-05-13T04:34:20Z

src/main.rs

+            Some(format) => {
+                if !format.starts_with("json") {
+                    eprintln!("error: non-JSON `message-format` is not supported");
+                    std::process::exit(1);


nit: std:: isn't needed since process is already imported. (showed up as a lint in my editor)

oh neat, is that lint builtin to your editor or does it come from clippy or something? I've wanted something like this but haven't seen such a lint in my editor

I use CLion with the IntelliJ-Rust plugin. The lint was recently added:
intellij-rust/intellij-rust#7410
https://intellij-rust.github.io/2022/04/25/changelog-169.html

Cool, I wonder if there would be an appetite for something like that in rust-analyzer... they have some similar stuff with e.g. merging use 🤔

AzureMarker · 2022-05-13T04:37:20Z

src/main.rs


-        let messages = Message::parse_stream(stdout_reader)
-            .collect::<io::Result<Vec<Message>>>()
+        let buf_reader: &mut dyn BufRead = if self.message_format == Self::DEFAULT_MESSAGE_FORMAT {


Cool pattern!

Cargo.toml

Allow passthrough when message-format is specified

935594b

AzureMarker reviewed Apr 15, 2022

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

Parse message-format up front and use TeeReader

8023bfb

We can still parse the JSON output, even if Also: - remove some unused deps. - make sure we use eprintln so it doesn't interfere with any JSON output the user might want

AzureMarker approved these changes May 13, 2022

View reviewed changes

Meziu reviewed May 15, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Update to 2021 edition

1ebde46

Meziu merged commit 3b187bc into rust3ds:master May 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passthrough when `--message-format` is specified #15

Allow passthrough when `--message-format` is specified #15

ian-h-chamberlain commented Apr 8, 2022

AzureMarker commented Apr 8, 2022

ian-h-chamberlain commented Apr 10, 2022

AzureMarker commented Apr 15, 2022

AzureMarker commented Apr 15, 2022

ian-h-chamberlain commented Apr 21, 2022

ian-h-chamberlain commented May 12, 2022

AzureMarker May 13, 2022

ian-h-chamberlain May 13, 2022

AzureMarker May 13, 2022

ian-h-chamberlain May 13, 2022

AzureMarker May 13, 2022

ian-h-chamberlain May 14, 2022

AzureMarker May 13, 2022

Allow passthrough when --message-format is specified #15

Allow passthrough when --message-format is specified #15

Conversation

ian-h-chamberlain commented Apr 8, 2022

AzureMarker commented Apr 8, 2022

ian-h-chamberlain commented Apr 10, 2022

AzureMarker commented Apr 15, 2022

AzureMarker commented Apr 15, 2022

ian-h-chamberlain commented Apr 21, 2022

ian-h-chamberlain commented May 12, 2022

AzureMarker May 13, 2022

Choose a reason for hiding this comment

ian-h-chamberlain May 13, 2022

Choose a reason for hiding this comment

AzureMarker May 13, 2022

Choose a reason for hiding this comment

ian-h-chamberlain May 13, 2022

Choose a reason for hiding this comment

AzureMarker May 13, 2022

Choose a reason for hiding this comment

ian-h-chamberlain May 14, 2022

Choose a reason for hiding this comment

AzureMarker May 13, 2022

Choose a reason for hiding this comment

Allow passthrough when `--message-format` is specified #15

Allow passthrough when `--message-format` is specified #15