Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp +++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp @@ -23,39 +23,183 @@ breakStmt().bind("break"), expr(ignoringImplicit(cxxThrowExpr().bind("throw"))))); Finder->addMatcher( - compoundStmt(forEach( - ifStmt(unless(isConstexpr()), - // FIXME: Explore alternatives for the - // `if (T x = ...) {... return; } else { }` - // pattern: - // * warn, but don't fix; - // * fix by pulling out the variable declaration out of - // the condition. - unless(hasConditionVariableStatement(anything())), - hasThen(stmt(anyOf(InterruptsControlFlow, - compoundStmt(has(InterruptsControlFlow))))), - hasElse(stmt().bind("else"))) - .bind("if"))), + compoundStmt( + forEach(ifStmt(unless(isConstexpr()), + hasThen(stmt( + anyOf(InterruptsControlFlow, + compoundStmt(has(InterruptsControlFlow))))), + hasElse(stmt().bind("else"))) + .bind("if"))) + .bind("cs"), this); } +const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) { + if (const auto *DeclRef = dyn_cast_or_null(Node)) { + if (DeclRef->getDecl()->getID() == DeclIdentifier) { + return DeclRef; + } + } else { + for (const auto &ChildNode : Node->children()) { + if (auto Result = findUsage(ChildNode, DeclIdentifier)) { + return Result; + } + } + } + return nullptr; +} + +const DeclRefExpr * +findUsageRange(const Stmt *Node, + const llvm::iterator_range &DeclIdentifiers) { + if (const auto *DeclRef = dyn_cast_or_null(Node)) { + if (llvm::is_contained(DeclIdentifiers, DeclRef->getDecl()->getID())) { + return DeclRef; + } + } else { + for (const auto &ChildNode : Node->children()) { + if (auto Result = findUsageRange(ChildNode, DeclIdentifiers)) { + return Result; + } + } + } + return nullptr; +} + +const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) { + auto InitDeclStmt = dyn_cast_or_null(If->getInit()); + if (!InitDeclStmt) + return nullptr; + if (InitDeclStmt->isSingleDecl()) { + auto InitDecl = InitDeclStmt->getSingleDecl(); + assert(isa(InitDecl) && "SingleDecl must be a VarDecl"); + return findUsage(If->getElse(), InitDecl->getID()); + } + llvm::SmallVector DeclIdentifiers; + for (const auto &Decl : InitDeclStmt->decls()) { + assert(isa(Decl) && "Init Decls must be a VarDecl"); + DeclIdentifiers.push_back(Decl->getID()); + } + return findUsageRange(If->getElse(), DeclIdentifiers); +} + +const DeclRefExpr *checkConditionVarUsageInElse(const IfStmt *If) { + auto CondVar = If->getConditionVariable(); + return CondVar != nullptr ? findUsage(If->getElse(), CondVar->getID()) + : nullptr; +} + +bool containsDeclInScope(const Stmt *Node) { + if (isa(Node)) { + return true; + } + if (auto Compound = dyn_cast(Node)) { + return llvm::any_of(Compound->body(), [](const Stmt *SubNode) { + return isa(SubNode); + }); + } + return false; +} + +void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context, + const Stmt *Else, SourceLocation ElseLoc) { + auto Remap = [&](SourceLocation Loc) { + return Context.getSourceManager().getExpansionLoc(Loc); + }; + auto TokLen = [&](SourceLocation Loc) { + return Lexer::MeasureTokenLength(Loc, Context.getSourceManager(), + Context.getLangOpts()); + }; + + if (auto CS = dyn_cast(Else)) { + Diag << tooling::fixit::createRemoval(ElseLoc); + const auto LBrace = CS->getLBracLoc(); + const auto RBrace = CS->getRBracLoc(); + const auto RangeStart = Remap(LBrace).getLocWithOffset(TokLen(LBrace)); + const auto RangeEnd = Remap(RBrace).getLocWithOffset(-1); + + const auto Repl = Lexer::getSourceText( + CharSourceRange::getTokenRange(RangeStart, RangeEnd), + Context.getSourceManager(), Context.getLangOpts()); + Diag << tooling::fixit::createReplacement(CS->getSourceRange(), Repl); + } else { + const auto ElseExpandedLoc = Remap(ElseLoc); + const auto EndLoc = Remap(Else->getEndLoc()); + + const auto Repl = Lexer::getSourceText( + CharSourceRange::getTokenRange( + ElseExpandedLoc.getLocWithOffset(TokLen(ElseLoc)), EndLoc), + Context.getSourceManager(), Context.getLangOpts()); + Diag << tooling::fixit::createReplacement( + SourceRange(ElseExpandedLoc, EndLoc), Repl); + } +} + void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) { - const auto *If = Result.Nodes.getNodeAs("if"); + const auto If = Result.Nodes.getNodeAs("if"); + const auto Else = Result.Nodes.getNodeAs("else"); + const auto OuterScope = Result.Nodes.getNodeAs("cs"); + const bool IsLastInScope = OuterScope->body_back() == If; + + if (!IsLastInScope && containsDeclInScope(Else)) { + // { + // ... + // if (a) { return } + // else { int b = 0; } cant remove the else as b will leak its scope + // ... + // } + return; + } + SourceLocation ElseLoc = If->getElseLoc(); - std::string ControlFlowInterruptor; - for (const auto *BindingName : {"return", "continue", "break", "throw"}) - if (Result.Nodes.getNodeAs(BindingName)) - ControlFlowInterruptor = BindingName; + llvm::StringRef ControlFlowInterruptor = [&]() -> llvm::StringRef { + for (llvm::StringRef BindingName : {"return", "continue", "break", "throw"}) + if (Result.Nodes.getNodeAs(BindingName)) + return BindingName; + return {}; + }(); + if (checkInitDeclUsageInElse(If) != nullptr || + checkConditionVarUsageInElse(If) != nullptr) { + if (IsLastInScope) { + // If the if statement is the last statement its enclosing statements + // scope, we can pull the decl out of the if statement. + DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'", + clang::DiagnosticIDs::Level::Remark) + << ControlFlowInterruptor; + if (If->hasInitStorage()) { + Diag << tooling::fixit::createReplacement( + SourceRange(If->getIfLoc()), + (tooling::fixit::getText(*If->getInit(), *Result.Context) + + llvm::StringRef("\n") + + tooling::fixit::getText(If->getIfLoc(), *Result.Context)) + .str()) + << tooling::fixit::createRemoval(If->getInit()->getSourceRange()); + removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc); + } else { // condition variable + auto VDeclStmt = If->getConditionVariableDeclStmt(); + auto VDecl = If->getConditionVariable(); + auto Repl = (tooling::fixit::getText(*VDeclStmt, *Result.Context) + + llvm::StringRef(";\n") + + tooling::fixit::getText(If->getIfLoc(), *Result.Context)) + .str(); + Diag << tooling::fixit::createReplacement(SourceRange(If->getIfLoc()), + Repl) + << tooling::fixit::createReplacement(VDeclStmt->getSourceRange(), + VDecl->getName()); + removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc); + } + } else { + // warn, but don't attempt an autofix + diag(ElseLoc, "do not use 'else' after '%0'", + clang::DiagnosticIDs::Level::Remark) + << ControlFlowInterruptor; + } + return; + } DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'") << ControlFlowInterruptor; - Diag << tooling::fixit::createRemoval(ElseLoc); - - // FIXME: Removing the braces isn't always safe. Do a more careful analysis. - // FIXME: Change clang-format to correctly un-indent the code. - if (const auto *CS = Result.Nodes.getNodeAs("else")) - Diag << tooling::fixit::createRemoval(CS->getLBracLoc()) - << tooling::fixit::createRemoval(CS->getRBracLoc()); + removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc); } } // namespace readability Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp +++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions +// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions -std=c++17 namespace std { struct string { @@ -15,15 +15,15 @@ if (a > 0) return; else // comment-0 - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return' - // CHECK-FIXES: {{^}} // comment-0 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} // comment-0 return; if (a > 0) { return; } else { // comment-1 - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' - // CHECK-FIXES: {{^}} } // comment-1 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-1 return; } @@ -60,15 +60,15 @@ if (a < 10) return; else // comment-5 - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' - // CHECK-FIXES: {{^}} // comment-5 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} // comment-5 f(0); } else { if (a > 10) return; else // comment-6 - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' - // CHECK-FIXES: {{^}} // comment-6 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} // comment-6 f(0); } } @@ -78,29 +78,29 @@ if (x) { continue; } else { // comment-7 - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue' - // CHECK-FIXES: {{^}} } // comment-7 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue' + // CHECK-FIXES: {{^}} } // comment-7 x++; } if (x) { break; } else { // comment-8 - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break' - // CHECK-FIXES: {{^}} } // comment-8 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break' + // CHECK-FIXES: {{^}} } // comment-8 x++; } if (x) { throw 42; } else { // comment-9 - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw' - // CHECK-FIXES: {{^}} } // comment-9 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw' + // CHECK-FIXES: {{^}} } // comment-9 x++; } if (x) { throw my_exception("foo"); } else { // comment-10 - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw' - // CHECK-FIXES: {{^}} } // comment-10 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw' + // CHECK-FIXES: {{^}} } // comment-10 x++; } } @@ -109,11 +109,68 @@ extern int *g(); extern void h(int **x); -int *decl_in_condition() { +int *declInConditionUsedInElse() { if (int *x = g()) { return x; - } else { + } else { // comment-11 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-11 h(&x); return x; } } +int *declInConditionUnusedInElse() { + if (int *x = g()) { + h(&x); + return x; + } else { // comment-12 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-12 + return nullptr; + } +} + +int *varInitAndCondition() { + if (int *X = g(); X != nullptr) { + return X; + } else { // comment-13 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-13 + h(&X); + return X; + } +} + +int *varInitAndConditionUnusedInElse() { + if (int *X = g(); X != nullptr) { + return X; + } else { // comment-14 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-14 + return nullptr; + } +} + +int *initAndCondition() { + int *X; + if (X = g(); X != nullptr) { + return X; + } else { // comment-15 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-15 + h(&X); + return X; + } +} + +int *varInitAndConditionUnusedInElseWithDecl() { + int *Y = g(); + if (int *X = g(); X != nullptr) { + return X; + } else { // comment-16 + // CHECK-FIXES-NOT{{^}} } // comment-16 + int *Y = g(); + h(&Y); + } + return Y; +} Index: clang/docs/LibASTMatchersReference.html =================================================================== --- clang/docs/LibASTMatchersReference.html +++ clang/docs/LibASTMatchersReference.html @@ -3009,6 +3009,23 @@ +Matcher<Expr>nullPointerConstant +
Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  matches the initializer for v1, v2, v3, cp, and ip. Does not match the
+  initializer for i.
+
+ + Matcher<FieldDecl>hasBitWidthunsigned Width 
Matches non-static data members that are bit-fields of the specified
 bit width.
@@ -3761,6 +3778,18 @@
 Example matches y (matcher = parmVarDecl(hasDefaultArgument()))
 void x(int val) {}
 void y(int val = 0) {}
+
+Deprecated. Use hasInitializer() instead to be able to
+match on the contents of the default argument.  For example:
+
+void x(int val = 7) {}
+void y(int val = 42) {}
+parmVarDecl(hasInitializer(integerLiteral(equals(42))))
+  matches the parameter of y
+
+A matcher such as
+  parmVarDecl(hasInitializer(anything()))
+is equivalent to parmVarDecl(hasDefaultArgument()).
@@ -4479,23 +4508,6 @@ -Matcher<internal::Matcher<Expr>>nullPointerConstant -
Matches expressions that resolve to a null pointer constant, such as
-GNU's __null, C++11's nullptr, or C's NULL macro.
-
-Given:
-  void *v1 = NULL;
-  void *v2 = nullptr;
-  void *v3 = __null; // GNU extension
-  char *cp = (char *)0;
-  int *ip = 0;
-  int i = 0;
-expr(nullPointerConstant())
-  matches the initializer for v1, v2, v3, cp, and ip. Does not match the
-  initializer for i.
-
- - Matcher<internal::Matcher<NamedDecl>>hasAnyNameStringRef, ..., StringRef 
Matches NamedDecl nodes that have any of the specified names.
 
@@ -5082,6 +5094,29 @@
+Matcher<CXXForRangeStmt>hasInitStatementMatcher<Stmt> InnerMatcher +
Matches selection statements with initializer.
+
+Given:
+ void foo() { 
+   if (int i = foobar(); i > 0) {}
+   switch (int i = foobar(); i) {}
+   for (auto& a = get_range(); auto& x : a) {} 
+ }
+ void bar() { 
+   if (foobar() > 0) {}
+   switch (foobar()) {}
+   for (auto& x : get_range()) {} 
+ }
+ifStmt(hasInitStatement(anything()))
+  matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+  matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+  matches the range for statement in foo but not in bar.
+
+ + Matcher<CXXForRangeStmt>hasLoopVariableMatcher<VarDecl> InnerMatcher 
Matches the initialization statement of a for loop.
 
@@ -6206,6 +6241,29 @@
+Matcher<IfStmt>hasInitStatementMatcher<Stmt> InnerMatcher +
Matches selection statements with initializer.
+
+Given:
+ void foo() { 
+   if (int i = foobar(); i > 0) {}
+   switch (int i = foobar(); i) {}
+   for (auto& a = get_range(); auto& x : a) {} 
+ }
+ void bar() { 
+   if (foobar() > 0) {}
+   switch (foobar()) {}
+   for (auto& x : get_range()) {} 
+ }
+ifStmt(hasInitStatement(anything()))
+  matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+  matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+  matches the range for statement in foo but not in bar.
+
+ + Matcher<IfStmt>hasThenMatcher<Stmt> InnerMatcher 
Matches the then-statement of an if statement.
 
@@ -6943,6 +7001,29 @@
+Matcher<SwitchStmt>hasInitStatementMatcher<Stmt> InnerMatcher +
Matches selection statements with initializer.
+
+Given:
+ void foo() { 
+   if (int i = foobar(); i > 0) {}
+   switch (int i = foobar(); i) {}
+   for (auto& a = get_range(); auto& x : a) {} 
+ }
+ void bar() { 
+   if (foobar() > 0) {}
+   switch (foobar()) {}
+   for (auto& x : get_range()) {} 
+ }
+ifStmt(hasInitStatement(anything()))
+  matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+  matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+  matches the range for statement in foo but not in bar.
+
+ + Matcher<TagType>hasDeclarationconst Matcher<Decl> InnerMatcher 
Matches a node if the declaration associated with that node
 matches the given matcher.
@@ -7170,6 +7251,22 @@
+Matcher<T>traverseast_type_traits::TraversalKind TK, const Matcher<T> InnerMatcher +
Causes all nested matchers to be matched with the specified traversal kind.
+
+Given
+  void foo()
+  {
+      int i = 3.0;
+  }
+The matcher
+  traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+    varDecl(hasInitializer(floatLiteral().bind("init")))
+  )
+matches the variable declaration with "init" bound to the "3.0".
+
+ + Matcher<TypedefNameDecl>hasTypeMatcher<QualType> InnerMatcher 
Matches if the expression's or declaration's type matches a type
 matcher.
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===================================================================
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///    if (int i = foobar(); i > 0) {}
+///    switch (int i = foobar(); i) {}
+///    for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///    if (foobar() > 0) {}
+///    switch (foobar()) {}
+///    for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+                                                          CXXForRangeStmt),
+                          internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===================================================================
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
                          LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+                      ifStmt(hasInitStatement(anything())),
+                      LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+                         ifStmt(hasInitStatement(anything()))));
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+                         ifStmt(hasInitStatement(anything()))));
+  EXPECT_TRUE(matches(
+      "void baz(int i) { switch (int j = i; j) { default: break; } }",
+      switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+                         switchStmt(hasInitStatement(anything()))));
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+                      "int items[] = {};"
+                      "for (auto &arr = items; auto &item : arr) {}"
+                      "}",
+                      cxxForRangeStmt(hasInitStatement(anything())),
+                      LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+                         "int items[] = {};"
+                         "for (auto &item : items) {}"
+                         "}",
+                         cxxForRangeStmt(hasInitStatement(anything()))));
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
     matches("template struct C {}; C c;",