refactor(arg): use UnknownArgumentValueParser to suggest arg for unknown args

Author: weihanglo

src/bin/cargo/commands/bench.rs

@@ -43,7 +43,7 @@ pub fn cli() -> Command {
         )
         .arg_features()
         .arg_jobs()
-        .arg(flag("keep-going", "Use `--no-fail-fast` instead").hide(true)) // See rust-lang/cargo#11702
+        .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/test.rs

@@ -49,7 +49,7 @@ pub fn cli() -> Command {
         )
         .arg_features()
         .arg_jobs()
-        .arg(flag("keep-going", "Use `--no-fail-fast` instead").hide(true)) // See rust-lang/cargo#11702
+        .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/vendor.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/cargo/util/command_prelude.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;
@@ -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)
     }

tests/testsuite/bench.rs

@@ -1681,11 +1681,16 @@ fn cargo_bench_no_keep_going() {
     p.cargo("bench --keep-going")
         .with_stderr(
             "\
-error: unexpected argument `--keep-going` found
+error: unexpected argument '--keep-going' found
 
-  tip: to run as many benchmarks as possible without failing fast, use `--no-fail-fast`",
+  tip: use `--no-fail-fast` to run as many tests as possible regardless of failure
+
+Usage: cargo bench [OPTIONS] [BENCHNAME] [-- [args]...]
+
+For more information, try '--help'.
+",
         )
-        .with_status(101)
+        .with_status(1)
         .run();
 }
 

tests/testsuite/test.rs

@@ -4854,11 +4854,16 @@ fn cargo_test_no_keep_going() {
     p.cargo("test --keep-going")
         .with_stderr(
             "\
-error: unexpected argument `--keep-going` found
+error: unexpected argument '--keep-going' found
 
-  tip: to run as many tests as possible without failing fast, use `--no-fail-fast`",
+  tip: use `--no-fail-fast` to run as many tests as possible regardless of failure
+
+Usage: cargo test [OPTIONS] [TESTNAME] [-- [args]...]
+
+For more information, try '--help'.
+",
         )
-        .with_status(101)
+        .with_status(1)
         .run();
 }