Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions clap_builder/src/parser/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,16 @@ impl<'cmd> Validator<'cmd> {

fn is_missing_required_ok(&self, a: &Arg, conflicts: &Conflicts) -> bool {
debug!("Validator::is_missing_required_ok: {}", a.get_id());

// If this argument is conditionally required (i.e., required by other present arguments
// through the 'requires' relationship), it's NOT OK for it to be missing, even if it
// conflicts with other arguments. However, directly required arguments (marked as
// required(true)) can still be bypassed by conflicts.
if self.required.contains(a.get_id()) && !a.is_required_set() {
debug!("Validator::is_missing_required_ok: false (conditionally required)");
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this might fix the immediate problem, I'm worried that this is too brittle and we can change things in other code and not realize this needs to be update as well.


if !conflicts.gather_conflicts(self.cmd, a.get_id()).is_empty() {
debug!("Validator::is_missing_required_ok: true (self)");
return true;
Expand Down
67 changes: 67 additions & 0 deletions tests/builder/issue_4707.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that #4520 is the core issue and it hasn't been accepted yet, requiring further analysis for correctness and whether this would constitute a breaking change.

For future contributions, please keep in mind

  • We prefer to resolve issues before moving on to PRs
  • Commits should be atomic, including tests passing
  • We do ask for tests to be added in commits before the behavior changes but that is to show the existing behavior
  • derive tests should go in the derive testsuite, not the builder one
  • tests should be grouped by and named for their functionality, not issue numbers

See also https://github.com/clap-rs/clap/blob/master/CONTRIBUTING.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So wait, is 4707 not a workable issue? I have to wait until 4520 is resolved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are the same concept (how should requires interact with conflicts) and that question needs to be addressed before either moves forward. Rather than split that conversation between two related issues, I recommend we centralize it on #4520. That doesn't mean that #4520 needs to be implemented first. In fact, its likely that a fix for one will fix the other or be just a one or two line change.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,56 @@
/// These tests verify that the requires validation works correctly.
use clap::{Arg, ArgAction, ArgGroup, Command, error::ErrorKind};

#[cfg(feature = "derive")]
mod derive_tests {
use clap::{ArgGroup, Parser};

#[derive(Parser, Debug)]
#[clap(group = ArgGroup::new("command").multiple(false))]
struct Args {
#[clap(long, group = "command")]
read: bool,

#[clap(long, group = "command")]
write: bool,

#[clap(long, requires = "read")]
show_hex: bool,
}

#[test]
fn issue_4707_original_derive_example() {
// This is the exact example from the GitHub issue
// It should fail when --show-hex is used without --read
let result = Args::try_parse_from(["test", "--show-hex"]);

assert!(result.is_err(), "Should fail because --show-hex requires --read");
assert_eq!(result.unwrap_err().kind(), clap::error::ErrorKind::MissingRequiredArgument);
}

#[test]
fn issue_4707_derive_example_with_write() {
// Test the problematic case: using --write and --show-hex together
// This should fail because --show-hex requires --read, but --write and --read are mutually exclusive
let result = Args::try_parse_from(["test", "--write", "--show-hex"]);

assert!(result.is_err(), "Should fail because --show-hex requires --read but --write and --read are mutually exclusive");
assert_eq!(result.unwrap_err().kind(), clap::error::ErrorKind::MissingRequiredArgument);
}

#[test]
fn issue_4707_derive_example_valid_case() {
// This should succeed when --read and --show-hex are used together
let result = Args::try_parse_from(["test", "--read", "--show-hex"]);

assert!(result.is_ok(), "Should succeed when --read and --show-hex are used together");
let args = result.unwrap();
assert!(args.read);
assert!(args.show_hex);
assert!(!args.write);
}
}

#[test]
fn issue_4707_requires_should_be_validated_when_args_are_in_group() {
// This test ensures that `requires` validation is NOT bypassed
Expand Down Expand Up @@ -92,3 +142,20 @@ fn issue_4707_complex_interaction_test() {
let result3 = cmd.clone().try_get_matches_from(vec!["test", "-v", "-o"]);
assert!(result3.is_ok(), "Should succeed when dependency is provided");
}

#[test]
fn issue_4707_exact_github_example_builder() {
// This reproduces the exact case from the GitHub issue using clap builder instead of derive
let cmd = Command::new("test")
.arg(Arg::new("read").long("read").action(ArgAction::SetTrue).group("command"))
.arg(Arg::new("write").long("write").action(ArgAction::SetTrue).group("command"))
.arg(Arg::new("show_hex").long("show-hex").action(ArgAction::SetTrue).requires("read"))
.group(ArgGroup::new("command").multiple(false));

// This exact case should fail: --write --show-hex
// Because --show-hex requires --read, but --read and --write are mutually exclusive
let result = cmd.try_get_matches_from(["test", "--write", "--show-hex"]);

assert!(result.is_err(), "Should fail because --show-hex requires --read but --write and --read are mutually exclusive");
assert_eq!(result.unwrap_err().kind(), ErrorKind::MissingRequiredArgument);
}