Skip to content

Commit

Permalink
fix(parser): low index multiples work with flags
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@epage
epage committed Jul 20, 2022
1 parent 5e02445 commit 48f087d
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 71 deletions.
142 changes: 71 additions & 71 deletions src/parser/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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);
Expand Down
35 changes: 35 additions & 0 deletions tests/builder/multiple_values.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,41 @@ fn low_index_positional_with_flag() {
assert!(*m.get_one::<bool>("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::<String>("input")
.unwrap()
.into_iter()
.map(String::from)
.collect::<Vec<_>>(),
vec![
"3".to_owned(),
"4".to_owned(),
"5".to_owned(),
"6".to_owned(),
"7".to_owned()
],
);
assert_eq!(m.get_one::<String>("output").unwrap(), "8");
assert_eq!(m.get_one::<String>("one").unwrap(), "1");
assert_eq!(m.get_one::<String>("two").unwrap(), "2");
assert!(!*m.get_one::<bool>("yes").unwrap());
}

#[test]
fn multiple_value_terminator_option() {
let m = Command::new("lip")
Expand Down

0 comments on commit 48f087d

Please sign in to comment.