Allow cargo-fmt to handle formatting individual files.

[ytmimi]

Summary

This PR adds a long requested feature to cargo-fmt. Users will now be able to specify individual files inside their package to format. The motivating use case for this change is to make it easier for maintainers and developers of editor/ide plugins to implement format file on save behavior. The command looks like this:

# using the short name
cargo fmt -s src/main.rs

# using the long name
cargo fmt --src-file src/main.rs

Not addressed by this PR:

• Formatting any arbitrary rust file using cargo fmt
• Formatting all files within a directory using cargo fmt.

Things To Do Before PR Is Officially Ready

• write tests for the new feature
  • command line option parsing tests
  • specifying and formatting a single file
  • specifying and formatting multiple files
  • tests that enforce you can't use -s with -p and --all
  • formatting files within a cargo workspace
• figure out if these changes apply to cargo workspaces.
  • through manual testing I was able to confirm that these changes allow you to format an arbitrary rust file within a workspace's package.
• Update docs to mention the new feature
  • probably want to give examples and mention which other option's the new -s option is incompatible with.

[calebcartwright]

Excellent work getting this pulled together so quickly! I want to first summarize the primary goal/use case of this new feature given that it's been primarily discussed offline so far.

rustfmt doesn't particularly need, nor does it make use of, Edition values itself. However, rustfmt uses the internal parser and AST modules from the compiler, which do care about the Edition, so rustfmt accepts Edition as an input so that it can pass it along to the compiler. Much like rustc, rustfmt has no awareness of/dependency on Cargo.toml files where Edition values are typically defined, and rustfmt also assumes an Edition value of 2015 if no explicit Edition is provided, just like rustc.

The decoupling of cargo aspects from rustfmt is intentional and desirable, both from an overall design standpoint and so that rustfmt can be easily used for input strings, single *.rs files, etc. Users can pass their desired edition value to rustfmt via the rustfmt config file or the --edition arg.

cargo fmt supports formatting entire projects/crates in a cargo-aware manner, which includes the automatic detection and utilization of Edition values defined in the respective Cargo.toml files.

This gives a lot of options to users that are running rustfmt, since they can either cargo fmt to format everything, or manually run rustfmt for more selective/controlled formatting.

However, one scenario that is a bit tricky today is the various plugins and extensions for code editors and IDEs which support the increasingly common "format on save" behavior, where they will automatically invoke a formatter when users save changes to files in the IDE/editor (the formatter being rustfmt in the case of .rs files). These editor plugins are in a tricky spot, because they only want to format a single file in these cases (the file that was modified), so they can't just run cargo fmt which format everything, but they can't easily run rustfmt directly because those plugins don't know every relevant arg, specifically the Edition, should be.

That is the specific use case we want to solve for here: we want to provide a built in way to run an Edition-aware rustfmt invocation against a single file with a target audience of editor/IDE plugins so that they no longer have to either reinvent that wheel or require all of their users add a rustfmt.toml file to their projects.

Human users can and should still continue to use the existing invocations (i.e. cargo fmt or rustfmt foo.rs --edition 2018)

[calebcartwright]

Given the above, some things to consider around the implementation:

• the input to the cargo fmt arg should be constrained to a single file; we shouldn't accept an arbitrary number of inputs for each of which we'd have to run metadata resolution commands
• how do we want to resolve a relative path input? I presume relative to the current working directory, but what are we doing/should we do if the --manifest-path argument is also provided? Would the input be resolved relative to that path or still the cwd?
• for the non-target audience of a human developer invoking something odd/conflicting with direct args to rustfmt, e.g. cargo fmt -s foo.rs -- bar.rs --edition 2021 --config hard_tabs=true, how are we handling this (I.e. what would the rustfmt invocation(s) be, a single rustfmt foo.rs --edition 2021 --config hard_tabs=true)?

[ytmimi]

• the input to the cargo fmt arg should be constrained to a single file; we shouldn't accept an arbitrary number of inputs for each of which we'd have to run metadata resolution commands

Got it. I'll update it to no longer accept an arbitrary number of files, and restrict it to a single file.

• how do we want to resolve a relative path input? I presume relative to the current working directory, but what are we doing/should we do if the --manifest-path argument is also provided? Would the input be resolved relative to that path or still the cwd?

I personally feel like we should always resolve paths based on the on the current working directory. It does feel a little weird to change the path resolution based on the manifest path.

• for the non-target audience of a human developer invoking something odd/conflicting with direct args to rustfmt, e.g. cargo fmt -s foo.rs -- bar.rs --edition 2021 --config hard_tabs=true, how are we handling this (I.e. what would the rustfmt invocation(s) be, a single rustfmt foo.rs --edition 2021 --config hard_tabs=true)?

I think in a scenario like the one you just posed, where a user invokes cago fmt but still tries to pass a file path to rustfmt directly should result in an error.

As cargo-fmt is currently implemented you can do something like this: cargo fmt -v --check --all -- src\macros.rs -v

It might make sense for us to add the following check to make sure users can never pass a file path to rustfmt through cargo-fmt.

if opts.rustfmt_options.iter().any(|s|s.ends_with(".rs")) {
    print_usage_to_stderr("cannot pass rust files to rustfmt through cargo-fmt");
    return FAILURE;
}

Are there any other arguments we shouldn't let users pass along? I think it probably makes sense to write some tests to cover cases where users are passing values along to rustfmt. There don't seem to be too many integration tests that currently cover these cases.

[ytmimi]

Also, when it comes to the edition, if a user does pass --edition to rustfmt, we want to honor that right? As it currently stands if you run this cargo run --bin cargo-fmt -- -- --edition=2015 This is what you'll get:

Option 'edition' given more than once
error: process didn't exit successfully: `target\debug\cargo-fmt.exe -- -v --edition=2015` (exit code: 1)

If we pass -v to cargo-fmt we see what's happening: rustfmt --edition 2021 -v --edition=2015 (...files) is what's getting invoked.

I bring this up because it seems like there's currently issues with some arguments that cargo-fmt passes to rustfmt.

I'd also expect to see the edition I passed in from the command line displayed in the verbose output, but the edition from the cargo.toml file is output (running the same command as above):

[bin (2021)] path/to/rustfmt/src/bin/main.rs
...

and what I expect to be output

[bin (2015)] path/to/rustfmt/src/bin/main.rs
...

Does it make sense to include this check to disallow passing the --edition to rustfmt from cargo-fmt?

if opts.rustfmt_options.iter().any(|s| s.contains("--edition")) {
    print_usage_to_stderr("cannot pass '--edition' to rustfmt through cargo-fmt");
    return FAILURE;
}

If we want to allow users to be able to provide an edition then this seems like a separate issue that I can open / work on a PR to fix.

[calebcartwright]

It does feel a little weird to change the path resolution based on the manifest path.

Fair enough, just thinking about scenarios where someone has to do something like cargo fmt --manifest-path foo/bar/baz/Cargo.toml -s foo/bar/baz/src/main.rs

It might make sense for us to add the following check to make sure users can never pass a file path to rustfmt through cargo-fmt.

That's an interesting idea! IIRC that's technically permitted today, though I guess cargo fmt ends up tacking that along with each rustfmt invocation, meaning that file will get formatted duplicatively for every target in the workspace.

Does it make sense to include this check to disallow passing the --edition to rustfmt from cargo-fmt?

Good observation, thanks for sharing. I vaguely remember thinking about this when I did some work on the emitters and IIRC I added a bit of logic into cargo fmt to try to prevent it from tacking on duplicative/conflicting arguments with those passed directly to rustfmt.

I want to think on this a bit more, but I am currently leaning towards trying to limit rustfmt args on cargo fmt in cases of direct or potential conflicts

[ytmimi]

I want to think on this a bit more, but I am currently leaning towards trying to limit rustfmt args on cargo fmt in cases of direct or potential conflicts

Definitely take some time to think on it! I'm also leaning towards limiting the arguments that can be passed along to rustfmt from cargo fmt since it probably simplifies the implementation and it's probably better for us to report back to users when they've passed conflicting arguments.

Once I know how you'd like to move forward I'll get back to working on the PR

[calebcartwright]

since it probably simplifies the implementation and it's probably better for us to report back to users when they've passed conflicting arguments

That's a good point, you've sold me 😄 I'd like to ensure that the two buckets of changes (new -s option for editor/IDE plugins, and the improved option filtering) get released together, but will defer to you on whether you think it'll be easier to do both in one PR or split them

[ytmimi]

I'd like to ensure that the two buckets of changes (new -s option for editor/IDE plugins, and the improved option filtering) get released together, but will defer to you on whether you think it'll be easier to do both in one PR

I think It's definitely doable to get them added at the same time but I'll let you know if I run into any issues. I've already mentioned a few inconsistent arguments, and I'll be sure to add checks for them. If you can think of any other arguments that we shouldn't allow cargo-fmt to pass along to rustfmt please let me know. I'll also try to do some brainstorming as I continue to work on this PR.

[ytmimi]

Check-In

Just want to give a follow up on the progress so far:

1. We now only accept a single file
2. cargo-fmt exits early if we try to:

• use package related arguments with the --src-file option. Specifically -p and --all.
• we try to pass along any rust file to rustfmt through cargo-fmt
• we try to pass the --edition option to rustfmt through cargo-fmt

3. we've got test cases that cover the new option. I still probably want to add a few more just to be sure that everything is good.

Things I'm still working through

Currently the target resolution for source files (implemented in get_target_from_src_file) compares the path of the source file to the parent path of the target. Basically trying to figure out which target path is the closest parent or sibling in the directory structure.

I was wondering if you thought there might be a better way to handle the target resolution or if you think it's appropriate to have a fallback for the kind and edition in the event that we can't find a suitable target?

Currently this works just fine: cargo run --bin cargo-fmt -- -v --check -s src\test\mod_resolver.rs, but this fails because we can't find a target: cargo run --bin cargo-fmt -- -v --check -s tests\target\trailing_commas.rs

Since the main use case for this feature is to better help IDE/editors implement format on save I think it makes sense to have a default for kind (or at least one way to dynamically determine one, e.g if the path contains "test" we assume the kind is test) and use the default edition of 2015 if we can't determine what the appropriate one should be. rustfmt doesn't actually care what the kind of the target is right?

[ytmimi]

Thinking through conflicting arguments:

• This seems odd, but it's harmless: cargo run --bin cargo-fmt -- --version -- --version
• It looks like --message-format and --emit conflict, but we're already handling that. I don't think there are tests, so I'll make sure to add some.

I might be jumping the gun here, but I think that with the few checks I've added we pretty much handle all the conflicting arguments that could be passed to rustfmt. As always, your input is much appreciated to let me know if I've overlooked something.

[calebcartwright]

Have done a very quick pass through this, and some initial reactions:

• we need to ensure that cargo metadata is invoked from the location of the provided source file (it wasn't clear to me that's happening)
• Given the above point, does it even make sense to accept the combination of --manifest-path and --source-file? Those could obviously diverge significantly
• I think the mechanism of edition discovery you've added is a pretty reasonable estimate (the association/derivation of an arbitrary input file with an edition is the hard problem of all this) though I think it'll be important to note in our docs that it is a best effort guestimate. I think the only way we could do it with any level of precision would require some far deeper inspection post AST loading and module resolution (that would in turn probably require a more advanced rustfmt input configuration that cargo fmt could leverage for this particular strategy)

[ytmimi]

• we need to ensure that cargo metadata is invoked from the location of the provided source file (it wasn't clear to me that's happening)
• Given the above point, does it even make sense to accept the combination of --manifest-path and --source-file? Those could obviously diverge significantly

All of the existing get_targets* function (get_targets_root_only, get_targets_recursive, get_targets_with_hitlist) start by calling get_cargo_metadata. I thought that would be enough to get the correct cargo metadata since it works for the existing functions.

Given the current implementation, In a scenario where a user passes both the --manifest-path and --src-file, and the manifest doesn't define a relevant target, the worst that can happen is that cargo-fmt will exit early with the error message "Failed to find targets". Maybe not the clearest error message, but it does indicate that something went wrong.

To ensure that we're using the right cargo metadata it probably makes sense for us to auto discover the cargo metadata. I'll go ahead and add that check. In general, I think it's better to be conservative with what we allow and we can always remove some of these checks in the future if it makes sense to do so.

• I think the mechanism of edition discovery you've added is a pretty reasonable estimate (the association/derivation of an arbitrary input file with an edition is the hard problem of all this) though I think it'll be important to note in our docs that it is a best effort guestimate. I think the only way we could do it with any level of precision would require some far deeper inspection post AST loading and module resolution (that would in turn probably require a more advanced rustfmt input configuration that cargo fmt could leverage for this particular strategy)

I'm glad that you're on board with the current best effort solution that I put together. In the future we can definitely come back and revise it, but I think it's a good starter solution.

[calebcartwright]

All of the existing get_targets* function (get_targets_root_only, get_targets_recursive, get_targets_with_hitlist) start by calling get_cargo_metadata. I thought that would be enough to get the correct cargo metadata since it works for the existing functions

The difference is that all of the other use cases are targeted at a human audience where invoking the metadata command from the current working directory is desirable/ideal. This use case is targeted at they very niche programmatic invocation from IDE editors/plugins, and I don't think we can (or at least should) assume those will set the cwd to the modified file for their invocation of cargo fmt -s .... Additionally, since the option will accept an arbitrary path, we need to make sure that we're checking the metadata from that provided path because while there's not any realistic utility in someone running cargo fmt -s ../../some-other-project-with-other-metatada/src/foo.rs from a different project/workspace (let's call it bar as the cwd), it would still be technically incorrect to query the metadata of the cwd.

If it helps, the big difference is that the existing discovery modes are "format my current context without me having to provide specifics" whereas this new feature is almost the inverse paradigm of "format this precise thing I've specified"

[ytmimi]

I see. I wasn't thinking about it that way. In that case it definitely makes more sense to prevent users from passing the --manifest-path. Is it okay to just pass None to get_cargo_metadata or should we add something like this to try and more directly derive the manifest from the given file?

fn get_cargo_metadata_from_source_file(path: PathBuf) -> Option<cargo_metadata::Metadata> {
    for p in path.ancestors() {
        let manifest = p.join("Cargo.toml");
        if manifest.exists() {
            return get_cargo_metadata(Some(manifest)).ok()
        }
    }
    None
}

[nickbp]

(EDIT: To be clear I doubt this is a reasonable option, but figured it made sense to mention in case it was useful somehow...)

Hello, I've read through the discussion and I was curious if it would make sense for the filename(s) to be presented as positional arguments like this, where filenames would go before the --?:

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

I think there are two upsides to having the files be positional arguments:

• In my thinking, this results in cargo fmt [file...] becoming a single spot for basic project and file formatting by end users, but this is very subjective
• Slightly less typing

Meanwhile some downsides I can think of would be:

• It arguably doesn't line up with the existing -p/--package argument, which does a similar thing but for packages.
• There would likely be issues trying to format a filename that starts with a -, but I've never seen that.
• This will take up the "slot" for positional arguments before --, removing the option for something else to use it in the future.

In any case, here are some examples of how this would look. These are invalid commands if you tried them in cargo fmt today, so I wouldn't expect for there to be compatibility issues:

# format one file
$ cargo fmt file.rs

# format two files
$ cargo fmt file.rs otherfile.rs

# format one file with cargo options
$ cargo fmt --check file.rs

# format one file with cargo options and rustfmt options
$ cargo fmt --check file.rs -- --color never

# format two files with cargo options and rustfmt options
$ cargo fmt --check file.rs otherfile.rs -- --color never

To be clear I don't have a strong opinion either way but figured it was worth mentioning in case it was useful. The fact that there's already a -p for specifying packages does make me think that using -s for specifying files would ultimately fit better with the current structure overall.

[nickbp]

For the --metadata-path discussion, it looks like MetadataCommand has a current_dir(&Path) option, which looks like it would allow deferring the ancestor scan for Cargo.toml to the underlying cargo metadata command.

To take advantage of this, get_cargo_metadata() could get an additional parameter for specifying a cwd:

fn get_cargo_metadata(manifest_path: Option<&Path>, cwd: Option<&Path>) -> Result<cargo_metadata::Metadata, io::Error> {
    let mut cmd = cargo_metadata::MetadataCommand::new();
    cmd.no_deps();
    if let Some(manifest_path) = manifest_path {
        cmd.manifest_path(manifest_path);
    }
    // NEW:
    if let Some(cwd) = cwd {
        cmd.current_dir(cwd);
    }
    cmd.other_options(vec![String::from("--offline")]);

    match cmd.exec() {
        Ok(metadata) => Ok(metadata),
        Err(_) => {
            cmd.other_options(vec![]);
            match cmd.exec() {
                Ok(metadata) => Ok(metadata),
                Err(error) => Err(io::Error::new(io::ErrorKind::Other, error.to_string())),
            }
        }
    }
}

Then the existing calls to get_cargo_metadata() in main.rs would use set the new cwd parameter of None to keep their existing behavior, while the new call in get_target_from_src_file() could specify cwd=src_file.parent(). This would make it as if cargo metadata was being called from the same directory as the file being formatted, which I think would result in the desired behavior in terms of finding the right package config for a given target file?

[ytmimi]

@nickbp Thanks so much for taking the time to looking into the MetadataCommand. Your proposal definitely seems like a worthwhile approach to consider for the get_target_from_src_file() code path, so I'll see if including your suggestion works!

To address your first comment, I don't think we want to allow users to be able to call cargo fmt like:

cargo fmt [file...]

As @calebcartwright mentioned in his first comment on this post:

cargo fmt supports formatting entire projects/crates in a cargo-aware manner, which includes the automatic detection and utilization of Edition values defined in the respective Cargo.toml files.

This gives a lot of options to users that are running rustfmt, since they can either cargo fmt to format everything, or manually run rustfmt for more selective/controlled formatting.

Since cargo fmt is primarily meant to be used to format entire projects in a cargo aware manner I personally don't think adding file arguments are in line with that use case. As mentioned, if users want fine grained control over individual file formatting they should use rustfmt directly.

Since the primary goal of this feature is to aide editor/plugin authors in implementing "format on save" in a cargo-aware manner I also don't think it makes sense to allow you to pass more than one file. My initial implementation for the feature did allow you to pass multiple files, but that idea was already rejected, so just want us to avoid bringing that back up.

In any case, here are some examples of how this would look. These are invalid commands if you tried them in cargo fmt today, so I wouldn't expect for there to be compatibility issues:

Again, really appreciate your input here, and just want to share some of my thoughts/opinions. Although the examples you pointed out might not work, there are still currently ways to pass files to rustfmt through cargo fmt by passing them after the --. For example:

cargo fmt -- file.rs

We had a little discussion about this above and I think this hack of passing individual files directly to rustfmt is inconsistent with cargo fmt's goal of formatting entire projects (not to mention inefficient because the entire project gets formatted an then the specified file gets formatted again). I think one of the major goals of this PR is to help remove some of this inconsistency and prevent some combinations of conflicting arguments.

All that being said, I'm personally leaning towards the --src-file option, but am totally open to moving in a differnet direction.

[nickbp]

To be clear I think the -s/--src-file method is great as-is. If anything, if this was later revisited and cargo fmt [file...] later became valid, there would probably still be a need to have an explicit -s option anyway. For example, if a filename started with a - for some reason, then the availability of an explicit -s -filename.rs would make it easier to handle that case.

In any case I don't want to derail the feature and I think it's great to go ahead with -s for the reasons you said. I don't personally have a stake in this outside of wanting to have an automated format current file shortcut for my own use so any arg format is great by me.

[ytmimi]

To be clear I think the -s/--src-file method is great as-is. If anything, if this was later revisited and cargo fmt [file...] later became valid, there would probably still be a need to have an explicit -s option anyway. For example, if a filename started with a - for some reason, then the availability of an explicit -s -filename.rs would make it easier to handle that case.

Awesome! I think we're in agreement then! I also don't want to rule out us adding the ability to pass files as positional arguments in the future but it's not something I want to work to support with this PR. If you happen to take a look through the implementation and have any ideas for ways that things could be improved please let me know!

[calebcartwright]

Confess I've not read through all the subsequent dialog word for word, but gather from latest comments that everything is caught up.

There's two outstanding items I'd like to see before moving ahead with releasing this feature, neither necessarily strictly related to this PR.

• We need to invert rustfmt's behavior to truly support single-file formatting by default (vs. current state of specified file(s) + imported out of line mods) in a manner that's also available on stable. This is largely a process item where I need to get the blessing to do so from dev tools
• I want to ensure we can also support RA's model, as well as any other editor plugin that's working with the string-based approach using the in mem buffer of the modified file contents (Allow to query whether a path is ignored/When using rustfmt via stdin allow passing a path #5137). I don't know if RA already has the edition info in context at that point, so potentially some research here

[ytmimi]

Got it! so I imagine we'd need to stabilize --skip-children (ref #5024), so that cargo fmt could pass that along when formatting with the --src-file option.

I'll also set aside some time to do some research into how editor plugins are invoking rustfmt to make sure that we make sure to support that through cargo fmt.

[calebcartwright]

Got it! so I imagine we'd need to stabilize --skip-children (ref #5024), so that cargo fmt could pass that along when formatting with the --src-file option.

In spirit yes, though more precisely the opposite. We've wanted to invert the behavior to be non-recursive by default (with a recursive opt-in) for a while. It's an easy technical change, and actually already landed on a different branch, but it's more of a process blocker/question.

I'll also set aside some time to do some research into how editor plugins are invoking rustfmt to make sure that we make sure to support that through cargo fmt.

Sounds good, though to be clear that's not strictly a blocker for this PR. It's just that it's in the same vein of the editor experience as this one and worth looking at especially while this is blocked.

Also I don't think we necessarily need to review all the various editor plugins either, a handful (like RLS) actually use the library API, but most of them just run rustfmt either by invoking the binary against the file, or as in the case of RA against a string that they then emit back out. It's really that latter case that we want to try to support relative to the ignore configuration option, as I believe everything else is already sorted (other than the edition piece which is addressed here of course)

[ytmimi]

Just opened #5266, which should hopefully address the ignore issue more broadly when passing values via stdin. I wrote up that PR after doing some digging into how rust-analyzer calls rustfmt (including a link to the code):

fn run_rustfmt(
    snap: &GlobalStateSnapshot,
    text_document: TextDocumentIdentifier,
    range: Option<lsp_types::Range>,
) -> Result<Option<Vec<lsp_types::TextEdit>>> {
    let file_id = from_proto::file_id(snap, &text_document.uri)?;
    let file = snap.analysis.file_text(file_id)?;
    let crate_ids = snap.analysis.crate_for(file_id)?;
    
    let line_index = snap.file_line_index(file_id)?;

    let mut rustfmt = match snap.config.rustfmt() {
        RustfmtConfig::Rustfmt { extra_args, enable_range_formatting } => {
            /* A FEW LINES OF CODE WERE REMOVED FORE DEMONSTRATION */

            if let Some(&crate_id) = crate_ids.first() {
                // Assume all crates are in the same edition
                let edition = snap.analysis.crate_edition(crate_id)?;
                cmd.arg("--edition");
                cmd.arg(edition.to_string());
            }
    ...

I want to highlight these lines because they show hat rust-analizer has everything it needs in context at this point to utilize the newly proposed option in #5266.

1. The path to the file can be fetched using the text_document.uri.to_file_path() method, and could easily be passed as a hint to rustfmt.

2. In the case of rust-analyzer the crate edition is fetched using snap.analysis.crate_edition(crate_id)?, so it seems like rust-analyzer wouldn't necessarily need to migrate to using the cargo fmt changes we're working on here, but it might be worth looking deeper at the crate_edition method to see if we could use a similar implementation for our use case here.

[pankdm]

What do you folks think of using the -f / --file option? This is all very minor feedback, but I just felt this might be a little bit more intuitive and more consistent with existing -p / --package.

[ytmimi]

@pankdm thanks for the input. It's hard for me to recall why I originally named the options -s / --src-file 🤔. I don't see why it couldn't be -f / --file.