From 48f087d09036a7e6c5702da214f9f5f094b80cef Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Jul 2022 17:44:49 -0500 Subject: [PATCH] fix(parser): low index multiples work with flags We had some tests for this but not sufficient obviously. The problem is we were tweaking the positional argument counter when processing flags and not just positional arguments. Delaying it until after flags seems to fix this. Fixes #3959 --- src/parser/parser.rs | 142 +++++++++++++++---------------- tests/builder/multiple_values.rs | 35 ++++++++ 2 files changed, 106 insertions(+), 71 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 2352eff6a27..ad2bc6e9ca6 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -98,77 +98,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg_os.to_value_os().as_raw_bytes() ); - // Correct pos_counter. - pos_counter = { - let is_second_to_last = pos_counter + 1 == positional_count; - - // The last positional argument, or second to last positional - // argument may be set to .multiple_values(true) or `.multiple_occurrences(true)` - let low_index_mults = is_second_to_last - && self - .cmd - .get_positionals() - .any(|a| a.is_multiple() && (positional_count != a.index.unwrap_or(0))) - && self - .cmd - .get_positionals() - .last() - .map_or(false, |p_name| !p_name.is_last_set()); - - let missing_pos = self.cmd.is_allow_missing_positional_set() - && is_second_to_last - && !trailing_values; - - debug!( - "Parser::get_matches_with: Positional counter...{}", - pos_counter - ); - debug!( - "Parser::get_matches_with: Low index multiples...{:?}", - low_index_mults - ); - - if low_index_mults || missing_pos { - let skip_current = if let Some(n) = raw_args.peek(&args_cursor) { - if let Some(arg) = self - .cmd - .get_positionals() - .find(|a| a.index == Some(pos_counter)) - { - // If next value looks like a new_arg or it's a - // subcommand, skip positional argument under current - // pos_counter(which means current value cannot be a - // positional argument with a value next to it), assume - // current value matches the next arg. - self.is_new_arg(&n, arg) - || self - .possible_subcommand(n.to_value(), valid_arg_found) - .is_some() - } else { - true - } - } else { - true - }; - - if skip_current { - debug!("Parser::get_matches_with: Bumping the positional counter..."); - pos_counter + 1 - } else { - pos_counter - } - } else if trailing_values - && (self.cmd.is_allow_missing_positional_set() || contains_last) - { - // Came to -- and one positional has .last(true) set, so we go immediately - // to the last (highest index) positional - debug!("Parser::get_matches_with: .last(true) and --, setting last pos"); - positional_count - } else { - pos_counter - } - }; - // Has the user already passed '--'? Meaning only positional args follow if !trailing_values { if self.cmd.is_subcommand_precedence_over_arg_set() @@ -376,6 +305,77 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } + // Correct pos_counter. + pos_counter = { + let is_second_to_last = pos_counter + 1 == positional_count; + + // The last positional argument, or second to last positional + // argument may be set to .multiple_values(true) or `.multiple_occurrences(true)` + let low_index_mults = is_second_to_last + && self + .cmd + .get_positionals() + .any(|a| a.is_multiple() && (positional_count != a.index.unwrap_or(0))) + && self + .cmd + .get_positionals() + .last() + .map_or(false, |p_name| !p_name.is_last_set()); + + let missing_pos = self.cmd.is_allow_missing_positional_set() + && is_second_to_last + && !trailing_values; + + debug!( + "Parser::get_matches_with: Positional counter...{}", + pos_counter + ); + debug!( + "Parser::get_matches_with: Low index multiples...{:?}", + low_index_mults + ); + + if low_index_mults || missing_pos { + let skip_current = if let Some(n) = raw_args.peek(&args_cursor) { + if let Some(arg) = self + .cmd + .get_positionals() + .find(|a| a.index == Some(pos_counter)) + { + // If next value looks like a new_arg or it's a + // subcommand, skip positional argument under current + // pos_counter(which means current value cannot be a + // positional argument with a value next to it), assume + // current value matches the next arg. + self.is_new_arg(&n, arg) + || self + .possible_subcommand(n.to_value(), valid_arg_found) + .is_some() + } else { + true + } + } else { + true + }; + + if skip_current { + debug!("Parser::get_matches_with: Bumping the positional counter..."); + pos_counter + 1 + } else { + pos_counter + } + } else if trailing_values + && (self.cmd.is_allow_missing_positional_set() || contains_last) + { + // Came to -- and one positional has .last(true) set, so we go immediately + // to the last (highest index) positional + debug!("Parser::get_matches_with: .last(true) and --, setting last pos"); + positional_count + } else { + pos_counter + } + }; + if let Some(arg) = self.cmd.get_keymap().get(&pos_counter) { if arg.is_last_set() && !trailing_values { let _ = self.resolve_pending(matcher); diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 76191773813..2715c5a032e 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -1226,6 +1226,41 @@ fn low_index_positional_with_flag() { assert!(*m.get_one::("flg").expect("defaulted by clap")); } +#[test] +fn low_index_positional_with_extra_flags() { + let cmd = Command::new("test") + .arg(Arg::new("yes").long("yes").action(ArgAction::SetTrue)) + .arg(Arg::new("one").long("one").action(ArgAction::Set)) + .arg(Arg::new("two").long("two").action(ArgAction::Set)) + .arg(Arg::new("input").multiple_values(true).required(true)) + .arg(Arg::new("output").required(true)); + let m = cmd.try_get_matches_from([ + "test", "--one", "1", "--two", "2", "3", "4", "5", "6", "7", "8", + ]); + + assert!(m.is_ok(), "{:?}", m.unwrap_err().kind()); + let m = m.unwrap(); + + assert_eq!( + m.get_many::("input") + .unwrap() + .into_iter() + .map(String::from) + .collect::>(), + vec![ + "3".to_owned(), + "4".to_owned(), + "5".to_owned(), + "6".to_owned(), + "7".to_owned() + ], + ); + assert_eq!(m.get_one::("output").unwrap(), "8"); + assert_eq!(m.get_one::("one").unwrap(), "1"); + assert_eq!(m.get_one::("two").unwrap(), "2"); + assert!(!*m.get_one::("yes").unwrap()); +} + #[test] fn multiple_value_terminator_option() { let m = Command::new("lip")