From 811acc45c1a0fb8212fbcf44cf369cd19f0b7a3c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 08:45:03 -0600 Subject: [PATCH 01/10] test(parser): Group subcommandc conflict tests --- tests/builder/conflicts.rs | 78 +++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 00011844136..7e76b2a552c 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -684,6 +684,45 @@ fn exclusive_with_required() { cmd.clone().try_get_matches_from(["bug", "--test"]).unwrap(); } +#[test] +#[cfg(feature = "error-context")] +fn subcommand_conflict_error_message() { + static CONFLICT_ERR: &str = "\ +error: unexpected argument 'sub1' found + +Usage: test [OPTIONS] + test + +For more information, try '--help'. +"; + + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .arg(arg!(-p --place <"place id"> "Place ID to open")) + .subcommand( + Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))), + ); + + utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true); +} + +#[test] +fn subcommand_conflict_negates_required() { + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .subcommand(Command::new("config")) + .arg(arg!(-p --place <"place id"> "Place ID to open").required(true)); + + let result = cmd.try_get_matches_from(["test", "config"]); + assert!( + result.is_ok(), + "args_conflicts_with_subcommands should ignore required: {}", + result.unwrap_err() + ); + let m = result.unwrap(); + assert_eq!(m.subcommand_name().unwrap(), "config"); +} + #[test] fn args_negate_subcommands_one_level() { let res = Command::new("disablehelp") @@ -729,42 +768,3 @@ fn args_negate_subcommands_two_levels() { Some("sub2") ); } - -#[test] -#[cfg(feature = "error-context")] -fn subcommand_conflict_error_message() { - static CONFLICT_ERR: &str = "\ -error: unexpected argument 'sub1' found - -Usage: test [OPTIONS] - test - -For more information, try '--help'. -"; - - let cmd = Command::new("test") - .args_conflicts_with_subcommands(true) - .arg(arg!(-p --place <"place id"> "Place ID to open")) - .subcommand( - Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))), - ); - - utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true); -} - -#[test] -fn subcommand_conflict_negates_required() { - let cmd = Command::new("test") - .args_conflicts_with_subcommands(true) - .subcommand(Command::new("config")) - .arg(arg!(-p --place <"place id"> "Place ID to open").required(true)); - - let result = cmd.try_get_matches_from(["test", "config"]); - assert!( - result.is_ok(), - "args_conflicts_with_subcommands should ignore required: {}", - result.unwrap_err() - ); - let m = result.unwrap(); - assert_eq!(m.subcommand_name().unwrap(), "config"); -} From 11fd6ca269b67956344389aede834bb8d60c72e1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 08:45:40 -0600 Subject: [PATCH 02/10] test(parser): Clarify test name --- tests/builder/conflicts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 7e76b2a552c..9d7827921cc 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -686,7 +686,7 @@ fn exclusive_with_required() { #[test] #[cfg(feature = "error-context")] -fn subcommand_conflict_error_message() { +fn option_conflicts_with_subcommand() { static CONFLICT_ERR: &str = "\ error: unexpected argument 'sub1' found From 70da3a8608a3103ef68df7b079246141f2624842 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 08:46:47 -0600 Subject: [PATCH 03/10] test(parser): Reduce size of test --- tests/builder/conflicts.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 9d7827921cc..d623f8c9508 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -699,9 +699,7 @@ For more information, try '--help'. let cmd = Command::new("test") .args_conflicts_with_subcommands(true) .arg(arg!(-p --place <"place id"> "Place ID to open")) - .subcommand( - Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))), - ); + .subcommand(Command::new("sub1")); utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true); } From 06bff1c95503536953cb6ddb308f52d15b6afe57 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 08:48:23 -0600 Subject: [PATCH 04/10] test(parser): Check subcommands conflict with positionals --- tests/builder/conflicts.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index d623f8c9508..77f9d5cae17 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -704,6 +704,26 @@ For more information, try '--help'. utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true); } +#[test] +#[cfg(feature = "error-context")] +fn positional_conflicts_with_subcommand() { + static CONFLICT_ERR: &str = "\ +error: unexpected argument 'sub1' found + +Usage: test + test + +For more information, try '--help'. +"; + + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .arg(arg!( "some arg")) + .subcommand(Command::new("sub1")); + + utils::assert_output(cmd, "test value1 sub1", CONFLICT_ERR, true); +} + #[test] fn subcommand_conflict_negates_required() { let cmd = Command::new("test") From e2b18f199f77d18b272732907c1fe991e20ec1c2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 10 Jan 2024 20:14:59 -0600 Subject: [PATCH 05/10] test(parser): Show flag behavior --- tests/builder/conflicts.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 77f9d5cae17..799dcd3d0dd 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -724,6 +724,30 @@ For more information, try '--help'. utils::assert_output(cmd, "test value1 sub1", CONFLICT_ERR, true); } +#[test] +#[cfg(feature = "error-context")] +fn flag_conflicts_with_subcommand_long_flag() { + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .arg(arg!(--hello)) + .subcommand(Command::new("sub").long_flag("sub")); + + let res = cmd.try_get_matches_from(vec!["", "--hello", "--sub"]); + assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind()); +} + +#[test] +#[cfg(feature = "error-context")] +fn flag_conflicts_with_subcommand_short_flag() { + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .arg(arg!(--hello)) + .subcommand(Command::new("sub").short_flag('s')); + + let res = cmd.try_get_matches_from(vec!["", "--hello", "-s"]); + assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind()); +} + #[test] fn subcommand_conflict_negates_required() { let cmd = Command::new("test") From 69c0509198b3fdc1da54271979cb642903efc2c7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 09:14:37 -0600 Subject: [PATCH 06/10] test(parser): Verify conflicts with precedence --- tests/builder/conflicts.rs | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 799dcd3d0dd..72d9a986250 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -748,6 +748,48 @@ fn flag_conflicts_with_subcommand_short_flag() { assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind()); } +#[test] +#[cfg(feature = "error-context")] +fn positional_conflicts_with_subcommand_precedent() { + static CONFLICT_ERR: &str = "\ +error: unexpected argument 'sub' found + +Usage: test + test + +For more information, try '--help'. +"; + + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .subcommand_precedence_over_arg(true) + .arg(arg!( "some arg")) + .subcommand(Command::new("sub")); + + utils::assert_output(cmd, "test hello sub", CONFLICT_ERR, true); +} + +#[test] +#[cfg(feature = "error-context")] +fn flag_conflicts_with_subcommand_precedent() { + static CONFLICT_ERR: &str = "\ +error: unexpected argument 'sub' found + +Usage: test [OPTIONS] + test + +For more information, try '--help'. +"; + + let cmd = Command::new("test") + .args_conflicts_with_subcommands(true) + .subcommand_precedence_over_arg(true) + .arg(arg!(--hello)) + .subcommand(Command::new("sub")); + + utils::assert_output(cmd, "test --hello sub", CONFLICT_ERR, true); +} + #[test] fn subcommand_conflict_negates_required() { let cmd = Command::new("test") From e47d8a2a660c97b2f37369e503e781156527304f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 09:47:00 -0600 Subject: [PATCH 07/10] refactor(parser): Clarify arg error --- clap_builder/src/parser/parser.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index 93616d68a64..d24d37ade63 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -490,7 +490,17 @@ impl<'cmd> Parser<'cmd> { && self.cmd.has_positionals() && (arg_os.is_long() || arg_os.is_short()); - if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found) { + if self.cmd.has_subcommands() { + if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found { + return ClapError::unknown_argument( + self.cmd, + arg_os.display().to_string(), + None, + suggested_trailing_arg, + Usage::new(self.cmd).create_usage_with_title(&[]), + ); + } + let candidates = suggestions::did_you_mean( &arg_os.display().to_string(), self.cmd.all_subcommand_names(), @@ -508,9 +518,7 @@ impl<'cmd> Parser<'cmd> { } // If the argument must be a subcommand. - if self.cmd.has_subcommands() - && (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set()) - { + if !self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set() { return ClapError::unrecognized_subcommand( self.cmd, arg_os.display().to_string(), From ea00ef3051fd5a993b766062a4e9defc777bf233 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 10:17:35 -0600 Subject: [PATCH 08/10] refactor(error): Allow more conflict sources --- clap_builder/src/error/format.rs | 53 ++++++++++++++++---------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/clap_builder/src/error/format.rs b/clap_builder/src/error/format.rs index 49e617d071f..3b7dfb434a3 100644 --- a/clap_builder/src/error/format.rs +++ b/clap_builder/src/error/format.rs @@ -146,12 +146,10 @@ fn write_dynamic_context( match error.kind() { ErrorKind::ArgumentConflict => { - let invalid_arg = error.get(ContextKind::InvalidArg); - let prior_arg = error.get(ContextKind::PriorArg); - if let (Some(ContextValue::String(invalid_arg)), Some(prior_arg)) = - (invalid_arg, prior_arg) - { - if ContextValue::String(invalid_arg.clone()) == *prior_arg { + let mut prior_arg = error.get(ContextKind::PriorArg); + if let Some(ContextValue::String(invalid_arg)) = error.get(ContextKind::InvalidArg) { + if Some(&ContextValue::String(invalid_arg.clone())) == prior_arg { + prior_arg = None; let _ = write!( styled, "the argument '{}{invalid_arg}{}' cannot be used multiple times", @@ -165,36 +163,39 @@ fn write_dynamic_context( invalid.render(), invalid.render_reset() ); + } + } else { + styled.push_str(error.kind().as_str().unwrap()); + } - match prior_arg { - ContextValue::Strings(values) => { - styled.push_str(":"); - for v in values { - let _ = write!( - styled, - "\n{TAB}{}{v}{}", - invalid.render(), - invalid.render_reset() - ); - } - } - ContextValue::String(value) => { + if let Some(prior_arg) = prior_arg { + match prior_arg { + ContextValue::Strings(values) => { + styled.push_str(":"); + for v in values { let _ = write!( styled, - " '{}{value}{}'", + "\n{TAB}{}{v}{}", invalid.render(), invalid.render_reset() ); } - _ => { - styled.push_str(" one or more of the other specified arguments"); - } + } + ContextValue::String(value) => { + let _ = write!( + styled, + " '{}{value}{}'", + invalid.render(), + invalid.render_reset() + ); + } + _ => { + styled.push_str(" one or more of the other specified arguments"); } } - true - } else { - false } + + true } ErrorKind::NoEquals => { let invalid_arg = error.get(ContextKind::InvalidArg); From a7e04a53e4a9e310663a0cdb5bfb0753d84b82bd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 10:20:19 -0600 Subject: [PATCH 09/10] fix(parser): Improve subcommand conflict error --- clap_builder/src/error/format.rs | 9 +++++++++ clap_builder/src/error/mod.rs | 28 ++++++++++++++++++++++++++++ clap_builder/src/parser/parser.rs | 16 ++++++++++++---- tests/builder/conflicts.rs | 8 ++++---- 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/clap_builder/src/error/format.rs b/clap_builder/src/error/format.rs index 3b7dfb434a3..2895a39f691 100644 --- a/clap_builder/src/error/format.rs +++ b/clap_builder/src/error/format.rs @@ -164,6 +164,15 @@ fn write_dynamic_context( invalid.render_reset() ); } + } else if let Some(ContextValue::String(invalid_arg)) = + error.get(ContextKind::InvalidSubcommand) + { + let _ = write!( + styled, + "the subcommand '{}{invalid_arg}{}' cannot be used with", + invalid.render(), + invalid.render_reset() + ); } else { styled.push_str(error.kind().as_str().unwrap()); } diff --git a/clap_builder/src/error/mod.rs b/clap_builder/src/error/mod.rs index af90e2756f1..c0be1aa69e3 100644 --- a/clap_builder/src/error/mod.rs +++ b/clap_builder/src/error/mod.rs @@ -391,6 +391,34 @@ impl Error { err } + pub(crate) fn subcommand_conflict( + cmd: &Command, + sub: String, + mut others: Vec, + usage: Option, + ) -> Self { + let mut err = Self::new(ErrorKind::ArgumentConflict).with_cmd(cmd); + + #[cfg(feature = "error-context")] + { + let others = match others.len() { + 0 => ContextValue::None, + 1 => ContextValue::String(others.pop().unwrap()), + _ => ContextValue::Strings(others), + }; + err = err.extend_context_unchecked([ + (ContextKind::InvalidSubcommand, ContextValue::String(sub)), + (ContextKind::PriorArg, others), + ]); + if let Some(usage) = usage { + err = err + .insert_context_unchecked(ContextKind::Usage, ContextValue::StyledStr(usage)); + } + } + + err + } + pub(crate) fn empty_value(cmd: &Command, good_vals: &[String], arg: String) -> Self { Self::invalid_value(cmd, "".to_owned(), good_vals, arg) } diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index d24d37ade63..3be08ce81ef 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -444,7 +444,12 @@ impl<'cmd> Parser<'cmd> { } else { // Start error processing let _ = self.resolve_pending(matcher); - return Err(self.match_arg_error(&arg_os, valid_arg_found, trailing_values)); + return Err(self.match_arg_error( + &arg_os, + valid_arg_found, + trailing_values, + matcher, + )); } } @@ -470,6 +475,7 @@ impl<'cmd> Parser<'cmd> { arg_os: &clap_lex::ParsedArg<'_>, valid_arg_found: bool, trailing_values: bool, + matcher: &ArgMatcher, ) -> ClapError { // If argument follows a `--` if trailing_values { @@ -492,11 +498,13 @@ impl<'cmd> Parser<'cmd> { if self.cmd.has_subcommands() { if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found { - return ClapError::unknown_argument( + return ClapError::subcommand_conflict( self.cmd, arg_os.display().to_string(), - None, - suggested_trailing_arg, + matcher + .arg_ids() + .map(|id| self.cmd.find(id).unwrap().to_string()) + .collect(), Usage::new(self.cmd).create_usage_with_title(&[]), ); } diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 72d9a986250..386b82bc31f 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -688,7 +688,7 @@ fn exclusive_with_required() { #[cfg(feature = "error-context")] fn option_conflicts_with_subcommand() { static CONFLICT_ERR: &str = "\ -error: unexpected argument 'sub1' found +error: the subcommand 'sub1' cannot be used with '--place ' Usage: test [OPTIONS] test @@ -708,7 +708,7 @@ For more information, try '--help'. #[cfg(feature = "error-context")] fn positional_conflicts_with_subcommand() { static CONFLICT_ERR: &str = "\ -error: unexpected argument 'sub1' found +error: the subcommand 'sub1' cannot be used with '' Usage: test test @@ -752,7 +752,7 @@ fn flag_conflicts_with_subcommand_short_flag() { #[cfg(feature = "error-context")] fn positional_conflicts_with_subcommand_precedent() { static CONFLICT_ERR: &str = "\ -error: unexpected argument 'sub' found +error: the subcommand 'sub' cannot be used with '' Usage: test test @@ -773,7 +773,7 @@ For more information, try '--help'. #[cfg(feature = "error-context")] fn flag_conflicts_with_subcommand_precedent() { static CONFLICT_ERR: &str = "\ -error: unexpected argument 'sub' found +error: the subcommand 'sub' cannot be used with '--hello' Usage: test [OPTIONS] test From f529ec398c4262296d5e6eb063b1b52d875d7f03 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 11 Jan 2024 10:25:57 -0600 Subject: [PATCH 10/10] fix(parser): Ensure subcommand flags can conflict Fixes #5297 --- clap_builder/src/parser/parser.rs | 11 +++++++++++ tests/builder/conflicts.rs | 24 ++++++++++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index 3be08ce81ef..e840efc9566 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -454,6 +454,17 @@ impl<'cmd> Parser<'cmd> { } if let Some(ref pos_sc_name) = subcmd_name { + if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found { + return Err(ClapError::subcommand_conflict( + self.cmd, + pos_sc_name.clone(), + matcher + .arg_ids() + .map(|id| self.cmd.find(id).unwrap().to_string()) + .collect(), + Usage::new(self.cmd).create_usage_with_title(&[]), + )); + } let sc_name = self .cmd .find_subcommand(pos_sc_name) diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 386b82bc31f..6e3cd08ddac 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -727,25 +727,41 @@ For more information, try '--help'. #[test] #[cfg(feature = "error-context")] fn flag_conflicts_with_subcommand_long_flag() { + static CONFLICT_ERR: &str = "\ +error: the subcommand 'sub' cannot be used with '--hello' + +Usage: test [OPTIONS] + test + +For more information, try '--help'. +"; + let cmd = Command::new("test") .args_conflicts_with_subcommands(true) .arg(arg!(--hello)) .subcommand(Command::new("sub").long_flag("sub")); - let res = cmd.try_get_matches_from(vec!["", "--hello", "--sub"]); - assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind()); + utils::assert_output(cmd, "test --hello --sub", CONFLICT_ERR, true); } #[test] #[cfg(feature = "error-context")] fn flag_conflicts_with_subcommand_short_flag() { + static CONFLICT_ERR: &str = "\ +error: the subcommand 'sub' cannot be used with '--hello' + +Usage: test [OPTIONS] + test + +For more information, try '--help'. +"; + let cmd = Command::new("test") .args_conflicts_with_subcommands(true) .arg(arg!(--hello)) .subcommand(Command::new("sub").short_flag('s')); - let res = cmd.try_get_matches_from(vec!["", "--hello", "-s"]); - assert!(res.is_ok(), "error: {:?}", res.unwrap_err().kind()); + utils::assert_output(cmd, "test --hello -s", CONFLICT_ERR, true); } #[test]