[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

177–182

nit:

185–190

Nit: as above

192–197

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.

208–218

This can be rewritten as a range for

for (CallGraphNode* GNode : SCC) ...

214

228–231

[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 ↗

(On Diff #439132)

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 ↗

(On Diff #439132)

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

3841 ↗

(On Diff #441832)

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

 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 
 using namespace clang::ast_matchers;
 using clang::tidy::utils::hasPtrOrReferenceInFunc;
 
 namespace clang {
+namespace ast_matchers {
+/// 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();
+}
+} // namespace ast_matchers
 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(mapAnyOf(functionDecl, /* block callee */ varDecl)
+                          .with(isNoReturnDecl))),
+      objcMessageExpr(Internal, callee(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