Auto merge of #12529 - weihanglo:clap-suggestions, r=epage

refactor: Use clap to suggest alternative argument for unsupported arguments

Author: bors

Cargo.lock

@@ -28,7 +28,7 @@ cargo-test-macro = { path = "crates/cargo-test-macro" }
 cargo-test-support = { path = "crates/cargo-test-support" }
 cargo-util = { version = "0.2.6", path = "crates/cargo-util" }
 cargo_metadata = "0.14.0"
-clap = "4.3.19"
+clap = "4.3.23"
 core-foundation = { version = "0.9.3", features = ["mac_os_10_7_support"] }
 crates-io = { version = "0.38.0", path = "crates/crates-io" }
 criterion = { version = "0.5.1", features = ["html_reports"] }

Cargo.toml

@@ -42,8 +42,8 @@ pub fn cli() -> Command {
             "Benchmark all targets",
         )
         .arg_features()
-        .arg_jobs_without_keep_going()
-        .arg(flag("keep-going", "Use `--no-fail-fast` instead").hide(true)) // See rust-lang/cargo#11702
+        .arg_jobs()
+        .arg_unsupported_keep_going()
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Build for the target triple")
         .arg_target_dir()
@@ -56,16 +56,6 @@ pub fn cli() -> Command {
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let ws = args.workspace(config)?;
 
-    if args.keep_going() {
-        return Err(anyhow::format_err!(
-            "\
-unexpected argument `--keep-going` found
-
-  tip: to run as many benchmarks as possible without failing fast, use `--no-fail-fast`"
-        )
-        .into());
-    }
-
     let mut compile_opts = args.compile_options(
         config,
         CompileMode::Bench,

src/bin/cargo/commands/bench.rs

@@ -31,7 +31,7 @@ pub fn cli() -> Command {
         .arg_features()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
-        .arg_jobs()
+        .arg_parallel()
         .arg_target_triple("Build for the target triple")
         .arg_target_dir()
         .arg(

src/bin/cargo/commands/build.rs

@@ -29,7 +29,7 @@ pub fn cli() -> Command {
             "Check all targets",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Check artifacts in release mode, with optimizations")
         .arg_profile("Check artifacts with the specified profile")
         .arg_target_triple("Check for the target triple")

src/bin/cargo/commands/check.rs

@@ -32,7 +32,7 @@ pub fn cli() -> Command {
             "Document only the specified example",
             "Document all examples",
         )
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Build for the target triple")

src/bin/cargo/commands/doc.rs

@@ -47,7 +47,7 @@ pub fn cli() -> Command {
             "Fix all targets (default)",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Fix artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Fix for the target triple")

src/bin/cargo/commands/fix.rs

@@ -75,7 +75,7 @@ pub fn cli() -> Command {
             "Install all examples",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg(flag(
             "debug",
             "Build in debug mode (with the 'dev' profile) instead of release mode",

src/bin/cargo/commands/install.rs

@@ -33,7 +33,7 @@ pub fn cli() -> Command {
         .arg_features()
         .arg_target_triple("Build for the target triple")
         .arg_target_dir()
-        .arg_jobs()
+        .arg_parallel()
         .arg_manifest_path()
         .after_help("Run `cargo help package` for more detailed information.\n")
 }

src/bin/cargo/commands/package.rs

@@ -20,7 +20,7 @@ pub fn cli() -> Command {
         .arg_quiet()
         .arg_package("Package to publish")
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_target_triple("Build for the target triple")
         .arg_target_dir()
         .arg_manifest_path()

src/bin/cargo/commands/publish.rs

@@ -30,7 +30,7 @@ pub fn cli() -> Command {
             "Name of the example target to run",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Build for the target triple")

src/bin/cargo/commands/run.rs

@@ -44,7 +44,7 @@ pub fn cli() -> Command {
             "Build all targets",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Target triple which compiles will be for")

src/bin/cargo/commands/rustc.rs

@@ -32,7 +32,7 @@ pub fn cli() -> Command {
             "Build all targets",
         )
         .arg_features()
-        .arg_jobs()
+        .arg_parallel()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Build for the target triple")

src/bin/cargo/commands/rustdoc.rs

@@ -48,8 +48,8 @@ pub fn cli() -> Command {
             "Test all targets (does not include doctests)",
         )
         .arg_features()
-        .arg_jobs_without_keep_going()
-        .arg(flag("keep-going", "Use `--no-fail-fast` instead").hide(true)) // See rust-lang/cargo#11702
+        .arg_jobs()
+        .arg_unsupported_keep_going()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_profile("Build artifacts with the specified profile")
         .arg_target_triple("Build for the target triple")
@@ -66,16 +66,6 @@ pub fn cli() -> Command {
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let ws = args.workspace(config)?;
 
-    if args.keep_going() {
-        return Err(anyhow::format_err!(
-            "\
-unexpected argument `--keep-going` found
-
-  tip: to run as many tests as possible without failing fast, use `--no-fail-fast`"
-        )
-        .into());
-    }
-
     let mut compile_opts = args.compile_options(
         config,
         CompileMode::Test,

src/bin/cargo/commands/test.rs

@@ -32,15 +32,27 @@ pub fn cli() -> Command {
             "versioned-dirs",
             "Always include version in subdir name",
         ))
-        .arg(flag("no-merge-sources", "Not supported").hide(true))
-        .arg(flag("relative-path", "Not supported").hide(true))
-        .arg(flag("only-git-deps", "Not supported").hide(true))
-        .arg(flag("disallow-duplicates", "Not supported").hide(true))
+        .arg(unsupported("no-merge-sources"))
+        .arg(unsupported("relative-path"))
+        .arg(unsupported("only-git-deps"))
+        .arg(unsupported("disallow-duplicates"))
         .arg_quiet()
         .arg_manifest_path()
         .after_help("Run `cargo help vendor` for more detailed information.\n")
 }
 
+fn unsupported(name: &'static str) -> Arg {
+    // When we moved `cargo vendor` into Cargo itself we didn't stabilize a few
+    // flags, so try to provide a helpful error message in that case to ensure
+    // that users currently using the flag aren't tripped up.
+    let value_parser = clap::builder::UnknownArgumentValueParser::suggest("the crates.io `cargo vendor` command has been merged into Cargo")
+        .and_suggest(format!("and the flag `--{name}` isn't supported currently"))
+        .and_suggest("to continue using the flag, execute `cargo-vendor vendor ...`")
+        .and_suggest("to suggest this flag supported in Cargo, file an issue at <https://github.com/rust-lang/cargo/issues/new>");
+
+    flag(name, "").value_parser(value_parser).hide(true)
+}
+
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     // We're doing the vendoring operation ourselves, so we don't actually want
     // to respect any of the `source` configuration in Cargo itself. That's
@@ -50,34 +62,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
         config.values_mut()?.remove("source");
     }
 
-    // When we moved `cargo vendor` into Cargo itself we didn't stabilize a few
-    // flags, so try to provide a helpful error message in that case to ensure
-    // that users currently using the flag aren't tripped up.
-    let crates_io_cargo_vendor_flag = if args.flag("no-merge-sources") {
-        Some("--no-merge-sources")
-    } else if args.flag("relative-path") {
-        Some("--relative-path")
-    } else if args.flag("only-git-deps") {
-        Some("--only-git-deps")
-    } else if args.flag("disallow-duplicates") {
-        Some("--disallow-duplicates")
-    } else {
-        None
-    };
-    if let Some(flag) = crates_io_cargo_vendor_flag {
-        return Err(anyhow::format_err!(
-            "\
-the crates.io `cargo vendor` command has now been merged into Cargo itself
-and does not support the flag `{}` currently; to continue using the flag you
-can execute `cargo-vendor vendor ...`, and if you would like to see this flag
-supported in Cargo itself please feel free to file an issue at
-https://github.com/rust-lang/cargo/issues/new
-",
-            flag
-        )
-        .into());
-    }
-
     let ws = args.workspace(config)?;
     let path = args
         .get_one::<PathBuf>("path")

src/bin/cargo/commands/vendor.rs

@@ -14,6 +14,7 @@ use crate::util::{
 use crate::CargoResult;
 use anyhow::bail;
 use cargo_util::paths;
+use clap::builder::UnknownArgumentValueParser;
 use std::ffi::{OsStr, OsString};
 use std::path::Path;
 use std::path::PathBuf;
@@ -82,8 +83,8 @@ pub trait CommandExt: Sized {
         )
     }
 
-    fn arg_jobs(self) -> Self {
-        self.arg_jobs_without_keep_going()._arg(
+    fn arg_parallel(self) -> Self {
+        self.arg_jobs()._arg(
             flag(
                 "keep-going",
                 "Do not abort the build as soon as there is an error (unstable)",
@@ -92,7 +93,7 @@ pub trait CommandExt: Sized {
         )
     }
 
-    fn arg_jobs_without_keep_going(self) -> Self {
+    fn arg_jobs(self) -> Self {
         self._arg(
             opt("jobs", "Number of parallel jobs, defaults to # of CPUs.")
                 .short('j')
@@ -102,6 +103,12 @@ pub trait CommandExt: Sized {
         )
     }
 
+    fn arg_unsupported_keep_going(self) -> Self {
+        let msg = "use `--no-fail-fast` to run as many tests as possible regardless of failure";
+        let value_parser = UnknownArgumentValueParser::suggest(msg);
+        self._arg(flag("keep-going", "").value_parser(value_parser).hide(true))
+    }
+
     fn arg_targets_all(
         self,
         lib: &'static str,
@@ -431,7 +438,7 @@ pub trait ArgMatchesExt {
     }
 
     fn keep_going(&self) -> bool {
-        self.flag("keep-going")
+        self.maybe_flag("keep-going")
     }
 
     fn targets(&self) -> Vec<String> {
@@ -777,6 +784,8 @@ pub trait ArgMatchesExt {
 
     fn flag(&self, name: &str) -> bool;
 
+    fn maybe_flag(&self, name: &str) -> bool;
+
     fn _value_of(&self, name: &str) -> Option<&str>;
 
     fn _values_of(&self, name: &str) -> Vec<String>;
@@ -797,6 +806,17 @@ impl<'a> ArgMatchesExt for ArgMatches {
             .unwrap_or(false)
     }
 
+    // This works around before an upstream fix in clap for `UnknownArgumentValueParser` accepting
+    // generics arguments. `flag()` cannot be used with `--keep-going` at this moment due to
+    // <https://github.com/clap-rs/clap/issues/5081>.
+    fn maybe_flag(&self, name: &str) -> bool {
+        self.try_get_one::<bool>(name)
+            .ok()
+            .flatten()
+            .copied()
+            .unwrap_or_default()
+    }
+
     fn _value_of(&self, name: &str) -> Option<&str> {
         ignore_unknown(self.try_get_one::<String>(name)).map(String::as_str)
     }

src/cargo/util/command_prelude.rs

@@ -1671,24 +1671,6 @@ fn json_artifact_includes_executable_for_benchmark() {
         .run();
 }
 
-#[cargo_test]
-fn cargo_bench_no_keep_going() {
-    let p = project()
-        .file("Cargo.toml", &basic_bin_manifest("foo"))
-        .file("src/main.rs", "")
-        .build();
-
-    p.cargo("bench --keep-going")
-        .with_stderr(
-            "\
-error: unexpected argument `--keep-going` found
-
-  tip: to run as many benchmarks as possible without failing fast, use `--no-fail-fast`",
-        )
-        .with_status(101)
-        .run();
-}
-
 #[cargo_test(nightly, reason = "bench")]
 fn cargo_bench_print_env_verbose() {
     let p = project()

tests/testsuite/bench.rs

@@ -1 +1,2 @@
 mod help;
+mod no_keep_going;

tests/testsuite/cargo_bench/mod.rs

@@ -0,0 +1,3 @@
+[package]
+name = "foo"
+version = "0.1.0"

tests/testsuite/cargo_bench/no_keep_going/in/Cargo.toml

@@ -0,0 +1,19 @@
+use cargo_test_support::curr_dir;
+use cargo_test_support::CargoCommand;
+use cargo_test_support::Project;
+
+#[cargo_test]
+fn case() {
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+    let cwd = &project_root;
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("bench")
+        .arg("--keep-going")
+        .current_dir(cwd)
+        .assert()
+        .code(1)
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+}

tests/testsuite/cargo_bench/no_keep_going/in/src/lib.rs

@@ -0,0 +1,7 @@
+error: unexpected argument '--keep-going' found
+
+  tip: use `--no-fail-fast` to run as many tests as possible regardless of failure
+
+Usage: cargo[EXE] bench [OPTIONS] [BENCHNAME] [-- [args]...]
+
+For more information, try '--help'.

tests/testsuite/cargo_bench/no_keep_going/mod.rs

@@ -1 +1,2 @@
 mod help;
+mod no_keep_going;

tests/testsuite/cargo_bench/no_keep_going/stderr.log

@@ -0,0 +1,3 @@
+[package]
+name = "foo"
+version = "0.1.0"