[llvm] [CommandLine] Do not suggest really hidden opts in nearest lookup

[mgorny]

Summary

Skip 'really hidden' options when performing lookup of the nearest
option when invalid option was passed. Since these options aren't even
documented in --help-hidden, it seems inconsistent to suggest them
to users.

This fixes clang-tools-extra test failures due to unexpected suggestions
when linking the tools to LLVM dylib (that provides more options than
the subset of LLVM libraries linked directly).

[sammccall]

Sounds like a good idea to me.

Could you extend the "OptionErrorMessageSuggest" test in llvm/unittests/Support/CommandLineTest.cpp to cover this case?

llvm/lib/Support/CommandLine.cpp

595

Rather than not considering them (and offering a worse option, which may be confusing) you might consider checking Best after the loop so we just offer no suggestion if the best one is hidden.

Not sure which behavior is better, up to you.

[mgorny]

llvm/lib/Support/CommandLine.cpp

595

The clang-tools-extra tests in question rely on having the 'next best' served.

I think the current behavior is more predictable, as adding new 'really hidden' options won't cause the current suggestions to suddenly disappear.

[mgorny]

(I'm working on the test)

[njames93]

llvm/lib/Support/CommandLine.cpp

595

Are they the ones added in D80879. If it is, the CHECK-NEXT lines in there could probably be done away with anyway. The important part was making sure it didn't crash.

However tests in llvm/unittests/Support/CommandLineTests should be added for this change.

595

I'm not too fond of Sams suggestion as it isn't great for the case when a really hidden option had a slightly shorter distanced name to another non hidden option, You will lose the valid option suggestion.
I kind of prefer the way of ignoring really hidden options, but putting an upper limit on the distance for suggested changes. I almost feel like the distance should be a function of the OptionLength - A 50 character option differing by 2 characters has much more confidence than a 3 character option differing by 2. Can't think of a good mapping for it though, My first suggestion would be based on the square root of the length

[sammccall]

I don't feel strongly about the inside/outside loop, let's stick with what you've proposed then.

LG with a test.

[mgorny]

Added a test that adds a hidden option that is closer to argv. Confirmed that it fails without the change, passes with the change.

[mgorny]

In D82001#2098158, @sammccall wrote:

I don't feel strongly about the inside/outside loop, let's stick with what you've proposed then.

LG with a test.

Thanks. Could you take a quick look at the test, just to be sure?

[lattner]

Makes sense, thanks!

[njames93]

LGTM

llvm/lib/Support/CommandLine.cpp

static Option *LookupNearestOption(StringRef Arg,
 
   // Find the closest match.
   Option *Best = nullptr;
   unsigned BestDistance = 0;
   for (StringMap<Option *>::const_iterator it = OptionsMap.begin(),
                                            ie = OptionsMap.end();
        it != ie; ++it) {
     Option *O = it->second;
+    // Do not suggest really hidden options (not shown in any help).
+    if (O->getOptionHiddenFlag() == ReallyHidden)
+      continue;
+
     SmallVector<StringRef, 16> OptionNames;
     O->getExtraOptionNames(OptionNames);
     if (O->hasArgStr())
       OptionNames.push_back(O->ArgStr);
 
     bool PermitValue = O->getValueExpectedFlag() != cl::ValueDisallowed;
     StringRef Flag = PermitValue ? LHS : Arg;
     for (auto Name : OptionNames) {

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, OptionErrorMessageSuggest) {
   OS.flush();
   EXPECT_FALSE(Errs.find("prog: Did you mean '--aluminium'?\n") ==
                std::string::npos);
   Errs.clear();
 
   cl::ResetAllOptionOccurrences();
 }
 
+TEST(CommandLineTest, OptionErrorMessageSuggestNoHidden) {
+  // We expect that 'really hidden' option do not show up in option
+  // suggestions.
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> OptLong("aluminium", cl::desc("Some long option"));
+  StackOption<bool> OptLong2("aluminum", cl::desc("Bad option"),
+                             cl::ReallyHidden);
+
+  const char *args[] = {"prog", "--alumnum"};
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+
+  EXPECT_FALSE(cl::ParseCommandLineOptions(2, args, StringRef(), &OS));
+  OS.flush();
+  EXPECT_FALSE(Errs.find("prog: Did you mean '--aluminium'?\n") ==
+               std::string::npos);
+  Errs.clear();
+
+  cl::ResetAllOptionOccurrences();
+}
+
 TEST(CommandLineTest, Callback) {
   cl::ResetCommandLineParser();
 
   StackOption<bool> OptA("a", cl::desc("option a"));
   StackOption<bool> OptB(
       "b", cl::desc("option b -- This option turns on option a"),
       cl::callback([&](const bool &) { OptA = true; }));
   StackOption<bool> OptC(