Configure flycheck using workspace.discoverConfig #18043

cormacrelf · 2024-09-04T04:03:37Z

Recap of recent developments with non-Cargo build systems

We have workspace.discoverConfig, which configures a command to (basically) spit out a rust-project.json on stdout and treats it like you put a rust-project.json file on disk.
JSON projects have a nice section for runnables, but they're currently limited to RunnableKind { Check, Run, TestOne, }. You cannot have the JSON project configure its own flychecks. Which means that configuring a non-Cargo build system is a complicated dance of workspace.discoverConfig + check.overrideCommand + "check.workspace": false.
The {label} substitution (available for runnables) was not usable for the flycheck command. I think because the only users of discoverConfig have been content with $saved_file so far. So we currently have build labels in the JSON project that can't even be used for flychecking.

What this does

This PR adds this functionality:

Allows the discoverConfig to describe how to flycheck crates, using a new runnable kind of "flycheck".
Allows all types of flycheck override commands to use {label}. If you are not using discoverConfig / rust-project.json and therefore don't have build labels attached to crates, then {label} means the cargo canonical name. Previously we only supported $saved_file here.
The flycheck begin notification now includes a pretty-print of the actual command in use (if you're not just letting RA run cargo). Super useful to know that things got configured ok, at the bottom of your VSCode window.

And fixes:

Flychecks were being run on random downstream crates instead of the crate containing the file you saved, which would randomly result in cargo check-ing a bigger downstream dependency, leading to unnecessarily slow flychecks and wasted CPU cycles. See the comments in notification.rs. I noticed this because the buck2 rust-project check command does not give diagnostics for dependencies of the target you ask for.

Notes for users

For people actually using this, it works well in VSCode if you configure rust-analyzer.workspace.discoverConfig in your settings.json, as per the manual. However, because rust-analyzer.toml support is still in its infancy, to get this going in Neovim, you will need my PR to rustaceanvim to get the settings into the initialization request, and I advise having rustaceanvim autoload settings from a file called rust-analyzer.json.

Tagging: @alibektas @davidbarsky

Say you had A <- B <- C. If you save a file in A, the variable `crate_root_paths` is populated with all three crates in that order. However the one we actually ran flycheck on, was simply the first one of A, B, or C to appear in `project.crates()`. Which is unordered. This affected - `[check] workspace = false` config where flycheck runs on individual crates - non-Cargo build environments especially Usually it was invisible because cargo prints out the warnings found in A if you build B or C. But this is wasteful if B or C is a big crate. So it was probably showing up as rust-analyzer constantly trying to build random parts of your dependency graph on save. Not every build tool does what cargo does either. Buck2's `[diag.json]` subtargets do not contain warnings from upstream crates. That's how I discovered this problem; warnings weren't showing up at all.

…-project.toml

…rust-project.json

…roject.json runnable

…alse

crates/rust-analyzer/src/handlers/notification.rs

…:Cargo

…he one we were looking for

davidbarsky

some assorted comments. I'll do another pass later.

davidbarsky · 2024-09-05T15:23:34Z

crates/project-model/src/project_json.rs

+    /// Template for checking a target, emitting rustc JSON diagnostics.
+    /// May include {label} which will get the label from the `build` section of a crate.
+    Flycheck,


I don't think that you need to add a new enum variant called Flycheck—I always intended the RunnableKind::Check variant to be used for Flycheck, but I never actually wired it up. Happy to have Check be renamed to Flycheck if Check can be (briefly) deserialized.

davidbarsky · 2024-09-05T15:24:26Z

crates/rust-analyzer/src/handlers/notification.rs

-                // Trigger flychecks for the only crate which the target belongs to
-                Some((_, krate)) => vec![krate],
-                None => {
+            tracing::debug!("flycheck scope: {:?}", scope);


Suggested change

tracing::debug!("flycheck scope: {:?}", scope);

tracing::debug!(?scope);

davidbarsky · 2024-09-05T15:29:33Z

crates/rust-analyzer/src/handlers/notification.rs

-                    TargetSpec::ProjectJson(p) => (p.label, p.crate_id),
-                };
+            #[derive(Debug)]
+            enum FlycheckScope {


tiny style nit: maybe move this declaration out of this scope and maybe into the flycheck crate?

davidbarsky · 2024-09-05T15:35:04Z

crates/rust-analyzer/src/handlers/notification.rs

+                        // A <- B <- C
+                        //
+                        // [1]: But see FIXME above where we flatten.
+                        crate_root_paths.iter().find_map(|root| {


i think i'll have further comments on this section later once i understand it a little more.

davidbarsky · 2024-09-05T15:35:42Z

crates/rust-analyzer/src/handlers/notification.rs

+                            !world.config.flycheck_workspace(source_root_id)
+                                || flycheck.cannot_run_workspace()
+                                    // No point flychecking the whole workspace when you edited a
+                                    // main.rs. It cannot have dependencies.


do you mean "reverse dependencies"?

davidbarsky · 2024-09-05T15:55:03Z