[NFC][analyzer] Multipart checker refactor 2: NullabilityChecker

[NagyDonat]

Simplify NullabilityChecker.cpp with new multipart checker framework introduced in 2709998. This is part of a commit series that will perform analogous changes in all checker classes that implement multiple user-facing checker parts (with separate names).

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

Simplify NullabilityChecker.cpp with new multipart checker framework introduced in 2709998. This is part of a commit series that will perform analogous changes in all checker classes that implement multiple user-facing checker parts (with separate names).

---

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

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (+81-76)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 04472bb3895a7..8fe7d21ca7984 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -112,25 +112,38 @@ class NullabilityChecker
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 
-  enum CheckKind {
-    CK_NullPassedToNonnull,
-    CK_NullReturnedFromNonnull,
-    CK_NullableDereferenced,
-    CK_NullablePassedToNonnull,
-    CK_NullableReturnedFromNonnull,
-    CK_NumCheckKinds
+  // FIXME: This enumeration of checker parts is extremely similar to the
+  // ErrorKind enum. It would be nice to unify them to simplify the code.
+  // FIXME: The modeling checker NullabilityBase is a dummy "empty checker
+  // part" that registers this checker class without enabling any of the real
+  // checker parts. As far as I understand no other checker references it, so
+  // it should be removed.
+  enum : CheckerPartIdx {
+    NullabilityBase,
+    NullPassedToNonnullChecker,
+    NullReturnedFromNonnullChecker,
+    NullableDereferencedChecker,
+    NullablePassedToNonnullChecker,
+    NullableReturnedFromNonnullChecker,
+    NumCheckerParts
   };
 
-  bool ChecksEnabled[CK_NumCheckKinds] = {false};
-  CheckerNameRef CheckNames[CK_NumCheckKinds];
-  mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
-
-  const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
-    if (!BTs[Kind])
-      BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
-                                  categories::MemoryError));
-    return BTs[Kind];
-  }
+  // FIXME: Currently the `Description` fields of these `BugType`s are all
+  // identical ("Nullability") -- they should be more descriptive than this.
+  // NOTE: NullabilityBase is a dummy checker part that does nothing, so its
+  // bug type is left empty.
+  BugType BugTypes[NumCheckerParts] = {
+      {this, NullabilityBase, "", ""},
+      {this, NullPassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableDereferencedChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullablePassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError}};
 
   // When set to false no nullability information will be tracked in
   // NullabilityMap. It is possible to catch errors like passing a null pointer
@@ -163,17 +176,16 @@ class NullabilityChecker
   ///
   /// When \p SuppressPath is set to true, no more bugs will be reported on this
   /// path by this checker.
-  void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
-                                 ExplodedNode *N, const MemRegion *Region,
-                                 CheckerContext &C,
+  void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error,
+                                 CheckerPartIdx Idx, ExplodedNode *N,
+                                 const MemRegion *Region, CheckerContext &C,
                                  const Stmt *ValueExpr = nullptr,
                                  bool SuppressPath = false) const;
 
-  void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
-                 const MemRegion *Region, BugReporter &BR,
+  void reportBug(StringRef Msg, ErrorKind Error, CheckerPartIdx Idx,
+                 ExplodedNode *N, const MemRegion *Region, BugReporter &BR,
                  const Stmt *ValueExpr = nullptr) const {
-    const std::unique_ptr<BugType> &BT = getBugType(CK);
-    auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
+    auto R = std::make_unique<PathSensitiveBugReport>(BugTypes[Idx], Msg, N);
     if (Region) {
       R->markInteresting(Region);
       R->addVisitor<NullabilityBugVisitor>(Region);
@@ -479,7 +491,7 @@ static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N,
 }
 
 void NullabilityChecker::reportBugIfInvariantHolds(
-    StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
+    StringRef Msg, ErrorKind Error, CheckerPartIdx Idx, ExplodedNode *N,
     const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr,
     bool SuppressPath) const {
   ProgramStateRef OriginalState = N->getState();
@@ -491,7 +503,7 @@ void NullabilityChecker::reportBugIfInvariantHolds(
     N = C.addTransition(OriginalState, N);
   }
 
-  reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
+  reportBug(Msg, Error, Idx, N, Region, C.getBugReporter(), ValueExpr);
 }
 
 /// Cleaning up the program state.
@@ -545,19 +557,19 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
   if (!TrackedNullability)
     return;
 
-  if (ChecksEnabled[CK_NullableDereferenced] &&
+  if (isPartEnabled(NullableDereferencedChecker) &&
       TrackedNullability->getValue() == Nullability::Nullable) {
     BugReporter &BR = *Event.BR;
     // Do not suppress errors on defensive code paths, because dereferencing
     // a nullable pointer is always an error.
     if (Event.IsDirectDereference)
       reportBug("Nullable pointer is dereferenced",
-                ErrorKind::NullableDereferenced, CK_NullableDereferenced,
+                ErrorKind::NullableDereferenced, NullableDereferencedChecker,
                 Event.SinkNode, Region, BR);
     else {
       reportBug("Nullable pointer is passed to a callee that requires a "
                 "non-null",
-                ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
+                ErrorKind::NullablePassedToNonnull, NullableDereferencedChecker,
                 Event.SinkNode, Region, BR);
     }
   }
@@ -711,7 +723,8 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
 
   bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
                                   Nullness == NullConstraint::IsNull);
-  if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
+  if (isPartEnabled(NullReturnedFromNonnullChecker) &&
+      NullReturnedFromNonNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       !InSuppressedMethodFamily) {
     static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
@@ -725,7 +738,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
     OS << " returned from a " << C.getDeclDescription(D) <<
           " that is expected to return a non-null value";
     reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
-                              CK_NullReturnedFromNonnull, N, nullptr, C,
+                              NullReturnedFromNonnullChecker, N, nullptr, C,
                               RetExpr);
     return;
   }
@@ -746,7 +759,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
       State->get<NullabilityMap>(Region);
   if (TrackedNullability) {
     Nullability TrackedNullabValue = TrackedNullability->getValue();
-    if (ChecksEnabled[CK_NullableReturnedFromNonnull] &&
+    if (isPartEnabled(NullableReturnedFromNonnullChecker) &&
         Nullness != NullConstraint::IsNotNull &&
         TrackedNullabValue == Nullability::Nullable &&
         RequiredNullability == Nullability::Nonnull) {
@@ -759,7 +772,8 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
             " that is expected to return a non-null value";
 
       reportBugIfInvariantHolds(OS.str(), ErrorKind::NullableReturnedToNonnull,
-                                CK_NullableReturnedFromNonnull, N, Region, C);
+                                NullableReturnedFromNonnullChecker, N, Region,
+                                C);
     }
     return;
   }
@@ -810,7 +824,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
 
     unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
 
-    if (ChecksEnabled[CK_NullPassedToNonnull] &&
+    if (isPartEnabled(NullPassedToNonnullChecker) &&
         Nullness == NullConstraint::IsNull &&
         ArgExprTypeLevelNullability != Nullability::Nonnull &&
         RequiredNullability == Nullability::Nonnull &&
@@ -825,7 +839,8 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
       OS << " passed to a callee that requires a non-null " << ParamIdx
          << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
       reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull,
-                                CK_NullPassedToNonnull, N, nullptr, C, ArgExpr,
+                                NullPassedToNonnullChecker, N, nullptr, C,
+                                ArgExpr,
                                 /*SuppressPath=*/false);
       return;
     }
@@ -842,7 +857,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
           TrackedNullability->getValue() != Nullability::Nullable)
         continue;
 
-      if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+      if (isPartEnabled(NullablePassedToNonnullChecker) &&
           RequiredNullability == Nullability::Nonnull &&
           isDiagnosableCall(Call)) {
         ExplodedNode *N = C.addTransition(State);
@@ -851,16 +866,16 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
         OS << "Nullable pointer is passed to a callee that requires a non-null "
            << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
         reportBugIfInvariantHolds(OS.str(), ErrorKind::NullablePassedToNonnull,
-                                  CK_NullablePassedToNonnull, N, Region, C,
+                                  NullablePassedToNonnullChecker, N, Region, C,
                                   ArgExpr, /*SuppressPath=*/true);
         return;
       }
-      if (ChecksEnabled[CK_NullableDereferenced] &&
+      if (isPartEnabled(NullableDereferencedChecker) &&
           Param->getType()->isReferenceType()) {
         ExplodedNode *N = C.addTransition(State);
         reportBugIfInvariantHolds("Nullable pointer is dereferenced",
                                   ErrorKind::NullableDereferenced,
-                                  CK_NullableDereferenced, N, Region, C,
+                                  NullableDereferencedChecker, N, Region, C,
                                   ArgExpr, /*SuppressPath=*/true);
         return;
       }
@@ -1295,7 +1310,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
 
   bool NullAssignedToNonNull = (LocNullability == Nullability::Nonnull &&
                                 RhsNullness == NullConstraint::IsNull);
-  if (ChecksEnabled[CK_NullPassedToNonnull] && NullAssignedToNonNull &&
+  if (isPartEnabled(NullPassedToNonnullChecker) && NullAssignedToNonNull &&
       ValNullability != Nullability::Nonnull &&
       ValueExprTypeLevelNullability != Nullability::Nonnull &&
       !isARCNilInitializedLocal(C, S)) {
@@ -1314,7 +1329,8 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
     OS << (LocType->isObjCObjectPointerType() ? "nil" : "Null");
     OS << " assigned to a pointer which is expected to have non-null value";
     reportBugIfInvariantHolds(OS.str(), ErrorKind::NilAssignedToNonnull,
-                              CK_NullPassedToNonnull, N, nullptr, C, ValueStmt);
+                              NullPassedToNonnullChecker, N, nullptr, C,
+                              ValueStmt);
     return;
   }
 
@@ -1340,14 +1356,15 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
     if (RhsNullness == NullConstraint::IsNotNull ||
         TrackedNullability->getValue() != Nullability::Nullable)
       return;
-    if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+    if (isPartEnabled(NullablePassedToNonnullChecker) &&
         LocNullability == Nullability::Nonnull) {
       static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
       reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
                                 "which is expected to have non-null value",
                                 ErrorKind::NullableAssignedToNonnull,
-                                CK_NullablePassedToNonnull, N, ValueRegion, C);
+                                NullablePassedToNonnullChecker, N, ValueRegion,
+                                C);
     }
     return;
   }
@@ -1394,38 +1411,26 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State,
   }
 }
 
-void ento::registerNullabilityBase(CheckerManager &mgr) {
-  mgr.registerChecker<NullabilityChecker>();
-}
-
-bool ento::shouldRegisterNullabilityBase(const CheckerManager &mgr) {
-  return true;
-}
-
-#define REGISTER_CHECKER(name, trackingRequired)                               \
-  void ento::register##name##Checker(CheckerManager &mgr) {                    \
-    NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>();        \
-    checker->ChecksEnabled[NullabilityChecker::CK_##name] = true;              \
-    checker->CheckNames[NullabilityChecker::CK_##name] =                       \
-        mgr.getCurrentCheckerName();                                           \
-    checker->NeedTracking = checker->NeedTracking || trackingRequired;         \
-    checker->NoDiagnoseCallsToSystemHeaders =                                  \
-        checker->NoDiagnoseCallsToSystemHeaders ||                             \
-        mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
-            checker, "NoDiagnoseCallsToSystemHeaders", true);                  \
+#define REGISTER_CHECKER(Part, TrackingRequired)                               \
+  void ento::register##Part(CheckerManager &Mgr) {                             \
+    auto *Checker =                                                            \
+        Mgr.registerChecker<NullabilityChecker, NullabilityChecker::Part>();   \
+    Checker->NeedTracking = Checker->NeedTracking || TrackingRequired;         \
+    Checker->NoDiagnoseCallsToSystemHeaders =                                  \
+        Checker->NoDiagnoseCallsToSystemHeaders ||                             \
+        Mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
+            Checker, "NoDiagnoseCallsToSystemHeaders", true);                  \
   }                                                                            \
                                                                                \
-  bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) {        \
-    return true;                                                               \
-  }
-
-// The checks are likely to be turned on by default and it is possible to do
-// them without tracking any nullability related information. As an optimization
-// no nullability information will be tracked when only these two checks are
-// enables.
-REGISTER_CHECKER(NullPassedToNonnull, false)
-REGISTER_CHECKER(NullReturnedFromNonnull, false)
-
-REGISTER_CHECKER(NullableDereferenced, true)
-REGISTER_CHECKER(NullablePassedToNonnull, true)
-REGISTER_CHECKER(NullableReturnedFromNonnull, true)
+  bool ento::shouldRegister##Part(const CheckerManager &) { return true; }
+
+// These checker parts are likely to be turned on by default and they don't
+// need the tracking of nullability related information. As an optimization
+// nullability information won't be tracked when the rest are disabled.
+REGISTER_CHECKER(NullabilityBase, false)
+REGISTER_CHECKER(NullPassedToNonnullChecker, false)
+REGISTER_CHECKER(NullReturnedFromNonnullChecker, false)
+
+REGISTER_CHECKER(NullableDereferencedChecker, true)
+REGISTER_CHECKER(NullablePassedToNonnullChecker, true)
+REGISTER_CHECKER(NullableReturnedFromNonnullChecker, true)

[steakhal]

Have you tried to directly pass a BugType ref instead of an index?
An integer index is less typed than a BugType, so it's easier to mess up and do unintended things with it.
Have you tried passing a BugType here?

[NagyDonat]

Good idea, I'll implement it.

I didn't think about this question and just mechanically translated the enum CheckKind (essentially another integer type) to CheckerPartIdx -- but in this particular checker it will be only used to get BugTypes[Idx] so I can switch to passing a bug type ref.

[NagyDonat]

I implemented this suggestion in 3ca23ce -- but this led to repeating BugTypes[ ... ] nine times and I feel that it might be a bit too verbose.

I would slightly prefer switching back to using CheckerPartIdx as the type of these parameters (to make these already cumbersome calls a bit less verbose), but I can also accept using BugType if you say that it's better.

[steakhal]

This feels like a technical difficulty.
You could create a bug(checker) member fn that gives you the right BugType. Or a reference member to the right BugType.

I'll have a look at your current PR tomorrow to give you concrete suggestions.

[NagyDonat]

This feels like a technical difficulty. You could create a bug(checker) member fn that gives you the right BugType.

The repeated code fragment is already very short: using bug types instead of indices means that I need to write BugTypes[LongAndConvolutedNameChecker] instead of plain LongAndConvolutedNameChecker. Introducing a method as you suggested wouldn't be a significant improvement bug(LongAndConvolutedNameChecker) is approximately the same as the array indexing.

Overall, I'm very close to indifferent in this question -- the stakes are negligible.

[NagyDonat]

Before this PR, the registration functions of the (real) checker parts actually tested the presence of the checker option nullability.NullabilityBase:NoDiagnoseCallsToSystemHeaders, where nullability.NullabilityBase was the hidden dummy checker part that was added as a prerequisite of all the "real" checker parts. (Note that -- ironically -- registerNullabilityBase() did not check for the presence of this option which was nominally attached to it.)

This was working because in the checker option handling code specifying an option on the group level (e.g. saying nullability:NoDiagnoseCallsToSystemHeaders=true) is roughly equivalent to passing it to every checker within the group.

In commit d490c74 I'm turning this option into a "real" group-level option by passing the group name to getCheckerBooleanOption.

---

By the way the name of this option is really ugly -- it should be DiagnoseCallsToSystemHeaders with negated meaning -- but that's a problem for another commit.

[steakhal]

You could use a static constexpr StringLiteral instead.

[NagyDonat]

Yes, that's probably a better choice.

[steakhal]

I wonder if you could hoist stuff like this to make these fit in 1 line each.

  static constexpr llvm::StringLiteral Desc = "Nullability";
  static constexpr llvm::StringLiteral Cat = categories::MemoryError;
  BugType BugTypes[NumCheckerParts] = {
    {this, NullPassedToNonnullChecker, Desc, Cat},
    {this, NullReturnedFromNonnullChecker, Desc, Cat},
    {this, NullableDereferencedChecker, Desc, Cat},
    {this, NullablePassedToNonnullChecker, Desc, Cat},
    {this, NullableReturnedFromNonnullChecker, Desc, Cat},
  };
  const auto &NullPassedToNonnullBug = BugTypes[NullPassedToNonnullChecker];
  const auto &NullReturnedFromNonnullBug = BugTypes[NullReturnedFromNonnullChecker];
  const auto &NullableDereferencedBug = BugTypes[NullableDereferencedChecker];
  const auto &NullablePassedToNonnullBug = BugTypes[NullablePassedToNonnullChecker];
  const auto &NullableReturnedFromNonnullBug = BugTypes[NullableReturnedFromNonnullChecker];

This way later you could just directly refer to NullPassedToNonnullBug.

---

Every time I see this CheckerPart enum, I question myself, why do we need this? And I can't convince myself that this is the right way.

Correct me if I'm wrong, but we don't really need a BugType array. We also don't really need a checker part enum either.

We could design a system that is less coupled. Something like this:

// Assuming we have a strong-type something like this: struct CheckerPart {};
static inline CheckerPart NullPassedToNonnullChecker;
static inline CheckerPart NullReturnedFromNonnullChecker;
static inline CheckerPart NullableDereferencedChecker;
const BugType NullPassedToNonnullBug{this, NullPassedToNonnullChecker, Desc, Cat},
// more bug types.

The address of the CheckerPart object identifies the "part". The "CheckerPart" could contain a mutable bool flag to know if it's enabled or not.

[steakhal]

I have something like this in mind:

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
index 3a635e0d0125..75c7786aea3e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
@@ -29,7 +29,7 @@ class BugType {
 private:
   struct CheckerPartRef {
     const CheckerBase *Checker;
-    CheckerPartIdx Idx;
+    const CheckerPart &Part;
   };
   using CheckerNameInfo = std::variant<CheckerNameRef, CheckerPartRef>;
 
@@ -55,14 +55,14 @@ public:
   // pointer to the checker and later use that to query the name.
   BugType(const CheckerBase *Checker, StringRef Desc,
           StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
-      : CheckerName(CheckerPartRef{Checker, DefaultPart}), Description(Desc),
-        Category(Cat), SuppressOnSink(SuppressOnSink) {}
+      : CheckerName(Checker->getName()), 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,
+  BugType(const CheckerBase *Checker, const CheckerPart &Part, StringRef Desc,
           StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
-      : CheckerName(CheckerPartRef{Checker, Idx}), Description(Desc),
+      : CheckerName(CheckerPartRef{Checker, Part}), Description(Desc),
         Category(Cat), SuppressOnSink(SuppressOnSink) {}
   virtual ~BugType() = default;
 
@@ -72,8 +72,8 @@ public:
     if (const auto *CNR = std::get_if<CheckerNameRef>(&CheckerName))
       return *CNR;
 
-    auto [Checker, Idx] = std::get<CheckerPartRef>(CheckerName);
-    return Checker->getName(Idx);
+    auto [Checker, Part] = std::get<CheckerPartRef>(CheckerName);
+    return Checker->getName(Part);
   }
 
   /// isSuppressOnSink - Returns true if bug reports associated with this bug
diff --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h b/clang/include/clang/StaticAnalyzer/Core/Checker.h
index a54c5bee612f..e63b3454b853 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Checker.h
+++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h
@@ -17,6 +17,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Casting.h"
 
 namespace clang {
@@ -488,31 +489,30 @@ 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;
+  llvm::SmallDenseMap<const CheckerPart *, CheckerNameRef> RegisteredNames;
+  CheckerNameRef FirstRegisteredName;
 
   friend class ::clang::ento::CheckerManager;
 
 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;
+  CheckerNameRef getName() const {
+    assert(!RegisteredNames.empty());
+    return FirstRegisteredName;
   }
 
-  bool isPartEnabled(CheckerPartIdx Idx) const {
-    return Idx < RegisteredNames.size() && RegisteredNames[Idx].has_value();
+  CheckerNameRef getName(const CheckerPart &Part) const {
+    auto It = RegisteredNames.find(&Part);
+    assert(It != RegisteredNames.end() &&
+           "Requested checker part is not registered!");
+    return It->second;
   }
 
-  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.");
+  void registerCheckerPart(const CheckerPart &Part, CheckerNameRef Name) {
+    if (RegisteredNames.empty())
+      FirstRegisteredName = Name;
 
-    if (Idx >= RegisteredNames.size())
-      RegisteredNames.resize(Idx + 1, std::nullopt);
-
-    assert(!RegisteredNames[Idx] && "Repeated registration of checker a part!");
-    RegisteredNames[Idx] = Name;
+    bool Inserted = RegisteredNames.insert({&Part, Name}).second;
+    assert(Inserted && "Repeated registration of checker a part!");
   }
 
   StringRef getTagDescription() const override {
@@ -521,11 +521,10 @@ public:
     // 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";
+    assert(!RegisteredNames.empty());
+    if (RegisteredNames.empty())
+      return "Unregistered checker";
+    return FirstRegisteredName;
   }
 
   /// Debug state dump callback, see CheckerManager::runCheckersForPrintState.
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 03ffadd346d0..b0ffef197246 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -116,18 +116,18 @@ public:
   operator StringRef() const { return Name; }
 };
 
-/// A single checker class (and its singleton instance) can act as the
-/// implementation of several (user-facing or modeling) checker parts that
-/// have shared state and logic, but have their own names and can be enabled or
-/// disabled separately.
-/// Each checker class that implement multiple parts introduces its own enum
-/// type to assign small numerical indices (0, 1, 2 ...) to their parts. The
-/// type alias 'CheckerPartIdx' is conceptually the union of these enum types.
-using CheckerPartIdx = unsigned;
-
-/// If a checker doesn't have multiple parts, then its single part is
-/// represented by this index.
-constexpr inline CheckerPartIdx DefaultPart = 0;
+// / A single checker class (and its singleton instance) can act as the
+// / implementation of several (user-facing or modeling) checker parts that
+// / have shared state and logic, but have their own names and can be enabled or
+// / disabled separately.
+// / Each checker class that implement multiple parts introduces its own enum
+// / type to assign small numerical indices (0, 1, 2 ...) to their parts. The
+// / type alias 'CheckerPartIdx' is conceptually the union of these enum types.
+struct CheckerPart {
+  const CheckerBase &Checker;
+  bool IsEnabled = false;
+  explicit CheckerPart(const CheckerBase *Checker) : Checker(*Checker) {}
+};
 
 enum class ObjCMessageVisitKind {
   Pre,
@@ -196,11 +196,12 @@ public:
   void reportInvalidCheckerOptionValue(const CheckerBase *C,
                                        StringRef OptionName,
                                        StringRef ExpectedValueDesc) const {
-    reportInvalidCheckerOptionValue(C, DefaultPart, OptionName,
+    reportInvalidCheckerOptionValue(C, /*Part=*/nullptr, OptionName,
                                     ExpectedValueDesc);
   }
 
-  void reportInvalidCheckerOptionValue(const CheckerBase *C, CheckerPartIdx Idx,
+  void reportInvalidCheckerOptionValue(const CheckerBase *C,
+                                       const CheckerPart *Part,
                                        StringRef OptionName,
                                        StringRef ExpectedValueDesc) const;
 
@@ -221,12 +222,13 @@ public:
   /// existing checker object (while registering their names).
   ///
   /// \returns a pointer to the checker object.
-  template <typename CHECKER, CheckerPartIdx Idx = DefaultPart, typename... AT>
+  template <typename CHECKER, CheckerPart(CHECKER::*PartSelector) = nullptr,
+            typename... AT>
   CHECKER *registerChecker(AT &&...Args) {
     // This assert could be removed but then we need to make sure that calls
     // registering different parts of the same checker pass the same arguments.
     static_assert(
-        Idx == DefaultPart || !sizeof...(AT),
+        PartSelector == nullptr || !sizeof...(AT),
         "Argument forwarding isn't supported with multi-part checkers!");
 
     CheckerTag Tag = getTag<CHECKER>();
@@ -240,7 +242,10 @@ public:
     }
 
     CHECKER *Result = static_cast<CHECKER *>(Ref.get());
-    Result->registerCheckerPart(Idx, CurrentCheckerName);
+    CheckerPart &Part = Result->*PartSelector;
+    Result->registerCheckerPart(Part, CurrentCheckerName);
+    assert(!Part.IsEnabled);
+    Part.IsEnabled = true;
     return Result;
   }
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
index 3dd57732305b..a765f30a71a1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -34,14 +34,11 @@ class DivZeroChecker : public Checker<check::PreStmt<BinaryOperator>> {
 
 public:
   /// This checker class implements several user facing checkers
-  enum : CheckerPartIdx {
-    DivideZeroChecker,
-    TaintedDivChecker,
-    NumCheckerParts
-  };
-  BugType BugTypes[NumCheckerParts] = {
-      {this, DivideZeroChecker, "Division by zero"},
-      {this, TaintedDivChecker, "Division by zero", categories::TaintedData}};
+  CheckerPart DivideZeroChecker{this};
+  CheckerPart TaintedDivChecker{this};
+  const BugType DivisionByZeroBug{this, DivideZeroChecker, "Division by zero"};
+  const BugType DivisionByTaintedBug{
+      this, TaintedDivChecker, "Division by zero", categories::TaintedData};
 
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
@@ -56,11 +53,11 @@ static const Expr *getDenomExpr(const ExplodedNode *N) {
 
 void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
                                CheckerContext &C) const {
-  if (!isPartEnabled(DivideZeroChecker))
+  if (!DivideZeroChecker.IsEnabled)
     return;
   if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
-    auto R = std::make_unique<PathSensitiveBugReport>(
-        BugTypes[DivideZeroChecker], Msg, N);
+    auto R =
+        std::make_unique<PathSensitiveBugReport>(DivisionByZeroBug, Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     C.emitReport(std::move(R));
   }
@@ -69,11 +66,11 @@ void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
 void DivZeroChecker::reportTaintBug(
     StringRef Msg, ProgramStateRef StateZero, CheckerContext &C,
     llvm::ArrayRef<SymbolRef> TaintedSyms) const {
-  if (!isPartEnabled(TaintedDivChecker))
+  if (!TaintedDivChecker.IsEnabled)
     return;
   if (ExplodedNode *N = C.generateNonFatalErrorNode(StateZero)) {
-    auto R = std::make_unique<PathSensitiveBugReport>(
-        BugTypes[TaintedDivChecker], Msg, N);
+    auto R =
+        std::make_unique<PathSensitiveBugReport>(DivisionByTaintedBug, Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     for (auto Sym : TaintedSyms)
       R->markInteresting(Sym);
@@ -127,13 +124,13 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
 }
 
 void ento::registerDivZeroChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<DivZeroChecker, DivZeroChecker::DivideZeroChecker>();
+  Mgr.registerChecker<DivZeroChecker, &DivZeroChecker::DivideZeroChecker>();
 }
 
 bool ento::shouldRegisterDivZeroChecker(const CheckerManager &) { return true; }
 
 void ento::registerTaintedDivChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<DivZeroChecker, DivZeroChecker::TaintedDivChecker>();
+  Mgr.registerChecker<DivZeroChecker, &DivZeroChecker::TaintedDivChecker>();
 }
 
 bool ento::shouldRegisterTaintedDivChecker(const CheckerManager &) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
index 5b0d303ee5bb..5a58e4cc6553 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -42,13 +42,14 @@ namespace {
 class VirtualCallChecker
     : public Checker<check::BeginFunction, check::EndFunction, check::PreCall> {
 public:
-  enum : CheckerPartIdx { PureChecker, ImpureChecker, NumCheckerParts };
-
-  BugType BugTypes[NumCheckerParts] = {
-      {this, PureChecker, "Pure virtual method call",
-       categories::CXXObjectLifecycle},
-      {this, ImpureChecker, "Unexpected loss of virtual dispatch",
-       categories::CXXObjectLifecycle}};
+  CheckerPart PureChecker{this};
+  CheckerPart ImpureChecker{this};
+  const BugType CallingPureVirtualMethodBug{this, PureChecker,
+                                            "Pure virtual method call",
+                                            categories::CXXObjectLifecycle};
+  const BugType UnexpectedVirtualDispatchBug{
+      this, ImpureChecker, "Unexpected loss of virtual dispatch",
+      categories::CXXObjectLifecycle};
 
   bool ShowFixIts = false;
 
@@ -147,15 +148,14 @@ void VirtualCallChecker::checkPreCall(const CallEvent &Call,
   if (!N)
     return;
 
-  const CheckerPartIdx Part = IsPure ? PureChecker : ImpureChecker;
-
-  if (!isPartEnabled(Part)) {
+  const CheckerPart &Part = IsPure ? PureChecker : ImpureChecker;
+  if (!Part.IsEnabled) {
     // The respective check is disabled.
     return;
   }
-
-  auto Report =
-      std::make_unique<PathSensitiveBugReport>(BugTypes[Part], OS.str(), N);
+  const BugType &Bug =
+      IsPure ? CallingPureVirtualMethodBug : UnexpectedVirtualDispatchBug;
+  auto Report = std::make_unique<PathSensitiveBugReport>(Bug, OS.str(), N);
 
   if (ShowFixIts && !IsPure) {
     // FIXME: These hints are valid only when the virtual call is made
@@ -210,7 +210,7 @@ void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction,
 }
 
 void ento::registerPureVirtualCallChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<VirtualCallChecker, VirtualCallChecker::PureChecker>();
+  Mgr.registerChecker<VirtualCallChecker, &VirtualCallChecker::PureChecker>();
 }
 
 bool ento::shouldRegisterPureVirtualCallChecker(const CheckerManager &Mgr) {
@@ -219,7 +219,7 @@ bool ento::shouldRegisterPureVirtualCallChecker(const CheckerManager &Mgr) {
 
 void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.registerChecker<VirtualCallChecker,
-                                  VirtualCallChecker::ImpureChecker>();
+                                  &VirtualCallChecker::ImpureChecker>();
   Chk->ShowFixIts = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
       Mgr.getCurrentCheckerName(), "ShowFixIts");
 }
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 7ae86f133904..c8281df38427 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -50,11 +50,11 @@ bool CheckerManager::hasPathSensitiveCheckers() const {
 }
 
 void CheckerManager::reportInvalidCheckerOptionValue(
-    const CheckerBase *C, CheckerPartIdx Idx, StringRef OptionName,
+    const CheckerBase *C, const CheckerPart *Part, StringRef OptionName,
     StringRef ExpectedValueDesc) const {
 
   getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
-      << (llvm::Twine(C->getName(Idx)) + ":" + OptionName).str()
+      << (llvm::Twine(C->getName(*Part)) + ":" + OptionName).str()
       << ExpectedValueDesc;
 }

[NagyDonat]

Correct me if I'm wrong, but we don't really need a BugType array.

Yep, you're completely right, and I should have noticed this -- e.g. DivZeroChecker also declares the bug types as individual variables (and not arrays).

We also don't really need a checker part enum either. [...]

When I designed the current framework I implicitly assumed that we will need a checker part identifier type which acts as an index/key for a (perhaps associative) array containing the names / registration status, because this was a common element in all the individual checkers plus the old abandoned SubChecker draft commit that was written by Kristóf Umann several years ago. (I spent lots of time on thinking about details within this box, but I didn't consider out-of-the box ideas.) Now that I think about your suggestion, I see that it could lead to a simpler alternative framework.

The operations that a checker part type needs to support:

• The constructor of BugType is called from the constructor of a checker and needs to take a checker part argument (so the checker part should be available at this point).
• BugType stores this checker part as a data member (so all checkers should use the same CheckerPart type).
• CheckerManager::registerChecker takes a checker part as an argument (in fact, currently as a template argument, but this may change) and binds the name injected from Checkers.td to it. (Binding the name implicitly marks the part as enabled, because enabled parts need to have names, while others don't have names.)
• The enabled/disabled status of a checker part may be queried within various checker callbacks.
• The name of the checker part is queried by the bug types associated with it and may be queried elsewhere within the checker callbacks.
• getTagDescription() currently iterates over the checker parts to find one that has a name. This is the only case where iteration over checker parts happen, so perhaps it should be replaced by an alternative implementation.

These could be satisfied by a checker part type that looks like

class CheckerPart {
  std::optional<CheckerNameRef> Name;
public:
  void enable(CheckerManager &Mgr) {
    assert(!Name && "Checker part registered twice!");
    Name = Mgr.getCurrentCheckerName();
  }    
  bool isEnabled() { return static_cast<bool>(Name); }
  CheckerNameRef getName() { return *Name; }
}

class FooOrBarChecker {
public:
  CheckerPart Foo, Bar;
private:
  BugType FooBug{Foo, "Foo does not work"};
  // ...
}

void registerFooChecker(CheckerManager &Mgr) {
  Mgr.registerChecker<FooOrBarChecker>()->Foo.enable(Mgr);
}

I think this could be a significant simplification -- I'll flesh it out in a separate PR.

[NagyDonat]

I have something like this in mind: [...]

I didn't see your detailed implementation idea before writing my own. I think storing the CheckerNameRef as a data member of the CheckerPart is a significant advantage because this way we won't need a CheckerPart $\to$ CheckerNameRef map as a data member of the CheckerBase, but I will probably borrow some ideas from your approach.

[steakhal]

Feel free to flash out an implementation. I'm happy that it inspired you.
I'd suggest blocking this PR until you played with the new ideas first.
There shouldn't be a rush for migrating to the current framework I think.

[NagyDonat]

I'm happy that it inspired you.

Thanks for the ideas!

I'd suggest blocking this PR until you played with the new ideas first. There shouldn't be a rush for migrating to the current framework I think.

Completely agree.

[NagyDonat]

I converted this PR to draft because I'll examine the ideas described in the inline comment #132250 (comment) . I'll probably tweak the framework based on those ideas, then revisit refactoring this checker.