search

rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.3k stars 1.61k forks source link

rustfmt with overrideCommand gets invoked with incorrect relative paths in complex projects #18222

Open mrkajetanp opened 1 month ago

mrkajetanp commented 1 month ago

When setting rust-analyzer.rustfmt.overrideCommand to a non-absolute path, in complex projects rustfmt ends up getting invoked with an incorrect path and as a result LSP formatting no longer works.

For instance, the rust-lang/rust project requires the overrideCommand to be set to (relative to the checkout root) build/host/rustfmt/bin/rustfmt. However, if the path is left at that, rust-analyzer will join it with its workspace root, which can be different from the checkout root.

From glancing at the sources, this line here is the culprit.

For instance, when editing a file under src/bootstrap, rust-analyzer appears to find the Cargo.toml in src/bootstrap as opposed to the one at the actual root. Trying to format the file then tries to run the command (..)/src/bootstrap/build/host/rustfmt/bin/rustfmt, which does not exist and thus the formatting fails.

Editors currently get around this by just setting an absolute path to the rustfmt binary directly.

This could be solved by an option similar to invocationStrategy for rustfmt, which would make the override command get joined with the overall root rather than the specific workspace root.

I could probably implement some fix for this myself, just wanted to ask for opinions from people involved here before trying to implement anything.

rust-analyzer version: rust-analyzer 1.83.0-nightly (12b26c1 2024-09-07) rustc version: rustc 1.83.0-nightly (12b26c13f 2024-09-07)

Veykril commented 1 month ago

"checkout root" as a concept doesn't really exist here, a checkout is a git thing (I presume you mean the repo root). The only thing we can work with here is the workspace root, which is what the editor reports as the folder being opened. So what we do here is correct as far as I am concerned.

mrkajetanp commented 1 month ago

I suppose the terminology I should have used is "workspace root" and "project root" which is more consistent with how the manual refers to those, you're right. Is there any specific reason why we wouldn't be able to replicate what is already being done with invocationStrategy?

rust-analyzer.cargo.buildScripts.invocationStrategy (default: "per_workspace") Specifies the invocation strategy to use when running the build scripts command. If per_workspace is set, the command will be executed for each Rust workspace with the workspace as the working directory. If once is set, the command will be executed once with the opened project as the working directory. This config only has an effect when rust-analyzer.cargo.buildScripts.overrideCommand is set.

The problem I'm describing is that the rustfmt override command always gets the workspace root prepended to it, while the project root is almost always a better choice. At the very least having an option to change it like for buildScripts would be enough. Either the workspace root is the same as the project root, which is usually the case and then it doesn't matter, or it's different and then I'm pretty sure users would be expecting the path to be relative to the project root rather than relative to whichever subproject/crate they happen to be editing.

mrkajetanp commented 1 month ago

Having done some more digging, there is a discrepancy where if I set rust-analyzer.procMacro.server to a custom path it will be joined to the project root (config.root_path), but rustfmt will be joined to a different path (spec.workspace_root) as described above. The PR linked above fixes this and makes it work as expected for me.