Lazy subcommand propagation, avoid redundant _build() function call

rustfmt and clippy

Indepth cleaning

Apply suggestions

clap_generate/src/generators/mod.rs

@@ -117,7 +117,7 @@ pub trait Generator {
 
     /// Gets all the short options, their visible aliases and flags of a [`clap::App`](../clap/struct.App.html).
     /// Includes `h` and `V` depending on the [`clap::AppSettings`](../clap/enum.AppSettings.html).
-    fn shorts_and_visible_aliases<'help>(p: &App<'help>) -> Vec<char> {
+    fn shorts_and_visible_aliases(p: &App) -> Vec<char> {
         debug!("shorts: name={}", p.get_name());
 
         let mut shorts_and_visible_aliases: Vec<char> = p
@@ -155,7 +155,7 @@ pub trait Generator {
 
     /// Gets all the long options and flags of a [`clap::App`](../clap/struct.App.html).
     /// Includes `help` and `version` depending on the [`clap::AppSettings`](../clap/enum.AppSettings.html).
-    fn longs<'help>(p: &App<'help>) -> Vec<String> {
+    fn longs(p: &App) -> Vec<String> {
         debug!("longs: name={}", p.get_name());
 
         let mut longs: Vec<String> = p

clap_generate/src/lib.rs

@@ -181,10 +181,8 @@ where
 {
     app.set_bin_name(bin_name);
 
-    if !app.is_set(clap::AppSettings::Built) {
+    app._build();
-        app._build();
+    app._build_bin_names();
-        app._build_bin_names();
-    }
 
     G::generate(app, buf)
 }

src/build/app/mod.rs

@@ -31,17 +31,6 @@ use crate::{
     Result as ClapResult, INTERNAL_ERROR_MSG,
 };
 
-// @TODO FIXME (@CreepySkeleton): some of these variants (None) are never constructed
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub(crate) enum Propagation {
-    To(Id),
-    Full,
-    #[cfg_attr(not(test), allow(unused))]
-    NextLevel,
-    #[allow(unused)]
-    None,
-}
-
 /// Represents a command line interface which is made up of all possible
 /// command line arguments and subcommands. Interface arguments and settings are
 /// configured using the "builder pattern." Once all configuration is complete,
@@ -1958,9 +1947,7 @@ impl<'help> App<'help> {
     pub fn generate_usage(&mut self) -> String {
         // If there are global arguments, or settings we need to propagate them down to subcommands
         // before parsing incase we run into a subcommand
-        if !self.settings.is_set(AppSettings::Built) {
+        self._build();
-            self._build();
-        }
 
         let mut parser = Parser::new(self);
         parser._build();
@@ -2267,9 +2254,7 @@ impl<'help> App<'help> {
 
         // If there are global arguments, or settings we need to propagate them down to subcommands
         // before parsing in case we run into a subcommand
-        if !self.settings.is_set(AppSettings::Built) {
+        self._build();
-            self._build();
-        }
 
         // do the real parsing
         let mut parser = Parser::new(self);
@@ -2292,47 +2277,50 @@ impl<'help> App<'help> {
     #[doc(hidden)]
     pub fn _build(&mut self) {
         debug!("App::_build");
+        if !self.settings.is_set(AppSettings::Built) {
+            // Make sure all the globally set flags apply to us as well
+            self.settings = self.settings | self.g_settings;
 
-        // Make sure all the globally set flags apply to us as well
+            self._propagate();
-        self.settings = self.settings | self.g_settings;
 
-        self._propagate(Propagation::Full);
+            self._derive_display_order();
+            self._create_help_and_version();
 
-        self._derive_display_order();
+            let mut pos_counter = 1;
-        self._create_help_and_version();
+            for a in self.args.args.iter_mut() {
+                // Fill in the groups
+                for g in &a.groups {
+                    if let Some(ag) = self.groups.iter_mut().find(|grp| grp.id == *g) {
+                        ag.args.push(a.id.clone());
+                    } else {
+                        let mut ag = ArgGroup::with_id(g.clone());
+                        ag.args.push(a.id.clone());
+                        self.groups.push(ag);
+                    }
+                }
 
-        let mut pos_counter = 1;
+                // Figure out implied settings
-        for a in self.args.args.iter_mut() {
+                if a.is_set(ArgSettings::Last) {
-            // Fill in the groups
+                    // if an arg has `Last` set, we need to imply DontCollapseArgsInUsage so that args
-            for g in &a.groups {
+                    // in the usage string don't get confused or left out.
-                if let Some(ag) = self.groups.iter_mut().find(|grp| grp.id == *g) {
+                    self.settings.set(AppSettings::DontCollapseArgsInUsage);
-                    ag.args.push(a.id.clone());
+                    self.settings.set(AppSettings::ContainsLast);
-                } else {
+                }
-                    let mut ag = ArgGroup::with_id(g.clone());
+                a._build();
-                    ag.args.push(a.id.clone());
+                if a.short.is_none() && a.long.is_none() && a.index.is_none() {
-                    self.groups.push(ag);
+                    a.index = Some(pos_counter);
+                    pos_counter += 1;
                 }
             }
 
-            // Figure out implied settings
+            self.args._build();
-            if a.is_set(ArgSettings::Last) {
+
-                // if an arg has `Last` set, we need to imply DontCollapseArgsInUsage so that args
+            #[cfg(debug_assertions)]
-                // in the usage string don't get confused or left out.
+            self::debug_asserts::assert_app(self);
-                self.settings.set(AppSettings::DontCollapseArgsInUsage);
+            self.settings.set(AppSettings::Built);
-                self.settings.set(AppSettings::ContainsLast);
+        } else {
-            }
+            debug!("App::_build: already built");
-            a._build();
-            if a.short.is_none() && a.long.is_none() && a.index.is_none() {
-                a.index = Some(pos_counter);
-                pos_counter += 1;
-            }
         }
-
-        self.args._build();
-        self.settings.set(AppSettings::Built);
-
-        #[cfg(debug_assertions)]
-        self::debug_asserts::assert_app(self);
     }
 
     fn _panic_on_missing_help(&self, help_required_globally: bool) {
@@ -2376,7 +2364,8 @@ impl<'help> App<'help> {
         two_elements_of(self.groups.iter().filter(|a| condition(a)))
     }
 
-    pub(crate) fn _propagate(&mut self, prop: Propagation) {
+    /// Propagate one and only one level.
+    pub(crate) fn _propagate(&mut self) {
         macro_rules! propagate_subcmd {
             ($_self:expr, $sc:expr) => {{
                 // We have to create a new scope in order to tell rustc the borrow of `sc` is
@@ -2418,24 +2407,8 @@ impl<'help> App<'help> {
 
         debug!("App::_propagate:{}", self.name);
 
-        match prop {
+        for sc in &mut self.subcommands {
-            Propagation::NextLevel | Propagation::Full => {
+            propagate_subcmd!(self, sc);
-                for sc in &mut self.subcommands {
-                    propagate_subcmd!(self, sc);
-                    if prop == Propagation::Full {
-                        sc._propagate(prop.clone());
-                    }
-                }
-            }
-            Propagation::To(id) => {
-                let mut sc = self
-                    .subcommands
-                    .iter_mut()
-                    .find(|sc| sc.id == id)
-                    .expect(INTERNAL_ERROR_MSG);
-                propagate_subcmd!(self, sc);
-            }
-            Propagation::None => (),
         }
     }
 

src/build/app/tests.rs

@@ -1,4 +1,4 @@
-use crate::{build::app::Propagation, App, AppSettings};
+use crate::{App, AppSettings};
 
 #[test]
 fn global_version() {
@@ -6,7 +6,7 @@ fn global_version() {
         .setting(AppSettings::GlobalVersion)
         .version("1.1")
         .subcommand(App::new("sub1"));
-    app._propagate(Propagation::NextLevel);
+    app._propagate();
     assert_eq!(app.subcommands[0].version, Some("1.1"));
 }
 
@@ -15,7 +15,7 @@ fn global_setting() {
     let mut app = App::new("test")
         .global_setting(AppSettings::ColoredHelp)
         .subcommand(App::new("subcmd"));
-    app._propagate(Propagation::NextLevel);
+    app._propagate();
     assert!(app
         .subcommands
         .iter()
@@ -30,7 +30,7 @@ fn global_settings() {
         .global_setting(AppSettings::ColoredHelp)
         .global_setting(AppSettings::TrailingVarArg)
         .subcommand(App::new("subcmd"));
-    app._propagate(Propagation::NextLevel);
+    app._propagate();
     assert!(app
         .subcommands
         .iter()

src/build/usage_parser.rs

@@ -44,9 +44,11 @@ impl<'help> UsageParser<'help> {
 
     pub(crate) fn parse(mut self) -> Arg<'help> {
         debug!("UsageParser::parse");
-        let mut arg = Arg::default();
+        let mut arg = Arg {
-        arg.disp_ord = 999;
+            disp_ord: 999,
-        arg.unified_ord = 999;
+            unified_ord: 999,
+            ..Default::default()
+        };
         loop {
             debug!("UsageParser::parse:iter: pos={}", self.pos);
             self.stop_at(token);

src/parse/parser.rs

@@ -6,7 +6,6 @@ use std::{
 
 // Internal
 use crate::{
-    build::app::Propagation,
     build::AppSettings as AS,
     build::{App, Arg, ArgSettings},
     mkeymap::KeyType,
@@ -865,9 +864,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                     help_help = true;
                     break; // Maybe?
                 }
-                if let Some(id) = sc.find_subcommand(cmd).map(|x| x.id.clone()) {
-                    sc._propagate(Propagation::To(id));
-                }
 
                 if let Some(mut c) = sc.find_subcommand(cmd).cloned() {
                     c._build();
@@ -975,11 +971,6 @@ impl<'help, 'app> Parser<'help, 'app> {
             }
         }
 
-        if let Some(x) = self.app.find_subcommand(sc_name) {
-            let id = x.id.clone();
-            self.app._propagate(Propagation::To(id));
-        }
-
         if let Some(sc) = self.app.subcommands.iter_mut().find(|s| s.name == sc_name) {
             let mut sc_matcher = ArgMatcher::default();
             // Display subcommand name, short and long in usage