Infinite recursion bug (#1316)

fix a bug that led to infinite recursion on subcommand option groups
with fallthrough enabled.

FIxes #1315

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: phlptp

README.md

@@ -1077,11 +1077,13 @@ option_groups. These are:
   along with the parent for subcommands with fallthrough.
 - `.get_option_no_throw(name)`: Get an option pointer by option name. This
   function will return a `nullptr` instead of throwing if the option is not
-  available.
+  available. This method will not search parents of option_options or nameless
+  subcommands regardless of fallthrough status 🚧, this behavior is slightly
+  different from `get_option`.
 - `.get_options(filter)`: Get the list of all defined option pointers (useful
   for processing the app for custom output formats). If used on a subcommand
   will also get options that are in the parent app if the subcommand has
-  fallthrough.
+  fallthrough (and is not nameless 🚧).
 - `.parse_order()`: Get the list of option pointers in the order they were
   parsed (including duplicates).
 - `.formatter(std::shared_ptr<formatterBase> fmt)`: Set a custom formatter for

include/CLI/App.hpp

@@ -776,6 +776,7 @@ class App {
             throw IncorrectConstruction("option group names may not contain newlines or null characters");
         }
         auto option_group = std::make_shared<T>(std::move(group_description), group_name, this);
+        option_group->fallthrough(false);
         auto *ptr = option_group.get();
         // move to App_p for overload resolution on older gcc versions
         App_p app_ptr = std::static_pointer_cast<App>(option_group);
@@ -1139,22 +1140,10 @@ class App {
     CLI11_NODISCARD const Option *get_option_no_throw(std::string option_name) const noexcept;
 
     /// Get an option by name
-    CLI11_NODISCARD const Option *get_option(std::string option_name) const {
-        const auto *opt = get_option_no_throw(option_name);
-        if(opt == nullptr) {
-            throw OptionNotFound(option_name);
-        }
-        return opt;
-    }
+    CLI11_NODISCARD const Option *get_option(std::string option_name) const;
 
     /// Get an option by name (non-const version)
-    CLI11_NODISCARD Option *get_option(std::string option_name) {
-        auto *opt = get_option_no_throw(option_name);
-        if(opt == nullptr) {
-            throw OptionNotFound(option_name);
-        }
-        return opt;
-    }
+    CLI11_NODISCARD Option *get_option(std::string option_name);
 
     /// Shortcut bracket operator for getting a pointer to an option
     const Option *operator[](const std::string &option_name) const { return get_option(option_name); }

include/CLI/impl/App_inl.hpp

@@ -848,7 +848,7 @@ CLI11_INLINE std::vector<const Option *> App::get_options(const std::function<bo
             options.insert(options.end(), subcopts.begin(), subcopts.end());
         }
     }
-    if(fallthrough_ && parent_ != nullptr) {
+    if(fallthrough_ && parent_ != nullptr && !name_.empty()) {
         const auto *fallthrough_parent = _get_fallthrough_parent();
         std::vector<const Option *> subcopts = fallthrough_parent->get_options(filter);
         for(const auto *opt : subcopts) {
@@ -879,7 +879,7 @@ CLI11_INLINE std::vector<Option *> App::get_options(const std::function<bool(Opt
             options.insert(options.end(), subcopts.begin(), subcopts.end());
         }
     }
-    if(fallthrough_ && parent_ != nullptr) {
+    if(fallthrough_ && parent_ != nullptr && !name_.empty()) {
         auto *fallthrough_parent = _get_fallthrough_parent();
         std::vector<Option *> subcopts = fallthrough_parent->get_options(filter);
         for(auto *opt : subcopts) {
@@ -893,6 +893,36 @@ CLI11_INLINE std::vector<Option *> App::get_options(const std::function<bool(Opt
     return options;
 }
 
+/// Get an option by name
+CLI11_NODISCARD CLI11_INLINE const Option *App::get_option(std::string option_name) const {
+    const auto *opt = get_option_no_throw(option_name);
+    if(opt == nullptr) {
+        if(fallthrough_ && parent_ != nullptr && name_.empty()) {
+            // as a special case option groups with fallthrough enabled can also check the parent for options if the
+            // option is not found in the group this will not recurse as the internal call is to the no_throw version
+            // which will not check the parent again for option groups even with fallthrough enabled
+            return _get_fallthrough_parent()->get_option(option_name);
+        }
+        throw OptionNotFound(option_name);
+    }
+    return opt;
+}
+
+/// Get an option by name (non-const version)
+CLI11_NODISCARD CLI11_INLINE Option *App::get_option(std::string option_name) {
+    auto *opt = get_option_no_throw(option_name);
+    if(opt == nullptr) {
+        if(fallthrough_ && parent_ != nullptr && name_.empty()) {
+            // as a special case option groups with fallthrough enabled can also check the parent for options if the
+            // option is not found in the group this will not recurse as the internal call is to the no_throw version
+            // which will not check the parent again for option groups even with fallthrough enabled
+            return _get_fallthrough_parent()->get_option(option_name);
+        }
+        throw OptionNotFound(option_name);
+    }
+    return opt;
+}
+
 CLI11_NODISCARD CLI11_INLINE Option *App::get_option_no_throw(std::string option_name) noexcept {
     for(Option_p &opt : options_) {
         if(opt->check_name(option_name)) {
@@ -908,7 +938,9 @@ CLI11_NODISCARD CLI11_INLINE Option *App::get_option_no_throw(std::string option
             }
         }
     }
-    if(fallthrough_ && parent_ != nullptr) {
+    if(fallthrough_ && parent_ != nullptr && !name_.empty()) {
+        // if there is fallthrough and a parent and this is not an option_group then also check the parent for the
+        // option
         return _get_fallthrough_parent()->get_option_no_throw(option_name);
     }
     return nullptr;
@@ -929,7 +961,7 @@ CLI11_NODISCARD CLI11_INLINE const Option *App::get_option_no_throw(std::string
             }
         }
     }
-    if(fallthrough_ && parent_ != nullptr) {
+    if(fallthrough_ && parent_ != nullptr && !name_.empty()) {
         return _get_fallthrough_parent()->get_option_no_throw(option_name);
     }
     return nullptr;

tests/OptionGroupTest.cpp

@@ -843,7 +843,27 @@ TEST_CASE_METHOD(ManyGroups, "OptionFind", "[optiongroup]") {
     g1->fallthrough();
     auto *opt_name = g1->get_option("--base");
     CHECK(opt_name == opt_main);
+    CHECK_THROWS_AS(g1->get_option("--notfound"), CLI::OptionNotFound);
     auto const *g1_const = g1;
     const auto *opt_name_const = g1_const->get_option("--base");
     CHECK(opt_name_const == opt_main);
+    CHECK_THROWS_AS(g1_const->get_option("--notfound"), CLI::OptionNotFound);
+}
+
+// from https://github.com/CLIUtils/CLI11/issues/1315
+TEST_CASE_METHOD(TApp, "SubcommandOptionGroupWithFallthrough", "[optiongroup]") {
+    // code from https://github.com/The0Dev
+    bool flag{false};
+    std::string str;
+    app.add_flag("--flag,!--no-flag", flag, "Enable a flag");
+
+    // Disabling this prevents the issue:
+    app.fallthrough(true);
+
+    auto *sub = app.add_subcommand("sub", "Execute a subcommand");
+
+    auto *group = sub->add_option_group("GROUP");
+
+    // possible failure
+    CHECK_NOTHROW(group->add_option("-p,--path", str, "An option"));
 }