From 7c141914ad77aff7c81599586bea96956cc8d346 Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Mon, 8 Nov 2021 23:18:03 -0500 Subject: [PATCH] Allow cargo-fmt to handle formatting individual files. Adds an additional command line option ``--src-file`` / ``-s`` It should be noted that this does not allow cargo-fmt to format an arbitrary rust file. The file must be contained with a target specified in a Cargo.toml file. Adjustments were also made to prevent cargo-fmt from passing along certain arguments to rustfmt. For example, users can no longer pass rust files to rustfmt through cargo-fmt. Additionally, users cannot pass --edition to cargo-fmt, since editions are read from the Cargo.toml. Tests were added in ``src/cargo-fmt/test/mod.rs`` and integration tests were added to ``tests/cargo-fmt/main.rs`` --- src/cargo-fmt/main.rs | 92 +++++++++++++++++++++++++++++++++++++-- src/cargo-fmt/test/mod.rs | 60 +++++++++++++++++++++++++ tests/cargo-fmt/main.rs | 39 +++++++++++++++++ 3 files changed, 188 insertions(+), 3 deletions(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 3542536f29b..2788301a583 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -49,6 +49,10 @@ pub struct Opts { )] packages: Vec, + /// Specify a source file to format + #[clap(short = 's', long = "src-file", value_name = "src-file")] + src_file: Option, + /// Specify path to Cargo.toml #[clap(long = "manifest-path", value_name = "manifest-path")] manifest_path: Option, @@ -94,6 +98,28 @@ fn execute() -> i32 { let opts = Opts::parse_from(args); + if opts.src_file.is_some() & !opts.packages.is_empty() { + print_usage_to_stderr("cannot format source files and packages at the same time"); + return FAILURE; + } + + if opts.src_file.is_some() & opts.format_all { + print_usage_to_stderr("cannot format all packages when specifying source files"); + return FAILURE; + } + + if opts.rustfmt_options.iter().any(|s| s.ends_with(".rs")) { + print_usage_to_stderr( + "cannot pass rust files to rustfmt through cargo-fmt. Use '--src-file' instead", + ); + return FAILURE; + } + + if opts.rustfmt_options.iter().any(|s| s.contains("--edition")) { + print_usage_to_stderr("cannot pass '--edition' to rustfmt through cargo-fmt"); + return FAILURE; + } + let verbosity = match (opts.verbose, opts.quiet) { (false, false) => Verbosity::Normal, (false, true) => Verbosity::Quiet, @@ -319,17 +345,23 @@ pub enum CargoFmtStrategy { /// Format every packages and dependencies. All, /// Format packages that are specified by the command line argument. - Some(Vec), + Packages(Vec), /// Format the root packages only. Root, + /// Format individual source files specified by the command line arguments. + SourceFile(PathBuf), } impl CargoFmtStrategy { pub fn from_opts(opts: &Opts) -> CargoFmtStrategy { + if let Some(ref src_file) = opts.src_file { + return CargoFmtStrategy::SourceFile(src_file.clone()); + } + match (opts.format_all, opts.packages.is_empty()) { (false, true) => CargoFmtStrategy::Root, (true, _) => CargoFmtStrategy::All, - (false, false) => CargoFmtStrategy::Some(opts.packages.clone()), + (false, false) => CargoFmtStrategy::Packages(opts.packages.clone()), } } } @@ -346,9 +378,12 @@ fn get_targets( CargoFmtStrategy::All => { get_targets_recursive(manifest_path, &mut targets, &mut BTreeSet::new())? } - CargoFmtStrategy::Some(ref hitlist) => { + CargoFmtStrategy::Packages(ref hitlist) => { get_targets_with_hitlist(manifest_path, hitlist, &mut targets)? } + CargoFmtStrategy::SourceFile(ref src_file) => { + get_target_from_src_file(manifest_path, &src_file, &mut targets)? + } } if targets.is_empty() { @@ -361,6 +396,57 @@ fn get_targets( } } +fn get_target_from_src_file( + manifest_path: Option<&Path>, + src_file: &PathBuf, + targets: &mut BTreeSet, +) -> Result<(), io::Error> { + let metadata = get_cargo_metadata(manifest_path)?; + + let get_target = |src_path: &Path| { + metadata + .packages + .iter() + .map(|p| p.targets.iter()) + .flatten() + .filter(|t| { + let kind = &t.kind[0]; + // to prevent formatting any arbitrary file within the root of our + // project we special case the build.rs script, becuase it's parent + // is the root of the project and we would always select the custom-build + // target in the event that we couldn't find a better target to associate + // with our file. + if kind == "custom-build" && !src_path.ends_with("build.rs") { + return false; + } + + if let Ok(target_path) = fs::canonicalize(&t.src_path) { + let target_dir = target_path + .parent() + .expect("Target src_path should have a parent directory"); + src_path.starts_with(target_dir) + } else { + false + } + }) + .max_by(|t1, t2| { + let t1_len = t1.src_path.components().count(); + let t2_len = t2.src_path.components().count(); + t1_len.cmp(&t2_len) + }) + }; + + let canonicalize = fs::canonicalize(&src_file)?; + if let Some(target) = get_target(&canonicalize) { + targets.insert(Target { + path: src_file.to_owned(), + kind: target.kind[0].clone(), + edition: target.edition.clone(), + }); + } + Ok(()) +} + fn get_targets_root_only( manifest_path: Option<&Path>, targets: &mut BTreeSet, diff --git a/src/cargo-fmt/test/mod.rs b/src/cargo-fmt/test/mod.rs index 56e52fbabb6..5fce88683cd 100644 --- a/src/cargo-fmt/test/mod.rs +++ b/src/cargo-fmt/test/mod.rs @@ -1,4 +1,5 @@ use super::*; +use std::path::PathBuf; mod message_format; mod targets; @@ -16,6 +17,7 @@ fn default_options() { assert_eq!(false, o.format_all); assert_eq!(None, o.manifest_path); assert_eq!(None, o.message_format); + assert_eq!(None, o.src_file); } #[test] @@ -27,6 +29,8 @@ fn good_options() { "p1", "-p", "p2", + "-s", + "file.rs", "--message-format", "short", "--check", @@ -39,6 +43,7 @@ fn good_options() { assert_eq!(false, o.version); assert_eq!(true, o.check); assert_eq!(vec!["p1", "p2"], o.packages); + assert_eq!(Some(PathBuf::from("file.rs")), o.src_file); assert_eq!(vec!["--edition", "2018"], o.rustfmt_options); assert_eq!(false, o.format_all); assert_eq!(Some(String::from("short")), o.message_format); @@ -90,6 +95,25 @@ fn multiple_packages_one_by_one() { assert_eq!(3, o.packages.len()); } +#[test] +fn cant_list_more_than_one_source_file() { + assert!( + Opts::command() + .try_get_matches_from(&["test", "-s", "src/a.rs", "--src-file", "src/b.rs"]) + .is_err() + ); + assert!( + !Opts::command() + .try_get_matches_from(&["test", "-s", "src/a.rs"]) + .is_err() + ); + assert!( + !Opts::command() + .try_get_matches_from(&["test", "--src-file", "src/b.rs"]) + .is_err() + ); +} + #[test] fn multiple_packages_grouped() { let o = Opts::parse_from(&[ @@ -139,3 +163,39 @@ fn empty_packages_4() { .is_err() ); } + +#[test] +fn empty_source_files_1() { + assert!( + Opts::command() + .try_get_matches_from(&["test", "-s"]) + .is_err() + ); +} + +#[test] +fn empty_source_files_2() { + assert!( + Opts::command() + .try_get_matches_from(&["test", "-s", "--", "--check"]) + .is_err() + ); +} + +#[test] +fn empty_source_files_3() { + assert!( + Opts::command() + .try_get_matches_from(&["test", "-s", "--verbose"]) + .is_err() + ); +} + +#[test] +fn empty_source_files_4() { + assert!( + Opts::command() + .try_get_matches_from(&["test", "-s", "--check"]) + .is_err() + ); +} diff --git a/tests/cargo-fmt/main.rs b/tests/cargo-fmt/main.rs index 348876cd264..8c0e79b4161 100644 --- a/tests/cargo-fmt/main.rs +++ b/tests/cargo-fmt/main.rs @@ -96,3 +96,42 @@ fn cargo_fmt_out_of_line_test_modules() { assert!(stdout.contains(&format!("Diff in {}", path.display()))) } } + +#[test] +fn cannot_pass_rust_files_to_rustfmt_through_caro_fmt() { + let (_, stderr) = cargo_fmt(&["--", "src/main.rs"]); + assert!(stderr.starts_with( + "cannot pass rust files to rustfmt through cargo-fmt. Use '--src-file' instead" + )) +} + +#[test] +fn cannot_pass_edition_to_rustfmt_through_caro_fmt() { + let (_, stderr) = cargo_fmt(&["--", "--edition", "2021"]); + assert!(stderr.starts_with("cannot pass '--edition' to rustfmt through cargo-fmt")) +} + +#[test] +fn specify_source_files_when_running_cargo_fmt() { + let path = "tests/cargo-fmt/source/workspaces/path-dep-above/e/src/main.rs"; + // test that we can run cargo-fmt on a single src file + assert_that!(&["-v", "--check", "-s", path], contains(path)); +} + +#[test] +fn specify_source_files_in_a_workspace_when_running_cargo_fmt() { + let path = "tests/cargo-fmt/source/workspaces/path-dep-above/ws/a/src/main.rs"; + assert_that!(&["-v", "--check", "-s", path], contains(path)); +} + +#[test] +fn formatting_source_files_and_packages_at_the_same_time_is_not_supported() { + let (_, stderr) = cargo_fmt(&["--check", "-s", "src/main.rs", "-p", "p1"]); + assert!(stderr.starts_with("cannot format source files and packages at the same time")) +} + +#[test] +fn formatting_source_files_and_using_package_related_arguments_is_not_supported() { + let (_, stderr) = cargo_fmt(&["--check", "--all", "-s", "src/main.rs"]); + assert!(stderr.starts_with("cannot format all packages when specifying source files")) +}