[nullability] Add support for `operator->` on `std::optional`.
We've repeatedly seen the pattern where people have a pointer stored in an
optional struct, perform a null-check on the pointer ("through" the optional),
then dereference it. Introducing limited support for optionals prevents false
positives on these code patterns.
In addition to `std::optional`, we also support
[`absl::optional`](https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional.h)
and
[`folly::Optional`](https://github.com/facebook/folly/blob/main/folly/Optional.h).
PiperOrigin-RevId: 609261743
Change-Id: I79a0673d1fee71071dc23338b0d67fd7b0728c3d
diff --git a/bazel/llvm.bzl b/bazel/llvm.bzl
index 52c9422..505cf5e 100644
--- a/bazel/llvm.bzl
+++ b/bazel/llvm.bzl
@@ -53,7 +53,7 @@
executable = False,
)
-LLVM_COMMIT_SHA = "4c6043de0b837d23699424d875057d00956d80ac"
+LLVM_COMMIT_SHA = "2e29c91b96832504b9008be5e095f7dd640cdea0"
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_analysis.cc b/nullability/pointer_nullability_analysis.cc
index 1450863..0babeb6 100644
--- a/nullability/pointer_nullability_analysis.cc
+++ b/nullability/pointer_nullability_analysis.cc
@@ -1061,28 +1061,42 @@
}
}
+void handleConstMemberCall(absl::Nonnull<const CallExpr *> CE,
+ dataflow::RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ TransferState<PointerNullabilityLattice> &State) {
+ if (!isSupportedRawPointerType(CE->getType()) || !CE->isPRValue() ||
+ RecordLoc == nullptr) {
+ // Perform default handling.
+ transferValue_CallExpr(CE, Result, State);
+ return;
+ }
+ PointerValue *PointerVal =
+ State.Lattice.getConstMethodReturnValue(*RecordLoc, CE, State.Env);
+ if (PointerVal) {
+ State.Env.setValue(*CE, *PointerVal);
+ initPointerFromTypeNullability(*PointerVal, CE, State);
+ }
+}
+
void transferValue_ConstMemberCall(
absl::Nonnull<const CXXMemberCallExpr *> MCE,
const MatchFinder::MatchResult &Result,
TransferState<PointerNullabilityLattice> &State) {
- if (!isSupportedRawPointerType(MCE->getType()) || !MCE->isPRValue()) {
- // We can't handle it as a special case, but still need to handle it.
- transferValue_CallExpr(MCE, Result, State);
- return;
- }
- dataflow::RecordStorageLocation *RecordLoc =
- dataflow::getImplicitObjectLocation(*MCE, State.Env);
- if (RecordLoc == nullptr) {
- // We can't handle it as a special case, but still need to handle it.
- transferValue_CallExpr(MCE, Result, State);
- return;
- }
- PointerValue *PointerVal =
- State.Lattice.getConstMethodReturnValue(*RecordLoc, MCE, State.Env);
- if (PointerVal) {
- State.Env.setValue(*MCE, *PointerVal);
- initPointerFromTypeNullability(*PointerVal, MCE, State);
- }
+ handleConstMemberCall(
+ MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+}
+
+void transferValue_OptionalOperatorArrowCall(
+ absl::Nonnull<const CXXOperatorCallExpr *> OCE,
+ const MatchFinder::MatchResult &Result,
+ TransferState<PointerNullabilityLattice> &State) {
+ auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ // `optional::operator->` isn't necessarily const, but it behaves the way we
+ // model "const member calls": It always returns the same pointer if the
+ // optional wasn't mutated in the meantime.
+ handleConstMemberCall(OCE, RecordLoc, Result, State);
}
void handleNonConstMemberCall(absl::Nonnull<const CallExpr *> CE,
@@ -1484,6 +1498,9 @@
transferValue_AccessorCall)
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isOptionalOperatorArrowCall(),
+ transferValue_OptionalOperatorArrowCall)
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
.CaseOfCFGStmt<CXXOperatorCallExpr>(
diff --git a/nullability/pointer_nullability_lattice.h b/nullability/pointer_nullability_lattice.h
index 898e55f..33b0590 100644
--- a/nullability/pointer_nullability_lattice.h
+++ b/nullability/pointer_nullability_lattice.h
@@ -75,13 +75,14 @@
// the CXXMemberCallExpr to have a supported pointer type.
absl::Nonnull<dataflow::PointerValue *> getConstMethodReturnValue(
const dataflow::RecordStorageLocation &RecordLoc,
- absl::Nonnull<const CXXMemberCallExpr *> MCE,
- dataflow::Environment &Env) {
+ absl::Nonnull<const CallExpr *> CE, dataflow::Environment &Env) {
auto &ObjMap = ConstMethodReturnValues[&RecordLoc];
- auto it = ObjMap.find(MCE->getMethodDecl());
+ 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(MCE->getType()));
- ObjMap.insert({MCE->getMethodDecl(), PV});
+ auto *PV = cast<dataflow::PointerValue>(Env.createValue(CE->getType()));
+ ObjMap.insert({DirectCallee, PV});
return PV;
}
@@ -119,7 +120,7 @@
// from that const method.
llvm::SmallDenseMap<
const dataflow::RecordStorageLocation *,
- llvm::SmallDenseMap<const CXXMethodDecl *, dataflow::PointerValue *>>
+ llvm::SmallDenseMap<const FunctionDecl *, dataflow::PointerValue *>>
ConstMethodReturnValues;
};
diff --git a/nullability/pointer_nullability_matchers.cc b/nullability/pointer_nullability_matchers.cc
index 854b68e..7ac6378 100644
--- a/nullability/pointer_nullability_matchers.cc
+++ b/nullability/pointer_nullability_matchers.cc
@@ -57,6 +57,7 @@
using ast_matchers::isInStdNamespace;
using ast_matchers::isMemberInitializer;
using ast_matchers::memberExpr;
+using ast_matchers::ofClass;
using ast_matchers::parameterCountIs;
using ast_matchers::pointee;
using ast_matchers::pointerType;
@@ -109,6 +110,13 @@
callee(cxxMethodDecl(parameterCountIs(0), isConst())));
}
+Matcher<Stmt> isOptionalOperatorArrowCall() {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("->"),
+ callee(cxxMethodDecl(ofClass(hasAnyName(
+ "::std::optional", "::absl::optional", "::folly::Optional")))));
+}
+
Matcher<Stmt> isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
diff --git a/nullability/pointer_nullability_matchers.h b/nullability/pointer_nullability_matchers.h
index acd8b9f..739ea88 100644
--- a/nullability/pointer_nullability_matchers.h
+++ b/nullability/pointer_nullability_matchers.h
@@ -46,6 +46,7 @@
ast_matchers::internal::Matcher<Stmt> isPointerReturn();
ast_matchers::internal::Matcher<CXXCtorInitializer> isCtorMemberInitializer();
ast_matchers::internal::Matcher<Stmt> isZeroParamConstMemberCall();
+ast_matchers::internal::Matcher<Stmt> isOptionalOperatorArrowCall();
ast_matchers::internal::Matcher<Stmt> isNonConstMemberCall();
ast_matchers::internal::Matcher<Stmt> isNonConstMemberOperatorCall();
ast_matchers::internal::Matcher<Stmt> isSmartPointerGlValue();
diff --git a/nullability/pointer_nullability_matchers_test.cc b/nullability/pointer_nullability_matchers_test.cc
index 13f924c..6687aeb 100644
--- a/nullability/pointer_nullability_matchers_test.cc
+++ b/nullability/pointer_nullability_matchers_test.cc
@@ -153,5 +153,62 @@
isSmartPointerBoolConversionCall()));
}
+TEST(PointerNullabilityTest, MatchOptionalOperatorArrowCall) {
+ llvm::StringRef Input(R"cc(
+ namespace std {
+ template <class T>
+ struct optional {
+ T* operator->();
+ T& operator*();
+ };
+ } // namespace std
+
+ namespace absl {
+ template <class T>
+ struct optional {
+ T* operator->();
+ };
+ } // namespace absl
+
+ namespace folly {
+ template <class T>
+ struct Optional {
+ T* operator->();
+ };
+ } // namespace folly
+
+ // Lots of libraries that used to define their own optional now make it an
+ // alias for `std::optional`, so we want to support this too.
+ template <class T>
+ using StdOptionalAlias = std::optional<T>;
+
+ template <class T>
+ struct optional {
+ T* operator->();
+ };
+
+ struct S {
+ int i;
+ };
+ )cc");
+
+ EXPECT_TRUE(matches(Input, "void target(std::optional<S> o){ o->i; }",
+ isOptionalOperatorArrowCall()));
+ EXPECT_TRUE(matches(Input, "void target(absl::optional<S> o){ o->i; }",
+ isOptionalOperatorArrowCall()));
+ EXPECT_TRUE(matches(Input, "void target(folly::Optional<S> o){ o->i; }",
+ isOptionalOperatorArrowCall()));
+ EXPECT_TRUE(matches(Input, "void target(StdOptionalAlias<S> o){ o->i; }",
+ isOptionalOperatorArrowCall()));
+
+ // Not one of the supported optional classes.
+ EXPECT_FALSE(matches(Input, "void target(optional<S> o){ o->i; }",
+ isOptionalOperatorArrowCall()));
+
+ // Not `operator->`.
+ EXPECT_FALSE(matches(Input, "void target(std::optional<S> o){ *o; }",
+ isOptionalOperatorArrowCall()));
+}
+
} // namespace
} // namespace clang::tidy::nullability
diff --git a/nullability/test/function_calls.cc b/nullability/test/function_calls.cc
index 4c1d926..b8ef90e 100644
--- a/nullability/test/function_calls.cc
+++ b/nullability/test/function_calls.cc
@@ -861,6 +861,35 @@
)cc"));
}
+TEST(PointerNullabilityTest, OptionalOperatorArrowCall) {
+ // Check that repeated accesses to a pointer behind an optional are considered
+ // to yield the same pointer -- but only if the optional is not modified in
+ // the meantime.
+ EXPECT_TRUE(checkDiagnostics(R"cc(
+ namespace std {
+ template <class T>
+ struct optional {
+ bool has_value() const;
+ T* operator->();
+ };
+ } // namespace std
+
+ struct S {
+ int* _Nullable p;
+ };
+
+ void target(std::optional<S> opt1, std::optional<S> opt2) {
+ if (!opt1.has_value() || !opt2.has_value()) return;
+ *opt1->p; // [[unsafe]]
+ if (opt1->p != nullptr) {
+ *opt1->p;
+ opt1 = opt2;
+ *opt1->p; // [[unsafe]]
+ }
+ }
+ )cc"));
+}
+
TEST(PointerNullabilityTest, FieldUndefinedValue) {
EXPECT_TRUE(checkDiagnostics(R"cc(
struct C {
