Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a clear diagnostics command to flycheck. #14590

Closed
wants to merge 1 commit into from

Conversation

matts1
Copy link

@matts1 matts1 commented Apr 17, 2023

This will allow for watcher-style flychecks. Currently, we can only run flychecks on every save. This allows for a long-running flycheck that can update diagnostics between file saves (eg. update on every build).

Rationale:
I'm trying to improve the experience developing using bazel + rust (see issue). The developer experience I'm looking at is:

  1. Dev invokes "ibazel build //:my_crate //:my_crate_test". This ensures that whenever the source code changes, it rebuilds. both the crate and the test for the crate.
  2. The discover project command is automatically invoked when you open main.rs, which generates a file listing the .rustc-output files mentioned below.
  3. Dev modifies my_crate/src/main.rs and saves in vscode.
  4. bazel is automatically invoked on save, and under the hood invokes rustc, which generates bazel-out/k8-fastbuild/bin/my_crate/my_crate.rustc-output and bazel-out/k8-fastbuild/bin/my_crate_test/my_crate_test.rustc_output for the original crate and the test crate respectively.
  5. The non-test crate finishes building
  6. A watcher script would see that my_crate.rustc-output has been updated, and update the rust-analyzer result for the file.
  7. The test crate finishes building
  8. The watcher script would see that my_crate_test.rustc-output has been updated, and update the rust-analyzer result for the file.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2023
@@ -461,6 +470,9 @@ impl CargoActor {
let mut read_at_least_one_stdout_message = false;
let mut read_at_least_one_stderr_message = false;
let process_line = |line: &str, error: &mut String| {
if line == "CLEAR" {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to some sort of json format or something instead to be consistent to the rest of the file, but didn't bother implementing it since I don't know what the right structure for something like that would be.

This will allow for watcher-style flychecks. Currently, we can only run
flychecks on every save. This allows for a long-running flycheck that
can update diagnostics between file saves (eg. update on every build).

Rationale:
I'm trying to improve the experience developing using bazel + rust (see [issue](bazelbuild/rules_rust#1657)). The developer experience I'm looking at is:

1. Dev invokes "ibazel build //:my_crate //:my_crate_test". This ensures that whenever the source code changes, it rebuilds. both the crate and the test for the crate.
2. The discover project command is automatically invoked when you open main.rs, which generates a file listing the `.rustc-output` files mentioned below.
3. Dev modifies `my_crate/src/main.rs` and saves in vscode.
4. bazel is automatically invoked on save, and under the hood invokes rustc, which generates `bazel-out/k8-fastbuild/bin/my_crate/my_crate.rustc-output` and `bazel-out/k8-fastbuild/bin/my_crate_test/my_crate_test.rustc_output` for the original crate and the test crate respectively.
5. The non-test crate finishes building
6. A watcher script would see that `my_crate.rustc-output` has been updated, and update the rust-analyzer result for the file.
7. The test crate finishes building
8. The watcher script would see that `my_crate_test.rustc-output` has been updated, and update the rust-analyzer result for the file.
@HKalbasi
Copy link
Member

It is not clear to me that how bazel can preemptively send commands to the r-a process? Is it keep itself alive first time server invoke it on save? If so, what will happen on later saves? If it is not about some interactive and long time user experience, I assume the clear command is used to de-duplicate diagnostics. In this case, isn't it possible for bazel to deduplicate diagnostics in itself and only send the new diagnostics to the rust-analyzer?

@matts1
Copy link
Author

matts1 commented Apr 20, 2023

To clarify, bazel doesn't send anything to r-a. When you run the discover project command, bazel generates a list of .rustc-output files that it will output. There's a seperate watcher script that is watching these files that will send things to r-a. The problem isn't that the command is unable to de-duplicate diagnostics. The problem is that because the check command is only invoked on save, there's no way to update it multiple times in a single save.

I'll give an example where we have 3 build targets:

  • //:lib (lib.rs)
  • //:bin (main.rs, depends on //:lib)
  • //:bin_test (main.rs, depends on //:lib)

Let's assume we update main.rs, save, and then run bazel build //:all. Here, bazel generates three .rustc-output files - one per rustc invocation.

As a user, my expectation would be that it updates main.rs with either the updated contents of bin.rustc-output or bin-test.rustc-output (but not both, otherwise you get duplication of errors and typing). The problem is literally impossible to do correctly, for two reasons:

  • We want to update multiple times per save
  • The check command isn't informed which file is updated - just that a file has been updated.

This leaves us with a few options for the check command, each of which has its own problems:

  • cat *.rustc-output
    • This outputs the rustc-output for the last build, not the current one.
  • Watch those files and wait for an update to a single file and update them
    • lib.rustc-output gets updated first, and if I cat that, then I'll be sending off an old version of bin.rustc-output / bin_test.rustc-output.
  • Watch those files and wait for an update to all files
    • What if I instead had invoked bazel build //:bin. Then bin_test.rustc-output would never get updated, so this wouldn't work

Even if we ensured that the check command was told which file is updated, we come across the following race condition from above:

  • Bazel successfully compiles //:bin and writes to bin.rustc-output
  • Bazel fails to compile //:bin_test and writes to bin_test.rustc-output, since the problem was in the #[cfg(test)] section.
  • Here, we want to initially update it to the bin.rustc-output and then later change it to bin_test.rustc-output when we realise we have better information available.

@Veykril
Copy link
Member

Veykril commented Apr 25, 2023

Current rust-analyzer assumes oneshot processes for flycheck. I could see us maybe (not 100% certain) adding support for watcher style processes, but those should probably be handled differently than an adhoc CLEAR message

@matts1
Copy link
Author

matts1 commented Apr 26, 2023

Closing because as discussed elsewhere, an extension for rust-analyzer with vscode is required anyway, so we can just have that extension call the command clearFlycheck.

@matts1 matts1 closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants