[clang-tidy] Aliasing: Add more support for lambda captures.

[NoQ]

Summary

D96215 takes care of the situation where the variable is captured into a nearby lambda. This patch takes care of the situation where the current function is the lambda and the variable is one of its captures from an enclosing scope.

The analogous problem for ^{blocks} is already handled automagically by the previous patch. That said, I added two FIXME tests that demonstrate an unrelated (but fairly disturbing) problem: because BlockDecl isn't a FunctionDecl, the forFunction() matcher doesn't behave as expected. This time it manifests through alias analysis analyzing more code than necessary but there are other annoying effects of this bug that I'll get to later.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/utils/Aliasing.cpp

76

const auto* could be used because type is spelled in same statement.

[NoQ]

Fxd!

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1–2

You need to add -fblocks here to enable block support to run the test.

[NoQ]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1–2

I added them in D96215 but didn't rebase this patch on top of that patch.

[NoQ]

Rebase to address the comment! (and satisfy pre-merge checks)

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/utils/Aliasing.cpp

 /// Return whether \p S is a reference to the declaration of \p Var.
 static bool isAccessForVar(const Stmt *S, const VarDecl *Var) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(S))
     return DRE->getDecl() == Var;
 
   return false;
 }
 
+static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) {
+  return llvm::any_of(RD->captures(), [Var](const LambdaCapture &C) {
+    return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef &&
+           C.getCapturedVar() == Var;
+  });
+}
+
 /// Return whether \p Var has a pointer or reference in \p S.
 static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
   // Treat block capture by reference as a form of taking a reference.
   if (Var->isEscapingByref())
     return true;
 
   if (const auto *DS = dyn_cast<DeclStmt>(S)) {
     for (const Decl *D : DS->getDeclGroup()) {
       if (const auto *LeftVar = dyn_cast<VarDecl>(D)) {
         if (LeftVar->hasInit() && LeftVar->getType()->isReferenceType()) {
           return isAccessForVar(LeftVar->getInit(), Var);
         }
       }
     }
   } else if (const auto *UnOp = dyn_cast<UnaryOperator>(S)) {
     if (UnOp->getOpcode() == UO_AddrOf)
       return isAccessForVar(UnOp->getSubExpr(), Var);
   } else if (const auto *LE = dyn_cast<LambdaExpr>(S)) {
     // Treat lambda capture by reference as a form of taking a reference.
-    return llvm::any_of(LE->captures(), [Var](const LambdaCapture &C) {
-      return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef &&
-             C.getCapturedVar() == Var;
-    });
+    return capturesByRef(LE->getLambdaClass(), Var);
   }
 
   return false;
 }
 
 /// Return whether \p Var has a pointer or reference in \p S.
 static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
   if (isPtrOrReferenceForVar(S, Var))
     return true;
 
   for (const Stmt *Child : S->children()) {
     if (!Child)
       continue;
 
     if (hasPtrOrReferenceInStmt(Child, Var))
       return true;
   }
 
   return false;
 }
 
+static bool refersToEnclosingLambdaCaptureByRef(const FunctionDecl *Func,
+                                                const VarDecl *Var) {
+  const auto *MD = dyn_cast<CXXMethodDecl>(Func);
+  if (!MD)
+    return false;
+
+  const CXXRecordDecl *RD = MD->getParent();
+  if (!RD->isLambda())
+    return false;
+
+  return capturesByRef(RD, Var);
+}
+
 bool hasPtrOrReferenceInFunc(const FunctionDecl *Func, const VarDecl *Var) {
-  return hasPtrOrReferenceInStmt(Func->getBody(), Var);
+  return hasPtrOrReferenceInStmt(Func->getBody(), Var) ||
+         refersToEnclosingLambdaCaptureByRef(Func, Var);
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

accept_block(^{
     if (finished) {}
   });
   while (!finished) {
     // FIXME: Should warn.
     wait();
   }
 }
 
+void finish_at_any_time(bool *finished);
+
+void lambda_capture_with_loop_inside_lambda_bad() {
+  bool finished = false;
+  auto lambda = [=]() {
+    while (!finished) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+      wait();
+    }
+  };
+  finish_at_any_time(&finished);
+  lambda();
+}
+
+void lambda_capture_with_loop_inside_lambda_bad_init_capture() {
+  bool finished = false;
+  auto lambda = [captured_finished=finished]() {
+    while (!captured_finished) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: this loop is infinite; none of its condition variables (captured_finished) are updated in the loop body [bugprone-infinite-loop]
+      wait();
+    }
+  };
+  finish_at_any_time(&finished);
+  lambda();
+}
+
+void lambda_capture_with_loop_inside_lambda_good() {
+  bool finished = false;
+  auto lambda = [&]() {
+    while (!finished) {
+      wait(); // No warning: the variable may be updated
+              // from outside the lambda.
+    }
+  };
+  finish_at_any_time(&finished);
+  lambda();
+}
+
+void lambda_capture_with_loop_inside_lambda_good_init_capture() {
+  bool finished = false;
+  auto lambda = [&captured_finished=finished]() {
+    while (!captured_finished) {
+      wait(); // No warning: the variable may be updated
+              // from outside the lambda.
+    }
+  };
+  finish_at_any_time(&finished);
+  lambda();
+}
+
+void block_capture_with_loop_inside_block_bad() {
+  bool finished = false;
+  auto block = ^() {
+    while (!finished) {
+      // FIXME: This should warn. It currently reacts to &finished
+      // outside the block which ideally shouldn't have any effect.
+      wait();
+    }
+  };
+  finish_at_any_time(&finished);
+  block();
+}
+
+void block_capture_with_loop_inside_block_bad_simpler() {
+  bool finished = false;
+  auto block = ^() {
+    while (!finished) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+      wait();
+    }
+  };
+  block();
+}
+
+void block_capture_with_loop_inside_block_good() {
+  __block bool finished = false;
+  auto block = ^() {
+    while (!finished) {
+      wait(); // No warning: the variable may be updated
+              // from outside the block.
+    }
+  };
+  finish_at_any_time(&finished);
+  block();
+}
+
 void evaluatable(bool CondVar) {
   for (; false && CondVar;) {
   }
   while (false && CondVar) {
   }
   do {
   } while (false && CondVar);
 }

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

 // RUN: %check_clang_tidy %s bugprone-redundant-branch-condition %t \
 // RUN:                   -- -- -fblocks
 
 extern unsigned peopleInTheBuilding;
 extern unsigned fireFighters;
 
 bool isBurning();
 bool isReallyBurning();
 bool isCollapsing();
 bool tryToExtinguish(bool&);
void capture_by_block_but_not_mutate() {
   if (x) {
     wait();
     // FIXME: Should warn.
     if (x) {
     }
   }
 }
 
+void mutate_at_any_time(bool *x);
+
+void capture_with_branches_inside_lambda_bad() {
+  bool x = true;
+  accept_callback([=]() {
+    if (x) {
+      wait();
+      if (x) {
+        // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+      }
+    }
+  });
+  mutate_at_any_time(&x);
+}
+
+void capture_with_branches_inside_lambda_good() {
+  bool x = true;
+  accept_callback([&]() {
+    if (x) {
+      wait();
+      if (x) {
+      }
+    }
+  });
+  mutate_at_any_time(&x);
+}
+
+void capture_with_branches_inside_block_bad() {
+  bool x = true;
+  accept_callback(^{
+    if (x) {
+      wait();
+      if (x) {
+         // FIXME: Should warn. It currently reacts to &x outside the block
+         // which ideally shouldn't have any effect.
+      }
+    }
+  });
+  mutate_at_any_time(&x);
+}
+
+void capture_with_branches_inside_block_bad_simpler() {
+  bool x = true;
+  accept_callback(^{
+    if (x) {
+      wait();
+      if (x) {
+        // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+      }
+    }
+  });
+}
+
+void capture_with_branches_inside_block_good() {
+  __block bool x = true;
+  accept_callback(^{
+    if (x) {
+      wait();
+      if (x) {
+      }
+    }
+  });
+  mutate_at_any_time(&x);
+}
+
 // GNU Expression Statements
 
 void negative_gnu_expression_statement() {
   bool onFire = isBurning();
   if (({ doSomething(); onFire; })) {
     tryToExtinguish(onFire);
     if (({ doSomething(); onFire; })) {
       // NO-MESSAGE: fire may have been extinguished