[clang-tidy] Add a utility Matcher to match the next statement within a statement sequence

[tbourvon]

Summary

This adds a utility matcher (which is placed in util/Matchers.h, because it uses functions from ASTMatchFinder.h which cannot be used from ASTMatchers.h) used by D37014).
This matcher matches the next statement in a statement sequence (such as CompoundStatement). It handles switch statements gracefully.

[aaron.ballman]

clang-tidy/utils/Matchers.h

16

This will require linking in the clangAnalysis library as well; are we sure we want to take on this dependency here for all matchers?

58–60

Why does it cause the crash? Should that crash not be fixed instead of applying this workaround?

70

No need for the parenthetical. It's generally understood what a CFG is, so you can just use the acronym.

74–75

Elide braces (here and elsewhere).

81

Formatting (here and elsewhere): you should run the patch through clang-format.

[tbourvon]

clang-tidy/utils/Matchers.h

16

Do you have a specific solution in mind? We could make the matcher local to the check it is being used in (see D37014), but I think it could prove useful for other checks...

58–60

I'm not entirely sure if this is expected behavior or not. In terms of AST, switch statements are a bit special in terms of how they are represented (each case contains all the subsequent cases as its children IIRC).
There probably is a way to make the CFG work in these cases, but I honestly don't have the time to look into that and attempt a fix. Couldn't this be good enough for now, maybe with a FIXME?

[aaron.ballman]

clang-tidy/utils/Matchers.h

16

No solution in mind -- I was mostly wondering if @alexfh was okay picking up this dependency or not, because it will impact all the clang-tidy modules.

58–60

I'm uncomfortable simply working around known crashes rather than fixing them, even with FIXME comments, because that just means we accrue more technical debt with no plan in place to pay it down in the future. However, I'll let @alexfh make the final call on it.

[tbourvon]

@alexfh What is your opinion regarding adding this dependency?

[alexfh]

In D42624#1027506, @tbourvon wrote:

@alexfh What is your opinion regarding adding this dependency?

I'm fine with the dependency, and I'd probably make the matcher local to the check until there is at least one more use case for it in the codebase.

[sylvestre.ledru]

@aaron.ballman Hello, Do you think it is ready to land? Thanks

[aaron.ballman]

In D42624#1031073, @sylvestre.ledru wrote:

@aaron.ballman Hello, Do you think it is ready to land? Thanks

@alexfh was asking to make this matcher local to the check rather than add it to Matchers.h, so I think this patch should be abandoned and rolled into D37014 directly, but the dependency is fine to pick up in that patch.

clang-tidy/utils/Matchers.h

 //===--- Matchers.h - clang-tidy-------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H
 
 #include "TypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CFG.h"
 
 namespace clang {
 namespace tidy {
 namespace matchers {
 
 AST_MATCHER(BinaryOperator, isAssignmentOperator) {
   return Node.isAssignmentOp();
 }

 }
 
 // Returns QualType matcher for references to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
   using namespace ast_matchers;
   return referenceType(pointee(qualType(isConstQualified())));
 }
 
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor,
+              ast_matchers::internal::Matcher<Stmt>, InnerMatcher) {
+  using namespace ast_matchers;
+
+  // We get the first parent, making sure that we're not in a case statement
+  // not in a compound statement directly inside a switch, because this causes
+  // the buildCFG call to crash.
+  auto Parent = selectFirst<Stmt>(
+    "parent",
+    match(
+      stmt(hasAncestor(stmt(
+        unless(caseStmt()),
+        unless(compoundStmt(hasParent(switchStmt()))),
+        stmt().bind("parent")))),
+    Node, Finder->getASTContext()));
+
+  // We build a Control Flow Graph (CFG) from the parent statement.
+  std::unique_ptr<CFG> StatementCFG =
+    CFG::buildCFG(nullptr, const_cast<Stmt*>(Parent), &Finder->getASTContext(),
+                  CFG::BuildOptions());
+
+  if (!StatementCFG) {
+    return false;
+  }
+
+  // We iterate through all the CFGBlocks, which basically means that we go over
+  // all the possible branches of the code and therefore cover all statements.
+  for (auto& Block : *StatementCFG) {
+    if (!Block) {
+      continue;
+    }
+
+    // We iterate through all the statements of the block.
+    bool ReturnNextStmt = false;
+    for (auto& BlockItem : *Block) {
+      Optional<CFGStmt> CFGStatement = BlockItem.getAs<CFGStmt>();
+      if (!CFGStatement) {
+        if (ReturnNextStmt) {
+          return false;
+        }
+
+        continue;
+      }
+
+      // If we found the next statement, we apply the inner matcher and return
+      // the result.
+      if (ReturnNextStmt) {
+        return InnerMatcher.matches(*CFGStatement->getStmt(), Finder, Builder);
+      }
+
+      if (CFGStatement->getStmt() == &Node) {
+        ReturnNextStmt = true;
+      }
+    }
+  }
+
+  // If we didn't find a successor, we just return false.
+  return false;
+}
+
 } // namespace matchers
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H

unittests/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANG_LINT_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
+  MatchersUtilsTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp)
 
 target_link_libraries(ClangTidyTests
   PRIVATE

unittests/clang-tidy/MatchersUtilsTest.cpp

+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
+#include "utils/Matchers.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace ast_matchers;
+using namespace matchers;
+
+TEST(StatementMatcher, HasSuccessor) {
+  StatementMatcher DeclHasSuccessorReturnStmt =
+    declStmt(hasSuccessor(returnStmt()));
+
+  EXPECT_TRUE(matches(
+    "void foo() { int bar; return; }",
+    DeclHasSuccessorReturnStmt));
+  EXPECT_TRUE(notMatches(
+    "void foo() { int bar; bar++; return; }",
+    DeclHasSuccessorReturnStmt));
+}
+
+}
+}
+}