Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Run more parse asserts during build #3288

Merged
merged 1 commit into from Jan 12, 2022
Merged

fix: Run more parse asserts during build #3288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
172 changes: 171 additions & 1 deletion src/build/app/debug_asserts.rs
View file
Open in desktop
@@ -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
Open in desktop
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