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.

[nickbp]

Had this problem with enabling the format-on-save feature in emacs/rustic-mode. Ended up just configuring rustic-mode to always specify --edition=2021, since going back to every project and workstation to write a dedicated rustfmt.toml isn't really feasible.

Thinking about the problem, I don't understand why Cargo.toml can't be checked for this value. That's the standard location for specifying the edition in any Rust project, and it's unreasonable for rustfmt to stand alone in ignoring it. Even coming at it from the angle of breaking out formatting-specific configuration into a separate rustfmt.toml, it still doesn't make sense to require that the edition to be specified there, since edition isn't a formatting setting to begin with. To me it looks like it was only tossed into rustfmt.toml as a hack because the Rust compiler started requiring it, and that it's just a quirk of history that rustfmt didn't just read it from the correct location to begin with.

The current awkwardness of having to specify the edition in several unrelated places really reveals itself when noticing that even the rustfmt repo itself specifies an edition (2018) in Cargo.toml, but not in rustfmt.toml. So it's clear that the current situation isn't really practical.

I think reasonable handling would be to check for a project file named Cargo.toml as a part of the same directory scan that's already occurring for rustfmt.toml, and to honor the edition specified in Cargo.toml if rustfmt.toml doesn't exist or doesn't specify it. Later on the edition setting could be deprecated and from rustfmt.toml entirely, since it again doesn't really have anything to do with tweaking the code formatting.

[calebcartwright]

Thanks to everyone for sharing your perspective! However, I'm going to close this issue because it's gotten too lengthy and hyperfocused on a single, non-viable solution to a problem such that it's no longer a great fit for discussion of the problem itself.

The root problem is that editor/ide plugins don't have a manner in which to format a single file in a cargo-aware manner. This is exacerbated by the fact that rustfmt, like other tooling, has to maintain the same default edition as rustc (not our preference but not our choice either), and developers wanting to avoid having to add a rustfmt config file.

Editor/IDE plugins would run into this exact same issue if they needed to run other tools directly, e.g. rustdoc vs cargo doc, or rustc vs cargo build/check/etc., so it's quite inaccurate to claim that rustfmt is "standing alone" on this. The only difference is that those plugins are running rustfmt directly because of that root problem whereas they are using the cargo-aware tooling for other activities.

We're simply not going to address the problem with the proposed solution because it's not the right approach. The separation between the cargo-aware tooling/wrappers and direct/non-cargo-aware tooling is intentional and we're not going to break that.

We do, however, already have a better solution to the problem that's already in progress and largely complete from a technical perspective. It's just pending on process due to the type of change it may introduce.

I will post a note here once we've gotten that in place for broader awareness.

[nickbp]

Your point about keeping rustfmt as a low-level tool parallel to rustc sounds reasonable to me. It does feel like this ultimately stems from there being no current solution to the common use case of "I want to press a button that auto-formats this file that I'm editing" as one might find as a first-class feature in other formatting tools like black or gofmt. Ultimately I'm happy as long as there's a user-facing Rust command included in the standard tooling that I can use to format a file in a standard project.

As an aside once such a command exists I think it'd make sense to deprecate the edition field from rustfmt.toml and only be a commandline argument, since it duplicates what's already handled in Cargo.toml and easily falls out of sync (as is currently the case with the rustfmt repo itself!). Similarly, a .rustfmt.toml specifying an edition in one's userdir would just cause problems for repos that don't use that edition.

After my above comment, I'd already put together a quick self-contained update to src/config/mod.rs that has it automatically find the nearest Cargo.toml alongside the existing scan for rustfmt.toml. With this example patch, rustfmt will only fall back to 2015 if no edition is specified by either any rustfmt.toml, nor a Cargo.toml in the directory tree. I think this is effectively the behavior that any newcomer running rustfmt to format a file would expect to happen.

Thinking about it overnight, another better angle could be to support formatting individual files in cargo fmt, rather than only the entire project like it does today. This would retain the structure of rustfmt being equivalent to rustc in terms of being a command that end-users wouldn't generally interact with. Instead day-to-day formatting for both entire projects and individual files by the end user could be via cargo fmt.

I think in an ideal world the syntax would look like this, where the files would be provided via positional arguments before any --:

Old:
$ cargo fmt [FLAGS] [OPTIONS] [-- <rustfmt_options>]
New:
$ cargo fmt [FLAGS] [OPTIONS] [<file.rs> [<file2.rs> ...]] [-- <rustfmt_options>]

Here's some example usage, which notably are all invalid commands in today's cargo fmt, so there wouldn't be much of an issue in terms of backward compatibility:

$ cargo fmt file.rs otherfile.rs # << woohoo!!!
$ cargo fmt file.rs
$ cargo fmt --check file.rs
$ cargo fmt --check file.rs -- --edition 2015

You'd mentioned that there's already a plan in the works elsewhere for supporting formatting individual files, what is the venue for that discussion? As may be apparent I've got opinions... In the meantime I'll see about putting together another example demonstrating individual file support via cargo fmt.

[ytmimi]

@nickbp thanks for sharing your ideas on this. For your reference @calebcartwright and I are working to support single file formatting in cargo fmt (#5071). As @calebcartwright mentioned:

We do, however, already have a better solution to the problem that's already in progress and largely complete from a technical perspective. It's just pending on process due to the type of change it may introduce.

So to reiterate we've got most of the implementation details down, but there are still a few things that we need to think through before it's 100% ready (mostly around the interaction of certain conflicting command line options). Feel free to take a look at the thread and leave some feedback there!

Although the PR was opened some time ago I've been prioritizing some other rustfmt initiatives and plan to come back to it when those have wrapped up!

[nickbp]

OK that's great! Thanks for the info, and for already putting so much effort into the fix. I'll follow along in the PR.

I'll leave a comment about the above cargo fmt positional filename arguments idea but I wouldn't be surprised if there was a problem that I was missing with that structure...