[clang-tidy] bugprone-infinite-loop: React to ObjC ivars and messages in condition.

[NoQ]

Summary

If the loop condition is a value of an instance variable, a property value, or a message result value, it's a good indication that the loop is not infinite and we have a really hard time proving the opposite so suppress the warning.

Most of the newly added tests were already passing before the patch; the only actually new test is testArrayCountProperty(); it was sufficient to either add the ObjCMessageExpr case or the ObjCPropertyRefExpr case to suppress it. I added all three though, and added a bunch of other tests, because the reason they were passing previously was relatively bad and I want to make sure things keep working even when these other reasons are eliminated.

For example, all tests with ...WithConstant in their name were passing because the checker was unable to identify the condition variable. I guess this heuristic eliminates some intentionally infinite loops and guarantees that the warning message always makes sense but I can't say that I fully understand it.

Other tests were passing because the checker notices a non-integral-type variable (arr) in the condition. This restriction too can easily lead to false negatives so I can't really get behind making it permanent.

The only reason it wasn't noticing arr in the testArrayCountProperty() test was that OpaqueValueExpr was hiding it from the checker due to how its children() are empty (don't include its getSourceExpr()!). Namely, the AST generated for the property access in this test is as follows:

PseudoObjectExpr 0x7fb33d8b10c0 <col:10, col:14> 'NSUInteger':'unsigned long'
|-ObjCPropertyRefExpr 0x7fb33d8b1060 <col:10, col:14> '<pseudo-object type>' lvalue objcproperty Kind=PropertyRef Property="count" Messaging=Getter
| `-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
|   `-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
|     `-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var 0x7fb33d8b0e08 'arr' 'NSArray *'
|-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
| `-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
|   `-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var 0x7fb33d8b0e08 'arr' 'NSArray *'
`-ObjCMessageExpr 0x7fb33d8b1090 <col:14> 'NSUInteger':'unsigned long' selector=count
  `-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
    `-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
      `-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var 0x7fb33d8b0e08 'arr' 'NSArray *'

You can see that all three DeclRefExprs are shadowed by their respective OpaqueValueExprs. Both ObjCPropertyRefExpr and ObjCMessageExpr are the give-aways, as well as the entire parent PseudoObjectExpr. Hence the patch.

Another solution I could have implemented would have been to actively traverse OpaqueValueExpr->getSourceExpr() to make sure that the suppression keeps working. I wasn't comfortable implementing it because I don't fully understand OpaqueValueExpr (which isn't ObjC-specific, it shows up in normal C++ as well IIRC) so I was worried about unexpected consequences.

[NoQ]

Fix typo in tests.

[vsavchenko]

Tests look thorough, it should only reduce the number of warnings and should only affect Obj-C. So, I'd say that it's safe and sound! Great job!

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

64–66

Situations like this make me think that variadic isa is not such a bad idea 😅

[NoQ]

We do actually have variadic isa<>!

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

if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl())) {
 
       if (!Var->getType().getTypePtr()->isIntegerType())
         return true;
 
       return hasPtrOrReferenceInFunc(Func, Var) ||
              isChanged(LoopStmt, Var, Context);
       // FIXME: Track references.
     }
-  } else if (isa<MemberExpr>(Cond) || isa<CallExpr>(Cond)) {
+  } else if (isa<MemberExpr, CallExpr,
+                 ObjCIvarRefExpr, ObjCPropertyRefExpr, ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
     return true;
   }
 
   return false;
 }
 
 /// Return whether at least one variable of `Cond` changed in `LoopStmt`.
 static bool isAtLeastOneCondVarChanged(const Decl *Func, const Stmt *LoopStmt,

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

 // RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc
+
+typedef __typeof(sizeof(int)) NSUInteger;
+
+@interface NSArray
++(instancetype)alloc;
+-(instancetype)init;
+@property(readonly) NSUInteger count;
+-(void)addObject: (id)anObject;
+@end
 
 @interface I
 -(void) instanceMethod;
 +(void) classMethod;
++(instancetype)alloc;
+-(instancetype)init;
 @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)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++;
   }
 }
 @end
+
+void testArrayCount() {
+  NSArray *arr = [[NSArray alloc] init];
+  NSUInteger max_count = 10;
+  while ([arr count] < max_count) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}
+
+void testArrayCountWithConstant() {
+  NSArray *arr = [[NSArray alloc] init];
+  while ([arr count] < 10) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}
+
+void testArrayCountProperty() {
+  NSArray *arr = [[NSArray alloc] init];
+  NSUInteger max_count = 10;
+  while (arr.count < max_count) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}
+
+void testArrayCountPropertyWithConstant() {
+  NSArray *arr = [[NSArray alloc] init];
+  while (arr.count < 10) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}
+
+@interface MyArray {
+  @public NSUInteger _count;
+}
++(instancetype)alloc;
+-(instancetype)init;
+-(void)addObject: (id)anObject;
+
+-(void)populate;
+@end
+
+@implementation MyArray
+-(void)populate {
+  NSUInteger max_count = 10;
+  while (_count < max_count) {
+    // No warning. Array count is updated on every iteration.
+    [self addObject: [[I alloc] init]];
+  }
+}
+
+-(void)populateWithConstant {
+  while (_count < 10) {
+    // No warning. Array count is updated on every iteration.
+    [self addObject: [[I alloc] init]];
+  }
+}
+@end
+
+void testArrayCountIvar() {
+  MyArray *arr = [[MyArray alloc] init];
+  NSUInteger max_count = 10;
+  while (arr->_count < max_count) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}
+
+void testArrayCountIvarWithConstant() {
+  MyArray *arr = [[MyArray alloc] init];
+  while (arr->_count < 10) {
+    // No warning. Array count is updated on every iteration.
+    [arr addObject: [[I alloc] init]];
+  }
+}