rustfmt: cargo fmt --all is slow in a workspace project

@calebcartwright Gentle bump on this — have you had a time to think more about this vs #4722?

@jonhoo - stretched rather thin and having to juggle 20 different things. This is on the top of my list for whenever I get a chance to actually work on some rustfmt development, but right now my limited rustfmt bandwidth is preoccupied with other general support and maintenance activities.

I also want to stress that there is no either/or here. We’re definitely going to make this change, and there’s a good chance we’ll probably go forward with #4722 as well.

However, really important to note that #4722 would only be at play with explicit workspaces, but if you have an explicit workspace you do not need to use --all anyway (unless you just really want to format your entire workspace by executing the command from within one of your subdirectories). If you’ve not set up an explicit workspace, but instead have just structured your project with relative path depdencies between your packages (e.g. like Clippy or RLS), then you do need --all and #4722 has 0 impact on that use case.

I think the main issue here is the disconnect between what the --all flag means and when it’s actually needed with cargo fmt vs. what folks, understandbly but often incorrectly, assume based on what --all meant previously with other cargo subcommands. (Refs https://github.com/rust-lang/rustfmt/issues/3911)

Background

There’s only a few use cases I recall where the --all flag with cargo fmt is currently helpful/meaningful:

1. Running cargo fmt in a subdirectory of an explicit workspace where you want to run formatting commands against the entire workspace/all the explicit workspace members (all members are formatted by default when running from the root of an explicit workspace)
2. Running cargo fmt in the root directory of an implicit workspace where you want to run formatting commands against all implicit workspace members
3. Running cargo fmt against a given package which has one more local/path based dependencies where you want to run the formatting commands against the current package and that local/path based package (as well as any local/path based deps that package may have, recursively).

I suspect that the majority of instances where folks are using the --all flag with cargo fmt falls into those first two buckets, while the third is relatively more of an edge case.

cargo fmt currently leverages the cargo metadata command in order to discover the workspace information which is then used to construct the corresponding rustfmt commands; cargo fmt does not directly parse Cargo.toml files.

Accordingly, in order to support the second (and third) use case, cargo fmt --all runs cargo metadata without the --no-deps flag (due to https://github.com/rust-lang/cargo/issues/7483) to ensure that the cargo metadata output contains the information that cargo fmt needs in order to properly locate and format those implicit workspace members/path-based dependencies.

AIUI, running cargo metadata without the --no-deps flag results in the registry index hit that is presumably the cause of the increased run time folks are seeing which is probably rather apparent in CI-type environments.

Next Steps

We’ve tried updating the help text for the --all flag though that certainly hasn’t solved the disconnect, and I believe we probably need to take additional action to try to ameliorate the confusion.

Perhaps some non-mutually exlusive actions we could consider:

• Create/augment cargo fmt documentation to try to provide better guidance on when --all is required vs. when it should likely be avoided.
• Find a way to support formatting implicit workspaces without the index implications and associated perf hit
  • Stick with cargo metadata and work on addressing https://github.com/rust-lang/cargo/issues/7483
  • Find an alternative to cargo metadata to get workspace information (parse Cargo.toml files directly, etc.)
• Change behavior of existing cargo fmt flags and/or add new flags (this would almost certainly result in a breaking change that’d require a major version bump). For example, we could add a --workspace or even --explicit-workspace) flag that would support the first use case (explicit workspaces) and use --all, or even rename/replace it with --implicit-workspace to support the second and third use case

The resolution for this will be in the next release. Purely anecdotal data points on my slow machine so treat them as such, but suffice to say that folks using --all should notice some marked performance improvements. I’m observing execution time going from multiple minutes down to 1-2 seconds in an uncached/clean environment, and from 30 seconds down to that same 1-2 seconds in a cached/dev laptop type env.

Available on nightly as of 2021-10-24 (perhaps the day before), slated to land on stable as part of v1.58

Thanks @jonhoo, planning on looking at it this week

so we can maybe even fix this on the next beta (which I think is Dec 31st)

Won’t commit to any target timeframe just yet, especially given the holidays, but yes will try to get this out soon once possible to proceed. Unfortunately due the various non-rustup channels through which rustfmt is distributed and can be used there will be a bit of extra complexity to handle working with older versions of cargo that may not the newer changes.

I’m also not sure if the cargo_metadata crate will need to first be updated as well before we can start taking advantage of https://github.com/rust-lang/cargo/pull/8994