Skip to content

Commit

Permalink
Merge pull request #2309 from ldm0/parser0
Browse files Browse the repository at this point in the history 
Dehack, Revert #1856
  • Loading branch information
@pksunkara
pksunkara authored Jan 30, 2021
2 parents 56ffb5e + 0df169b commit 8d5a021
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 119 deletions.
4 changes: 2 additions & 2 deletions src/output/usage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> {
.filter(|pos| !pos.is_set(ArgSettings::Last))
{
debug!("Usage::get_args_tag:iter:{}", pos.name);
let in_required_group = self.p.app.groups_for_arg(&pos.id).any(|grp_s| {
let required = self.p.app.groups_for_arg(&pos.id).any(|grp_s| {
debug!("Usage::get_args_tag:iter:{:?}:iter:{:?}", pos.name, grp_s);
// if it's part of a required group we don't want to count it
self.p
Expand All @@ -232,7 +232,7 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> {
.iter()
.any(|g| g.required && (g.id == grp_s))
});
if !in_required_group {
if !required {
count += 1;
debug!(
"Usage::get_args_tag:iter: {} Args not required or hidden",
Expand Down
87 changes: 15 additions & 72 deletions src/parse/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ use crate::{

#[derive(Debug, PartialEq, Clone)]
pub(crate) enum ParseResult {
Flag(Id),
Flag,
FlagSubCommand(String),
// subcommand name, whether there are more shorts args remaining
FlagSubCommandShort(String, bool),
Opt(Id),
Pos(Id),
MaybeHyphenValue,
NotFound,
ValuesDone(Id),
ValuesDone,
}

#[derive(Debug)]
Expand Down Expand Up @@ -386,10 +386,7 @@ impl<'help, 'app> Parser<'help, 'app> {
needs_val_of
);
match needs_val_of {
ParseResult::Flag(ref id)
| ParseResult::Opt(ref id)
| ParseResult::ValuesDone(ref id) => {
self.maybe_inc_pos_counter(&mut pos_counter, id);
ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => {
continue;
}
ParseResult::FlagSubCommand(ref name) => {
Expand Down Expand Up @@ -417,10 +414,7 @@ impl<'help, 'app> Parser<'help, 'app> {
needs_val_of
);
match needs_val_of {
ParseResult::Opt(ref id)
| ParseResult::Flag(ref id)
| ParseResult::ValuesDone(ref id) => {
self.maybe_inc_pos_counter(&mut pos_counter, id);
ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => {
continue;
}
ParseResult::FlagSubCommandShort(ref name, done) => {
Expand Down Expand Up @@ -484,7 +478,7 @@ impl<'help, 'app> Parser<'help, 'app> {
if low_index_mults || missing_pos {
if let Some(n) = remaining_args.get(0) {
needs_val_of = match needs_val_of {
ParseResult::ValuesDone(id) => ParseResult::ValuesDone(id),
ParseResult::ValuesDone => ParseResult::ValuesDone,
_ => {
if let Some(p) = self
.app
Expand All @@ -493,7 +487,7 @@ impl<'help, 'app> Parser<'help, 'app> {
{
ParseResult::Pos(p.id.clone())
} else {
ParseResult::ValuesDone(Id::empty_hash())
ParseResult::ValuesDone
}
}
};
Expand Down Expand Up @@ -699,57 +693,6 @@ impl<'help, 'app> Parser<'help, 'app> {
}
}

// HACK:
// When we have a group that is NOT multiple AND has both
// an option and a positional arg, only one of them must be passed.
//
// The problem here is that clap decides which positional arg it's looking at
// (the one from group or the following one) based on the counter that is incremented
// every time clap stores a positional arg.
//
// When a non positional option is passed, this counter is not incremented.
// In other words, the group has already been "occupied" by the option, but the
// counter still points to the positional argument that belongs to this group.
// If the option is followed by yet another positional arg, it will land in
// the group, erroneously.
//
// This is a pretty much fundamental problem with the current parsing algorithm
// and the function below is simply a hack that solves the problem at hand,
// but it does so at cost of minor regression in error messages (see tests/groups.rs).
//
// In order to resolve it properly, we need to rewrite the parser entirely.
// I don't really feel like it right now, sorry.
// The parser is big and scary and full of legacy.
fn maybe_inc_pos_counter(&self, pos_counter: &mut usize, id: &Id) {
debug!("Parser::maybe_inc_pos_counter: arg = {:?}", id);

let arg = &self.app[id];

debug!("Parser::maybe_inc_pos_counter: is it positional?");
// will be incremented by other means.
if arg.is_positional() {
debug!("Yes");
return;
}
debug!("No");

for group in self.app.groups_for_arg(&arg.id) {
debug!("Parser::maybe_inc_pos_counter: group={:?}", group);
let group = self.app.groups.iter().find(|g| g.id == group);

debug!("Parser::maybe_inc_pos_counter: Checking args in the group...");
if let Some(group) = group {
for arg in group.args.iter() {
debug!("Parser::maybe_inc_pos_counter: arg={:?}", arg);
if self.app[arg].is_positional() {
debug!("Parser::maybe_inc_pos_counter: Incrementing counter!");
*pos_counter += 1;
}
}
}
}
}

// Checks if the arg matches a subcommand name, or any of its aliases (if defined)
fn possible_subcommand(&self, arg_os: &ArgStr) -> Option<&str> {
debug!("Parser::possible_subcommand: arg={:?}", arg_os);
Expand Down Expand Up @@ -896,7 +839,7 @@ impl<'help, 'app> Parser<'help, 'app> {
|| (self.is_set(AS::AllowNegativeNumbers)
&& arg_os.to_string_lossy().parse::<f64>().is_ok())
}
ParseResult::ValuesDone(..) => return true,
ParseResult::ValuesDone => return true,
_ => false,
};

Expand Down Expand Up @@ -1116,7 +1059,7 @@ impl<'help, 'app> Parser<'help, 'app> {
self.check_for_help_and_version_str(&arg)?;
self.parse_flag(opt, matcher)?;

return Ok(ParseResult::Flag(opt.id.clone()));
return Ok(ParseResult::Flag);
} else if let Some(sc_name) = self.possible_long_flag_subcommand(&arg) {
return Ok(ParseResult::FlagSubCommand(sc_name.to_string()));
} else if self.is_set(AS::AllowLeadingHyphen) {
Expand Down Expand Up @@ -1286,13 +1229,13 @@ impl<'help, 'app> Parser<'help, 'app> {
// @TODO @soundness: if doesn't have an equal, but requires equal is ValuesDone?!
if no_val && min_vals_zero && !has_eq && needs_eq {
debug!("Parser::parse_opt: More arg vals not required...");
return Ok(ParseResult::ValuesDone(opt.id.clone()));
return Ok(ParseResult::ValuesDone);
} else if no_val || (mult && !needs_delim) && !has_eq && matcher.needs_more_vals(opt) {
debug!("Parser::parse_opt: More arg vals required...");
return Ok(ParseResult::Opt(opt.id.clone()));
}
debug!("Parser::parse_opt: More arg vals not required...");
Ok(ParseResult::ValuesDone(opt.id.clone()))
Ok(ParseResult::ValuesDone)
}

fn add_val_to_arg(
Expand All @@ -1313,13 +1256,13 @@ impl<'help, 'app> Parser<'help, 'app> {
if val.is_empty() {
Ok(self.add_single_val_to_arg(arg, val, matcher, ty)?)
} else {
let mut iret = ParseResult::ValuesDone(arg.id.clone());
let mut iret = ParseResult::ValuesDone;
for v in val.split(delim) {
iret = self.add_single_val_to_arg(arg, &v, matcher, ty)?;
}
// If there was a delimiter used, we're not looking for more values
if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) {
iret = ParseResult::ValuesDone(arg.id.clone());
iret = ParseResult::ValuesDone;
}
Ok(iret)
}
Expand All @@ -1346,7 +1289,7 @@ impl<'help, 'app> Parser<'help, 'app> {
// @TODO @docs @p4 docs should probably note that terminator doesn't get an index
if let Some(t) = arg.terminator {
if t == v {
return Ok(ParseResult::ValuesDone(arg.id.clone()));
return Ok(ParseResult::ValuesDone);
}
}

Expand All @@ -1361,7 +1304,7 @@ impl<'help, 'app> Parser<'help, 'app> {
if matcher.needs_more_vals(arg) {
return Ok(ParseResult::Opt(arg.id.clone()));
}
Ok(ParseResult::ValuesDone(arg.id.clone()))
Ok(ParseResult::ValuesDone)
}

fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<ParseResult> {
Expand All @@ -1374,7 +1317,7 @@ impl<'help, 'app> Parser<'help, 'app> {
matcher.inc_occurrence_of(&grp);
}

Ok(ParseResult::Flag(flag.id.clone()))
Ok(ParseResult::Flag)
}

fn remove_overrides(&mut self, matcher: &mut ArgMatcher) {
Expand Down
36 changes: 8 additions & 28 deletions tests/groups.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,38 +11,18 @@ USAGE:
For more information try --help";

static REQ_GROUP_CONFLICT_USAGE: &str =
"error: The argument '<base>' cannot be used with '--delete'
USAGE:
clap-test <base|--delete>
For more information try --help";

#[allow(unused)]
static REQ_GROUP_CONFLICT_REV: &str = "error: The argument '--delete' cannot be used with '<base>'
"error: The argument '--delete' cannot be used with '<base>'
USAGE:
clap-test <base|--delete>
For more information try --help";

static REQ_GROUP_CONFLICT_ONLY_OPTIONS: &str =
"error: Found argument '--all' which wasn't expected, or isn't valid in this context
\tIf you tried to supply `--all` as a value rather than a flag, use `-- --all`
USAGE:
clap-test <-a|--delete>
For more information try --help";

// FIXME: This message has regressed after https://github.com/clap-rs/clap/pull/1856
// Need to roll back somehow.
static REQ_GROUP_CONFLICT_REV_DEGRADED: &str =
"error: Found argument 'base' which wasn't expected, or isn't valid in this context
"error: The argument '--delete' cannot be used with '--all'
USAGE:
clap-test <base|--delete>
clap-test <--all|--delete>
For more information try --help";

Expand Down Expand Up @@ -214,10 +194,9 @@ fn req_group_with_conflict_usage_string() {
.required(true),
);

assert!(utils::compare_output2(
assert!(utils::compare_output(
app,
"clap-test --delete base",
REQ_GROUP_CONFLICT_REV_DEGRADED,
REQ_GROUP_CONFLICT_USAGE,
true
));
Expand All @@ -226,7 +205,7 @@ fn req_group_with_conflict_usage_string() {
#[test]
fn req_group_with_conflict_usage_string_only_options() {
let app = App::new("req_group")
.arg(Arg::from("<all> -a, -all 'All'").conflicts_with("delete"))
.arg(Arg::from("<all> -a --all 'All'").conflicts_with("delete"))
.arg(Arg::from(
"<delete> -d, --delete 'Remove the base commit information'",
))
Expand All @@ -235,11 +214,10 @@ fn req_group_with_conflict_usage_string_only_options() {
.args(&["all", "delete"])
.required(true),
);
assert!(utils::compare_output2(
assert!(utils::compare_output(
app,
"clap-test --delete --all",
REQ_GROUP_CONFLICT_ONLY_OPTIONS,
REQ_GROUP_CONFLICT_USAGE,
true
));
}
Expand Down Expand Up @@ -307,6 +285,7 @@ fn group_acts_like_arg() {
assert!(m.is_present("mode"));
}

/* This is used to be fixed in a hack, we need to find a better way to fix it.
#[test]
fn issue_1794() {
let app = clap::App::new("hello")
Expand All @@ -332,3 +311,4 @@ fn issue_1794() {
assert_eq!(m.value_of("pos2"), Some("positional"));
assert!(m.is_present("option1"));
}
*/
17 changes: 0 additions & 17 deletions tests/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,6 @@ pub fn compare_output(l: App, args: &str, right: &str, stderr: bool) -> bool {
compare(left, right)
}

pub fn compare_output2(l: App, args: &str, right1: &str, right2: &str, stderr: bool) -> bool {
let mut buf = Cursor::new(Vec::with_capacity(50));
let res = l.try_get_matches_from(args.split(' ').collect::<Vec<_>>());
let err = res.unwrap_err();
write!(&mut buf, "{}", err).unwrap();
let content = buf.into_inner();
let left = String::from_utf8(content).unwrap();
assert_eq!(
stderr,
err.use_stderr(),
"Should Use STDERR failed. Should be {} but is {}",
stderr,
err.use_stderr()
);
compare(&*left, right1) || compare(&*left, right2)
}

// Legacy tests from the python script days

pub fn complex_app() -> App<'static> {
Expand Down

0 comments on commit 8d5a021

Please sign in to comment.