[nullability] Model return values for const methods returning bool.
So far, we were only modeling return values for const methods returning
pointers, but it makes sense to do this for booleans too:
- This allows us to model `operator bool()` and `has_value()` on types such as
`std::optional` and `std::function`. We've seen a pattern where a pointer
is nonnull iff a `std::optional` or `std::function` has a value, and the
pointer is then later dereferenced only if the corresponding other object has
a value. This pattern appears to be common enough to be worth supporting.
- Producing a value for these boolean return values may actually help
convergence. It's likely that they will be used in a terminator condition at
some point; if we don't have a value for them, we will create a fresh atom
for the terminator condition, and this is likely worse for convergence.
PiperOrigin-RevId: 619932515
Change-Id: Ifac0cb4f814c7697484a3c52f198f097bbb952d1
diff --git a/nullability/pointer_nullability_analysis.cc b/nullability/pointer_nullability_analysis.cc
index f314a59..6d1374a 100644
--- a/nullability/pointer_nullability_analysis.cc
+++ b/nullability/pointer_nullability_analysis.cc
@@ -1068,18 +1068,20 @@
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
TransferState<PointerNullabilityLattice> &State) {
- if (!isSupportedRawPointerType(CE->getType()) || !CE->isPRValue() ||
- RecordLoc == nullptr) {
+ if ((!isSupportedRawPointerType(CE->getType()) &&
+ !CE->getType()->isBooleanType()) ||
+ !CE->isPRValue() || RecordLoc == nullptr) {
// Perform default handling.
transferValue_CallExpr(CE, Result, State);
return;
}
- PointerValue *PointerVal =
+ Value *Val =
State.Lattice.getConstMethodReturnValue(*RecordLoc, CE, State.Env);
- if (PointerVal) {
- State.Env.setValue(*CE, *PointerVal);
+ if (Val == nullptr) return;
+
+ State.Env.setValue(*CE, *Val);
+ if (auto *PointerVal = dyn_cast<PointerValue>(Val))
initPointerFromTypeNullability(*PointerVal, CE, State);
- }
}
void transferValue_ConstMemberCall(
diff --git a/nullability/pointer_nullability_lattice.cc b/nullability/pointer_nullability_lattice.cc
index ea51f29..466dc3c 100644
--- a/nullability/pointer_nullability_lattice.cc
+++ b/nullability/pointer_nullability_lattice.cc
@@ -4,6 +4,7 @@
#include "nullability/pointer_nullability_lattice.h"
+#include <cassert>
#include <functional>
#include <optional>
@@ -24,7 +25,7 @@
namespace {
using dataflow::LatticeJoinEffect;
-using dataflow::PointerValue;
+using dataflow::Value;
// Returns overridden nullability information associated with a declaration.
// For now we only track top-level decl nullability symbolically and check for
@@ -59,18 +60,19 @@
return Iterator->second;
}
-absl::Nullable<dataflow::PointerValue *>
+absl::Nullable<dataflow::Value *>
PointerNullabilityLattice::getConstMethodReturnValue(
const dataflow::RecordStorageLocation &RecordLoc,
absl::Nonnull<const CallExpr *> CE, dataflow::Environment &Env) {
+ assert(CE->getType()->isPointerType() || CE->getType()->isBooleanType());
auto &ObjMap = ConstMethodReturnValues[&RecordLoc];
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr) return nullptr;
auto it = ObjMap.find(DirectCallee);
if (it != ObjMap.end()) return it->second;
- auto *PV = cast<dataflow::PointerValue>(Env.createValue(CE->getType()));
- ObjMap.insert({DirectCallee, PV});
- return PV;
+ dataflow::Value *Val = Env.createValue(CE->getType());
+ if (Val != nullptr) ObjMap.insert({DirectCallee, Val});
+ return Val;
}
void PointerNullabilityLattice::overrideNullabilityFromDecl(
@@ -101,7 +103,7 @@
const auto &OtherDeclToVal = It->second;
auto &JoinedDeclToVal = JoinedMap[Loc];
for (auto [Func, Val] : DeclToVal) {
- PointerValue *OtherVal = OtherDeclToVal.lookup(Func);
+ Value *OtherVal = OtherDeclToVal.lookup(Func);
if (OtherVal == nullptr || OtherVal != Val) {
Effect = LatticeJoinEffect::Changed;
continue;
diff --git a/nullability/pointer_nullability_lattice.h b/nullability/pointer_nullability_lattice.h
index b05254a..c9c0c6e 100644
--- a/nullability/pointer_nullability_lattice.h
+++ b/nullability/pointer_nullability_lattice.h
@@ -59,10 +59,10 @@
absl::Nonnull<const Expr *> E,
const std::function<TypeNullability()> &GetNullability);
- // Gets the PointerValue associated with the RecordStorageLocation and
- // MethodDecl of the CallExpr, creating one if it doesn't yet exist. Requires
- // the CXXMemberCallExpr to have a supported pointer type.
- absl::Nullable<dataflow::PointerValue *> getConstMethodReturnValue(
+ // Returns the `Value` associated with the `RecordStorageLocation` and
+ // `MethodDecl` of `CE`, creating one if it doesn't yet exist.
+ // The type of `CE` must be either a raw pointer or boolean.
+ absl::Nullable<dataflow::Value *> getConstMethodReturnValue(
const dataflow::RecordStorageLocation &RecordLoc,
absl::Nonnull<const CallExpr *> CE, dataflow::Environment &Env);
@@ -89,7 +89,7 @@
// from that const method.
using ConstMethodReturnValuesType = llvm::SmallDenseMap<
const dataflow::RecordStorageLocation *,
- llvm::SmallDenseMap<const FunctionDecl *, dataflow::PointerValue *>>;
+ llvm::SmallDenseMap<const FunctionDecl *, dataflow::Value *>>;
ConstMethodReturnValuesType ConstMethodReturnValues;
};
diff --git a/nullability/test/function_calls.cc b/nullability/test/function_calls.cc
index 321b9fe..04ae877 100644
--- a/nullability/test/function_calls.cc
+++ b/nullability/test/function_calls.cc
@@ -903,6 +903,28 @@
)cc"));
}
+TEST(PointerNullabilityTest, ConstMethodReturningBool) {
+ // This tests (indirectly) that we also model const methods returning
+ // booleans. We use `operator bool()` as the specific const method because
+ // this then also gives us coverage of this special case (which is quite
+ // common, for example in `std::function`).
+ EXPECT_TRUE(checkDiagnostics(R"cc(
+ struct S {
+ operator bool() const;
+ };
+
+ void target(S s) {
+ int *p = nullptr;
+ int i = 0;
+ if (s) p = &i;
+ if (s)
+ // Dereference is safe because we know `operator bool()` will return the
+ // same thing both times.
+ *p;
+ }
+ )cc"));
+}
+
TEST(PointerNullabilityTest, NonConstMethodClearsPointerMembers) {
// This is a crash repro.
EXPECT_TRUE(checkDiagnostics(R"cc(
