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. Remains unchanged as per fix(config): search for config file recursively on --config-path #7115 (comment).

[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]

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

[rami3l]

@matthewhughes934 No problem, I have rewritten the PR as suggested. Please review!

[matthewhughes934]

I think it's clearer if we add a separate function for the user dir logic, rather than overloading with a bool argument.

Suggested change

	Some(path) if path.is_dir() => resolve_project_file(path, false),
	Some(path) if path.is_dir() => resolve_project_file(path).or(config_from_user_dirs())?,

where

fn config_from_user_dirs() -> Result<Option<PathBuf>, Error> {
    for dir in [dirs::home_dir(), dirs::config_dir()].iter().flatten() {
        if let Some(path) = get_toml_path(&dir)? {
            return Ok(Some(path));
        }
    }
    Ok(None)
}

View changes since the review

[matthewhughes934]

what does this part of the verify that the previous one didn't?

View changes since the review

[matthewhughes934]

I don't think this test catches anything: the file is already formatted correctly, and I don't think rustfmt some_file.rs will print anything to stdout if it did format (but it will with --check) I think

Suggested change

	let args = ["--config-path", src_dir, &src_file];
	let (stdout, stderr) = rustfmt(&args);
	assert_eq!(stderr, "");
	// Due to `disable_all_formatting = true` in `tests/config/issue_4660/inner_lib/rustfmt.toml`,
	// the source file should not be modified.
	assert_eq!(stdout, "");
	let args = ["--config-path", src_dir, "--check", &src_file];
	let (stdout, stderr) = rustfmt(&args);
	assert_eq!(stderr, "");
	// Due to `disable_all_formatting = true` in `tests/config/issue_4660/inner_lib/rustfmt.toml`,
	// the source file should not be modified.
	assert_eq!(stdout, "");

The main.rs should be rewritten to something that would format, e.g.

fn main() {
    println!(
        "Hello, world!");
}

View changes since the review