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

[NoQ]

Summary

The utility function clang::tidy::utils::hasPtrOrReferenceInFunc() scans the function for pointer/reference aliases to a given variable. It currently scans for operator & over that variable and for declarations of references to that variable.

I'm arguing that it should scan for lambda captures by reference as well. If a lambda captures a variable by reference it basically has a reference to that variable that the user of the lambda can access at any time, including on background thread, which may have meaningful impact on checkers. The same applies to blocks (an Apple extension that brings closures to C and Objective-C; blocks are also implicitly convertible with lambdas when both facilities are available).

The patch fixes false positives in both checkers that use this functionality: bugprone-infinite-loop and bugprone-redundant-branch-condition. There are a few false negatives it introduces as a tradeoff: if the lambda captures a variable by reference but never actually mutates it we'll no longer warn but we should (as demonstrated by FIXME tests). These false negatives are architecturally annoying to deal with because hasPtrOrReferenceInFunc() detects aliasing rather than mutation, so it'll have to be a different facility entirely. They're also rudimentary because there's rarely a good reason to write such code; it probably deserves a separate warning to relax capture kind to a by-value or by-const-reference capture (which is often explicit). That said, C++'s [&] would probably capture a lot of stuff by reference unnecessarily and it'll often make their code worse if developers are forced to write down all captures manually instead.

[aaron.ballman]

Notionally, I think it makes sense to treat byref captures as a form of aliasing. Should structured bindings be treated similarly as well (not necessarily as part of this patch)?

[NoQ]

In D96215#2548492, @aaron.ballman wrote:

Should structured bindings be treated similarly as well (not necessarily as part of this patch)?

Umm, looks like we're both missing the elephant in the room: passing a variable into a function by reference.

int &hidden_reference(int &x) {
  return x;
}

void test_hidden_reference() {
  int x = 0;
  int &y = hidden_reference(x);
  for (; x < 10; ++y) { // Warns ¯\_(ツ)_/¯
  }
}

With this taken care of, I hope structured bindings will be handled automatically because whatever's unwrapped couldn't have obtained a reference to our local variable without going through a function first (eg., the constructor). Unless there's some aggregate magic going on... but in this case, again, we have to take care of aggregate magic and structured bindings aren't at fault.

[aaron.ballman]

In D96215#2550227, @NoQ wrote:

In D96215#2548492, @aaron.ballman wrote:

Should structured bindings be treated similarly as well (not necessarily as part of this patch)?

Umm, looks like we're both missing the elephant in the room: passing a variable into a function by reference.

Additionally, pointers to non-const objects. I had assumed both of these were intentionally out-of-scope because of flow sensitivity though.

int &hidden_reference(int &x) {
  return x;
}

void test_hidden_reference() {
  int x = 0;
  int &y = hidden_reference(x);
  for (; x < 10; ++y) { // Warns ¯\_(ツ)_/¯
  }
}

With this taken care of, I hope structured bindings will be handled automatically because whatever's unwrapped couldn't have obtained a reference to our local variable without going through a function first (eg., the constructor). Unless there's some aggregate magic going on... but in this case, again, we have to take care of aggregate magic and structured bindings aren't at fault.

FWIW, I was thinking of code like: https://godbolt.org/z/jK53G5 but, I don't think it needs to be handled right now in this patch.

[NoQ]

Trivial rebase.

[NoQ]

I covered pass-by-reference-into-function in D101790 and decomposition (from a certain point of view) in D101791.

[xazax.hun]

I'm arguing that it should scan for lambda captures by reference as well.

What about immediately invoked lambdas? Would it make sense to exclude those?

[xazax.hun]

Don't mind my previous comment :) I missed the conversation about function by-ref arguments. :)

[njames93]

In D96215#2550227, @NoQ wrote:

Umm, looks like we're both missing the elephant in the room: passing a variable into a function by reference.

int &hidden_reference(int &x) {
  return x;
}

void test_hidden_reference() {
  int x = 0;
  int &y = hidden_reference(x);
  for (; x < 10; ++y) { // Warns ¯\_(ツ)_/¯
  }
}

With this taken care of, I hope structured bindings will be handled automatically because whatever's unwrapped couldn't have obtained a reference to our local variable without going through a function first (eg., the constructor). Unless there's some aggregate magic going on... but in this case, again, we have to take care of aggregate magic and structured bindings aren't at fault.

Just my $0.02, We shouldn't worry too much about pathological cases like that. I imagine that code would almost never appear in the wild. And for cases where you don't have access to definition for hidden_reference , there's not much that can be done anyway.

[NoQ]

Unforget to add -fblocks to the bugprone-redundant-branch-condition test. Thanks pre-commit testing! - it wasn't failing on my machine, I guess it's enabled by default on darwin.

[NoQ]

In D96215#2735546, @njames93 wrote:

Just my $0.02, We shouldn't worry too much about pathological cases like that. I imagine that code would almost never appear in the wild. And for cases where you don't have access to definition for hidden_reference , there's not much that can be done anyway.

That's not what I typically tell myself; it really amazes me that for almost all such pathological cases there appear to be users who write such code on a regular basis.

That said, this specific false positive with references (unlike the two lambda/block false positives in this patch and the next patch) wasn't derived from a bug report *yet*. So i'm not super worried about it.

[xazax.hun]

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

45

Will this cover generalized captures? E.g.:

int i;
auto lambda = [foo=&i] { return *foo; };

[NoQ]

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

45

This one's good; we still react on the operator & inside the init-expression. This one's worse however: [&foo = i] (i had such test for the next patch but not for this patch). Let me patch this up.

[aaron.ballman]

In D96215#2737085, @NoQ wrote:

In D96215#2735546, @njames93 wrote:

Just my $0.02, We shouldn't worry too much about pathological cases like that. I imagine that code would almost never appear in the wild. And for cases where you don't have access to definition for hidden_reference , there's not much that can be done anyway.

That's not what I typically tell myself; it really amazes me that for almost all such pathological cases there appear to be users who write such code on a regular basis.

+1, Hyrum's Law is real. :-) (or :-(, more accurately).

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

45–48

Should this use capturesByRef() from https://reviews.llvm.org/D101787?

[NoQ]

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

45–48

Yes and https://reviews.llvm.org/D101787 already converts this code to use capturesByRef() as soon as it introduces it!

[aaron.ballman]

LGTM!

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

45–48

Ah! It wasn't clear to me that's how this patch was stacking up, thanks for the explanation.

[NoQ]

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

45–48

I've been using the phabricator's parent/child revisions feature (aka the Stack tab). I guess i should pronounce such relations out loud because it's not super apparent.

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

 //===------------- Aliasing.cpp - clang-tidy ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Aliasing.h"
 
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// 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;
 }
 
 /// 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 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))

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

-// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t \
+// RUN:                   -- -- -fexceptions -fblocks
 
 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 lambda_capture() {
   }
 
   do {
     int *p = &i;
     (*p)++;
   } while (i < Limit);
 }
 
+template <typename T> void accept_callback(T t) {
+  // Potentially call the callback.
+  // Possibly on a background thread or something.
+}
+
+void accept_block(void (^)(void)) {
+  // Potentially call the callback.
+  // Possibly on a background thread or something.
+}
+
+void wait(void) {
+  // Wait for the previously passed callback to be called.
+}
+
+void lambda_capture_from_outside() {
+  bool finished = false;
+  accept_callback([&]() {
+    finished = true;
+  });
+  while (!finished) {
+    wait();
+  }
+}
+
+void lambda_capture_from_outside_by_value() {
+  bool finished = false;
+  accept_callback([finished]() {
+    if (finished) {}
+  });
+  while (!finished) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+    wait();
+  }
+}
+
+void lambda_capture_from_outside_but_unchanged() {
+  bool finished = false;
+  accept_callback([&finished]() {
+    if (finished) {}
+  });
+  while (!finished) {
+    // FIXME: Should warn.
+    wait();
+  }
+}
+
+void block_capture_from_outside() {
+  __block bool finished = false;
+  accept_block(^{
+    finished = true;
+  });
+  while (!finished) {
+    wait();
+  }
+}
+
+void block_capture_from_outside_by_value() {
+  bool finished = false;
+  accept_block(^{
+    if (finished) {}
+  });
+  while (!finished) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+    wait();
+  }
+}
+
+void block_capture_from_outside_but_unchanged() {
+  __block bool finished = false;
+  accept_block(^{
+    if (finished) {}
+  });
+  while (!finished) {
+    // FIXME: Should warn.
+    wait();
+  }
+}
+
 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: %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&);
if (onFire) {
     if (isHot && onFire) {
       // NO-MESSAGE: new check is in the `else` branch
       // FIXME: handle `else` branches and negated conditions
       scream();
     }
   }
 }
 
+// Lambda / block captures.
+
+template <typename T> void accept_callback(T t) {
+  // Potentially call the callback.
+  // Possibly on a background thread or something.
+}
+
+void accept_block(void (^)(void)) {
+  // Potentially call the callback.
+  // Possibly on a background thread or something.
+}
+
+void wait(void) {
+  // Wait for the previously passed callback to be called.
+}
+
+void capture_and_mutate_by_lambda() {
+  bool x = true;
+  accept_callback([&]() { x = false; });
+  if (x) {
+    wait();
+    if (x) {
+    }
+  }
+}
+
+void lambda_capture_by_value() {
+  bool x = true;
+  accept_callback([x]() { if (x) {} });
+  if (x) {
+    wait();
+    if (x) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+    }
+  }
+}
+
+void capture_by_lambda_but_not_mutate() {
+  bool x = true;
+  accept_callback([&]() { if (x) {} });
+  if (x) {
+    wait();
+    // FIXME: Should warn.
+    if (x) {
+    }
+  }
+}
+
+void capture_and_mutate_by_block() {
+  __block bool x = true;
+  accept_block(^{ x = false; });
+  if (x) {
+    wait();
+    if (x) {
+    }
+  }
+}
+
+void block_capture_by_value() {
+  bool x = true;
+  accept_block(^{ if (x) {} });
+  if (x) {
+    wait();
+    if (x) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+    }
+  }
+}
+
+void capture_by_block_but_not_mutate() {
+  __block bool x = true;
+  accept_callback(^{ if (x) {} });
+  if (x) {
+    wait();
+    // FIXME: Should warn.
+    if (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