[analyzer] Prettify checker registration and unittest code

This commit tweaks the interface of `CheckerRegistry::addChecker` to
make it more practical for plugins and tests:
- The most general overload (where `DocsUri` appears in the parameter
  list) is turned into a private method which is only used for loading
  the checkers described in `Checkers.td`. Note that currently _nothing_
  queries the documentation URI from the checker registry (it's only
  used by the logic that generates the clang-tidy documentation, but
  that loads it directly from `Checkers.td` without involving the
  `CheckerRegistry`), so there is no reason to require anyone to provide
  this value. (In fact, it may be a good idea to remove it completely
  from the `CheckerRegistry`.)
- A new public overload is introduced which differs from the most
  general one by omitting the `DocsUri` argument and making the
  `IsHidden` parameter optional (by setting it to `false` by default).
  This will cover the needs of plugins that want maximal customization.
- The templated overload (which takes the checker type as a
  template paarameter + the checker name and description as parameters)
  also loses the `DocsUri` parameter.
- A new method `addMockChecker<T>` is added for use in unit tests.

In addition to propagating these changes (e.g. using `addMockChecker` in
unittests), this commit clarifies, corrects and extends lots of comments
and performs various minor code quality improvements in the code of
unittests and example plugins.

I originally wrote the bulk of this commit when I was planning to add an
extra parameter to `addChecker` in order to implement some technical
details of the CheckerFamily framework. At the end I decided against
adding that extra parameter, so this cleanup was left out of the PR
llvm#139256 and I'm merging it now
as a separate commit (after minor tweaks).

This commit is mostly NFC: the only functional change is that the public
checker registration functions no longer require (or accept) `DocsUri`
as an argument, and the `IsHidden` argument is optional for both public
overloads of `addChecker` (and not just the "basic" templated one).

Author: NagyDonat

clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

@@ -38,29 +38,29 @@
 // function clang_registerCheckers. For example:
 //
 //    extern "C"
-//    void clang_registerCheckers (CheckerRegistry &registry) {
-//      registry.addChecker<MainCallChecker>("example.MainCallChecker",
-//        "Disallows calls to functions called main");
+//    void clang_registerCheckers(CheckerRegistry &Registry) {
+//      Registry.addChecker<MainCallChecker>(
+//                    "example.MainCallChecker",
+//                    "Disallows calls to functions called main");
 //    }
 //
-// The first method argument is the full name of the checker, including its
-// enclosing package. By convention, the registered name of a checker is the
-// name of the associated class (the template argument).
-// The second method argument is a short human-readable description of the
-// checker.
+// The first argument of this templated method is the full name of the checker
+// (including its package), while the second argument is a short description
+// that is printed by `-analyzer-checker-help`.
 //
-// The clang_registerCheckers function may add any number of checkers to the
-// registry. If any checkers require additional initialization, use the three-
-// argument form of CheckerRegistry::addChecker.
+// A plugin may register several separate checkers by calling `addChecker()`
+// multiple times. If a checker requires custom registration functions (e.g.
+// checker option handling) use the non-templated variant of `addChecker` that
+// takes two callback functions as the first two parameters.
 //
 // To load a checker plugin, specify the full path to the dynamic library as
 // the argument to the -load option in the cc1 frontend. You can then enable
 // your custom checker using the -analyzer-checker:
 //
-//   clang -cc1 -load </path/to/plugin.dylib> -analyze
-//     -analyzer-checker=<example.MainCallChecker>
+//   clang -cc1 -load /path/to/plugin.dylib -analyze
+//     -analyzer-checker=example.MainCallChecker
 //
-// For a complete working example, see examples/analyzer-plugin.
+// For complete examples, see clang/lib/Analysis/plugins/SampleAnalyzer
 
 #ifndef CLANG_ANALYZER_API_VERSION_STRING
 // FIXME: The Clang version string is not particularly granular;
@@ -112,26 +112,35 @@ class CheckerRegistry {
     return true;
   }
 
-public:
-  /// Adds a checker to the registry. Use this non-templated overload when your
-  /// checker requires custom initialization.
-  void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction sfn,
+  /// Adds a checker to the registry. This private, most general variant is
+  /// intended for loading the checker definitions from `Checkers.td`.
+  /// FIXME: The checker registr should not bother with loading `DocsUri`
+  /// becaus it is (as of now) never queried from the checker registry.
+  void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction Sfn,
                   StringRef FullName, StringRef Desc, StringRef DocsUri,
                   bool IsHidden);
 
-  /// Adds a checker to the registry. Use this templated overload when your
-  /// checker does not require any custom initialization.
-  /// This function isn't really needed and probably causes more headaches than
-  /// the tiny convenience that it provides, but external plugins might use it,
-  /// and there isn't a strong incentive to remove it.
+public:
+  /// Adds a checker to the registry. Use this for a checker defined in a
+  /// plugin if it requires custom registration functions.
+  void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction Sfn,
+                  StringRef FullName, StringRef Desc, bool IsHidden = false) {
+    addChecker(Fn, Sfn, FullName, Desc, "NoDocsUri", IsHidden);
+  }
+
+  /// Adds a checker to the registry. Use this for a checker defined in a
+  /// plugin if it doesn't require custom registration functions.
   template <class T>
-  void addChecker(StringRef FullName, StringRef Desc, StringRef DocsUri,
-                  bool IsHidden = false) {
-    // Avoid MSVC's Compiler Error C2276:
-    // http://msdn.microsoft.com/en-us/library/850cstw1(v=VS.80).aspx
+  void addChecker(StringRef FullName, StringRef Desc, bool IsHidden = false) {
     addChecker(&CheckerRegistry::initializeManager<CheckerManager, T>,
-               &CheckerRegistry::returnTrue<T>, FullName, Desc, DocsUri,
-               IsHidden);
+               &CheckerRegistry::returnTrue<T>, FullName, Desc,
+               /*IsHidden=*/IsHidden);
+  }
+
+  /// Add a mock checker to the registry for testing purposes, without
+  /// specifying metadata that is not relevant in simple tests.
+  template <class T> void addMockChecker(StringRef FullName) {
+    addChecker<T>(FullName, "MockCheckerDescription");
   }
 
   /// Makes the checker with the full name \p fullName depend on the checker

clang/lib/Analysis/plugins/CheckerDependencyHandling/CheckerDependencyHandling.cpp

@@ -2,6 +2,9 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 
+// This barebones plugin is used by clang/test/Analysis/checker-plugins.c
+// to test dependency handling among checkers loaded from plugins.
+
 using namespace clang;
 using namespace ento;
 
@@ -15,12 +18,11 @@ struct DependendentChecker : public Checker<check::BeginFunction> {
 } // end anonymous namespace
 
 // Register plugin!
-extern "C" void clang_registerCheckers(CheckerRegistry &registry) {
-  registry.addChecker<Dependency>("example.Dependency", "", "");
-  registry.addChecker<DependendentChecker>("example.DependendentChecker", "",
-                                           "");
+extern "C" void clang_registerCheckers(CheckerRegistry &Registry) {
+  Registry.addMockChecker<Dependency>("example.Dependency");
+  Registry.addMockChecker<DependendentChecker>("example.DependendentChecker");
 
-  registry.addDependency("example.DependendentChecker", "example.Dependency");
+  Registry.addDependency("example.DependendentChecker", "example.Dependency");
 }
 
 extern "C" const char clang_analyzerAPIVersionString[] =

clang/lib/Analysis/plugins/CheckerOptionHandling/CheckerOptionHandling.cpp

@@ -5,6 +5,9 @@
 using namespace clang;
 using namespace ento;
 
+// This barebones plugin is used by clang/test/Analysis/checker-plugins.c
+// to test option handling on checkers loaded from plugins.
+
 namespace {
 struct MyChecker : public Checker<check::BeginFunction> {
   void checkBeginFunction(CheckerContext &Ctx) const {}
@@ -25,13 +28,11 @@ bool shouldRegisterMyChecker(const CheckerManager &mgr) { return true; }
 } // end anonymous namespace
 
 // Register plugin!
-extern "C" void clang_registerCheckers(CheckerRegistry &registry) {
-  registry.addChecker(registerMyChecker, shouldRegisterMyChecker,
-                      "example.MyChecker", "Example Description",
-                      "example.mychecker.documentation.nonexistent.html",
-                      /*isHidden*/false);
+extern "C" void clang_registerCheckers(CheckerRegistry &Registry) {
+  Registry.addChecker(registerMyChecker, shouldRegisterMyChecker,
+                      "example.MyChecker", "Example Description");
 
-  registry.addCheckerOption(/*OptionType*/ "bool",
+  Registry.addCheckerOption(/*OptionType*/ "bool",
                             /*CheckerFullName*/ "example.MyChecker",
                             /*OptionName*/ "ExampleOption",
                             /*DefaultValStr*/ "false",

clang/lib/Analysis/plugins/SampleAnalyzer/MainCallChecker.cpp

@@ -3,12 +3,16 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 
+// This simple plugin is used by clang/test/Analysis/checker-plugins.c
+// to test the use of a checker that is defined in a plugin.
+
 using namespace clang;
 using namespace ento;
 
 namespace {
 class MainCallChecker : public Checker<check::PreStmt<CallExpr>> {
-  mutable std::unique_ptr<BugType> BT;
+
+  BugType BT{this, "call to main", "example analyzer plugin"};
 
 public:
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -33,21 +37,17 @@ void MainCallChecker::checkPreStmt(const CallExpr *CE,
     if (!N)
       return;
 
-    if (!BT)
-      BT.reset(new BugType(this, "call to main", "example analyzer plugin"));
-
     auto report =
-        std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
+        std::make_unique<PathSensitiveBugReport>(BT, BT.getDescription(), N);
     report->addRange(Callee->getSourceRange());
     C.emitReport(std::move(report));
   }
 }
 
 // Register plugin!
-extern "C" void clang_registerCheckers(CheckerRegistry &registry) {
-  registry.addChecker<MainCallChecker>(
-      "example.MainCallChecker", "Disallows calls to functions called main",
-      "");
+extern "C" void clang_registerCheckers(CheckerRegistry &Registry) {
+  Registry.addChecker<MainCallChecker>("example.MainCallChecker",
+                                       "Example Description");
 }
 
 extern "C" const char clang_analyzerAPIVersionString[] =

clang/unittests/StaticAnalyzer/BlockEntranceCallbackTest.cpp

@@ -91,8 +91,7 @@ void addBlockEntranceTester(AnalysisASTConsumer &AnalysisConsumer,
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
     Registry.addChecker(&registerChecker<BlockEntranceCallbackTester>,
                         &shouldAlwaysRegister, "test.BlockEntranceTester",
-                        "EmptyDescription", "EmptyDocsUri",
-                        /*IsHidden=*/false);
+                        "EmptyDescription");
   });
 }
 
@@ -102,8 +101,7 @@ void addBranchConditionTester(AnalysisASTConsumer &AnalysisConsumer,
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
     Registry.addChecker(&registerChecker<BranchConditionCallbackTester>,
                         &shouldAlwaysRegister, "test.BranchConditionTester",
-                        "EmptyDescription", "EmptyDocsUri",
-                        /*IsHidden=*/false);
+                        "EmptyDescription");
   });
 }
 

clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp

@@ -119,8 +119,8 @@ class TestAction : public ASTFrontendAction {
         std::make_unique<VerifyPathDiagnosticConsumer>(
             std::move(ExpectedDiags), Compiler.getSourceManager()));
     AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-      Registry.addChecker<InterestingnessTestChecker>("test.Interestingness",
-                                                      "Description", "");
+      Registry.addMockChecker<InterestingnessTestChecker>(
+          "test.Interestingness");
     });
     Compiler.getAnalyzerOpts().CheckersAndPackages = {
         {"test.Interestingness", true}};

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

@@ -616,8 +616,7 @@ void addCallDescChecker(AnalysisASTConsumer &AnalysisConsumer,
                         AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"test.CallDescChecker", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<CallDescChecker>("test.CallDescChecker", "Description",
-                                         "");
+    Registry.addMockChecker<CallDescChecker>("test.CallDescChecker");
   });
 }
 

clang/unittests/StaticAnalyzer/CallEventTest.cpp

@@ -55,8 +55,7 @@ void addCXXDeallocatorChecker(AnalysisASTConsumer &AnalysisConsumer,
                               AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"test.CXXDeallocator", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<CXXDeallocatorChecker>("test.CXXDeallocator",
-                                               "Description", "");
+    Registry.addMockChecker<CXXDeallocatorChecker>("test.CXXDeallocator");
   });
 }
 

clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp

@@ -33,10 +33,8 @@ void addEvalFooCheckers(AnalysisASTConsumer &AnalysisConsumer,
   AnOpts.CheckersAndPackages = {{"test.EvalFoo1", true},
                                 {"test.EvalFoo2", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<EvalCallFoo1>("test.EvalFoo1", "EmptyDescription",
-                                      "EmptyDocsUri");
-    Registry.addChecker<EvalCallFoo2>("test.EvalFoo2", "EmptyDescription",
-                                      "EmptyDocsUri");
+    Registry.addMockChecker<EvalCallFoo1>("test.EvalFoo1");
+    Registry.addMockChecker<EvalCallFoo2>("test.EvalFoo2");
   });
 }
 } // namespace

clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp

@@ -77,26 +77,25 @@ void addExprEngineVisitPreChecker(AnalysisASTConsumer &AnalysisConsumer,
                                   AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"ExprEngineVisitPreChecker", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<ExprEngineVisitPreChecker>("ExprEngineVisitPreChecker",
-                                                   "Desc", "DocsURI");
+    Registry.addMockChecker<ExprEngineVisitPreChecker>(
+        "ExprEngineVisitPreChecker");
   });
 }
 
 void addExprEngineVisitPostChecker(AnalysisASTConsumer &AnalysisConsumer,
                                    AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"ExprEngineVisitPostChecker", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<ExprEngineVisitPostChecker>(
-        "ExprEngineVisitPostChecker", "Desc", "DocsURI");
+    Registry.addMockChecker<ExprEngineVisitPostChecker>(
+        "ExprEngineVisitPostChecker");
   });
 }
 
 void addMemAccessChecker(AnalysisASTConsumer &AnalysisConsumer,
                          AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"MemAccessChecker", true}};
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-    Registry.addChecker<MemAccessChecker>("MemAccessChecker", "Desc",
-                                          "DocsURI");
+    Registry.addMockChecker<MemAccessChecker>("MemAccessChecker");
   });
 }