From 2d7874948f303d979c20fbb4e184c8b50dfaab15 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Sep 2022 16:21:08 -0500 Subject: [PATCH 1/2] fix(parser): SetFalse should also not allow self-override --- src/builder/action.rs | 2 +- src/parser/parser.rs | 2 +- tests/builder/action.rs | 7 +++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index dc7b79a7437..01d1f07bc1b 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -169,7 +169,7 @@ pub enum ArgAction { /// .action(clap::ArgAction::SetFalse) /// ); /// - /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); + /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap(); /// assert!(matches.contains_id("flag")); /// assert_eq!( /// matches.get_one::("flag").copied(), diff --git a/src/parser/parser.rs b/src/parser/parser.rs index fb43923f8c5..24550db27cc 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1240,7 +1240,7 @@ impl<'cmd> Parser<'cmd> { raw_vals }; - if matcher.remove(&arg.id) && self.cmd.is_args_override_self() { + if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() { return Err(ClapError::argument_conflict( self.cmd, arg.to_string(), diff --git a/tests/builder/action.rs b/tests/builder/action.rs index d9b24873b85..4c5bf8106cd 100644 --- a/tests/builder/action.rs +++ b/tests/builder/action.rs @@ -240,8 +240,15 @@ fn set_false() { assert_eq!(matches.contains_id("mammal"), true); assert_eq!(matches.index_of("mammal"), Some(1)); + let result = cmd + .clone() + .try_get_matches_from(["test", "--mammal", "--mammal"]); + let err = result.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::ArgumentConflict); + let matches = cmd .clone() + .args_override_self(true) .try_get_matches_from(["test", "--mammal", "--mammal"]) .unwrap(); assert_eq!(*matches.get_one::("mammal").unwrap(), false); From 3683e2c791f8b8fcfd25a958f2fc5eebde4a52ed Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 28 Sep 2022 16:10:02 -0500 Subject: [PATCH 2/2] fix(parser): Allow one-off self-overrides bat needed this. See also #4261 --- src/builder/debug_asserts.rs | 5 ----- src/parser/parser.rs | 12 +++++++++--- tests/builder/posix_compatible.rs | 26 +++++++++++++++++++++++--- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index e40d9369e97..684e5a31d55 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -686,11 +686,6 @@ fn assert_arg(arg: &Arg) { "Argument '{}' cannot conflict with itself", arg.get_id(), ); - assert!( - !arg.overrides.iter().any(|x| *x == arg.id), - "Argument '{}' cannot override itself, its the default", - arg.get_id(), - ); assert_eq!( arg.get_action().takes_values(), diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 24550db27cc..319d25c187e 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1180,7 +1180,9 @@ impl<'cmd> Parser<'cmd> { self.cur_idx.set(self.cur_idx.get() + 1); debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); } - if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() { + if matcher.remove(&arg.id) + && !(self.cmd.is_args_override_self() || arg.overrides.contains(arg.get_id())) + { return Err(ClapError::argument_conflict( self.cmd, arg.to_string(), @@ -1221,7 +1223,9 @@ impl<'cmd> Parser<'cmd> { raw_vals }; - if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() { + if matcher.remove(&arg.id) + && !(self.cmd.is_args_override_self() || arg.overrides.contains(arg.get_id())) + { return Err(ClapError::argument_conflict( self.cmd, arg.to_string(), @@ -1240,7 +1244,9 @@ impl<'cmd> Parser<'cmd> { raw_vals }; - if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() { + if matcher.remove(&arg.id) + && !(self.cmd.is_args_override_self() || arg.overrides.contains(arg.get_id())) + { return Err(ClapError::argument_conflict( self.cmd, arg.to_string(), diff --git a/tests/builder/posix_compatible.rs b/tests/builder/posix_compatible.rs index c6e202aa8a8..f5e7739da98 100644 --- a/tests/builder/posix_compatible.rs +++ b/tests/builder/posix_compatible.rs @@ -1,16 +1,36 @@ use clap::{arg, error::ErrorKind, Arg, ArgAction, Command}; #[test] -#[should_panic = "Argument 'flag' cannot override itself"] fn flag_overrides_itself() { - Command::new("posix") + let res = Command::new("posix") .arg( arg!(--flag "some flag" ) .action(ArgAction::SetTrue) .overrides_with("flag"), ) - .build(); + .try_get_matches_from(vec!["", "--flag", "--flag"]); + assert!(res.is_ok(), "{}", res.unwrap_err()); + let m = res.unwrap(); + assert!(*m.get_one::("flag").expect("defaulted by clap")); +} + +#[test] +fn option_overrides_itself() { + let res = Command::new("posix") + .arg( + arg!(--opt "some option") + .required(false) + .overrides_with("opt"), + ) + .try_get_matches_from(vec!["", "--opt=some", "--opt=other"]); + assert!(res.is_ok(), "{}", res.unwrap_err()); + let m = res.unwrap(); + assert!(m.contains_id("opt")); + assert_eq!( + m.get_one::("opt").map(|v| v.as_str()), + Some("other") + ); } #[test]