Introducing clap on tidy

Cargo.lock

@@ -580,9 +580,9 @@ dependencies = [
 
 [[package]]
 name = "clap"
-version = "4.5.51"
+version = "4.5.54"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "4c26d721170e0295f191a69bd9a1f93efcdb0aff38684b61ab5750468972e5f5"
+checksum = "c6e6ff9dcd79cff5cd969a17a545d79e84ab086e444102a591e288a8aa3ce394"
 dependencies = [
  "clap_builder",
  "clap_derive",
@@ -600,9 +600,9 @@ dependencies = [
 
 [[package]]
 name = "clap_builder"
-version = "4.5.51"
+version = "4.5.54"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "75835f0c7bf681bfd05abe44e965760fea999a5286c6eb2d59883634fd02011a"
+checksum = "fa42cf4d2b7a41bc8f663a7cab4031ebafa1bf3875705bfaf8466dc60ab52c00"
 dependencies = [
  "anstream",
  "anstyle",
@@ -5622,6 +5622,7 @@ version = "0.1.0"
 dependencies = [
  "build_helper",
  "cargo_metadata 0.21.0",
+ "clap",
  "fluent-syntax",
  "globset",
  "ignore",

src/bootstrap/src/core/build_steps/test.rs

@@ -1293,19 +1293,19 @@ impl Step for Tidy {
     /// for the `dev` or `nightly` channels.
     fn run(self, builder: &Builder<'_>) {
         let mut cmd = builder.tool_cmd(Tool::Tidy);
-        cmd.arg(&builder.src);
-        cmd.arg(&builder.initial_cargo);
-        cmd.arg(&builder.out);
+        cmd.arg(format!("--root-path={}", &builder.src.display()));
+        cmd.arg(format!("--cargo-path={}", &builder.initial_cargo.display()));
+        cmd.arg(format!("--output-dir={}", &builder.out.display()));
         // Tidy is heavily IO constrained. Still respect `-j`, but use a higher limit if `jobs` hasn't been configured.
         let jobs = builder.config.jobs.unwrap_or_else(|| {
             8 * std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get) as u32
         });
-        cmd.arg(jobs.to_string());
+        cmd.arg(format!("--concurrency={jobs}"));
         // pass the path to the yarn command used for installing js deps.
         if let Some(yarn) = &builder.config.yarn {
-            cmd.arg(yarn);
+            cmd.arg(format!("--npm-path={}", yarn.display()));
         } else {
-            cmd.arg("yarn");
+            cmd.arg("--npm-path=yarn");
         }
         if builder.is_verbose() {
             cmd.arg("--verbose");

src/tools/tidy/Cargo.toml

@@ -20,6 +20,7 @@ fluent-syntax = "0.12"
 similar = "2.5.0"
 toml = "0.7.8"
 tempfile = "3.15.0"
+clap = "4.5.54"
 
 [features]
 build-metrics = ["dep:serde"]

src/tools/tidy/src/alphabetical/tests.rs

@@ -37,7 +37,7 @@ fn bless_test(before: &str, after: &str) {
     let temp_path = tempfile::Builder::new().tempfile().unwrap().into_temp_path();
     std::fs::write(&temp_path, before).unwrap();
 
-    let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::new(&["--bless".to_owned()]));
+    let tidy_ctx = TidyCtx::new(Path::new("/"), false, TidyFlags::new(true));
 
     let mut check = tidy_ctx.start_check("alphabetical-test");
     check_lines(&temp_path, before, &tidy_ctx, &mut check);

src/tools/tidy/src/arg_parser.rs

@@ -0,0 +1,102 @@
+use std::num::NonZeroUsize;
+use std::path::PathBuf;
+
+use clap::{Arg, ArgAction, ArgMatches, Command, value_parser};
+
+#[cfg(test)]
+mod tests;
+
+#[derive(Debug, Clone)]
+pub struct TidyArgParser {
+    pub root_path: PathBuf,
+    pub cargo: PathBuf,
+    pub output_directory: PathBuf,
+    pub concurrency: NonZeroUsize,
+    pub npm: PathBuf,
+    pub verbose: bool,
+    pub bless: bool,
+    pub extra_checks: Option<Vec<String>>,
+    pub pos_args: Vec<String>,
+}
+
+impl TidyArgParser {
+    fn command() -> Command {
+        Command::new("rust-tidy")
+            .arg(
+                Arg::new("root_path")
+                    .help("path of the root directory")
+                    .long("root-path")
+                    .required(true)
+                    .value_parser(value_parser!(PathBuf)),
+            )
+            .arg(
+                Arg::new("cargo")
+                    .help("path of cargo")
+                    .long("cargo-path")
+                    .required(true)
+                    .value_parser(value_parser!(PathBuf)),
+            )
+            .arg(
+                Arg::new("output_directory")
+                    .help("path of output directory")
+                    .long("output-dir")
+                    .required(true)
+                    .value_parser(value_parser!(PathBuf)),
+            )
+            .arg(
+                Arg::new("concurrency")
+                    .help("number of threads working concurrently")
+                    .long("concurrency")
+                    .required(true)
+                    .value_parser(value_parser!(NonZeroUsize)),
+            )
+            .arg(
+                Arg::new("npm")
+                    .help("path of npm")
+                    .long("npm-path")
+                    .required(true)
+                    .value_parser(value_parser!(PathBuf)),
+            )
+            .arg(Arg::new("verbose").help("verbose").long("verbose").action(ArgAction::SetTrue))
+            .arg(Arg::new("bless").help("target files are modified").long("bless").action(ArgAction::SetTrue))
+            .arg(
+                Arg::new("extra_checks")
+                    .help("extra checks")
+                    .long("extra-checks")
+                    .value_delimiter(',')
+                    .action(ArgAction::Append),
+            )
+            .arg(Arg::new("pos_args").help("for extra checks. you can specify configs and target files for external check tools").action(ArgAction::Append).last(true))
+    }
+
+    fn build(matches: ArgMatches) -> Self {
+        let mut tidy_flags = Self {
+            root_path: matches.get_one::<PathBuf>("root_path").unwrap().clone(),
+            cargo: matches.get_one::<PathBuf>("cargo").unwrap().clone(),
+            output_directory: matches.get_one::<PathBuf>("output_directory").unwrap().clone(),
+            concurrency: *matches.get_one::<NonZeroUsize>("concurrency").unwrap(),
+            npm: matches.get_one::<PathBuf>("npm").unwrap().clone(),
+            verbose: *matches.get_one::<bool>("verbose").unwrap(),
+            bless: *matches.get_one::<bool>("bless").unwrap(),
+            extra_checks: None,
+            pos_args: vec![],
+        };
+
+        if let Some(extra_checks) = matches.get_many::<String>("extra_checks") {
+            tidy_flags.extra_checks = Some(extra_checks.map(|s| s.to_string()).collect::<Vec<_>>());
+        }
+
+        tidy_flags.pos_args = matches
+            .get_many::<String>("pos_args")
+            .unwrap_or_default()
+            .map(|v| v.to_string())
+            .collect::<Vec<_>>();
+
+        tidy_flags
+    }
+
+    pub fn parse() -> Self {
+        let matches = Self::command().get_matches();
+        Self::build(matches)
+    }
+}

src/tools/tidy/src/arg_parser/tests.rs

@@ -0,0 +1,168 @@
+use std::path::PathBuf;
+
+use crate::arg_parser::TidyArgParser;
+
+// Test all arguments
+#[test]
+fn test_tidy_parser_full() {
+    let args = vec![
+        "rust-tidy",
+        "--root-path",
+        "/home/user/rust",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--concurrency",
+        "16",
+        "--npm-path",
+        "yarn",
+        "--verbose",
+        "--bless",
+        "--extra-checks",
+        "if-installed:auto:js,auto:if-installed:py,if-installed:auto:cpp,if-installed:auto:spellcheck",
+        "--", // pos_args
+        "some-file",
+        "some-file2",
+    ];
+    let cmd = TidyArgParser::command();
+    let parsed_args = TidyArgParser::build(cmd.get_matches_from(args));
+
+    assert_eq!(parsed_args.root_path, PathBuf::from("/home/user/rust"));
+    assert_eq!(
+        parsed_args.cargo,
+        PathBuf::from("/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo")
+    );
+    assert_eq!(parsed_args.output_directory, PathBuf::from("/home/user/rust/build"));
+    assert_eq!(parsed_args.concurrency.get(), 16);
+    assert_eq!(parsed_args.npm, PathBuf::from("yarn"));
+    assert!(parsed_args.verbose);
+    assert!(parsed_args.bless);
+    assert_eq!(
+        parsed_args.extra_checks,
+        Some(vec![
+            "if-installed:auto:js".to_string(),
+            "auto:if-installed:py".to_string(),
+            "if-installed:auto:cpp".to_string(),
+            "if-installed:auto:spellcheck".to_string(),
+        ])
+    );
+    assert_eq!(parsed_args.pos_args, vec!["some-file".to_string(), "some-file2".to_string()]);
+}
+
+// The parser can take required args any order
+#[test]
+fn test_tidy_parser_any_order() {
+    let args = vec![
+        "rust-tidy",
+        "--npm-path",
+        "yarn",
+        "--concurrency",
+        "16",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+        "--root-path",
+        "/home/user/rust",
+    ];
+    let cmd = TidyArgParser::command();
+    let parsed_args = TidyArgParser::build(cmd.get_matches_from(args));
+
+    assert_eq!(parsed_args.root_path, PathBuf::from("/home/user/rust"));
+    assert_eq!(
+        parsed_args.cargo,
+        PathBuf::from("/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo")
+    );
+    assert_eq!(parsed_args.output_directory, PathBuf::from("/home/user/rust/build"));
+    assert_eq!(parsed_args.concurrency.get(), 16);
+    assert_eq!(parsed_args.npm, PathBuf::from("yarn"));
+}
+
+// --root-path is required
+#[test]
+fn test_tidy_parser_missing_root_path() {
+    let args = vec![
+        "rust-tidy",
+        "--npm-path",
+        "yarn",
+        "--concurrency",
+        "16",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+    ];
+    let cmd = TidyArgParser::command();
+    assert!(cmd.try_get_matches_from(args).is_err());
+}
+
+// --cargo-path is required
+#[test]
+fn test_tidy_parser_missing_cargo_path() {
+    let args = vec![
+        "rust-tidy",
+        "--npm-path",
+        "yarn",
+        "--concurrency",
+        "16",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--root-path",
+        "/home/user/rust",
+    ];
+    let cmd = TidyArgParser::command();
+    assert!(cmd.try_get_matches_from(args).is_err());
+}
+
+// --output-dir is required
+#[test]
+fn test_tidy_parser_missing_output_dir() {
+    let args = vec![
+        "rust-tidy",
+        "--npm-path",
+        "yarn",
+        "--concurrency",
+        "16",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+        "--root-path",
+        "/home/user/rust",
+    ];
+    let cmd = TidyArgParser::command();
+    assert!(cmd.try_get_matches_from(args).is_err());
+}
+
+// --concurrency is required
+#[test]
+fn test_tidy_parser_missing_concurrency() {
+    let args = vec![
+        "rust-tidy",
+        "--npm-path",
+        "yarn",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+        "--root-path",
+        "/home/user/rust",
+    ];
+    let cmd = TidyArgParser::command();
+    assert!(cmd.try_get_matches_from(args).is_err());
+}
+
+// --npm-path is required
+#[test]
+fn test_tidy_parser_missing_npm_path() {
+    let args = vec![
+        "rust-tidy",
+        "--concurrency",
+        "16",
+        "--output-dir",
+        "/home/user/rust/build",
+        "--cargo-path",
+        "/home/user/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo",
+    ];
+    let cmd = TidyArgParser::command();
+    assert!(cmd.try_get_matches_from(args).is_err());
+}

src/tools/tidy/src/diagnostics.rs

@@ -14,16 +14,8 @@ pub struct TidyFlags {
 }
 
 impl TidyFlags {
-    pub fn new(cfg_args: &[String]) -> Self {
-        let mut flags = Self::default();
-
-        for arg in cfg_args {
-            match arg.as_str() {
-                "--bless" => flags.bless = true,
-                _ => continue,
-            }
-        }
-        flags
+    pub fn new(bless: bool) -> Self {
+        Self { bless }
     }
 }
 

src/tools/tidy/src/extra_checks/mod.rs

@@ -58,8 +58,8 @@ pub fn check(
     tools_path: &Path,
     npm: &Path,
     cargo: &Path,
-    extra_checks: Option<&str>,
-    pos_args: &[String],
+    extra_checks: Option<Vec<String>>,
+    pos_args: Vec<String>,
     tidy_ctx: TidyCtx,
 ) {
     let mut check = tidy_ctx.start_check("extra_checks");
@@ -88,8 +88,8 @@ fn check_impl(
     tools_path: &Path,
     npm: &Path,
     cargo: &Path,
-    extra_checks: Option<&str>,
-    pos_args: &[String],
+    extra_checks: Option<Vec<String>>,
+    pos_args: Vec<String>,
     tidy_ctx: &TidyCtx,
 ) -> Result<(), Error> {
     let show_diff =
@@ -99,9 +99,7 @@ fn check_impl(
     // Split comma-separated args up
     let mut lint_args = match extra_checks {
         Some(s) => s
-            .strip_prefix("--extra-checks=")
-            .unwrap()
-            .split(',')
+            .iter()
             .map(|s| {
                 if s == "spellcheck:fix" {
                     eprintln!("warning: `spellcheck:fix` is no longer valid, use `--extra-checks=spellcheck --bless`");