fix: ensure `rustfmt` runs when configured with `./` #15600

davidbarsky · 2023-09-12T18:45:11Z

(Hopefully) resolves #15595. This change kinda approaches canonicalization—which I am not a fan of—but only in service of making ./-configured commands run correctly.

Longer-term, I feel like this code should be removed once rustfmt supports recursive searches of configuration files or interpolation of values like ${workspace_folder} lands in rust-analyzer.

Testing

I cloned rustc, setup rust-analyzer as suggested in the rustc dev guide, saved and formatted files in src/tools/miri and compiler, and saw rustfmt (seemingly) correctly.

davidbarsky · 2023-09-12T18:46:24Z

@RalfJung Sorry for the delay: my initial, simple fix didn't work for simply specifying something like rustfmt. Please try this out, maybe?

davidbarsky · 2023-09-12T18:47:21Z

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

+                    let roots = snap
+                        .workspaces
+                        .iter()
+                        .flat_map(|ws| ws.workspace_definition_path())
+                        .collect::<Vec<&AbsPath>>();
+
+                    let abs: Option<AbsPathBuf> = roots.into_iter().find_map(|base| {
+                        let abs = base.join(rest);
+                        std::fs::metadata(&abs).ok().map(|_| abs)
+                    });


I'm not a fan of this approach, but I can't find a better way to determine what workspace a TextDocument is a part of.

Take a look at CargoTargetSpec::for_file, that function has to solve the same problem of relating a file to its workspace(s)

RalfJung · 2023-09-12T19:10:30Z

I'm afraid I won't be able to try this, I don't have the setup to build RA myself.

Why isn't this as simple as this?

let command = self.current_dir().unwrap().join(command);

Before #15564, command was implicitly resolved in the current_dir, now we just need to do that explicitly.

RalfJung · 2023-09-12T19:13:20Z

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

+
+            // to support rustc's suggested, default configuration
+            let mut cmd = match components.next() {
+                Some(std::path::Component::CurDir) => {


This should probably work with build/host/rustfmt as well; special-casing the leading ./ sounds very fragile.

Ye, we can just Path::join the rustfmt path onto the base here. If the path is absolute it'll replace the base, otherwise it"ll append which is exactly the semantics we want to have here.

Veykril · 2023-09-12T19:56:43Z

Longer-term, I feel like this code should be removed once rustfmt supports recursive searches of configuration files or interpolation of values like ${workspace_folder} lands in rust-analyzer.

This should already be working for vscode fwiw, we should probably have the default vscode settings in rust-lang/rust make use of these now that you mention that.

And ralf brought up a good point here, should relative paths be relative to the servers working dir, or relative to the workspace root. I think workspace does make more sense though, as the working dir of the server could be anywhere in theory.

davidbarsky · 2023-09-12T20:53:56Z

Appreciate the feedback, y'all.

And ralf brought up a good point here, should relative paths be relative to the servers working dir, or relative to the workspace root. I think workspace does make more sense though, as the working dir of the server could be anywhere in theory.

I'm going make to the path relative to the workspace root because I've seen the server be in weird spots.

davidbarsky · 2023-09-13T20:40:57Z

Sorry for the delay in responding/further elaborating, folks. Had a muscle spasm in my neck earlier and it's left me a little incapacitated. I'm substantially better now, though.

I'll address the comments in order and expand on details where I should've expanded earlier.

@RalfJung:

Why isn't this as simple as this?
let command = self.current_dir().unwrap().join(command);
Before #15564, command was implicitly resolved in the current_dir, now we just need to do that explicitly.

Prior to #15564, current_dir was whatever workspace the user has currently opened in their editor. The fact that ./ happened to work is, in my view, a happy and convenient accident. The trouble that emerges is that rust-analyzer-the-server can have multiple interpretations of what "the current workspace" is:

It can be whatever ProjectWorkspace::workspace_definition_path corresponds to, of which there can be multiple ProjectWorkspaces, so it might not be clear as to what workspace ./ is referring to.
- This is not something that I can reasonably tackle in this issue.
The parent of CargoTargetSpec::for_file might not necessarily be the workspace root. That might refer to, in Miri's case, src/tools/miri, not the the repository root, which the existing, suggested configuration would prefer.
- This becomes an issue when I saved src/tools/miri/src/machine.rs and used CargoTargetSpec::for_file to determine the owning workspace and joined ./build/host/rustfmt/bin/rustfmt. In my case, I got something along the lines of /Users/dbarsky/Developer/rust/src/tools/miri/Cargo.toml/./build/host/rustfmt/rustfmt, which does not exist.

I bring up these complications not to filibuster a solution, but rather, outline my thoughts, which are not settled. I'd like to fix this before Monday.

I'd personally prefer to avoid the solution that entails joining whatever custom command was passed with any of the paths, as that might foreclose an option to use $PATH-discovered commands in the future ¹. In that case, I see several paths moving forward:

Special-case the leading ./, which is not ideal and fragile. This is the existing PR, but given the feedback, that doesn't appear to be desired.
Ensure that build/host/rustfmt also works. I'm decently sure that I can make this work without removing the possibility of $PATH-discovery in the implementation by running std::fs::metadata(path) on the joined path, but it seems unfortunate to do that each time the file gets formatted. I suppose I can do that once during the language server's initialization phase.
Change the rustc dev guide to make use of ${workspacePath} instead of ./. Optionally, I can add errors to rust-analyzer informing users of the risks of ./, but I don't know if users would find this to be bothersome.
Add a $PATH-based interpolation key to the extension, but opting-out seems to be a bit trickier than opting in with workspace-relative paths.

Anyways, I can do any of the above options, just let me know what you prefer.

At work, I've setup rust-analyzer to point a ${userHome}-rooted path in the monorepo (thanks for that fix, Lukas!), but I've recently learned that the more correct approach would be to delegate to arcanist in the form of arc format. This would allow the team that manages the Rust toolchain at work to move things around not worry about breaking folks. ↩

RalfJung · 2023-09-13T21:50:36Z

I'd personally prefer to avoid the solution that entails joining whatever custom command was passed with any of the paths, as that might foreclose an option to use $PATH-discovered commands in the future

Ah, that's fair. My goal was to bring the behavior back to what it was before #15564, but I assume PATH-discovered commands worked then.

Sadly it doesn't seem like Command supports resolving the binary in a different path than what it is executed in.

However, applying shell logic, what should work is:

if the command name contains no path seperator, use it as-is and rely on PATH discovery
otherwise, join it from the current working directory

I appreciate that the current working directory (of the server!) is arbitrary and might not match user intuition, but it matches the behavior before #15564, and the primary priority right now should be to fix the regression. Getting a proper idea of which path to base this on can wait, IMO.

Veykril · 2023-09-14T07:19:00Z

Taking a look at this in a bit, but setting the path to "${workspaceFolder}/build/host/rustfmt/bin/rustfmt" should yield the previous behavior for the time being as well.

RalfJung · 2023-09-14T07:22:29Z

Oh, that already works in currently released RA? I can try that.

RalfJung · 2023-09-14T07:54:39Z

Yes that seems to work. :) I filed rust-lang/rust#115836 to update the docs.

Would be great to have this documented in the setting's doc string.

Veykril · 2023-09-14T08:02:33Z

Well, technically speaking this is a VSCode feature that we re-implemented in the extension for a very few selected ones, https://code.visualstudio.com/docs/editor/variables-reference
(so this only applies to VSCode)

Veykril · 2023-09-14T09:38:05Z

The parent of CargoTargetSpec::for_file might not necessarily be the workspace root. That might refer to, in Miri's case, src/tools/miri, not the the repository root, which the existing, suggested configuration would prefer.
- This becomes an issue when I saved src/tools/miri/src/machine.rs and used CargoTargetSpec::for_file to determine the owning workspace and joined ./build/host/rustfmt/bin/rustfmt. In my case, I got something along the lines of /Users/dbarsky/Developer/rust/src/tools/miri/Cargo.toml/./build/host/rustfmt/rustfmt, which does not exist.

Huh that is odd, miri is a member of the main workspace so that should work here just fine?

However, applying shell logic, what should work is:

if the command name contains no path seperator, use it as-is and rely on PATH discovery

otherwise, join it from the current working directory

That sounds like a good idea to me (replacing working directory with workspace root).

davidbarsky · 2023-09-15T14:55:39Z

That sounds like a good idea to me (replacing working directory with workspace root).

Will take that approach, then!

davidbarsky · 2023-10-03T20:46:48Z

Whoops, sorry: I thought I had pushed this a while ago, but I updated this PR according to feedback. It turns out that I was using CargoTargetSpec::cargo_toml instead of CargoTargetSpec::workspace_root, which does find the rustc workspace root.

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

Veykril · 2023-10-06T10:50:09Z

@bors r+

bors · 2023-10-06T10:50:11Z

📌 Commit 0973d78 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-10-06T10:51:18Z

⌛ Testing commit 0973d78 with merge b39875b...

…r=Veykril fix: ensure `rustfmt` runs when configured with `./` (Hopefully) resolves #15595. This change kinda approaches canonicalization—which I am not a fan of—but only in service of making `./`-configured commands run correctly. Longer-term, I feel like this code should be removed once `rustfmt` supports recursive searches of configuration files or interpolation of values like `${workspace_folder}` lands in rust-analyzer. ## Testing I cloned `rustc`, setup rust-analyzer as suggested in the [`rustc` dev guide](https://rustc-dev-guide.rust-lang.org/building/suggested.html#configuring-rust-analyzer-for-rustc), saved and formatted files in `src/tools/miri` and `compiler`, and saw `rustfmt` (seemingly) correctly.

bors · 2023-10-06T10:55:56Z

💔 Test failed - checks-actions

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

Veykril · 2023-10-06T11:32:46Z

@bors r+

bors · 2023-10-06T11:32:48Z

📌 Commit b3ebc9a has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-10-06T11:33:54Z

⌛ Testing commit b3ebc9a with merge b1f89a8...

bors · 2023-10-06T11:49:19Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b1f89a8 to master...

fix: ensure rustfmt runs when configured with ./

2974416

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2023

davidbarsky commented Sep 12, 2023

View reviewed changes

RalfJung reviewed Sep 12, 2023

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2023

address PR feedback.

a8ec77d

davidbarsky requested a review from Veykril October 3, 2023 20:47

Veykril reviewed Oct 4, 2023

View reviewed changes

crates/rust-analyzer/src/handlers/request.rs Outdated Show resolved Hide resolved

Veykril reviewed Oct 6, 2023

View reviewed changes

crates/rust-analyzer/src/handlers/request.rs Outdated Show resolved Hide resolved

Veykril reviewed Oct 6, 2023

View reviewed changes

crates/rust-analyzer/src/handlers/request.rs Outdated Show resolved Hide resolved

Check for both path separators on windows

b3ebc9a

Veykril force-pushed the davidbarsky/broken-rustfmt-in-ra branch from 5522357 to b3ebc9a Compare October 6, 2023 11:26

bors merged commit b1f89a8 into rust-lang:master Oct 6, 2023

davidbarsky deleted the davidbarsky/broken-rustfmt-in-ra branch October 23, 2023 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure `rustfmt` runs when configured with `./` #15600

fix: ensure `rustfmt` runs when configured with `./` #15600

davidbarsky commented Sep 12, 2023

davidbarsky commented Sep 12, 2023

davidbarsky Sep 12, 2023

Veykril Sep 12, 2023

RalfJung commented Sep 12, 2023

RalfJung Sep 12, 2023

Veykril Sep 12, 2023

Veykril commented Sep 12, 2023 •

edited

Loading

davidbarsky commented Sep 12, 2023

davidbarsky commented Sep 13, 2023

RalfJung commented Sep 13, 2023 •

edited

Loading

Veykril commented Sep 14, 2023

RalfJung commented Sep 14, 2023 via email

RalfJung commented Sep 14, 2023

Veykril commented Sep 14, 2023

Veykril commented Sep 14, 2023

davidbarsky commented Sep 15, 2023

davidbarsky commented Oct 3, 2023

Veykril commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

Veykril commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

fix: ensure rustfmt runs when configured with ./ #15600

fix: ensure rustfmt runs when configured with ./ #15600

Conversation

davidbarsky commented Sep 12, 2023

Testing

davidbarsky commented Sep 12, 2023

davidbarsky Sep 12, 2023

Choose a reason for hiding this comment

Veykril Sep 12, 2023

Choose a reason for hiding this comment

RalfJung commented Sep 12, 2023

RalfJung Sep 12, 2023

Choose a reason for hiding this comment

Veykril Sep 12, 2023

Choose a reason for hiding this comment

Veykril commented Sep 12, 2023 • edited Loading

davidbarsky commented Sep 12, 2023

davidbarsky commented Sep 13, 2023

Footnotes

RalfJung commented Sep 13, 2023 • edited Loading

Veykril commented Sep 14, 2023

RalfJung commented Sep 14, 2023 via email

RalfJung commented Sep 14, 2023

Veykril commented Sep 14, 2023

Veykril commented Sep 14, 2023

davidbarsky commented Sep 15, 2023

davidbarsky commented Oct 3, 2023

Veykril commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

Veykril commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

bors commented Oct 6, 2023

fix: ensure `rustfmt` runs when configured with `./` #15600

fix: ensure `rustfmt` runs when configured with `./` #15600

Veykril commented Sep 12, 2023 •

edited

Loading

RalfJung commented Sep 13, 2023 •

edited

Loading