[nullability] Split diagnosis into two callbacks.
The framework recently added the ability to run post-analysis callbacks not just
on the state after the transfer function for a `CFGElement` has been run but
also before it.
Conceptually, diagnostic callbacks usually make more sense to run on the
"before" state because we usually want to check preconditions for the operation,
so almost all diagnostics are now run in this way.
This makes the diagnostic callback for smart pointers more intuitive: When we
were running this on the "after" state, to check whether we were assigning a
nullable pointer to a nonnull variable, we somewhat unintuitively had to check
the nullability of the _left-hand_ side (because the right-hand side of a smart
pointer assignment is always null after the assignment has been carried out).
Now that the callback is running on the "before" state, we can more intuitively
check the nullability of the right-hand side.
More importantly, this will enable us to perform diagnosis correctly on the `++`
and `--` operators. For these, we need to access the "before" state to check
whether the pointer being incremented is nullable. (The "after" state of the
pointer is always non-null.)
There is one check that we are still running on the "after" state, namely
`diagnoseMovedFromNonnullSmartPointer`. This needs to be run on the "after"
state because in the "before" state, the pointer and its nullability properties
may not yet be initialized.
PiperOrigin-RevId: 646351557
Change-Id: Ib51e4d236bfc8662f55ef3e6537d65bb1a9d0c57
diff --git a/bazel/llvm.bzl b/bazel/llvm.bzl
index b6c2c34..57e831b 100644
--- a/bazel/llvm.bzl
+++ b/bazel/llvm.bzl
@@ -53,7 +53,7 @@
executable = False,
)
-LLVM_COMMIT_SHA = "e5b0c210cc4cdaae7075ad2d4aa1efe4eb4cb0c5"
+LLVM_COMMIT_SHA = "5cd0ba30f53d11835dbfd05ad4071d397387fb04"
def llvm_loader_repository_dependencies():
# This *declares* the dependency, but it won't actually be *downloaded* unless it's used.
diff --git a/nullability/pointer_nullability_diagnosis.cc b/nullability/pointer_nullability_diagnosis.cc
index abc9175..d278d62 100644
--- a/nullability/pointer_nullability_diagnosis.cc
+++ b/nullability/pointer_nullability_diagnosis.cc
@@ -184,13 +184,8 @@
if (!LHSNullability) return {};
return diagnoseAssignmentLike(
- Op->getArg(0)->getType(), *LHSNullability,
- // Because `State` reflects the state after the assignment has already
- // happened, we need to get the assigned value from the LHS, i.e.
- // `Op->getArg(0)`. Using `Op->getArg(1)` doesn't work, because it is null
- // after a move-assignment.
- Op->getArg(0), State.Env, *Result.Context,
- PointerNullabilityDiagnostic::Context::Assignment);
+ Op->getArg(0)->getType(), *LHSNullability, Op->getArg(1), State.Env,
+ *Result.Context, PointerNullabilityDiagnostic::Context::Assignment);
}
SmallVector<PointerNullabilityDiagnostic> diagnoseSmartPointerReset(
@@ -585,74 +580,64 @@
}
}
-DiagTransferFunc pointerNullabilityDiagnoser(
+DiagTransferFunc pointerNullabilityDiagnoserBefore() {
+ // Almost all diagnosis callbacks should be run before the transfer function
+ // has been applied because we want to check preconditions for the operation
+ // performed by the `CFGElement`.
+ return CFGMatchSwitchBuilder<const DiagTransferState,
+ SmallVector<PointerNullabilityDiagnostic>>()
+ // (*)
+ .CaseOfCFGStmt<UnaryOperator>(isPointerDereference(), diagnoseDereference)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("*"),
+ diagnoseSmartPointerDereference)
+ // ([])
+ .CaseOfCFGStmt<ArraySubscriptExpr>(isPointerSubscript(),
+ diagnoseSubscript)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("[]"),
+ diagnoseSmartPointerDereference)
+ // (->)
+ .CaseOfCFGStmt<MemberExpr>(isPointerArrow(), diagnoseArrow)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("->"),
+ diagnoseSmartPointerDereference)
+ // (=) / `reset()`
+ .CaseOfCFGStmt<BinaryOperator>(
+ binaryOperator(hasOperatorName("="), hasLHS(isPointerExpr())),
+ diagnoseAssignment)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("="),
+ diagnoseSmartPointerAssignment)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isSmartPointerMethodCall("reset"),
+ diagnoseSmartPointerReset)
+ // Check compatibility of parameter assignments and return values.
+ .CaseOfCFGStmt<CallExpr>(callExpr(), diagnoseCallExpr)
+ .CaseOfCFGStmt<CXXConstructExpr>(cxxConstructExpr(),
+ diagnoseConstructExpr)
+ .CaseOfCFGStmt<ReturnStmt>(isPointerReturn(), diagnoseReturn)
+ // Check compatibility of member initializers.
+ .CaseOfCFGInit<CXXCtorInitializer>(isCtorMemberInitializer(),
+ diagnoseMemberInitializer)
+ // Check compatibility of initializer lists.
+ .CaseOfCFGStmt<InitListExpr>(initListExpr(), diagnoseInitListExpr)
+ .Build();
+}
+
+DiagTransferFunc pointerNullabilityDiagnoserAfter(
const AllowedMovedFromNonnullSmartPointerExprs &AllowedMovedFromNonnull) {
- DiagTransferFunc MainDiagnoser =
- CFGMatchSwitchBuilder<const DiagTransferState,
- SmallVector<PointerNullabilityDiagnostic>>()
- // (*)
- .CaseOfCFGStmt<UnaryOperator>(isPointerDereference(),
- diagnoseDereference)
- .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("*"),
- diagnoseSmartPointerDereference)
- // ([])
- .CaseOfCFGStmt<ArraySubscriptExpr>(isPointerSubscript(),
- diagnoseSubscript)
- .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("[]"),
- diagnoseSmartPointerDereference)
- // (->)
- .CaseOfCFGStmt<MemberExpr>(isPointerArrow(), diagnoseArrow)
- .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("->"),
- diagnoseSmartPointerDereference)
- // (=) / `reset()`
- .CaseOfCFGStmt<BinaryOperator>(
- binaryOperator(hasOperatorName("="), hasLHS(isPointerExpr())),
- diagnoseAssignment)
- .CaseOfCFGStmt<CXXOperatorCallExpr>(isSmartPointerOperatorCall("="),
- diagnoseSmartPointerAssignment)
- .CaseOfCFGStmt<CXXMemberCallExpr>(isSmartPointerMethodCall("reset"),
- diagnoseSmartPointerReset)
- // Check compatibility of parameter assignments and return values.
- .CaseOfCFGStmt<CallExpr>(callExpr(), diagnoseCallExpr)
- .CaseOfCFGStmt<CXXConstructExpr>(cxxConstructExpr(),
- diagnoseConstructExpr)
- .CaseOfCFGStmt<ReturnStmt>(isPointerReturn(), diagnoseReturn)
- // Check compatibility of member initializers.
- .CaseOfCFGInit<CXXCtorInitializer>(isCtorMemberInitializer(),
- diagnoseMemberInitializer)
- // Check compatibility of initializer lists.
- .CaseOfCFGStmt<InitListExpr>(initListExpr(), diagnoseInitListExpr)
- .Build();
-
- // We need a second transfer function to diagnose moved-from nonnull smart
- // pointers because it needs to look at all expressions of smart pointer type.
- // We can't do this in the existing transfer function because some of the
- // existing cases may have smart pointer type, and only one of the cases is
- // ever run.
- DiagTransferFunc MovedFromNonnullPointerDiagnoser =
- CFGMatchSwitchBuilder<const DiagTransferState,
- SmallVector<PointerNullabilityDiagnostic>>()
- .CaseOfCFGStmt<Expr>(
- expr(hasType(isSupportedSmartPointer()), isGLValue()),
- [&AllowedMovedFromNonnull](absl::Nonnull<const Expr *> E,
- const MatchFinder::MatchResult &Result,
- const DiagTransferState &State)
- -> SmallVector<PointerNullabilityDiagnostic> {
- if (AllowedMovedFromNonnull.allowed(E)) return {};
- return diagnoseMovedFromNonnullSmartPointer(E, Result, State);
- })
- .Build();
-
- return [MainDiagnoser = std::move(MainDiagnoser),
- MovedFromNonnullPointerDiagnoser =
- std::move(MovedFromNonnullPointerDiagnoser)](
- const CFGElement &Elt, ASTContext &ASTCtx,
- const DiagTransferState &State) {
- SmallVector<PointerNullabilityDiagnostic> Diags =
- MainDiagnoser(Elt, ASTCtx, State);
- Diags.append(MovedFromNonnullPointerDiagnoser(Elt, ASTCtx, State));
- return Diags;
- };
+ return CFGMatchSwitchBuilder<const DiagTransferState,
+ SmallVector<PointerNullabilityDiagnostic>>()
+ // `diagnoseMovedFromNonnullSmartPointer` needs to be run after the
+ // transfer function has been applied so that the pointer and its
+ // nullability properties are guaranteed be initialized (through
+ // `ensureSmartPointerInitialized()`).
+ .CaseOfCFGStmt<Expr>(
+ expr(hasType(isSupportedSmartPointer()), isGLValue()),
+ [&AllowedMovedFromNonnull](absl::Nonnull<const Expr *> E,
+ const MatchFinder::MatchResult &Result,
+ const DiagTransferState &State)
+ -> SmallVector<PointerNullabilityDiagnostic> {
+ if (AllowedMovedFromNonnull.allowed(E)) return {};
+ return diagnoseMovedFromNonnullSmartPointer(E, Result, State);
+ })
+ .Build();
}
} // namespace
@@ -705,16 +690,26 @@
PointerNullabilityAnalysis Analysis(Ctx, Env, Pragmas);
- auto Result = dataflow::runDataflowAnalysis(
- *CFG, Analysis, Env,
- [&, Diagnoser(pointerNullabilityDiagnoser(AllowedMovedFromNonnull))](
+ dataflow::CFGEltCallbacks<PointerNullabilityAnalysis> PostAnalysisCallbacks;
+ PostAnalysisCallbacks.Before =
+ [&, Diagnoser = pointerNullabilityDiagnoserBefore()](
const CFGElement &Elt,
const dataflow::DataflowAnalysisState<PointerNullabilityLattice>
&State) mutable {
auto EltDiagnostics = Diagnoser(Elt, Ctx, {State.Lattice, State.Env});
llvm::move(EltDiagnostics, std::back_inserter(Diags));
- },
- MaxBlockVisits);
+ };
+ PostAnalysisCallbacks.After =
+ [&,
+ Diagnoser = pointerNullabilityDiagnoserAfter(AllowedMovedFromNonnull)](
+ const CFGElement &Elt,
+ const dataflow::DataflowAnalysisState<PointerNullabilityLattice>
+ &State) mutable {
+ auto EltDiagnostics = Diagnoser(Elt, Ctx, {State.Lattice, State.Env});
+ llvm::move(EltDiagnostics, std::back_inserter(Diags));
+ };
+ auto Result = dataflow::runDataflowAnalysis(
+ *CFG, Analysis, Env, PostAnalysisCallbacks, MaxBlockVisits);
if (!Result) return Result.takeError();
if (Solver->reachedLimit())
return llvm::createStringError(llvm::errc::interrupted,
