Merge pull request #382 from pacak/complete3

Completion: move careful handling of mix of positional and non-positional items

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 {

src/complete_gen.rs

@@ -166,14 +166,15 @@ 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,
                 group: None,
                 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);
+                    }
                 }
             }
         }

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);
                 }

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());
+            }
         }
     }