OptTable SubCommand is not usable with positional command-line arguments

[cachemeifyoucan]

#155026 adds the support for subcommand for llvm::OptTable.

However, the new addition cannot be used when the tool needs to have additional positional arguments. The current implementation will give up when there are more than 1 positional arguments.

Ideally, the desired behavior is that the first matching subcommand is treated as real sub-command, while the rest of the positional arguments are treated as INPUT.

[Prabhuk]

Thank you for filing this bug. The positional arguments handling is implemented in a flexible manner. In the current design, we are not enforcing subcommand to be the first positional argument. It can appear in any location in the command line invocation. When there are multiple positional commands in the command line, instead of defaulting this to an error, we provide a callback for the user of the opttable to handle it the way they wish.

Please see the example usage of this in the example project below:
https://github.com/llvm/llvm-project/blob/main/llvm/examples/OptSubcommand/llvm-hello-sub.cpp#L68-L75

If this is sufficient for the scenario you have in mind, please close this bug. If not, please share a few command line invocations that you have in mind which cannot be handled with the current design. This will help me to improve the behavior in a way it can be useful for the scenarios you are dealing with.

[cachemeifyoucan]

This implementation is definitely not enough for any usage. There are lots of things can go wrong with current implementation. Mainly, the handler function doesn't actually do anything.

• Invocation: tool subcommand input, this will return empty sub-command no matter what is happening inside handler function. I can't get the proper subcommand unless I am willing to reimplement the parsing logic in the handler again.
• If you have an input (e.g. a file name) that happens to be the same as a subcommand, then I will get conflicting subcommand, and adds more complexity into the handler function.

Basically, the supposed added flexibility means it can't achieve anything unless I want to write a parser inside the handler.

[cachemeifyoucan]

diff --git a/llvm/lib/Option/ArgList.cpp b/llvm/lib/Option/ArgList.cpp
index 2f4e21257af0..9f9d63f212c8 100644
--- a/llvm/lib/Option/ArgList.cpp
+++ b/llvm/lib/Option/ArgList.cpp
@@ -230,10 +230,8 @@ StringRef ArgList::getSubCommand(
     HandleMultipleSubcommands(SubCommands);
     return {};
   }
-  if (!OtherPositionals.empty()) {
+  if (!OtherPositionals.empty())
     HandleOtherPositionals(OtherPositionals);
-    return {};
-  }

   if (SubCommands.size() == 1)
     return SubCommands.front();

This will make the option a lot more usable, but there are still some corner cases you can hit, like the second bullet from top.
Also tool subcommand input1 input2 and tool input1 subcommand input2 gets the same callback so there is no way to differentiate that.

[cachemeifyoucan]

Also if I have my input the same as command like the case I mentioned above, I will potentially get input from the multiple subcommands. Then this is confusing:

tool command input other
tool command other input

^ Both above will get command + input in multiple command array, other in other positional array. so there are no way to tell which one is the first argument.

[Prabhuk]

diff --git a/llvm/lib/Option/ArgList.cpp b/llvm/lib/Option/ArgList.cpp
index 2f4e21257af0..9f9d63f212c8 100644
--- a/llvm/lib/Option/ArgList.cpp
+++ b/llvm/lib/Option/ArgList.cpp
@@ -230,10 +230,8 @@ StringRef ArgList::getSubCommand(
     HandleMultipleSubcommands(SubCommands);
     return {};
   }
-  if (!OtherPositionals.empty()) {
+  if (!OtherPositionals.empty())
     HandleOtherPositionals(OtherPositionals);
-    return {};
-  }

   if (SubCommands.size() == 1)
     return SubCommands.front();

This seems to be a valid fix for what apparently is a bug in current implementation on handling positionals which are not registered as subcommands in the corresponding td file. Would you like to send a patch for this or would you like me to do it?

For the 2 other cases that you have mentioned:

1. File name as the same name as subcommand name
2. Order of non-subcommand positionals in the command line where there are more than one

Please note the design for subcommands in OptTable was done to provide a viable alternative to the existing cl tool. The requirements above did not come up in the discourse discussion post and am not sure if they are supported in the cl tool. I will be happy to work with you to address these new requirements. Also please point out if I have not listed any of your requirements in the list above. Thank you.

[cachemeifyoucan]

Please note the design for subcommands in OptTable was done to provide a viable alternative to the existing cl tool

I would expect to be at least function equivalent with basic usage. For example for current tool using cl::opt, llvm-profdata

$ touch show
$ llvm-profdata show show
error: show: empty raw profile file

This should be easily implementable.

[cachemeifyoucan]

Fix1: #166859

To fix the other case, the callback model needs to be updated.

[Prabhuk]

Thanks for the first fix. For the llvm-profdata show show -- Though it is possible to support this, we should consider not supporting this corner case. How often in practice are we going to run in to this case? As we are in a place where there are no users of OptTable subcommand feature yet, we can hold off on adding support for cases until we have a need. We do not need to make OptTable subcommand a like-a-like replacement for cl tool and inherit all the choices they made.

[cachemeifyoucan]

I think this is a legit use case. For example, the example above is using that as a file, and you don't want to put restrictions on what the file can be named.

The reality is, it is normal that llvm-profdata show show is interpreted as show is an input, and the corner case is that you treat as conflict arguments.

If I am re-designing, the callback can be:

/// \p FirstCommand, detected first subcommand, empty string if no subcommand
/// \p OtherPositionals, all other positional arguments other than the first subcommand
/// \p HasConflictCommands, other positional arguments has strings that matches subcommand
/// \returns true if all errors are handled and parse is successful, false if parse failed.
bool handleCommandLine(StringRef FirstCommand, ArrayRef<StringRef> OtherPositionals, bool HasConflictCommands);

[Prabhuk]

I am more or less on the same page with you. Here are a few requirements which shouldn't conflict with your requirements:

1. Only one valid subcommand. If there are 2 distinct subcommands in the command line that should still be an error. It would mean that the redesign of callback's first param is simply "SubCommand" instead of "FirstCommand".
2. A valid subcommand may appear in any position in the command line.
3. llvm-profdata show show -- I still think llvm-profdata show <anyotherfilename> should be supported and llvm-profdata show show should be an error. But I am not going to die on this hill :) Feel free to implement this behavior and I will not object.

It may be worth posting your conclusions as a response to the discourse post here: https://discourse.llvm.org/t/subcommand-feature-support-in-llvm-opttable/88098

[cachemeifyoucan]

For your requirements, do you have any reference tool or CommandLine parsing library that has similar semantics?

I will not die on my requirement to make profdata show show to work so I would like to understand your reason for making such a requirement.