From 03f132129bb3dea7c851c66afd73f892925d3921 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 6 May 2022 11:56:30 -0500 Subject: [PATCH 1/4] fix(parser): Always put in arg `""` for external subcommands (unstable) This allows distinguishing external subcommands from built-in subcommands which can especially be confusing when escaping subcommands. Fixes #3263 --- src/parse/arg_matcher.rs | 14 ++++++++++++ src/parse/parser.rs | 22 +++++++++++++------ tests/builder/app_settings.rs | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs index 323fb5b9abc..bda95eb4b5c 100644 --- a/src/parse/arg_matcher.rs +++ b/src/parse/arg_matcher.rs @@ -85,6 +85,7 @@ impl ArgMatcher { } } + #[cfg(not(feature = "unstable-v4"))] pub(crate) fn get_mut(&mut self, arg: &Id) -> Option<&mut MatchedArg> { self.0.args.get_mut(arg) } @@ -142,6 +143,19 @@ impl ArgMatcher { ma.inc_occurrences(); } + #[cfg(feature = "unstable-v4")] + pub(crate) fn inc_occurrence_of_external(&mut self, allow_invalid_utf8: bool) { + let id = &Id::empty_hash(); + debug!( + "ArgMatcher::inc_occurrence_of_external: id={:?}, allow_invalid_utf8={}", + id, allow_invalid_utf8 + ); + let ma = self.entry(id).or_insert(MatchedArg::new()); + ma.update_ty(ValueSource::CommandLine); + ma.invalid_utf8_allowed(allow_invalid_utf8); + ma.inc_occurrences(); + } + pub(crate) fn add_val_to(&mut self, arg: &Id, val: OsString, ty: ValueSource, append: bool) { if append { self.append_val_to(arg, val, ty); diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 5ca9cfe8d0d..6262d18d887 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -428,26 +428,34 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Collect the external subcommand args let mut sc_m = ArgMatcher::new(self.cmd); + let allow_invalid_utf8 = self + .cmd + .is_allow_invalid_utf8_for_external_subcommands_set(); + #[cfg(feature = "unstable-v4")] + { + sc_m.inc_occurrence_of_external(allow_invalid_utf8); + } for v in raw_args.remaining(&mut args_cursor) { - let allow_invalid_utf8 = self - .cmd - .is_allow_invalid_utf8_for_external_subcommands_set(); if !allow_invalid_utf8 && v.to_str().is_none() { return Err(ClapError::invalid_utf8( self.cmd, Usage::new(self.cmd).create_usage_with_title(&[]), )); } + let external_id = &Id::empty_hash(); sc_m.add_val_to( - &Id::empty_hash(), + external_id, v.to_os_string(), ValueSource::CommandLine, false, ); - sc_m.get_mut(&Id::empty_hash()) - .expect("just inserted") - .invalid_utf8_allowed(allow_invalid_utf8); + #[cfg(not(feature = "unstable-v4"))] + { + sc_m.get_mut(external_id) + .expect("just inserted") + .invalid_utf8_allowed(allow_invalid_utf8); + } } matcher.subcommand(SubCommand { diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index 3013406b816..50d305afd91 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -851,6 +851,46 @@ fn issue_1093_allow_ext_sc() { utils::assert_output(cmd, "clap-test --help", ALLOW_EXT_SC, false); } +#[test] +#[cfg(not(feature = "unstable-v4"))] +fn allow_ext_sc_empty_args() { + let res = Command::new("clap-test") + .version("v1.4.8") + .allow_external_subcommands(true) + .allow_invalid_utf8_for_external_subcommands(true) + .try_get_matches_from(vec!["clap-test", "external-cmd"]); + + assert!(res.is_ok(), "{}", res.unwrap_err()); + + match res.unwrap().subcommand() { + Some((name, args)) => { + assert_eq!(name, "external-cmd"); + assert_eq!(args.values_of_lossy(""), None); + } + _ => unreachable!(), + } +} + +#[test] +#[cfg(feature = "unstable-v4")] +fn allow_ext_sc_empty_args() { + let res = Command::new("clap-test") + .version("v1.4.8") + .allow_external_subcommands(true) + .allow_invalid_utf8_for_external_subcommands(true) + .try_get_matches_from(vec!["clap-test", "external-cmd"]); + + assert!(res.is_ok(), "{}", res.unwrap_err()); + + match res.unwrap().subcommand() { + Some((name, args)) => { + assert_eq!(name, "external-cmd"); + assert_eq!(args.values_of_lossy(""), Some(vec![])); + } + _ => unreachable!(), + } +} + #[test] fn allow_ext_sc_when_sc_required() { let res = Command::new("clap-test") From 20ff4ce05a7bcd9d3bd5271c954fa75251312ff0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 6 May 2022 12:38:23 -0500 Subject: [PATCH 2/4] doc(parser): Document external subcommand escaping behavior --- src/build/command.rs | 5 +++++ tests/builder/app_settings.rs | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/build/command.rs b/src/build/command.rs index 4ba7c844727..1ba2629109b 100644 --- a/src/build/command.rs +++ b/src/build/command.rs @@ -2709,11 +2709,16 @@ impl<'help> App<'help> { /// Assume unexpected positional arguments are a [`subcommand`]. /// + /// Arguments will be stored in the `""` argument in the [`ArgMatches`] + /// /// **NOTE:** Use this setting with caution, /// as a truly unexpected argument (i.e. one that is *NOT* an external subcommand) /// will **not** cause an error and instead be treated as a potential subcommand. /// One should check for such cases manually and inform the user appropriately. /// + /// **NOTE:** A built-in subcommand will be parsed as an external subcommand when escaped with + /// `--`. + /// /// # Examples /// /// ```rust diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index 50d305afd91..20c5109b140 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -930,6 +930,25 @@ fn external_subcommand_looks_like_built_in() { } } +#[test] +fn built_in_subcommand_escaped() { + let res = Command::new("cargo") + .version("1.26.0") + .allow_external_subcommands(true) + .allow_invalid_utf8_for_external_subcommands(true) + .subcommand(Command::new("install")) + .try_get_matches_from(vec!["cargo", "--", "install", "foo"]); + assert!(res.is_ok(), "{}", res.unwrap_err()); + let m = res.unwrap(); + match m.subcommand() { + Some((name, args)) => { + assert_eq!(name, "install"); + assert_eq!(args.values_of_lossy(""), Some(vec!["foo".to_string()])); + } + _ => panic!("external_subcommand didn't work"), + } +} + #[test] fn aaos_flags() { // flags From 65538e21a87bb90d51d10ab4a1f59c259b3554ce Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 6 May 2022 15:46:34 -0500 Subject: [PATCH 3/4] fix(derive): Detect escaped ambiguous subcommands (unstable) --- clap_derive/src/derives/subcommand.rs | 14 +++++-- tests/derive/subcommands.rs | 56 +++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index f8aa52c860f..7d123479857 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -484,9 +484,17 @@ fn gen_from_arg_matches( Unnamed(..) => abort_call_site!("{}: tuple enums are not supported", variant.ident), }; - quote! { - if #sub_name == #subcommand_name_var { - return ::std::result::Result::Ok(#name :: #variant_name #constructor_block) + if cfg!(feature = "unstable-v4") { + quote! { + if #sub_name == #subcommand_name_var && !#sub_arg_matches_var.is_present("") { + return ::std::result::Result::Ok(#name :: #variant_name #constructor_block) + } + } + } else { + quote! { + if #sub_name == #subcommand_name_var { + return ::std::result::Result::Ok(#name :: #variant_name #constructor_block) + } } } }); diff --git a/tests/derive/subcommands.rs b/tests/derive/subcommands.rs index 0986df574cf..7dad15f210b 100644 --- a/tests/derive/subcommands.rs +++ b/tests/derive/subcommands.rs @@ -547,3 +547,59 @@ fn skip_subcommand() { let res = Opt::try_parse_from(&["test", "skip"]); assert_eq!(res.unwrap_err().kind(), clap::ErrorKind::UnknownArgument,); } + +#[test] +#[cfg(feature = "unstable-v4")] +fn built_in_subcommand_escaped() { + #[derive(Debug, PartialEq, Parser)] + enum Command { + Install { + arg: Option, + }, + #[clap(external_subcommand)] + Custom(Vec), + } + + assert_eq!( + Command::try_parse_from(&["test", "install", "arg"]).unwrap(), + Command::Install { + arg: Some(String::from("arg")) + } + ); + assert_eq!( + Command::try_parse_from(&["test", "--", "install"]).unwrap(), + Command::Custom(vec![String::from("install")]) + ); + assert_eq!( + Command::try_parse_from(&["test", "--", "install", "arg"]).unwrap(), + Command::Custom(vec![String::from("install"), String::from("arg")]) + ); +} + +#[test] +#[cfg(not(feature = "unstable-v4"))] +fn built_in_subcommand_escaped() { + #[derive(Debug, PartialEq, Parser)] + enum Command { + Install { + arg: Option, + }, + #[clap(external_subcommand)] + Custom(Vec), + } + + assert_eq!( + Command::try_parse_from(&["test", "install", "arg"]).unwrap(), + Command::Install { + arg: Some(String::from("arg")) + } + ); + assert_eq!( + Command::try_parse_from(&["test", "--", "install"]).unwrap(), + Command::Install { arg: None } + ); + assert_eq!( + Command::try_parse_from(&["test", "--", "install", "arg"]).unwrap(), + Command::Install { arg: None } + ); +} From eb155786bbd3447594cba5c26c83093f4b5709e7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 6 May 2022 15:49:33 -0500 Subject: [PATCH 4/4] fix(parser): Assert on unknown args when using external subcommands (unstable) --- src/parse/arg_matcher.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs index bda95eb4b5c..31804a46fcc 100644 --- a/src/parse/arg_matcher.rs +++ b/src/parse/arg_matcher.rs @@ -30,7 +30,11 @@ impl ArgMatcher { // // See clap-rs/clap#3263 #[cfg(debug_assertions)] + #[cfg(not(feature = "unstable-v4"))] disable_asserts: _cmd.is_allow_external_subcommands_set(), + #[cfg(debug_assertions)] + #[cfg(feature = "unstable-v4")] + disable_asserts: false, ..Default::default() }) }