From b55ebc9f7f0364c67ef48e8cbcc308b139f1a72a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 18 Aug 2023 14:19:54 -0500 Subject: [PATCH 1/3] test(parser): Show bad Unknown bug on flags --- tests/builder/error.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/builder/error.rs b/tests/builder/error.rs index bfdf623cb5b..bd94c6bd701 100644 --- a/tests/builder/error.rs +++ b/tests/builder/error.rs @@ -225,6 +225,10 @@ fn unknown_argument_option() { ) .hide(true), ]); + + let res = cmd.clone().try_get_matches_from(["test"]); + assert!(res.is_ok()); + let res = cmd.try_get_matches_from(["test", "--cwd", ".."]); assert!(res.is_err()); let err = res.unwrap_err(); @@ -257,6 +261,10 @@ fn unknown_argument_flag() { ) .hide(true), ]); + + let res = cmd.clone().try_get_matches_from(["test"]); + assert!(res.is_err()); + let res = cmd.try_get_matches_from(["test", "--ignored"]); assert!(res.is_err()); let err = res.unwrap_err(); From 6720240577577d185b886921290323c5dc647af7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 18 Aug 2023 14:28:15 -0500 Subject: [PATCH 2/3] feat(parser): Report source to value parsers --- clap_builder/src/builder/value_parser.rs | 72 +++++++++++++++++++++++- clap_builder/src/parser/parser.rs | 20 ++++--- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/clap_builder/src/builder/value_parser.rs b/clap_builder/src/builder/value_parser.rs index ab06ae5a432..38fbb57ffd3 100644 --- a/clap_builder/src/builder/value_parser.rs +++ b/clap_builder/src/builder/value_parser.rs @@ -3,6 +3,7 @@ use std::ops::RangeBounds; use crate::builder::Str; use crate::builder::StyledStr; +use crate::parser::ValueSource; use crate::util::AnyValue; use crate::util::AnyValueId; @@ -236,8 +237,9 @@ impl ValueParser { cmd: &crate::Command, arg: Option<&crate::Arg>, value: &std::ffi::OsStr, + source: ValueSource, ) -> Result { - self.any_value_parser().parse_ref(cmd, arg, value) + self.any_value_parser().parse_ref_(cmd, arg, value, source) } /// Describes the content of `AnyValue` @@ -594,6 +596,16 @@ trait AnyValueParser: Send + Sync + 'static { value: &std::ffi::OsStr, ) -> Result; + fn parse_ref_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: &std::ffi::OsStr, + _source: ValueSource, + ) -> Result { + self.parse_ref(cmd, arg, value) + } + fn parse( &self, cmd: &crate::Command, @@ -601,6 +613,16 @@ trait AnyValueParser: Send + Sync + 'static { value: std::ffi::OsString, ) -> Result; + fn parse_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: std::ffi::OsString, + _source: ValueSource, + ) -> Result { + self.parse(cmd, arg, value) + } + /// Describes the content of `AnyValue` fn type_id(&self) -> AnyValueId; @@ -626,6 +648,17 @@ where Ok(AnyValue::new(value)) } + fn parse_ref_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: &std::ffi::OsStr, + source: ValueSource, + ) -> Result { + let value = ok!(TypedValueParser::parse_ref_(self, cmd, arg, value, source)); + Ok(AnyValue::new(value)) + } + fn parse( &self, cmd: &crate::Command, @@ -636,6 +669,17 @@ where Ok(AnyValue::new(value)) } + fn parse_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: std::ffi::OsString, + source: ValueSource, + ) -> Result { + let value = ok!(TypedValueParser::parse_(self, cmd, arg, value, source)); + Ok(AnyValue::new(value)) + } + fn type_id(&self) -> AnyValueId { AnyValueId::of::() } @@ -716,6 +760,19 @@ pub trait TypedValueParser: Clone + Send + Sync + 'static { value: &std::ffi::OsStr, ) -> Result; + /// Parse the argument value + /// + /// When `arg` is `None`, an external subcommand value is being parsed. + fn parse_ref_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: &std::ffi::OsStr, + _source: ValueSource, + ) -> Result { + self.parse_ref(cmd, arg, value) + } + /// Parse the argument value /// /// When `arg` is `None`, an external subcommand value is being parsed. @@ -728,6 +785,19 @@ pub trait TypedValueParser: Clone + Send + Sync + 'static { self.parse_ref(cmd, arg, &value) } + /// Parse the argument value + /// + /// When `arg` is `None`, an external subcommand value is being parsed. + fn parse_( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: std::ffi::OsString, + _source: ValueSource, + ) -> Result { + self.parse(cmd, arg, value) + } + /// Reflect on enumerated value properties /// /// Error checking should not be done with this; it is mostly targeted at user-facing diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index 81eddc9fd61..98b28eeb73d 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -421,7 +421,12 @@ impl<'cmd> Parser<'cmd> { sc_m.start_occurrence_of_external(self.cmd); for raw_val in raw_args.remaining(&mut args_cursor) { - let val = ok!(external_parser.parse_ref(self.cmd, None, raw_val)); + let val = ok!(external_parser.parse_ref( + self.cmd, + None, + raw_val, + ValueSource::CommandLine + )); let external_id = Id::from_static_ref(Id::EXTERNAL); sc_m.add_val_to(&external_id, val, raw_val.to_os_string()); } @@ -1032,6 +1037,7 @@ impl<'cmd> Parser<'cmd> { &self, arg: &Arg, raw_vals: Vec, + source: ValueSource, matcher: &mut ArgMatcher, ) -> ClapResult<()> { debug!("Parser::push_arg_values: {raw_vals:?}"); @@ -1044,7 +1050,7 @@ impl<'cmd> Parser<'cmd> { self.cur_idx.get() ); let value_parser = arg.get_value_parser(); - let val = ok!(value_parser.parse_ref(self.cmd, Some(arg), &raw_val)); + let val = ok!(value_parser.parse_ref(self.cmd, Some(arg), &raw_val, source)); matcher.add_val_to(arg.get_id(), val, raw_val); matcher.add_index_to(arg.get_id(), self.cur_idx.get()); @@ -1153,7 +1159,7 @@ impl<'cmd> Parser<'cmd> { )); } self.start_custom_arg(matcher, arg, source); - ok!(self.push_arg_values(arg, raw_vals, matcher)); + ok!(self.push_arg_values(arg, raw_vals, source, matcher)); if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { debug!( "Parser::react not enough values passed in, leaving it to the validator to complain", @@ -1170,7 +1176,7 @@ impl<'cmd> Parser<'cmd> { debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); } self.start_custom_arg(matcher, arg, source); - ok!(self.push_arg_values(arg, raw_vals, matcher)); + ok!(self.push_arg_values(arg, raw_vals, source, matcher)); if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { debug!( "Parser::react not enough values passed in, leaving it to the validator to complain", @@ -1196,7 +1202,7 @@ impl<'cmd> Parser<'cmd> { )); } self.start_custom_arg(matcher, arg, source); - ok!(self.push_arg_values(arg, raw_vals, matcher)); + ok!(self.push_arg_values(arg, raw_vals, source, matcher)); Ok(ParseResult::ValuesDone) } ArgAction::SetFalse => { @@ -1217,7 +1223,7 @@ impl<'cmd> Parser<'cmd> { )); } self.start_custom_arg(matcher, arg, source); - ok!(self.push_arg_values(arg, raw_vals, matcher)); + ok!(self.push_arg_values(arg, raw_vals, source, matcher)); Ok(ParseResult::ValuesDone) } ArgAction::Count => { @@ -1233,7 +1239,7 @@ impl<'cmd> Parser<'cmd> { matcher.remove(arg.get_id()); self.start_custom_arg(matcher, arg, source); - ok!(self.push_arg_values(arg, raw_vals, matcher)); + ok!(self.push_arg_values(arg, raw_vals, source, matcher)); Ok(ParseResult::ValuesDone) } ArgAction::Help => { From 56135f3ff3c2bd8c5a403e26f779cfd488ec3d33 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 18 Aug 2023 14:29:37 -0500 Subject: [PATCH 3/3] fix(builder): UnknownValueParser shouldn't error on flag absense Fixes #5079 --- clap_builder/src/builder/value_parser.rs | 65 +++++++++++++++--------- tests/builder/error.rs | 2 +- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/clap_builder/src/builder/value_parser.rs b/clap_builder/src/builder/value_parser.rs index 38fbb57ffd3..1f0ef925f08 100644 --- a/clap_builder/src/builder/value_parser.rs +++ b/clap_builder/src/builder/value_parser.rs @@ -2225,35 +2225,52 @@ impl TypedValueParser for UnknownArgumentValueParser { type Value = String; fn parse_ref( + &self, + cmd: &crate::Command, + arg: Option<&crate::Arg>, + value: &std::ffi::OsStr, + ) -> Result { + TypedValueParser::parse_ref_(self, cmd, arg, value, ValueSource::CommandLine) + } + + fn parse_ref_( &self, cmd: &crate::Command, arg: Option<&crate::Arg>, _value: &std::ffi::OsStr, + source: ValueSource, ) -> Result { - let arg = match arg { - Some(arg) => arg.to_string(), - None => "..".to_owned(), - }; - let err = crate::Error::unknown_argument( - cmd, - arg, - self.arg.as_ref().map(|s| (s.as_str().to_owned(), None)), - false, - crate::output::Usage::new(cmd).create_usage_with_title(&[]), - ); - #[cfg(feature = "error-context")] - let err = { - debug_assert_eq!( - err.get(crate::error::ContextKind::Suggested), - None, - "Assuming `Error::unknown_argument` doesn't apply any `Suggested` so we can without caution" - ); - err.insert_context_unchecked( - crate::error::ContextKind::Suggested, - crate::error::ContextValue::StyledStrs(self.suggestions.clone()), - ) - }; - Err(err) + match source { + ValueSource::DefaultValue => { + TypedValueParser::parse_ref_(&StringValueParser::new(), cmd, arg, _value, source) + } + ValueSource::EnvVariable | ValueSource::CommandLine => { + let arg = match arg { + Some(arg) => arg.to_string(), + None => "..".to_owned(), + }; + let err = crate::Error::unknown_argument( + cmd, + arg, + self.arg.as_ref().map(|s| (s.as_str().to_owned(), None)), + false, + crate::output::Usage::new(cmd).create_usage_with_title(&[]), + ); + #[cfg(feature = "error-context")] + let err = { + debug_assert_eq!( + err.get(crate::error::ContextKind::Suggested), + None, + "Assuming `Error::unknown_argument` doesn't apply any `Suggested` so we can without caution" + ); + err.insert_context_unchecked( + crate::error::ContextKind::Suggested, + crate::error::ContextValue::StyledStrs(self.suggestions.clone()), + ) + }; + Err(err) + } + } } } diff --git a/tests/builder/error.rs b/tests/builder/error.rs index bd94c6bd701..833a614d265 100644 --- a/tests/builder/error.rs +++ b/tests/builder/error.rs @@ -263,7 +263,7 @@ fn unknown_argument_flag() { ]); let res = cmd.clone().try_get_matches_from(["test"]); - assert!(res.is_err()); + assert!(res.is_ok()); let res = cmd.try_get_matches_from(["test", "--ignored"]); assert!(res.is_err());