[Bug] Parser assumes positional arguments which happen to match subcommand names are subcommands

[nathanielhourt]

Consider this demo program:

#include <string>
#include <iostream>

#include "CLI11.hpp"

using std::string;
using std::cout;
using std::endl;

int main(int argc, char** argv) {
    CLI::App app{"Buggy demo app"};
    app.require_subcommand();

    auto foo = app.add_subcommand("foo", "Foo Description", false);
    auto bar = app.add_subcommand("bar", "Bar Description", false);
    string baz;
    bar->add_option("baz", baz, "Baz Description", true)->required();

    foo->set_callback([] {
        cout << "Bad" << endl;
    });
    bar->set_callback([&baz] {
        cout << "Good; baz was " << baz << endl;
    });

    try {
        app.parse(argc, argv);
    } catch (CLI::ParseError& e) {
        return app.exit(e);
    }
}

The app has two subcommands, foo and bar, where bar takes a required positional argument named baz. Suppose I want to run bar with baz set to "qux":

$ ./demo bar qux
Good; baz was qux

This works great! But suppose instead I want baz set to "foo":

$ ./demo bar foo
ERROR: RequiredError: baz
Foo Description
Usage: ./a.out foo

The parser has seen "foo" and given up parsing arguments for bar, but it instead context switched to the foo subcommand.

There might be a use case for running multiple subcommands in the same invocation of the program, but the behavior we're seeing here is clearly incorrect, especially since there is no apparent way to fix it: adding app.fallthrough(false) and/or bar->fallthrough(false) do not solve the issue.

The only workaround I see so far is to convert baz to a named argument:

    bar->add_option("--baz", baz, "Baz Description", true)->required();

This allows me to slip it by the parser correctly:

$ ./demo bar --baz foo
Good; baz was foo

[nathanielhourt]

Another workaround:

$ ./demo bar -- foo
Good; baz was foo

I see now the issue is in the _recognize method, which checks if the name appears to be a subcommand, and if so, declares it as one.

[nathanielhourt]

I have submitted a naive PR to fix the issue, #24; however, a better fix may be possible. :)

[henryiii]

I'm thinking the correct behavior would be as follows:

• 1 Required arguments, no fall through

$ ./demo bar foo # bar is arg, foo is subcommands
$ ./demo foo other # ERROR TOO MANY ARGUMENTS
$ ./demo foo # foo is arg, no subcommand 

• 1 Required arguments, fall through:

$ ./demo bar other # bar is subcommand, other is arg
$ ./demo other bar # bar is subcommand, other is arg
$ ./demo bar foo # bar is subcommand, foo is arg

• Variable arguments, no fall through or fall through:

$ ./demo bar foo # bar, foo are subcommands

Are there missing combinations? Is that the most logical behavior?

[nathanielhourt]

Hmm, about half of those make no sense to me. :P

$ ./demo bar foo # bar is arg, foo is subcommands
bar is arg? Seems like it should be a subcommand. Arg to what? Neither demo nor foo take args.

$ ./demo foo other # ERROR TOO MANY ARGUMENTS
Makes sense

$ ./demo foo # foo is arg, no subcommand
Again, arg to what? It has to be a subcommand

$ ./demo bar other # bar is subcommand, other is arg
Makes sense

$ ./demo other bar # bar is subcommand, other is arg
I can't see a reason why an argument to a subcommand would ever come before the subcommand itself...

$ ./demo bar foo # bar is subcommand, foo is arg
Makes sense

$ ./demo bar foo # bar, foo are subcommands
Seems like foo should still be the arg to bar here.

Am I misunderstanding?

[henryiii]

Ahh, I was associating demo with baz. I think that remains general (since a command and a subcommand are the same thing). So can you revaluate if those make sense with

app->add_option("baz", baz, "Baz Description", true)->required();

?

[nathanielhourt]

Ahh, OK, that makes much more sense now.

So what you've described is roughly equivalent to:

• If fallthrough is disabled, parse tokens as positionals first, then subcommands as a fallback
• If fallthrough is enabled, parse tokens as subcommands first, then positionals as a fallback

[henryiii]

I'm going to change that a bit, on an attempt to write it:

• If fallthrough is enabled or disabled, parse tokens as required positionals first, then subcommands as a fallback, then optional positionals

In this case, required positionals cannot fall through. Falling through positionals are dangerous, since there's no "name" associated with them.

[henryiii]

Does the code in the sub com branch do what you'd expect? I think I went with something more like what I described first. A little cleanup will be needed tomorrow.

[nathanielhourt]

Yep, the subcom branch works for my use case.

[henryiii]

Polished and merged. Feel free to look at the added tests to verify that it does provide the expected behavior.