Repository: clap-rs/clap
Pull request #1346 (2018-09-27 00:30:13)

fix(suggestions): Dont mention unused subcommands
Let's say I have a CLI app like this: 
```
foo bar --baz
foo bam --baz
```

And a user invokes it as: `foo --baz bar`

We would currently suggest something like `"Did you mean to put '--baz' after the subcommand 'bam'"`, even though the user clearly didn't show the intent to invoke `bam`, but wanted to invoke `bar`.

This PR fixes this by reading the entire argument line and checking for potential occurences of all subcommands. Of course this is super fuzzy because we have no idea what is supposed to be a value of an option, and what is part of the subcommand, but it's slightly better than just taking the first subcommand that happens to have the option.


Patch:
diff --git a/src/app/parser.rs b/src/app/parser.rs
index fb1e5c0a..cb367cf6 100644
--- a/src/app/parser.rs
+++ b/src/app/parser.rs
@@ -928,7 +928,7 @@ where
                     }
 
                     if arg_os.starts_with(b"--") {
-                        needs_val_of = self.parse_long_arg(matcher, &arg_os)?;
+                        needs_val_of = self.parse_long_arg(matcher, &arg_os, it)?;
                         debugln!(
                             "Parser:get_matches_with: After parse_long_arg {:?}",
                             needs_val_of
@@ -1554,11 +1554,16 @@ where
         }
     }
 
-    fn parse_long_arg(
+    fn parse_long_arg<I, T>(
         &mut self,
         matcher: &mut ArgMatcher<'a>,
         full_arg: &OsStr,
-    ) -> ClapResult<ParseResult<'a>> {
+        it: &mut Peekable<I>,
+    ) -> ClapResult<ParseResult<'a>>
+    where
+        I: Iterator<Item = T>,
+        T: Into<OsString> + Clone,
+    {
         // maybe here lifetime should be 'a
         debugln!("Parser::parse_long_arg;");
 
@@ -1614,8 +1619,14 @@ where
         }
 
         debugln!("Parser::parse_long_arg: Didn't match anything");
-        self.did_you_mean_error(arg.to_str().expect(INVALID_UTF8), matcher)
-            .map(|_| ParseResult::NotFound)
+
+        let args_rest: Vec<_> = it.map(|x| x.clone().into()).collect();
+        let args_rest2: Vec<_> = args_rest.iter().map(|x| x.to_str().expect(INVALID_UTF8)).collect();
+        self.did_you_mean_error(
+            arg.to_str().expect(INVALID_UTF8),
+            matcher,
+            &args_rest2[..]
+        ).map(|_| ParseResult::NotFound)
     }
 
     #[cfg_attr(feature = "lints", allow(len_zero))]
@@ -1872,9 +1883,9 @@ where
         Ok(ParseResult::Flag)
     }
 
-    fn did_you_mean_error(&self, arg: &str, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> {
+    fn did_you_mean_error(&self, arg: &str, matcher: &mut ArgMatcher<'a>, args_rest: &[&str]) -> ClapResult<()> {
         // Didn't match a flag or option
-        let suffix = suggestions::did_you_mean_flag_suffix(arg, longs!(self), &self.subcommands);
+        let suffix = suggestions::did_you_mean_flag_suffix(arg, &args_rest, longs!(self), &self.subcommands);
 
         // Add the arg to the matches to build a proper usage string
         if let Some(name) = suffix.1 {
diff --git a/src/suggestions.rs b/src/suggestions.rs
index 23832cb3..06071d21 100644
--- a/src/suggestions.rs
+++ b/src/suggestions.rs
@@ -44,6 +44,7 @@ where
 #[cfg_attr(feature = "lints", allow(needless_lifetimes))]
 pub fn did_you_mean_flag_suffix<'z, T, I>(
     arg: &str,
+    args_rest: &'z [&str],
     longs: I,
     subcommands: &'z [App],
 ) -> (String, Option<&'z str>)
@@ -51,16 +52,18 @@ where
     T: AsRef<str> + 'z,
     I: IntoIterator<Item = &'z T>,
 {
-    match did_you_mean(arg, longs) {
-        Some(candidate) => {
-            let suffix = format!(
-                "\n\tDid you mean {}{}?",
-                Format::Good("--"),
-                Format::Good(candidate)
+    if let Some(candidate) = did_you_mean(arg, longs) {
+        let suffix = format!(
+            "\n\tDid you mean {}{}?",
+            Format::Good("--"),
+            Format::Good(candidate)
             );
-            return (suffix, Some(candidate));
-        }
-        None => for subcommand in subcommands {
+        return (suffix, Some(candidate));
+    }
+
+    subcommands
+        .into_iter()
+        .filter_map(|subcommand| {
             let opts = subcommand
                 .p
                 .flags
@@ -68,18 +71,27 @@ where
                 .filter_map(|f| f.s.long)
                 .chain(subcommand.p.opts.iter().filter_map(|o| o.s.long));
 
-            if let Some(candidate) = did_you_mean(arg, opts) {
-                let suffix = format!(
-                    "\n\tDid you mean to put '{}{}' after the subcommand '{}'?",
-                    Format::Good("--"),
-                    Format::Good(candidate),
-                    Format::Good(subcommand.get_name())
-                );
-                return (suffix, Some(candidate));
-            }
-        },
-    }
-    (String::new(), None)
+            let candidate = match did_you_mean(arg, opts) {
+                Some(candidate) => candidate,
+                None => return None
+            };
+            let score = match args_rest.iter().position(|x| *x == subcommand.get_name()) {
+                Some(score) => score,
+                None => return None
+            };
+
+            let suffix = format!(
+                "\n\tDid you mean to put '{}{}' after the subcommand '{}'?",
+                Format::Good("--"),
+                Format::Good(candidate),
+                Format::Good(subcommand.get_name())
+            );
+
+            Some((score, (suffix, Some(candidate))))
+        })
+        .min_by_key(|&(score, _)| score)
+        .map(|(_, suggestion)| suggestion)
+        .unwrap_or_else(|| (String::new(), None))
 }
 
 /// Returns a suffix that can be empty, or is the standard 'did you mean' phrase


Test patch:
diff --git a/tests/subcommands.rs b/tests/subcommands.rs
index de2f8b56..6257aaf0 100644
--- a/tests/subcommands.rs
+++ b/tests/subcommands.rs
@@ -42,15 +42,6 @@ USAGE:
 
 For more information try --help";
 
-#[cfg(feature = "suggestions")]
-static DYM_ARG: &'static str = "error: Found argument '--subcm' which wasn't expected, or isn't valid in this context
-\tDid you mean to put '--subcmdarg' after the subcommand 'subcmd'?
-
-USAGE:
-    dym [SUBCOMMAND]
-
-For more information try --help";
-
 #[test]
 fn subcommand() {
     let m = App::new("test")
@@ -137,10 +128,34 @@ fn subcmd_did_you_mean_output() {
 #[test]
 #[cfg(feature="suggestions")]
 fn subcmd_did_you_mean_output_arg() {
+    static EXPECTED: &'static str = "error: Found argument '--subcmarg' which wasn't expected, or isn't valid in this context
+\tDid you mean to put '--subcmdarg' after the subcommand 'subcmd'?
+
+USAGE:
+    dym [SUBCOMMAND]
+
+For more information try --help";
+
     let app = App::new("dym")
         .subcommand(SubCommand::with_name("subcmd")
             .arg_from_usage("-s --subcmdarg [subcmdarg] 'tests'") );
-    assert!(test::compare_output(app, "dym --subcm foo", DYM_ARG, true));
+    assert!(test::compare_output(app, "dym --subcmarg subcmd", EXPECTED, true));
+}
+
+#[test]
+#[cfg(feature="suggestions")]
+fn subcmd_did_you_mean_output_arg_false_positives() {
+    static EXPECTED: &'static str = "error: Found argument '--subcmarg' which wasn't expected, or isn't valid in this context
+
+USAGE:
+    dym [SUBCOMMAND]
+
+For more information try --help";
+
+    let app = App::new("dym")
+        .subcommand(SubCommand::with_name("subcmd")
+            .arg_from_usage("-s --subcmdarg [subcmdarg] 'tests'") );
+    assert!(test::compare_output(app, "dym --subcmarg foo", EXPECTED, true));
 }
 
 #[test]
@@ -198,4 +213,4 @@ fn issue_1031_args_with_same_name_no_more_vals() {
     let m = res.unwrap();
     assert_eq!(m.value_of("ui-path"), Some("value"));
     assert_eq!(m.subcommand_name(), Some("signer"));
-}
\ No newline at end of file
+}