[analyzer][NFC] Introduce framework for checker families (llvm#139256)

The checker classes (i.e. classes derived from `CheckerBase` via the
utility template `Checker<...>`) act as intermediates between the user
and the analyzer engine, so they have two interfaces:
- On the frontend side, they have a public name, can be enabled or
disabled, can accept checker options and can be reported as the source
of bug reports.
- On the backend side, they can handle various checker callbacks and
they "leave a mark" on the `ExplodedNode`s that are created by them.
(These `ProgramPointTag` marks are internal: they appear in debug logs
and can be queried by checker logic; but the user doesn't see them.)

In a significant majority of the checkers there is 1:1 correspondence
between these sides, but there are also many checker classes where
several related user-facing checkers share the same backend class.
Historically each of these "multi-part checker" classes had its own
hacks to juggle its multiple names, which led to lots of ugliness like
lazy initialization of `mutable std::unique_ptr<BugType>` members and
redundant data members (when a checker used its custom `CheckNames`
array and ignored the inherited single `Name`).

My recent commit 2709998 tried to unify
and standardize these existing solutions to get rid of some of the
technical debt, but it still used enum values to identify the checker
parts within a "multi-part" checker class, which led to some ugliness.

This commit introduces a new framework which takes a more direct,
object-oriented approach: instead of identifying checker parts with
`{parent checker object, index of part}` pairs, the parts of a
multi-part checker become stand-alone objects that store their own name
(and enabled/disabled status) as a data member.

This is implemented by separating the functionality of `CheckerBase`
into two new classes: `CheckerFrontend` and `CheckerBackend`. The name
`CheckerBase` is kept (as a class derived from both `CheckerFrontend`
and `CheckerBackend`), so "simple" checkers that use `CheckerBase` and
`Checker<...>` continues to work without changes. However we also get
first-class support for the "many frontends - one backend" situation:
- The class `CheckerFamily<...>` works exactly like `Checker<...>` but
inherits from `CheckerBackend` instead of `CheckerBase`, so it won't
have a superfluous single `Name` member.
- Classes deriving from `CheckerFamily` can freely own multiple
`CheckerFrontend` data members, which are enabled within the
registration methods corresponding to their name and can be used to
initialize the `BugType`s that they can emit.

In this scheme each `CheckerFamily` needs to override the pure virtual
method `ProgramPointTag::getTagDescription()` which returns a string
which represents that class for debugging purposes. (Previously this
used the name of one arbitrary sub-checker, which was passable for
debugging purposes, but not too elegant.)

I'm planning to implement follow-up commits that convert all the
"multi-part" checkers to this `CheckerFamily` framework.

Author: NagyDonat

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;