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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, aaron.ballman, xazax.hun, vsavchenko.
Herald added subscribers: martong, mgehre, rnkovacs.
NoQ requested review of this revision.
Herald added a project: clang-tools-extra.

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 `DeclRefExpr`s are shadowed by their respective `OpaqueValueExpr`s. 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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D102294

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.mm

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D102294.344607.patch
Type: text/x-patch
Size: 3847 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210512/5ce78ad8/attachment.bin>