From bfca1935569762f4d0bdf299dfbf7e5850f8f583 Mon Sep 17 00:00:00 2001 From: Omer Tuchfeld Date: Tue, 3 Dec 2024 19:48:45 +0100 Subject: [PATCH 1/3] refactor(asserts): Tuple destructuring Use tuple destructuring to make the code clearer Signed-off-by: Omer Tuchfeld --- clap_builder/src/builder/debug_asserts.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clap_builder/src/builder/debug_asserts.rs b/clap_builder/src/builder/debug_asserts.rs index 9e6154ab66a..6a1fc7935ea 100644 --- a/clap_builder/src/builder/debug_asserts.rs +++ b/clap_builder/src/builder/debug_asserts.rs @@ -139,12 +139,12 @@ pub(crate) fn assert_app(cmd: &Command) { } // requires, r_if, r_unless - for req in &arg.requires { + for (_predicate, req_id) in &arg.requires { assert!( - cmd.id_exists(&req.1), + cmd.id_exists(req_id), "Command {}: Argument or group '{}' specified in 'requires*' for '{}' does not exist", cmd.get_name(), - req.1, + req_id, arg.get_id(), ); } From 52aad0ea1a24f95ecc737bd5d11152747a62e055 Mon Sep 17 00:00:00 2001 From: Omer Tuchfeld Date: Tue, 3 Dec 2024 19:56:21 +0100 Subject: [PATCH 2/3] test(asserts): Add test for self requires Add a test that shows that clap doesn't complain when a flag requires itself. This test demonstrates existing broken behavior, ideally it should panic. It will be fixed in the next commit. Signed-off-by: Omer Tuchfeld --- tests/builder/require.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/builder/require.rs b/tests/builder/require.rs index ea0f09f57ef..d4aa2ac8396 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -1478,3 +1478,12 @@ For more information, try '--help'. "; utils::assert_output(cmd, "test --require-first --second", EXPECTED, true); } + +#[test] +/// This test demonstrates existing broken behavior, ideally it should panic +fn requires_self() { + let result = Command::new("flag_required") + .arg(arg!(-f --flag "some flag").requires("flag")) + .try_get_matches_from(vec![""]); + assert!(result.is_ok()); +} From 29d9e8844fda2a97902de76e47053552e3cddd39 Mon Sep 17 00:00:00 2001 From: Omer Tuchfeld Date: Tue, 3 Dec 2024 19:58:37 +0100 Subject: [PATCH 3/3] fix(assert): Prevent arguments from requiring self It's non-sensical for an argument to require itself, so it must be a mistake, and should be prevented. This is arguably a breaking change, but of the spacebar heating kind. Signed-off-by: Omer Tuchfeld --- clap_builder/src/builder/debug_asserts.rs | 6 ++++++ tests/builder/require.rs | 5 ++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/clap_builder/src/builder/debug_asserts.rs b/clap_builder/src/builder/debug_asserts.rs index 6a1fc7935ea..001ef8a1201 100644 --- a/clap_builder/src/builder/debug_asserts.rs +++ b/clap_builder/src/builder/debug_asserts.rs @@ -140,6 +140,12 @@ pub(crate) fn assert_app(cmd: &Command) { // requires, r_if, r_unless for (_predicate, req_id) in &arg.requires { + assert!( + &arg.id != req_id, + "Argument {} cannot require itself", + arg.get_id() + ); + assert!( cmd.id_exists(req_id), "Command {}: Argument or group '{}' specified in 'requires*' for '{}' does not exist", diff --git a/tests/builder/require.rs b/tests/builder/require.rs index d4aa2ac8396..14592ad0a9e 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -1480,10 +1480,9 @@ For more information, try '--help'. } #[test] -/// This test demonstrates existing broken behavior, ideally it should panic +#[should_panic = "Argument flag cannot require itself"] fn requires_self() { - let result = Command::new("flag_required") + let _result = Command::new("flag_required") .arg(arg!(-f --flag "some flag").requires("flag")) .try_get_matches_from(vec![""]); - assert!(result.is_ok()); }