Generalize support for const member functions
Treat consecutive const member function calls as idempotent, even when they aren't simple accessors/the implementation is not visible.
PiperOrigin-RevId: 580596960
Change-Id: Id75da4b24ccf48e63b6d4e8060dbaf261c690169
diff --git a/nullability/pointer_nullability_analysis.cc b/nullability/pointer_nullability_analysis.cc
index b02582d..be1e0f8 100644
--- a/nullability/pointer_nullability_analysis.cc
+++ b/nullability/pointer_nullability_analysis.cc
@@ -537,6 +537,21 @@
}
}
+void transferFlowSensitiveConstMemberCall(
+ const CXXMemberCallExpr *MCE, const MatchFinder::MatchResult &Result,
+ TransferState<PointerNullabilityLattice> &State) {
+ if (!isSupportedPointerType(MCE->getType())) return;
+ dataflow::RecordStorageLocation *RecordLoc =
+ dataflow::getImplicitObjectLocation(*MCE, State.Env);
+ if (RecordLoc == nullptr) return;
+ PointerValue *PointerVal =
+ State.Lattice.getConstMethodReturnValue(*RecordLoc, MCE, State.Env);
+ if (PointerVal) {
+ State.Env.setValue(*MCE, *PointerVal);
+ initPointerFromTypeNullability(*PointerVal, MCE, State);
+ }
+}
+
void transferFlowSensitiveNonConstMemberCall(
const CXXMemberCallExpr *MCE, const MatchFinder::MatchResult &Result,
TransferState<PointerNullabilityLattice> &State) {
@@ -549,6 +564,7 @@
Value *V = State.Env.createValue(Field->getType());
State.Env.setValue(*FieldLoc, *V);
}
+ State.Lattice.clearConstMethodReturnValues(*RecordLoc);
}
// The nullability of the Expr itself still needs to be handled.
transferFlowSensitiveCallExpr(MCE, Result, State);
@@ -868,6 +884,8 @@
transferFlowSensitiveNullPointer)
.CaseOfCFGStmt<CXXMemberCallExpr>(isSupportedPointerAccessorCall(),
transferFlowSensitiveAccessorCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
+ transferFlowSensitiveConstMemberCall)
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
transferFlowSensitiveNonConstMemberCall)
.CaseOfCFGStmt<CallExpr>(isCallExpr(), transferFlowSensitiveCallExpr)
diff --git a/nullability/pointer_nullability_lattice.h b/nullability/pointer_nullability_lattice.h
index 932fbb3..6457ca5 100644
--- a/nullability/pointer_nullability_lattice.h
+++ b/nullability/pointer_nullability_lattice.h
@@ -12,9 +12,13 @@
#include "absl/container/flat_hash_map.h"
#include "absl/log/check.h"
#include "nullability/type_nullability.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/AST/Expr.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/FunctionExtras.h"
@@ -62,6 +66,25 @@
return Iterator->second;
}
+ // 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.
+ dataflow::PointerValue *getConstMethodReturnValue(
+ const dataflow::RecordStorageLocation &RecordLoc,
+ const CXXMemberCallExpr *MCE, dataflow::Environment &Env) {
+ auto &ObjMap = ConstMethodReturnValues[&RecordLoc];
+ auto it = ObjMap.find(MCE->getMethodDecl());
+ if (it != ObjMap.end()) return it->second;
+ auto *PV = cast<dataflow::PointerValue>(Env.createValue(MCE->getType()));
+ ObjMap.insert({MCE->getMethodDecl(), PV});
+ return PV;
+ }
+
+ void clearConstMethodReturnValues(
+ const dataflow::RecordStorageLocation &RecordLoc) {
+ ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
// If nullability for the decl D has been overridden, patch N to reflect it.
// (N is the nullability of an access to D).
void overrideNullabilityFromDecl(const Decl *D, TypeNullability &N) const;
@@ -69,13 +92,29 @@
bool operator==(const PointerNullabilityLattice &Other) const { return true; }
dataflow::LatticeJoinEffect join(const PointerNullabilityLattice &Other) {
- return dataflow::LatticeJoinEffect::Unchanged;
+ if (ConstMethodReturnValues.empty())
+ return dataflow::LatticeJoinEffect::Unchanged;
+ // Conservatively, just clear the `ConstMethodReturnValues` map entirely.
+ // This means that we can't check the return value from a const method
+ // before a join, then call the method again to use the pointer after the
+ // join -- we'll get a false positive in this case.
+ // TODO(b/309667920): Add code to actually join the maps if it turns out
+ // these types of false positives are common.
+ ConstMethodReturnValues.clear();
+ return dataflow::LatticeJoinEffect::Changed;
}
private:
// Owned by the PointerNullabilityAnalysis object, shared by all lattice
// elements within one analysis run.
NonFlowSensitiveState &NFS;
+
+ // Maps a record storage location and const method to the value to return
+ // from that const method.
+ llvm::SmallDenseMap<
+ const dataflow::RecordStorageLocation *,
+ llvm::SmallDenseMap<const CXXMethodDecl *, dataflow::PointerValue *>>
+ ConstMethodReturnValues;
};
inline std::ostream &operator<<(std::ostream &OS,
diff --git a/nullability/pointer_nullability_matchers.cc b/nullability/pointer_nullability_matchers.cc
index 00b1720..fc3d74c 100644
--- a/nullability/pointer_nullability_matchers.cc
+++ b/nullability/pointer_nullability_matchers.cc
@@ -39,6 +39,7 @@
using ast_matchers::isConst;
using ast_matchers::isMemberInitializer;
using ast_matchers::memberExpr;
+using ast_matchers::parameterCountIs;
using ast_matchers::returnStmt;
using ast_matchers::statementCountIs;
using ast_matchers::unaryOperator;
@@ -75,6 +76,11 @@
return cxxCtorInitializer(isMemberInitializer());
}
+Matcher<Stmt> isZeroParamConstMemberCall() {
+ return cxxMemberCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+}
+
Matcher<Stmt> isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
diff --git a/nullability/pointer_nullability_matchers.h b/nullability/pointer_nullability_matchers.h
index 4e67076..e6641e1 100644
--- a/nullability/pointer_nullability_matchers.h
+++ b/nullability/pointer_nullability_matchers.h
@@ -30,6 +30,7 @@
ast_matchers::internal::Matcher<Stmt> isPointerReturn();
ast_matchers::internal::Matcher<Stmt> isConstructExpr();
ast_matchers::internal::Matcher<CXXCtorInitializer> isCtorMemberInitializer();
+ast_matchers::internal::Matcher<Stmt> isZeroParamConstMemberCall();
ast_matchers::internal::Matcher<Stmt> isNonConstMemberCall();
ast_matchers::internal::Matcher<Stmt> isSupportedPointerAccessorCall();
diff --git a/nullability/pointer_nullability_matchers_test.cc b/nullability/pointer_nullability_matchers_test.cc
index 5ae437b..77c0717 100644
--- a/nullability/pointer_nullability_matchers_test.cc
+++ b/nullability/pointer_nullability_matchers_test.cc
@@ -62,5 +62,28 @@
isSupportedPointerAccessorCall()));
}
+TEST(PointerNullabilityTest, MatchConstMemberFunctions) {
+ llvm::StringRef Input(R"cc(
+ class C {
+ public:
+ int *_Nullable get() const;
+ int *_Nullable get(int i) const;
+ int *_Nullable get_with_default_arg(int i = 0) const;
+ int *_Nullable get_nonconst();
+ };
+ C foo() { return C(); }
+ )cc");
+ EXPECT_TRUE(matches(Input, "void target(){ C().get(); }",
+ isZeroParamConstMemberCall()));
+ EXPECT_TRUE(matches(Input, "void target(){ foo().get(); }",
+ isZeroParamConstMemberCall()));
+
+ EXPECT_FALSE(matches(Input, "void target(){ C().get(0); }",
+ isZeroParamConstMemberCall()));
+ EXPECT_FALSE(matches(Input, "void target(){ C().get_with_default_arg(); }",
+ isZeroParamConstMemberCall()));
+ EXPECT_FALSE(matches(Input, "void target(){ C().get_nonconst(); }",
+ isZeroParamConstMemberCall()));
+}
} // namespace
} // namespace clang::tidy::nullability
diff --git a/nullability/test/function_calls.cc b/nullability/test/function_calls.cc
index 51a5c3b..aadaba8 100644
--- a/nullability/test/function_calls.cc
+++ b/nullability/test/function_calls.cc
@@ -678,6 +678,60 @@
)cc"));
}
+TEST(PointerNullabilityTest, ConstMethodNoImpl) {
+ EXPECT_TRUE(checkDiagnostics(R"cc(
+ struct C {
+ int *_Nullable property() const;
+ void may_mutate();
+ };
+ void target() {
+ C obj;
+ if (obj.property() != nullptr) {
+ obj.may_mutate();
+ *obj.property(); // [[unsafe]]
+ };
+ if (obj.property() != nullptr) *obj.property();
+ }
+ )cc"));
+}
+
+TEST(PointerNullabilityTest, ConstMethodEarlyReturn) {
+ EXPECT_TRUE(checkDiagnostics(R"cc(
+ struct C {
+ int *_Nullable property() const;
+ };
+ void target() {
+ C c;
+ if (!c.property()) return;
+ // No false positive in this case, as there is no join.
+ *c.property();
+ }
+ )cc"));
+}
+
+TEST(PointerNullabilityTest, ConstMethodEarlyReturnWithConditional) {
+ EXPECT_TRUE(checkDiagnostics(R"cc(
+ struct C {
+ int *_Nullable property() const;
+ };
+ bool cond();
+ void some_operation(int);
+ void target() {
+ C c;
+ if (!c.property()) return;
+ if (cond()) {
+ some_operation(1);
+ } else {
+ some_operation(2);
+ }
+ // TODO(b/309667920): False positive below due to clearing the const
+ // method return values on join. If this is a pattern that occurs
+ // frequently in real code, we need to fix this.
+ *c.property(); // [[unsafe]]
+ }
+ )cc"));
+}
+
TEST(PointerNullabilityTest, FieldUndefinedValue) {
EXPECT_TRUE(checkDiagnostics(R"cc(
struct C {
