Skip to content

Commit ec3d8e6

Browse files
njames93AaronBallman
njames93
authored and
AaronBallman
committed
Handle init statements in readability-else-after-return
Adds a new ASTMatcher condition called 'hasInitStatement()' that matches if, switch and range-for statements with an initializer. Reworked clang-tidy readability-else-after-return to handle variables in the if condition or init statements in c++17 ifs. Also checks if removing the else would affect object lifetimes in the else branch. Fixes PR44364.
1 parent a81cb1b commit ec3d8e6

File tree

8 files changed

+524
-55
lines changed

8 files changed

+524
-55
lines changed

‎clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

Lines changed: 210 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -10,52 +10,234 @@
1010
#include "clang/AST/ASTContext.h"
1111
#include "clang/ASTMatchers/ASTMatchFinder.h"
1212
#include "clang/Tooling/FixIt.h"
13+
#include "llvm/ADT/SmallVector.h"
1314

1415
using namespace clang::ast_matchers;
1516

1617
namespace clang {
1718
namespace tidy {
1819
namespace readability {
1920

21+
namespace {
22+
static const char ReturnStr[] = "return";
23+
static const char ContinueStr[] = "continue";
24+
static const char BreakStr[] = "break";
25+
static const char ThrowStr[] = "throw";
26+
static const char WarningMessage[] = "do not use 'else' after '%0'";
27+
static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
28+
29+
const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
30+
if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Node)) {
31+
if (DeclRef->getDecl()->getID() == DeclIdentifier) {
32+
return DeclRef;
33+
}
34+
} else {
35+
for (const Stmt *ChildNode : Node->children()) {
36+
if (const DeclRefExpr *Result = findUsage(ChildNode, DeclIdentifier)) {
37+
return Result;
38+
}
39+
}
40+
}
41+
return nullptr;
42+
}
43+
44+
const DeclRefExpr *
45+
findUsageRange(const Stmt *Node,
46+
const llvm::iterator_range<int64_t *> &DeclIdentifiers) {
47+
if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Node)) {
48+
if (llvm::is_contained(DeclIdentifiers, DeclRef->getDecl()->getID())) {
49+
return DeclRef;
50+
}
51+
} else {
52+
for (const Stmt *ChildNode : Node->children()) {
53+
if (const DeclRefExpr *Result =
54+
findUsageRange(ChildNode, DeclIdentifiers)) {
55+
return Result;
56+
}
57+
}
58+
}
59+
return nullptr;
60+
}
61+
62+
const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) {
63+
const auto *InitDeclStmt = dyn_cast_or_null<DeclStmt>(If->getInit());
64+
if (!InitDeclStmt)
65+
return nullptr;
66+
if (InitDeclStmt->isSingleDecl()) {
67+
const Decl *InitDecl = InitDeclStmt->getSingleDecl();
68+
assert(isa<VarDecl>(InitDecl) && "SingleDecl must be a VarDecl");
69+
return findUsage(If->getElse(), InitDecl->getID());
70+
}
71+
llvm::SmallVector<int64_t, 4> DeclIdentifiers;
72+
for (const Decl *ChildDecl : InitDeclStmt->decls()) {
73+
assert(isa<VarDecl>(ChildDecl) && "Init Decls must be a VarDecl");
74+
DeclIdentifiers.push_back(ChildDecl->getID());
75+
}
76+
return findUsageRange(If->getElse(), DeclIdentifiers);
77+
}
78+
79+
const DeclRefExpr *checkConditionVarUsageInElse(const IfStmt *If) {
80+
const VarDecl *CondVar = If->getConditionVariable();
81+
return CondVar != nullptr ? findUsage(If->getElse(), CondVar->getID())
82+
: nullptr;
83+
}
84+
85+
bool containsDeclInScope(const Stmt *Node) {
86+
if (isa<DeclStmt>(Node)) {
87+
return true;
88+
}
89+
if (const auto *Compound = dyn_cast<CompoundStmt>(Node)) {
90+
return llvm::any_of(Compound->body(), [](const Stmt *SubNode) {
91+
return isa<DeclStmt>(SubNode);
92+
});
93+
}
94+
return false;
95+
}
96+
97+
void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
98+
const Stmt *Else, SourceLocation ElseLoc) {
99+
auto Remap = [&](SourceLocation Loc) {
100+
return Context.getSourceManager().getExpansionLoc(Loc);
101+
};
102+
auto TokLen = [&](SourceLocation Loc) {
103+
return Lexer::MeasureTokenLength(Loc, Context.getSourceManager(),
104+
Context.getLangOpts());
105+
};
106+
107+
if (const auto *CS = dyn_cast<CompoundStmt>(Else)) {
108+
Diag << tooling::fixit::createRemoval(ElseLoc);
109+
SourceLocation LBrace = CS->getLBracLoc();
110+
SourceLocation RBrace = CS->getRBracLoc();
111+
SourceLocation RangeStart =
112+
Remap(LBrace).getLocWithOffset(TokLen(LBrace) + 1);
113+
SourceLocation RangeEnd = Remap(RBrace).getLocWithOffset(-1);
114+
115+
llvm::StringRef Repl = Lexer::getSourceText(
116+
CharSourceRange::getTokenRange(RangeStart, RangeEnd),
117+
Context.getSourceManager(), Context.getLangOpts());
118+
Diag << tooling::fixit::createReplacement(CS->getSourceRange(), Repl);
119+
} else {
120+
SourceLocation ElseExpandedLoc = Remap(ElseLoc);
121+
SourceLocation EndLoc = Remap(Else->getEndLoc());
122+
123+
llvm::StringRef Repl = Lexer::getSourceText(
124+
CharSourceRange::getTokenRange(
125+
ElseExpandedLoc.getLocWithOffset(TokLen(ElseLoc) + 1), EndLoc),
126+
Context.getSourceManager(), Context.getLangOpts());
127+
Diag << tooling::fixit::createReplacement(
128+
SourceRange(ElseExpandedLoc, EndLoc), Repl);
129+
}
130+
}
131+
} // namespace
132+
133+
ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,
134+
ClangTidyContext *Context)
135+
: ClangTidyCheck(Name, Context),
136+
WarnOnUnfixable(Options.get(WarnOnUnfixableStr, true)) {}
137+
138+
void ElseAfterReturnCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
139+
Options.store(Opts, WarnOnUnfixableStr, WarnOnUnfixable);
140+
}
141+
20142
void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
21143
const auto InterruptsControlFlow =
22-
stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
23-
breakStmt().bind("break"),
24-
expr(ignoringImplicit(cxxThrowExpr().bind("throw")))));
144+
stmt(anyOf(returnStmt().bind(ReturnStr), continueStmt().bind(ContinueStr),
145+
breakStmt().bind(BreakStr),
146+
expr(ignoringImplicit(cxxThrowExpr().bind(ThrowStr)))));
25147
Finder->addMatcher(
26-
compoundStmt(forEach(
27-
ifStmt(unless(isConstexpr()),
28-
// FIXME: Explore alternatives for the
29-
// `if (T x = ...) {... return; } else { <use x> }`
30-
// pattern:
31-
// * warn, but don't fix;
32-
// * fix by pulling out the variable declaration out of
33-
// the condition.
34-
unless(hasConditionVariableStatement(anything())),
35-
hasThen(stmt(anyOf(InterruptsControlFlow,
36-
compoundStmt(has(InterruptsControlFlow))))),
37-
hasElse(stmt().bind("else")))
38-
.bind("if"))),
148+
compoundStmt(
149+
forEach(ifStmt(unless(isConstexpr()),
150+
hasThen(stmt(
151+
anyOf(InterruptsControlFlow,
152+
compoundStmt(has(InterruptsControlFlow))))),
153+
hasElse(stmt().bind("else")))
154+
.bind("if")))
155+
.bind("cs"),
39156
this);
40157
}
41158

42159
void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
43160
const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
161+
const auto *Else = Result.Nodes.getNodeAs<Stmt>("else");
162+
const auto *OuterScope = Result.Nodes.getNodeAs<CompoundStmt>("cs");
163+
164+
bool IsLastInScope = OuterScope->body_back() == If;
44165
SourceLocation ElseLoc = If->getElseLoc();
45-
std::string ControlFlowInterruptor;
46-
for (const auto *BindingName : {"return", "continue", "break", "throw"})
47-
if (Result.Nodes.getNodeAs<Stmt>(BindingName))
48-
ControlFlowInterruptor = BindingName;
49166

50-
DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'")
51-
<< ControlFlowInterruptor;
52-
Diag << tooling::fixit::createRemoval(ElseLoc);
167+
auto ControlFlowInterruptor = [&]() -> llvm::StringRef {
168+
for (llvm::StringRef BindingName :
169+
{ReturnStr, ContinueStr, BreakStr, ThrowStr})
170+
if (Result.Nodes.getNodeAs<Stmt>(BindingName))
171+
return BindingName;
172+
return {};
173+
}();
174+
175+
if (!IsLastInScope && containsDeclInScope(Else)) {
176+
if (WarnOnUnfixable) {
177+
// Warn, but don't attempt an autofix.
178+
diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
179+
}
180+
return;
181+
}
53182

54-
// FIXME: Removing the braces isn't always safe. Do a more careful analysis.
55-
// FIXME: Change clang-format to correctly un-indent the code.
56-
if (const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("else"))
57-
Diag << tooling::fixit::createRemoval(CS->getLBracLoc())
58-
<< tooling::fixit::createRemoval(CS->getRBracLoc());
183+
if (checkConditionVarUsageInElse(If) != nullptr) {
184+
if (IsLastInScope) {
185+
// If the if statement is the last statement its enclosing statements
186+
// scope, we can pull the decl out of the if statement.
187+
DiagnosticBuilder Diag =
188+
diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
189+
<< ControlFlowInterruptor;
190+
if (checkInitDeclUsageInElse(If) != nullptr) {
191+
Diag << tooling::fixit::createReplacement(
192+
SourceRange(If->getIfLoc()),
193+
(tooling::fixit::getText(*If->getInit(), *Result.Context) +
194+
llvm::StringRef("\n"))
195+
.str())
196+
<< tooling::fixit::createRemoval(If->getInit()->getSourceRange());
197+
}
198+
const DeclStmt *VDeclStmt = If->getConditionVariableDeclStmt();
199+
const VarDecl *VDecl = If->getConditionVariable();
200+
std::string Repl =
201+
(tooling::fixit::getText(*VDeclStmt, *Result.Context) +
202+
llvm::StringRef(";\n") +
203+
tooling::fixit::getText(If->getIfLoc(), *Result.Context))
204+
.str();
205+
Diag << tooling::fixit::createReplacement(SourceRange(If->getIfLoc()),
206+
Repl)
207+
<< tooling::fixit::createReplacement(VDeclStmt->getSourceRange(),
208+
VDecl->getName());
209+
removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
210+
} else if (WarnOnUnfixable) {
211+
// Warn, but don't attempt an autofix.
212+
diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
213+
}
214+
return;
215+
}
216+
217+
if (checkInitDeclUsageInElse(If) != nullptr) {
218+
if (IsLastInScope) {
219+
// If the if statement is the last statement its enclosing statements
220+
// scope, we can pull the decl out of the if statement.
221+
DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
222+
<< ControlFlowInterruptor;
223+
Diag << tooling::fixit::createReplacement(
224+
SourceRange(If->getIfLoc()),
225+
(tooling::fixit::getText(*If->getInit(), *Result.Context) +
226+
"\n" +
227+
tooling::fixit::getText(If->getIfLoc(), *Result.Context))
228+
.str())
229+
<< tooling::fixit::createRemoval(If->getInit()->getSourceRange());
230+
removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
231+
} else if (WarnOnUnfixable) {
232+
// Warn, but don't attempt an autofix.
233+
diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
234+
}
235+
return;
236+
}
237+
238+
DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
239+
<< ControlFlowInterruptor;
240+
removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
59241
}
60242

61243
} // namespace readability

‎clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,14 @@ namespace readability {
2020
/// http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
2121
class ElseAfterReturnCheck : public ClangTidyCheck {
2222
public:
23-
ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context)
24-
: ClangTidyCheck(Name, Context) {}
23+
ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context);
24+
25+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
2526
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
2627
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
28+
29+
private:
30+
const bool WarnOnUnfixable;
2731
};
2832

2933
} // namespace readability

‎clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-warn.cpp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
2+
// RUN: -config='{CheckOptions: [ \
3+
// RUN: {key: readability-else-after-return.WarnOnUnfixable, value: 0}, \
4+
// RUN: ]}'
5+
6+
int h(int);
7+
8+
int lifeTimeExtensionTests(int a) {
9+
if (a > 0) {
10+
return a;
11+
} else {
12+
// CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
13+
int b = 0;
14+
h(b);
15+
}
16+
if (int b = a) {
17+
return a;
18+
} else {
19+
// CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
20+
b++;
21+
}
22+
if (int b = a) { // comment-0
23+
// CHECK-FIXES: {{^}} int b = a;
24+
// CHECK-FIXES-NEXT: {{^}}if (b) { // comment-0
25+
return a;
26+
} else { // comment-0
27+
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
28+
// CHECK-FIXES: {{^}} } // comment-0
29+
return b;
30+
}
31+
}

0 commit comments

Comments
 (0)