Repository: clap-rs/clap
Pull request #2191 (2020-10-30 20:41:26)

fix(clap_generate): zsh completion generation panic
This PR fixes a panic that would occur when a zsh completion file was being generated 
for an app that contains global arguments that have conflicts with other arguments which aren't 
present in all the subcommands that the global argument would be applied to.

The added test would panic before the fix.

Only the test added without the fix to observe the problem.
git clone --branch zsh_completion_bug https://github.com/ahkrr/clap.git
cd clap/clap_generate/tests && cargo test

I haven't found an open issue regarding this panic.



Patch:
diff --git a/clap_generate/src/generators/shells/zsh.rs b/clap_generate/src/generators/shells/zsh.rs
index 331c4497..010b73c3 100644
--- a/clap_generate/src/generators/shells/zsh.rs
+++ b/clap_generate/src/generators/shells/zsh.rs
@@ -43,7 +43,7 @@ _{name}() {{
 
 _{name} \"$@\"",
                 name = app.get_bin_name().unwrap(),
-                initial_args = get_args_of(app),
+                initial_args = get_args_of(app, None),
                 subcommands = get_subcommands_of(app),
                 subcommand_details = subcommand_details(app)
             )
@@ -225,7 +225,8 @@ fn get_subcommands_of(p: &App) -> String {
             bin_name,
         );
         let mut v = vec![format!("({})", name)];
-        let subcommand_args = get_args_of(parser_of(p, &*bin_name).expect(INTERNAL_ERROR_MSG));
+        let subcommand_args =
+            get_args_of(parser_of(p, &*bin_name).expect(INTERNAL_ERROR_MSG), Some(p));
 
         if !subcommand_args.is_empty() {
             v.push(subcommand_args);
@@ -299,12 +300,12 @@ fn parser_of<'help, 'app>(p: &'app App<'help>, bin_name: &str) -> Option<&'app A
 //    -C: modify the $context internal variable
 //    -s: Allow stacking of short args (i.e. -a -b -c => -abc)
 //    -S: Do not complete anything after '--' and treat those as argument values
-fn get_args_of(p: &App) -> String {
+fn get_args_of(p: &App, p_global: Option<&App>) -> String {
     debug!("get_args_of");
 
     let mut ret = vec![String::from("_arguments \"${_arguments_options[@]}\" \\")];
-    let opts = write_opts_of(p);
-    let flags = write_flags_of(p);
+    let opts = write_opts_of(p, p_global);
+    let flags = write_flags_of(p, p_global);
     let positionals = write_positionals_of(p);
 
     let sc_or_a = if p.has_subcommands() {
@@ -401,7 +402,7 @@ fn escape_value(string: &str) -> String {
         .replace(" ", "\\ ")
 }
 
-fn write_opts_of(p: &App) -> String {
+fn write_opts_of(p: &App, p_global: Option<&App>) -> String {
     debug!("write_opts_of");
 
     let mut ret = vec![];
@@ -410,7 +411,7 @@ fn write_opts_of(p: &App) -> String {
         debug!("write_opts_of:iter: o={}", o.get_name());
 
         let help = o.get_about().map_or(String::new(), escape_help);
-        let conflicts = arg_conflicts(p, o);
+        let conflicts = arg_conflicts(p, o, p_global);
 
         // @TODO @soundness should probably be either multiple occurrences or multiple values and
         // not both
@@ -475,28 +476,45 @@ fn write_opts_of(p: &App) -> String {
     ret.join("\n")
 }
 
-fn arg_conflicts(app: &App, arg: &Arg) -> String {
-    let conflicts = app.get_arg_conflicts_with(arg);
+fn arg_conflicts(app: &App, arg: &Arg, app_global: Option<&App>) -> String {
+    fn push_conflicts(conflicts: &[&Arg], res: &mut Vec<String>) {
+        for conflict in conflicts {
+            if let Some(s) = conflict.get_short() {
+                res.push(format!("-{}", s));
+            }
 
-    if conflicts.is_empty() {
-        return String::new();
+            if let Some(l) = conflict.get_long() {
+                res.push(format!("--{}", l));
+            }
+        }
     }
 
     let mut res = vec![];
-    for conflict in conflicts {
-        if let Some(s) = conflict.get_short() {
-            res.push(format!("-{}", s));
+    match (app_global, arg.get_global()) {
+        (Some(x), true) => {
+            let conflicts = x.get_arg_conflicts_with(arg);
+
+            if conflicts.is_empty() {
+                return String::new();
+            }
+
+            push_conflicts(&conflicts, &mut res);
         }
+        (_, _) => {
+            let conflicts = app.get_arg_conflicts_with(arg);
+
+            if conflicts.is_empty() {
+                return String::new();
+            }
 
-        if let Some(l) = conflict.get_long() {
-            res.push(format!("--{}", l));
+            push_conflicts(&conflicts, &mut res);
         }
-    }
+    };
 
     format!("({})", res.join(" "))
 }
 
-fn write_flags_of(p: &App) -> String {
+fn write_flags_of(p: &App, p_global: Option<&App>) -> String {
     debug!("write_flags_of;");
 
     let mut ret = vec![];
@@ -505,7 +523,7 @@ fn write_flags_of(p: &App) -> String {
         debug!("write_flags_of:iter: f={}", f.get_name());
 
         let help = f.get_about().map_or(String::new(), escape_help);
-        let conflicts = arg_conflicts(p, &f);
+        let conflicts = arg_conflicts(p, &f, p_global);
 
         let multiple = if f.is_set(ArgSettings::MultipleOccurrences) {
             "*"
diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs
index d92795cc..e6ae738d 100644
--- a/src/build/app/mod.rs
+++ b/src/build/app/mod.rs
@@ -236,23 +236,98 @@ impl<'help> App<'help> {
         self.get_opts().filter(|a| a.get_help_heading().is_none())
     }
 
+    // Get a list of subcommands which contain the provided Argument
+    //
+    // This command will only include subcommands in its list for which the subcommands
+    // parent also contains the Argument.
+    //
+    // This search follows the propagation rules of global arguments.
+    // It is useful to finding subcommands, that have inherited a global argument.
+    //
+    // **NOTE:** In this case only Sucommand_1 will be included
+    //   Subcommand_1 (contains Arg)
+    //     Subcommand_1.1 (doesn't contain Arg)
+    //       Subcommand_1.1.1 (contains Arg)
+    //
+    fn get_subcommands_containing(&self, arg: &Arg) -> Vec<&App<'help>> {
+        let mut vec = std::vec::Vec::new();
+        for idx in 0..self.subcommands.len() {
+            if self.subcommands[idx]
+                .args
+                .args
+                .iter()
+                .any(|ar| ar.id == arg.id)
+            {
+                vec.push(&self.subcommands[idx]);
+                // TODO rustc >= 1.46: change  .get(dix).unwrap()  to [idx]
+                // improved readability
+                vec.append(
+                    &mut self
+                        .subcommands
+                        .get(idx)
+                        .unwrap()
+                        .get_subcommands_containing(arg),
+                );
+            }
+        }
+        vec
+    }
+
+    // Get a unique list of all arguments of all commands and continuous subcommands the given argument conflicts with.
+    //
+    // This behavior follows the propagation rules of global arguments.
+    // It is useful for finding conflicts for arguments declared as global.
+    //
+    // ### Panics
+    //
+    // If the given arg contains a conflict with an argument that is unknown to
+    // this `App`.
+    fn get_global_arg_conflicts_with(&self, arg: &Arg) -> Vec<&Arg<'help>> // FIXME: This could probably have been an iterator
+    {
+        arg.blacklist
+            .iter()
+            .map(|id| {
+                self.args
+                    .args
+                    .iter()
+                    .chain(
+                        self.get_subcommands_containing(arg)
+                            .iter()
+                            .flat_map(|x| x.args.args.iter()),
+                    )
+                    .find(|arg| arg.id == *id)
+                    .expect(
+                        "App::get_arg_conflicts_with: \
+                    The passed arg conflicts with an arg unknown to the app",
+                    )
+            })
+            .collect()
+    }
+
     /// Get a list of all arguments the given argument conflicts with.
     ///
+    /// If the provided argument is declared as global, the conflicts will be determined
+    /// based on the propagation rules of global arguments.
+    ///
     /// ### Panics
     ///
     /// If the given arg contains a conflict with an argument that is unknown to
     /// this `App`.
     pub fn get_arg_conflicts_with(&self, arg: &Arg) -> Vec<&Arg<'help>> // FIXME: This could probably have been an iterator
     {
-        arg.blacklist
-            .iter()
-            .map(|id| {
-                self.args.args.iter().find(|arg| arg.id == *id).expect(
-                    "App::get_arg_conflicts_with: \
+        if arg.global {
+            self.get_global_arg_conflicts_with(arg)
+        } else {
+            arg.blacklist
+                .iter()
+                .map(|id| {
+                    self.args.args.iter().find(|arg| arg.id == *id).expect(
+                        "App::get_arg_conflicts_with: \
                     The passed arg conflicts with an arg unknown to the app",
-                )
-            })
-            .collect()
+                    )
+                })
+                .collect()
+        }
     }
 
     /// Returns `true` if the given [`AppSettings`] variant is currently set in
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 3b8b8680..4fa3cd07 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -172,6 +172,11 @@ impl<'help> Arg<'help> {
     pub fn get_value_hint(&self) -> ValueHint {
         self.value_hint
     }
+
+    /// Get information on if this argument is global or not
+    pub fn get_global(&self) -> bool {
+        self.global
+    }
 }
 
 impl<'help> Arg<'help> {


Test patch:
diff --git a/clap_generate/tests/generate_completions.rs b/clap_generate/tests/generate_completions.rs
new file mode 100644
index 00000000..32b2c47c
--- /dev/null
+++ b/clap_generate/tests/generate_completions.rs
@@ -0,0 +1,26 @@
+use clap::{App, Arg};
+use clap_generate::{generate, generators::*};
+use std::io;
+
+#[test]
+fn generate_completions() {
+    let mut app = App::new("test_app")
+        .arg(
+            Arg::new("config")
+                .short('c')
+                .conflicts_with("v")
+                .global(true),
+        )
+        .arg(Arg::new("v").short('v'))
+        .subcommand(
+            App::new("test")
+                .about("Subcommand")
+                .arg(Arg::new("debug").short('d')),
+        );
+
+    generate::<Bash, _>(&mut app, "test_app", &mut io::sink());
+    generate::<Fish, _>(&mut app, "test_app", &mut io::sink());
+    generate::<PowerShell, _>(&mut app, "test_app", &mut io::sink());
+    generate::<Elvish, _>(&mut app, "test_app", &mut io::sink());
+    generate::<Zsh, _>(&mut app, "test_app", &mut io::sink());
+}