Seemingly incompatible design between rust-project.json and checkOnSave.overrideCommand

[googleson78]

Hey all!

I have a couple of questions:

1. Is it expected that in a code base where with Cargo you would have many Cargo.toml files spread across multiple folders, you only use one rust-project.json?
2. Is there a way to give some info on what file we want to "cargo check" (i.e. what file triggered the invocation of checkOnSave.overrideCommand)? For example, passing it as an argument to checkOnSave.overrideCommand.

My personal investigation has reached "yes, you should have only one rust-project.json" for 1.:

• in rust-project.json you reference deps with their index in the array for the same rust-project.json file, and I don't know how this could be compatible with multiple rust-project.jsons
• trying to run rust-analyzer with multiple rust-project.jsons doesn't work - e.g. cross rust-project.json goto def doesn't work

and "no" for 2.:

• it seems that nothing dynamic is passed to the executable when it's run (in the source code) - only the presupplied args and the environment from checkOnSave.extraEnv are passed, which both appear to be static

This combination is very unfortunate, because now, there is no way for the tool that's going to be used instead of cargo check to know what it should be building - in a huge repo, this is not viable.

I assume that when we're using cargo this "what should I build" issue is resolved by the fact that cargo check is run in the root of the workspace, which is determined by a Cargo.toml file. However, in the rust-project.json, this is no longer a solution, because we can have only one rust-project.json file, hence in order to be correct, the only thing checkOnSave.overrideCommand could do is build everything in the workspace.

My proposed solutions are to do one of

• "fix" 2. so that we pass some info to checkOnSave.overrideCommand (seems to be much easier)
• "fix" 1. to allow autodiscovery and combining of rust-project.json files - seems to be much more difficult to implement, but perhaps it's a better approach in the long run, so that the assumptions made for cargo based projects are also valid for rust-project.json based ones?

I would be happy to implement some version of "fix" 2., if we could come to an agreement on what that would be.

Context:
I'm trying to implement diagnostics support for rust-analyzer+rules_rust, which relies on checkOnSave.overrideCommand/cargo check.

[Veykril]

your second point will be fixed by config interpolation/substitution support, see #13297. I'm currently thinking about how what to use a interpolation syntax and the like.

[googleson78]

I'm still a bit unclear on the details - what specifically do you envision being interpolatable that will solve this issue? Something like $loadedFile (or whatever the lsp terminology is on this)?

[Veykril]

Basically we would expose some kind of interpolation var that when used will change how we invoke the command (you already took a look at #13128 it seems). It's very good timing that you are raising this for rust-project.json now and not later as well, so I can incorporate that into my brainstorming :). In #13128 the behaviors of how we invoke it are basically gonna be, run command once in the root dir, per workspace in the root with the manifest path if the variable was used or per workspace in the workspace dir. From my understanding here per workspace in the root with the manifest path if the variable was used would work for your use case right? If not it would be great if you could elaborate on your requirements (sorry if I ask you to repeat yourself here, I just want to clear on things).

Of course things would change if we allow multiple rust-project.json, though I'd have to look into that first to know if that is actually something we want to have.

[googleson78]

To be on the same page, I'm making the following assumptions about what things mean (please do correct me!):

root - the working directory of the rust-analyzer process
workspace - in the cargo world, a place where a Cargo.toml is located. In the rust-project.json world - the location where rust-project.json is located
manifest file - Cargo.toml/rust-project.json

I guess from the things you mentioned I would need
d) none of the above - run once in the root dir and also provide information about the file being checked (e.g via an interpolation var), so that my check command (in this case bazel) can somehow figure out what targets it needs to build/check
In the terminology of the PR you have, this sounds like once_per_root+something gets interpolated.

Maybe I'm missing something about per workspace in the root with the manifest path if the variable was used but I only really want to check the current file that the user is looking at, which I can approximate by checking all the targets/crates that that file is a part of. I don't want to run the check command for every "workspace" that is available.

I guess the per workspace in the root with the manifest path if the variable was used option would work if we could somehow teach rust-analyzer about custom manifest files (in my case that would be BUILD files that include specific strings in them, e.g. rust_library, rust_binary or rust_test). But again, maybe I'm misunderstanding what a "manifest file" is supposed to be.

I guess in some workflows it is also sensible to check all of the files everywhere in the project at once (so you immediately get an overview of all the errors in a project), which we can already do today, but again, would be slow.

EDIT: I now realise that due to there being only one workspace in the rust-project.json variant, per workspace in the root with the manifest path if the variable was used is half-right, except again, I need the path to the file being processed, and not necessarily the path to the manifest (in this case rust-project.json)

[Veykril]

d) none of the above - run once in the root dir and also provide information about the file being checked (e.g via an interpolation var), so that my check command (in this case bazel) can somehow figure out what targets it needs to build/check
In the terminology of the PR you have, this sounds like once_per_root+something gets interpolated.

This doesn't really make sense though, what would r-a pass to bazel? If we only invoke the command once there is no dynamic data at play here, at which point you can just encode whatever your check command needs in the additional args.

Maybe I'm missing something about per workspace in the root with the manifest path if the variable was used but I only really want to check the current file that the user is looking at, which I can approximate by checking all the targets/crates that that file is a part of. I don't want to run the check command for every "workspace" that is available.

Ah, so you want the file path of whatever file was saved. That would be something we could add as an interpolation var for checkOnSave. To clarify #13128 technically only adds two things (it's independent from interpolation in a sense), per_workspace invocations and once_in_root. Usages of interpolated variables then may change additional things. So for this use case the idea would be for you to use once_in_root and an interpolation variable that expands to the file path being saved.

I guess the per workspace in the root with the manifest path if the variable was used option would work if we could somehow teach rust-analyzer about custom manifest files (in my case that would be BUILD files that include specific strings in them, e.g. rust_library, rust_binary or rust_test). But again, maybe I'm misunderstanding what a "manifest file" is supposed to be.

A manifest file would be the Cargo.toml of the relevant workspace, I don't really know rust-project.json too well but given it is there to not be dependent on cargo I would imagine the term manifest to not mean anything there, so the var wouldn't make sense for rust-project.json projects.

Thanks for clarifying these things, this is really helpful in figuring things out here :)

[woody77]

My project runs clippy on the entire (8000 crate) codebase. The first run is slow (obviously), but afterwards it's surprisingly fast to run clippy across that many targets.

[googleson78]

Thanks for the very quick responses! Your last comment wrt the file path of the saved file being available as an interpolation variable is spot on with what I was envisioning as a concrete solution.

I'll proceed with my prototype implementation that just builds everything for now, and hopefully I can get a lucky repeat of what @woody77 encountered with only the first run taking a lot of time.

[Veykril]

Coming back to this (I am currently digging into ProjectJson a bit to understand how it works better)

is it expected that in a code base where with Cargo you would have many Cargo.toml files spread across multiple folders, you only use one rust-project.json?

A rust-project.json is a replacement per top level workspace Cargo.toml, not per crate Cargo.toml. So each separate cargo workspace would get a rust-project.json I believe.

[googleson78]

What do

• cargo workspace
• "top level workspace Cargo.toml"

mean?

Most likely I'm missing something, but I don't see how you can have multiple rust-project.jsons and have the crates within depend on each other in order to have IDE features work while crossing the boundaries of a single rust-project.json.

[bjorn3]

A cargo workspace is a collection of crates which share a lockfile for dependency resolution and the target/ build directory. You can build all crates in a cargo workspace by passing --workspace to cargo. This is what rust-analyzer does. The top level workspace Cargo.toml is the Cargo.toml file at the root of the workspace. It contains a list of all members of the workspace as well as the [workspace] section to indicate that it is a cargo workspace.

[Veykril]

If your workspaces depend on each other, you'll have to list the dependencies and crates explicitly for both, that is you'll have to repeat yourself for the crates that are used in multiple workspaces as rust-project.json doesn't allow workspace hierarchies like cargo does.

[googleson78]

@Veykril, as per your comment here - #13297 (comment), would you accept a patch adding very small bits of interpolation, likely only path to the file that was just saved? I think that would be enough for the rules_rust application that I originally opened this for.

[Veykril]

I have already opened #13528 which adds parts of this. Not the $saved_file one that you desire yet though (I do want to have that as well), because there is a slight design problem here. r-a invokes checkOnSave on all workspaces when it starts up to pre-populate diagnostics once, $saved_file wouldn't work there though. So I am currently pondering what the best choice of behavior here is, either don't run checkOnSave on start up if $saved_file is used or put some dummy file in there to notify this being the initial run (whatever that dummy file would be).