Repository: clap-rs/clap
Pull request #4380 (2022-10-13 16:21:25)

fix(error): Don't suggest '--' when it doesn't help
This is a first pass at #2766 so we can collect further feedback


Patch:
diff --git a/src/error/context.rs b/src/error/context.rs
index d9921b4e..e0055bb2 100644
--- a/src/error/context.rs
+++ b/src/error/context.rs
@@ -29,6 +29,8 @@ pub enum ContextKind {
     SuggestedValue,
     /// Trailing argument
     TrailingArg,
+    /// Potential fix for the user
+    SuggestedTrailingArg,
     /// A usage string
     Usage,
     /// An opaque message to the user
@@ -51,6 +53,7 @@ impl ContextKind {
             Self::SuggestedSubcommand => Some("Suggested Subcommand"),
             Self::SuggestedArg => Some("Suggested Argument"),
             Self::SuggestedValue => Some("Suggested Value"),
+            Self::SuggestedTrailingArg => Some("Suggested Trailing Argument"),
             Self::TrailingArg => Some("Trailing Argument"),
             Self::Usage => None,
             Self::Custom => None,
diff --git a/src/error/format.rs b/src/error/format.rs
index 15208b32..39ddea1b 100644
--- a/src/error/format.rs
+++ b/src/error/format.rs
@@ -360,7 +360,8 @@ fn write_dynamic_context(error: &crate::error::Error, styled: &mut StyledStr) ->
 
                 let invalid_arg = error.get(ContextKind::InvalidArg);
                 if let Some(ContextValue::String(invalid_arg)) = invalid_arg {
-                    if invalid_arg.starts_with('-') {
+                    let suggested_trailing_arg = error.get(ContextKind::SuggestedTrailingArg);
+                    if suggested_trailing_arg == Some(&ContextValue::Bool(true)) {
                         styled.none("\n\n");
                         styled.none(TAB);
                         styled.none("If you tried to supply '");
diff --git a/src/error/mod.rs b/src/error/mod.rs
index 932e9065..184761c9 100644
--- a/src/error/mod.rs
+++ b/src/error/mod.rs
@@ -638,6 +638,7 @@ impl<F: ErrorFormatter> Error<F> {
         cmd: &Command,
         arg: String,
         did_you_mean: Option<(String, Option<String>)>,
+        suggested_trailing_arg: bool,
         usage: Option<StyledStr>,
     ) -> Self {
         let mut err = Self::new(ErrorKind::UnknownArgument).with_cmd(cmd);
@@ -662,6 +663,12 @@ impl<F: ErrorFormatter> Error<F> {
                     );
                 }
             }
+            if suggested_trailing_arg {
+                err = err.insert_context_unchecked(
+                    ContextKind::SuggestedTrailingArg,
+                    ContextValue::Bool(true),
+                );
+            }
         }
 
         err
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index 9a224cd8..b6fb18e6 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -175,7 +175,12 @@ impl<'cmd> Parser<'cmd> {
                                 .remaining(&mut args_cursor)
                                 .map(|x| x.to_str().expect(INVALID_UTF8))
                                 .collect();
-                            return Err(self.did_you_mean_error(&arg, matcher, &remaining_args));
+                            return Err(self.did_you_mean_error(
+                                &arg,
+                                matcher,
+                                &remaining_args,
+                                trailing_values,
+                            ));
                         }
                         ParseResult::UnneededAttachedValue { rest, used, arg } => {
                             let _ = self.resolve_pending(matcher);
@@ -257,10 +262,14 @@ impl<'cmd> Parser<'cmd> {
                         }
                         ParseResult::NoMatchingArg { arg } => {
                             let _ = self.resolve_pending(matcher);
+                            // We already know it looks like a flag
+                            let suggested_trailing_arg =
+                                !trailing_values && self.cmd.has_positionals();
                             return Err(ClapError::unknown_argument(
                                 self.cmd,
                                 arg,
                                 None,
+                                suggested_trailing_arg,
                                 Usage::new(self.cmd).create_usage_with_title(&[]),
                             ));
                         }
@@ -374,10 +383,14 @@ impl<'cmd> Parser<'cmd> {
             if let Some(arg) = self.cmd.get_keymap().get(&pos_counter) {
                 if arg.is_last_set() && !trailing_values {
                     let _ = self.resolve_pending(matcher);
+                    // Its already considered a positional, we don't need to suggest turning it
+                    // into one
+                    let suggested_trailing_arg = false;
                     return Err(ClapError::unknown_argument(
                         self.cmd,
                         arg_os.display().to_string(),
                         None,
+                        suggested_trailing_arg,
                         Usage::new(self.cmd).create_usage_with_title(&[]),
                     ));
                 }
@@ -524,10 +537,14 @@ impl<'cmd> Parser<'cmd> {
             );
         }
 
+        let suggested_trailing_arg = !trailing_values
+            && self.cmd.has_positionals()
+            && (arg_os.is_long() || arg_os.is_short());
         ClapError::unknown_argument(
             self.cmd,
             arg_os.display().to_string(),
             None,
+            suggested_trailing_arg,
             Usage::new(self.cmd).create_usage_with_title(&[]),
         )
     }
@@ -1511,6 +1528,7 @@ impl<'cmd> Parser<'cmd> {
         arg: &str,
         matcher: &mut ArgMatcher,
         remaining_args: &[&str],
+        trailing_values: bool,
     ) -> ClapError {
         debug!("Parser::did_you_mean_error: arg={}", arg);
         // Didn't match a flag or option
@@ -1549,10 +1567,16 @@ impl<'cmd> Parser<'cmd> {
             .cloned()
             .collect();
 
+        // `did_you_mean` is a lot more likely and should cause us to skip the `--` suggestion
+        //
+        // In theory, this is only called for `--long`s, so we don't need to check
+        let suggested_trailing_arg =
+            did_you_mean.is_none() && !trailing_values && self.cmd.has_positionals();
         ClapError::unknown_argument(
             self.cmd,
             format!("--{}", arg),
             did_you_mean,
+            suggested_trailing_arg,
             Usage::new(self.cmd)
                 .required(&required)
                 .create_usage_with_title(&*used),


Test patch:
diff --git a/examples/derive_ref/interop_tests.md b/examples/derive_ref/interop_tests.md
index 2645a615..b013b0c8 100644
--- a/examples/derive_ref/interop_tests.md
+++ b/examples/derive_ref/interop_tests.md
@@ -37,8 +37,6 @@ $ interop_augment_args --unknown
 ? failed
 error: Found argument '--unknown' which wasn't expected, or isn't valid in this context
 
-  If you tried to supply '--unknown' as a value rather than a flag, use '-- --unknown'
-
 Usage: interop_augment_args[EXE] [OPTIONS]
 
 For more information try '--help'
@@ -74,8 +72,6 @@ $ interop_augment_subcommands derived --unknown
 ? failed
 error: Found argument '--unknown' which wasn't expected, or isn't valid in this context
 
-  If you tried to supply '--unknown' as a value rather than a flag, use '-- --unknown'
-
 Usage: interop_augment_subcommands[EXE] derived [OPTIONS]
 
 For more information try '--help'
@@ -245,8 +241,6 @@ $ interop_flatten_hand_args --unknown
 ? failed
 error: Found argument '--unknown' which wasn't expected, or isn't valid in this context
 
-  If you tried to supply '--unknown' as a value rather than a flag, use '-- --unknown'
-
 Usage: interop_flatten_hand_args[EXE] [OPTIONS]
 
 For more information try '--help'
diff --git a/tests/builder/error.rs b/tests/builder/error.rs
index f927955f..59f034ac 100644
--- a/tests/builder/error.rs
+++ b/tests/builder/error.rs
@@ -128,3 +128,62 @@ For more information try '--help'
 ";
     assert_error(err, expected_kind, MESSAGE, true);
 }
+
+#[test]
+#[cfg(feature = "error-context")]
+fn suggest_trailing() {
+    let cmd = Command::new("rg").arg(arg!([PATTERN]));
+
+    let res = cmd.try_get_matches_from(["rg", "--foo"]);
+    assert!(res.is_err());
+    let err = res.unwrap_err();
+    let expected_kind = ErrorKind::UnknownArgument;
+    static MESSAGE: &str = "\
+error: Found argument '--foo' which wasn't expected, or isn't valid in this context
+
+  If you tried to supply '--foo' as a value rather than a flag, use '-- --foo'
+
+Usage: rg [PATTERN]
+
+For more information try '--help'
+";
+    assert_error(err, expected_kind, MESSAGE, true);
+}
+
+#[test]
+#[cfg(feature = "error-context")]
+fn trailing_already_in_use() {
+    let cmd = Command::new("rg").arg(arg!([PATTERN]));
+
+    let res = cmd.try_get_matches_from(["rg", "--", "--foo", "--foo"]);
+    assert!(res.is_err());
+    let err = res.unwrap_err();
+    let expected_kind = ErrorKind::UnknownArgument;
+    static MESSAGE: &str = "\
+error: Found argument '--foo' which wasn't expected, or isn't valid in this context
+
+Usage: rg [PATTERN]
+
+For more information try '--help'
+";
+    assert_error(err, expected_kind, MESSAGE, true);
+}
+
+#[test]
+#[cfg(feature = "error-context")]
+fn cant_use_trailing() {
+    let cmd = Command::new("test");
+
+    let res = cmd.try_get_matches_from(["test", "--foo"]);
+    assert!(res.is_err());
+    let err = res.unwrap_err();
+    let expected_kind = ErrorKind::UnknownArgument;
+    static MESSAGE: &str = "\
+error: Found argument '--foo' which wasn't expected, or isn't valid in this context
+
+Usage: test
+
+For more information try '--help'
+";
+    assert_error(err, expected_kind, MESSAGE, true);
+}
diff --git a/tests/builder/opts.rs b/tests/builder/opts.rs
index 86258e26..cccd1146 100644
--- a/tests/builder/opts.rs
+++ b/tests/builder/opts.rs
@@ -451,8 +451,6 @@ error: Found argument '--optio' which wasn't expected, or isn't valid in this co
 
   Did you mean '--option'?
 
-  If you tried to supply '--optio' as a value rather than a flag, use '-- --optio'
-
 Usage: clap-test --option <opt>... [positional] [positional2] [positional3]...
 
 For more information try '--help'
@@ -551,8 +549,6 @@ error: Found argument '--files-without-matches' which wasn't expected, or isn't
 
   Did you mean '--files-without-match'?
 
-  If you tried to supply '--files-without-matches' as a value rather than a flag, use '-- --files-without-matches'
-
 Usage: ripgrep-616 --files-without-match
 
 For more information try '--help'
diff --git a/tests/builder/subcommands.rs b/tests/builder/subcommands.rs
index 509f3dfc..58206fa2 100644
--- a/tests/builder/subcommands.rs
+++ b/tests/builder/subcommands.rs
@@ -145,8 +145,6 @@ error: Found argument '--subcmarg' which wasn't expected, or isn't valid in this
 
   Did you mean to put '--subcmdarg' after the subcommand 'subcmd'?
 
-  If you tried to supply '--subcmarg' as a value rather than a flag, use '-- --subcmarg'
-
 Usage: dym [COMMAND]
 
 For more information try '--help'
@@ -165,8 +163,6 @@ fn subcmd_did_you_mean_output_arg_false_positives() {
     static EXPECTED: &str = "\
 error: Found argument '--subcmarg' which wasn't expected, or isn't valid in this context
 
-  If you tried to supply '--subcmarg' as a value rather than a flag, use '-- --subcmarg'
-
 Usage: dym [COMMAND]
 
 For more information try '--help'
diff --git a/tests/ui/error_stderr.toml b/tests/ui/error_stderr.toml
index 80f46113..da9e7469 100644
--- a/tests/ui/error_stderr.toml
+++ b/tests/ui/error_stderr.toml
@@ -5,8 +5,6 @@ stdout = ""
 stderr = """
 error: Found argument '--unknown-argument' which wasn't expected, or isn't valid in this context
 
-  If you tried to supply '--unknown-argument' as a value rather than a flag, use '-- --unknown-argument'
-
 Usage: stdio-fixture[EXE] [OPTIONS] [COMMAND]
 
 For more information try '--help'