Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CommandLine] Better report unknown subcommands #74811

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

igorkudrin
Copy link
Collaborator

The patch improves the reporting for the first option in the command line when it looks like a subcommand name but does not match any defined.

Before the patch:

> prog baz
prog: Unknown command line argument 'baz'.  Try: 'prog --help'

With the patch:

> prog baz
prog: Unknown subcommand 'baz'.  Try: 'prog --help'
prog: Did you mean 'bar'?

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 8, 2023

@llvm/pr-subscribers-llvm-support

Author: Igor Kudrin (igorkudrin)

Changes

The patch improves the reporting for the first option in the command line when it looks like a subcommand name but does not match any defined.

Before the patch:

> prog baz
prog: Unknown command line argument 'baz'.  Try: 'prog --help'

With the patch:

> prog baz
prog: Unknown subcommand 'baz'.  Try: 'prog --help'
prog: Did you mean 'bar'?

Full diff: https://github.com/llvm/llvm-project/pull/74811.diff

2 Files Affected:

  • (modified) llvm/lib/Support/CommandLine.cpp (+33-12)
  • (modified) llvm/unittests/Support/CommandLineTest.cpp (+21)
diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index a7e0cae8b855d..b76c2cc8be6da 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -425,7 +425,7 @@ class CommandLineParser {
       return nullptr;
     return Opt;
   }
-  SubCommand *LookupSubCommand(StringRef Name);
+  SubCommand *LookupSubCommand(StringRef Name, std::string &NearestString);
 };
 
 } // namespace
@@ -550,9 +550,13 @@ Option *CommandLineParser::LookupOption(SubCommand &Sub, StringRef &Arg,
   return I->second;
 }
 
-SubCommand *CommandLineParser::LookupSubCommand(StringRef Name) {
+SubCommand *CommandLineParser::LookupSubCommand(StringRef Name,
+                                                std::string &NearestString) {
   if (Name.empty())
     return &SubCommand::getTopLevel();
+  // Find the closest match if there is no subcommand named 'Name'.
+  SubCommand *BestMatch = nullptr;
+  unsigned BestDistance = 0;
   for (auto *S : RegisteredSubCommands) {
     if (S == &SubCommand::getAll())
       continue;
@@ -561,7 +565,19 @@ SubCommand *CommandLineParser::LookupSubCommand(StringRef Name) {
 
     if (StringRef(S->getName()) == StringRef(Name))
       return S;
+
+    unsigned Distance =
+        Name.edit_distance(S->getName(), /*AllowReplacements=*/true,
+                           /*MaxEditDistance=*/BestDistance);
+    if (!BestMatch || Distance < BestDistance) {
+      BestMatch = S;
+      BestDistance = Distance;
+    }
   }
+
+  if (BestMatch)
+    NearestString = BestMatch->getName();
+
   return &SubCommand::getTopLevel();
 }
 
@@ -1527,10 +1543,12 @@ bool CommandLineParser::ParseCommandLineOptions(int argc,
 
   int FirstArg = 1;
   SubCommand *ChosenSubCommand = &SubCommand::getTopLevel();
+  std::string NearestSubCommandString;
   if (argc >= 2 && argv[FirstArg][0] != '-') {
     // If the first argument specifies a valid subcommand, start processing
     // options from the second argument.
-    ChosenSubCommand = LookupSubCommand(StringRef(argv[FirstArg]));
+    ChosenSubCommand =
+        LookupSubCommand(StringRef(argv[FirstArg]), NearestSubCommandString);
     if (ChosenSubCommand != &SubCommand::getTopLevel())
       FirstArg = 2;
   }
@@ -1687,21 +1705,24 @@ bool CommandLineParser::ParseCommandLineOptions(int argc,
     }
 
     if (!Handler) {
-      if (SinkOpts.empty()) {
+      if (!SinkOpts.empty()) {
+        for (Option *SinkOpt : SinkOpts)
+          SinkOpt->addOccurrence(i, "", StringRef(argv[i]));
+        continue;
+      } else if (i == 1 && !NearestSubCommandString.empty()) {
+        *Errs << ProgramName << ": Unknown subcommand '" << argv[i]
+              << "'.  Try: '" << argv[0] << " --help'\n"
+              << ProgramName << ": Did you mean '" << NearestSubCommandString
+              << "'?\n";
+      } else {
         *Errs << ProgramName << ": Unknown command line argument '" << argv[i]
               << "'.  Try: '" << argv[0] << " --help'\n";
-
-        if (NearestHandler) {
+        if (NearestHandler)
           // If we know a near match, report it as well.
           *Errs << ProgramName << ": Did you mean '"
                 << PrintArg(NearestHandlerString, 0) << "'?\n";
-        }
-
-        ErrorParsing = true;
-      } else {
-        for (Option *SinkOpt : SinkOpts)
-          SinkOpt->addOccurrence(i, "", StringRef(argv[i]));
       }
+      ErrorParsing = true;
       continue;
     }
 
diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index 381fe70b6b481..4ad20cf5febdf 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -2206,4 +2206,25 @@ TEST(CommandLineTest, DefaultValue) {
   EXPECT_EQ(1, StrInitOption.getNumOccurrences());
 }
 
+TEST(CommandLineTest, UnknownSubcommand) {
+  cl::ResetCommandLineParser();
+
+  StackSubCommand SC1("foo", "Foo subcommand");
+  StackSubCommand SC2("bar", "Bar subcommand");
+  StackOption<bool> SC1Opt("f", cl::sub(SC1));
+  StackOption<bool> SC2Opt("b", cl::sub(SC2));
+  StackOption<bool> TopOpt("t");
+
+  const char *Args[] = {"prog", "baz", "-p"};
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(std::size(Args), Args, StringRef(), &OS));
+  EXPECT_EQ(Errs,
+            "prog: Unknown subcommand 'baz'.  Try: 'prog --help'\n"
+            "prog: Did you mean 'bar'?\n"
+            "prog: Unknown command line argument '-p'.  Try: 'prog --help'\n"
+            "prog: Did you mean '-t'?\n");
+}
+
 } // anonymous namespace

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although a few minor suggestions you might want to adopt.

llvm/lib/Support/CommandLine.cpp Outdated Show resolved Hide resolved
llvm/unittests/Support/CommandLineTest.cpp Outdated Show resolved Hide resolved
llvm/unittests/Support/CommandLineTest.cpp Outdated Show resolved Hide resolved
@@ -561,7 +565,19 @@ SubCommand *CommandLineParser::LookupSubCommand(StringRef Name) {

if (StringRef(S->getName()) == StringRef(Name))
return S;

unsigned Distance =
Name.edit_distance(S->getName(), /*AllowReplacements=*/true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In clang and lld, we do something like if (OptTbl.findNearest(ArgString, Nearest, VisibilityMask) > 1) to reject a candidate with a too large edit distance

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed.

Copy link

github-actions bot commented Dec 13, 2023

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

The patch improves the reporting for the first option in the command
line when it looks like a subcommand name but does not match any
defined.

Before the patch:
```
> prog baz
prog: Unknown command line argument 'baz'.  Try: 'prog --help'
```

With the patch:
```
> prog baz
prog: Unknown subcommand 'baz'.  Try: 'prog --help'
prog: Did you mean 'bar'?
```
}

auto reportUnknownArgument = [&](bool IsArg,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think the LLVM consensus is that lambdas are objects, not functions, so should be named with upper-case, i.e. ReportUnknownArgument.

llvm/lib/Support/CommandLine.cpp Outdated Show resolved Hide resolved
@igorkudrin igorkudrin merged commit de8ee03 into llvm:main Dec 14, 2023
4 checks passed
@igorkudrin igorkudrin deleted the cl-unknown-subcommand branch December 14, 2023 01:29
igorkudrin added a commit that referenced this pull request Dec 14, 2023
Buildbots reported:
.../CommandLine.cpp:1626:13: error: variable 'NearestHandler' set but not used [-Werror,-Wunused-but-set-variable]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants