Repository: pacak/bpaf
Pull request #382 (2024-08-01 13:48:16)

Completion: move careful handling of mix of positional and non-positional items
Extracted from #337

- track shell completions origin - named or positional and use similar to other completions
- when combining completions from two branches - use both or use one that captured earlier value


Patch:
diff --git a/src/args.rs b/src/args.rs
index 78ed79e..f567ad4 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -55,6 +55,11 @@ impl Args<'_> {
     ///     .unwrap_stdout();
     /// assert_eq!(r, "-f");
     /// ```
+    ///
+    /// Note to self: shell passes "" as a parameter in situations like foo `--bar TAB`, bpaf
+    /// completion stubs adopt this conventions add pass it along. This is needed so completer can
+    /// tell the difference between `--bar` being completed or an argument to it in the example
+    /// above.
     #[cfg(feature = "autocomplete")]
     #[must_use]
     pub fn set_comp(mut self, rev: usize) -> Self {
diff --git a/src/complete_gen.rs b/src/complete_gen.rs
index 10ee608..3fdd004 100644
--- a/src/complete_gen.rs
+++ b/src/complete_gen.rs
@@ -166,7 +166,7 @@ impl State {
 }
 
 impl Complete {
-    pub(crate) fn push_shell(&mut self, op: ShellComp, depth: usize) {
+    pub(crate) fn push_shell(&mut self, op: ShellComp, is_argument: bool, depth: usize) {
         self.comps.push(Comp::Shell {
             extra: CompExtra {
                 depth,
@@ -174,6 +174,7 @@ impl Complete {
                 help: None,
             },
             script: op,
+            is_argument,
         });
     }
 
@@ -224,10 +225,7 @@ pub(crate) struct CompExtra {
 #[derive(Clone, Debug)]
 pub(crate) enum Comp {
     /// short or long flag
-    Flag {
-        extra: CompExtra,
-        name: ShortLong,
-    },
+    Flag { extra: CompExtra, name: ShortLong },
 
     /// argument + metadata
     Argument {
@@ -256,12 +254,15 @@ pub(crate) enum Comp {
     Metavariable {
         extra: CompExtra,
         meta: &'static str,
+        /// AKA not positional
         is_argument: bool,
     },
 
     Shell {
         extra: CompExtra,
         script: ShellComp,
+        /// AKA not positional
+        is_argument: bool,
     },
 }
 
@@ -348,6 +349,9 @@ fn pair_to_os_string<'a>(pair: (&'a Arg, &'a OsStr)) -> Option<(&'a Arg, &'a str
     Some((pair.0, pair.1.to_str()?))
 }
 
+/// What is the preceeding item, if any
+///
+/// Mostly is there to tell if we are trying to complete and argument or not...
 #[derive(Debug, Copy, Clone)]
 enum Prefix<'a> {
     NA,
@@ -374,7 +378,7 @@ impl State {
         // try get a current item to complete - must be non-virtual right most one
         // value must be present here, and can fail only for non-utf8 values
         // can't do much completing with non-utf8 values since bpaf needs to print them to stdout
-        let (_, lit) = items.next()?;
+        let (cur, lit) = items.next()?;
 
         // For cases like "-k=val", "-kval", "--key=val", "--key val"
         // last value is going  to be either Arg::Word or Arg::ArgWord
@@ -389,13 +393,18 @@ impl State {
             _ => (false, lit),
         };
 
+        let is_named = match cur {
+            Arg::Short(_, _, _) | Arg::Long(_, _, _) => true,
+            Arg::ArgWord(_) | Arg::Word(_) | Arg::PosWord(_) => false,
+        };
+
         let prefix = match preceeding {
             Some((Arg::Short(s, true, _os), _lit)) => Prefix::Short(*s),
             Some((Arg::Long(l, true, _os), _lit)) => Prefix::Long(l.as_str()),
             _ => Prefix::NA,
         };
 
-        let (items, shell) = comp.complete(lit, pos_only, prefix);
+        let (items, shell) = comp.complete(lit, pos_only, is_named, prefix);
 
         Some(match comp.output_rev {
             0 => render_test(&items, &shell, full_lit),
@@ -480,10 +489,9 @@ impl Comp {
     fn only_value(&self) -> bool {
         match self {
             Comp::Flag { .. } | Comp::Argument { .. } | Comp::Command { .. } => false,
-            Comp::Metavariable { is_argument, .. } | Comp::Value { is_argument, .. } => {
-                *is_argument
-            }
-            Comp::Shell { .. } => true,
+            Comp::Metavariable { is_argument, .. }
+            | Comp::Value { is_argument, .. }
+            | Comp::Shell { is_argument, .. } => *is_argument,
         }
     }
     fn is_pos(&self) -> bool {
@@ -500,6 +508,7 @@ impl Complete {
         &self,
         arg: &str,
         pos_only: bool,
+        is_named: bool,
         prefix: Prefix,
     ) -> (Vec<ShowComp>, Vec<ShellComp>) {
         let mut items: Vec<ShowComp> = Vec::new();
@@ -588,7 +597,9 @@ impl Complete {
                 }
 
                 Comp::Shell { script, .. } => {
-                    shell.push(*script);
+                    if !is_named {
+                        shell.push(*script);
+                    }
                 }
             }
         }
diff --git a/src/complete_shell.rs b/src/complete_shell.rs
index 91a83df..d009b81 100644
--- a/src/complete_shell.rs
+++ b/src/complete_shell.rs
@@ -97,8 +97,8 @@ where
         let depth = args.depth();
         if let Some(comp) = args.comp_mut() {
             for ci in comp_items {
-                if ci.is_metavar().is_some() {
-                    comp.push_shell(self.op, depth);
+                if let Some(is_argument) = ci.is_metavar() {
+                    comp.push_shell(self.op, is_argument, depth);
                 } else {
                     comp.push_comp(ci);
                 }
diff --git a/src/structs.rs b/src/structs.rs
index baf59fc..695d7ec 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -368,29 +368,54 @@ fn this_or_that_picks_first(
     };
 
     #[cfg(feature = "autocomplete")]
-    let len_a = args_a.len();
-
-    #[cfg(feature = "autocomplete")]
-    let len_b = args_b.len();
-
-    #[cfg(feature = "autocomplete")]
-    if let (Some(a), Some(b)) = (args_a.comp_mut(), args_b.comp_mut()) {
-        // if both parsers managed to consume the same amount - including 0, keep
-        // results from both, otherwise keep results from one that consumed more
-        let (keep_a, keep_b) = match res {
-            Ok((true, _)) => (true, false),
-            Ok((false, _)) => (false, true),
-            Err(_) => match len_a.cmp(&len_b) {
-                std::cmp::Ordering::Less => (true, false),
-                std::cmp::Ordering::Equal => (true, true),
-                std::cmp::Ordering::Greater => (false, true),
-            },
-        };
-        if keep_a {
-            comp_stash.extend(a.drain_comps());
+    {
+        let mut keep_a = true;
+        let mut keep_b = true;
+        if args_a.len() != args_b.len() {
+            // If neither parser consumed anything - both can produce valid completions, otherwise
+            // look for the first "significant" consume and keep that parser
+            //
+            // This is needed to preserve completion from a choice between a positional and a flag
+            // See https://github.com/pacak/bpaf/issues/303 for more details
+            if let (Some(_), Some(_)) = (args_a.comp_mut(), args_b.comp_mut()) {
+                'check: for (ix, arg) in args_a.items.iter().enumerate() {
+                    // During completion process named and unnamed arguments behave
+                    // different - `-` and `--` are positional arguments, but we want to produce
+                    // named items too. An empty string is also a special type of item that
+                    // gets passed when user starts completion without passing any actual data.
+                    //
+                    // All other strings are either valid named items or valid positional items
+                    // those are hopefully follow the right logic for being parsed/not parsed
+                    if ix + 1 == args_a.items.len() {
+                        let os = arg.os_str();
+                        if os.is_empty() || os == "-" || os == "--" {
+                            break 'check;
+                        }
+                    }
+                    if let (Some(a), Some(b)) = (args_a.present(ix), args_b.present(ix)) {
+                        match (a, b) {
+                            (false, true) => {
+                                keep_b = false;
+                                break 'check;
+                            }
+                            (true, false) => {
+                                keep_a = false;
+                                break 'check;
+                            }
+                            _ => {}
+                        }
+                    }
+                }
+            }
         }
-        if keep_b {
-            comp_stash.extend(b.drain_comps());
+
+        if let (Some(a), Some(b)) = (args_a.comp_mut(), args_b.comp_mut()) {
+            if keep_a {
+                comp_stash.extend(a.drain_comps());
+            }
+            if keep_b {
+                comp_stash.extend(b.drain_comps());
+            }
         }
     }
 


Test patch:
diff --git a/tests/chezmoi.rs b/tests/chezmoi.rs
new file mode 100644
index 0000000..411a156
--- /dev/null
+++ b/tests/chezmoi.rs
@@ -0,0 +1,231 @@
+use bpaf::*;
+use std::{path::PathBuf, str::FromStr};
+
+#[derive(Copy, Clone, Debug)]
+pub enum Style {
+    /// Program is in PATH
+    InPath,
+    /// Program is in .utils of chezmoi source state
+    InSrc,
+}
+
+impl FromStr for Style {
+    type Err = &'static str;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "path" => Ok(Style::InPath),
+            "src" => Ok(Style::InSrc),
+            _ => Err("Not valid"),
+        }
+    }
+}
+
+/// Parser for `--style`
+fn style() -> impl Parser<Style> {
+    const DEFAULT: Style = Style::InPath;
+
+    fn complete_fn(input: &String) -> Vec<(&'static str, Option<&'static str>)> {
+        [("path", Some("Is path")), ("src", Some("Is src"))]
+            .into_iter()
+            .filter(|(name, _)| name.starts_with(input))
+            .collect()
+    }
+
+    short('t')
+        .long("style")
+        .help("help message for style")
+        .argument::<String>("STYLE")
+        .complete(complete_fn)
+        .parse(|x| x.parse())
+        .fallback(DEFAULT)
+}
+
+#[derive(Debug, Bpaf)]
+#[bpaf(options, version)]
+pub enum Options {
+    /// Process a single file (containing settings).
+    Process(#[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))] PathBuf),
+    Add {
+        /// Add a file
+        #[bpaf(short('a'), long("add"))]
+        _a: (),
+        #[bpaf(external)]
+        style: Style,
+        #[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))]
+        files: Vec<PathBuf>,
+    },
+    Smart {
+        /// Smartly add a file
+        #[bpaf(short('s'), long("smart-add"))]
+        _a: (),
+        #[bpaf(external)]
+        style: Style,
+        #[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))]
+        files: Vec<PathBuf>,
+    },
+    Doctor {
+        /// Perform environment sanity check
+        #[bpaf(long("doctor"))]
+        _a: (),
+    },
+    Update {
+        /// Perform self update
+        #[bpaf(short('u'), long("upgrade"))]
+        _a: (),
+        /// Do not ask for confirmation before applying updates
+        #[bpaf(long("no-confirm"))]
+        no_confirm: bool,
+    },
+}
+
+#[test]
+fn completion_test_1() {
+    let parser = options();
+
+    let r = parser
+        .run_inner(Args::from(&[""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+
+    let expected = "\
+--add\t--add\t\tAdd a file
+--style\t--style=STYLE\t\thelp message for style
+--smart-add\t--smart-add\t\tSmartly add a file
+--style\t--style=STYLE\t\thelp message for style
+--doctor\t--doctor\t\tPerform environment sanity check
+--upgrade\t--upgrade\t\tPerform self update
+--no-confirm\t--no-confirm\t\tDo not ask for confirmation before applying updates
+
+File { mask: None }
+File { mask: None }
+File { mask: None }
+";
+    assert_eq!(r, expected);
+
+    let r = parser
+        .run_inner(Args::from(&["--"]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    assert_eq!(r, expected);
+
+    let r = parser
+        .run_inner(Args::from(&["--s"]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+
+    // style is valid in two different branches, as far as bpaf is concerned they might be
+    // different. Hopefully shell can deduplicate it?
+    let expected = "\
+--style\t--style=STYLE\t\thelp message for style
+--smart-add\t--smart-add\t\tSmartly add a file
+--style\t--style=STYLE\t\thelp message for style
+
+";
+    assert_eq!(r, expected);
+
+    let r = parser
+        .run_inner(Args::from(&["--smart-add", ""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    let expected = "\
+--style\t--style=STYLE\t\thelp message for style
+
+File { mask: None }
+";
+    assert_eq!(r, expected);
+}
+
+#[test]
+fn completion_test_2() {
+    #[derive(Debug, Bpaf, Clone)]
+    #[bpaf(options, version)]
+    pub enum Options {
+        /// Process a single file (containing settings).
+        Process(#[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))] PathBuf),
+
+        /// Perform environment sanity check
+        #[bpaf(long("doctor"))]
+        Doctor,
+    }
+
+    let parser = options();
+
+    let r = parser
+        .run_inner(Args::from(&["-"]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+
+    let expected = "\
+--doctor\t--doctor\t\tPerform environment sanity check
+
+File { mask: None }
+";
+    assert_eq!(r, expected);
+}
+
+#[test]
+fn completion_test_3() {
+    #[derive(Debug, Bpaf, Clone)]
+    #[bpaf(options, version)]
+    pub enum Options {
+        /// Process a single file (containing settings).
+        Process(#[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))] PathBuf),
+
+        /// Perform environment sanity check
+        #[bpaf(long("doctor"))]
+        Doctor,
+
+        /// Print docs
+        #[bpaf(long("document"))]
+        Doc,
+    }
+
+    let parser = options();
+
+    let r = parser
+        .run_inner(Args::from(&["--"]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+
+    let expected = "\
+--doctor\t--doctor\t\tPerform environment sanity check
+--document\t--document\t\tPrint docs
+
+File { mask: None }
+";
+    assert_eq!(r, expected);
+}
+
+#[test]
+fn completion_test_4() {
+    #[derive(Debug, Bpaf, Clone)]
+    #[bpaf(options, version)]
+    pub enum Options {
+        /// Process a single file (containing settings).
+        Process(#[bpaf(positional("FILE"), complete_shell(ShellComp::File{mask: None}))] PathBuf),
+
+        /// Perform environment sanity check
+        #[bpaf(long("doctor"))]
+        Doctor,
+
+        /// Print docs
+        #[bpaf(long("document"))]
+        Doc,
+    }
+
+    let parser = options();
+
+    let r = parser
+        .run_inner(Args::from(&["--d"]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+
+    let expected = "\
+--doctor\t--doctor\t\tPerform environment sanity check
+--document\t--document\t\tPrint docs
+
+";
+
+    assert_eq!(r, expected);
+}
diff --git a/tests/completion.rs b/tests/completion.rs
index 6a7ab1d..d4caf76 100644
--- a/tests/completion.rs
+++ b/tests/completion.rs
@@ -1,6 +1,63 @@
 #![allow(clippy::ptr_arg)]
 use bpaf::*;
 
+#[test]
+fn mixing_shell_and_positional_1_flag_or_pos() {
+    let arg = || short('b').help("Option b").req_flag(10);
+    let pos = || {
+        positional::<String>("DISTANCE")
+            .complete_shell(ShellComp::File { mask: None })
+            .guard(|s| !s.is_empty(), "unreachable")
+            .parse(|s| s.parse::<usize>())
+    };
+
+    let r = construct!([arg(), pos()])
+        .to_options()
+        .run_inner(Args::from(&[""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    assert_eq!(r, "-b\t-b\t\tOption b\n\nFile { mask: None }\n");
+}
+
+#[test]
+fn mixing_shell_and_positional_2_arg_or_pos() {
+    let arg = || short('b').help("Option b").argument::<usize>("HELLO");
+    let pos = || positional::<usize>("DISTANCE").complete_shell(ShellComp::File { mask: None });
+
+    let r = construct!([arg(), pos()])
+        .to_options()
+        .run_inner(Args::from(&[""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    assert_eq!(r, "-b\t-b=HELLO\t\tOption b\n\nFile { mask: None }\n");
+}
+
+#[test]
+fn mixing_shell_and_positional_3_flag_and_pos() {
+    let arg = || short('b').help("Option b").req_flag(10);
+    let pos = || positional::<usize>("DISTANCE").complete_shell(ShellComp::File { mask: None });
+
+    let r1 = construct!(arg(), pos())
+        .to_options()
+        .run_inner(Args::from(&[""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    assert_eq!(r1, "-b\t-b\t\tOption b\n\nFile { mask: None }\n");
+}
+
+#[test]
+fn mixing_shell_and_positional_4_arg_and_pos() {
+    let arg = || short('b').help("Option b").argument::<usize>("HELLO");
+    let pos = || positional::<usize>("DISTANCE").complete_shell(ShellComp::File { mask: None });
+
+    let r = construct!(arg(), pos())
+        .to_options()
+        .run_inner(Args::from(&[""]).set_comp(0))
+        .unwrap_err()
+        .unwrap_stdout();
+    assert_eq!(r, "-b\t-b=HELLO\t\tOption b\n\nFile { mask: None }\n");
+}
+
 #[test]
 fn static_complete_test_1() {
     let a = short('a').long("avocado").help("Use avocado").switch();