Recursive `--config-path` doesn't recurse

[lnicola]

Describe the bug

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 it doesn't seem to recurse:

$ rustfmt --config-path ~/Downloads/test_issue/my_crate/src/main.rs --edition 2018 < ~/Downloads/test_issue/my_crate/src/main.rs
Could not parse TOML: expected an equals, found an identifier at line 1 column 4

$ rustfmt --config-path ~/Downloads/test_issue/my_crate/src/ --edition 2018 < ~/Downloads/test_issue/my_crate/src/main.rs
Error: unable to find a config file for the given path: `~/Downloads/test_issue/my_crate/src/`

$ rustfmt --config-path ~/Downloads/test_issue/my_crate/src --edition 2018 < ~/Downloads/test_issue/my_crate/src/main.rs
Error: unable to find a config file for the given path: `~/Downloads/test_issue/my_crate/src`

$ rustfmt --config-path ~/Downloads/test_issue/my_crate --edition 2018 < ~/Downloads/test_issue/my_crate/src/main.rs
fn fjsdlkfjklsdajflksdjlfdasdsadsadsaadkjdskljflkasj_jflksjdklfjsdlkjflkdjslkfjas(a: i32, b: i32, c: i32) {
    println!("TEST");
}

fn main() {
    fjsdlkfjklsdajflksdjlfdasdsadsadsaadkjdskljflkasj_jflksjdklfjsdlkjflkdjslkfjas(1, 2, 3);
    println!("Hello, world!");
}

$ rustfmt --config-path ~/Downloads/test_issue/ --edition 2018 < ~/Downloads/test_issue/my_crate/src/main.rs
Error: unable to find a config file for the given path: `~/Downloads/test_issue/`

To Reproduce

Test project is the one from rust-lang/rust-analyzer#6973 (comment)

Expected behavior

rustfmt --config-path follows the same logic as rustfmt

Meta

• rustfmt version: rustfmt 1.4.32-nightly (216a643 2021-01-16)
• From where did you install rustfmt?: rustup
• How do you run rustfmt: rustfmt

[calebcartwright]

I want to say that this has already been fixed in source, but hasn't been backported to a 1.x release yet. Will dig back through the issue tracker to check though, and if so will see if I can't get that fix cherry-picked for the next v1.4.33 release

[lnicola]

Status: no change, this still isn't working.

[BenjaminBrienen]

status of this?
https://github.com/rust-lang/rust-analyzer/blob/2b485d7f2360b2878dd2f4072ff39d82d76bed4d/crates/rust-analyzer/src/handlers/request.rs#L2250

[ytmimi]

No update. I don't think anyone is working on this.

[rami3l]

@ytmimi I think I can probably have a look at this? It's damaging rustup's use of rustfmt 👀

[ytmimi]

@rami3l That would be great. Let me know if you have questions once you get started.

[rami3l]

@ytmimi It looks like config_path() is merging two options in one which might be confusing. Currently it works like so:

rustfmt/src/config/mod.rs

Lines 537 to 548 in 2f80bd5

	Some(path) if path.is_dir() => {
	let config_file_path = get_toml_path(path)?;
	if config_file_path.is_some() {
	Ok(config_file_path)
	} else {
	config_path_not_found(path.to_str().unwrap())
	}
	}
	Some(path) => Ok(Some(
	// Canonicalize only after checking above that the `path.exists()`.
	path.canonicalize()?,
	)),

I agree that, in the first place, --config-path=<dir> should start the path search for [".rustfmt.toml", "rustfmt.toml"], and continue searching with dir := dir.parent if none is found:

rustfmt/src/config/mod.rs

Lines 369 to 380 in 2f80bd5

	loop {
	match get_toml_path(&current) {
	Ok(Some(path)) => return Ok(Some(path)),
	Err(e) => return Err(e),
	_ => (),
	}
	// If the current directory has no parent, we're done searching.
	if !current.pop() {
	break;
	}
	}

However there is another operation mode, where you can pass --config-path=<toml> to use a TOML proper (and it could be using any reasonable name rather than the predefined ones), which my rust-lang/rustup#5065 depended upon.

To fully mirror the logic (and to resolve rust-lang/rustup#5069), we need to recursively search from the given path in both modes. So I propose the following semantics for the latter mode: split <toml> into <dir> and <filename>, then repeat the search like the former case, starting from <dir> and look for ["<filename>"]. Does that sound reasonable to you?

---

To provide more background to the above design, in rust-lang/rustup#5069 I have proposed the following config which doesn't work today (but it works if I call rustfmt +nightly --config-path=.rustfmt.unstable.toml <file> directly at the project root):

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

... this is because rust-analyzer wants to evoke rustfmt with CWD set to the given file's containing dir:

https://github.com/rust-lang/rust-analyzer/blob/6aeeb8cf02741e5da07a1615a05faa506cecb21a/crates/rust-analyzer/src/handlers/request.rs#L2418-L2424

... and surely enough it won't find that file, because the TOML is at the project root as mentioned above.

[rami3l]

@ytmimi Many thanks for approving my patch in #7115!

However, since the scope of that change had been strictly limited to --config-path=<dir> because of @matthewhughes934's suggestion, my original concern of the interactions between recursive paths searching and --config-path=<toml> remains unresolved.

Looking at the expected usage in r-a as linked above, do you think I should mirror the current design in r-a or go back to the drawing board in rustfmt (my personal preference would be the former)? In other words, do you consider recursive searching WRT --config-path=<toml> a possible feature to be implemented here? 🙏

---

Update: I've added rust-lang/rust-analyzer#23412 to track the r-a part of this feature.

[ytmimi]

I've got to think on whether the recursive behavior should apply for --config-path=<toml>. My first thought is that if you provide --config-path=<toml> and the file doesn't exist then that should be an error (or at least a warning) because you're pointing rustfmt at a configuration that doesn't exist.

[rami3l]

@ytmimi No problem! I will fix the rest on r-a's side then. It's okay to me either way. Thanks again for the response :)