[clang-tooling] Prevent llvm::fatal_error on invalid CLI option

[serge-sans-paille]

Summary

Fail gracefully instead. Prevent further misuse by enforcing the factory builder instead of the constructor.

[serge-sans-paille]

ping ? This change looks harmless to me, I'd appreciate an ack though.

[alexfh]

This is quite a bit of extra boilerplate, but it results in a more transparent error handling. However, the documentation would need to be changed (clang/docs/LibASTMatchersTutorial.rst). And what about other tools (e.g. clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp, clang-tools-extra/clang-include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp, etc.)?

[serge-sans-paille]

Address doc + tool-extra issues

[serge-sans-paille]

Thanks @alexfh ! Should be good now.

[serge-sans-paille]

@alexfh gentle ping?

[alexfh]

Thanks! Looks good now.

[nridge]

It would be nice to update the code example in the LibTooling documentation as well

clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp

for (auto Line : Lines)
     Patterns.push_back(std::string(Line.trim()));
   return Patterns;
 }
 
 } // anonymous namespace
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
-  tooling::CommonOptionsParser OptionsParser(argc, argv,
-                                             ChangeNamespaceCategory);
+  auto ExpectedParser =
+      tooling::CommonOptionsParser::create(argc, argv, ChangeNamespaceCategory);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  tooling::CommonOptionsParser &OptionsParser = ExpectedParser.get();
   const auto &Files = OptionsParser.getSourcePathList();
   tooling::RefactoringTool Tool(OptionsParser.getCompilations(), Files);
   llvm::ErrorOr<std::vector<std::string>> AllowedPatterns =
       GetAllowedSymbolPatterns();
   if (!AllowedPatterns) {
     llvm::errs() << "Failed to open allow file " << AllowedFile << ". "
                  << AllowedPatterns.getError().message() << "\n";
     return 1;

clang-tools-extra/clang-include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp

bool Merge(llvm::StringRef MergeDir, llvm::StringRef OutputFile) {
   WriteSymbolInfosToStream(OS, Symbols);
   return true;
 }
 
 } // namespace clang
 } // namespace find_all_symbols
 
 int main(int argc, const char **argv) {
-  CommonOptionsParser OptionsParser(argc, argv, FindAllSymbolsCategory);
+  auto ExpectedParser =
+      CommonOptionsParser::create(argc, argv, FindAllSymbolsCategory);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+
+  CommonOptionsParser &OptionsParser = ExpectedParser.get();
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
 
   std::vector<std::string> sources = OptionsParser.getSourcePathList();
   if (sources.empty()) {
     llvm::errs() << "Must specify at least one one source file.\n";
     return 1;
   }

clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp

if (&Info != &HeaderInfos.back())
       OS << ",\n";
   }
   OS << "\n";
   OS << "  ]\n";
   OS << "}\n";
 }
 
 int includeFixerMain(int argc, const char **argv) {
-  tooling::CommonOptionsParser options(argc, argv, IncludeFixerCategory);
+  auto ExpectedParser =
+      tooling::CommonOptionsParser::create(argc, argv, IncludeFixerCategory);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  tooling::CommonOptionsParser &options = ExpectedParser.get();
   tooling::ClangTool tool(options.getCompilations(),
                           options.getSourcePathList());
 
   llvm::StringRef SourceFilePath = options.getSourcePathList().front();
   // In STDINMode, we override the file content with the <stdin> input.
   // Since `tool.mapVirtualFile` takes `StringRef`, we define `Code` outside of
   // the if-block so that `Code` is not released after the if-block.
   std::unique_ptr<llvm::MemoryBuffer> Code;

clang-tools-extra/clang-move/tool/ClangMove.cpp

cl::desc("Dump all declarations in old header (JSON format) to stdout. If "
              "the option is specified, other command options will be ignored. "
              "An empty JSON will be returned if old header isn't specified."),
     cl::cat(ClangMoveCategory));
 
 } // namespace
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
-  tooling::CommonOptionsParser OptionsParser(argc, argv, ClangMoveCategory);
+  auto ExpectedParser =
+      tooling::CommonOptionsParser::create(argc, argv, ClangMoveCategory);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  tooling::CommonOptionsParser &OptionsParser = ExpectedParser.get();
 
   if (OldDependOnNew && NewDependOnOld) {
     llvm::errs() << "Provide either --old_depend_on_new or "
                     "--new_depend_on_old. clang-move doesn't support these two "
                     "options at same time (It will introduce include cycle).\n";
     return 1;
   }
 

clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp

static cl::list<std::string> FieldsOrder("fields-order", cl::CommaSeparated,
                                          cl::cat(ClangReorderFieldsCategory));
 
 static cl::opt<bool> Inplace("i", cl::desc("Overwrite edited files."),
                              cl::cat(ClangReorderFieldsCategory));
 
 const char Usage[] = "A tool to reorder fields in C/C++ structs/classes.\n";
 
 int main(int argc, const char **argv) {
-  tooling::CommonOptionsParser OP(argc, argv, ClangReorderFieldsCategory,
-                                  Usage);
+  auto ExpectedParser = tooling::CommonOptionsParser::create(
+      argc, argv, ClangReorderFieldsCategory, cl::OneOrMore, Usage);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+
+  tooling::CommonOptionsParser &OP = ExpectedParser.get();
 
   auto Files = OP.getSourcePathList();
   tooling::RefactoringTool Tool(OP.getCompilations(), Files);
 
   reorder_fields::ReorderFieldsAction Action(RecordName, FieldsOrder,
                                              Tool.getReplacements());
 
   auto Factory = tooling::newFrontendActionFactory(&Action);

clang-tools-extra/test/clang-query/invalid-command-line.cpp

 // RUN: not clang-query --invalid-arg 2>&1 | FileCheck %s
 
-// CHECK: error: [CommonOptionsParser]: clang-query{{(\.exe)?}}: Unknown command line argument '--invalid-arg'.  Try: 'clang-query{{(\.exe)?}} --help'
+// CHECK: error: clang-query{{(\.exe)?}}: Unknown command line argument '--invalid-arg'.  Try: 'clang-query{{(\.exe)?}} --help'
 // CHECK-NEXT: clang-query{{(\.exe)?}}: Did you mean '--extra-arg'?

clang-tools-extra/test/clang-tidy/infrastructure/invalid-command-line.cpp

 // RUN: not clang-tidy --invalid-arg 2>&1 | FileCheck %s
 
-// CHECK: error: [CommonOptionsParser]: clang-tidy{{(\.exe)?}}: Unknown command line argument '--invalid-arg'.  Try: 'clang-tidy{{(\.exe)?}} --help'
+// CHECK: error: clang-tidy{{(\.exe)?}}: Unknown command line argument '--invalid-arg'.  Try: 'clang-tidy{{(\.exe)?}} --help'
 // CHECK-NEXT: clang-tidy{{(\.exe)?}}: Did you mean '--extra-arg'?

clang/docs/LibASTMatchersTutorial.rst

.. code-block:: c++
       // command-line options related to the compilation database and input files.
       // It's nice to have this help message in all tools.
       static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
 
       // A help message for this specific tool can be added afterwards.
       static cl::extrahelp MoreHelp("\nMore help text...\n");
 
       int main(int argc, const char **argv) {
-        CommonOptionsParser OptionsParser(argc, argv, MyToolCategory);
+        auto ExpectedParser = CommonOptionsParser::create(argc, argv, MyToolCategory);
+        if (!ExpectedParser) {
+          // Fail gracefully for unsupported options.
+          llvm::errs() << ExpectedParser.takeError();
+          return 1;
+        }
+        CommonOptionsParser& OptionsParser = ExpectedParser.get();
         ClangTool Tool(OptionsParser.getCompilations(),
                        OptionsParser.getSourcePathList());
         return Tool.run(newFrontendActionFactory<clang::SyntaxOnlyAction>().get());
       }
 
 And that's it! You can compile our new tool by running ninja from the
 ``build`` directory.
 
.. code-block:: c++
         }
       };
 
 And change ``main()`` to:
 
 .. code-block:: c++
 
       int main(int argc, const char **argv) {
-        CommonOptionsParser OptionsParser(argc, argv, MyToolCategory);
+        auto ExpectedParser = CommonOptionsParser::create(argc, argv, MyToolCategory);
+        if (!ExpectedParser) {
+          // Fail gracefully for unsupported options.
+          llvm::errs() << ExpectedParser.takeError();
+          return 1;
+        }
+        CommonOptionsParser& OptionsParser = ExpectedParser.get();
         ClangTool Tool(OptionsParser.getCompilations(),
                        OptionsParser.getSourcePathList());
 
         LoopPrinter Printer;
         MatchFinder Finder;
         Finder.addMatcher(LoopMatcher, &Printer);
 
         return Tool.run(newFrontendActionFactory(&Finder).get());

clang/include/clang/Tooling/CommonOptionsParser.h

 /// int main(int argc, const char **argv) {
 ///   CommonOptionsParser OptionsParser(argc, argv, MyToolCategory);
 ///   ClangTool Tool(OptionsParser.getCompilations(),
 ///                  OptionsParser.getSourcePathList());
 ///   return Tool.run(newFrontendActionFactory<SyntaxOnlyAction>().get());
 /// }
 /// \endcode
 class CommonOptionsParser {
-public:
-  /// Parses command-line, initializes a compilation database.
-  ///
-  /// This constructor can change argc and argv contents, e.g. consume
-  /// command-line options used for creating FixedCompilationDatabase.
-  ///
-  /// All options not belonging to \p Category become hidden.
-  ///
-  /// This constructor exits program in case of error.
-  CommonOptionsParser(int &argc, const char **argv,
-                      llvm::cl::OptionCategory &Category,
-                      const char *Overview = nullptr)
-      : CommonOptionsParser(argc, argv, Category, llvm::cl::OneOrMore,
-                            Overview) {}
 
+protected:
   /// Parses command-line, initializes a compilation database.
   ///
   /// This constructor can change argc and argv contents, e.g. consume
   /// command-line options used for creating FixedCompilationDatabase.
   ///
   /// All options not belonging to \p Category become hidden.
   ///
   /// It also allows calls to set the required number of positional parameters.
-  CommonOptionsParser(int &argc, const char **argv,
-                      llvm::cl::OptionCategory &Category,
-                      llvm::cl::NumOccurrencesFlag OccurrencesFlag,
-                      const char *Overview = nullptr);
+  CommonOptionsParser(
+      int &argc, const char **argv, llvm::cl::OptionCategory &Category,
+      llvm::cl::NumOccurrencesFlag OccurrencesFlag = llvm::cl::OneOrMore,
 
+public:
   /// A factory method that is similar to the above constructor, except
   /// this returns an error instead exiting the program on error.
   static llvm::Expected<CommonOptionsParser>
   create(int &argc, const char **argv, llvm::cl::OptionCategory &Category,
-         llvm::cl::NumOccurrencesFlag OccurrencesFlag,
+         llvm::cl::NumOccurrencesFlag OccurrencesFlag = llvm::cl::OneOrMore,
          const char *Overview = nullptr);
 
   /// Returns a reference to the loaded compilations database.
   CompilationDatabase &getCompilations() {
     return *Compilations;
   }
 
   /// Returns a list of source file paths to process.

clang/lib/Tooling/CommonOptionsParser.cpp

llvm::Error CommonOptionsParser::init(
   Compilations =
       FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
   if (!ErrorMessage.empty())
     ErrorMessage.append("\n");
   llvm::raw_string_ostream OS(ErrorMessage);
   // Stop initializing if command-line option parsing failed.
   if (!cl::ParseCommandLineOptions(argc, argv, Overview, &OS)) {
     OS.flush();
-    return llvm::make_error<llvm::StringError>("[CommonOptionsParser]: " +
-                                                   ErrorMessage,
+    return llvm::make_error<llvm::StringError>(ErrorMessage,
                                                llvm::inconvertibleErrorCode());
   }
 
   cl::PrintOptionValues();
 
   SourcePathList = SourcePaths;
   if ((OccurrencesFlag == cl::ZeroOrMore || OccurrencesFlag == cl::Optional) &&
       SourcePathList.empty())

clang/tools/clang-check/ClangCheck.cpp

int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   // Initialize targets for clang module support.
   llvm::InitializeAllTargets();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmPrinters();
   llvm::InitializeAllAsmParsers();
 
-  CommonOptionsParser OptionsParser(argc, argv, ClangCheckCategory);
+  auto ExpectedParser =
+      CommonOptionsParser::create(argc, argv, ClangCheckCategory);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  CommonOptionsParser &OptionsParser = ExpectedParser.get();
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
 
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
   Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
   Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

 int main(int argc, const char **argv) {
   // Print a stack trace if we signal out.
   sys::PrintStackTraceOnErrorSignal(argv[0], false);
   PrettyStackTraceProgram X(argc, argv);
 
   const char *Overview = "\nThis tool collects the USR name and location "
                          "of external definitions in the source files "
                          "(excluding headers).\n";
-  CommonOptionsParser OptionsParser(argc, argv, ClangExtDefMapGenCategory,
-                                    cl::ZeroOrMore, Overview);
+  auto ExpectedParser = CommonOptionsParser::create(
+      argc, argv, ClangExtDefMapGenCategory, cl::ZeroOrMore, Overview);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  CommonOptionsParser &OptionsParser = ExpectedParser.get();
 
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
 
   return Tool.run(newFrontendActionFactory<MapExtDefNamesAction>().get());
 }

clang/tools/clang-refactor/ClangRefactor.cpp

 
 } // end anonymous namespace
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   ClangRefactorTool RefactorTool;
 
-  CommonOptionsParser Options(
+  auto ExpectedParser = CommonOptionsParser::create(
       argc, argv, cl::GeneralCategory, cl::ZeroOrMore,
       "Clang-based refactoring tool for C, C++ and Objective-C");
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  CommonOptionsParser &Options = ExpectedParser.get();
 
   if (auto Err = RefactorTool.Init()) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
     return 1;
   }
 
   auto ActionFactory = RefactorTool.getFrontendActionFactory();
   if (!ActionFactory) {

clang/tools/clang-rename/ClangRename.cpp

 static cl::opt<std::string>
     Input("input", cl::desc("YAML file to load oldname-newname pairs from."),
           cl::Optional, cl::cat(ClangRenameOptions));
 static cl::opt<bool> Force("force",
                            cl::desc("Ignore nonexistent qualified names."),
                            cl::cat(ClangRenameOptions));
 
 int main(int argc, const char **argv) {
-  tooling::CommonOptionsParser OP(argc, argv, ClangRenameOptions);
+  auto ExpectedParser =
+      tooling::CommonOptionsParser::create(argc, argv, ClangRenameOptions);
+  if (!ExpectedParser) {
+    llvm::errs() << ExpectedParser.takeError();
+    return 1;
+  }
+  tooling::CommonOptionsParser &OP = ExpectedParser.get();
 
   if (!Input.empty()) {
     // Populate QualifiedNames and NewNames from a YAML file.
     ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
         llvm::MemoryBuffer::getFile(Input);
     if (!Buffer) {
       errs() << "clang-rename: failed to read " << Input << ": "
              << Buffer.getError().message() << "\n";