Skip to content

Commit

Permalink
fix: Run more parse asserts during build
Browse files Browse the repository at this point in the history 
This will help prevent issues from being deployed to users like in clap-rs#3281

I do not consider this a breaking change because any normal operation
will assert anyways.
  • Loading branch information
@epage
epage committed Jan 12, 2022
1 parent 63a3667 commit de275e7
Show file tree
Hide file tree
Showing 2 changed files with 171 additions and 178 deletions.
172 changes: 171 additions & 1 deletion src/build/app/debug_asserts.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::{
build::arg::{debug_asserts::assert_arg, ArgProvider},
mkeymap::KeyType,
util::Id,
App, AppSettings, ArgSettings, ValueHint,
App, AppSettings, Arg, ArgSettings, ValueHint,
};
use std::cmp::Ordering;

Expand Down Expand Up @@ -274,6 +275,8 @@ pub(crate) fn assert_app(app: &App) {
detect_duplicate_flags(&long_flags, "long");
detect_duplicate_flags(&short_flags, "short");

_verify_positionals(app);

if let Some(help_template) = app.template {
assert!(
!help_template.contains("{flags}"),
Expand Down Expand Up @@ -410,3 +413,170 @@ fn assert_app_flags(app: &App) {
#[cfg(feature = "unstable-multicall")]
checker!(Multicall conflicts NoBinaryName);
}

#[cfg(debug_assertions)]
fn _verify_positionals(app: &App) -> bool {
debug!("App::_verify_positionals");
// Because you must wait until all arguments have been supplied, this is the first chance
// to make assertions on positional argument indexes
//
// First we verify that the index highest supplied index, is equal to the number of
// positional arguments to verify there are no gaps (i.e. supplying an index of 1 and 3
// but no 2)

let highest_idx = app
.args
.keys()
.filter_map(|x| {
if let KeyType::Position(n) = x {
Some(*n)
} else {
None
}
})
.max()
.unwrap_or(0);

let num_p = app.args.keys().filter(|x| x.is_position()).count();

assert!(
highest_idx == num_p,
"Found positional argument whose index is {} but there \
are only {} positional arguments defined",
highest_idx,
num_p
);

// Next we verify that only the highest index has takes multiple arguments (if any)
let only_highest = |a: &Arg| a.is_multiple() && (a.index.unwrap_or(0) != highest_idx);
if app.get_positionals().any(only_highest) {
// First we make sure if there is a positional that allows multiple values
// the one before it (second to last) has one of these:
// * a value terminator
// * ArgSettings::Last
// * The last arg is Required

// We can't pass the closure (it.next()) to the macro directly because each call to
// find() (iterator, not macro) gets called repeatedly.
let last = &app.args[&KeyType::Position(highest_idx)];
let second_to_last = &app.args[&KeyType::Position(highest_idx - 1)];

// Either the final positional is required
// Or the second to last has a terminator or .last(true) set
let ok = last.is_set(ArgSettings::Required)
|| (second_to_last.terminator.is_some() || second_to_last.is_set(ArgSettings::Last))
|| last.is_set(ArgSettings::Last);
assert!(
ok,
"When using a positional argument with .multiple_values(true) that is *not the \
last* positional argument, the last positional argument (i.e. the one \
with the highest index) *must* have .required(true) or .last(true) set."
);

// We make sure if the second to last is Multiple the last is ArgSettings::Last
let ok = second_to_last.is_multiple() || last.is_set(ArgSettings::Last);
assert!(
ok,
"Only the last positional argument, or second to last positional \
argument may be set to .multiple_values(true)"
);

// Next we check how many have both Multiple and not a specific number of values set
let count = app
.get_positionals()
.filter(|p| {
p.settings.is_set(ArgSettings::MultipleOccurrences)
|| (p.settings.is_set(ArgSettings::MultipleValues) && p.num_vals.is_none())
})
.count();
let ok = count <= 1
|| (last.is_set(ArgSettings::Last)
&& last.is_multiple()
&& second_to_last.is_multiple()
&& count == 2);
assert!(
ok,
"Only one positional argument with .multiple_values(true) set is allowed per \
command, unless the second one also has .last(true) set"
);
}

let mut found = false;

if app.is_set(AppSettings::AllowMissingPositional) {
// Check that if a required positional argument is found, all positions with a lower
// index are also required.
let mut foundx2 = false;

for p in app.get_positionals() {
if foundx2 && !p.is_set(ArgSettings::Required) {
assert!(
p.is_set(ArgSettings::Required),
"Found non-required positional argument with a lower \
index than a required positional argument by two or more: {:?} \
index {:?}",
p.name,
p.index
);
} else if p.is_set(ArgSettings::Required) && !p.is_set(ArgSettings::Last) {
// Args that .last(true) don't count since they can be required and have
// positionals with a lower index that aren't required
// Imagine: prog <req1> [opt1] -- <req2>
// Both of these are valid invocations:
// $ prog r1 -- r2
// $ prog r1 o1 -- r2
if found {
foundx2 = true;
continue;
}
found = true;
continue;
} else {
found = false;
}
}
} else {
// Check that if a required positional argument is found, all positions with a lower
// index are also required
for p in (1..=num_p).rev().filter_map(|n| app.args.get(&n)) {
if found {
assert!(
p.is_set(ArgSettings::Required),
"Found non-required positional argument with a lower \
index than a required positional argument: {:?} index {:?}",
p.name,
p.index
);
} else if p.is_set(ArgSettings::Required) && !p.is_set(ArgSettings::Last) {
// Args that .last(true) don't count since they can be required and have
// positionals with a lower index that aren't required
// Imagine: prog <req1> [opt1] -- <req2>
// Both of these are valid invocations:
// $ prog r1 -- r2
// $ prog r1 o1 -- r2
found = true;
continue;
}
}
}
assert!(
app.get_positionals()
.filter(|p| p.is_set(ArgSettings::Last))
.count()
< 2,
"Only one positional argument may have last(true) set. Found two."
);
if app
.get_positionals()
.any(|p| p.is_set(ArgSettings::Last) && p.is_set(ArgSettings::Required))
&& app.has_subcommands()
&& !app.is_set(AppSettings::SubcommandsNegateReqs)
{
panic!(
"Having a required positional argument with .last(true) set *and* child \
subcommands without setting SubcommandsNegateReqs isn't compatible."
);
}

true
}
177 changes: 0 additions & 177 deletions src/parse/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@ impl<'help, 'app> Parser<'help, 'app> {
pub(crate) fn _build(&mut self) {
debug!("Parser::_build");

#[cfg(debug_assertions)]
self._verify_positionals();

for group in &self.app.groups {
if group.required {
let idx = self.required.insert(group.id.clone());
Expand All @@ -76,180 +73,6 @@ impl<'help, 'app> Parser<'help, 'app> {
}
}

#[cfg(debug_assertions)]
fn _verify_positionals(&self) -> bool {
debug!("Parser::_verify_positionals");
// Because you must wait until all arguments have been supplied, this is the first chance
// to make assertions on positional argument indexes
//
// First we verify that the index highest supplied index, is equal to the number of
// positional arguments to verify there are no gaps (i.e. supplying an index of 1 and 3
// but no 2)

let highest_idx = self
.app
.args
.keys()
.filter_map(|x| {
if let KeyType::Position(n) = x {
Some(*n)
} else {
None
}
})
.max()
.unwrap_or(0);

//_highest_idx(&self.positionals);

let num_p = self.app.args.keys().filter(|x| x.is_position()).count();

assert!(
highest_idx == num_p,
"Found positional argument whose index is {} but there \
are only {} positional arguments defined",
highest_idx,
num_p
);

// Next we verify that only the highest index has takes multiple arguments (if any)
let only_highest = |a: &Arg| a.is_multiple() && (a.index.unwrap_or(0) != highest_idx);
if self.app.get_positionals().any(only_highest) {
// First we make sure if there is a positional that allows multiple values
// the one before it (second to last) has one of these:
// * a value terminator
// * ArgSettings::Last
// * The last arg is Required

// We can't pass the closure (it.next()) to the macro directly because each call to
// find() (iterator, not macro) gets called repeatedly.
let last = &self.app.args[&KeyType::Position(highest_idx)];
let second_to_last = &self.app.args[&KeyType::Position(highest_idx - 1)];

// Either the final positional is required
// Or the second to last has a terminator or .last(true) set
let ok = last.is_set(ArgSettings::Required)
|| (second_to_last.terminator.is_some()
|| second_to_last.is_set(ArgSettings::Last))
|| last.is_set(ArgSettings::Last);
assert!(
ok,
"When using a positional argument with .multiple_values(true) that is *not the \
last* positional argument, the last positional argument (i.e. the one \
with the highest index) *must* have .required(true) or .last(true) set."
);

// We make sure if the second to last is Multiple the last is ArgSettings::Last
let ok = second_to_last.is_multiple() || last.is_set(ArgSettings::Last);
assert!(
ok,
"Only the last positional argument, or second to last positional \
argument may be set to .multiple_values(true)"
);

// Next we check how many have both Multiple and not a specific number of values set
let count = self
.app
.get_positionals()
.filter(|p| {
p.settings.is_set(ArgSettings::MultipleOccurrences)
|| (p.settings.is_set(ArgSettings::MultipleValues) && p.num_vals.is_none())
})
.count();
let ok = count <= 1
|| (last.is_set(ArgSettings::Last)
&& last.is_multiple()
&& second_to_last.is_multiple()
&& count == 2);
assert!(
ok,
"Only one positional argument with .multiple_values(true) set is allowed per \
command, unless the second one also has .last(true) set"
);
}

let mut found = false;

if self.is_set(AS::AllowMissingPositional) {
// Check that if a required positional argument is found, all positions with a lower
// index are also required.
let mut foundx2 = false;

for p in self.app.get_positionals() {
if foundx2 && !p.is_set(ArgSettings::Required) {
assert!(
p.is_set(ArgSettings::Required),
"Found non-required positional argument with a lower \
index than a required positional argument by two or more: {:?} \
index {:?}",
p.name,
p.index
);
} else if p.is_set(ArgSettings::Required) && !p.is_set(ArgSettings::Last) {
// Args that .last(true) don't count since they can be required and have
// positionals with a lower index that aren't required
// Imagine: prog <req1> [opt1] -- <req2>
// Both of these are valid invocations:
// $ prog r1 -- r2
// $ prog r1 o1 -- r2
if found {
foundx2 = true;
continue;
}
found = true;
continue;
} else {
found = false;
}
}
} else {
// Check that if a required positional argument is found, all positions with a lower
// index are also required
for p in (1..=num_p).rev().filter_map(|n| self.app.args.get(&n)) {
if found {
assert!(
p.is_set(ArgSettings::Required),
"Found non-required positional argument with a lower \
index than a required positional argument: {:?} index {:?}",
p.name,
p.index
);
} else if p.is_set(ArgSettings::Required) && !p.is_set(ArgSettings::Last) {
// Args that .last(true) don't count since they can be required and have
// positionals with a lower index that aren't required
// Imagine: prog <req1> [opt1] -- <req2>
// Both of these are valid invocations:
// $ prog r1 -- r2
// $ prog r1 o1 -- r2
found = true;
continue;
}
}
}
assert!(
self.app
.get_positionals()
.filter(|p| p.is_set(ArgSettings::Last))
.count()
< 2,
"Only one positional argument may have last(true) set. Found two."
);
if self
.app
.get_positionals()
.any(|p| p.is_set(ArgSettings::Last) && p.is_set(ArgSettings::Required))
&& self.app.has_subcommands()
&& !self.is_set(AS::SubcommandsNegateReqs)
{
panic!(
"Having a required positional argument with .last(true) set *and* child \
subcommands without setting SubcommandsNegateReqs isn't compatible."
);
}

true
}

// Should we color the help?
pub(crate) fn color_help(&self) -> ColorChoice {
#[cfg(feature = "color")]
Expand Down

0 comments on commit de275e7

Please sign in to comment.