fix: project json runnable kind bin substitution

[TroyKomodo]

fixes a bug related to substitution in project json bin not correctly replacing the {label} tag with the label of the current item.

[lukasoyen]

I am not sure this is a fix, as the old behavior was not a bug, but documented:

rust-analyzer/docs/book/src/non_cargo_based_projects.md

Lines 203 to 207 in 3d2c40d

	/// The args for testOne can contain two template strings:
	/// `{label}` and `{test_id}`. `{label}` will be replaced
	/// with the `Build::label` and `{test_id}` will be replaced
	/// with the test name.
	kind: 'testOne' | string;

.

Though we have another comment that suggests this might be a bug:

rust-analyzer/crates/project-model/src/project_json.rs

Lines 316 to 319 in 3d2c40d

	/// The args can contain two template strings: `{label}` and `{test_id}`.
	/// rust-analyzer will find and replace `{label}` with [`Build::label`] and
	/// `{test_id}` with the test name.
	pub args: Vec<String>,

I am all for something like this change. I would propose to introduce a new "binOne" similar to "testOne" where this substitution applies.

cc @davidbarsky @Wilfred

[Wilfred]

I added the kind: 'testOne' | string; in #18878, as testOne was the only value that I could see was actually in use.

I'm not necessarily opposed to applying the substitution on all runnables, but we should update the non_cargo_based_projects.md accordingly. test_id makes no sense in other runnable types at least. I think we'd also need support for other types of runnables -- AFAICS this PR isn't useful on its own.

[TroyKomodo]

On the comment about other runnable types, currently there is only TestOne and Run. The Check type is not used anywhere in the code see #20367

[TroyKomodo]

Honestly I would prefer if we go with a solution I have on my fork. See #20359 (comment)

By adding the runnables to the crate itself instead of having a global runnables.

The global runnables in my opinion were not well thought out since it's very likely in the case of external build systems that you have different targets for different runnables on the same crate. With the current design you can't really do this because we only have one label to substitute.

[Wilfred]

The global runnables in my opinion were not well thought out since it's very likely in the case of external build systems that you have different targets for different runnables on the same crate.

I'd actually argue that's a compelling case for global runnables! :)

By having a single command that rust-analyzer runs when you modify foo.rs, you can do nice things like merge duplicate build warnings (having a foo-proj and foo-proj-unittest is common in both buck and cargo). It also makes the JSON much smaller.

[TroyKomodo]

I'm not sure I follow, I am referring to a single crate that has different labels depending on the runnable.

Right now in the bazel implementation we always prioritize the test target to allow for the run test button on unit tests to work, naturally this means that the run binary button does not.

https://github.com/bazelbuild/rules_rust/blob/a44ac6e1bc66edcd4e45b9277eb51ff0981de742/tools/rust_analyzer/aquery.rs#L239-L245

It seems buck2 does something similar here https://github.com/facebook/buck2/blob/5a565c288858a249e7043a27c2e271c92daf35bd/integrations/rust-project/src/buck.rs#L368-L373.

There are only 2 runnables that work, TestOne and Run and currently as implemented nobody allows for both to be used at the same time. In my mind the ideal solution would be to have both working. The per crate runnables was one solution to the problem another solution might be to add multiple substitution strings (other than just {label}).

It also makes the JSON much smaller.

I don't believe this is an issue at all. This json is not being added to the VCS and if you use the discover command is simply piped and does not even write to disk. The argument that "larger json slower parse times" isnt really true. This file does not get nearly large enough for that to matter. In my case its just under 1MB (pretty-printed). I am writing it out because I want to debug the generated file when using discover command. These runnables are only added to the workspace crates not all dependencies.

> ls -lah .rust-project.bazel.json
-rw-r--r-- 1 troy users 935K Aug  9 13:07 .rust-project.bazel.json

[Wilfred]

Yeah, so test crates are tricky. In rust-project we don't just take the test crate, instead we attempt to merge the dependencies between the foo and foo-unittest crates (see facebook/buck2@f6ea0a4 and follow-up commits like facebook/buck2@4e4aaf8).

Being able to say "I've modified foo.rs, what command should I run?" is easier when you have a single check command. If you have N crates that include foo.rs and they each have a check command, what should rust-analyzer do? Can it run them all, can they be run in parallel, and how do you merge the results?

FWIW cargo deduplicates errors across crates as of Rust 1.55 and we have similar logic in a rust-check foo.rs command. This is nice because it's easy to write

// oops forgot cfg(test)
use some_test_helper;

and you get a squiggle complaining that your import isn't always used.

Regarding JSON size, I agree it's not a big deal. I've seen rust-project.json files up to ~20 MiB in the last week, which isn't a huge problem. Adding a lot of redundancy to the file format would make reading the files during debugging slightly more annoying, but that's all.

[TroyKomodo]

Yeah, so test crates are tricky. In rust-project we don't just take the test crate, instead we attempt to merge the dependencies between the foo and foo-unittest crates

It is largely the same in the bazel implementation.

Being able to say "I've modified foo.rs, what command should I run?" is easier when you have a single check command. If you have N crates that include foo.rs and they each have a check command, what should rust-analyzer do? Can it run them all, can they be run in parallel, and how do you merge the results?

I largely agree that having the same file be apart of multiple crates is problematic when deciding which commands to run. However this PR is specifically related to the run binary type. I do want to also bring in my changes on my RA fork which include changes to complete the implementation for #20367.

--

That being said, I am still unclear on what exactly is wrong with this PR particularly. Most of the arguments in this PR are related to changes which I intent to bring forward as a more general solution to #20359 so perhaps we should better move those discussions there?

[cormacrelf]

#18043 incorporated this change and has been merged, so this is done.