[clang-tidy] Aliasing: Add support for passing the variable into a function by reference.

[NoQ]

Summary

This is a fairly basic form of aliasing that as we've noticed in D96215 hasn't been covered yet.

At the same time this patch causes unfortunate false negatives on the regression tests for bugprone-redundant-branch-condition that has a builtin incomplete but slightly more robust solution to the same problem. These false negatives are something that we didn't really have a right to warn at until we've had a more flow-sensitive alias analysis. Like, they're valid warnings but we can't emit them with the analysis that we have without consciously introducing false positives as well. So I think it's a judgement call. I'm definitely in favor of landing it despite the false negatives it introduces but this may go against clang-tidy's philosophy on this issue so I'm open to not landing it.

The patch uses AnyCall in order to cover 5 different AST node kinds for call-like expressions none of which inherit from CallExpr (apart from CallExpr itself).

[NoQ]

Unforget to git add Objective-{C,C++} tests.

Apart from testing the patch, some of these tests demonstrate that the checker doesn't really work in Objective-C. The problem is that forFunction() doesn't treat Objective-C methods as functions - a similar problem that caused false negatives demonstrated D101787. I hope to address this soon.

[NoQ]

+Adam, the original author of bugprone-redundant-branch-condition. Adam, do you have any thoughts regarding the tests regressed by this patch? Are they something we should absolutely keep warning on?

[baloghadamsoftware]

In D101790#2734937, @NoQ wrote:

+Adam, the original author of bugprone-redundant-branch-condition. Adam, do you have any thoughts regarding the tests regressed by this patch? Are they something we should absolutely keep warning on?

Actually, this checker did not give any false positives whever I tested it on several open-source projects. I can agree with the one where the function starts another thread which changes its parameter, but the rest, where the local variable is mutated after or in the branch I cannot see any reason to disappear. Can you tell me an example where these are false positives? Why do these warnings disappear?

[aaron.ballman]

I agree that all the comments labeled FIXME are ones where I'd expect to see the diagnostic triggered.

[NoQ]

I completely agree with that as well, these are 100% true positives, which is why I left them as fixmes. The reason why I think we can't have them is that we don't have an appropriate alias analysis implemented. Namely, we have two analyses, one inside the checker that leaves out false positives like the one in negative_by_val(), the other is the universal/reusable analysis in Utils that i'm patching up that doesn't support happens-before relation.

I believe the right way forward is to unify these analysis by making the analysis in Utils account for happens-before relation. Ideally this means re-implementing it over the CFG which probably isn't even that hard, it's just a simple graph traversal problem. I may look into it in June or so.

[NoQ]

Rebase on top of D102214 (that patch will probably land sooner than this patch and there are test conflicts).

[aaron.ballman]

In D101790#2748474, @NoQ wrote:

I completely agree with that as well, these are 100% true positives, which is why I left them as fixmes. The reason why I think we can't have them is that we don't have an appropriate alias analysis implemented. Namely, we have two analyses, one inside the checker that leaves out false positives like the one in negative_by_val(), the other is the universal/reusable analysis in Utils that i'm patching up that doesn't support happens-before relation.

I believe the right way forward is to unify these analysis by making the analysis in Utils account for happens-before relation. Ideally this means re-implementing it over the CFG which probably isn't even that hard, it's just a simple graph traversal problem. I may look into it in June or so.

I think that sounds like the right way forward. Thank you for looking into the unification!

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/Analysis/AnyCall.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))
if (const auto *DS = dyn_cast<DeclStmt>(S)) {
     return capturesByRef(LE->getLambdaClass(), Var);
   } else if (const auto *ILE = dyn_cast<InitListExpr>(S)) {
     return llvm::any_of(ILE->inits(), [Var](const Expr *ChildE) {
       // If the child expression is a reference to Var, this means that it's
       // used as an initializer of a reference-typed field. Otherwise
       // it would have been surrounded with an implicit lvalue-to-rvalue cast.
       return isAccessForVar(ChildE, Var);
     });
+  } else if (Optional<AnyCall> Call = AnyCall::forExpr(S)) {
+    return llvm::any_of(Call->arguments(), [Var](const Expr *ArgE) {
+      return isAccessForVar(ArgE, 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

 void test_structured_bindings_bad() {
   int x = 0;
   AggregateWithValue val { x };
   auto &[y] = val;
   for (; x < 10; ++y) {
       // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
   }
 }
+
+int &hidden_reference(int &x) {
+  return x;
+}
+
+void test_hidden_reference() {
+  int x = 0;
+  int &y = hidden_reference(x);
+  for (; x < 10; ++y) {
+    // No warning. The loop is finite because 'y' is a reference to 'x'.
+  }
+}
+
+struct HiddenReference {
+  int &y;
+  HiddenReference(int &x) : y(x) {}
+};
+
+void test_HiddenReference() {
+  int x = 0;
+  int &y = HiddenReference(x).y;
+  for (; x < 10; ++y) {
+    // No warning. The loop is finite because 'y' is a reference to 'x'.
+  }
+}

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

 // RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks
 
 @interface I
 -(void) instanceMethod;
 +(void) classMethod;
++(int &) hiddenReferenceTo: (int &)x;
 @end
 
 void plainCFunction() {
   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 testHiddenReference() {
+  int i = 0;
+  int &j = [I hiddenReferenceTo: i];
+  while (i < 10) {
+    // No warning. 'j' is a reference to 'i'.
+    j++;
+  }
+}
+
 @implementation I
 - (void)instanceMethod {
   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)classMethod {
   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++;
   }
 }
+
++(int &) hiddenReferenceTo: (int &) x {
+  return x;
+}
 @end

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

void negative_by_val(bool onFire) {
   if (tryToExtinguishByVal(onFire) && onFire) {
     if (tryToExtinguish(onFire) && onFire) {
       // NO-MESSAGE: fire may have been extinguished
       scream();
     }
   }
   if (tryToExtinguish(onFire) && onFire) {
     if (tryToExtinguishByVal(onFire) && onFire) {
-      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
+      // NO-MESSAGE: technically tryToExtinguish() may launch
+      // a background thread to extinguish the fire while tryToExtinguishByVal()
+      // may be waiting for that thread to finish.
       scream();
     }
   }
 }
 
+bool &hidden_reference(bool &flag) {
+  return flag;
+}
+
+void test_hidden_reference() {
+  bool onFire = isBurning();
+  bool onFireRef = hidden_reference(onFire);
+  if (onFire) {
+    onFireRef = false;
+    if (onFire) {
+      // NO-MESSAGE: fire was extinguished by the above assignment
+    }
+  }
+}
+
 void negative_reassigned() {
   bool onFire = isBurning();
   if (onFire) {
     onFire = isReallyBurning();
     if (onFire) {
       // NO-MESSAGE: it was a false alarm then
       scream();
     }
   }
 }
 
 //===--- Special Positives ------------------------------------------------===//
 
 // Condition variable mutated in or after the inner loop
 
 void positive_direct_mutated_after_inner() {
   bool onFire = isBurning();
   if (onFire) {
     if (onFire) {
-      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
-      // CHECK-FIXES: {{^\ *$}}
+      // FIXME: This should warn.
       scream();
     }
-    // CHECK-FIXES: {{^\ *$}}
     tryToExtinguish(onFire);
   }
 }
 
 void positive_indirect_mutated_after_inner() {
   bool onFire = isBurning();
   if (onFire) {
     if (someOtherCondition()) {
       if (onFire) {
-        // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
-        // CHECK-FIXES: {{^\ *$}}
+        // FIXME: This should warn.
         scream();
       }
-      // CHECK-FIXES: {{^\ *$}}
     }
     tryToExtinguish(onFire);
   }
 }
 
 void positive_indirect2_mutated_after_inner() {
   bool onFire = isBurning();
   if (onFire) {
     if (someOtherCondition()) {
       if (onFire) {
-        // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
-        // CHECK-FIXES: {{^\ *$}}
+        // FIXME: This should warn.
         scream();
       }
-      // CHECK-FIXES: {{^\ *$}}
       tryToExtinguish(onFire);
     }
   }
 }
 
 void positive_mutated_in_inner() {
   bool onFire = isBurning();
   if (onFire) {
     if (onFire) {
-      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
-      // CHECK-FIXES: {{^\ *$}}
+      // FIXME: This should warn.
       tryToExtinguish(onFire);
       scream();
     }
     // CHECK-FIXES: {{^\ *$}}
   }
 }
 
 void positive_or_lhs_with_side_effect() {