Skip to content

Commit

Permalink
fix(parser): Track the most explicit value source
Browse files Browse the repository at this point in the history
We were only tracking the last value source (default, env, cli, etc).
This works for args because they only come from one source.  Groups
however can come from multiple sources and this was making us treat a
group with a default value as being completely from defaults despite
some values maybe being from the commandline.

We now track the highest precedence value for a group.

Fixes #3330
  • Loading branch information
epage committed Jan 24, 2022
1 parent afd0342 commit c6664af
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 9 deletions.
10 changes: 5 additions & 5 deletions src/parse/arg_matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ impl ArgMatcher {
let id = &arg.id;
debug!("ArgMatcher::inc_occurrence_of_arg: id={:?}", id);
let ma = self.entry(id).or_insert(MatchedArg::new());
ma.set_ty(ValueType::CommandLine);
ma.update_ty(ValueType::CommandLine);
ma.set_ignore_case(arg.is_set(ArgSettings::IgnoreCase));
ma.invalid_utf8_allowed(arg.is_set(ArgSettings::AllowInvalidUtf8));
ma.occurs += 1;
Expand All @@ -144,7 +144,7 @@ impl ArgMatcher {
pub(crate) fn inc_occurrence_of_group(&mut self, id: &Id) {
debug!("ArgMatcher::inc_occurrence_of_group: id={:?}", id);
let ma = self.entry(id).or_insert(MatchedArg::new());
ma.set_ty(ValueType::CommandLine);
ma.update_ty(ValueType::CommandLine);
ma.occurs += 1;
}

Expand All @@ -161,13 +161,13 @@ impl ArgMatcher {
// specific circumstances, like only add one occurrence for flag
// when we met: `--flag=one,two`).
let ma = self.entry(arg).or_default();
ma.set_ty(ty);
ma.update_ty(ty);
ma.push_val(val);
}

fn append_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
let ma = self.entry(arg).or_default();
ma.set_ty(ty);
ma.update_ty(ty);
ma.append_val(val);
}

Expand All @@ -178,7 +178,7 @@ impl ArgMatcher {

pub(crate) fn add_index_to(&mut self, arg: &Id, idx: usize, ty: ValueType) {
let ma = self.entry(arg).or_default();
ma.set_ty(ty);
ma.update_ty(ty);
ma.push_index(idx);
}

Expand Down
8 changes: 4 additions & 4 deletions src/parse/matches/matched_arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ impl MatchedArg {
})
}

pub(crate) fn set_ty(&mut self, ty: ValueType) {
self.ty = ty;
pub(crate) fn update_ty(&mut self, ty: ValueType) {
self.ty = self.ty.max(ty);
}

pub(crate) fn set_ignore_case(&mut self, yes: bool) {
Expand All @@ -142,13 +142,13 @@ impl Default for MatchedArg {
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) enum ValueType {
Unknown,
DefaultValue,
#[cfg(feature = "env")]
EnvVariable,
CommandLine,
DefaultValue,
}

#[cfg(test)]
Expand Down
37 changes: 37 additions & 0 deletions tests/builder/conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,43 @@ fn arg_conflicts_with_group() {
}
}

#[test]
fn arg_conflicts_with_group_with_multiple_sources() {
let mut app = clap::App::new("group_conflict")
.arg(clap::arg!(-f --flag "some flag").conflicts_with("gr"))
.group(clap::ArgGroup::new("gr").multiple(true))
.arg(
clap::arg!(--some <name> "some arg")
.required(false)
.group("gr"),
)
.arg(
clap::arg!(--other <secs> "other arg")
.required(false)
.default_value("1000")
.group("gr"),
);

let result = app.try_get_matches_from_mut(vec!["myprog", "-f"]);
if let Err(err) = result {
panic!("{}", err);
}

let result = app.try_get_matches_from_mut(vec!["myprog", "--some", "usb1"]);
if let Err(err) = result {
panic!("{}", err);
}

let result = app.try_get_matches_from_mut(vec!["myprog", "--some", "usb1", "--other", "40"]);
if let Err(err) = result {
panic!("{}", err);
}

let result = app.try_get_matches_from_mut(vec!["myprog", "-f", "--some", "usb1"]);
let err = result.err().unwrap();
assert_eq!(err.kind, ErrorKind::ArgumentConflict);
}

#[test]
fn group_conflicts_with_arg() {
let mut app = App::new("group_conflict")
Expand Down

0 comments on commit c6664af

Please sign in to comment.