Add an option to save the json output from rustc to pass to rust-analyzer

[googleson78]

Goal

This is intended to be used to provide output that rust-analyzers checkOnSave.overrideCommand expects, so that we can get inline diagnostics inside rust-analyzer when using rules_rust.

There are a few unresolved issues which I've detailed below, some of them quite core, but I'm posting this in-progress work to get some more opinions, and especially to hear if you like this approach at all. I'm very open to discussing everything here!

Current implementation

The core idea is to have a flag (output_diagnostics) that toggles whether we save rustc output. Once this rustc output is saved, a user can then gather all the files containing the rustc json output and simply cat them or whatever, e.g. they can set checkOnSave.overrideCommand to the following script:

#!/bin/bash

bazel build <target> --@rules_rust//:output_diagnostics=true --output_groups=rust_lib_rustc_output,rust_metadata_rustc_output 2>&1 >/dev/null || true

cat $(find -L bazel-bin/rs -name '*.rustc-output')

Unresolved questions

Core idea

Is this approach alright?

Do we instead want to have an entirely separate target for this? One could imagine implementing an aspect that reuses most of the implementation details for the rust_* targets. That might be unfortunate because it might end up doing more recompilation

Better UX

Builtin target for outputting the files

It would be great if we had some builtin target that somehow accomplishes what the bash script from above does.

We could also rely on some bazel specific features to find our required files instead of relying on coreutils like find.

If rust-lang/rust-analyzer#13529 goes through we can even generate that file and insert the target in question there.

Docs

Obviously this needs documentation - I'm not sure where to put it. We need to finalise everything else before we do this bit, I think.

[googleson78]

I am particularly unhappy about this check happening on every loop, but I do not know of a way to remove it apart from constructing two different functions with duplicated code except this bit.

I considered abstracting with a passed lambda, but I would guess(?) a function call is more heavyweight compared to a a tag check. In any case, I'm not sure this is a performance bottleneck at all, considering we're doing loads more IO. It's just not pretty.

[googleson78]

Did I somehow cause these build failures - they seem to be 404ing on fetching something?

[scentini]

The actual error is

(09:29:08) ERROR: /workdir/test/unit/pipelined_compilation/BUILD.bazel:4:33: in wrap rule //test/unit/pipelined_compilation:wrapper_with_metadata:
--
  | Traceback (most recent call last):
  | File "/workdir/test/unit/pipelined_compilation/wrap.bzl", line 60, column 32, in _wrap_impl
  | return rustc_compile_action(
  | File "/workdir/rust/private/rustc.bzl", line 1202, column 28, in rustc_compile_action
  | ctx.actions.run(
  | Error in run: at index 0 of outputs, got element of type NoneType, want File

I don't think that has anything to do with fetching, looks like the build failures are related to the PR. The issue is that we end up trying to run an action without passing outputs (or passing a None instead of an Artifact)

[googleson78]

Thanks, I must have gotten a line mismatch..

[googleson78]

Not sure what the windows errors are about :/

[googleson78]

Tried to place the json output files in the same directory (via sibling) as the corresponding .rlib/.rmeta, but it doesn't seem to be helping the permissions issue(?).

[scentini]

My educated guess is that this is caused by two separate actions trying to generate the same output. If both actions have the same file declared as an output, bazel will error out; I think the clippy aspect doesn't have the rustc_output file declared as an output, but it still tries to generate it, at which point it clashes with the rustc_output from the regular rustc compile action. This doesn't happen outside windows because the actions are sandboxed, and the rustc_output from the clippy action gets discarded at the end; windows however doesn't support sandboxing. We don't need to produce rustc_output when running clippy, right?

[matts1]

I personally found that cat $(find -L bazel-bin/rs -name '*.rustc-output') didn't work for me (bazel-bin/rs didn't exist). I was able to find the bazel-bin directory instead, but it was extremely slow.

The following worked for me:

bazel build <target> --@rules_rust//:output_diagnostics=true --output_groups=rust_lib_rustc_output,rust_metadata_rustc_output 2>&1 >/dev/null || true
bazel cquery <target> --@rules_rust//:output_diagnostics=true --output_groups=rust_lib_rustc_output,rust_metadata_rustc_output --output starlark --starlark:file=get_outputs.cquery

with get_outputs.cquery containing:

def format(target):
  crate_info = [v for k, v in providers(target).items() if k.endswith("%CrateInfo")]
  if not crate_info:
    return ""
  crate_info = crate_info[0]
  rustc_output = crate_info.rust_lib_rustc_output
  if rustc_output == None:
    return ""
  return rustc_output.path

[googleson78]

Oops, the rs part was from the concrete project I was trying to get this to work for.

FTR, I am currently no longer working on this, so any interested parties should feel free to pick this up.

[matts1]

In that case, I'd be happy to pick this up. My opinions:

• Is a flag the right approach?
  • Yes - This needs to be able to be enabled / disabled as configuration rather than code, and having it as a global config seems like the right approach to me
• Is the general approach good?
  • Yes - I like the idea of a file that is never depended on, because:
    • Bazel won't generate this file by default, so we're not affecting performance
    • We shouldn't add seperate targets for this because as you said, it would mean we have to invoke rustc additional times
• Documentation
  • I don't think we should document this directly. Rather, we should document this as part of the rust-analyzer documentation.

This is the user experience I want. I've prototyped this and have it mostly working:

VScode has a 'discover project command'. It invokes this with all files you currently have open, and treats the stdout as the rust-project.json. I suggest adding extra options to gen_rust_project so we can run bazel run @rules_rust//tools/rust_analyzer:gen_rust_project -- --output-stdout --rustc_output_file_list=rustc_output_files.txt --files /path/to/workspace/my_crate/src/main.rs ....

• The output-stdout flag would cause rust-project.json to output to stdout instead of <workspace_root>/rust-project.json
• The --files flag would take in files instead of targets. It would search for the first directory above the source which contains a build file, and convert it to that target's package (eg. //my_crate:all)
• The --rustc_output_file_list would invoke bazel cquery to determine which files would be generated, and output those to a file. In the example above, it would write bazel-out/k8-fastbuild/bin/my_crate/my_crate.rustc-output to rustc_output_files.txt

Vscode would give live feedback for rust code via a command that collected all of those files and generated one large file out of all of them. This appears like it could be achieved with this feature, but I was unable to get it to work, so I'm currently using the on-save feature, which has issues with race conditions (since vscode only updates on save, rather than when the files are updated, if you update multiple files it doesn't work).

These files would be updated whenever the user runs "bazel build", if they have the configs --@rules_rust//:output_diagnostics=true --output_groups=+rust_lib_rustc_output,+rust_metadata_rustc_output. I would put in the documentation a recommendation to put that in your bazelrc, and a suggestion that this works very well with ibazel if you want live feedback.