Consider CLI option for cargo.toml path & pull settings out of cargo.toml

[mitranim]

This is based on the discussion in mitranim/sublime-rust-fmt#11. Users expect rustfmt to be run with respect to cargo.toml settings such as edition. Currently rustfmt supports them as CLI options. One option for plugins is to parse cargo.toml, pull out these options and pass them to rustfmt, example: rust-lang/rust-analyzer#2477. I feel like it's the wrong approach. Let's simply pass to rustfmt the location of cargo.toml, and have it pull out any settings it supports.

This way:

• Users don't need to wait for editor plugins to implement support for new options.
• Editor plugins don't need to include a TOML parser (huge blocker for mitranim/sublime-rust-fmt since I'd like to avoid dependencies if possible).
• rustfmt CLI options don't have to grow (maybe).

Speaking of giving rustfmt the config file location. Maybe we could have rustfmt automatically look for all the config files it supports, and simply tell it the innermost directory where to start the search? This way, editor plugins don't have to be updated in the future to support yet another config file. Thoughts?

[calebcartwright]

hey @mitranim - my thoughts below on a few specific points (many of which you noted in the linked thread).

Users expect rustfmt to be run with respect to cargo.toml settings such as edition

cargo fmt is the cargo-aware wrapper around rustfmt that detects various cargo metadata from the workspace, including edition, to provide as parameters to rustfmt.

IMHO, we should not attempt duplicate that behavior within rustfmt, especially since rustfmt can be used to format rust files without the presence of any Cargo.toml file, and even strings of rust code via stdin (which is indeed leveraged by some rustfmt users)

$ echo "fn main () {        }" | rustfmt --check
Diff in stdin at line 1:
-fn main () {        }
+fn main() {}

Currently rustfmt supports them as CLI options.

rustfmt allows for config options to specified via the cli, but it also supports configuration via the .rustfmt.toml configuration file. For users that are running rustfmt directly (whether via the terminal, editor, or anything else) and need to override any rustfmt's defaults, the guidance is that those users need provide those configuration values (edition or otherwise) via the config file and/or cli args.

One such thread where this has been discussed:
#3501 (comment)

(below quote cross posted comment from the author of the linked issue)

It is not common to use rustfmt.toml to specify edition

I'd disagree with this for the above reasons. If folks are running rustfmt directly with an edition override then they're either doing it via the config file or cli override.

I think the main driver is that the default edition in rustfmt 1.x is 2015, but we'll have the 2018 as the default in rustfmt 2.x which should help most cases (users will still be able to override that default if they really need to use the 2015 edition)

[LPGhatguy]

@calebcartwright
I think it's important to note that a lot of people don't use rustfmt directly. As you pointed out, cargo fmt is the cargo-aware wrapper and is what people use.

Editor plugins don't have a good way to use cargo fmt, as far as I can tell. Digging through the docs, I couldn't find a way to format a single file in the context of a Cargo project with cargo fmt. This makes me think that today, plugins must use rustfmt.

My editor plugin is the only place I run rustfmt directly. Everywhere else, I'm running cargo fmt, which doesn't require redundant configuration to run with the correct edition. If cargo fmt has an interface to format a single file, then that seems like the right solution.

Changing rustfmt's default edition to 2018 is a different issue. I feel that with the 2021 edition on the horizon, we'll run into this same issue again in a year! There are also still 2015 edition projects that I contribute to, and it seems useful for rustfmt to stay configured with the correct edition for those by default.

[calebcartwright]

hi @LPGhatguy 👋

I understand that it'd be convenient in some situations to have rustfmt auto-detect the edition, and why you'd prefer that solution for your use case of auto-running rustfmt in your editor.

However, there are cons with having the rustfmt cli and/or lib auto-detect the edition in the broader rustfmt context, including for the cargo-less use cases rustfmt has to support. IMO those cons outweigh that pro of convenience.

I know you'd prefer not to, but my suggestion for you is still to include a rustfmt.toml config file in your projects that need the 2018 edition. As I mentioned above, it's not uncommon for folks to do this and/or add the cli edition override when running directly. I've included a few examples for reference.

https://github.com/rust-lang/rust-clippy/blob/master/rustfmt.toml#L5
https://github.com/rust-lang/rust/blob/master/src/bootstrap/format.rs#L14
https://github.com/rust-random/rand/blob/master/rustfmt.toml#L22
https://github.com/libra/libra/blob/master/rustfmt.toml#L1
https://github.com/tokio-rs/tokio/blob/master/ci/azure-rustfmt.yml#L17

FWIW, there's a lot of similarities here with rustc. The default edition in rustc is also still 2015, so just like with rustfmt you'd have to include a --edition 2018 when running rustc directly, even if there's a Cargo.toml file that specifies 2018 for the edition.

Ultimately though, this is up to @topecongiro. If the decision is to include auto-detection of edition to the lib and/or cli, then I'm happy to help implement and support it. I just don't think that's the right solution, and I believe easiest and quickest solution for your use case is to include a rustfmt config file with the edition.

[LPGhatguy]

@calebcartwright
Oh! Sorry, I didn't mean to come off as advocating for making rustfmt parse Cargo.toml. I think there might be another solution to this problem.

If cargo fmt were made usable from editor plugins in a similar way to how rustfmt is today, I think we could sidestep a lot of work. As-is, projects either need to add rustfmt.toml, which is not the default in new Rust projects, or editor plugins need to find and parse Cargo.toml themselves.

[calebcartwright]

No worries!

If cargo fmt were made usable from editor plugins in a similar way to how rustfmt is today, I think we could sidestep a lot of work

I wouldn't recommend editors try to use cargo fmt for the format-on-save of a single file either today, and definitely wouldn't do so after we release rustfmt 2.x because we're inverting --skip-children and --recursive.

cargo fmt detects other inputs that it passes to rustfmt, including the entry point file for each target for each package within the workspace, so using cargo fmt in editor on-save scenarios would almost always result in formatting additional files.

@mitranim I know you don't want to deal directly with toml parsing in your extension, but if you'd like to support the use case, would something like running the cargo metadata --no-deps --format-version 1 (emits json formatting) command work for you? You'd be able to grab the edition from that output to use for the --edition arg to rustfmt, and wouldn't have to pull in any additional deps/parsers since json is part of the standard lib.

[mitranim]

Thanks for all the considerations.

On my system, in a small Cargo project, getting the edition via cargo metadata takes around 35ms, while simply parsing cargo.toml is under 1ms. For comparison, the rustfmt call is 50-70ms. This is done on every file save, and the added delay from metadata is perceptible.

I can see the value of keeping rustfmt Cargo-unaware, and deferring to cargo fmt. Currently it's not usable by plugins because it reads and writes files by itself. The Sublime Text plugin formats the buffer before save. For various reasons, formatting after save is a much worse user experience. If cargo fmt supported stdio, as long as it's not slower, it would be an interesting alternative.

[mitranim]

What do you think, how realistic is it to add stdio support to cargo fmt to make it usable by plugins?

[calebcartwright]

Ultimately the ask here, as I understand it, is to have the ability to format a single file with a non-default edition without having to tell rustfmt which non-default edition to use.

Is that correct?

[mitranim]

Apologies for the slow response, been using the weekend to unplug. The ask is a bit more general: have the Rust formatting tools (either cargo fmt or rustfmt) encapsulate the "knowledge" of Cargo configuration, and run with respect to it, without requiring the various editor plugins to "mediate" this knowledge between Cargo and rustfmt.

[calebcartwright]

Apologies for the slow response,

No worries @mitranim

without requiring the various editor plugins to "mediate" this knowledge between Cargo and rustfmt

Besides the Edition, what specific other data points from Cargo do you feel your editior plugin needs to pass to rustfmt?

Perhaps I'm forgetting something, but I think you may be overestimating the quantity of data points from Cargo that are used for rustfmt. cargo fmt's main purpose is to simplify the process of formatting an entire workspace by constructing the corresponding rustfmt command(s), and the vast majority of that process is determining the input file(s) to be passed to rustfmt.

There's really only a handful of core items that cargo fmt always passes as args/flag to rustfmt:

1. the input file(s) to be formatted
2. the corresponding edition value(s)
3. the --recursive flag (starting with rustfmt 2.0)

There's some minor transformation of optional args/flags folks expect to see with any cargo subcommand (like --message-format) into their corresponding rustfmt flag (--emit), but those aren't really relevant here.

no. 1 consists of all the entry point files for every target for every package within the workspace, and obviously no. 2 comes from the detected editions, and these are the only Cargo configuration data points that are detected and passed to rustfmt.

For example in the case of a simple lib project, running cargo fmt is identical to running rustfmt src/lib.rs --recursive --edition 2018, and then obviously the list input files arg would be much larger for multi target packages and/or multi package workspaces.

In the context of a "format modified file on save" capability within an editor plugin, you definitely wouldn't want to run rustfmt with the --recursive flag (no. 3) nor would you want to run rustfmt against the entry point files (no. 1) So IMO that really only leaves the edition (no. 2)

The ask is a bit more general: have the Rust formatting tools (either cargo fmt or rustfmt) encapsulate the "knowledge" of Cargo configuration, and run with respect to it

I'd still suggest what's needed is not general, but very specific to the edition for the reasons outlined above.

For a format-on-save feature, the only arg that should be passed to rustfmt is the file that was modified, and potentially, the --edition flag if there's a desire to have the feature work in the absence of a rustfmt.toml config file.

The only concrete ask I see is to have something in the rustfmt ecosystem detect that edition value so that editor plugins don't have to. Basically, "something" that results in --> rustfmt file-that-was-just-modified.rs --edition 2018

cargo fmt already covers the "general" case of formatting a workspace with respect to all the cargo information. rustfmt formats the specified input files with respect to the provided rustfmt config options. Neither support that use case of detecting only the edition.

how realistic is it to add stdio support to cargo fmt to make it usable by plugins?

I'm slightly less opposed to trying to add this within cargo fmt vs rustfmt, but I don't think cargo fmt is the right solution either. If this auto-edition detection capability were to be supported within the rustfmt ecosystem, I think it'd be better off as a separate binary (like git-rustfmt and rustfmt-format-diff) that's included in the rustfmt install.

IMO trying to support this particular use case directly within cargo fmt would require too many new flags/options that would introduce the possibility for far too many confusing and conflicting flag combinations (cargo fmt -p foo --some-new-option-to-parse-cargo-file bar/Cargo.toml -- --edition 2021). That would add way too much complexity to the code and I also believe that such an increased flag/arg surface would be a source of confusion that would degrade the cargo fmt user experience.

[mitranim]

Oh, I thought I responded, but didn't actually send it. 🤦‍♀ I apologize for taking so much of your time with this.

I can see where you're coming from. Between final users, plugins, rustfmt and cargo, nobody really wants to deal with additional settings. 🙂 I might indeed be overestimating how many settings might be added in the future, so I'll defer to you on this. Feel free to close the issue for now.

[calebcartwright]

Between final users, plugins, rustfmt and cargo, nobody really wants to deal with additional settings

It's not really a question about additional settings on the rustfmt side. It's just that something needs to provide a value for the edition setting to rustfmt, and rustfmt recommends users do that via the rustfmt config file. However, it seems it's common for editor plugins to provide a format-on-save feature to users even in the absence of users providing that edition value to rustfmt (config file, editor config/settings, etc.).

That's a bit of an edge case relative to the main rustfmt cases IMO, though I do agree that it'd be valuable if something in the rustfmt ecosystem supported that use case so individual editor plugins didn't have to each re-invent their own solution.

I just think that it'd be easier for that use case to be supported by something new vs. trying to cram it into rustfmt or cargo fmt. That way editor plugins could call something like:

rustfmt-on-save file-just-modified.rs

which would be capable of detecting the corresponding edition and translating into the corresponding rustfmt invocation:

rustfmt file-just-modified.rs --edition 2018

There's other binaries included with the rustfmt package (like git-rustfmt) so there's some precedence there.

Maybe something like that could be done in the long term, but for the foreseeable future users would be better off following the rustfmt recommendation to include the rustfmt config file.