[Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[ziqingluo-90]

Summary

Fixing the bug that there is missing handling of noreturn attributes for ObjC nodes. If infinite loop checker sees a call to a function with a noreturn attribute, it shall never report a warning.

[njames93]

Can this patch be split in two, it seems strange to be fixing 2 unrelated bugs in one patch.
One fix for the ObjC nodes and another for the patch for the static local variables.

Also please can you run git clang-format over this patch.

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp

164–169

nit:

172–177

Nit: as above

179–184

Nit:

Also sometimes, some of the stmts in children can be null and that's expected, is there any reason we are disregarding that case.

195–205

This can be rewritten as a range for

for (CallGraphNode* GNode : SCC) ...

201

215–218

[ziqingluo-90]

Separate the change adding handling of noreturn attributes for ObjC nodes. Have run clang-format.

[NoQ]

Nice!

There's usually some bureaucracy when creating new matchers, i.e. there should be documentation and unittests covering them.

clang/include/clang/ASTMatchers/ASTMatchers.h

5099

Can we make a polymorphic matcher hasNoReturnAttr that can accept either decl or type?

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[ziqingluo-90]

rebased with the latest main's HEAD

[ziqingluo-90]

adjusted my changes with respect to the recent file structure changes in clang-tidy test

[ziqingluo-90]

I saw @aaron.ballman 's comment "We typically don't add AST matchers until we have a need for them to be used in-tree (ASTMatchers.h is already really expensive to parse; adding matchers for everything possible with AST nodes would be prohibitively expensive)." in this patch. So I think the matchers for testing noreturn attributes is too specific to be in the ASTMatcher.h. Moving them to where they are used. While the objcMessageCallee is sort of the Obj-C counterpart of the callee matcher, so I think it is general enough to stay.

[ziqingluo-90]

adding a unit test for the ASTMatcher objcMessageCallee added in ASTMatcher.h

[ziqingluo-90]

In D128314#3605588, @NoQ wrote:

Nice!

There's usually some bureaucracy when creating new matchers, i.e. there should be documentation and unittests covering them.

I have added a unit test and auto-generated document for it.

clang/include/clang/ASTMatchers/ASTMatchers.h

5099

I've moved these two matchers, which seems are specific to the infinite loop checker, to InfiniteLoopCheck.cpp. Maybe they no longer need to be generalized by polymorphism.

[NoQ]

Sounds great! I have a nitpick but other than that I think this fix is good to go.

clang/include/clang/ASTMatchers/ASTMatchers.h

3832

It looks like documentation traditionally starts with a capital letter and ends with .

[njames93]

Sorry to do this again, but could this be split up again, one patch for the new matcher and the tests associated with it, then another for the actual bug fix.
Also cc @klimek as he is the code owner of ASTMatchers

[ziqingluo-90]

In D128314#3635861, @njames93 wrote:

Sorry to do this again, but could this be split up again, one patch for the new matcher and the tests associated with it, then another for the actual bug fix.
Also cc @klimek as he is the code owner of ASTMatchers

Makes sense. I have created a new review request for the ASTMatcher. This patch now depends on it.

[NoQ]

Thanks! Good to go after the matchers patch is committed.

[ziqingluo-90]

The callee ASTMatcher overloading patch has landed in LLVM repo. I update this patch to use callee for matching objective-C message callee methods.

[NoQ]

Still looks good to me, let's commit!

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp

 using clang::tidy::utils::hasPtrOrReferenceInFunc;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 static internal::Matcher<Stmt>
 loopEndingStmt(internal::Matcher<Stmt> Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher<QualType> isNoReturnFunType =
+      ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher<Decl> isNoReturnDecl =
+      anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+            varDecl(hasType(blockPointerType(pointee(isNoReturnFunType)))));
+
   return stmt(anyOf(
       mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-      callExpr(Internal, callee(functionDecl(isNoReturn())))));
+      callExpr(Internal, callee(functionDecl(isNoReturnDecl))),
+      objcMessageExpr(Internal, objcMessageCallee(isNoReturnDecl)),
+      callExpr(Internal, callee(varDecl(isNoReturnDecl)))));
 }
 
 /// Return whether `Var` was changed in `LoopStmt`.
 static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var,
                       ASTContext *Context) {
   if (const auto *ForLoop = dyn_cast<ForStmt>(LoopStmt))
     return (ForLoop->getInc() &&
             ExprMutationAnalyzer(*ForLoop->getInc(), *Context)

 void InfiniteLoopCheck::registerMatchers(MatchFinder *Finder) {
   const auto LoopCondition = allOf(
       hasCondition(
           expr(forCallable(decl().bind("func"))).bind("condition")),
       unless(hasBody(hasDescendant(
           loopEndingStmt(forCallable(equalsBoundNode("func")))))));
 
   Finder->addMatcher(mapAnyOf(whileStmt, doStmt, forStmt)
                          .with(LoopCondition)
                          .bind("loop-stmt"),
                      this);
 }
 
 void InfiniteLoopCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Cond = Result.Nodes.getNodeAs<Expr>("condition");
   const auto *LoopStmt = Result.Nodes.getNodeAs<Stmt>("loop-stmt");
   const auto *Func = Result.Nodes.getNodeAs<Decl>("func");
 
   if (isKnownToHaveValue(*Cond, *Result.Context, false))
     return;
 
   bool ShouldHaveConditionVariables = true;
   if (const auto *While = dyn_cast<WhileStmt>(LoopStmt)) {
     if (const VarDecl *LoopVarDecl = While->getConditionVariable()) {
       if (const Expr *Init = LoopVarDecl->getInit()) {
         ShouldHaveConditionVariables = false;
         Cond = Init;
       }
     }
   }
 
   if (ExprMutationAnalyzer::isUnevaluated(LoopStmt, *LoopStmt, *Result.Context))
     return;
 
   if (isAtLeastOneCondVarChanged(Func, LoopStmt, Cond, Result.Context))
     return;
 
   std::string CondVarNames = getCondVarNames(Cond);
   if (ShouldHaveConditionVariables && CondVarNames.empty())
     return;
 
   if (CondVarNames.empty()) {
     diag(LoopStmt->getBeginLoc(),
          "this loop is infinite; it does not check any variables in the"
          " condition");
   } else {
     diag(LoopStmt->getBeginLoc(),
          "this loop is infinite; none of its condition variables (%0)"
          " are updated in the loop body")
       << CondVarNames;
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm

+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+    // no warning, function term has C noreturn attribute
+    term();
+  }
+  while (i < 10) {
+    // no warning, class method baz has noreturn attribute
+    [I baz];
+  }
+  while (i + j < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+    [I foo];
+  }
+  while (i + j < 10) {
+    [I foo];
+    [I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+    block();
+  }
+  while (i < 10) {
+    // no warning, the block has "noreturn" arribute
+    block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+    [I bar];
+  }
+}
+@end

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// implemented in terms of implicit casts.
 AST_MATCHER_P(CallExpr, callee, internal::Matcher<Stmt>,
               InnerMatcher) {
   const Expr *ExprNode = Node.getCallee();
   return (ExprNode != nullptr &&
           InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"))))
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+              internal::Matcher<ObjCMethodDecl>, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+          InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
 /// Example matches y.x() (matcher = callExpr(callee(
 ///                                    cxxMethodDecl(hasName("x")))))
 /// \code
 ///   class Y { public: void x(); };
 ///   void z() { Y y; y.x(); }

 /// \endcode
 /// functionDecl(isNoReturn())
 ///   matches all of those except
 /// \code
 ///   void nope();
 /// \endcode
 AST_MATCHER(FunctionDecl, isNoReturn) { return Node.isNoReturn(); }
 
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr<NoReturnAttr>() || Node.hasAttr<CXX11NoReturnAttr>() ||
+         Node.hasAttr<C11NoReturnAttr>();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+
 /// Matches the return type of a function declaration.
 ///
 /// Given:
 /// \code
 ///   class X { int f() { return 1; } };
 /// \endcode
 /// cxxMethodDecl(returns(asString("int")))
 ///   matches int f() { return 1; }