Skip to content

Commit 422325a

Browse files
Googlercopybara-github
Googler
authored and
copybara-github
committed
Support for more smart-pointer-like class caching (classes with * and -> returning the expected type)
Use the smart pointer caching utilities (matcher and transfer function) from clang::dataflow (llvm/llvm-project#120249) to handle some more accessor caching cases. This is similar to the `transferValue_OptionalOperatorArrowCall` in that it can treat the non-const overload as const, but it also allows mixing the operator * vs -> vs value() between the check and the deref. It also handles more classes like absl::StatusOr. Change the test stub in `OptionalOperatorArrowCall` a bit, because the heuristic for matching smart pointers classes requires `*` and `->` (which the real version of std::optional would have). PiperOrigin-RevId: 718973436 Change-Id: I1bdf3061ce5658e38e7f6b0225bdbed512ccc301
1 parent 65afc3b commit 422325a

File tree

6 files changed

+100
-89
lines changed

6 files changed

+100
-89
lines changed

‎bazel/llvm.bzl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def _llvm_loader_repository(repository_ctx):
5353
executable = False,
5454
)
5555

56-
LLVM_COMMIT_SHA = "c6e7b4a61ab8718d9ac9d1d32f7d2d0cd0b19a7f"
56+
LLVM_COMMIT_SHA = "4f26edd5e9eb3b6cea19e15ca8fb2c8416b82fa8"
5757

5858
def llvm_loader_repository_dependencies():
5959
# This *declares* the dependency, but it won't actually be *downloaded* unless it's used.

‎nullability/pointer_nullability_analysis.cc

Lines changed: 56 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
#include "clang/Analysis/FlowSensitive/Formula.h"
4141
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
4242
#include "clang/Analysis/FlowSensitive/RecordOps.h"
43+
#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
4344
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
4445
#include "clang/Analysis/FlowSensitive/Value.h"
4546
#include "clang/Basic/Builtins.h"
@@ -1245,6 +1246,19 @@ void transferValue_AccessorCall(
12451246
}
12461247
}
12471248

1249+
std::function<void(StorageLocation &)>
1250+
initCallbackForStorageLocationIfSmartPointer(absl::Nonnull<const CallExpr *> CE,
1251+
dataflow::Environment &Env) {
1252+
if (!isSupportedSmartPointerType(CE->getType()))
1253+
return [](StorageLocation &Loc) {};
1254+
return [CE, &Env](StorageLocation &Loc) {
1255+
setSmartPointerValue(cast<RecordStorageLocation>(Loc),
1256+
cast<PointerValue>(Env.createValue(
1257+
underlyingRawPointerType(CE->getType()))),
1258+
Env);
1259+
};
1260+
}
1261+
12481262
void handleConstMemberCall(
12491263
absl::Nonnull<const CallExpr *> CE,
12501264
absl::Nullable<dataflow::RecordStorageLocation *> RecordLoc,
@@ -1257,13 +1271,8 @@ void handleConstMemberCall(
12571271
if (RecordLoc != nullptr && isSupportedSmartPointerType(CE->getType())) {
12581272
StorageLocation *Loc =
12591273
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
1260-
*RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
1261-
setSmartPointerValue(
1262-
cast<RecordStorageLocation>(Loc),
1263-
cast<PointerValue>(State.Env.createValue(
1264-
underlyingRawPointerType(CE->getType()))),
1265-
State.Env);
1266-
});
1274+
*RecordLoc, CE, State.Env,
1275+
initCallbackForStorageLocationIfSmartPointer(CE, State.Env));
12671276
if (Loc == nullptr) return;
12681277

12691278
if (CE->isGLValue()) {
@@ -1316,18 +1325,6 @@ void transferValue_ConstMemberOperatorCall(
13161325
handleConstMemberCall(OCE, RecordLoc, Result, State);
13171326
}
13181327

1319-
void transferValue_OptionalOperatorArrowCall(
1320-
absl::Nonnull<const CXXOperatorCallExpr *> OCE,
1321-
const MatchFinder::MatchResult &Result,
1322-
TransferState<PointerNullabilityLattice> &State) {
1323-
auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
1324-
State.Env.getStorageLocation(*OCE->getArg(0)));
1325-
// `optional::operator->` isn't necessarily const, but it behaves the way we
1326-
// model "const member calls": It always returns the same pointer if the
1327-
// optional wasn't mutated in the meantime.
1328-
handleConstMemberCall(OCE, RecordLoc, Result, State);
1329-
}
1330-
13311328
void handleNonConstMemberCall(absl::Nonnull<const CallExpr *> CE,
13321329
dataflow::RecordStorageLocation *RecordLoc,
13331330
const MatchFinder::MatchResult &Result,
@@ -1841,13 +1838,50 @@ auto buildValueTransferer() {
18411838
transferValue_WeakPtrLockCall)
18421839
.CaseOfCFGStmt<CXXMemberCallExpr>(isSupportedPointerAccessorCall(),
18431840
transferValue_AccessorCall)
1841+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
1842+
dataflow::isSmartPointerLikeOperatorStar(),
1843+
[](const CXXOperatorCallExpr *CE,
1844+
const MatchFinder::MatchResult &Result,
1845+
TransferState<PointerNullabilityLattice> &State) {
1846+
auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
1847+
State.Env.getStorageLocation(*CE->getArg(0)));
1848+
dataflow::transferSmartPointerLikeCachedDeref(
1849+
CE, RecordLoc, State,
1850+
initCallbackForStorageLocationIfSmartPointer(CE, State.Env));
1851+
})
1852+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
1853+
dataflow::isSmartPointerLikeOperatorArrow(),
1854+
[](const CXXOperatorCallExpr *CE,
1855+
const MatchFinder::MatchResult &Result,
1856+
TransferState<PointerNullabilityLattice> &State) {
1857+
auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
1858+
State.Env.getStorageLocation(*CE->getArg(0)));
1859+
dataflow::transferSmartPointerLikeCachedGet(
1860+
CE, RecordLoc, State,
1861+
initCallbackForStorageLocationIfSmartPointer(CE, State.Env));
1862+
})
1863+
.CaseOfCFGStmt<CXXMemberCallExpr>(
1864+
dataflow::isSmartPointerLikeValueMethodCall(),
1865+
[](const CXXMemberCallExpr *CE,
1866+
const MatchFinder::MatchResult &Result,
1867+
TransferState<PointerNullabilityLattice> &State) {
1868+
dataflow::transferSmartPointerLikeCachedDeref(
1869+
CE, getImplicitObjectLocation(*CE, State.Env), State,
1870+
initCallbackForStorageLocationIfSmartPointer(CE, State.Env));
1871+
})
1872+
.CaseOfCFGStmt<CXXMemberCallExpr>(
1873+
dataflow::isSmartPointerLikeGetMethodCall(),
1874+
[](const CXXMemberCallExpr *CE,
1875+
const MatchFinder::MatchResult &Result,
1876+
TransferState<PointerNullabilityLattice> &State) {
1877+
dataflow::transferSmartPointerLikeCachedGet(
1878+
CE, getImplicitObjectLocation(*CE, State.Env), State,
1879+
initCallbackForStorageLocationIfSmartPointer(CE, State.Env));
1880+
})
18441881
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
18451882
transferValue_ConstMemberCall)
18461883
.CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
18471884
transferValue_ConstMemberOperatorCall)
1848-
.CaseOfCFGStmt<CXXOperatorCallExpr>(
1849-
isOptionalOperatorArrowCall(),
1850-
transferValue_OptionalOperatorArrowCall)
18511885
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
18521886
transferValue_NonConstMemberCall)
18531887
.CaseOfCFGStmt<CXXOperatorCallExpr>(

‎nullability/pointer_nullability_matchers.cc

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,6 @@ Matcher<Stmt> isZeroParamConstMemberOperatorCall() {
118118
callee(cxxMethodDecl(parameterCountIs(0), isConst())));
119119
}
120120

121-
Matcher<Stmt> isOptionalOperatorArrowCall() {
122-
return cxxOperatorCallExpr(
123-
hasOverloadedOperatorName("->"),
124-
callee(cxxMethodDecl(ofClass(hasAnyName(
125-
"::std::optional", "::absl::optional", "::folly::Optional")))));
126-
}
127-
128121
Matcher<Stmt> isNonConstMemberCall() {
129122
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
130123
}

‎nullability/pointer_nullability_matchers.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ ast_matchers::internal::Matcher<Stmt> isPointerReturn();
5151
ast_matchers::internal::Matcher<CXXCtorInitializer> isCtorMemberInitializer();
5252
ast_matchers::internal::Matcher<Stmt> isZeroParamConstMemberCall();
5353
ast_matchers::internal::Matcher<Stmt> isZeroParamConstMemberOperatorCall();
54-
ast_matchers::internal::Matcher<Stmt> isOptionalOperatorArrowCall();
5554
ast_matchers::internal::Matcher<Stmt> isNonConstMemberCall();
5655
ast_matchers::internal::Matcher<Stmt> isNonConstMemberOperatorCall();
5756
ast_matchers::internal::Matcher<Stmt> isSmartPointerArrowMemberExpr();

‎nullability/pointer_nullability_matchers_test.cc

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -159,62 +159,5 @@ TEST(PointerNullabilityTest, MatchSmartPointerBoolConversionCall) {
159159
isSmartPointerBoolConversionCall()));
160160
}
161161

162-
TEST(PointerNullabilityTest, MatchOptionalOperatorArrowCall) {
163-
llvm::StringRef Input(R"cc(
164-
namespace std {
165-
template <class T>
166-
struct optional {
167-
T* operator->();
168-
T& operator*();
169-
};
170-
} // namespace std
171-
172-
namespace absl {
173-
template <class T>
174-
struct optional {
175-
T* operator->();
176-
};
177-
} // namespace absl
178-
179-
namespace folly {
180-
template <class T>
181-
struct Optional {
182-
T* operator->();
183-
};
184-
} // namespace folly
185-
186-
// Lots of libraries that used to define their own optional now make it an
187-
// alias for `std::optional`, so we want to support this too.
188-
template <class T>
189-
using StdOptionalAlias = std::optional<T>;
190-
191-
template <class T>
192-
struct optional {
193-
T* operator->();
194-
};
195-
196-
struct S {
197-
int i;
198-
};
199-
)cc");
200-
201-
EXPECT_TRUE(matches(Input, "void target(std::optional<S> o){ o->i; }",
202-
isOptionalOperatorArrowCall()));
203-
EXPECT_TRUE(matches(Input, "void target(absl::optional<S> o){ o->i; }",
204-
isOptionalOperatorArrowCall()));
205-
EXPECT_TRUE(matches(Input, "void target(folly::Optional<S> o){ o->i; }",
206-
isOptionalOperatorArrowCall()));
207-
EXPECT_TRUE(matches(Input, "void target(StdOptionalAlias<S> o){ o->i; }",
208-
isOptionalOperatorArrowCall()));
209-
210-
// Not one of the supported optional classes.
211-
EXPECT_FALSE(matches(Input, "void target(optional<S> o){ o->i; }",
212-
isOptionalOperatorArrowCall()));
213-
214-
// Not `operator->`.
215-
EXPECT_FALSE(matches(Input, "void target(std::optional<S> o){ *o; }",
216-
isOptionalOperatorArrowCall()));
217-
}
218-
219162
} // namespace
220163
} // namespace clang::tidy::nullability

‎nullability/test/function_calls.cc

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1106,7 +1106,7 @@ TEST(PointerNullabilityTest, NonConstMethodClearsPointerMembersInExpr) {
11061106
)cc"));
11071107
}
11081108

1109-
TEST(PointerNullabilityTest, OptionalOperatorArrowCall) {
1109+
TEST(PointerNullabilityTest, OptionalOperatorArrowAndStarCall) {
11101110
// Check that repeated accesses to a pointer behind an optional are considered
11111111
// to yield the same pointer -- but only if the optional is not modified in
11121112
// the meantime.
@@ -1116,6 +1116,9 @@ TEST(PointerNullabilityTest, OptionalOperatorArrowCall) {
11161116
struct optional {
11171117
bool has_value() const;
11181118
T* operator->();
1119+
const T* operator->() const;
1120+
const T& operator*() const;
1121+
const T& value() const;
11191122
};
11201123
} // namespace std
11211124
@@ -1128,13 +1131,52 @@ TEST(PointerNullabilityTest, OptionalOperatorArrowCall) {
11281131
*opt1->p; // [[unsafe]]
11291132
if (opt1->p != nullptr) {
11301133
*opt1->p;
1134+
*((*opt1).p);
1135+
*(opt1.value().p);
11311136
opt1 = opt2;
11321137
*opt1->p; // [[unsafe]]
11331138
}
11341139
}
11351140
)cc"));
11361141
}
11371142

1143+
TEST(PointerNullabilityTest, StatusOrOperatorArrowAndStarCall) {
1144+
// Check that repeated accesses to a pointer behind a StatusOr (or similar
1145+
// smart pointer-like class) are considered to yield the same pointer --
1146+
// but only if it is not modified in the meantime.
1147+
EXPECT_TRUE(checkDiagnostics(R"cc(
1148+
namespace absl {
1149+
template <typename T>
1150+
class StatusOr {
1151+
public:
1152+
bool ok() const;
1153+
const T& operator*() const&;
1154+
T& operator*() &;
1155+
const T* operator->() const;
1156+
T* operator->();
1157+
const T& value() const;
1158+
T& value();
1159+
};
1160+
} // namespace absl
1161+
1162+
struct S {
1163+
int* _Nullable p;
1164+
};
1165+
1166+
void target(absl::StatusOr<S> sor1, absl::StatusOr<S> sor2) {
1167+
if (!sor1.ok() || !sor2.ok()) return;
1168+
*sor1->p; // [[unsafe]]
1169+
if (sor1->p != nullptr) {
1170+
*sor1->p;
1171+
*((*sor1).p);
1172+
*(sor1.value().p);
1173+
sor1 = sor2;
1174+
*sor1->p; // [[unsafe]]
1175+
}
1176+
}
1177+
)cc"));
1178+
}
1179+
11381180
TEST(PointerNullabilityTest, FieldUndefinedValue) {
11391181
EXPECT_TRUE(checkDiagnostics(R"cc(
11401182
struct C {

0 commit comments

Comments
 (0)