Merge #2203

2203: Fix suggestions of a subcommand shadowing valid input r=pksunkara a=ldm0



Co-authored-by: Donough Liu <ldm2993593805@163.com>

src/build/app/mod.rs

@@ -163,7 +163,7 @@ impl<'help> App<'help> {
             .map(|a| a.0)
     }
 
-    /// Iterate through the *visible* short aliases for this subcommand.
+    /// Iterate through the *visible* long aliases for this subcommand.
     #[inline]
     pub fn get_visible_long_flag_aliases(&self) -> impl Iterator<Item = &'help str> + '_ {
         self.long_flag_aliases
@@ -2090,7 +2090,7 @@ impl<'help> App<'help> {
                 let p = Path::new(name);
 
                 if let Some(f) = p.file_name() {
-                    if let Some(s) = f.to_os_string().to_str() {
+                    if let Some(s) = f.to_str() {
                         if self.bin_name.is_none() {
                             self.bin_name = Some(s.to_owned());
                         }

src/output/usage.rs

@@ -146,6 +146,7 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> {
         if self.p.app.has_visible_subcommands() && incl_reqs
             || self.p.is_set(AS::AllowExternalSubcommands)
         {
+            let placeholder = self.p.app.subcommand_placeholder.unwrap_or("SUBCOMMAND");
             if self.p.is_set(AS::SubcommandsNegateReqs) || self.p.is_set(AS::ArgsNegateSubcommands)
             {
                 if !self.p.is_set(AS::ArgsNegateSubcommands) {
@@ -153,25 +154,25 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> {
                     usage.push_str(&*self.create_help_usage(false));
 
                     usage.push_str(" <");
-                    usage.push_str(self.p.app.subcommand_placeholder.unwrap_or("SUBCOMMAND"));
+                    usage.push_str(placeholder);
                     usage.push_str(">");
                 } else {
                     usage.push_str("\n    ");
                     usage.push_str(&*name);
 
                     usage.push_str(" <");
-                    usage.push_str(self.p.app.subcommand_placeholder.unwrap_or("SUBCOMMAND"));
+                    usage.push_str(placeholder);
                     usage.push_str(">");
                 }
             } else if self.p.is_set(AS::SubcommandRequired)
                 || self.p.is_set(AS::SubcommandRequiredElseHelp)
             {
                 usage.push_str(" <");
-                usage.push_str(self.p.app.subcommand_placeholder.unwrap_or("SUBCOMMAND"));
+                usage.push_str(placeholder);
                 usage.push_str(">");
             } else {
                 usage.push_str(" [");
-                usage.push_str(self.p.app.subcommand_placeholder.unwrap_or("SUBCOMMAND"));
+                usage.push_str(placeholder);
                 usage.push_str("]");
             }
         }

src/parse/parser.rs

@@ -490,31 +490,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                     // get the next value from the iterator
                     continue;
                 }
-
-                if !(self.is_set(AS::ArgsNegateSubcommands) && self.is_set(AS::ValidArgFound)
-                    || self.is_set(AS::AllowExternalSubcommands)
-                    || self.is_set(AS::InferSubcommands))
-                {
-                    let cands = suggestions::did_you_mean(
-                        &*arg_os.to_string_lossy(),
-                        self.app.all_subcommand_names(),
-                    );
-                    if !cands.is_empty() {
-                        let cands: Vec<_> =
-                            cands.iter().map(|cand| format!("'{}'", cand)).collect();
-                        return Err(ClapError::invalid_subcommand(
-                            arg_os.to_string_lossy().to_string(),
-                            cands.join(" or "),
-                            self.app
-                                .bin_name
-                                .as_ref()
-                                .unwrap_or(&self.app.name)
-                                .to_string(),
-                            Usage::new(self).create_usage_with_title(&[]),
-                            self.app.color(),
-                        ));
-                    }
-                }
             }
 
             let positional_count = self
@@ -672,54 +647,55 @@ impl<'help, 'app> Parser<'help, 'app> {
                 external_subcommand = true;
 
                 break;
-            } else if !self.has_args()
-                || self.is_set(AS::InferSubcommands) && self.has_subcommands()
-            {
-                let cands = suggestions::did_you_mean(
-                    &*arg_os.to_string_lossy(),
-                    self.app.all_subcommand_names(),
-                );
-                if !cands.is_empty() {
-                    let cands: Vec<_> = cands.iter().map(|cand| format!("'{}'", cand)).collect();
-                    return Err(ClapError::invalid_subcommand(
-                        arg_os.to_string_lossy().to_string(),
-                        cands.join(" or "),
-                        self.app
-                            .bin_name
-                            .as_ref()
-                            .unwrap_or(&self.app.name)
-                            .to_string(),
-                        Usage::new(self).create_usage_with_title(&[]),
-                        self.app.color(),
-                    ));
-                } else {
-                    return Err(ClapError::unrecognized_subcommand(
-                        arg_os.to_string_lossy().to_string(),
-                        self.app
-                            .bin_name
-                            .as_ref()
-                            .unwrap_or(&self.app.name)
-                            .to_string(),
-                        self.app.color(),
-                    ));
-                }
-            } else if self.is_set(AS::TrailingValues) {
-                // If argument followed by a `--`
-                if self.possible_subcommand(&arg_os).is_some() {
-                    return Err(ClapError::unnecessary_double_dash(
-                        arg_os.to_string_lossy().to_string(),
-                        Usage::new(self).create_usage_with_title(&[]),
-                        self.app.color(),
-                    ));
-                } else {
-                    return Err(ClapError::unknown_argument(
-                        arg_os.to_string_lossy().to_string(),
-                        None,
-                        Usage::new(self).create_usage_with_title(&[]),
-                        self.app.color(),
-                    ));
-                }
             } else {
+                // Start error processing
+
+                // If argument follows a `--`
+                if self.is_set(AS::TrailingValues) {
+                    // If the arg matches a subcommand name, or any of its aliases (if defined)
+                    if self.possible_subcommand(&arg_os).is_some() {
+                        return Err(ClapError::unnecessary_double_dash(
+                            arg_os.to_string_lossy().to_string(),
+                            Usage::new(self).create_usage_with_title(&[]),
+                            self.app.color(),
+                        ));
+                    }
+                } else {
+                    let cands = suggestions::did_you_mean(
+                        &arg_os.to_string_lossy(),
+                        self.app.all_subcommand_names(),
+                    );
+                    // If the argument looks like a subcommand.
+                    if !cands.is_empty() {
+                        let cands: Vec<_> =
+                            cands.iter().map(|cand| format!("'{}'", cand)).collect();
+                        return Err(ClapError::invalid_subcommand(
+                            arg_os.to_string_lossy().to_string(),
+                            cands.join(" or "),
+                            self.app
+                                .bin_name
+                                .as_ref()
+                                .unwrap_or(&self.app.name)
+                                .to_string(),
+                            Usage::new(self).create_usage_with_title(&[]),
+                            self.app.color(),
+                        ));
+                    }
+                    // If the argument must be a subcommand.
+                    if !self.has_args()
+                        || self.is_set(AS::InferSubcommands) && self.has_subcommands()
+                    {
+                        return Err(ClapError::unrecognized_subcommand(
+                            arg_os.to_string_lossy().to_string(),
+                            self.app
+                                .bin_name
+                                .as_ref()
+                                .unwrap_or(&self.app.name)
+                                .to_string(),
+                            self.app.color(),
+                        ));
+                    }
+                }
                 return Err(ClapError::unknown_argument(
                     arg_os.to_string_lossy().to_string(),
                     None,

tests/subcommands.rs

@@ -433,3 +433,23 @@ fn subcommand_used_after_double_dash() {
         true
     ));
 }
+
+#[test]
+fn subcommand_after_argument() {
+    let m = App::new("myprog")
+        .arg(Arg::new("some_text"))
+        .subcommand(App::new("test"))
+        .get_matches_from(vec!["myprog", "teat", "test"]);
+    assert_eq!(m.value_of("some_text"), Some("teat"));
+    assert_eq!(m.subcommand().unwrap().0, "test");
+}
+
+#[test]
+fn subcommand_after_argument_looks_like_help() {
+    let m = App::new("myprog")
+        .arg(Arg::new("some_text"))
+        .subcommand(App::new("test"))
+        .get_matches_from(vec!["myprog", "helt", "test"]);
+    assert_eq!(m.value_of("some_text"), Some("helt"));
+    assert_eq!(m.subcommand().unwrap().0, "test");
+}