[clang-tidy] Reduce false positives for `bugprone-infinite-loop` with dependent expressions

[fwolff]

Summary

Fixes PR#52081 by attempting to detect some of the "obvious" cases where a loop condition is known to be false despite depending on template parameters.

[fwolff]

The test failed on Windows. I don't have a machine to reproduce this, but maybe adding -fno-delayed-template-parsing helps.

[aaron.ballman]

Sorry about the delay in getting this reviewed, but it generally LGTM. Can you also add a release note about the fix? (I'm accepting the review and you can feel free to land with the requested changes made, but if you want another round of review before you land, just ask!)

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

120–140

My renaming may not be the best improvement to make, but Value wasn't giving me much indication of what to pass to it as a caller. It may also help to rename the function from isKnown() to isKnownToHaveValue() or something along those lines.

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

for (const Stmt *Child : Cond->children()) {
     std::string NewNames = getCondVarNames(Child);
     if (!Result.empty() && !NewNames.empty())
       Result += ", ";
     Result += NewNames;
   }
   return Result;
 }
 
-static bool isKnownFalse(const Expr &Cond, const ASTContext &Ctx) {
-  if (Cond.isValueDependent())
+static bool isKnownToHaveValue(const Expr &Cond, const ASTContext &Ctx,
+                               bool ExpectedValue) {
+  if (Cond.isValueDependent()) {
+    if (const auto *BinOp = dyn_cast<BinaryOperator>(&Cond)) {
+      // Conjunctions (disjunctions) can still be handled if at least one
+      // conjunct (disjunct) is known to be false (true).
+      if (!ExpectedValue && BinOp->getOpcode() == BO_LAnd)
+        return isKnownToHaveValue(*BinOp->getLHS(), Ctx, false) ||
+               isKnownToHaveValue(*BinOp->getRHS(), Ctx, false);
+      if (ExpectedValue && BinOp->getOpcode() == BO_LOr)
+        return isKnownToHaveValue(*BinOp->getLHS(), Ctx, true) ||
+               isKnownToHaveValue(*BinOp->getRHS(), Ctx, true);
+      if (BinOp->getOpcode() == BO_Comma)
+        return isKnownToHaveValue(*BinOp->getRHS(), Ctx, ExpectedValue);
+    } else if (const auto *UnOp = dyn_cast<UnaryOperator>(&Cond)) {
+      if (UnOp->getOpcode() == UO_LNot)
+        return isKnownToHaveValue(*UnOp->getSubExpr(), Ctx, !ExpectedValue);
+    } else if (const auto *Paren = dyn_cast<ParenExpr>(&Cond))
+      return isKnownToHaveValue(*Paren->getSubExpr(), Ctx, ExpectedValue);
+    else if (const auto *ImplCast = dyn_cast<ImplicitCastExpr>(&Cond))
+      return isKnownToHaveValue(*ImplCast->getSubExpr(), Ctx, ExpectedValue);
     return false;
+  }
   bool Result = false;
   if (Cond.EvaluateAsBooleanCondition(Result, Ctx))
-    return !Result;
+    return Result == ExpectedValue;
   return false;
 }
 
 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 (isKnownFalse(*Cond, *Result.Context))
+  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;

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`performance-inefficient-vector-operation
   the vector is a member of a structure.
 
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   <clang-tidy/checks/altera-struct-pack-align>` check for empty structs.
 
 - Fixed incorrect suggestions for :doc:`readability-container-size-empty
   <clang-tidy/checks/readability-container-size-empty>` when smart pointers are involved.
 
+- Fixed some false positives in :doc:`bugprone-infinite-loop
+  <clang-tidy/checks/bugprone-infinite-loop>` involving dependent expressions.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

 // RUN: %check_clang_tidy %s bugprone-infinite-loop %t \
-// RUN:                   -- -- -fexceptions -fblocks
+// RUN:                   -- -- -fexceptions -fblocks -fno-delayed-template-parsing
 
 void simple_infinite_loop1() {
   int i = 0;
   int j = 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]
     j++;
   }
void test_volatile_concrete_address(int i, int size) {
 
   // FIXME: This one should probably also be suppressed.
   // Whatever the developer is doing here, they can do that again anywhere else
   // which basically makes it a global.
   for (; *(int *)0x1234 < size;) {
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (size) are updated in the loop body [bugprone-infinite-loop]
   }
 }
+
+template <typename T>
+int some_template_fn() { return 1; }
+
+template <typename T>
+void test_dependent_condition() {
+  const int error = some_template_fn<T>();
+  do {
+  } while (false && error == 0);
+
+  const int val = some_template_fn<T>();
+  for (; !(val == 0 || true);) {
+  }
+
+  const int val2 = some_template_fn<T>();
+  for (; !(val2 == 0 || false);) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (val2) are updated in the loop body [bugprone-infinite-loop]
+  }
+
+  const int val3 = some_template_fn<T>();
+  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);
+}