[analyzer] Conversion to CheckerFamily: NullabilityChecker #143735
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076 This commit removes the dummy checker `nullability.NullabilityBase` because it was hidden from the users and didn't have any useful role except for helping the registration of the checker parts in the old ad-hoc system (which is replaced by the new standardized framework). Except for the removal of this dummy checker, no functional changes intended.
NagyDonat
requested review from
Xazax-hun,
haoNoQ,
steakhal,
ziqingluo-90 and
Szelethus
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThis commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076 This commit removes the dummy checker Except for the removal of this dummy checker, no functional changes intended. Patch is 21.47 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143735.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 2a96df80d1001..d6a1b9bbc1fdd 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -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"
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 461d01b452fd0..9744d1abf7790 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -81,11 +81,12 @@ enum class ErrorKind : int {
};
class NullabilityChecker
- : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
- check::PostCall, check::PostStmt<ExplicitCastExpr>,
- check::PostObjCMessage, check::DeadSymbols, eval::Assume,
- check::Location, check::Event<ImplicitNullDerefEvent>,
- check::BeginFunction> {
+ : public CheckerFamily<
+ check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
+ check::PostCall, check::PostStmt<ExplicitCastExpr>,
+ check::PostObjCMessage, check::DeadSymbols, eval::Assume,
+ check::Location, check::Event<ImplicitNullDerefEvent>,
+ check::BeginFunction> {
public:
// If true, the checker will not diagnose nullabilility issues for calls
@@ -113,25 +114,21 @@ 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
- };
-
- 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];
- }
+ StringRef getDebugTag() const override { return "NullabilityChecker"; }
+
+ // FIXME: All bug types share the same Description ("Nullability") since the
+ // creation of this checker. We should write more descriptive descriptions...
+ // or just eliminate the Description field if it is meaningless?
+ CheckerFrontendWithBugType NullPassedToNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullReturnedFromNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullableDereferenced{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullablePassedToNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullableReturnedFromNonnull{
+ "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
@@ -164,17 +161,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);
@@ -480,7 +476,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();
@@ -492,7 +488,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.
@@ -546,19 +542,19 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
if (!TrackedNullability)
return;
- if (ChecksEnabled[CK_NullableDereferenced] &&
+ if (NullableDereferenced.isEnabled() &&
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, NullableDereferenced,
Event.SinkNode, Region, BR);
else {
reportBug("Nullable pointer is passed to a callee that requires a "
"non-null",
- ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
+ ErrorKind::NullablePassedToNonnull, NullableDereferenced,
Event.SinkNode, Region, BR);
}
}
@@ -710,29 +706,28 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
- bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
- Nullness == NullConstraint::IsNull);
- if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
- RetExprTypeLevelNullability != Nullability::Nonnull &&
- !InSuppressedMethodFamily) {
- ExplodedNode *N = C.generateErrorNode(State);
- if (!N)
- return;
+ if (RequiredNullability == Nullability::Nonnull &&
+ Nullness == NullConstraint::IsNull) {
+ if (NullReturnedFromNonnull.isEnabled() &&
+ RetExprTypeLevelNullability != Nullability::Nonnull &&
+ !InSuppressedMethodFamily) {
+ ExplodedNode *N = C.generateErrorNode(State);
+ if (!N)
+ return;
- SmallString<256> SBuf;
- llvm::raw_svector_ostream OS(SBuf);
- OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
- 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);
- return;
- }
+ SmallString<256> SBuf;
+ llvm::raw_svector_ostream OS(SBuf);
+ OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
+ OS << " returned from a " << C.getDeclDescription(D)
+ << " that is expected to return a non-null value";
+ reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
+ NullReturnedFromNonnull, N, nullptr, C,
+ RetExpr);
+ return;
+ }
- // If null was returned from a non-null function, mark the nullability
- // invariant as violated even if the diagnostic was suppressed.
- if (NullReturnedFromNonNull) {
+ // If null was returned from a non-null function, mark the nullability
+ // invariant as violated even if the diagnostic was suppressed.
State = State->set<InvariantViolated>(true);
C.addTransition(State);
return;
@@ -746,7 +741,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
State->get<NullabilityMap>(Region);
if (TrackedNullability) {
Nullability TrackedNullabValue = TrackedNullability->getValue();
- if (ChecksEnabled[CK_NullableReturnedFromNonnull] &&
+ if (NullableReturnedFromNonnull.isEnabled() &&
Nullness != NullConstraint::IsNotNull &&
TrackedNullabValue == Nullability::Nullable &&
RequiredNullability == Nullability::Nonnull) {
@@ -758,7 +753,7 @@ 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);
+ NullableReturnedFromNonnull, N, Region, C);
}
return;
}
@@ -809,8 +804,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
- if (ChecksEnabled[CK_NullPassedToNonnull] &&
- Nullness == NullConstraint::IsNull &&
+ if (NullPassedToNonnull.isEnabled() && Nullness == NullConstraint::IsNull &&
ArgExprTypeLevelNullability != Nullability::Nonnull &&
RequiredNullability == Nullability::Nonnull &&
isDiagnosableCall(Call)) {
@@ -824,7 +818,7 @@ 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,
+ NullPassedToNonnull, N, nullptr, C, ArgExpr,
/*SuppressPath=*/false);
return;
}
@@ -841,7 +835,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
TrackedNullability->getValue() != Nullability::Nullable)
continue;
- if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+ if (NullablePassedToNonnull.isEnabled() &&
RequiredNullability == Nullability::Nonnull &&
isDiagnosableCall(Call)) {
ExplodedNode *N = C.addTransition(State);
@@ -850,17 +844,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,
+ NullablePassedToNonnull, N, Region, C,
ArgExpr, /*SuppressPath=*/true);
return;
}
- if (ChecksEnabled[CK_NullableDereferenced] &&
+ if (NullableDereferenced.isEnabled() &&
Param->getType()->isReferenceType()) {
ExplodedNode *N = C.addTransition(State);
- reportBugIfInvariantHolds("Nullable pointer is dereferenced",
- ErrorKind::NullableDereferenced,
- CK_NullableDereferenced, N, Region, C,
- ArgExpr, /*SuppressPath=*/true);
+ reportBugIfInvariantHolds(
+ "Nullable pointer is dereferenced", ErrorKind::NullableDereferenced,
+ NullableDereferenced, N, Region, C, ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
@@ -1294,7 +1287,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 (NullPassedToNonnull.isEnabled() && NullAssignedToNonNull &&
ValNullability != Nullability::Nonnull &&
ValueExprTypeLevelNullability != Nullability::Nonnull &&
!isARCNilInitializedLocal(C, S)) {
@@ -1312,7 +1305,7 @@ 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);
+ NullPassedToNonnull, N, nullptr, C, ValueStmt);
return;
}
@@ -1338,13 +1331,13 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
if (RhsNullness == NullConstraint::IsNotNull ||
TrackedNullability->getValue() != Nullability::Nullable)
return;
- if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+ if (NullablePassedToNonnull.isEnabled() &&
LocNullability == Nullability::Nonnull) {
ExplodedNode *N = C.addTransition(State, C.getPredecessor());
reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
"which is expected to have non-null value",
ErrorKind::NullableAssignedToNonnull,
- CK_NullablePassedToNonnull, N, ValueRegion, C);
+ NullablePassedToNonnull, N, ValueRegion, C);
}
return;
}
@@ -1391,28 +1384,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); \
+// The checker group "nullability" (which consists of the checkers that are
+// implemented in this file) has a group-level configuration option which
+// affects all the checkers in the group. As this is a completely unique
+// remnant of old design (this is the only group option in the analyzer), there
+// is no machinery to inject the group name from `Checkers.td`, so it is simply
+// hardcoded here:
+constexpr llvm::StringLiteral GroupName = "nullability";
+constexpr llvm::StringLiteral GroupOptName = "NoDiagnoseCallsToSystemHeaders";
+
+#define REGISTER_CHECKER(NAME, TRACKING_REQUIRED) \
+ void ento::register##NAME##Checker(CheckerManager &Mgr) { \
+ NullabilityChecker *Chk = Mgr.getChecker<NullabilityChecker>(); \
+ Chk->NAME.enable(Mgr); \
+ Chk->NeedTracking = Chk->NeedTracking || TRACKING_REQUIRED; \
+ Chk->NoDiagnoseCallsToSystemHeaders = \
+ Mgr.getAnalyzerOptions().getCheckerBooleanOption(GroupName, \
+ GroupOptName, true); \
} \
\
- bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
+ bool ento::shouldRegister##NAME##Checker(const CheckerManager &) { \
return true; \
}
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 66b9be9795f12..78ee00deea18d 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -34,7 +34,6 @@
// CHECK-NEXT: core.uninitialized.CapturedBlockVariable
// CHECK-NEXT: core.uninitialized.UndefReturn
// CHECK-NEXT: deadcode.DeadStores
-// CHECK-NEXT: nullability....
[truncated]
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThis commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076 This commit removes the dummy checker Except for the removal of this dummy checker, no functional changes intended. Patch is 21.47 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143735.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 2a96df80d1001..d6a1b9bbc1fdd 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -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"
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 461d01b452fd0..9744d1abf7790 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -81,11 +81,12 @@ enum class ErrorKind : int {
};
class NullabilityChecker
- : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
- check::PostCall, check::PostStmt<ExplicitCastExpr>,
- check::PostObjCMessage, check::DeadSymbols, eval::Assume,
- check::Location, check::Event<ImplicitNullDerefEvent>,
- check::BeginFunction> {
+ : public CheckerFamily<
+ check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
+ check::PostCall, check::PostStmt<ExplicitCastExpr>,
+ check::PostObjCMessage, check::DeadSymbols, eval::Assume,
+ check::Location, check::Event<ImplicitNullDerefEvent>,
+ check::BeginFunction> {
public:
// If true, the checker will not diagnose nullabilility issues for calls
@@ -113,25 +114,21 @@ 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
- };
-
- 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];
- }
+ StringRef getDebugTag() const override { return "NullabilityChecker"; }
+
+ // FIXME: All bug types share the same Description ("Nullability") since the
+ // creation of this checker. We should write more descriptive descriptions...
+ // or just eliminate the Description field if it is meaningless?
+ CheckerFrontendWithBugType NullPassedToNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullReturnedFromNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullableDereferenced{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullablePassedToNonnull{"Nullability",
+ categories::MemoryError};
+ CheckerFrontendWithBugType NullableReturnedFromNonnull{
+ "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
@@ -164,17 +161,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);
@@ -480,7 +476,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();
@@ -492,7 +488,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.
@@ -546,19 +542,19 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
if (!TrackedNullability)
return;
- if (ChecksEnabled[CK_NullableDereferenced] &&
+ if (NullableDereferenced.isEnabled() &&
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, NullableDereferenced,
Event.SinkNode, Region, BR);
else {
reportBug("Nullable pointer is passed to a callee that requires a "
"non-null",
- ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
+ ErrorKind::NullablePassedToNonnull, NullableDereferenced,
Event.SinkNode, Region, BR);
}
}
@@ -710,29 +706,28 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
- bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
- Nullness == NullConstraint::IsNull);
- if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
- RetExprTypeLevelNullability != Nullability::Nonnull &&
- !InSuppressedMethodFamily) {
- ExplodedNode *N = C.generateErrorNode(State);
- if (!N)
- return;
+ if (RequiredNullability == Nullability::Nonnull &&
+ Nullness == NullConstraint::IsNull) {
+ if (NullReturnedFromNonnull.isEnabled() &&
+ RetExprTypeLevelNullability != Nullability::Nonnull &&
+ !InSuppressedMethodFamily) {
+ ExplodedNode *N = C.generateErrorNode(State);
+ if (!N)
+ return;
- SmallString<256> SBuf;
- llvm::raw_svector_ostream OS(SBuf);
- OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
- 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);
- return;
- }
+ SmallString<256> SBuf;
+ llvm::raw_svector_ostream OS(SBuf);
+ OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
+ OS << " returned from a " << C.getDeclDescription(D)
+ << " that is expected to return a non-null value";
+ reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
+ NullReturnedFromNonnull, N, nullptr, C,
+ RetExpr);
+ return;
+ }
- // If null was returned from a non-null function, mark the nullability
- // invariant as violated even if the diagnostic was suppressed.
- if (NullReturnedFromNonNull) {
+ // If null was returned from a non-null function, mark the nullability
+ // invariant as violated even if the diagnostic was suppressed.
State = State->set<InvariantViolated>(true);
C.addTransition(State);
return;
@@ -746,7 +741,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
State->get<NullabilityMap>(Region);
if (TrackedNullability) {
Nullability TrackedNullabValue = TrackedNullability->getValue();
- if (ChecksEnabled[CK_NullableReturnedFromNonnull] &&
+ if (NullableReturnedFromNonnull.isEnabled() &&
Nullness != NullConstraint::IsNotNull &&
TrackedNullabValue == Nullability::Nullable &&
RequiredNullability == Nullability::Nonnull) {
@@ -758,7 +753,7 @@ 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);
+ NullableReturnedFromNonnull, N, Region, C);
}
return;
}
@@ -809,8 +804,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
- if (ChecksEnabled[CK_NullPassedToNonnull] &&
- Nullness == NullConstraint::IsNull &&
+ if (NullPassedToNonnull.isEnabled() && Nullness == NullConstraint::IsNull &&
ArgExprTypeLevelNullability != Nullability::Nonnull &&
RequiredNullability == Nullability::Nonnull &&
isDiagnosableCall(Call)) {
@@ -824,7 +818,7 @@ 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,
+ NullPassedToNonnull, N, nullptr, C, ArgExpr,
/*SuppressPath=*/false);
return;
}
@@ -841,7 +835,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
TrackedNullability->getValue() != Nullability::Nullable)
continue;
- if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+ if (NullablePassedToNonnull.isEnabled() &&
RequiredNullability == Nullability::Nonnull &&
isDiagnosableCall(Call)) {
ExplodedNode *N = C.addTransition(State);
@@ -850,17 +844,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,
+ NullablePassedToNonnull, N, Region, C,
ArgExpr, /*SuppressPath=*/true);
return;
}
- if (ChecksEnabled[CK_NullableDereferenced] &&
+ if (NullableDereferenced.isEnabled() &&
Param->getType()->isReferenceType()) {
ExplodedNode *N = C.addTransition(State);
- reportBugIfInvariantHolds("Nullable pointer is dereferenced",
- ErrorKind::NullableDereferenced,
- CK_NullableDereferenced, N, Region, C,
- ArgExpr, /*SuppressPath=*/true);
+ reportBugIfInvariantHolds(
+ "Nullable pointer is dereferenced", ErrorKind::NullableDereferenced,
+ NullableDereferenced, N, Region, C, ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
@@ -1294,7 +1287,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 (NullPassedToNonnull.isEnabled() && NullAssignedToNonNull &&
ValNullability != Nullability::Nonnull &&
ValueExprTypeLevelNullability != Nullability::Nonnull &&
!isARCNilInitializedLocal(C, S)) {
@@ -1312,7 +1305,7 @@ 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);
+ NullPassedToNonnull, N, nullptr, C, ValueStmt);
return;
}
@@ -1338,13 +1331,13 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
if (RhsNullness == NullConstraint::IsNotNull ||
TrackedNullability->getValue() != Nullability::Nullable)
return;
- if (ChecksEnabled[CK_NullablePassedToNonnull] &&
+ if (NullablePassedToNonnull.isEnabled() &&
LocNullability == Nullability::Nonnull) {
ExplodedNode *N = C.addTransition(State, C.getPredecessor());
reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
"which is expected to have non-null value",
ErrorKind::NullableAssignedToNonnull,
- CK_NullablePassedToNonnull, N, ValueRegion, C);
+ NullablePassedToNonnull, N, ValueRegion, C);
}
return;
}
@@ -1391,28 +1384,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); \
+// The checker group "nullability" (which consists of the checkers that are
+// implemented in this file) has a group-level configuration option which
+// affects all the checkers in the group. As this is a completely unique
+// remnant of old design (this is the only group option in the analyzer), there
+// is no machinery to inject the group name from `Checkers.td`, so it is simply
+// hardcoded here:
+constexpr llvm::StringLiteral GroupName = "nullability";
+constexpr llvm::StringLiteral GroupOptName = "NoDiagnoseCallsToSystemHeaders";
+
+#define REGISTER_CHECKER(NAME, TRACKING_REQUIRED) \
+ void ento::register##NAME##Checker(CheckerManager &Mgr) { \
+ NullabilityChecker *Chk = Mgr.getChecker<NullabilityChecker>(); \
+ Chk->NAME.enable(Mgr); \
+ Chk->NeedTracking = Chk->NeedTracking || TRACKING_REQUIRED; \
+ Chk->NoDiagnoseCallsToSystemHeaders = \
+ Mgr.getAnalyzerOptions().getCheckerBooleanOption(GroupName, \
+ GroupOptName, true); \
} \
\
- bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
+ bool ento::shouldRegister##NAME##Checker(const CheckerManager &) { \
return true; \
}
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 66b9be9795f12..78ee00deea18d 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -34,7 +34,6 @@
// CHECK-NEXT: core.uninitialized.CapturedBlockVariable
// CHECK-NEXT: core.uninitialized.UndefReturn
// CHECK-NEXT: deadcode.DeadStores
-// CHECK-NEXT: nullability....
[truncated]
|
NagyDonat
commented
Jun 11, 2025
| @@ -710,29 +706,28 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, | |||
| Nullability RetExprTypeLevelNullability = | |||
| getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType()); | |||
|
|
|||
| bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull && | |||
There was a problem hiding this comment.
I refactored this code block because the name of this local boolean NullReturnedFromNonNull was too similar to the checker frontend NullReturnedFromNonnull (which I introduced).
I eliminated the boolean instead of renaming it, because I feel that the old
bool B = some condition;
if (B && other conditions) {
...
}
if (B) {
...
}was less readable than the structure
if (some condition) {
if (other conditions) {
...
}
...
}that I'm introducing. (Deeply nested blocks are not ideal, but breaking up a logically coherent block into multiple if statements is even worse.)
|
|
||
| // FIXME: All bug types share the same Description ("Nullability") since the | ||
| // creation of this checker. We should write more descriptive descriptions... | ||
| // or just eliminate the Description field if it is meaningless? |
There was a problem hiding this comment.
How is the Description displayed to the user? Can we use the HelpText from Checkers.td as the Description?
There was a problem hiding this comment.
How is the
Descriptiondisplayed to the user?
The Description is included in the plist output (which is a machine-friendly "raw" output format), so tools that consume plist may display it to the user -- but e.g. CodeChecker ignores it. (In the HTML output, the Description of the bug type appears within a <!-- BUGTYPE ... --> comment, i.e. it's there perhaps for debugging purposes but not actually visible.)
Moreover, the Description of the BugType affects the hash and the ordering of the PathDiagnostic objects, so perturbing it will influence the analyzer results.
Can we use the
HelpTextfromCheckers.tdas the Description?
No, the HelpText is associated with a whole checker (more precisely, a user-facing CheckerFrontend like nullability.NullPassedToNonnull), while this Description is a data member of a BugType -- and a single CheckerFrontend may "own" multiple BugTypes. (In this particular checker there is 1:1 correspondence between the CheckerFrontends and BugTypes, so I'm using the trivial convenience wrapper CheckerFrontendWithBugType to initialize a CheckerFrontend and the single BugType which belongs to it, but this is not the general case.)
Also, AFAIK the Description of the bug type is usually a very short name-like string, while the HelpText is a somewhat longer documentation-like text.
|
I do not have many suggestions for this change. It's great that we can get rid of the meaningless |
steakhal
reviewed
Jun 15, 2025
Was broken by git-clang-format which is not aware of string concatenation.
|
@steakhal Thanks for spotting these broken lines – these blocks were automatically reformatted by |
steakhal
approved these changes
Jun 16, 2025
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 17, 2025
This commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076 This commit removes the dummy checker `nullability.NullabilityBase` because it was hidden from the users and didn't have any useful role except for helping the registration of the checker parts in the old ad-hoc system (which is replaced by the new standardized framework). Except for the removal of this dummy checker, no functional changes intended.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076
This commit removes the dummy checker
nullability.NullabilityBasebecause it was hidden from the users and didn't have any useful role except for helping the registration of the checker parts in the old ad-hoc system (which is replaced by the new standardized framework).Except for the removal of this dummy checker, no functional changes intended.