Only print help for subsubcommand when global settings subcommandneededelsehelp

[ZenithalHourlyRate]

Please complete the following tasks

• I have searched the discussions
• I have searched the existing issues

Rust Version

rustc 1.52.1 (9bc8c42bb 2021-05-09)

Clap Version

2.33.3

Minimal reproducible code

main.rs

#[macro_use]
extern crate clap;
use clap::{App, ArgMatches};

fn genkey(matches: &ArgMatches) {
    match matches.subcommand() {
        ("b", _) => {
            println!("Reached here");
        },
        _ => unimplemented!(),
    }
}

fn main() {
    let yaml = load_yaml!("cli.yml");
    let matches = App::from(yaml)
        .get_matches();

    match matches.subcommand() {
        ("a", Some(matches)) => genkey(matches),
        _ => unreachable!(),
    }
}

cli.yml

name: foo
global_settings:
    - subcommandrequiredelsehelp
subcommands:
  - a:
      about: First layer subcommand
      subcommands:
        - b:
              about: Second layer subcommand

Steps to reproduce the bug with the above code

$ cargo r -- a b

Actual Behaviour

$ cargo r -- a b
   Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `/home/zenithal/T/test/target/debug/foo a b`
foo-a-b
Second layer subcommand

USAGE:
    foo a b

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

Expected Behaviour

$ cargo r -- a b
   Compiling foo v0.1.0 (/home/zenithal/T/test)
    Finished dev [unoptimized + debuginfo] target(s) in 0.37s
     Running `/home/zenithal/T/test/target/debug/foo a b`
Reached here

Additional Context

No response

Debug Output

DEBUG:clap:Parser::add_subcommand: term_w=None, name=b
DEBUG:clap:Parser::add_subcommand: term_w=None, name=a
DEBUG:clap:Parser::propagate_settings: self=foo, g_settings=AppFlags(
    SC_REQUIRED_ELSE_HELP,
)
DEBUG:clap:Parser::propagate_settings: sc=a, settings=AppFlags(
    NEEDS_LONG_HELP | NEEDS_LONG_VERSION | NEEDS_SC_HELP | UTF8_NONE | COLOR_AUTO,
), g_settings=AppFlags(
    (empty),
)
DEBUG:clap:Parser::propagate_settings: self=a, g_settings=AppFlags(
    SC_REQUIRED_ELSE_HELP,
)
DEBUG:clap:Parser::propagate_settings: sc=b, settings=AppFlags(
    NEEDS_LONG_HELP | NEEDS_LONG_VERSION | NEEDS_SC_HELP | UTF8_NONE | COLOR_AUTO,
), g_settings=AppFlags(
    (empty),
)
DEBUG:clap:Parser::propagate_settings: self=b, g_settings=AppFlags(
    SC_REQUIRED_ELSE_HELP,
)
DEBUG:clap:Parser::get_matches_with;
DEBUG:clap:Parser::create_help_and_version;
DEBUG:clap:Parser::create_help_and_version: Building --help
DEBUG:clap:Parser::create_help_and_version: Building --version
DEBUG:clap:Parser::create_help_and_version: Building help
DEBUG:clap:Parser::get_matches_with: Begin parsing '"a"' ([97])
DEBUG:clap:Parser::is_new_arg:"a":NotFound
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: probably value
DEBUG:clap:Parser::is_new_arg: starts_new_arg=false
DEBUG:clap:Parser::possible_subcommand: arg="a"
DEBUG:clap:Parser::get_matches_with: possible_sc=true, sc=Some("a")
DEBUG:clap:Parser::parse_subcommand;
DEBUG:clap:usage::get_required_usage_from: reqs=[], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:Parser::parse_subcommand: About to parse sc=a
DEBUG:clap:Parser::parse_subcommand: sc settings=AppFlags(
    SC_REQUIRED_ELSE_HELP | NEEDS_LONG_HELP | NEEDS_LONG_VERSION | NEEDS_SC_HELP | UTF8_NONE | COLOR_AUTO,
)
DEBUG:clap:Parser::get_matches_with;
DEBUG:clap:Parser::create_help_and_version;
DEBUG:clap:Parser::create_help_and_version: Building --help
DEBUG:clap:Parser::create_help_and_version: Building --version
DEBUG:clap:Parser::create_help_and_version: Building help
DEBUG:clap:Parser::get_matches_with: Begin parsing '"b"' ([98])
DEBUG:clap:Parser::is_new_arg:"b":NotFound
DEBUG:clap:Parser::is_new_arg: arg_allows_tac=false
DEBUG:clap:Parser::is_new_arg: probably value
DEBUG:clap:Parser::is_new_arg: starts_new_arg=false
DEBUG:clap:Parser::possible_subcommand: arg="b"
DEBUG:clap:Parser::get_matches_with: possible_sc=true, sc=Some("b")
DEBUG:clap:Parser::parse_subcommand;
DEBUG:clap:usage::get_required_usage_from: reqs=[], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:Parser::parse_subcommand: About to parse sc=b
DEBUG:clap:Parser::parse_subcommand: sc settings=AppFlags(
    SC_REQUIRED_ELSE_HELP | NEEDS_LONG_HELP | NEEDS_LONG_VERSION | NEEDS_SC_HELP | UTF8_NONE | COLOR_AUTO,
)
DEBUG:clap:Parser::get_matches_with;
DEBUG:clap:Parser::create_help_and_version;
DEBUG:clap:Parser::create_help_and_version: Building --help
DEBUG:clap:Parser::create_help_and_version: Building --version
DEBUG:clap:Parser::get_matches_with: SubcommandRequiredElseHelp=true
DEBUG:clap:Help::write_parser_help;
DEBUG:clap:Help::write_parser_help;
DEBUG:clap:Parser::color;
DEBUG:clap:Parser::color: Color setting...Auto
DEBUG:clap:is_a_tty: stderr=true
DEBUG:clap:Help::new;
DEBUG:clap:Help::write_help;
DEBUG:clap:Help::write_default_help;
DEBUG:clap:Help::write_bin_name;
DEBUG:clap:Help::write_version;
DEBUG:clap:Help::write_default_help: writing about
DEBUG:clap:usage::create_usage_no_title;
DEBUG:clap:usage::get_required_usage_from: reqs=[], extra=None
DEBUG:clap:usage::get_required_usage_from: after init desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: no more children
DEBUG:clap:usage::get_required_usage_from: final desc_reqs=[]
DEBUG:clap:usage::get_required_usage_from: args_in_groups=[]
DEBUG:clap:usage::needs_flags_tag;
DEBUG:clap:usage::needs_flags_tag:iter: f=hclap_help;
DEBUG:clap:usage::needs_flags_tag:iter: f=vclap_version;
DEBUG:clap:usage::needs_flags_tag: [FLAGS] not required
DEBUG:clap:usage::create_help_usage: usage=foo a b
DEBUG:clap:Help::write_all_args;
DEBUG:clap:Help::write_args;
DEBUG:clap:Help::write_args: Current Longest...2
DEBUG:clap:Help::write_args: New Longest...6
DEBUG:clap:Help::write_args: Current Longest...6
DEBUG:clap:Help::write_args: New Longest...9
DEBUG:clap:Help::write_arg;
DEBUG:clap:Help::short;
DEBUG:clap:Help::long;
DEBUG:clap:Help::val: arg=--help
DEBUG:clap:Help::spec_vals: a=--help
DEBUG:clap:Help::val: Has switch...Yes
DEBUG:clap:Help::val: force_next_line...false
DEBUG:clap:Help::val: nlh...false
DEBUG:clap:Help::val: taken...21
DEBUG:clap:Help::val: help_width > (width - taken)...23 > (120 - 21)
DEBUG:clap:Help::val: longest...9
DEBUG:clap:Help::val: next_line...No
DEBUG:clap:write_spaces!: num=7
DEBUG:clap:Help::help;
DEBUG:clap:Help::help: Next Line...false
DEBUG:clap:Help::help: Too long...No
DEBUG:clap:Help::write_arg;
DEBUG:clap:Help::short;
DEBUG:clap:Help::long;
DEBUG:clap:Help::val: arg=--version
DEBUG:clap:Help::spec_vals: a=--version
DEBUG:clap:Help::val: Has switch...Yes
DEBUG:clap:Help::val: force_next_line...false
DEBUG:clap:Help::val: nlh...false
DEBUG:clap:Help::val: taken...21
DEBUG:clap:Help::val: help_width > (width - taken)...26 > (120 - 21)
DEBUG:clap:Help::val: longest...9
DEBUG:clap:Help::val: next_line...No
DEBUG:clap:write_spaces!: num=4
DEBUG:clap:Help::help;
DEBUG:clap:Help::help: Next Line...false
DEBUG:clap:Help::help: Too long...No

[epage]

Is the bug here that we are requiring a subcommand at a level that has no subcommands and the proposed fix would be to ignore subcommandrequiredelsehelp when there aren't subcommands?

clap is doing exactly what it was told. I'm a bit cautious of it trying out out guess the user. Personally, I would only recommend using subcommandrequiredelsehelp as a setting as its coupled to the behavior of each App level. In #1184, I do bring up the idea of allowing a more specific config to override the global. However, if we go forward with #2717, my expectation is we'd hard code which settings are layered and which are not and, as I expect this to be coupled to the App level, I had assumed we would not layer this setting and remove the idea of it being "global".

[heroin-moose]

It seems that I'm not the only one who expected clap to behave otherwise, so my 2cents. The documentation says:

Display help if no subcommands are present at runtime and exit gracefully (i.e. an empty run such as $ myprog).

While technically clap behaves exactly as it was told, IMO it's still a bit counter-intuitive. Most of the time if I have the following command structure:

foo
    bar
        show
        add
    baz
        sub-baz
            add
            show
...

I want exactly the same behavior for all subcommands: if it has subcommands but no further arguments, exit with help. This makes it easier for people who in a hurry just type foo baz sub-baz and get the help without the need to consult a manual page. However, it's natural to expect that commands without subcommands should not fail, because why should they? The command invocation is pretty clear and there are no side effects I can think about.

[epage]

It seems that I'm not the only one who expected clap to behave otherwise

In #2717 we are exploring removing *Settings in favor of methods. In doing so, we'd switch to a model similar to layered configs where we propagate settings to a subcommand when that subcommand doesn't explicitly have that setting set. Only some settings would opt-in to this model and it would be called out in the documentation.

The lack of a global_setting but instead marking what settings propagate would hopefully make this clearer in the future.

I want exactly the same behavior for all subcommands: if it has subcommands but no further arguments, exit with help. This makes it easier for people who in a hurry just type foo baz sub-baz and get the help without the need to consult a manual page. However, it's natural to expect that commands without subcommands should not fail, because why should they? The command invocation is pretty clear and there are no side effects I can think about.

Something not covered in this issue that was in my original reply to #3270 before I realized it was a dupe is why I wasn't going to layer subcommandrequiredelsehelp / why I don't think its appropriate as a global_setting. subcommandrequiredelsehelp has two pieces of policy combined: whether subcommands are required and how to respond when its missing. Whether subcommands are required is a policy specific to one set of subcommands in the hierarchy. If a new area is added with an optional subcommand, that requires removing the global setting and applying it everywhere if its even noticed / tested. So this seems more about saving effort which can backfire if an optional subcommand does get added and can make ensuring correctness more difficult.

Now, decoupling the two concepts somehow could open us up to one being global while the other isn't. We'd just need to come up with a clear name / policy for this new setting (we already have SubcommandRequired)

btw I'd be curious if you have any thoughts on #1334 since you have such a large subcommand hierarchy. I'm assuming you are likely to run into cases like git vs git stash in terms of how subcommands should be treated.

[heroin-moose]

So this seems more about saving effort which can backfire if an optional subcommand does get added and can make ensuring correctness more difficult.

Hmm... I see. Yeah, in the presence of optional subcommands it does gets in the way.

Now, decoupling the two concepts somehow could open us up to one being global while the other isn't. We'd just need to come up with a clear name / policy for this new setting (we already have SubcommandRequired)

Sounds good!

btw I'd be curious if you have any thoughts on #1334 since you have such a large subcommand hierarchy. I'm assuming you are likely to run into cases like git vs git stash in terms of how subcommands should be treated.

Well #1334 looks good. I would love to see some knobs to be able to do this:

$ foo
[fullblown nested list]
$ foo baz
[single-level not-nested list]

[epage]

For #1334, the way my proposal would work is

$ git --help  # #1334 is not enabled
[lists subcommands as if #1334 didn't exist]
$ git stash --help   # #1334 is enabled
[shows help for all stash subcommands like push, pop, etc]
$ git stash pop --help  # no subcommands
[shows pop's help as if #1334 didn't exist]

Is that the same thing you were showing?

The idea is that we'd only nest what the user opts into and adding additional subcommands would filter down the nesting to give a more specific view.

[heroin-moose]

Yeah!

[epage]

@heroin-moose I think I'm leaning towards this issue staying as is to make any decision on it easier to discover. If you'd like to see SubcommandRequiredElseHelp split up, could you create a separate issue?

[heroin-moose]

@heroin-moose I think I'm leaning towards this issue staying as is to make any decision on it easier to discover. If you'd like to see SubcommandRequiredElseHelp split up, could you create a separate issue?

Sure, I'll reference this one for the discussion and give a short summary. Perhaps I can make a PR (not promising anything :)).

[epage]

I wonder why we have SubcommandRequiredElseHelp (show help if no subcommand, regardless of presence of any args) when we also have ArgRequiredElseHelp (show help only for $ mycmd, no args or subcommands). I feel like ArgRequiredElseHelp with SubcommandRequired is the more appropriate behavior. The user did an action, we should give a more tailored message for what went wrong rather than giving them a generic message.

[epage]

29ca7b2 and #133 don't include details as to why we have both.

[epage]

I've gone ahead and created #3280.

As stated earlier, I'm leaning towards keeping SubcommandRequiredElseHelp doing literally what the user asked and looking to reject this issue. If there are thoughts on concerns about that, let us know!