Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4878a6
[NFC][analyzer] Multipart checker refactor 2: NullabilityChecker
NagyDonat Mar 20, 2025
2708bed
Remove hidden dummy checker part NullabilityBase
NagyDonat Mar 20, 2025
3ca23ce
Use `BugType` parameters for `reportBug{,IfInvariantHolds}`
NagyDonat Mar 20, 2025
d490c74
Special case NoDiagnoseCallsToSystemHeaders as group-level option
NagyDonat Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 27 additions & 29 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,39 +326,37 @@ def StdVariantChecker : Checker<"StdVariant">,

let ParentPackage = Nullability in {

def NullabilityBase : Checker<"NullabilityBase">,
HelpText<"Stores information during the analysis about nullability.">,
Documentation<NotDocumented>,
Hidden;

def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
HelpText<"Warns when a null pointer is passed to a pointer which has a "
"_Nonnull type.">,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullPassedToNonnullChecker
: Checker<"NullPassedToNonnull">,
HelpText<"Warns when a null pointer is passed to a pointer which has a "
"_Nonnull type.">,
Documentation<HasDocumentation>;

def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">,
HelpText<"Warns when a null pointer is returned from a function that has "
"_Nonnull return type.">,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullReturnedFromNonnullChecker
: Checker<"NullReturnedFromNonnull">,
HelpText<
"Warns when a null pointer is returned from a function that has "
"_Nonnull return type.">,
Documentation<HasDocumentation>;

def NullableDereferencedChecker : Checker<"NullableDereferenced">,
HelpText<"Warns when a nullable pointer is dereferenced.">,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullableDereferencedChecker
: Checker<"NullableDereferenced">,
HelpText<"Warns when a nullable pointer is dereferenced.">,
Documentation<HasDocumentation>;

def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">,
HelpText<"Warns when a nullable pointer is passed to a pointer which has a "
"_Nonnull type.">,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullablePassedToNonnullChecker
: Checker<"NullablePassedToNonnull">,
HelpText<
"Warns when a nullable pointer is passed to a pointer which has a "
"_Nonnull type.">,
Documentation<HasDocumentation>;

def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">,
HelpText<"Warns when a nullable pointer is returned from a function that has "
"_Nonnull return type.">,
Dependencies<[NullabilityBase]>,
Documentation<NotDocumented>;
def NullableReturnedFromNonnullChecker
: Checker<"NullableReturnedFromNonnull">,
HelpText<"Warns when a nullable pointer is returned from a function "
"that has "
"_Nonnull return type.">,
Documentation<NotDocumented>;

} // end "nullability"

Expand Down
156 changes: 82 additions & 74 deletions clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,25 +112,30 @@ 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.
enum : CheckerPartIdx {
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.
BugType BugTypes[NumCheckerParts] = {
{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}};
Comment on lines +126 to +138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
 }

Copy link
Contributor Author

@NagyDonat NagyDonat Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@NagyDonat NagyDonat Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// When set to false no nullability information will be tracked in
// NullabilityMap. It is possible to catch errors like passing a null pointer
Expand Down Expand Up @@ -163,17 +168,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,
const BugType &BT, 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, const BugType &BT,
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>(BT, Msg, N);
if (Region) {
R->markInteresting(Region);
R->addVisitor<NullabilityBugVisitor>(Region);
Expand Down Expand Up @@ -479,7 +483,7 @@ static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N,
}

void NullabilityChecker::reportBugIfInvariantHolds(
StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
StringRef Msg, ErrorKind Error, const BugType &BT, ExplodedNode *N,
const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr,
bool SuppressPath) const {
ProgramStateRef OriginalState = N->getState();
Expand All @@ -491,7 +495,7 @@ void NullabilityChecker::reportBugIfInvariantHolds(
N = C.addTransition(OriginalState, N);
}

reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
reportBug(Msg, Error, BT, N, Region, C.getBugReporter(), ValueExpr);
}

/// Cleaning up the program state.
Expand Down Expand Up @@ -545,20 +549,21 @@ 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,
Event.SinkNode, Region, BR);
reportBug(
"Nullable pointer is dereferenced", ErrorKind::NullableDereferenced,
BugTypes[NullableDereferencedChecker], Event.SinkNode, Region, BR);
else {
reportBug("Nullable pointer is passed to a callee that requires a "
"non-null",
ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
Event.SinkNode, Region, BR);
ErrorKind::NullablePassedToNonnull,
BugTypes[NullableDereferencedChecker], Event.SinkNode, Region,
BR);
}
}
}
Expand Down Expand Up @@ -711,7 +716,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");
Expand All @@ -725,8 +731,8 @@ 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,
RetExpr);
BugTypes[NullReturnedFromNonnullChecker], N,
nullptr, C, RetExpr);
return;
}

Expand All @@ -746,7 +752,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) {
Expand All @@ -759,7 +765,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);
BugTypes[NullableReturnedFromNonnullChecker], N,
Region, C);
}
return;
}
Expand Down Expand Up @@ -810,7 +817,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 &&
Expand All @@ -825,8 +832,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,
/*SuppressPath=*/false);
BugTypes[NullPassedToNonnullChecker], N,
nullptr, C, ArgExpr, /*SuppressPath=*/false);
return;
}

Expand All @@ -842,7 +849,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);
Expand All @@ -851,17 +858,17 @@ 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,
ArgExpr, /*SuppressPath=*/true);
BugTypes[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,
ArgExpr, /*SuppressPath=*/true);
BugTypes[NullableDereferencedChecker], N,
Region, C, ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
Expand Down Expand Up @@ -1295,7 +1302,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)) {
Expand All @@ -1314,7 +1321,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);
BugTypes[NullPassedToNonnullChecker], N, nullptr,
C, ValueStmt);
return;
}

Expand All @@ -1340,14 +1348,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);
BugTypes[NullablePassedToNonnullChecker], N,
ValueRegion, C);
}
return;
}
Expand Down Expand Up @@ -1394,35 +1403,34 @@ 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); \
// The checker group "nullability" consists of the checkers that are
// implemented as the parts of the checker class `NullabilityChecker`. These
// checkers share a checker option "nullability:NoDiagnoseCallsToSystemHeaders"
// which semantically belongs to the whole group and not just one checker from
// it. As this is a unique situation (I don't know about any other similar
// group-level option) there is no mechanism to inject this group name from
// e.g. Checkers.td, so I'm just hardcoding it here. (These are old stable
// checkers, I don't think that their name will change.)
Comment on lines +1406 to +1413
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define CHECKER_GROUP_NAME "nullability"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a static constexpr StringLiteral instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's probably a better choice.


#define REGISTER_CHECKER(Part, TrackingRequired) \
void ento::register##Part##Checker(CheckerManager &Mgr) { \
auto *Checker = Mgr.registerChecker<NullabilityChecker, \
NullabilityChecker::Part##Checker>(); \
Checker->NeedTracking = Checker->NeedTracking || TrackingRequired; \
Checker->NoDiagnoseCallsToSystemHeaders = \
Checker->NoDiagnoseCallsToSystemHeaders || \
Mgr.getAnalyzerOptions().getCheckerBooleanOption( \
CHECKER_GROUP_NAME, "NoDiagnoseCallsToSystemHeaders", true); \
} \
\
bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
bool ento::shouldRegister##Part##Checker(const CheckerManager &) { \
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.
// 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(NullPassedToNonnull, false)
REGISTER_CHECKER(NullReturnedFromNonnull, false)

Expand Down
1 change: 0 additions & 1 deletion clang/test/Analysis/analyzer-enabled-checkers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
// CHECK-NEXT: core.uninitialized.CapturedBlockVariable
// CHECK-NEXT: core.uninitialized.UndefReturn
// CHECK-NEXT: deadcode.DeadStores
// CHECK-NEXT: nullability.NullabilityBase
// CHECK-NEXT: nullability.NullPassedToNonnull
// CHECK-NEXT: nullability.NullReturnedFromNonnull
// CHECK-NEXT: security.insecureAPI.SecuritySyntaxChecker
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/bugfix-124477.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling,nullability.NullableDereferenced,nullability.NullabilityBase -x objective-c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling,nullability.NullableDereferenced -x objective-c %s
/*
This test is reduced from a static analyzer crash. The bug causing
the crash is explained in #124477. It can only be triggered in some
Expand Down
1 change: 0 additions & 1 deletion clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
// CHECK-NEXT: core.uninitialized.CapturedBlockVariable
// CHECK-NEXT: core.uninitialized.UndefReturn
// CHECK-NEXT: deadcode.DeadStores
// CHECK-NEXT: nullability.NullabilityBase
// CHECK-NEXT: nullability.NullPassedToNonnull
// CHECK-NEXT: nullability.NullReturnedFromNonnull
// CHECK-NEXT: security.insecureAPI.SecuritySyntaxChecker
Expand Down
Loading