Repository: clap-rs/clap
Pull request #5576 (2024-07-09 12:08:41)

feat(clap_complete): Support `-fbar` and `-f=bar` in native completions
<!--
Thanks for helping out!

Please link the appropriate issue from your PR.

If you don't have an issue, we'd recommend starting with one first so the PR can focus on the
implementation (unless its an obvious bug or documentation fix that will have
little conversation).
-->
related issue: https://github.com/clap-rs/clap/issues/3920

This is a later PR based on https://github.com/clap-rs/clap/pull/5539

### What is left
Error parsing for `--flag=bar` in `bash`
**more details:**
https://github.com/clap-rs/clap/blob/43e73682835653ac48f32cc786514553d697c693/clap_complete/src/dynamic/shells/mod.rs#L36-L48
The `comp_words` will be ['command', '--flag', '=', 'value'] rather than ['command', '--flag=value'] because of `COMP_WORDBREAKS`.



epage: Marked as draft until  #5539 is merged just to be clear that this is blocked on it
epage: > Error parsing for --flag=bar in bash

As the draft status was removed, I'm assuming this isn't a blocker for merge?  Could you give some detail on why?
shannmu: > As the draft status was removed, I'm assuming this isn't a blocker for merge? Could you give some detail on why?

This error is unrelated to the Rust part of the code because it is caused by `COMP WORDBREAKS`. `COMP WORDBREAKS` affects the behavior of bash completion and is a shell-specific thing, whereas the purpose of this PR is to implement shell-agnostic completion feature.
"What is left" is intended to remind us that https://github.com/clap-rs/clap/issues/3920 still has some unresolved issues, and perhaps we should address it as a separate issue.

Detail for `COMP_WORDBREAKS`
> COMP_WORDBREAKS
> The set of characters that the Readline library treats as word separators when performing word completion. If COMP_WORDBREAKS is unset, it loses its special properties, even if it is subsequently reset.

Patch:
diff --git a/clap_complete/src/dynamic/completer.rs b/clap_complete/src/dynamic/completer.rs
index 5f540c3d..fd614cb9 100644
--- a/clap_complete/src/dynamic/completer.rs
+++ b/clap_complete/src/dynamic/completer.rs
@@ -109,47 +109,17 @@ pub fn complete(
             } else {
                 state = ParseState::ValueDone;
             }
-        } else if let Some(mut short) = arg.to_short() {
-            let mut takes_value = false;
-            loop {
-                if let Some(Ok(opt)) = short.next_flag() {
-                    let opt = current_cmd.get_arguments().find(|a| {
-                        let shorts = a.get_short_and_visible_aliases();
-                        let is_find = shorts.map(|v| {
-                            let mut iter = v.into_iter();
-                            let c = iter.find(|c| *c == opt);
-                            c.is_some()
-                        });
-                        is_find.unwrap_or(false)
-                    });
-
-                    state = match opt.map(|o| o.get_action()) {
-                        Some(clap::ArgAction::Set) | Some(clap::ArgAction::Append) => {
-                            takes_value = true;
-                            if short.next_value_os().is_some() {
-                                ParseState::ValueDone
-                            } else {
-                                ParseState::Opt(opt.unwrap().clone())
-                            }
-                        }
-                        Some(clap::ArgAction::SetTrue) | Some(clap::ArgAction::SetFalse) => {
-                            ParseState::ValueDone
-                        }
-                        Some(clap::ArgAction::Count) => ParseState::ValueDone,
-                        Some(clap::ArgAction::Version) => ParseState::ValueDone,
-                        Some(clap::ArgAction::Help)
-                        | Some(clap::ArgAction::HelpShort)
-                        | Some(clap::ArgAction::HelpLong) => ParseState::ValueDone,
+        } else if let Some(short) = arg.to_short() {
+            let (_, takes_value_opt, mut short) = parse_shortflags(current_cmd, short);
+            match takes_value_opt {
+                Some(opt) => {
+                    state = match short.next_value_os() {
                         Some(_) => ParseState::ValueDone,
-                        None => ParseState::ValueDone,
+                        None => ParseState::Opt(opt.clone()),
                     };
-
-                    if takes_value {
-                        break;
-                    }
-                } else {
+                }
+                None => {
                     state = ParseState::ValueDone;
-                    break;
                 }
             }
         } else {
@@ -242,7 +212,7 @@ fn complete_arg(
                 completions.extend(hidden_longs_aliases(cmd));
             }
 
-            if arg.is_empty() || arg.is_stdio() || arg.is_short() {
+            if arg.is_empty() || arg.is_stdio() {
                 let dash_or_arg = if arg.is_empty() {
                     "-".into()
                 } else {
@@ -263,6 +233,50 @@ fn complete_arg(
                             .visible(true)
                         }),
                 );
+            } else if let Some(short) = arg.to_short() {
+                if !short.is_negative_number() {
+                    // Find the first takes_value 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| {
+                                    CompletionCandidate::new(format!(
+                                        "-{}{}{}",
+                                        leading_flags.to_string_lossy(),
+                                        if has_equal { "=" } else { "" },
+                                        comp.get_content().to_string_lossy()
+                                    ))
+                                    .help(comp.get_help().cloned())
+                                    .visible(comp.is_visible())
+                                }),
+                        );
+                    } else {
+                        completions.extend(shorts_and_visible_aliases(cmd).into_iter().map(
+                            |comp| {
+                                CompletionCandidate::new(format!(
+                                    "-{}{}",
+                                    leading_flags.to_string_lossy(),
+                                    comp.get_content().to_string_lossy()
+                                ))
+                                .help(comp.get_help().cloned())
+                                .visible(comp.is_visible())
+                            },
+                        ));
+                    }
+                }
             }
 
             if let Some(positional) = cmd
@@ -521,6 +535,53 @@ fn subcommands(p: &clap::Command) -> Vec<CompletionCandidate> {
         .collect()
 }
 
+/// Parse the short flags and find the first takes_value option.
+fn parse_shortflags<'s>(
+    cmd: &clap::Command,
+    mut short: clap_lex::ShortFlags<'s>,
+) -> (OsString, Option<clap::Arg>, clap_lex::ShortFlags<'s>) {
+    let takes_value_opt;
+    let mut leading_flags = OsString::new();
+    // Find the first takes_value option.
+    loop {
+        match short.next_flag() {
+            Some(Ok(opt)) => {
+                leading_flags.push(opt.to_string());
+                let opt = cmd.get_arguments().find(|a| {
+                    let shorts = a.get_short_and_visible_aliases();
+                    let is_find = shorts.map(|v| {
+                        let mut iter = v.into_iter();
+                        let c = iter.find(|c| *c == opt);
+                        c.is_some()
+                    });
+                    is_find.unwrap_or(false)
+                });
+                match opt.map(|o| o.get_action()) {
+                    Some(clap::ArgAction::Set) | Some(clap::ArgAction::Append) => {
+                        takes_value_opt = opt;
+                        break;
+                    }
+                    Some(clap::ArgAction::SetTrue)
+                    | Some(clap::ArgAction::SetFalse)
+                    | Some(clap::ArgAction::Count)
+                    | Some(clap::ArgAction::Version)
+                    | Some(clap::ArgAction::Help)
+                    | Some(clap::ArgAction::HelpShort)
+                    | Some(clap::ArgAction::HelpLong) => (),
+                    Some(_) => (),
+                    None => (),
+                }
+            }
+            Some(Err(_)) | None => {
+                takes_value_opt = None;
+                break;
+            }
+        }
+    }
+
+    (leading_flags, takes_value_opt.cloned(), short)
+}
+
 /// A completion candidate definition
 ///
 /// This makes it easier to add more fields to completion candidate,
diff --git a/src/_derive/mod.rs b/src/_derive/mod.rs
index b1c0b217..7ef6dd4e 100644
--- a/src/_derive/mod.rs
+++ b/src/_derive/mod.rs
@@ -291,17 +291,15 @@
 //!
 //! `clap` assumes some intent based on the type used:
 //!
-//! | Type                  | Effect                                               | Implies                                                     | Notes |
-//! |-----------------------|------------------------------------------------------|-------------------------------------------------------------|-------|
-//! | `()`                  | user-defined                                         | `.action(ArgAction::Set).required(false)`                   |       |
-//! | `bool`                | flag                                                 | `.action(ArgAction::SetTrue)`                               |       |
-//! | `Option<T>`           | optional argument                                    | `.action(ArgAction::Set).required(false)`                   |       |
-//! | `Option<Option<T>>`   | optional value for optional argument                 | `.action(ArgAction::Set).required(false).num_args(0..=1)`   |       |
-//! | `T`                   | required argument                                    | `.action(ArgAction::Set).required(!has_default)`            |       |
-//! | `Vec<T>`              | `0..` occurrences of argument                        | `.action(ArgAction::Append).required(false)`  |       |
-//! | `Option<Vec<T>>`      | `0..` occurrences of argument                        | `.action(ArgAction::Append).required(false)`  |       |
-//! | `Vec<Vec<T>>`         | `0..` occurrences of argument, grouped by occurrence | `.action(ArgAction::Append).required(false)`  | requires `unstable-v5` |
-//! | `Option<Vec<Vec<T>>>` | `0..` occurrences of argument, grouped by occurrence | `.action(ArgAction::Append).required(false)`  | requires `unstable-v5` |
+//! | Type                | Effect                               | Implies                                                     |
+//! |---------------------|--------------------------------------|-------------------------------------------------------------|
+//! | `()`                | user-defined                         | `.action(ArgAction::Set).required(false)`                   |
+//! | `bool`              | flag                                 | `.action(ArgAction::SetTrue)`                               |
+//! | `Option<T>`         | optional argument                    | `.action(ArgAction::Set).required(false)`                   |
+//! | `Option<Option<T>>` | optional value for optional argument | `.action(ArgAction::Set).required(false).num_args(0..=1)`   |
+//! | `T`                 | required argument                    | `.action(ArgAction::Set).required(!has_default)`            |
+//! | `Vec<T>`            | `0..` occurrences of argument        | `.action(ArgAction::Append).required(false)`  |
+//! | `Option<Vec<T>>`    | `0..` occurrences of argument        | `.action(ArgAction::Append).required(false)`  |
 //!
 //! In addition, [`.value_parser(value_parser!(T))`][crate::value_parser!] is called for each
 //! field.
@@ -311,9 +309,8 @@
 //!   - To force any inferred type (like `Vec<T>`) to be treated as `T`, you can refer to the type
 //!     by another means, like using `std::vec::Vec` instead of `Vec`.  For improving this, see
 //!     [#4626](https://github.com/clap-rs/clap/issues/4626).
-//! - `Option<Vec<T>>` and `Option<Vec<Vec<T>>` will be `None` instead of `vec![]` if no arguments are provided.
+//! - `Option<Vec<T>>` will be `None` instead of `vec![]` if no arguments are provided.
 //!   - This gives the user some flexibility in designing their argument, like with `num_args(0..)`
-//! - `Vec<Vec<T>>` will need [`Arg::num_args`][crate::Arg::num_args] set to be meaningful
 //!
 //! ## Doc Comments
 //!


Test patch:
diff --git a/clap_complete/tests/testsuite/dynamic.rs b/clap_complete/tests/testsuite/dynamic.rs
index 7087a8e0..81057cd5 100644
--- a/clap_complete/tests/testsuite/dynamic.rs
+++ b/clap_complete/tests/testsuite/dynamic.rs
@@ -402,6 +402,41 @@ pos_b
 pos_c"
         ]
     );
+
+    assert_data_eq!(
+        complete!(cmd, "-ci[TAB]", current_dir = Some(testdir_path)),
+        snapbox::str![
+            "-cia_file
+-cib_file
+-cic_dir/
+-cid_dir/"
+        ]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "-ci=[TAB]", current_dir = Some(testdir_path)),
+        snapbox::str![
+            "-ci=a_file
+-ci=b_file
+-ci=c_dir/
+-ci=d_dir/"
+        ]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "-ci=a[TAB]", current_dir = Some(testdir_path)),
+        snapbox::str!["-ci=a_file"]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "-ciF[TAB]", current_dir = Some(testdir_path)),
+        snapbox::str![""]
+    );
+
+    assert_data_eq!(
+        complete!(cmd, "-ciF=[TAB]", current_dir = Some(testdir_path)),
+        snapbox::str![""]
+    )
 }
 
 fn complete(cmd: &mut Command, args: impl AsRef<str>, current_dir: Option<&Path>) -> String {