Skip to content

Commit 9648b1c

Browse files
martinboehmemartinboehme
committed
[clang][dataflow] Make optional checker work for types derived from optional.
`llvm::MaybeAlign` does this, for example. It's not an option to simply ignore these derived classes because they get cast back to the optional classes (for example, simply when calling the optional member functions), and our transfer functions will then run on those optional classes and therefore require them to be properly initialized.
1 parent c089fa5 commit 9648b1c

File tree

2 files changed

+111
-67
lines changed

2 files changed

+111
-67
lines changed

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines changed: 81 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -64,38 +64,53 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) {
6464
return false;
6565
}
6666

67+
static const CXXRecordDecl *getOptionalBaseClass(const CXXRecordDecl *RD) {
68+
if (RD == nullptr)
69+
return nullptr;
70+
if (hasOptionalClassName(*RD))
71+
return RD;
72+
73+
if (!RD->hasDefinition())
74+
return nullptr;
75+
76+
for (const CXXBaseSpecifier &Base : RD->bases())
77+
if (Base.getAccessSpecifier() == AS_public)
78+
if (const CXXRecordDecl *BaseClass =
79+
getOptionalBaseClass(Base.getType()->getAsCXXRecordDecl()))
80+
return BaseClass;
81+
82+
return nullptr;
83+
}
84+
6785
namespace {
6886

6987
using namespace ::clang::ast_matchers;
7088
using LatticeTransferState = TransferState<NoopLattice>;
7189

72-
AST_MATCHER(CXXRecordDecl, hasOptionalClassNameMatcher) {
73-
return hasOptionalClassName(Node);
90+
AST_MATCHER(CXXRecordDecl, optionalOrDerivedClass) {
91+
return getOptionalBaseClass(&Node) != nullptr;
7492
}
7593

76-
DeclarationMatcher optionalClass() {
77-
return classTemplateSpecializationDecl(
78-
hasOptionalClassNameMatcher(),
79-
hasTemplateArgument(0, refersToType(type().bind("T"))));
80-
}
81-
82-
auto optionalOrAliasType() {
94+
auto desugarsToOptionalOrDerivedType() {
8395
return hasUnqualifiedDesugaredType(
84-
recordType(hasDeclaration(optionalClass())));
96+
recordType(hasDeclaration(cxxRecordDecl(optionalOrDerivedClass()))));
8597
}
8698

87-
/// Matches any of the spellings of the optional types and sugar, aliases, etc.
88-
auto hasOptionalType() { return hasType(optionalOrAliasType()); }
99+
/// Matches any of the spellings of the optional types and sugar, aliases,
100+
/// derived classes, etc.
101+
auto hasOptionalOrDerivedType() {
102+
return hasType(desugarsToOptionalOrDerivedType());
103+
}
89104

90105
auto isOptionalMemberCallWithNameMatcher(
91106
ast_matchers::internal::Matcher<NamedDecl> matcher,
92107
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
93108
auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
94109
: cxxThisExpr());
95110
return cxxMemberCallExpr(
96-
on(expr(Exception,
97-
anyOf(hasOptionalType(),
98-
hasType(pointerType(pointee(optionalOrAliasType())))))),
111+
on(expr(Exception, anyOf(hasOptionalOrDerivedType(),
112+
hasType(pointerType(pointee(
113+
desugarsToOptionalOrDerivedType())))))),
99114
callee(cxxMethodDecl(matcher)));
100115
}
101116

@@ -104,15 +119,15 @@ auto isOptionalOperatorCallWithName(
104119
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
105120
return cxxOperatorCallExpr(
106121
hasOverloadedOperatorName(operator_name),
107-
callee(cxxMethodDecl(ofClass(optionalClass()))),
122+
callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
108123
Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
109124
}
110125

111126
auto isMakeOptionalCall() {
112127
return callExpr(callee(functionDecl(hasAnyName(
113128
"std::make_optional", "base::make_optional",
114129
"absl::make_optional", "folly::make_optional"))),
115-
hasOptionalType());
130+
hasOptionalOrDerivedType());
116131
}
117132

118133
auto nulloptTypeDecl() {
@@ -129,19 +144,19 @@ auto inPlaceClass() {
129144

130145
auto isOptionalNulloptConstructor() {
131146
return cxxConstructExpr(
132-
hasOptionalType(),
147+
hasOptionalOrDerivedType(),
133148
hasDeclaration(cxxConstructorDecl(parameterCountIs(1),
134149
hasParameter(0, hasNulloptType()))));
135150
}
136151

137152
auto isOptionalInPlaceConstructor() {
138-
return cxxConstructExpr(hasOptionalType(),
153+
return cxxConstructExpr(hasOptionalOrDerivedType(),
139154
hasArgument(0, hasType(inPlaceClass())));
140155
}
141156

142157
auto isOptionalValueOrConversionConstructor() {
143158
return cxxConstructExpr(
144-
hasOptionalType(),
159+
hasOptionalOrDerivedType(),
145160
unless(hasDeclaration(
146161
cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor())))),
147162
argumentCountIs(1), hasArgument(0, unless(hasNulloptType())));
@@ -150,28 +165,30 @@ auto isOptionalValueOrConversionConstructor() {
150165
auto isOptionalValueOrConversionAssignment() {
151166
return cxxOperatorCallExpr(
152167
hasOverloadedOperatorName("="),
153-
callee(cxxMethodDecl(ofClass(optionalClass()))),
168+
callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
154169
unless(hasDeclaration(cxxMethodDecl(
155170
anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())))),
156171
argumentCountIs(2), hasArgument(1, unless(hasNulloptType())));
157172
}
158173

159174
auto isOptionalNulloptAssignment() {
160-
return cxxOperatorCallExpr(hasOverloadedOperatorName("="),
161-
callee(cxxMethodDecl(ofClass(optionalClass()))),
162-
argumentCountIs(2),
163-
hasArgument(1, hasNulloptType()));
175+
return cxxOperatorCallExpr(
176+
hasOverloadedOperatorName("="),
177+
callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
178+
argumentCountIs(2), hasArgument(1, hasNulloptType()));
164179
}
165180

166181
auto isStdSwapCall() {
167182
return callExpr(callee(functionDecl(hasName("std::swap"))),
168-
argumentCountIs(2), hasArgument(0, hasOptionalType()),
169-
hasArgument(1, hasOptionalType()));
183+
argumentCountIs(2),
184+
hasArgument(0, hasOptionalOrDerivedType()),
185+
hasArgument(1, hasOptionalOrDerivedType()));
170186
}
171187

172188
auto isStdForwardCall() {
173189
return callExpr(callee(functionDecl(hasName("std::forward"))),
174-
argumentCountIs(1), hasArgument(0, hasOptionalType()));
190+
argumentCountIs(1),
191+
hasArgument(0, hasOptionalOrDerivedType()));
175192
}
176193

177194
constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall";
@@ -181,21 +198,23 @@ auto isValueOrStringEmptyCall() {
181198
return cxxMemberCallExpr(
182199
callee(cxxMethodDecl(hasName("empty"))),
183200
onImplicitObjectArgument(ignoringImplicit(
184-
cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
185-
callee(cxxMethodDecl(hasName("value_or"),
186-
ofClass(optionalClass()))),
187-
hasArgument(0, stringLiteral(hasSize(0))))
201+
cxxMemberCallExpr(
202+
on(expr(unless(cxxThisExpr()))),
203+
callee(cxxMethodDecl(hasName("value_or"),
204+
ofClass(optionalOrDerivedClass()))),
205+
hasArgument(0, stringLiteral(hasSize(0))))
188206
.bind(ValueOrCallID))));
189207
}
190208

191209
auto isValueOrNotEqX() {
192210
auto ComparesToSame = [](ast_matchers::internal::Matcher<Stmt> Arg) {
193211
return hasOperands(
194212
ignoringImplicit(
195-
cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
196-
callee(cxxMethodDecl(hasName("value_or"),
197-
ofClass(optionalClass()))),
198-
hasArgument(0, Arg))
213+
cxxMemberCallExpr(
214+
on(expr(unless(cxxThisExpr()))),
215+
callee(cxxMethodDecl(hasName("value_or"),
216+
ofClass(optionalOrDerivedClass()))),
217+
hasArgument(0, Arg))
199218
.bind(ValueOrCallID)),
200219
ignoringImplicit(Arg));
201220
};
@@ -212,8 +231,9 @@ auto isValueOrNotEqX() {
212231
}
213232

214233
auto isCallReturningOptional() {
215-
return callExpr(hasType(qualType(anyOf(
216-
optionalOrAliasType(), referenceType(pointee(optionalOrAliasType()))))));
234+
return callExpr(hasType(qualType(
235+
anyOf(desugarsToOptionalOrDerivedType(),
236+
referenceType(pointee(desugarsToOptionalOrDerivedType()))))));
217237
}
218238

219239
template <typename L, typename R>
@@ -275,28 +295,23 @@ BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) {
275295
return HasValueVal;
276296
}
277297

278-
/// Returns true if and only if `Type` is an optional type.
279-
bool isOptionalType(QualType Type) {
280-
if (!Type->isRecordType())
281-
return false;
282-
const CXXRecordDecl *D = Type->getAsCXXRecordDecl();
283-
return D != nullptr && hasOptionalClassName(*D);
298+
QualType valueTypeFromOptionalDecl(const CXXRecordDecl &RD) {
299+
auto &CTSD = cast<ClassTemplateSpecializationDecl>(RD);
300+
return CTSD.getTemplateArgs()[0].getAsType();
284301
}
285302

286303
/// Returns the number of optional wrappers in `Type`.
287304
///
288305
/// For example, if `Type` is `optional<optional<int>>`, the result of this
289306
/// function will be 2.
290307
int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
291-
if (!isOptionalType(Type))
308+
const CXXRecordDecl *Optional =
309+
getOptionalBaseClass(Type->getAsCXXRecordDecl());
310+
if (Optional == nullptr)
292311
return 0;
293312
return 1 + countOptionalWrappers(
294313
ASTCtx,
295-
cast<ClassTemplateSpecializationDecl>(Type->getAsRecordDecl())
296-
->getTemplateArgs()
297-
.get(0)
298-
.getAsType()
299-
.getDesugaredType(ASTCtx));
314+
valueTypeFromOptionalDecl(*Optional).getDesugaredType(ASTCtx));
300315
}
301316

302317
StorageLocation *getLocBehindPossiblePointer(const Expr &E,
@@ -623,7 +638,7 @@ ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
623638
if (Options.IgnoreSmartPointerDereference) {
624639
auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr(
625640
anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")),
626-
unless(hasArgument(0, expr(hasOptionalType()))))));
641+
unless(hasArgument(0, expr(hasOptionalOrDerivedType()))))));
627642
return expr(
628643
anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse))));
629644
}
@@ -758,32 +773,35 @@ auto buildTransferMatchSwitch() {
758773

759774
// Comparisons (==, !=):
760775
.CaseOfCFGStmt<CXXOperatorCallExpr>(
761-
isComparisonOperatorCall(hasOptionalType(), hasOptionalType()),
776+
isComparisonOperatorCall(hasOptionalOrDerivedType(),
777+
hasOptionalOrDerivedType()),
762778
transferOptionalAndOptionalCmp)
763779
.CaseOfCFGStmt<CXXOperatorCallExpr>(
764-
isComparisonOperatorCall(hasOptionalType(), hasNulloptType()),
780+
isComparisonOperatorCall(hasOptionalOrDerivedType(),
781+
hasNulloptType()),
765782
[](const clang::CXXOperatorCallExpr *Cmp,
766783
const MatchFinder::MatchResult &, LatticeTransferState &State) {
767784
transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(0), State.Env);
768785
})
769786
.CaseOfCFGStmt<CXXOperatorCallExpr>(
770-
isComparisonOperatorCall(hasNulloptType(), hasOptionalType()),
787+
isComparisonOperatorCall(hasNulloptType(),
788+
hasOptionalOrDerivedType()),
771789
[](const clang::CXXOperatorCallExpr *Cmp,
772790
const MatchFinder::MatchResult &, LatticeTransferState &State) {
773791
transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(1), State.Env);
774792
})
775793
.CaseOfCFGStmt<CXXOperatorCallExpr>(
776794
isComparisonOperatorCall(
777-
hasOptionalType(),
778-
unless(anyOf(hasOptionalType(), hasNulloptType()))),
795+
hasOptionalOrDerivedType(),
796+
unless(anyOf(hasOptionalOrDerivedType(), hasNulloptType()))),
779797
[](const clang::CXXOperatorCallExpr *Cmp,
780798
const MatchFinder::MatchResult &, LatticeTransferState &State) {
781799
transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env);
782800
})
783801
.CaseOfCFGStmt<CXXOperatorCallExpr>(
784802
isComparisonOperatorCall(
785-
unless(anyOf(hasOptionalType(), hasNulloptType())),
786-
hasOptionalType()),
803+
unless(anyOf(hasOptionalOrDerivedType(), hasNulloptType())),
804+
hasOptionalOrDerivedType()),
787805
[](const clang::CXXOperatorCallExpr *Cmp,
788806
const MatchFinder::MatchResult &, LatticeTransferState &State) {
789807
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
@@ -843,13 +861,7 @@ auto buildDiagnoseMatchSwitch(
843861

844862
ast_matchers::DeclarationMatcher
845863
UncheckedOptionalAccessModel::optionalClassDecl() {
846-
return optionalClass();
847-
}
848-
849-
static QualType valueTypeFromOptionalType(QualType OptionalTy) {
850-
auto *CTSD =
851-
cast<ClassTemplateSpecializationDecl>(OptionalTy->getAsCXXRecordDecl());
852-
return CTSD->getTemplateArgs()[0].getAsType();
864+
return cxxRecordDecl(optionalOrDerivedClass());
853865
}
854866

855867
UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
@@ -858,9 +870,11 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
858870
TransferMatchSwitch(buildTransferMatchSwitch()) {
859871
Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
860872
[&Ctx](QualType Ty) -> llvm::StringMap<QualType> {
861-
if (!isOptionalType(Ty))
873+
const CXXRecordDecl *Optional =
874+
getOptionalBaseClass(Ty->getAsCXXRecordDecl());
875+
if (Optional == nullptr)
862876
return {};
863-
return {{"value", valueTypeFromOptionalType(Ty)},
877+
return {{"value", valueTypeFromOptionalDecl(*Optional)},
864878
{"has_value", Ctx.BoolTy}};
865879
});
866880
}

‎clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3383,6 +3383,36 @@ TEST_P(UncheckedOptionalAccessTest, LambdaCaptureStateNotPropagated) {
33833383
}
33843384
)");
33853385
}
3386+
3387+
TEST_P(UncheckedOptionalAccessTest, ClassDerivedFromOptional) {
3388+
ExpectDiagnosticsFor(R"(
3389+
#include "unchecked_optional_access_test.h"
3390+
3391+
struct Derived : public $ns::$optional<int> {};
3392+
3393+
void target(Derived opt) {
3394+
*opt; // [[unsafe]]
3395+
if (opt)
3396+
*opt;
3397+
}
3398+
)");
3399+
}
3400+
3401+
TEST_P(UncheckedOptionalAccessTest, ClassTemplateDerivedFromOptional) {
3402+
ExpectDiagnosticsFor(R"(
3403+
#include "unchecked_optional_access_test.h"
3404+
3405+
template <class T>
3406+
struct Derived : public $ns::$optional<T> {};
3407+
3408+
void target(Derived<int> opt) {
3409+
*opt; // [[unsafe]]
3410+
if (opt)
3411+
*opt;
3412+
}
3413+
)");
3414+
}
3415+
33863416
// FIXME: Add support for:
33873417
// - constructors (copy, move)
33883418
// - assignment operators (default, copy, move)

0 commit comments

Comments
 (0)