fix(config): search for config file recursively on --config-path

[rami3l]

Note

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

LLM has been used to analyze the existing usage of directory walking behavior.
The implementation of this patch is otherwise fully manual.

Closes #4660 based on the design discussed in #4660 (comment).

Background

The --config-path help description says:

        --config-path [Path for the configuration file]
                        Recursively searches the given path for the
                        rustfmt.toml config file. If not found reverts to the
                        input file path

However this description is not quite appropriate because:

• It's unclear whether the path being passed in is a directory, or a TOML file.
• Recursive search doesn't really happen when this argument is passed to rustfmt.

To be more precise, the current semantics of --config-path is that when it receives a path, it chooses to enter one of the following modes in the below fallback order:

• Base dir mode: If path is exists and metadata says it's a dir, then search for predefined names ([".rustfmt.toml", "rustfmt.toml"]) under path only. Bails out if none is found.
• TOML mode: If path exists and the metadata says it's not a dir, then interpret it as a TOML file. Bails out if none is found.

Proposed solution

This PR changes it to:

• Base dir mode: If path terminates with /, or if it exists and metadata says it's a dir, then search for predefined names ([".rustfmt.toml", "rustfmt.toml"]) under path and all its parents. Bails out if none is found.
• TOML mode: Otherwise, interpret it as a TOML file by searching for its filename under its parent dir and all parents of the latter. Bails out if none is found.

[rami3l]

I still some concerns regarding the distinction between the dir mode and the TOML mode, so reconverting to draft.

[rami3l]

Addressed the concern regarding mode switching; updated both the PR description accordingly to reflect the new design. This PR is ready for review 🙏

[matthewhughes934]

is .is_dir not a sufficient check here?

View changes since the review

[rami3l]

@matthewhughes934 Originally that had been largely sufficient, but with the new semantics, the path to be passed in through the base dir mode might not exist immediately, and has to be found via recursive search. In this case, a trailing / makes it clear that we are looking for a directory rather than a TOML file.

This should be able to match how rust-analyzer is calling rustfmt: it is passing the parent directory of the file, which itself is passed in via stdin.

[matthewhughes934]

but with the new semantics, the path to be passed in through the base dir mode might not exist immediately

Can you explain that case more? If someone passes as path that doesn't existing to --config-path I would expect it to error out.

My primary concern here is that / is not a path separator on Windows mostly because of Windows weirdness I expect simply checking for \ might also not be sufficient here

[rami3l]

Can you explain that case more? If someone passes as path that doesn't existing to --config-path I would expect it to error out.

@matthewhughes934 I totally get your point no, and my apologies for potential confusion. Here is the full background:

Firstly, when rust-analyzer tries to format dir/file.rs, it wants to pass --config-path=dir to it, and that directory may or may not have the expected .rustfmt.toml or rustfmt.toml, so a recursive search should be initiated starting from there. However, due to #4660 being unresolved, rust-analyzer has instead used the hack of cding to dir when running rustfmt.

From there, I'd agree that --config-path=dir should bail out when dir doesn't exist.

Secondly, however, if the user wants to use a different name for the config file, taking the example mentioned in #4660 (comment):

rustfmt = { extraArgs = { "+nightly", "--config-path=.rustfmt.unstable.toml" } },

The file (.rustfmt.unstable.toml in the above example) is better placed at CWD, but it may totally be at a different place. This time I OTOH don't want --config-path=file to bail out immediately because I may want to use ../../file etc.

The crux of the problem here, it seems to me, is that --config-path is overloaded with two meanings and this has made it hard for us to disambiguate, for which some manual intervention must be involved.

The trailing / is, as you said, probably not the best idea for disambiguation.

[matthewhughes934]

Secondly, however, if the user wants to use a different name for the config file, taking the example mentioned in #4660 (comment):

rustfmt = { extraArgs = { "+nightly", "--config-path=.rustfmt.unstable.toml" } },

The file (.rustfmt.unstable.toml in the above example) is better placed at CWD, but it may totally be at a different place

Could you expand more on this use case, where is .rustfmt.unstable.toml expected to be? Is the use case something like: I have opened my editor at the root of a repo with several packages, each of which might have different rustfmt.toml, so if I edit a file in one of them I want the config from that package to apply?

my_repo
|
|---- first_crate/
    |---- rustfmt.toml
    |---- src/ # files under here use the rustfmt.toml above
|---- second_crate
    |---- rustfmt.toml
    |---- src/ # files under here use a different config

[rami3l]

@matthewhughes934 TLDR, I'll interpret this case as "I want to override the config file name, but at the same time I also want to override the base directory, but unfortunately they are under the same flag".

In the case I posted above, I have my_repo/.rustfmt.unstable.toml for sorting import sections (unfortunately the feature is unstable-only so we want to override the file name to make it clear, the idea is borrowed from rustls).

Unfortunately, rustfmt isn't always invoked at the workspace root. For example, to format my_repo/first_crate/src/foo/bar.rs, the CWD for rust-analyzer is my_repo/first_crate/src/foo/. So the same --config-path=.rustfmt.unstable.toml is not resolved to a valid path in this case. And even if rust-analyzer is updated to not cd the rustfmt instance before formatting, it should still be launched at my_repo/ for this to work. Same thing for cargo +nightly fmt -- --config-path=.rustfmt.unstable.toml, because the config path is forwarded to rustfmt verbatim.

Looking back, of course you can argue that it's not rustfmt's responsibility of getting the base directory right so that this config can work. If that is the case for you, I think I can combine our ideas and propose the following semantics, please tell me if it looks better to you:

1. Base dir mode (identical with my proposal): If path exists and metadata says it's a dir, then search for predefined names ([".rustfmt.toml", "rustfmt.toml"]) under path and all its parents. Bails out if none is found.
2. TOML mode (identical with the current mainline): If path exists and the metadata says it's not a dir, then interpret it as a TOML file. Bails out if none is found.

If you agree with this, I can change the feature commit quite quickly to reflect it.

[matthewhughes934]

1. Base dir mode (identical with my proposal): If path exists and metadata says it's a dir, then search for predefined names ([".rustfmt.toml", "rustfmt.toml"]) under path and all its parents. Bails out if none is found.
2. TOML mode (identical with the current mainline): If path exists and the metadata says it's not a dir, then interpret it as a TOML file. Bails out if none is found.

👍 this sounds good to me, I think the directory behaviour sounds closer to what's documented in the help output.

It might be worth further discussing the file behaviour (there's also some discussion on it with #5206), but that's best done separately to this change.

CC @ytmimi since you were discussing on the original issue