rust-lang / rustfmt

Format Rust code
https://rust-lang.github.io/rustfmt/
Apache License 2.0
5.96k stars 877 forks source link

Feature request: Add the option to configure rustfmt in the `Cargo.toml` metadata tables #6296

Closed J-Westin closed 2 weeks ago

J-Westin commented 2 weeks ago

The Cargo.toml specification includes [project.metadata] and [workspace.metadata] tables, which allows external tools to store arbitrary data. These tables could be used to configure rustfmt, in addition to the existing .rustfmt.toml and rustfmt.toml files.

Configuration data for rustfmt, in the same format used in the existing .toml files, could be read from the [package.metadata.rustfmt] table. The following rustfmt.toml file:

max_width = 79

would be equivalent to the following Cargo.toml:

[package]
# ...

[dependencies]
# ...

[package.metadata.rustfmt]
max_width = 79
calebcartwright commented 2 weeks ago

Thanks for reaching out but I'm going to close.

This is something that's come up a few times before and has been discussed and considered, but ultimately rejected and that's still our position today.

Some of the more recent discussion is buried so deeply in threads that GitHub links don't work, but hopefully the one below still does because it has a relatively thorough summer of the issues and reasoning behind the decision (let me know if it's inaccessible though and will try to find a way to make that available):

https://github.com/rust-lang/rfcs/pull/3421#discussion_r1175723730

J-Westin commented 2 weeks ago

@calebcartwright Thank you for your comment. I had a look at the thread(s) you linked to, but I wasn't able to find any clear arguments for why this feature was "ultimately rejected". The only reasons I could find were that Cargo.toml would have to be parsed on every invocation, and that it might create feature inconsistency between official tools. I don't see either of these as particularly compelling arguments for blocking its implementation.

As you mentioned, I'm probably missing some discussion buried in the GH comment system. Would you mind giving a short summary of why this feature is ultimately not being considered?

calebcartwright commented 2 weeks ago

Did you read the entirety of the comment I posted, including the collapsed sections? I ask only so I know what needs clarity or summarization but essentially:

Low level tools (rustc, rustfmt, etc.) aren't cargo-aware and that's by design. In the context of cargo-based configuration that doesn't have much impact in cases where low level tools are rarely run directly, but in the case of formatting both the lower level rustfmt is run regularly (by code editors and some humans) and cargo fmt is also run regularly, e.g. in CI or by humans.

Everything would be fine if configs lived in Cargo.toml and formatting were only ever (or at least primarily) invoked via cargo fmt, but that's not the case, and in order to prevent rustfmt and cargo fmt from contradicting each other, you either have to break the intentional separation of concerns and start having lower level tools become cargo-aware (which does not solely include the parsing of Cargo.toml files, but also requires having to discover the in-scope Cargo.toml file which requires interrogating Cargo workspace data), or you have to somehow change the paradigm so that the common invocation of the lower level tool goes away

Additionally, the former would result in a direct violation of rustfmt's stability guarantee with the passage of RFC 3338

calebcartwright commented 2 weeks ago

There's also the issue of precedence and the current feature/availability to have differing rustfmt configurations for individual directories at a more granular level via local .rustfmt.toml configuration files.

cargo fmt is just a cargo-aware invocation of rustfmt commands, and rustfmt allows configuration options to be specified on the command line via --config ... which takes the top precedence.

However, if cargo fmt was to just pick up rustfmt config settings from the Cargo.toml files it detects, cargo fmt wouldn't be able to pass those on to rustfmt with the existing --config ... option because that would break existing behavior and a feature that's actively used.

Functionally the only way I could envision that working even in the cargo fmt invocation scenario would be for rustfmt to have a new option to the effect of --lowest-precedence-config ... which would have to take a backseat to everything, and that just makes things rather clunky and messy on rustfmt itself

J-Westin commented 2 weeks ago

I only skimmed through the thread you linked to get an idea of what the opposing arguments were. I appreciate the points you provided here.

From the description of the [project/workspace.metadata] tables in the Cargo docs, the data stored there is explicitly ignored by Cargo itself, serving essentially as a way to "piggy-back" off the Cargo.toml file. Its intended use appears similar to the pyproject.toml:[tool] table used by many Python tools. Since Cargo ignores the metadata tables, I don't see any reason why cargo fmt and rustfmt should prioritize them differently. A tool that uses a metadata table could be interpreted as not being "cargo-aware", since it's effectively just a data table that happens to be in a file called Cargo.toml.

Either way, I'm sure people have made the same arguments as I have. I also appreciate that stability guarantees put a high burden of care on the addition of these types of features.