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

Changes from 1 commit
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
157 changes: 81 additions & 76 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,38 @@ class NullabilityChecker
void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
const char *Sep) const override;

enum CheckKind {
CK_NullPassedToNonnull,
CK_NullReturnedFromNonnull,
CK_NullableDereferenced,
CK_NullablePassedToNonnull,
CK_NullableReturnedFromNonnull,
CK_NumCheckKinds
// FIXME: This enumeration of checker parts is extremely similar to the
// ErrorKind enum. It would be nice to unify them to simplify the code.
// FIXME: The modeling checker NullabilityBase is a dummy "empty checker
// part" that registers this checker class without enabling any of the real
// checker parts. As far as I understand no other checker references it, so
// it should be removed.
enum : CheckerPartIdx {
NullabilityBase,
NullPassedToNonnullChecker,
NullReturnedFromNonnullChecker,
NullableDereferencedChecker,
NullablePassedToNonnullChecker,
NullableReturnedFromNonnullChecker,
NumCheckerParts
};

bool ChecksEnabled[CK_NumCheckKinds] = {false};
CheckerNameRef CheckNames[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];

const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
if (!BTs[Kind])
BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
categories::MemoryError));
return BTs[Kind];
}
// FIXME: Currently the `Description` fields of these `BugType`s are all
// identical ("Nullability") -- they should be more descriptive than this.
// NOTE: NullabilityBase is a dummy checker part that does nothing, so its
// bug type is left empty.
BugType BugTypes[NumCheckerParts] = {
{this, NullabilityBase, "", ""},
{this, NullPassedToNonnullChecker, "Nullability",
categories::MemoryError},
{this, NullReturnedFromNonnullChecker, "Nullability",
categories::MemoryError},
{this, NullableDereferencedChecker, "Nullability",
categories::MemoryError},
{this, NullablePassedToNonnullChecker, "Nullability",
categories::MemoryError},
{this, NullableReturnedFromNonnullChecker, "Nullability",
categories::MemoryError}};

// When set to false no nullability information will be tracked in
// NullabilityMap. It is possible to catch errors like passing a null pointer
Expand Down Expand Up @@ -163,17 +176,16 @@ class NullabilityChecker
///
/// When \p SuppressPath is set to true, no more bugs will be reported on this
/// path by this checker.
void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
ExplodedNode *N, const MemRegion *Region,
CheckerContext &C,
void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error,
CheckerPartIdx Idx, ExplodedNode *N,
const MemRegion *Region, CheckerContext &C,
const Stmt *ValueExpr = nullptr,
bool SuppressPath = false) const;

void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
const MemRegion *Region, BugReporter &BR,
void reportBug(StringRef Msg, ErrorKind Error, CheckerPartIdx Idx,
ExplodedNode *N, const MemRegion *Region, BugReporter &BR,
const Stmt *ValueExpr = nullptr) const {
const std::unique_ptr<BugType> &BT = getBugType(CK);
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
auto R = std::make_unique<PathSensitiveBugReport>(BugTypes[Idx], Msg, N);
if (Region) {
R->markInteresting(Region);
R->addVisitor<NullabilityBugVisitor>(Region);
Expand Down Expand Up @@ -479,7 +491,7 @@ static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N,
}

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

reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
reportBug(Msg, Error, Idx, N, Region, C.getBugReporter(), ValueExpr);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll implement it.

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

}

/// Cleaning up the program state.
Expand Down Expand Up @@ -545,19 +557,19 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
if (!TrackedNullability)
return;

if (ChecksEnabled[CK_NullableDereferenced] &&
if (isPartEnabled(NullableDereferencedChecker) &&
TrackedNullability->getValue() == Nullability::Nullable) {
BugReporter &BR = *Event.BR;
// Do not suppress errors on defensive code paths, because dereferencing
// a nullable pointer is always an error.
if (Event.IsDirectDereference)
reportBug("Nullable pointer is dereferenced",
ErrorKind::NullableDereferenced, CK_NullableDereferenced,
ErrorKind::NullableDereferenced, NullableDereferencedChecker,
Event.SinkNode, Region, BR);
else {
reportBug("Nullable pointer is passed to a callee that requires a "
"non-null",
ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
ErrorKind::NullablePassedToNonnull, NullableDereferencedChecker,
Event.SinkNode, Region, BR);
}
}
Expand Down Expand Up @@ -711,7 +723,8 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,

bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
if (isPartEnabled(NullReturnedFromNonnullChecker) &&
NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
!InSuppressedMethodFamily) {
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
Expand All @@ -725,7 +738,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
OS << " returned from a " << C.getDeclDescription(D) <<
" that is expected to return a non-null value";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
CK_NullReturnedFromNonnull, N, nullptr, C,
NullReturnedFromNonnullChecker, N, nullptr, C,
RetExpr);
return;
}
Expand All @@ -746,7 +759,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
State->get<NullabilityMap>(Region);
if (TrackedNullability) {
Nullability TrackedNullabValue = TrackedNullability->getValue();
if (ChecksEnabled[CK_NullableReturnedFromNonnull] &&
if (isPartEnabled(NullableReturnedFromNonnullChecker) &&
Nullness != NullConstraint::IsNotNull &&
TrackedNullabValue == Nullability::Nullable &&
RequiredNullability == Nullability::Nonnull) {
Expand All @@ -759,7 +772,8 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
" that is expected to return a non-null value";

reportBugIfInvariantHolds(OS.str(), ErrorKind::NullableReturnedToNonnull,
CK_NullableReturnedFromNonnull, N, Region, C);
NullableReturnedFromNonnullChecker, N, Region,
C);
}
return;
}
Expand Down Expand Up @@ -810,7 +824,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,

unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;

if (ChecksEnabled[CK_NullPassedToNonnull] &&
if (isPartEnabled(NullPassedToNonnullChecker) &&
Nullness == NullConstraint::IsNull &&
ArgExprTypeLevelNullability != Nullability::Nonnull &&
RequiredNullability == Nullability::Nonnull &&
Expand All @@ -825,7 +839,8 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
OS << " passed to a callee that requires a non-null " << ParamIdx
<< llvm::getOrdinalSuffix(ParamIdx) << " parameter";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull,
CK_NullPassedToNonnull, N, nullptr, C, ArgExpr,
NullPassedToNonnullChecker, N, nullptr, C,
ArgExpr,
/*SuppressPath=*/false);
return;
}
Expand All @@ -842,7 +857,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
TrackedNullability->getValue() != Nullability::Nullable)
continue;

if (ChecksEnabled[CK_NullablePassedToNonnull] &&
if (isPartEnabled(NullablePassedToNonnullChecker) &&
RequiredNullability == Nullability::Nonnull &&
isDiagnosableCall(Call)) {
ExplodedNode *N = C.addTransition(State);
Expand All @@ -851,16 +866,16 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
OS << "Nullable pointer is passed to a callee that requires a non-null "
<< ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NullablePassedToNonnull,
CK_NullablePassedToNonnull, N, Region, C,
NullablePassedToNonnullChecker, N, Region, C,
ArgExpr, /*SuppressPath=*/true);
return;
}
if (ChecksEnabled[CK_NullableDereferenced] &&
if (isPartEnabled(NullableDereferencedChecker) &&
Param->getType()->isReferenceType()) {
ExplodedNode *N = C.addTransition(State);
reportBugIfInvariantHolds("Nullable pointer is dereferenced",
ErrorKind::NullableDereferenced,
CK_NullableDereferenced, N, Region, C,
NullableDereferencedChecker, N, Region, C,
ArgExpr, /*SuppressPath=*/true);
return;
}
Expand Down Expand Up @@ -1295,7 +1310,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,

bool NullAssignedToNonNull = (LocNullability == Nullability::Nonnull &&
RhsNullness == NullConstraint::IsNull);
if (ChecksEnabled[CK_NullPassedToNonnull] && NullAssignedToNonNull &&
if (isPartEnabled(NullPassedToNonnullChecker) && NullAssignedToNonNull &&
ValNullability != Nullability::Nonnull &&
ValueExprTypeLevelNullability != Nullability::Nonnull &&
!isARCNilInitializedLocal(C, S)) {
Expand All @@ -1314,7 +1329,8 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
OS << (LocType->isObjCObjectPointerType() ? "nil" : "Null");
OS << " assigned to a pointer which is expected to have non-null value";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilAssignedToNonnull,
CK_NullPassedToNonnull, N, nullptr, C, ValueStmt);
NullPassedToNonnullChecker, N, nullptr, C,
ValueStmt);
return;
}

Expand All @@ -1340,14 +1356,15 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
if (RhsNullness == NullConstraint::IsNotNull ||
TrackedNullability->getValue() != Nullability::Nullable)
return;
if (ChecksEnabled[CK_NullablePassedToNonnull] &&
if (isPartEnabled(NullablePassedToNonnullChecker) &&
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
"which is expected to have non-null value",
ErrorKind::NullableAssignedToNonnull,
CK_NullablePassedToNonnull, N, ValueRegion, C);
NullablePassedToNonnullChecker, N, ValueRegion,
C);
}
return;
}
Expand Down Expand Up @@ -1394,38 +1411,26 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State,
}
}

void ento::registerNullabilityBase(CheckerManager &mgr) {
mgr.registerChecker<NullabilityChecker>();
}

bool ento::shouldRegisterNullabilityBase(const CheckerManager &mgr) {
return true;
}

#define REGISTER_CHECKER(name, trackingRequired) \
void ento::register##name##Checker(CheckerManager &mgr) { \
NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>(); \
checker->ChecksEnabled[NullabilityChecker::CK_##name] = true; \
checker->CheckNames[NullabilityChecker::CK_##name] = \
mgr.getCurrentCheckerName(); \
checker->NeedTracking = checker->NeedTracking || trackingRequired; \
checker->NoDiagnoseCallsToSystemHeaders = \
checker->NoDiagnoseCallsToSystemHeaders || \
mgr.getAnalyzerOptions().getCheckerBooleanOption( \
checker, "NoDiagnoseCallsToSystemHeaders", true); \
#define REGISTER_CHECKER(Part, TrackingRequired) \
void ento::register##Part(CheckerManager &Mgr) { \
auto *Checker = \
Mgr.registerChecker<NullabilityChecker, NullabilityChecker::Part>(); \
Checker->NeedTracking = Checker->NeedTracking || TrackingRequired; \
Checker->NoDiagnoseCallsToSystemHeaders = \
Checker->NoDiagnoseCallsToSystemHeaders || \
Mgr.getAnalyzerOptions().getCheckerBooleanOption( \
Checker, "NoDiagnoseCallsToSystemHeaders", true); \
} \
\
bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
return true; \
}

// The checks are likely to be turned on by default and it is possible to do
// them without tracking any nullability related information. As an optimization
// no nullability information will be tracked when only these two checks are
// enables.
REGISTER_CHECKER(NullPassedToNonnull, false)
REGISTER_CHECKER(NullReturnedFromNonnull, false)

REGISTER_CHECKER(NullableDereferenced, true)
REGISTER_CHECKER(NullablePassedToNonnull, true)
REGISTER_CHECKER(NullableReturnedFromNonnull, true)
bool ento::shouldRegister##Part(const CheckerManager &) { return true; }

// These checker parts are likely to be turned on by default and they don't
// need the tracking of nullability related information. As an optimization
// nullability information won't be tracked when the rest are disabled.
REGISTER_CHECKER(NullabilityBase, false)
REGISTER_CHECKER(NullPassedToNonnullChecker, false)
REGISTER_CHECKER(NullReturnedFromNonnullChecker, false)

REGISTER_CHECKER(NullableDereferencedChecker, true)
REGISTER_CHECKER(NullablePassedToNonnullChecker, true)
REGISTER_CHECKER(NullableReturnedFromNonnullChecker, true)
Loading