Skip to content

[analyzer] Conversion to CheckerFamily: NullabilityChecker #143735

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

Merged
merged 2 commits into from
Jun 17, 2025
Merged

[analyzer] Conversion to CheckerFamily: NullabilityChecker #143735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f0abfe
[analyzer] Conversion to CheckerFamily: NullabilityChecker
NagyDonat Jun 11, 2025
4aa5816
Fix wrapping of multiline strings
NagyDonat Jun 16, 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
53 changes: 24 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,34 @@ 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
177 changes: 84 additions & 93 deletions clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the Description displayed to the user? Can we use the HelpText from Checkers.td as the Description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is the Description displayed 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 HelpText from Checkers.td as 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.

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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -710,29 +706,28 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());

bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Copy link
Contributor Author

@NagyDonat NagyDonat Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

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;
Expand All @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}
Expand All @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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; \
}

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 @@ -34,7 +34,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
Loading