[CommandLine] Do not crash if an option has both ValueRequired and Grouping.

[ikudrin]

Summary

If an option, which requires a value, has a cl::Grouping formatting flag,
it works well as far as it is used at the end of a group (or not in a group).
However, if the option appears accidentally in the middle of a group,
the program just crashes. I believe it is better to show some useful
error message in that case.

[jhenderson]

According to the documentation (https://llvm.org/docs/CommandLine.html#controlling-other-formatting-options) this is illegal even for the last value:

Note that cl::Grouping options cannot have values.

Does it just work? If so, great, but please update the docs.

Regardless, we certainly shouldn't be asserting if we can get into that state, so this is a good change.

Your description mentioned ValueRequired. What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

unittests/Support/CommandLineTest.cpp

1146

The first option isn't really in the middle, after all, so this comment should be adjusted to say "elswhere in the group" or "not at the end of a group" or similar.

[ikudrin]

• Updated the documentation (feel free to correct my wording).
• Amended a comment in the test.

[ikudrin]

In D58499#1405561, @jhenderson wrote:

What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

As far as I can see, in case of grouped options, the value might be assigned only if it is a prefixed option. Did you mean a specific case? Could you provide it so that I understand you better?

Anyway, if anything needs fixing with ValueOptional, it should be a separate patch, right?

[jhenderson]

LGTM.

In D58499#1407211, @ikudrin wrote:

In D58499#1405561, @jhenderson wrote:

What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

As far as I can see, in case of grouped options, the value might be assigned only if it is a prefixed option. Did you mean a specific case? Could you provide it so that I understand you better?

Anyway, if anything needs fixing with ValueOptional, it should be a separate patch, right?

Yes, you're right. If there's any issue with ValueOptional, it should be a separate patch. I don't have a specific case in mind. I just want to make sure that the behaviour of something like "-abc" where -a, -b and -c are all options, and -b is ValueOptional makes sense (I'm not sure whether it should be equivalent to "-a -b=c" or "-a -b -c" though). Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired. Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

CC'ing @rupprecht, who may be interested in this change too.

[ikudrin]

In D58499#1408521, @jhenderson wrote:

LGTM.

Thanks!

I just want to make sure that the behaviour of something like "-abc" where -a, -b and -c are all options, and -b is ValueOptional makes sense (I'm not sure whether it should be equivalent to "-a -b=c" or "-a -b -c" though).

If I understand it right, a ValueOptional option can accept its value only if it is explicitly used in the -opt=value form. Moreover, as boolean options are of a ValueOptional kind implicitly, your sample is something very basic for checking how Grouping works at all. "-abc" should definitely be equivalent to "-a -b -c". It is a bit strange that there are no tests for that. Maybe the test suite is worth to be extended.

Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired.

You are right. We either need better wording in the documentation or improve the code so that the "=val" form is accepted with a group. I would prefer changing the documentation because no one needed that new behavior yet.

Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

Let's create a separate patch with additional tests for the existing behavior.

lib/Support/CommandLine.cpp

HandlePrefixedOrGroupedOption(StringRef &Arg, StringRef &Value,
   // have values.
   assert(isGrouping(PGOpt) && "Broken getOptionPred!");
 
   do {
     // Move current arg name out of Arg into OneArgName.
     StringRef OneArgName = Arg.substr(0, Length);
     Arg = Arg.substr(Length);
 
-    // Because ValueRequired is an invalid flag for grouped arguments,
-    // we don't need to pass argc/argv in.
-    assert(PGOpt->getValueExpectedFlag() != cl::ValueRequired &&
-           "Option can not be cl::Grouping AND cl::ValueRequired!");
+    if (PGOpt->getValueExpectedFlag() == cl::ValueRequired) {
+      ErrorParsing |= PGOpt->error("may not occur within a group!");
+      return nullptr;
+    }
+
+    // Because the value for the option is not required, we don't need to pass
+    // argc/argv in.
     int Dummy = 0;
     ErrorParsing |=
         ProvideOption(PGOpt, OneArgName, StringRef(), 0, nullptr, Dummy);
 
     // Get the next grouping option.
     PGOpt = getOptionPred(Arg, Length, isGrouping, OptionsMap);
   } while (PGOpt && Length != Arg.size());
 

unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, PrefixOptions) {
   EXPECT_TRUE(MacroDefs.empty());
   const char *args6[] = {"prog", "-DHAVE_FOO"};
   EXPECT_TRUE(
       cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));
   EXPECT_TRUE(MacroDefs.size() == 1);
   EXPECT_TRUE(MacroDefs.front().compare("HAVE_FOO") == 0);
 }
 
+TEST(CommandLineTest, GroupingWithValue) {
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> OptF("f", cl::Grouping, cl::desc("Some flag"));
+  StackOption<std::string> OptV("v", cl::Grouping,
+                                cl::desc("Grouping option with a value"));
+
+  // Should be possible to use an option which requires a value
+  // at the end of a group.
+  const char *args1[] = {"prog", "-fv", "val1"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(3, args1, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("val1", OptV.c_str());
+  cl::ResetAllOptionOccurrences();
+
+  // Should not crash if it is accidentally used in the middle of a group.
+  const char *args2[] = {"prog", "-vf", "val2"};
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(3, args2, StringRef(), &llvm::nulls()));
+}
+
 }  // anonymous namespace