[clang-tidy] bugfix in InfiniteLoopCheck to not print warnings for unevaluated loops

[NoQ]

Thx Dmitry!

Usama, I heard you have plans to make isUnevaluated() consume statements so that you could feed the entire loop into the machine without separating it into parts. You can either update this patch or do that as a follow-up patch.

Also I think it's time for you to ask for commit access, so please do! (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

[usama54321]

Added a separate check for unevaluated statements. Updated InfiniteLoopCheck to use new check

[usama54321]

Updating patch

[NoQ]

clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h

43–44

I suspect this may lead to subtle bugs when a Stmt * that actually points to an Expr at runtime would cause the first overload to be chosen.

Maybe teach canResolveToExpr() to accept an Stmt instead (with early return)?

clang/lib/Analysis/ExprMutationAnalyzer.cpp

202–204

We're trying to avoid auto when it makes the code less readable i.e. when the type isn't obvious from the context (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable).

[usama54321]

• updated canResolveToExpr to accept both statements and expressions. Removed unnecessary code

[NoQ]

clang/lib/Analysis/ExprMutationAnalyzer.cpp

234–236

Shouldn't this one be removed now?

[NoQ]

Whoops, no, this isn't an actual concern. Looks good then, please commit!

clang/lib/Analysis/ExprMutationAnalyzer.cpp

234–236

Nvm, it's the old interface.

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

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()) {

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

void test_dependent_condition() {
   do {
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (val3) are updated in the loop body [bugprone-infinite-loop]
   } while (1, (true) && val3 == 1);
 
   const int val4 = some_template_fn<T>();
   do {
   } while (1, (false) && val4 == 1);
 }
+
+void test_typeof() {
+  __typeof__({
+    for (int i = 0; i < 10; ++i) {
+    }
+    0;
+  }) x;
+}
+
+void test_typeof_infinite() {
+  __typeof__({
+    for (int i = 0; i < 10;) {
+    }
+    0;
+  }) x;
+}
+
+void test_typeof_while_infinite() {
+  __typeof__({
+    int i = 0;
+    while (i < 10) {
+    }
+    0;
+  }) x;
+}
+
+void test_typeof_dowhile_infinite() {
+  __typeof__({
+    int i = 0;
+    do {
+
+    } while (i < 10);
+    0;
+  }) x;
+}

clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h

public:
   bool isPointeeMutated(const Expr *Exp) {
     return findPointeeMutation(Exp) != nullptr;
   }
   bool isPointeeMutated(const Decl *Dec) {
     return findPointeeMutation(Dec) != nullptr;
   }
   const Stmt *findPointeeMutation(const Expr *Exp);
   const Stmt *findPointeeMutation(const Decl *Dec);
+  static bool isUnevaluated(const Stmt *Smt, const Stmt &Stm,
+                            ASTContext &Context);
 
 private:
   using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *);
   using ResultMap = llvm::DenseMap<const Expr *, const Stmt *>;
 
   const Stmt *findMutationMemoized(const Expr *Exp,
                                    llvm::ArrayRef<MutationFinder> Finders,
                                    ResultMap &MemoizedResults);
   const Stmt *tryEachDeclRef(const Decl *Dec, MutationFinder Finder);
 

clang/lib/Analysis/ExprMutationAnalyzer.cpp

while (const auto *BOComma =
              dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
     if (!BOComma->isCommaOp())
       break;
     Result = BOComma->getRHS();
   }
   return InnerMatcher.matches(*Result, Finder, Builder);
 }
 
-AST_MATCHER_P(Expr, canResolveToExpr, ast_matchers::internal::Matcher<Expr>,
+AST_MATCHER_P(Stmt, canResolveToExpr, ast_matchers::internal::Matcher<Stmt>,
               InnerMatcher) {
+  auto *Exp = dyn_cast<Expr>(&Node);
+  if (!Exp) {
+    return stmt().matches(Node, Finder, Builder);
+  }
+
   auto DerivedToBase = [](const ast_matchers::internal::Matcher<Expr> &Inner) {
     return implicitCastExpr(anyOf(hasCastKind(CK_DerivedToBase),
                                   hasCastKind(CK_UncheckedDerivedToBase)),
                             hasSourceExpression(Inner));
   };
   auto IgnoreDerivedToBase =
       [&DerivedToBase](const ast_matchers::internal::Matcher<Expr> &Inner) {
         return ignoringParens(expr(anyOf(Inner, DerivedToBase(Inner))));
auto const ElvisOperator = binaryConditionalOperator(anyOf(
       hasFalseExpression(ignoringParens(canResolveToExpr(InnerMatcher)))));
 
   auto const ComplexMatcher = ignoringParens(
       expr(anyOf(IgnoreDerivedToBase(InnerMatcher),
                  maybeEvalCommaExpr(IgnoreDerivedToBase(InnerMatcher)),
                  IgnoreDerivedToBase(ConditionalOperator),
                  IgnoreDerivedToBase(ElvisOperator))));
 
-  return ComplexMatcher.matches(Node, Finder, Builder);
+  return ComplexMatcher.matches(*Exp, Finder, Builder);
 }
 
 // Similar to 'hasAnyArgument', but does not work because 'InitListExpr' does
 // not have the 'arguments()' method.
 AST_MATCHER_P(InitListExpr, hasAnyInit, ast_matchers::internal::Matcher<Expr>,
               InnerMatcher) {
   for (const Expr *Arg : Node.inits()) {
     ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder);
const Stmt *ExprMutationAnalyzer::tryEachDeclRef(const Decl *Dec,
   for (const auto &RefNodes : Refs) {
     const auto *E = RefNodes.getNodeAs<Expr>(NodeID<Expr>::value);
     if ((this->*Finder)(E))
       return E;
   }
   return nullptr;
 }
 
-bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
-  return selectFirst<Expr>(
+bool ExprMutationAnalyzer::isUnevaluated(const Stmt *Exp, const Stmt &Stm,
+                                         ASTContext &Context) {
+  return selectFirst<Stmt>(
              NodeID<Expr>::value,
              match(
                  findAll(
-                     expr(canResolveToExpr(equalsNode(Exp)),
+                     stmt(canResolveToExpr(equalsNode(Exp)),
                           anyOf(
                               // `Exp` is part of the underlying expression of
                               // decltype/typeof if it has an ancestor of
                               // typeLoc.
                               hasAncestor(typeLoc(unless(
                                   hasAncestor(unaryExprOrTypeTraitExpr())))),
                               hasAncestor(expr(anyOf(
                                   // `UnaryExprOrTypeTraitExpr` is unevaluated
return selectFirst<Stmt>(
                                   // `GenericSelectionExpr` is unevaluated.
                                   genericSelectionExpr(hasControllingExpr(
                                       hasDescendant(equalsNode(Exp)))),
                                   cxxNoexceptExpr())))))
                          .bind(NodeID<Expr>::value)),
                  Stm, Context)) != nullptr;
 }
 
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+  return isUnevaluated(Exp, Stm, Context);
+}
+
 const Stmt *
 ExprMutationAnalyzer::findExprMutation(ArrayRef<BoundNodes> Matches) {
   return tryEachMatch<Expr>(Matches, this, &ExprMutationAnalyzer::findMutation);
 }
 
 const Stmt *
 ExprMutationAnalyzer::findDeclMutation(ArrayRef<BoundNodes> Matches) {
   return tryEachMatch<Decl>(Matches, this, &ExprMutationAnalyzer::findMutation);