[analyzer][NFC] Introduce framework for checker families

clang/include/clang/Analysis/ProgramPoint.h

@@ -40,8 +40,8 @@ class ProgramPointTag {
   ProgramPointTag(void *tagKind = nullptr) : TagKind(tagKind) {}
   virtual ~ProgramPointTag();
 
-  /// The description of this program point which will be displayed when the
-  /// ExplodedGraph is dumped in DOT format for debugging.
+  /// The description of this program point which will be dumped for debugging
+  /// purposes. Do not use in user-facing output!
   virtual StringRef getTagDescription() const = 0;
 
   /// Used to implement 'isKind' in subclasses.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

@@ -643,9 +643,10 @@ class BugReporter {
   /// reports.
   virtual void emitReport(std::unique_ptr<BugReport> R);
 
-  void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker,
-                       StringRef BugName, StringRef BugCategory,
-                       StringRef BugStr, PathDiagnosticLocation Loc,
+  void EmitBasicReport(const Decl *DeclWithIssue,
+                       const CheckerFrontend *Checker, StringRef BugName,
+                       StringRef BugCategory, StringRef BugStr,
+                       PathDiagnosticLocation Loc,
                        ArrayRef<SourceRange> Ranges = {},
                        ArrayRef<FixItHint> Fixits = {});
 

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

@@ -27,11 +27,7 @@ class BugReporter;
 
 class BugType {
 private:
-  struct CheckerPartRef {
-    const CheckerBase *Checker;
-    CheckerPartIdx Idx;
-  };
-  using CheckerNameInfo = std::variant<CheckerNameRef, CheckerPartRef>;
+  using CheckerNameInfo = std::variant<CheckerNameRef, const CheckerFrontend *>;
 
   const CheckerNameInfo CheckerName;
   const std::string Description;
@@ -43,7 +39,7 @@ class BugType {
 public:
   // Straightforward constructor where the checker name is specified directly.
   // TODO: As far as I know all applications of this constructor involve ugly
-  // hacks that could be avoided by switching to a different constructor.
+  // hacks that could be avoided by switching to the other constructor.
   // When those are all eliminated, this constructor should be removed to
   // eliminate the `variant` and simplify this class.
   BugType(CheckerNameRef CheckerName, StringRef Desc,
@@ -52,18 +48,11 @@ class BugType {
         SuppressOnSink(SuppressOnSink) {}
   // Constructor that can be called from the constructor of a checker object.
   // At that point the checker name is not yet available, but we can save a
-  // pointer to the checker and later use that to query the name.
-  BugType(const CheckerBase *Checker, StringRef Desc,
+  // pointer to the checker and use that to query the name.
+  BugType(const CheckerFrontend *Checker, StringRef Desc,
           StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
-      : CheckerName(CheckerPartRef{Checker, DefaultPart}), Description(Desc),
-        Category(Cat), SuppressOnSink(SuppressOnSink) {}
-  // Constructor that can be called from the constructor of a checker object
-  // when it has multiple parts with separate names. We save the name and the
-  // part index to be able to query the name of that part later.
-  BugType(const CheckerBase *Checker, CheckerPartIdx Idx, StringRef Desc,
-          StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
-      : CheckerName(CheckerPartRef{Checker, Idx}), Description(Desc),
-        Category(Cat), SuppressOnSink(SuppressOnSink) {}
+      : CheckerName(Checker), Description(Desc), Category(Cat),
+        SuppressOnSink(SuppressOnSink) {}
   virtual ~BugType() = default;
 
   StringRef getDescription() const { return Description; }
@@ -72,8 +61,7 @@ class BugType {
     if (const auto *CNR = std::get_if<CheckerNameRef>(&CheckerName))
       return *CNR;
 
-    auto [Checker, Idx] = std::get<CheckerPartRef>(CheckerName);
-    return Checker->getName(Idx);
+    return std::get<const CheckerFrontend *>(CheckerName)->getName();
   }
 
   /// isSuppressOnSink - Returns true if bug reports associated with this bug
@@ -82,6 +70,21 @@ class BugType {
   bool isSuppressOnSink() const { return SuppressOnSink; }
 };
 
+/// Trivial convenience class for the common case when a certain checker
+/// frontend always uses the same bug type. This way instead of writing
+/// ```
+///   CheckerFrontend LongCheckerFrontendName;
+///   BugType LongCheckerFrontendNameBug{LongCheckerFrontendName, "..."};
+/// ```
+/// we can use `CheckerFrontendWithBugType LongCheckerFrontendName{"..."}`.
+class CheckerFrontendWithBugType : public CheckerFrontend, public BugType {
+public:
+  CheckerFrontendWithBugType(StringRef Desc,
+                             StringRef Cat = categories::LogicError,
+                             bool SuppressOnSink = false)
+      : BugType(this, Desc, Cat, SuppressOnSink) {}
+};
+
 } // namespace ento
 
 } // end clang namespace

clang/include/clang/StaticAnalyzer/Core/Checker.h

@@ -484,83 +484,90 @@ class Call {
 
 } // end eval namespace
 
-class CheckerBase : public ProgramPointTag {
-  /// A single checker class (i.e. a subclass of `CheckerBase`) can implement
-  /// multiple user-facing checkers that have separate names and can be enabled
-  /// separately, but are backed by the same singleton checker object.
-  SmallVector<std::optional<CheckerNameRef>, 1> RegisteredNames;
-
-  friend class ::clang::ento::CheckerManager;
+/// A `CheckerFrontend` instance is what the user recognizes as "one checker":
+/// it has a public canonical name (injected from the `CheckerManager`), can be
+/// enabled or disabled, can have associated checker options and can be printed
+/// as the "source" of bug reports.
+/// The singleton instance of a simple `Checker<...>` is-a `CheckerFrontend`
+/// (for historical reasons, to preserve old straightforward code), while the
+/// singleton instance of a `CheckerFamily<...>` class owns multiple
+/// `CheckerFrontend` instances as data members.
+/// Modeling checkers that are hidden from the user but can be enabled or
+/// disabled separately (as dependencies of other checkers) are also considered
+/// to be `CheckerFrontend`s.
+class CheckerFrontend {
+  /// The `Name` is nullopt if and only if the checker is disabled.
+  std::optional<CheckerNameRef> Name;
 
 public:
-  CheckerNameRef getName(CheckerPartIdx Idx = DefaultPart) const {
-    assert(Idx < RegisteredNames.size() && "Checker part index is too large!");
-    std::optional<CheckerNameRef> Name = RegisteredNames[Idx];
-    assert(Name && "Requested checker part is not registered!");
-    return *Name;
-  }
-
-  bool isPartEnabled(CheckerPartIdx Idx) const {
-    return Idx < RegisteredNames.size() && RegisteredNames[Idx].has_value();
-  }
-
-  void registerCheckerPart(CheckerPartIdx Idx, CheckerNameRef Name) {
-    // Paranoia: notice if e.g. UINT_MAX is passed as a checker part index.
-    assert(Idx < 256 && "Checker part identifiers should be small integers.");
-
-    if (Idx >= RegisteredNames.size())
-      RegisteredNames.resize(Idx + 1, std::nullopt);
-
-    assert(!RegisteredNames[Idx] && "Repeated registration of checker a part!");
-    RegisteredNames[Idx] = Name;
-  }
-
-  StringRef getTagDescription() const override {
-    // When the ExplodedGraph is dumped for debugging (in DOT format), this
-    // method is called to attach a description to nodes created by this
-    // checker _class_. Ideally this should be recognizable identifier of the
-    // whole class, but for this debugging purpose it's sufficient to use the
-    // name of the first registered checker part.
-    for (const auto &OptName : RegisteredNames)
-      if (OptName)
-        return *OptName;
-
-    return "Unregistered checker";
+  void enable(CheckerManager &Mgr) {
+    assert(!Name && "Checker part registered twice!");
+    Name = Mgr.getCurrentCheckerName();
   }
+  bool isEnabled() const { return Name.has_value(); }
+  CheckerNameRef getName() const { return *Name; }
+};
 
+/// `CheckerBackend` is an abstract base class that serves as the common
+/// ancestor of all the `Checker<...>` and `CheckerFamily<...>` classes which
+/// can create `ExplodedNode`s (by acting as a `ProgramPointTag`) and can be
+/// registered to handle various checker callbacks. (Moreover the debug
+/// callback `printState` is also introduced here.)
+class CheckerBackend : public ProgramPointTag {
+public:
   /// Debug state dump callback, see CheckerManager::runCheckersForPrintState.
   /// Default implementation does nothing.
   virtual void printState(raw_ostream &Out, ProgramStateRef State,
                           const char *NL, const char *Sep) const;
 };
 
-/// Dump checker name to stream.
-raw_ostream& operator<<(raw_ostream &Out, const CheckerBase &Checker);
-
-/// Tag that can use a checker name as a message provider
-/// (see SimpleProgramPointTag).
-class CheckerProgramPointTag : public SimpleProgramPointTag {
+/// The non-templated common ancestor of all the simple `Checker<...>` classes.
+class CheckerBase : public CheckerFrontend, public CheckerBackend {
 public:
-  CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
-  CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
+  /// Attached to nodes created by this checker class when the ExplodedGraph is
+  /// dumped for debugging.
+  StringRef getTagDescription() const override;
 };
 
-template <typename CHECK1, typename... CHECKs>
-class Checker : public CHECK1, public CHECKs..., public CheckerBase {
+/// Simple checker classes that implement one frontend (i.e. checker name)
+/// should derive from this template and specify all the implemented callbacks
+/// (i.e. classes like `check::PreStmt` or `eval::Call`) as template arguments
+/// of `Checker`.
+template <typename... CHECKs>
+class Checker : public CheckerBase, public CHECKs... {
 public:
   template <typename CHECKER>
-  static void _register(CHECKER *checker, CheckerManager &mgr) {
-    CHECK1::_register(checker, mgr);
-    Checker<CHECKs...>::_register(checker, mgr);
+  static void _register(CHECKER *Chk, CheckerManager &Mgr) {
+    (CHECKs::_register(Chk, Mgr), ...);
   }
 };
 
-template <typename CHECK1>
-class Checker<CHECK1> : public CHECK1, public CheckerBase {
+/// Checker families (where a single backend class implements multiple related
+/// frontends) should derive from this template and specify all the implemented
+/// callbacks (i.e. classes like `check::PreStmt` or `eval::Call`) as template
+/// arguments of `FamilyChecker.`
+///
+/// NOTE: Classes deriving from `CheckerFamily` must implement the pure
+/// virtual method `StringRef getTagDescription()` which is inherited from
+/// `ProgramPointTag` and should return the name of the class as a string.
+///
+/// Obviously, this boilerplate is not a good thing, but unfortunately there is
+/// no portable way to stringify the name of a type (e.g. class), so any
+/// portable implementation of `getTagDescription` would need to take the
+/// name of the class from *somewhere* where it's present as a string -- and
+/// then directly placing it in a method override is much simpler than
+/// loading it from `Checkers.td`.
+///
+/// Note that the existing `CLASS` field in `Checkers.td` is not suitable for
+/// our goals, because instead of storing the same class name for each
+/// frontend, in fact each frontendchecker needs to have its own unique value
+/// there (to ensure that the names of the register methods are all unique).
+template <typename... CHECKs>
+class CheckerFamily : public CheckerBackend, public CHECKs... {
 public:
   template <typename CHECKER>
-  static void _register(CHECKER *checker, CheckerManager &mgr) {
-    CHECK1::_register(checker, mgr);
+  static void _register(CHECKER *Chk, CheckerManager &Mgr) {
+    (CHECKs::_register(Chk, Mgr), ...);
   }
 };
 
@@ -581,6 +588,20 @@ class EventDispatcher {
   }
 };
 
+/// Tag that can use a checker name as a message provider
+/// (see SimpleProgramPointTag).
+/// FIXME: This is a cargo cult class which is copied into several checkers but
+/// does not provide anything useful.
+/// The only added functionality provided by this class (compared to
+/// SimpleProgramPointTag) is that it composes the tag description string from
+/// two arguments -- but tag descriptions only appear in debug output so there
+/// is no reason to bother with this.
+class CheckerProgramPointTag : public SimpleProgramPointTag {
+public:
+  CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
+  CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
+};
+
 /// We dereferenced a location that may be null.
 struct ImplicitNullDerefEvent {
   SVal Location;