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).

[googleson78]

There could be a initialCheckOnSave that gets run instead of checkOnSave for the initial run? And it defaults to the value of checkOnSave if it's not set?

[Veykril]

Ye, that is a third option. I was hoping not to having to split the thing into two but we might have to as it does sound the most sensible without losing functionality.

[woody77]

The initial diagnostics run probably shouldn't be "initialCheckOnSave", but something like diagnosticsCheckOverrideCommand (although the linkage between it and checkOnSave is more obvious with the former)

[matklad]

For 1, as rust-project.json is essentially a reflection of the global "Project" in-memory structure of rust-analyzer, there should be only one of those. But it's totally valid to write some logic somewhere which would dynamically piece together rust-project.json required for some specific subview into the monorepo. I've explaing this a bit here: #13941

[matklad]

For 2., checkOnSave is fundamentally a hack which doesn't belong to rust-analyzer, which we only have because the world is such an imperfect place :)

I would suggest these three approaches to solve the "checkOnSave with bazel" class of problems, in the order of their technical soundness (which might not correspond to the order according to the immediate UX)

• Use https://build-server-protocol.github.io between the editor and the build system, bypassing rust-analyzer entirely. This is the way, what we want here is "run this particular kind of build after save and display results to the user", there's nothing here which requires semantic knowledge of Rust, and this functionality should be shared across languages (eg, hte same code should drive "check on save" for both Rust and C++). I don't know what's the practical state of BSP.
• If you are enjoying the constraint of only a single editor (eg, everyone uses VS Code, or some in-the-browser editor), then writing plugin for that editor which would call bazel and render its diagnostics to the user is a nice way to do something correct and useful without blocking on BSP. Eg, if BSP is not good, spend oneday writing the extension for VS Code, and that's it (I do think that the amount of meaningful code to write here is very small, and that the maintainance burden would be light).
• Well, as everyone already speaks LSP, and rust-analyzer speaks LSP, we can shovel this through ra. This is completely horribly wrong from the architectural point of view, but might be the practical path of least resistance. For this solution, allowing to override check-on-save command and adding some hooks to say what changes is not too bad (though, I'd pass a generic "what changed" object, rather than a file path. The common case should be a list of changed files and a list of changed crates (which crate correponds to this file is actually a tiny bit of semantic info ra is good at here!))

[googleson78]

This is getting slightly off-topic, but I'm interested in what you said:

"Use build-server-protocol.github.io between the editor and the build system, bypassing rust-analyzer entirely."

Does rust-analyzer not use some of the diagnostics that cargo/rustc report in order to drive things, e.g. if cargo/rustc reports something like "unknown identifier", does rust-analyzer not then try to suggest an "import missing identifier" action? I'm curious because this is basically how haskell-language-server and ghc work together - ghc reports errors and HLS basically parses those errors into code actions/code lenses.

In there, the only role of the build system is to provide a working ghc session with all the necessary loaded packages, i.e. there is no dependency on the underlying build system beyond that.

[bjorn3]

Does rust-analyzer not use some of the diagnostics that cargo/rustc report in order to drive things, e.g. if cargo/rustc reports something like "unknown identifier", does rust-analyzer not then try to suggest an "import missing identifier" action?

Rustc actually creates these suggestions on it's own. It exposes them in a machine readable way when using the json error format to allow other tools like cargo fix and rust-analyzer to apply them verbatim. AFAIK rust-analyzer never feeds error messages generated by rustc back into any analysis or generates it's own suggestions based on rustc error messages.

[matklad]

how haskell-language-server and ghc work together - ghc reports errors and HLS basically parses those errors into code actions/code lenses.

If all you do is runing compiler and displaying its errors, the LSP is conceptually not necessary. What you rather want is just some common JSON format for rich error messages with multiple spans, fixits and what not. So, the editor just runs the build system, parses errors, and displays all bells and whistles in editor, without requiring any kind of persistent process, and without knowing any details about the editor.

Sadly, we don't have such common JSON format, but we do have LSP, which creates the pressure to pipe everything through LSP, even though it could have been a good old UNIX style batch process.

[mibu138]

Just wanted to poke at this, coming from related issue #14217. I'm not sure what the takeaway is from this conversation. Is $saved_file going to make it in as an interpolation variable? Is there some workaround I could employ for the time being? Willing to hack r-a source code as a temporary fix.

I'm trying to use rustc directly to get these messages, and having r-a pass it the current file seems like a necesary thing.

[googleson78]

The agreement seems to be that the $saved_file thing might be accepted, as a stopgap solution, but there is no one working on that currently, afaik.

[cameron-martin]

Use https://build-server-protocol.github.io/ between the editor and the build system, bypassing rust-analyzer entirely.

It doesn't look like the BSP supports passing compilation errors to the client in a common format. Nor does it differentiate between compiling and typechecking. These just seem like missing features though. I've opened an issue here: build-server-protocol/build-server-protocol#646