Handle accessors that directly return a pointer member variable
This is to handle the common pattern where the return value of an accessor is tested to see if it is non-null and, if so, the accessor called again to dereference the pointer. For example (`get_const()` is annotated as nullable)
```
if (foo.get_const()) {
foo.get_const()->doThing(); // SAFE
}
```
Additionally, non-const member functions are treated as if they mutate the values of any non-static member variables of pointer type.
```
if (foo.get_const()) {
foo.nonconst_method();
foo.get_const()->doThing(); // UNSAFE
}
PiperOrigin-RevId: 574227988
Change-Id: Ie28fd056d576f0e41f01ffe7cbae198f60fe68e1
diff --git a/nullability/BUILD b/nullability/BUILD
index 7b06d42..4ca6a1c 100644
--- a/nullability/BUILD
+++ b/nullability/BUILD
@@ -24,6 +24,7 @@
hdrs = ["pointer_nullability_matchers.h"],
visibility = [
"//nullability/inference:__pkg__",
+ "//nullability/test:__pkg__",
],
deps = [
":type_nullability",
@@ -32,6 +33,19 @@
],
)
+cc_test(
+ name = "pointer_nullability_matchers_test",
+ srcs = ["pointer_nullability_matchers_test.cc"],
+ deps = [
+ ":pointer_nullability_matchers",
+ "@llvm-project//clang:ast_matchers",
+ "@llvm-project//clang:testing",
+ "@llvm-project//llvm:Support",
+ "@llvm-project//third-party/unittest:gtest",
+ "@llvm-project//third-party/unittest:gtest_main",
+ ],
+)
+
cc_library(
name = "pointer_nullability_analysis",
srcs = ["pointer_nullability_analysis.cc"],
diff --git a/nullability/pointer_nullability_analysis.cc b/nullability/pointer_nullability_analysis.cc
index 2624727..adcd9c8 100644
--- a/nullability/pointer_nullability_analysis.cc
+++ b/nullability/pointer_nullability_analysis.cc
@@ -33,6 +33,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Basic/Specifiers.h"
#include "llvm/Support/Debug.h"
+#include "llvm/Support/raw_ostream.h"
namespace clang::tidy::nullability {
@@ -461,6 +462,35 @@
FuncDecl->getParamDecl(i)->getType());
}
}
+void transferFlowSensitiveAccessorCall(
+ const CXXMemberCallExpr *MCE, const MatchFinder::MatchResult &Result,
+ TransferState<PointerNullabilityLattice> &State) {
+ auto *member = Result.Nodes.getNodeAs<clang::ValueDecl>("member-decl");
+ dataflow::RecordStorageLocation *RecordLoc =
+ dataflow::getImplicitObjectLocation(*MCE, State.Env);
+ StorageLocation *Loc = RecordLoc->getChild(*member);
+ if (auto *PointerVal = dyn_cast<PointerValue>(State.Env.getValue(*Loc))) {
+ State.Env.setValue(*MCE, *PointerVal);
+ initPointerFromTypeNullability(*PointerVal, MCE, State);
+ }
+}
+
+void transferFlowSensitiveNonConstMemberCall(
+ const CXXMemberCallExpr *MCE, const MatchFinder::MatchResult &Result,
+ TransferState<PointerNullabilityLattice> &State) {
+ // When a non-const member function is called, reset all pointer-type fields
+ // of the implicit object.
+ if (dataflow::RecordStorageLocation *RecordLoc =
+ dataflow::getImplicitObjectLocation(*MCE, State.Env)) {
+ for (const auto [Field, FieldLoc] : RecordLoc->children()) {
+ if (!isSupportedPointerType(Field->getType())) continue;
+ Value *V = State.Env.createValue(Field->getType());
+ State.Env.setValue(*FieldLoc, *V);
+ }
+ }
+ // The nullability of the Expr itself still needs to be handled.
+ transferFlowSensitiveCallExpr(MCE, Result, State);
+}
// If nullability for the decl D has been overridden, patch N to reflect it.
// (N is the nullability of an access to D).
@@ -783,6 +813,10 @@
// pointers to the non-flow-sensitive part of the analysis.
.CaseOfCFGStmt<Expr>(isNullPointerLiteral(),
transferFlowSensitiveNullPointer)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isSupportedPointerAccessorCall(),
+ transferFlowSensitiveAccessorCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
+ transferFlowSensitiveNonConstMemberCall)
.CaseOfCFGStmt<CallExpr>(isCallExpr(), transferFlowSensitiveCallExpr)
.CaseOfCFGStmt<Expr>(isPointerExpr(), transferFlowSensitivePointer)
// Handles comparison between 2 pointers.
diff --git a/nullability/pointer_nullability_matchers.cc b/nullability/pointer_nullability_matchers.cc
index be9ca0f..00b1720 100644
--- a/nullability/pointer_nullability_matchers.cc
+++ b/nullability/pointer_nullability_matchers.cc
@@ -13,24 +13,36 @@
using ast_matchers::anyOf;
using ast_matchers::binaryOperator;
+using ast_matchers::callee;
using ast_matchers::callExpr;
+using ast_matchers::compoundStmt;
using ast_matchers::cxxConstructExpr;
using ast_matchers::cxxCtorInitializer;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
using ast_matchers::cxxThisExpr;
+using ast_matchers::decl;
using ast_matchers::expr;
+using ast_matchers::has;
using ast_matchers::hasAnyOperatorName;
+using ast_matchers::hasBody;
using ast_matchers::hasCastKind;
+using ast_matchers::hasDeclaration;
using ast_matchers::hasOperands;
using ast_matchers::hasOperatorName;
using ast_matchers::hasReturnValue;
using ast_matchers::hasType;
using ast_matchers::hasUnaryOperand;
+using ast_matchers::ignoringParenImpCasts;
using ast_matchers::implicitCastExpr;
using ast_matchers::isArrow;
+using ast_matchers::isConst;
using ast_matchers::isMemberInitializer;
using ast_matchers::memberExpr;
using ast_matchers::returnStmt;
+using ast_matchers::statementCountIs;
using ast_matchers::unaryOperator;
+using ast_matchers::unless;
using ast_matchers::internal::Matcher;
Matcher<Stmt> isPointerExpr() { return expr(hasType(isSupportedPointer())); }
@@ -63,4 +75,19 @@
return cxxCtorInitializer(isMemberInitializer());
}
+Matcher<Stmt> isNonConstMemberCall() {
+ return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
+Matcher<Stmt> isSupportedPointerAccessorCall() {
+ return cxxMemberCallExpr(callee(cxxMethodDecl(hasBody(compoundStmt(
+ statementCountIs(1),
+ has(returnStmt(has(implicitCastExpr(
+ hasCastKind(CK_LValueToRValue),
+ has(ignoringParenImpCasts(
+ memberExpr(has(ignoringParenImpCasts(cxxThisExpr())),
+ hasType(isSupportedPointer()),
+ hasDeclaration(decl().bind("member-decl"))))))))))))));
+}
+
} // namespace clang::tidy::nullability
diff --git a/nullability/pointer_nullability_matchers.h b/nullability/pointer_nullability_matchers.h
index ea0ed26..4e67076 100644
--- a/nullability/pointer_nullability_matchers.h
+++ b/nullability/pointer_nullability_matchers.h
@@ -30,6 +30,8 @@
ast_matchers::internal::Matcher<Stmt> isPointerReturn();
ast_matchers::internal::Matcher<Stmt> isConstructExpr();
ast_matchers::internal::Matcher<CXXCtorInitializer> isCtorMemberInitializer();
+ast_matchers::internal::Matcher<Stmt> isNonConstMemberCall();
+ast_matchers::internal::Matcher<Stmt> isSupportedPointerAccessorCall();
} // namespace nullability
} // namespace tidy
diff --git a/nullability/pointer_nullability_matchers_test.cc b/nullability/pointer_nullability_matchers_test.cc
new file mode 100644
index 0000000..075b5ac
--- /dev/null
+++ b/nullability/pointer_nullability_matchers_test.cc
@@ -0,0 +1,63 @@
+// Part of the Crubit project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "nullability/pointer_nullability_matchers.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Testing/TestAST.h"
+#include "llvm/ADT/StringRef.h"
+#include "third_party/llvm/llvm-project/third-party/unittest/googletest/include/gtest/gtest.h"
+
+namespace clang::tidy::nullability {
+namespace {
+
+using ast_matchers::match;
+
+template <typename MatcherT>
+bool matches(llvm::StringRef base_input, llvm::StringRef test_input,
+ MatcherT Matcher) {
+ TestAST InputAST(base_input.str() + test_input.str());
+ return !match(Matcher, InputAST.context()).empty();
+}
+
+TEST(PointerNullabilityTest, MatchMemberFunctions) {
+ llvm::StringRef Input(R"cc(
+ struct DummyStruct {
+ int *p;
+ };
+ class C {
+ public:
+ int *_Nullable get() const { return x; }
+ int *_Nullable get_member_in_parens() const { return (x); }
+ int *_Nullable get_this_in_parens() const { return (this)->x; }
+ int *_Nullable get(int i) const { return x; }
+ int *_Nullable get_nonconst() { return x; }
+ int *_Nullable get_external() { return ds.p; }
+ void mutate(){};
+
+ private:
+ int *x;
+ DummyStruct ds;
+ };
+ )cc");
+
+ EXPECT_TRUE(matches(Input, "void target(){ C().get(); }",
+ isSupportedPointerAccessorCall()));
+ EXPECT_TRUE(matches(Input, "void target(){ C().get_member_in_parens(); }",
+ isSupportedPointerAccessorCall()));
+ EXPECT_TRUE(matches(Input, "void target(){ C().get_this_in_parens(); }",
+ isSupportedPointerAccessorCall()));
+ EXPECT_TRUE(matches(Input, "void target(){ C().get(0); }",
+ isSupportedPointerAccessorCall()));
+ EXPECT_TRUE(matches(Input, "void target(){ C().get_nonconst(); }",
+ isSupportedPointerAccessorCall()));
+
+ EXPECT_FALSE(matches(Input, "void target(){ C().mutate(); }",
+ isSupportedPointerAccessorCall()));
+ EXPECT_FALSE(matches(Input, "void target(){ C().get_external(); }",
+ isSupportedPointerAccessorCall()));
+}
+
+} // namespace
+} // namespace clang::tidy::nullability
