Repository: clap-rs/clap
Pull request #5949 (2025-03-15 13:39:19)

Fix arbitrary-cardinality positionals shadowing option name completion
NOTE This text is outdated; see commit messages.

TODO: should double-check the root cause and add automated tests

jj dynamic completions of option names work as expected ...

	$ COMPLETE=fish jj -- jj abandon --ignore-
	--ignore-working-copy
	--ignore-immutable

... unless there is a positional argument

	$ COMPLETE=fish jj -- jj abandon @ --ignore-

The positional arguments are defined in the "abandon" subcommand as:

```rust
#[derive(clap::Args, Clone, Debug)]
pub(crate) struct AbandonArgs {
    /// The revision(s) to abandon (default: @)
    #[arg(
        value_name = "REVSETS",
        add = ArgValueCandidates::new(complete::mutable_revisions)
    )]
    revisions_pos: Vec<RevisionArg>,
}
```

After clap_complete sees the first positional argument ("@") we are stuck
in "ParseState::Pos(..)" even when we get to parsing "--ignore-".  This is
because "revisions_pos" accepts an arbitrary number of positionals.

While in "ParseState::Pos(..)", we only produce completions for positionals
(via "complete_arg_value()").  This means we fail to produce option-name
completions like "--ignore-immutable".

This was introduced in f0bd4750 (feat(clap_complete): Support
multi-values of positional argument with `num_arg(N)`, 2024-07-26).
Presumably, the main intention of that commit was to allow options
with multiple, but finitely many arguments.

For the case where arbitrarily many arguments are allowed, let's get rid of
this restriction, to fix the false negative.

Ref: https://github.com/fish-shell/fish-shell/pull/11046#discussion_r1921501251


epage: > TODO: should double-check the root cause and add automated tests

Note that we tend to ask for PRs to be split into at least two commits
1. Add a test, showing the current broken behavior (it passes)
2. Fix the behavior and update  the test to still pass, with the diff of the test showing how the behavior changed
krobelus: > I'll possibly get to it.

well then I'll do it since I want this to be fixed.

Unless a revert is accepted (upstream or downstream) which seems unlikely given that the fix is within arm's reach.

I guess regressions in libraries are treated with different priority than regressions in applications.

epage: > I guess regressions in libraries are treated with different priority than regressions in applications.

I would ask that you be more considerate to the work of others.  Your way of framing the conversation is very off putting and can be discouraging for people to contribute.

Specifically in this case, this is an unstable feature that wasn't even ready for general testing at the point before the "regression" but was still under going initial development.  The change added initial support for `num_args`.  I would classify this issue being about the support being incomplete (handling fixed number of arguments but not fully handling variable number of arguments).

Yes, I was going to try to get to it but I didn't want to over commit and disappoint as I have a lot of things on my own personal plate at  this moment.

Patch:
diff --git a/clap_complete/src/engine/complete.rs b/clap_complete/src/engine/complete.rs
index 7b01f55e..d99734e6 100644
--- a/clap_complete/src/engine/complete.rs
+++ b/clap_complete/src/engine/complete.rs
@@ -157,100 +157,20 @@ fn complete_arg(
             {
                 completions.extend(complete_arg_value(arg.to_value(), positional, current_dir));
             }
-
-            if arg.is_empty() {
-                completions.extend(longs_and_visible_aliases(cmd));
-                completions.extend(hidden_longs_aliases(cmd));
-
-                let dash_or_arg = if arg.is_empty() {
-                    "-".into()
-                } else {
-                    arg.to_value_os().to_string_lossy()
-                };
-                completions.extend(
-                    shorts_and_visible_aliases(cmd)
-                        .into_iter()
-                        .map(|comp| comp.add_prefix(dash_or_arg.to_string())),
-                );
-            } else if arg.is_stdio() {
-                // HACK: Assuming knowledge of is_stdio
-                let dash_or_arg = if arg.is_empty() {
-                    "-".into()
-                } else {
-                    arg.to_value_os().to_string_lossy()
-                };
-                completions.extend(
-                    shorts_and_visible_aliases(cmd)
-                        .into_iter()
-                        .map(|comp| comp.add_prefix(dash_or_arg.to_string())),
-                );
-
-                completions.extend(longs_and_visible_aliases(cmd));
-                completions.extend(hidden_longs_aliases(cmd));
-            } else if arg.is_escape() {
-                // HACK: Assuming knowledge of is_escape
-                completions.extend(longs_and_visible_aliases(cmd));
-                completions.extend(hidden_longs_aliases(cmd));
-            } else if let Some((flag, value)) = arg.to_long() {
-                if let Ok(flag) = flag {
-                    if let Some(value) = value {
-                        if let Some(arg) = cmd.get_arguments().find(|a| a.get_long() == Some(flag))
-                        {
-                            completions.extend(
-                                complete_arg_value(value.to_str().ok_or(value), arg, current_dir)
-                                    .into_iter()
-                                    .map(|comp| comp.add_prefix(format!("--{flag}="))),
-                            );
-                        }
-                    } else {
-                        completions.extend(longs_and_visible_aliases(cmd).into_iter().filter(
-                            |comp| comp.get_value().starts_with(format!("--{flag}").as_str()),
-                        ));
-                        completions.extend(hidden_longs_aliases(cmd).into_iter().filter(|comp| {
-                            comp.get_value().starts_with(format!("--{flag}").as_str())
-                        }));
-                    }
-                }
-            } else if let Some(short) = arg.to_short() {
-                if !short.is_negative_number() {
-                    // Find the first takes_values option.
-                    let (leading_flags, takes_value_opt, mut short) = parse_shortflags(cmd, short);
-
-                    // Clone `short` to `peek_short` to peek whether the next flag is a `=`.
-                    if let Some(opt) = takes_value_opt {
-                        let mut peek_short = short.clone();
-                        let has_equal = if let Some(Ok('=')) = peek_short.next_flag() {
-                            short.next_flag();
-                            true
-                        } else {
-                            false
-                        };
-
-                        let value = short.next_value_os().unwrap_or(OsStr::new(""));
-                        completions.extend(
-                            complete_arg_value(value.to_str().ok_or(value), opt, current_dir)
-                                .into_iter()
-                                .map(|comp| {
-                                    let sep = if has_equal { "=" } else { "" };
-                                    comp.add_prefix(format!("-{leading_flags}{sep}"))
-                                }),
-                        );
-                    } else {
-                        completions.extend(
-                            shorts_and_visible_aliases(cmd)
-                                .into_iter()
-                                .map(|comp| comp.add_prefix(format!("-{leading_flags}"))),
-                        );
-                    }
-                }
-            }
+            completions.extend(complete_option(arg, cmd, current_dir));
         }
-        ParseState::Pos(..) => {
+        ParseState::Pos((_, num_arg)) => {
             if let Some(positional) = cmd
                 .get_positionals()
                 .find(|p| p.get_index() == Some(pos_index))
             {
                 completions.extend(complete_arg_value(arg.to_value(), positional, current_dir));
+                if positional
+                    .get_num_args()
+                    .is_some_and(|num_args| num_arg >= num_args.min_values())
+                {
+                    completions.extend(complete_option(arg, cmd, current_dir));
+                }
             }
         }
         ParseState::Opt((opt, count)) => {
@@ -297,6 +217,104 @@ fn complete_arg(
     Ok(completions)
 }
 
+fn complete_option(
+    arg: &clap_lex::ParsedArg<'_>,
+    cmd: &clap::Command,
+    current_dir: Option<&std::path::Path>,
+) -> Vec<CompletionCandidate> {
+    let mut completions = Vec::<CompletionCandidate>::new();
+    if arg.is_empty() {
+        completions.extend(longs_and_visible_aliases(cmd));
+        completions.extend(hidden_longs_aliases(cmd));
+
+        let dash_or_arg = if arg.is_empty() {
+            "-".into()
+        } else {
+            arg.to_value_os().to_string_lossy()
+        };
+        completions.extend(
+            shorts_and_visible_aliases(cmd)
+                .into_iter()
+                .map(|comp| comp.add_prefix(dash_or_arg.to_string())),
+        );
+    } else if arg.is_stdio() {
+        // HACK: Assuming knowledge of is_stdio
+        let dash_or_arg = if arg.is_empty() {
+            "-".into()
+        } else {
+            arg.to_value_os().to_string_lossy()
+        };
+        completions.extend(
+            shorts_and_visible_aliases(cmd)
+                .into_iter()
+                .map(|comp| comp.add_prefix(dash_or_arg.to_string())),
+        );
+
+        completions.extend(longs_and_visible_aliases(cmd));
+        completions.extend(hidden_longs_aliases(cmd));
+    } else if arg.is_escape() {
+        // HACK: Assuming knowledge of is_escape
+        completions.extend(longs_and_visible_aliases(cmd));
+        completions.extend(hidden_longs_aliases(cmd));
+    } else if let Some((flag, value)) = arg.to_long() {
+        if let Ok(flag) = flag {
+            if let Some(value) = value {
+                if let Some(arg) = cmd.get_arguments().find(|a| a.get_long() == Some(flag)) {
+                    completions.extend(
+                        complete_arg_value(value.to_str().ok_or(value), arg, current_dir)
+                            .into_iter()
+                            .map(|comp| comp.add_prefix(format!("--{flag}="))),
+                    );
+                }
+            } else {
+                completions.extend(
+                    longs_and_visible_aliases(cmd)
+                        .into_iter()
+                        .filter(|comp| comp.get_value().starts_with(format!("--{flag}").as_str())),
+                );
+                completions.extend(
+                    hidden_longs_aliases(cmd)
+                        .into_iter()
+                        .filter(|comp| comp.get_value().starts_with(format!("--{flag}").as_str())),
+                );
+            }
+        }
+    } else if let Some(short) = arg.to_short() {
+        if !short.is_negative_number() {
+            // Find the first takes_values option.
+            let (leading_flags, takes_value_opt, mut short) = parse_shortflags(cmd, short);
+
+            // Clone `short` to `peek_short` to peek whether the next flag is a `=`.
+            if let Some(opt) = takes_value_opt {
+                let mut peek_short = short.clone();
+                let has_equal = if let Some(Ok('=')) = peek_short.next_flag() {
+                    short.next_flag();
+                    true
+                } else {
+                    false
+                };
+
+                let value = short.next_value_os().unwrap_or(OsStr::new(""));
+                completions.extend(
+                    complete_arg_value(value.to_str().ok_or(value), opt, current_dir)
+                        .into_iter()
+                        .map(|comp| {
+                            let sep = if has_equal { "=" } else { "" };
+                            comp.add_prefix(format!("-{leading_flags}{sep}"))
+                        }),
+                );
+            } else {
+                completions.extend(
+                    shorts_and_visible_aliases(cmd)
+                        .into_iter()
+                        .map(|comp| comp.add_prefix(format!("-{leading_flags}"))),
+                );
+            }
+        }
+    }
+    completions
+}
+
 fn complete_arg_value(
     value: Result<&str, &OsStr>,
     arg: &clap::Arg,


Test patch:
diff --git a/clap_complete/tests/testsuite/engine.rs b/clap_complete/tests/testsuite/engine.rs
index a3a18912..09064e90 100644
--- a/clap_complete/tests/testsuite/engine.rs
+++ b/clap_complete/tests/testsuite/engine.rs
@@ -681,13 +681,13 @@ baz
 fn suggest_multi_positional() {
     let mut cmd = Command::new("dynamic")
         .arg(
-            clap::Arg::new("positional")
-                .value_parser(["pos_1, pos_2, pos_3"])
+            clap::Arg::new("positional-1")
+                .value_parser(["pos_1"])
                 .index(1),
         )
         .arg(
             clap::Arg::new("positional-2")
-                .value_parser(["pos_a", "pos_b", "pos_c"])
+                .value_parser(["pos_2_a", "pos_2_b", "pos_2_c"])
                 .index(2)
                 .num_args(3),
         )
@@ -701,27 +701,27 @@ fn suggest_multi_positional() {
     assert_data_eq!(
         complete!(cmd, "pos_1 pos_a [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 "#]]
     );
 
     assert_data_eq!(
         complete!(cmd, "pos_1 pos_a pos_b [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 "#]]
     );
 
     assert_data_eq!(
         complete!(cmd, "--format json pos_1 [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 --format
 --help	Print help
 "#]]
@@ -730,9 +730,9 @@ pos_c
     assert_data_eq!(
         complete!(cmd, "--format json pos_1 pos_a [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 "#]]
     );
 
@@ -747,18 +747,18 @@ pos_c
     assert_data_eq!(
         complete!(cmd, "--format json -- pos_1 pos_a [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 "#]]
     );
 
     assert_data_eq!(
         complete!(cmd, "--format json -- pos_1 pos_a pos_b [TAB]"),
         snapbox::str![[r#"
-pos_a
-pos_b
-pos_c
+pos_2_a
+pos_2_b
+pos_2_c
 "#]]
     );
 
@@ -768,6 +768,69 @@ pos_c
     );
 }
 
+#[test]
+fn suggest_multi_positional_unbounded() {
+    let mut cmd = Command::new("dynamic")
+        .arg(
+            clap::Arg::new("positional")
+                .value_parser(["pos_1", "pos_2"])
+                .index(1)
+                .num_args(2..),
+        )
+        .arg(
+            clap::Arg::new("--format")
+                .long("format")
+                .short('F')
+                .value_parser(["json", "yaml", "toml"]),
+        );
+
+    assert_data_eq!(
+        complete!(cmd, "pos_1 [TAB]"),
+        snapbox::str![[r#"
+pos_1
+pos_2
+"#]]
+    );
+
+    assert_data_eq!(complete!(cmd, "pos_1 --[TAB]"), snapbox::str![""]);
+
+    assert_data_eq!(
+        complete!(cmd, "pos_1 --format [TAB]"),
+        snapbox::str![[r#"
+json
+yaml
+toml
+"#]]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "pos_1 --format json [TAB]"),
+        snapbox::str![[r#"
+pos_1
+pos_2
+--format
+--help	Print help
+"#]]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "pos_1 pos_2 --[TAB]"),
+        snapbox::str![[r#"
+--format
+--help	Print help
+"#]]
+    );
+    assert_data_eq!(
+        complete!(cmd, "pos_1 pos_2 --format json [TAB]"),
+        snapbox::str![[r#"
+pos_1
+pos_2
+--format
+--help	Print help
+"#]]
+    );
+}
+
 #[test]
 fn suggest_delimiter_values() {
     let mut cmd = Command::new("delimiter")