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

NagyDonat · NagyDonat · commit d4878a62a693 · 2025-03-20T17:27:45.000+01:00
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).
diff --git 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)
