Skip to content

Commit

Permalink
fix(validate): Overrides always ignore required
Browse files Browse the repository at this point in the history
Before, if two arguments were required *and* overrode each other, then
`cmd --opt=1 --other=2` succeded but `cmd --other=2` failed despite
ignoring `--opt=1`.  Requiring `--opt=1` to be present but unavailable
doesn't help anyone and makes the behavior less predictable.

Now both commands will have the same behavior.
  • Loading branch information
epage committed Apr 21, 2022
1 parent de89c05 commit a484f62
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 15 deletions.
4 changes: 4 additions & 0 deletions src/build/arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4339,6 +4339,8 @@ impl<'help> Arg<'help> {
/// **NOTE:** When an argument is overridden it is essentially as if it never was used, any
/// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed
///
/// **NOTE:** Overriding an argument implies they [conflict][Arg::conflicts_with`].
///
/// **WARNING:** Positional arguments and options which accept
/// [`Arg::multiple_occurrences`] cannot override themselves (or we
/// would never be able to advance to the next positional). If a positional
Expand Down Expand Up @@ -4454,6 +4456,8 @@ impl<'help> Arg<'help> {
/// **NOTE:** When an argument is overridden it is essentially as if it never was used, any
/// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed
///
/// **NOTE:** Overriding an argument implies they [conflict][Arg::conflicts_with_all`].
///
/// # Examples
///
/// ```rust
Expand Down
6 changes: 1 addition & 5 deletions src/parse/parser.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Std
use std::{
cell::{Cell, RefCell},
cell::Cell,
ffi::{OsStr, OsString},
};

Expand All @@ -22,7 +22,6 @@ use crate::{INTERNAL_ERROR_MSG, INVALID_UTF8};

pub(crate) struct Parser<'help, 'cmd> {
pub(crate) cmd: &'cmd mut Command<'help>,
pub(crate) overridden: RefCell<Vec<Id>>,
seen: Vec<Id>,
cur_idx: Cell<usize>,
/// Index of the previous flag subcommand in a group of flags.
Expand All @@ -37,7 +36,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
pub(crate) fn new(cmd: &'cmd mut Command<'help>) -> Self {
Parser {
cmd,
overridden: Default::default(),
seen: Vec::new(),
cur_idx: Cell::new(0),
flag_subcmd_at: None,
Expand Down Expand Up @@ -1248,7 +1246,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
for override_id in &arg.overrides {
debug!("Parser::remove_overrides:iter:{:?}: removing", override_id);
matcher.remove(override_id);
self.overridden.borrow_mut().push(override_id.clone());
}

// Override anything that can override us
Expand All @@ -1263,7 +1260,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
for overrider_id in transitive {
debug!("Parser::remove_overrides:iter:{:?}: removing", overrider_id);
matcher.remove(overrider_id);
self.overridden.borrow_mut().push(overrider_id.clone());
}
}

Expand Down
14 changes: 4 additions & 10 deletions src/parse/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,16 +532,6 @@ impl<'help, 'cmd, 'parser> Validator<'help, 'cmd, 'parser> {
conflicts: &mut Conflicts,
) -> bool {
debug!("Validator::is_missing_required_ok: {}", a.name);
self.arg_has_conflicts(a, matcher, conflicts) || self.p.overridden.borrow().contains(&a.id)
}

fn arg_has_conflicts(
&self,
a: &Arg<'help>,
matcher: &ArgMatcher,
conflicts: &mut Conflicts,
) -> bool {
debug!("Validator::arg_has_conflicts: a={:?}", a.name);
let conflicts = conflicts.gather_conflicts(self.p.cmd, matcher, &a.id);
!conflicts.is_empty()
}
Expand Down Expand Up @@ -666,6 +656,10 @@ impl Conflicts {
}
}
}

// Overrides are implicitly conflicts
conf.extend(arg.overrides.iter().cloned());

conf
} else if let Some(group) = cmd.find_group(arg_id) {
group.conflicts.clone()
Expand Down
38 changes: 38 additions & 0 deletions tests/builder/app_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,44 @@ fn aaos_opts_w_other_overrides_rev_2() {
assert_eq!(m.value_of("other"), Some("val"));
}

#[test]
fn aaos_opts_w_override_as_conflict_1() {
// opts with other overrides, rev
let res = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.required(true)
.overrides_with("other"),
)
.arg(arg!(--other <val> "some other option").required(true))
.try_get_matches_from(vec!["", "--opt=some"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.is_present("opt"));
assert!(!m.is_present("other"));
assert_eq!(m.value_of("opt"), Some("some"));
}

#[test]
fn aaos_opts_w_override_as_conflict_2() {
// opts with other overrides, rev
let res = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.required(true)
.overrides_with("other"),
)
.arg(arg!(--other <val> "some other option").required(true))
.try_get_matches_from(vec!["", "--other=some"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(!m.is_present("opt"));
assert!(m.is_present("other"));
assert_eq!(m.value_of("other"), Some("some"));
}

#[test]
fn aaos_opts_mult() {
// opts with multiple
Expand Down

0 comments on commit a484f62

Please sign in to comment.